Opened 5 years ago

Closed 5 years ago

#17077 closed enhancement (implemented)

Improve coverage on src/or/config.c (parse_port_config)

Reported by: rjunior Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: testing, 028-triaged
Cc: Actual Points:
Parent ID: Points: small
Reviewer: Sponsor: SponsorS


The changes are in the branch "parse_port_config_tests"

Child Tickets

Change History (14)

comment:1 Changed 5 years ago by rjunior

Status: newneeds_review

comment:2 Changed 5 years ago by nickm

Quick observations:

  • It looks like the test_CL_PORT_* definitions in test_config are redundant. It would be better to just move the CL_PORT_* definitions somewhere that test_config.c and config.c can both see them.
  • Why is the - tor_free(the_tor_version); line in config.c? That seems like an accident.
  • Likewise, it looks like some lines in config.c got reformatted for no real reason.
  • In the test cases, it would be good if they tested more than just 'success/failure' on the parsing cases, but also the error messages that got logged on the failure cases.
  • Use tor_addr_eq, not tor_mem_op, to compare addresses for equality. It's permissible for two structs to be bytewise unequal but semantically equal.
  • Would it make sense to break each start group .... end group pair into its own function?

Questions to always ask:

  • Are there memory leaks in this code? (You can find out by running under valgrind; see doc/HACKING)
  • Does it compile when you configure with --enable-gcc-warnings?
  • Does make check-spaces give any format warnings?

comment:3 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

comment:4 Changed 5 years ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:5 Changed 5 years ago by nickm

Keywords: 028-triaged added

comment:6 Changed 5 years ago by nickm

Sponsor: SponsorS

comment:7 Changed 5 years ago by nickm

Points: small

mark these testing tickets in needs_review as 'small work remaining'

comment:8 Changed 5 years ago by olabini

The tor_free(the_tor_version) removed a duplicate free. You can see that the_tor_version is freed again further down in the function.

comment:9 Changed 5 years ago by olabini

I've fixed all the comments and pushed to the branch now. Everything except for doing more checking on error messages - that will take some more time.

comment:10 Changed 5 years ago by olabini

Severity: Normal

Not sure what the protocol is here - should this be changed back to needs_review when I've updated something?

comment:11 Changed 5 years ago by nickm

Status: needs_revisionneeds_review

Yup. When you think it's ready for another round of review, it goes back in needs-review.

comment:12 Changed 5 years ago by nickm

This removal makes me nervous:

-  tor_free(the_tor_version);

Is it correct? Can you explain why it's there?

The other changes look fine now.

comment:13 Changed 5 years ago by olabini


Understandable, without context. However, this is part of config_free_all, and there were two tor_free(the_tor_version); inside it, this patch just removed the duplicated one. If you look at the full function, you will see that the_tor_version will still be freed, further down in the function.

comment:14 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Makes sense. Merged it!

Note: See TracTickets for help on using tickets.