21/10/12 10:41, intrigeri wrote:
> hi,
>
> anonym wrote (18 Oct 2012 11:18:43 GMT) :
>> Implemented in bugfix/tordate_vs_tor_0.2.3.x
>
> Looks good.
>
> I'm worried about the reliance on exact log messages wording,
> but well...
Absolutely.
> Here follows some nitpicking on implementation details.
>
> I'm in doubt with the "grep -m 1" on Tor log file: don't we want the
> *last* occurrence of e.g. "certificate lifetime", rather than the
> first one, in case there would be several? This may very well be
> a rare edge case, or even impossible, but still, if we indeed want the
> last occurrence, I'd rather see this expressed explicitly in code.
At first I thought no, but there is indeed a rare edge case:
1. Start Tails with an incorrect clock.
2. For some reason tordate quits without finishing (so no
$TORDATE_DONE_FILE is written).
3. Leave the Tails session for at least a month.
4. Try to connect again, so tordate is run once more. The time will be
set to some time ~1 month back (when we ran step 1) so Tor's TLS
connection will fail.
Beyond that, what you say makes more sense any way. Fixed.
> Same for:
>> grep -q "\[warn\] Certificate \(not yet valid\|already expired\)." \
>> /var/log/tor/log
I'm usure what you mean here. Care to clarify?
I can see a small issue here since we only look for the existence of
such a line, so it may be an old line from a previous (failed) Tor
bootstrap attempt that's not relevant any more. That could make
is_clock_way_off() return true even if our clock is perfect, and hence
have it set to some old cert's valid-after date. AFAICT it will only
cause a problem in scenarios like the one I described in steps 1-4 above
(i.e. they rely on the same Tails session running for > a month).
I think we can just leave this one. Otherwise I suppose we could look
into clearing the Tor log each time it starts.
> About "grep -m 1 SIMPLIFIED_PATTERN | sed ...": I think this can be
> done in a more robust way with sed only, which would also be a way to
> get the last occurrence if that's what we want.
I can go as far as `sed -n s/.../.../p` to grop the grep, but I won't go
further (please do yourself if you feel it's important). More advanced
sed than that *usually* ends up making the code a lot harder to read.
OTOH, if there is a super simple *and* readable way to also only write
the last match in the above sed stanza, I'd be happy to learn about it.
> Please use long options (e.g. --max-count, --only-matching, etc.):
> more readable, easier to review and maintain.
>
> About "grep | tail -n1": I think "tac | grep --max-count 1",
> or sed alone, may be a bit cheaper. Not that we care much.
Again I think I'll go with what's easier to read, i.e. the `tail` approach.
>> Grep Tor's log instead of GETINFO the control port.
>> Through extensive testing it seems that opening a controller and/or
>> issuing "GETINFO status/bootstrap-phase" during early bootstrap gets
>> Tor stuck at "Bootstrapped 5%".
>
> Argh :( Upstream bug report?
Once I have something reproducible I will report it.
> Weird fact: Vidalia seems to be able to do the same without triggering
> this bug.
Vidalia does it rather differently; it gets the bootstrap progress from
STATUS_CLIENT events, not by GETINFO-bombing with a new control port
connection (AUTHENTICATE...QUIT) for each query. :)
> Nitpicking (about the commented-out tor_control_getinfo()): according
> to the Tor control protocol spec, even if Tor MAY accept LF alone,
> "Controllers SHOULD always send CRLF".
Fixed :).
>> (merged it into experimental).
>
> It seems you forgot to push.
Pushed now.
Cheers!
