[tor-bugs] #14846 [Tor]: Controller: retrieve an HS descriptor of a service run by a user

[Tor Bug Tracker & Wiki]

#14846: Controller: retrieve an HS descriptor of a service run by a user
-------------------------+-------------------------------------------------
     Reporter:  dgoulet  |      Owner:  donncha
         Type:           |     Status:  assigned
  enhancement            |  Milestone:  Tor: 0.2.7.x-final
     Priority:  trivial  |    Version:  Tor: 0.2.7
    Component:  Tor      |   Keywords:  SponsorR, tor-hs, controller,
   Resolution:           |  027-triaged-1-in, SponsorS
Actual Points:           |  Parent ID:  #3521
       Points:  small    |
-------------------------+-------------------------------------------------

Comment (by asn):

 Here is an initial review:

 - This could be made easier to review. A few later commits (like
 `997d596`) change code that was introduced by previous commits. I'd
 suggest rebasing the branch so that related changes happen in one commit
 if it can be done.

   Also, commit `a454cd93b` is a bit tricky to review. That's because it
 moves lots of code around, and also edits code in the same commit. Let me
 suggest, if it's possible, you make one commit that moves or re-indents
 code first, and then a second commit that alters code. Then it's easier to
 review the changes.

 - I agree that `rend_cache_lookup_v2_desc_as_service()` looks very similar
 to `rend_cache_lookup_entry()` (although you said
 `rend_cache_lookup_v2_desc_as_dir()` in your comment). Maybe you can merge
 your thing in `rend_cache_lookup_entry()` and also add an extra argument
 to distinguish between the client and the service-side cache? Would that
 make it look nice you think?

 - You do
 {{{
 for (i = 0; i < smartlist_len(descs); i++)
         rend_encoded_v2_service_descriptor_free(smartlist_get(descs, i));
 }}}
   we usually use `SMARTLIST_FOREACH` in such cases. Check it out.

 - You say:
 {{{
     } else {
       log_info(LD_REND, "Successfully stored created v2 rend
 descriptors!");
     }
 }}}
   is this true even after `997d596`?

 - Please document the definition of `rend_cache_service`.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/14846#comment:12>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online