Opened 3 years ago

Last modified 2 years ago

#18402 needs_revision enhancement

Reduce duplicate code in parse_*_time functions

Reported by: icanhasaccount Owned by:
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Minor Keywords: tor-util time refactor code-duplication
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: yawning Sponsor:

Description

parse_rfc1123_time and parse_iso_time_ call tor_timegm which duplicates most of the range checks - attached diff attempts to correct this.

Child Tickets

Attachments (1)

time.diff (3.2 KB) - added by icanhasaccount 3 years ago.

Download all attachments as: .zip

Change History (20)

Changed 3 years ago by icanhasaccount

Attachment: time.diff added

comment:1 Changed 3 years ago by nickm

Milestone: Tor: 0.2.9.x-final
Status: newneeds_review

comment:2 Changed 3 years ago by nickm

Points: small

comment:3 Changed 3 years ago by nickm

Keywords: TorCoreTeam201604 added

Every postponed needs_review ticket should get a review in April

comment:4 Changed 3 years ago by yawning

Reviewer: yawning

comment:5 Changed 3 years ago by yawning

Status: needs_reviewneeds_revision

Looks ok at a cursor glance, though invalid input to the new and improved functions will also now log at LD_BUG in addition to LD_GENERAL on invalid input (redundant and misleading) which is probably not what we want.

comment:6 Changed 3 years ago by nickm

Keywords: TorCoreTeam-postponed-201604 added; TorCoreTeam201604 removed

These two weren't done in april. They *could* get done in May, if progress happens.

comment:7 Changed 3 years ago by isabela

Points: small1

comment:8 Changed 3 years ago by nickm

Keywords: nickm-deferred-20160905 added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

Deferring many tickets that are in needs_revision with no progress noted for a while, where I think they could safely wait till 0.3.0 or later.

Please feel free to move these back to 0.2.9 if you finish the revisions soon.

comment:9 Changed 3 years ago by icanhasaccount

Hey all - I've had another look at this and its more complicated than I first thought.

Looking at the 3 parse_*_time functions in util.c:

parse_rfc1123_time: called from parse_http_response, or/directory.c
The result of parse_rfc1123_time isn't actually used anywhere in this function - could this be removed?

parse_http_time: called from directory_handle_command_get, /or/directory.c
if_modified_since always appears to be zero, so unless logging the failure result of parse_http_time is useful it seems this could be removed as well?

parse_iso_time_: called from rend_parse_v2_service_descriptor, or/routerparse.c and entry_guards_parse_state_for_guard_selection, or/entrynodes.c , both functions use the result of parse_is_time_ so this appears fine.

So before revising the diff I thought I'd check as a first step if parse_rfc1123_time and parse_http_time could (or should) be removed?

Thanks in advance

comment:10 in reply to:  9 ; Changed 3 years ago by teor

Replying to icanhasaccount:

Hey all - I've had another look at this and its more complicated than I first thought.

Looking at the 3 parse_*_time functions in util.c:

parse_rfc1123_time: called from parse_http_response, or/directory.c
The result of parse_rfc1123_time isn't actually used anywhere in this function - could this be removed?

parse_http_response is passed a date parameter, which is set to the result of parse_rfc1123_time. connection_dir_client_reached_eof uses it, but connection_read_https_proxy_response does not.

parse_http_time: called from directory_handle_command_get, /or/directory.c
if_modified_since always appears to be zero, so unless logging the failure result of parse_http_time is useful it seems this could be removed as well?

if_modified_since is only set to zero if tor_timegm fails. Otherwise, its value is provided by tor_timegm. And it is used by directory_handle_command_get.

parse_iso_time_: called from rend_parse_v2_service_descriptor, or/routerparse.c and entry_guards_parse_state_for_guard_selection, or/entrynodes.c , both functions use the result of parse_is_time_ so this appears fine.

And parse_iso_time_ (trailing underscore) is called by parse_iso_time (no trailing underscore), which is called by about 20 functions.

So before revising the diff I thought I'd check as a first step if parse_rfc1123_time and parse_http_time could (or should) be removed?

No, they need to stay.

comment:11 in reply to:  10 Changed 3 years ago by icanhasaccount

Replying to teor:

Replying to icanhasaccount:

Hey all - I've had another look at this and its more complicated than I first thought.

Looking at the 3 parse_*_time functions in util.c:

parse_rfc1123_time: called from parse_http_response, or/directory.c
The result of parse_rfc1123_time isn't actually used anywhere in this function - could this be removed?

parse_http_response is passed a date parameter, which is set to the result of parse_rfc1123_time. connection_dir_client_reached_eof uses it, but connection_read_https_proxy_response does not.

parse_http_time: called from directory_handle_command_get, /or/directory.c
if_modified_since always appears to be zero, so unless logging the failure result of parse_http_time is useful it seems this could be removed as well?

if_modified_since is only set to zero if tor_timegm fails. Otherwise, its value is provided by tor_timegm. And it is used by directory_handle_command_get.

Ah yes - you're correct - parse_rfc1123_time and parse_iso_time_ both return the result of tor_timegm,

but parse_http_time doesn't.

parse_iso_time_: called from rend_parse_v2_service_descriptor, or/routerparse.c and entry_guards_parse_state_for_guard_selection, or/entrynodes.c , both functions use the result of parse_is_time_ so this appears fine.

And parse_iso_time_ (trailing underscore) is called by parse_iso_time (no trailing underscore), which is called by about 20 functions.

So before revising the diff I thought I'd check as a first step if parse_rfc1123_time and parse_http_time could (or should) be removed?

No, they need to stay.

Yup - thank you for taking the time to read through my ramblings and for providing a detailed explanation.

I think its probably best to drop the original diff and close the report (the original diff isn't really correct either and no longer applies) - reducing the duplicated code would be worthwhile but its a task best left to folks more familiar with the tor code.

comment:12 Changed 3 years ago by teor

Ok, thanks for having a look at this.
I think we might keep this bug, because it's something we should do eventually for consistency.

Please don't let this discourage you from having a look at other tickets, I recommend the ones tagged "easy". (That's just our best guess, some of them might not be!)

https://trac.torproject.org/projects/tor/report/30

comment:13 in reply to:  12 Changed 3 years ago by icanhasaccount

Replying to teor:

Ok, thanks for having a look at this.
I think we might keep this bug, because it's something we should do eventually for consistency.

Please don't let this discourage you from having a look at other tickets, I recommend the ones tagged "easy". (That's just our best guess, some of them might not be!)

https://trac.torproject.org/projects/tor/report/30

Ah yep - in that case its fine to leave it open. Thank you for the link that's very helpful and kind of you - I'll have a look through and see if there are any I can take a stab at.

comment:14 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:15 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:16 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:17 Changed 2 years ago by nickm

Keywords: nickm-deferred-20160905 removed

comment:18 Changed 2 years ago by nickm

Keywords: TorCoreTeam-postponed-201604 removed

comment:19 Changed 2 years ago by nickm

Keywords: tor-util time refactor code-duplication added
Note: See TracTickets for help on using tickets.