[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #20853 [Core Tor/Tor]: rend_config_services should use service_is_ephemeral rather than old/new->directory
#20853: rend_config_services should use service_is_ephemeral rather than
old/new->directory
------------------------------------+------------------------------------
Reporter: teor | Owner: jryans
Type: defect | Status: needs_review
Priority: Medium | Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor | Version: Tor: 0.2.9.5-alpha
Severity: Normal | Resolution:
Keywords: easy refactor intro hs | Actual Points:
Parent ID: | Points: 0.1
Reviewer: | Sponsor:
------------------------------------+------------------------------------
Changes (by jryans):
* status: needs_revision => needs_review
Comment:
Updated patch at
https://github.com/jryans/tor/commits/service_is_ephemeral.
Replying to [comment:9 teor]:
> Replying to [comment:8 jryans]:
> > Replying to [comment:6 teor]:
> > > Similarly, we can't change this part of rend_config_services:
> > > {{{
> > > if (new->directory && old->directory &&
> > > !strcmp(old->directory, new->directory)) {
> > > }}}
> > > without adding explicit checks for the directories being valid
before doing a strcmp on them.
> >
> > Okay, I added explicit `BUG` checks for the directory in addition to
the ephemeral check for both of these cases.
>
> Thanks for this change.
> The `BUG(new->directory)` and the old check are inverted, they will log
a message when the directory exists, but we want to log a message when it
doesn't.
Ah, good catch! I inverted the part inside `BUG` to log in the correct
case, but I also have to invert outside `BUG` to chain with the rest of
the conditional. Not sure if `&& !BUG(!...) &&` is okay style.
> Oh, and can we do them in the same order in
rend_service_verify_single_onion_poison?
> (That is, the ephemeral check first, then the directory check?)
> Let's not log a BUG unless we really have to.
Makes sense, updated the order.
> > > > Not sure if a changes file was needed for this, but I added one
anyway just to get familiar with the process.
> > >
> > > Thanks! Minor comment changes don't get a changes file, everything
else does.
> > > (I've submitted a number of code changes that were "Refactoring, no
behaviour change", and then ended up being "unexpected bug in ticket XXXXX
because behaviour changed during refactor".)
> > >
> > > One nitpick:
> > >
> > > the changes file format starts with
> > > `Minor bugfix (optional categories):`
> > >
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n56
> >
> > I wasn't sure if you wanted me to add an optional category or change
to "Minor bugfix". The current file (using the group "Code simplification
and refactoring") passes the `lintChanges.py` script. Let me know if
there's still a change needed.
>
> Yes, please change it to 'Minor bugfix (hidden services)'.
Okay, updated. Added `bugfix on ...` as requested by lint tool as well.
Comment 1 suspected that #20526 made it into 0.2.9.5, but `git describe
--contains` on the commit from #20526 didn't find any tags, so I listed it
as not in any released version.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/20853#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs