Opened 11 months ago

Last modified 5 months ago

#29133 assigned defect

Refactor dirserv_read_measured_bandwidths

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt, tor-crypto, tor-dirauth, tor-bwauth, 041-proposed, teor-backlog
Cc: juga Actual Points:
Parent ID: Points: 3
Reviewer: Sponsor:

Description

In https://github.com/torproject/tor/pull/497 I made some notes about refactoring dirserv_read_measured_bandwidths(). Let's consider doing this in 0.4.1.

Child Tickets

TicketStatusOwnerSummaryComponent
#29132newClean up dirserv_read_measured_bandwidths after #26223Core Tor/Tor

Change History (7)

comment:1 Changed 11 months ago by juga

+1 to refactor this. It'd be awesome if we have time for it during the hackweek.

comment:2 Changed 11 months ago by juga

Cc: juga added

CC myself

comment:3 Changed 10 months ago by juga

i'm copying them here the notes about refactoring, since the PR comments are long.

Commented in https://github.com/torproject/tor/pull/497#discussion_r249245814:

This function keeps on getting bigger. If we just add code, it will get harder and harder to change.

Let's think about splitting up what this function has to do into separate functions:

    read a line from the file
    add the line to the hash
    process the line
        check the timestamp
            add it to the headers
            stop accepting timestamps
        check for a k=v header, the terminator, or a relay line
            add headers to the headers
            at the terminator or first relay line, stop accepting k=v headers
            add relay lines to the relays

Commented in https://github.com/torproject/tor/pull/497#discussion_r249244433:

Remove line

 /* Initialise line, so that we can't possibly run off the end. */

Commented in https://github.com/torproject/tor/pull/497#discussion_r249244629:

3. The comment about fgets is outdated: let's update it to say getdelim.

4. the timestamp check is outdated, if it succeeds, getdelim always NUL-terminates its output

    We can delete the strlen() check, and move the newline check.

Commented in https://github.com/torproject/tor/pull/497#discussion_r249244743

If we delete case 3 above, we need to move the newline check here:

-   line[strlen(line)-1] = '\0';
+   if (line[strlen(line)-1] == '\n')
+     line[strlen(line)-1] = '\0';

Commented in https://github.com/torproject/tor/pull/497#discussion_r249244842:

This EOF check is redundant, getline tells you when the input is done by returning <= 0.

  while (!feof(fp)) {

comment:4 Changed 10 months ago by juga

Parent ID: #26698

Unparenting so that #26698 can be closed.

comment:5 Changed 6 months ago by teor

Owner: set to teor
Sponsor: Sponsor31-can
Status: newassigned

This is a nice litte refactor we could do for sponsor 31.

comment:6 Changed 5 months ago by nickm

Points: 3

comment:7 Changed 5 months ago by teor

Keywords: teor-backlog added
Owner: teor deleted
Sponsor: Sponsor31-can

These items are not on our roadmap, and they do not have a sponsor.
But I might do them some day.

Note: See TracTickets for help on using tickets.