[tor-bugs] #14391 [Tor]: Refactor rend_cache_lookup_entry()

[Tor Bug Tracker & Wiki]

#14391: Refactor rend_cache_lookup_entry()
-----------------------------+--------------------------------
     Reporter:  dgoulet      |      Owner:
         Type:  enhancement  |     Status:  needs_revision
     Priority:  normal       |  Milestone:  Tor: 0.2.6.x-final
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  tor-hs
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+--------------------------------

Comment (by dgoulet):

 Replying to [comment:3 nickm]:
 > Notes:
 >
 >   * If we're using the convention that a negative return value indicates
 an error, we might want to add other negative values later on.  So let's
 have the callers of this function check for rend_cache_lookup_result < 0
 as well?  Similarly, when there's a switch(), you should handle the
 `default:` case.

 We could actually simply refactor this part where we look for < 0 and do a
 different action in the case of ENOENT and EINVAL instead of jumping
 around using the returned value. I'll do this in the v2.

 Default case fixed!

 >
 >   * This affects client behavior, and makes it so that if somebody
 _does_ serve a v0 descriptor, old clients will accept it but newer ones
 won't.  Generally, we don't worry about this kind of thing too much, but
 we should consider if this is one of the times when we *do* care.

 Can a HSDir accept a v0 descriptor? It's been deprecated in 0.2.2.1 so all
 HSDir right now can't handle it no? (I checked up to 0.2.3 and I can't see
 a way for a relay to accept a v0).

 If it's indeed possible, well sure we should handle it in the cache
 lookup.

 >
 >   * The documentation for rend_cache_lookup_entry doesn't explain the
 meanings of the different return values.  It probably should.

 Fixed!

 >
 >   * Yes, warn on version < 2.

 Fixed!

 >
 >   * See the comment you removed about #997.  Does that affect your
 reasoning for item 3 above?

 Difficult to tell because #997 goes back to 4 years ago for the last
 comment on it...

 My approach here was to split the current state of cache lookup and every
 call site that needs to test the intro points should do it explicitely and
 not rely on the cache lookup to tell you that an entry is not there
 because it's not usable.

 >
 >   * I think that for robustness, we should set *e to NULL on all error
 cases in rend_cache_lookup_entry, if e is not NULL.

 Hrm... I'm not too kean about that to be honest, changing the state of a
 returned variable on error seems incorrect to me. It should be the caller
 responsability to manage it's content before and after and not rely on a
 function to clean the state for us if not found. That's the return code
 purpose.

 However, you maintain the code so if this is the coding style you want to
 go with, I'm OK fixing that. :)

 See branch {{{bug14391_026_v2}}}.

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