Reduce duplicate code in parse_*_time functions
parse_rfc1123_time and parse_iso_time_ call tor_timegm which duplicates most of the range checks - attached diff attempts to correct this.
Trac:
Username: icanhasaccount
Activity
changed milestone to %Tor: unspecified
Trac:
Username: icanhasaccount
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.
Trac:
Milestone: Tor: 0.2.9.x-final to Tor: 0.2.???
Keywords: N/A deleted, nickm-deferred-20160905 added-
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
Trac:
Username: icanhasaccount 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.
-
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.
Trac:
Username: icanhasaccount 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!)
-
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!)
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.
Trac:
Username: icanhasaccount moved to tpo/core/tor#18402 (closed)
