Options can't be set on `HELLO VERSION ...`

[cryptix]

@eyedeekay Sorry for not paying more attention but I just saw that the options are mostly without effect.

before a05f2d6 they were sent after the newline of the HELLO VERSION ... message and shouldn't result in a working connection anyhow, which made me wonder...

This means all the options_test.go are mostly just there to guarantee compilation, not effect of the options.

[eyedeekay]

Sorry about that, I think I must have done that when I was learning how to set the options via the API and just left it in because I forgot it was there and didn't seem to stop things from working. But I'm confused as to why the options would be without effect, isn't that set here? Never mind, found the issue, pull incoming.

goSam/sessions.go

Line 22 in 702cc4d

r, err := c.sendCmd("SESSION CREATE STYLE=STREAM ID=%d DESTINATION=%s %s\n", id, dest, c.allOptions())

I hadn't considered that the tests didn't actually test for the settings taking effect, and in fact only test compilation with valid arguments. I can't get them from i2pcontrol without actually starting the tunnels, which I don't think we want to do with tests, right? So I guess... I could make a function that takes the same arguments as sendCmd and validates them, add it to the end of the tests?

[cryptix]

I can't get them from i2pcontrol without actually starting the tunnels, which I don't think we want to do with tests, right?

Do you see the tunnel before it is established? If so, I’d vouche for this. It would also give us safety that the options are not misspelled and implemented as assumed.

Testing full circuits (accepted connections) might give a lot of false negatives because of timeouts... so we can maybe only test the listener side

[cryptix]

I could make a function that takes the same arguments as sendCmd and validates them, add it to the end of the tests?

I saw you started to do this in #11 - i think it’s generally a good idea to have.

[eyedeekay]

Do you see the tunnel before it is established?

I've improved the tests in #11 to create sessions but leave them unconnected, then close them. That way, you can see that the options are set and the session is created but we don't have to connect to anything to complete the test.

[eyedeekay]

Is there anything else I can do to improve this part of the code that I've missed? I've been comparing the session-creation parts with the equivalent from kpetku/sam3 and they appear to be doing the same thing now. I suppose I could add spell-checking into the validCmd function, I need to create a dictionary of i2cp options for something else anyway, but with session creation being part of the tests I think it might be excessive. Any thoughts?

[eyedeekay]

Hey just wanted to touch base and confirm with you that I've observed the options are indeed being used, at least according to the /var/log/i2pd/i2pd.log. I don't know of any other way to confirm them besides the ones available now.

[cryptix]

Sorry I let you hanging on this for so long. If there is no way to query the connections through the SAM interface, I think we have to trust in our code.

Spell checking etc is definitely not worth it. If you want to build tests that grep/search the log files.. hmm.. okay but I'd rather not this depends on implementation specifics (i2pd cpp vs i2p java).

[eyedeekay]

Well now that the i2cp options are being passed as a string and not a representation of a slice, I'm pretty much comfortable trusting it. Apparently the options were unset partly because the bracket around the slice in the old version confused SAM. I like to be thorough, but my qualm about the logs is that I don't know to do it unless the logs are in a predictable place readable by the user running the test, which means if the hypothetical person running the tests used i2p as a service(thereby giving a predictable place for logs), go test would need to be run something like... sudo -u i2psvc go test on i2p and sudo -u i2pd go test on i2pd. If the tester ran i2p as a regular user, we'd need to define a particular log location to know what to parse. Seems like it would probably be pretty brittle in and of itself.

If it would help with our communication, I saw you had an account at notabug, too, if you'd rather develop there I had considered transitioning off github(notabug is also my first choice for a non-github repository hosting service, if I leave github, it will probably be for there). Unfortunately for me I made github too big a part of my workflow to be able to move all at once. I've got apt repositories hacked to run on github pages and crap like that that I'll need to replace but none of that affects gosam. If you'd rather work there I can kep in contact that way.

[cryptix]

... Seems like it would probably be pretty brittle in and of itself.

yeah, that is what I meant as well. I'd rather just document clearly where to find the option definitions so people can easily make patches when things change.

[eyedeekay]

Agreed. When I get home I will write a guide to making technical contributions and see what you think. At least for i2cp and tunnel options, it's all very uniform. I'm going to revise the comments on the options a bit too, to make the use a little clearer. I'll just add these to the existing pull request, if you don't mind.

[eyedeekay]

Do we want to just make my fork official? I've got fixes in for all of this, SAM3.1, resolved a bug with excessive SESSION CREATE requests, and made it a deb. I'm using this a ton and have an interest in maintaining and spreading it, I'd be happy to adopt it.