#25515 closed enhancement (fixed)

Add a unit test for geoip_load_file()

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-test, geoip
Cc: juga Actual Points:
Parent ID: Points:
Reviewer: isis Sponsor:

Description

The geoip_load_file() function has no test coverage. As a demo at the hackfest, I worked with Juga to fix that situation. Please review branch geoip_testing in my public repository.

Child Tickets

Attachments (2)

Change History (36)

comment:1 Changed 16 months ago by nickm

Status: assignedneeds_review

comment:2 Changed 16 months ago by nickm

Cc: juga added

Next steps, if we decide:

  • Test loading the geoip6 file.
  • Test that loading a second file truly replaces the first instead of adding to it.

comment:3 Changed 16 months ago by isis

Keywords: review-group-35 added
Reviewer: isis

comment:4 Changed 16 months ago by isis

LGTM

comment:5 Changed 15 months ago by isis

Status: needs_reviewmerge_ready

comment:6 Changed 15 months ago by nickm

Keywords: review-group-35 removed
Milestone: Tor: 0.3.4.x-finalTor: unspecified
Reviewer: isis
Status: merge_readyaccepted

merged to master; thanks!

Putting this back into new in case juga or someone else wants to do the other tests discussed above.

comment:7 Changed 15 months ago by nickm

Status: acceptednew

comment:8 Changed 15 months ago by isis

Is it possible that the SRCDIR "/src/config/geoip" file ends up somewhere different on windows? There's an experimental build (read: it could be lying) for part of the AppVeyor setup for #25549 which seems to claim it can't find that file.

comment:9 Changed 15 months ago by nickm

There was a bug here that I fixed in 398bef2592010e71692dd9c3b5b90d3751c48bb2 -- is the appveyor setup on a branch that includes that fix, or did it branch earlier?

comment:10 Changed 15 months ago by juga

Attaching test loading the geoip6 file as nickm commented above.

comment:11 Changed 15 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.4.x-final
Status: newneeds_review

comment:12 Changed 15 months ago by asn

Reviewer: isis

comment:13 Changed 15 months ago by saper

git log 398bef2592010e71692dd9c3b5b90d3751c48bb2 ^master

is empty, so it is included.

Last edited 15 months ago by saper (previous) (diff)

comment:14 Changed 15 months ago by juga

saper, what do you mean by the previous comment?

comment:15 Changed 15 months ago by saper

#define SRCDIR "/c/projects/appveyor"

Maybe my MinGW-based setup does not like cygwin paths.

Using "c:/projects/appveyor" seems to work. There is a piece of autoconf code that tries to do the same (determine the actual source directory), maybe it should be reused?

comment:16 Changed 15 months ago by saper

Similarly, I believe $BUILDDIR does not work either. It seems not to be used to open existing file if _WIN32 is defined.

comment:17 in reply to:  14 Changed 15 months ago by saper

Replying to juga:

saper, what do you mean by the previous comment?

It was a reply to a question if 398bef2592010e71692dd9c3b5b90d3751c48bb2 is included in my appveyor testing branch. The answer is yes, it is included.

comment:18 Changed 15 months ago by saper

I have taken another approach - instead of hacking our own values, I have resorted to $srcdir and $builddir already given to us by config.status.

The following branch survives gmake check on both AppVeyor and FreeBSD:

http://repo.or.cz/tor/appveyor.git/shortlog/refs/heads/build_src_directories

(it has its disadvantages, like inability to run tests from the arbitrary directory).

comment:19 Changed 15 months ago by saper

Formal question: should I submit another ticket with the patch?

comment:20 Changed 15 months ago by nickm

Formal question: should I submit another ticket with the patch?

I think that would be a good idea: fixing the win32 test behavior seems like a separate issue from increasing this test coverage.

comment:21 in reply to:  20 Changed 15 months ago by isis

Resolution: implemented
Status: needs_reviewclosed

Replying to nickm:

Formal question: should I submit another ticket with the patch?

I think that would be a good idea: fixing the win32 test behavior seems like a separate issue from increasing this test coverage.


Where did this ticket end up? I think I've been assigned as reviewer.

Also, I think there's nothing left to do there, at least not for test coverage. Closing as implemented, but feel free to undo if I'm mistaken.

comment:22 Changed 15 months ago by nickm

What about juga's patch above?

comment:23 Changed 15 months ago by juga

Added second patch for https://trac.torproject.org/projects/tor/timeline?from=2018-03-15T14%3A33%3A47Z&precision=second.
Reopening since patches aren't reviewed. Please, let me know if i should open a different ticket instead.

comment:24 in reply to:  description Changed 15 months ago by juga

Resolution: implemented
Status: closedreopened

Replying to nickm:

The geoip_load_file() function has no test coverage. As a demo at the hackfest, I worked with Juga to fix that situation. Please review branch geoip_testing in my public repository.

comment:25 Changed 15 months ago by nickm

Status: reopenedneeds_review

comment:26 Changed 15 months ago by isis

Couple trivial things:

  1. We can remove the "is this necessary?" comments on things that are tested twice between test_geoip_load_file() and test_geoip6_load_file(). I think it's fine to test the same thing more than once; it's better than not testing at all, or assuming another test will catch the bad behaviour you have in mind.
  2. couple typos: s/1nd/1st/ s/2st/2nd/
  3. I think we might need TT_FORK|SKIP_ON_WINDOWS for test_geoip6_load_file().
  4. Same as #3, but also for test_geoip_load_2nd_file().

I've applied your patches to a bug25515 branch based on the latest master, and the Travis tests fail because of the unused variables num_countries_geoip2 and num_countries_geoip. Since the patches didn't automatically apply for some reason, I went ahead and added some extra commits fixing the above nitpicks while I was manually applying.


