Opened 8 years ago

Closed 6 years ago

#4282 closed defect (fixed)

Extract common "make private stats dir" code from rephist.c, geoip.c

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: easy refactoring tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In several places in rephist.c and geoip.c, we do:

  /* Try to write to disk. */
  statsdir = get_datadir_fname("stats");
  if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) {
    log_warn(LD_HIST, "Unable to create stats/ directory!");
    goto done;
  }
  filename = get_datadir_fname2("stats", "buffer-stats");
  if (write_str_to_file(filename, str, 0) < 0)
    log_warn(LD_HIST, "Unable to write buffer stats to disk!");

Duplicated code is stupid. Some or all of this should get extracted into a new function or two. I'd suggest a function to "create the datadir and a subdirectory of the datadir as needed" and another function to write a file to a subdirectory of the datadir, creating that subdirectory as needed.

Child Tickets

Attachments (2)

ticket4282.patch (10.9 KB) - added by piet 6 years ago.
ticket4282.patch.v2 (25.7 KB) - added by piet 6 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 8 years ago by nickm

Keywords: easy refactoring added
Milestone: Tor: unspecified

comment:2 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:3 Changed 7 years ago by nickm

Component: Tor RelayTor

Changed 6 years ago by piet

Attachment: ticket4282.patch added

comment:4 Changed 6 years ago by piet

Status: newneeds_review

I added two new methods check_or_create_data_subdir() and write_to_data_subdir() to config.c and used them throughout rephist.c and geoip.c. Not sure config.c is the right place, though?

comment:5 Changed 6 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.5.x-final

Looks good to me for inclusion in 0.2.5.x once that forks. We'll want to fix it up so it matches our indentation style for where braces go at the start of functions, and where tabs vs spaces go. "make check-spaces" should catch some of these.

It would be great to have unit tests for this.

Changed 6 years ago by piet

Attachment: ticket4282.patch.v2 added

comment:6 Changed 6 years ago by piet

I fixed the indentations in my patch to match the conventions. (Btw, is there a documentation of the code conventions somewhere?)

I also tried to come up with some meaningful unit tests for both of the new functions.

comment:7 in reply to:  6 Changed 6 years ago by nickm

Replying to piet:

I fixed the indentations in my patch to match the conventions.

Thanks!

(Btw, is there a documentation of the code conventions somewhere?)

What there is, is in doc/HACKING in the Tor source code. I'm not sure how complete it is though.

I also tried to come up with some meaningful unit tests for both of the new functions.

Thanks!

I hope to review this again and merge it once 0.2.5 has its own branch -- hopefully some time this month.

comment:8 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Thanks again! I've cleaned it up a little and merged.

The changes were mostly about whitespace style, but 57e4324c425e6c9acd58c2270a183ee4d9b1e4aa was a critical logic error, and 58721ac24c684236c52c062a255c38f2f019fda5 included a fix that I needed to make the unit tests work.

Note: See TracTickets for help on using tickets.