Opened 21 months ago

Last modified 8 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, teor-backlog
Cc: juga Actual Points:
Parent ID: Points: 3
Reviewer: Sponsor:


In I made some notes about refactoring dirserv_read_measured_bandwidths(). Let's consider doing this in 0.4.1.

Child Tickets

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

Change History (8)

comment:1 Changed 21 months ago by juga

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

comment:2 Changed 21 months ago by juga

Cc: juga added

CC myself

comment:3 Changed 21 months ago by juga

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

Commented in

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

Remove line

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

Commented in

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

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

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

  while (!feof(fp)) {

comment:4 Changed 21 months ago by juga

Parent ID: #26698

Unparenting so that #26698 can be closed.

comment:5 Changed 16 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 16 months ago by nickm

Points: 3

comment:7 Changed 16 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.

comment:8 Changed 8 months ago by teor

Status: assignednew

Change tickets that are assigned to nobody to "new".

Note: See TracTickets for help on using tickets.