Less trivial issues:

  1. In your test_geoip_load_2nd_file() test, there's a FIXME:
 int num_countries_geoip = geoip_get_n_countries();

 /* Load 2st geoip (empty) file */
 const char FNAME2[] = SRCDIR "/src/test/geoip_dummy";
 tt_int_op(0, OP_EQ, geoip_load_file(AF_INET, FNAME2));

 int num_countries_geoip2 = geoip_get_n_countries();
 /* FIXME: should not this be different? */
 /* tt_int_op(num_countries_geoip, OP_NE, num_countries_geoip2); */

The relevant bits of code from geoip_load_file() are:

int
geoip_load_file(sa_family_t family, const char *filename)
{
  FILE *f;
  const char *msg = "";
  const or_options_t *options = get_options();
  int severity = options_need_geoip_info(options, &msg) ? LOG_WARN : LOG_INFO;
  crypto_digest_t *geoip_digest_env = NULL;

  tor_assert(family == AF_INET || family == AF_INET6);

  if (!(f = tor_fopen_cloexec(filename, "r"))) {
    log_fn(severity, LD_GENERAL, "Failed to open GEOIP file %s.  %s",
           filename, msg);
    return -1;
  }
  if (!geoip_countries)
    init_geoip_countries();

  if (family == AF_INET) {
    if (geoip_ipv4_entries) {
      SMARTLIST_FOREACH(geoip_ipv4_entries, geoip_ipv4_entry_t *, e,
                        tor_free(e));
      smartlist_free(geoip_ipv4_entries);
    }
    geoip_ipv4_entries = smartlist_new();
  } else { /* AF_INET6 */
    // [snip]
  }
  geoip_digest_env = crypto_digest_new();

  log_notice(LD_GENERAL, "Parsing GEOIP %s file %s.",
             (family == AF_INET) ? "IPv4" : "IPv6", filename);
  while (!feof(f)) {
    char buf[512];
    if (fgets(buf, (int)sizeof(buf), f) == NULL)
      break;
    crypto_digest_add_bytes(geoip_digest_env, buf, strlen(buf));
    /* FFFF track full country name. */
    geoip_parse_entry(buf, family);
  }
  /*XXXX abort and return -1 if no entries/illformed?*/
  fclose(f);

So it's clearing all the entries (but curiously not the countries!), which is quite non-intuitive and doesn't appear to be documented anywhere, so I'm not even sure if this is the intended behaviour. Then you're hitting the fclose() there since the file is empty, so nothing else happens.

I'm not sure what to do about the non-intuitive and possibly unintentional behaviour, i.e. whether we should also be clearing the countries in geoip_load_file() when we clear the entries? I think we actually want to be calling clear_geoip_db() in geoip_load_file().

comment:27 Changed 15 months ago by isis

Status: needs_reviewneeds_revision

comment:28 Changed 15 months ago by nickm

Since this ticket is only about adding unit tests, I'd suggest we hold off on changing geoip_load_file()'s behavior. We can't have it actually clear the countries without significant refactoring, since the list of countries is shared by both IPv4 and IPv6.

comment:29 Changed 14 months ago by juga

Thanks isis for your review and the fixes with the trivial things.
For the less trivial ones, and after nickm, i have removed the FIXME and the unused variables.

Since your branch is in github, i pushed to github too, you can see the diff here:
https://github.com/isislovecruft/tor/compare/bug25515...juga0:bug25515_rmunused_vars?diff=split&expand=1&name=bug25515_rmunused_vars

Should i attach the patch here or is it easier to just merge or comment my branch in github?.

Last edited 14 months ago by juga (previous) (diff)

comment:30 Changed 14 months ago by nickm

Status: needs_revisionneeds_review

comment:31 in reply to:  29 ; Changed 14 months ago by isis

Replying to juga:

Thanks isis for your review and the fixes with the trivial things.
For the less trivial ones, and after nickm, i have removed the FIXME and the unused variables.

Since your branch is in github, i pushed to github too, you can see the diff here:
https://github.com/isislovecruft/tor/compare/bug25515...juga0:bug25515_rmunused_vars?diff=split&expand=1&name=bug25515_rmunused_vars

Should i attach the patch here or is it easier to just merge or comment my branch in github?.


Thanks! Git is much easier for me personally, if that works okay for you!

comment:32 in reply to:  31 Changed 14 months ago by isis

Status: needs_reviewmerge_ready

Replying to isis:

Replying to juga:

Thanks isis for your review and the fixes with the trivial things.
For the less trivial ones, and after nickm, i have removed the FIXME and the unused variables.

Since your branch is in github, i pushed to github too, you can see the diff here:
https://github.com/isislovecruft/tor/compare/bug25515...juga0:bug25515_rmunused_vars?diff=split&expand=1&name=bug25515_rmunused_vars

Should i attach the patch here or is it easier to just merge or comment my branch in github?.


Thanks! Git is much easier for me personally, if that works okay for you!


juga's bug25515_rmunused_vars branch lgtm, but we need the extra newline at the EOF of src/test/test_geoip.c added back in.

comment:33 Changed 14 months ago by juga

Thanks, i made one more commit in that branch, recovering the newline.

comment:34 Changed 14 months ago by dgoulet

Keywords: tor-test geoip added
Resolution: fixed
Status: merge_readyclosed

make check-spaces is failing on juga0 branch because a wide line in test_geoip.c.

I've fixed it in ticket25515_034_01 with fixup commit: 276cd80a3217fa91. I made the branch ticket25515_034_01-squashed with the fixup squashed.

... and it is merged! Thanks!

Note: See TracTickets for help on using tickets.