Opened 4 months ago

Last modified 3 months ago

#29133 new 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
Cc: juga Actual Points:
Parent ID: Points:
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

Change History (4)

comment:1 Changed 4 months ago by juga

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

comment:2 Changed 4 months ago by juga

Cc: juga added

CC myself

comment:3 Changed 3 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 3 months ago by juga

Parent ID: #26698

Unparenting so that #26698 can be closed.

Note: See TracTickets for help on using tickets.