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

The changes are in the branch "options_test"

https://github.com/twstrike/tor_for_patching/tree/options_test

Trac:
Username: rjunior

changed milestone to %Tor: 0.2.8.x-final

added 028-triaged TorCoreTeam201602 component::core tor/tor milestone::Tor: 0.2.8.x-final points::small priority::medium reporter::rjunior resolution::fixed severity::normal sponsor::S status::closed testing type::enhancement labels

Trac:
Username: rjunior
Summary: Improve coverage on src/or/config.c to Improve coverage on src/or/config.c (options_validate)

Trac:
Username: rjunior
Status: new to needs_review

Trac:
Milestone: N/A to Tor: 0.2.8.x-final

Trac:
Keywords: N/A deleted, 028-triaged added

Trac:
Sponsor: N/A to SponsorS

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

Trac:
Points: N/A to small

I have just pushed some updates to the branch that fixes all whitespace issues.

There seems to be a large number of memory leaks here. Is that something you can fix?

I'm attacking the valgrind logs.

Trac:
Status: needs_review to needs_revision
Severity: N/A to Normal

Trac:
leaks_options

Sorry about that, I will take a look over the next few days hopefully.

I have now pushed a fix to the branch in question that fixes all memory leaks in our tests. There is one exception, where one test triggers OpenSSL to cache some ECC information.

There is also a small leak in the options_validate test that was there before - this has to do with the log capture, that uses a log callback. This log callback gets registered with a duplicated string for the filename, but I couldn't really figure out the right place to free this.

Trac:
Status: needs_revision to needs_review

Replying to olabini:

I have now pushed a fix to the branch in question that fixes all memory leaks in our tests. There is one exception, where one test triggers OpenSSL to cache some ECC information.

This is probably ok.

There is also a small leak in the options_validate test that was there before - this has to do with the log capture, that uses a log callback. This log callback gets registered with a duplicated string for the filename, but I couldn't really figure out the right place to free this.

Tinytest has a setup/teardown mechanism that could be used to deregister the callback and free the string.

Alternately, it can be deregistered/freed at the end of every test function.

(Or are you having trouble accessing the duplicated string from the tests? If it's static, it can be marked STATIC, and placed in the *_PRIVATE section of the header.)

Trac:
Status: needs_review to needs_revision

As I said, the leak was there before. I can definitely try to fix it in any of those ways, but it's likely to be a bit more invasive than just accessing a STATIC.

I have fixed this in the most appropriate way I could figure, by adding a small helper function to cleanly remove the log callback. Hopefully this should all be fine now.

Trac:
Status: needs_revision to needs_review

Replying to olabini:

I have fixed this in the most appropriate way I could figure, by adding a small helper function to cleanly remove the log callback. Hopefully this should all be fine now.

Couldn't you just have used mark_logs_temp() and close_temp_logs()?

Thank you, I didn't find those functions when looking for something suitable. The branch is now updated to use those instead.

Replying to olabini:

Thank you, I didn't find those functions when looking for something suitable. The branch is now updated to use those instead.

I would also swap clear_log_messages and close_temp_logs to be sure no messages are sent to the callback function in between those calls (which would then reallocated the messages smartlist, leading to another memory leak). It is unlikely to happen but it is better to be correct now than later IMO.