[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #26227 [Core Tor/Stem]: Review existing stem.client code
#26227: Review existing stem.client code
---------------------------+------------------------------
Reporter: dmr | Owner: dmr
Type: task | Status: needs_review
Priority: Medium | Milestone:
Component: Core Tor/Stem | Version:
Severity: Normal | Resolution:
Keywords: client | Actual Points:
Parent ID: | Points:
Reviewer: atagar | Sponsor:
---------------------------+------------------------------
Changes (by dmr):
* status: reopened => needs_review
Comment:
Replying to [comment:10 atagar]:
> Hi Dave, sorry about the delay! **Finally** done chewing through these
and merged.
>
> Work looks great! Many thanks for the pull request.
Hey Damian! Thanks for the review! Glad you got through it - sorry again
that it was such a big chunk to chew on.
I'm glancing through the commits you've added in between - they look
pretty self-explanatory, but I'll let you know if I have any questions,
and will definitely let you know if I have any comments.
> Opted to pass on three of the smaller commits...
>
> * **Commit 8353d83 (Remove redundant content-length checks):** Think I'd
prefer to pass on this one. Those conditionals are there so we provide
callers with nicer error messages. True, they might not be necessary but
not spotting a need to drop 'em.
>
> * **Commit 819c1a2 (Refactor fixed_cell_len determination into a
@staticmethod):** No longer necessary due to the LinkProtocol class
addition.
>
> * **Commit 79396f5 (Add TODO notes for Cell types without a unit test
definition):** Eh. Happy to add TODOs but these didn't add much.
>
> Everything else has been merged.
Cool! At a glance, it seems like these all make sense to leave out.
For
[[https://github.com/torproject/stem/pull/1/commits/8353d8399c27901df4412a1e1c8bb57ee3269229|8353d83]]
specifically:
I guess I didn't think about it from that perspective. I just noticed that
several other places relied on the Size `pop()` for the exception
behavior; these few seemed inconsistent.
In the case of the `NetinfoCell`, the code is practically unreachable, so
I'm not sure the check/message really adds much.
Either way, I think a good follow-up commit would be to annotate the if
check with a comment saying the purpose is for a nicer exception message.
(That way it won't look like the code //otherwise wouldn't// check size.)
> Feel free to reopen if there's anything I missed here.
Hey, in particular I've reopened this for the review comments in this
ticket!
Namely:
* [[comment:4|comment 4]]
* [[comment:5|comment 5]] (separate comment, 'cause [[comment:4]] was too
long)
* [[comment:9|comment 9]] (pending discussions: circ_id allocation;
padding, esp. comments in #26228 as called out above)
Setting to `needs_review` since that seems to be the most relevant thing
here.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26227#comment:13>
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