Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#1929 closed defect (implemented)

It's hard to specify a really long *Nodes option

Reported by: Sebastian Owned by:
Priority: Low Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Scott Bennett was quite confused by this (see this thread for details, ).

In theory we could change the ROUTERSET config type to be one of the types that you can only append to, not replace by setting another entry later on.

I'm not sure we should change this for 0.2.2.x, first because we are in feature freeze and second because this changes behaviour that someone might rely upon (setting the option via controller for example, expecting this to overwrite, not append to the previously specified entry). For this reason I'm not sure that's a good idea at all; now with Tor warning about this kind of configuration setup we should at least confuse fewer users about thinking that they can use more than one line to specify *Nodes.

Child Tickets

Change History (12)

comment:1 Changed 10 years ago by Sebastian

Component: - Select a componentTor Client

comment:2 Changed 10 years ago by nickm

So it seems like the basic problem here is "dealing with super-long lines is a pain in the butt". If that's so, then branch "continuation" in my public might solve the issue at minimal destabilizing. Thoughts?

comment:3 Changed 10 years ago by Sebastian

Looks like a fine start, but I bet this won't please Scott because you still can't add comments.See branch continuation in my repository for an addition of that feature.

comment:4 Changed 10 years ago by arma

Milestone: Tor: 0.2.2.x-final
Priority: normalminor
Status: newneeds_review

We should do something here for the poor screwed people who have been misusing the option all this time.

Also moving to needs_review since apparently there's a patch to review.

comment:5 Changed 10 years ago by nickm

I'm not thrilled with the comment syntax; nothing else does comments that way. If I understand right, the syntax is something like this?

ExcludeNodes \
# Node1337 is run by the Bavarian Illuminati \
  Node1337, \
# The operator of Node99 looked at me funny \

It changes the meaning of # and \, since you can no longer interpret # as "Ignore the rest of the line" and you can no longer interpret \ as "combine this line and the next, then process". Instead you need to treat # as meaning "Ignore the rest of the line except for a \ if the \ is at the end." It also changes the semantics of previously valid torrcs, such as

ContactInfo UberUser <> # /// Don't use my real email here! \\\
Log info file /home/nick.mathewson/projects/tor-info.log

Contrived, I admit.

I'm okay saying either "No comments for you" here, or with adding a comment/continuation syntax that doesn't change the meaning of old torrcs.

comment:6 Changed 10 years ago by Sebastian

The comment syntax is supposed to be like this:

ExcludeNodes \
# Node1337 is run by the Bavarian Illuminati
  Node1337, \
# The operator of Node99 looked at me funny

If you look at the unit test that I added, that should totally work. I also added a unit test for your example case to my branch, and that works, too. Did the commit that added a comment to the manpage have confusing words, or was the code confusing, or am I still missing something? Thanks!

comment:7 Changed 10 years ago by nickm

hm. that looks better than what I thought it did. I wonder if we can get the manpage to explain this with an example. Somebody could still screw this up with something like

ExcludeNodes \
# a looked at me funny \
   a, \
# b is run by the mafia \
   b, \
Contact ,commamaster

But that isnt' too plausible.

comment:8 Changed 10 years ago by Sebastian

I would rather have the ability to add comments than worry about that case, because it seems that the main reason why we do this anyways is that people want comments for specific entries. It would be great to get feedback from someone who actually cared for the feature.

I'll add an example to the manpage in a couple of days.

comment:9 Changed 10 years ago by nickm

I tweaked it in branch continuation in my repository.

I think, among other things, I removed a condition where we could run off the end of the string. When you did:

 } else if (*line == '#') {
   do {
   } while (*line && *line != '\n')
 ++line; // OOPS

consider what would happen when the last line in the file had a comment with no newline at the end. The loop would continue until *line=='\0', and then the second increment (marked "OOPS" above) would advance the line one more character. Ouch!

And a completely cosmetic thing: It seems odd to day "Implements bug 1929". Implementing a bug sounds like you're putting it _into_ the code. Since this is really a feature rather than a bugfix, I'm going with "resolves bug 1929", but "resolves ticket 1929" would also sound ok to me.

comment:10 Changed 10 years ago by Sebastian

Looks good for the code fixes, thanks.

Your documentation might want to do what I added in branch continuation in my repo, though, since we do allow not putting a nl at the end of the torrc.

comment:11 Changed 10 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Summary: Setting *Nodes config options more than once means last entry is usedIt's hard to specify a really long *Nodes option

Retitled. Also, merged and closing.

comment:12 Changed 8 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.