Opened 5 years ago

Closed 7 weeks ago

Last modified 5 weeks ago

#4998 closed defect (implemented)

MyFamily as a list

Reported by: weasel Owned by: Jigsaw52
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.11-alpha
Severity: Normal Keywords: tor-relay lorax easy intro
Cc: Actual Points:
Parent ID: #15060 Points:
Reviewer: Sponsor:

Description

Hi,

MyFamily currently is a config string. It'd be nice if it was a line list like RecommendedVersions as that would make configuration files a bit more readable.

Child Tickets

Attachments (1)

config.patch (556 bytes) - added by pingl 10 months ago.
config.c patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 years ago by nickm

  • Milestone set to Tor: unspecified

Sounds like a plausible idea to me. It would need to be a listlist of comma-separated entries, for backward compatibility.

comment:2 Changed 5 years ago by nickm

  • Keywords tor-relay added

comment:3 Changed 5 years ago by nickm

  • Component changed from Tor Relay to Tor

comment:4 Changed 2 years ago by nickm

  • Parent ID set to #15060

comment:5 Changed 21 months ago by nickm

  • Keywords lorax added

comment:6 Changed 16 months ago by teor

  • Keywords easy intro added
  • Severity set to Normal

Changed 10 months ago by pingl

config.c patch

comment:7 Changed 9 months ago by cypherpunks

  • Status changed from new to needs_review

comment:8 Changed 9 months ago by teor

  • Status changed from needs_review to needs_revision

The code that processes MyFamily still expects it to be a char * - this patch changes it to a smartlist * containing char *, but doesn't change how it's processed.

comment:9 Changed 2 months ago by Jigsaw52

  • Status changed from needs_revision to needs_review

comment:10 Changed 2 months ago by nickm

  • Milestone changed from Tor: unspecified to Tor: 0.3.1.x-final

comment:11 Changed 2 months ago by nickm

  • Status changed from needs_review to needs_revision

Getting there!

But it looks like this patch makes it so Tor would no longer accept the old format. That's not right, though: we need to make sure that the there can be multiple lines *and* multiple options on each line. Otherwise all old MyFamily lines will stop working.

comment:12 Changed 2 months ago by Jigsaw52

  • Status changed from needs_revision to needs_review

I does accept both multiple lines and multiple options on the same line. I have changed the test to actually test both cases and tested manually with the following lines on torrc:

MyFamily $1111111111111111111111111111111111111111, 1111111111111111111111111111111111111112, $1111111111111111111111111111111111111113
MyFamily 1111111111111111111111111111111111111114
MyFamily $1111111111111111111111111111111111111115

However, I did change the man page to mention only the new format. I did that to encourage the use of the new format. I can change the man page to mention both formats if you think it is better.

comment:13 Changed 2 months ago by Jigsaw52

  • Owner set to Jigsaw52
  • Status changed from needs_review to assigned

comment:14 Changed 2 months ago by Jigsaw52

  • Status changed from assigned to needs_review

comment:15 Changed 7 weeks ago by nickm

  • Status changed from needs_review to merge_ready

Oh, I see what you did. Yes, you're correct there. The code looks like it should work.

One thing that we try not to do any more, though: we try not to rewrite values that could get written back into the torrc by a SAVECONF command. What we usually do instead now is, when we want to normalize a configuration value, normalize it into a separate field of or_options_t. What do you think of the patch I've made on top of your branch, in my-family-list-fix-4498 in my public repository? The URL is

https://gitweb.torproject.org/nickm/tor.git/log/?h=my-family-list-fix-4498

comment:16 Changed 7 weeks ago by Jigsaw52

Thanks for the explanation.
I checked out out your branch and tested it and everything seems fine.

comment:17 Changed 7 weeks ago by nickm

  • Resolution set to implemented
  • Status changed from merge_ready to closed

Excellent; merged to master. Thanks for the patches!

comment:18 Changed 5 weeks ago by arma

Alas, this patch seems to have caused #22368 (a double free bug).

Note: See TracTickets for help on using tickets.