Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#14391 closed enhancement (implemented)

Refactor rend_cache_lookup_entry()

Reported by: dgoulet Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: tor-hs, 027-triaged-1-in
Cc: Actual Points:
Parent ID: Points: medium
Reviewer: Sponsor: SponsorR

Description

Here is why rend_cache_lookup_entry() should be refactored:

1) v0 descriptors are deprecated since 0.2.2.1 and not suppose to be alive in the network anymore. This function should only serve v2 version for now as the default.

2) It should return different error code depending on what's the actual error is. Right now, there is no distinction between a cache entry not found and an invalid query.

3) This function should NOT test if the intro points are usable or not. This adds some load on a function that should be "O(1)" and do one job. Furthermore, multiple callsites actually already test that doing twice the job...

4) While adding control event, it would be useful to be able to lookup a cache entry without having it checking the intro points. There are also places in the code that do want to lookup the cache entry without doing that.

Child Tickets

Change History (13)

comment:1 Changed 5 years ago by dgoulet

Status: newneeds_review

See branch bug14391_026_v1 for a stab at it.

There are two "XXX" still in the code that should be answered for which I'm uncertain.

comment:2 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-final

comment:3 Changed 5 years ago by 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.
  • 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.
  • The documentation for rend_cache_lookup_entry doesn't explain the meanings of the different return values. It probably should.
  • Yes, warn on version < 2.
  • See the comment you removed about #997. Does that affect your reasoning for item 3 above?
  • 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.

comment:4 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

comment:5 in reply to:  3 Changed 5 years ago by dgoulet

Replying to 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.

comment:6 Changed 5 years ago by dgoulet

Status: needs_revisionneeds_review

comment:7 Changed 5 years ago by andrea

This one looks fine to me.

comment:8 Changed 5 years ago by nickm

Can you add a changes file, please?

comment:9 Changed 5 years ago by dgoulet

Keywords: SponsorR added
Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final

So the code seems good (and andrea did review it also) but I feel we should postpone this one because I think that the logic of client cache lookup and "if usable intro points" could be improved here and further investiguated.

We don't need to rush this in 0.2.6 at all I think even though it does stuff twice sometimes, it's not the critical path and only client side. In the next week, I'll be able to make sure we do things properly with more testing.

comment:10 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:11 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

merged! (and added a changes file)

comment:12 Changed 4 years ago by isabela

Points: medium
Version: Tor: 0.2.7

comment:13 Changed 4 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR
Note: See TracTickets for help on using tickets.