hs: Decouple adding and validating a service

One commit that splits the rend_add_service() function into two functions. One actually adding the service to the global list and the second one to validate the service thus adding a new function: rend_validate_service()

We need this for prop224 code that will in two steps validate and then add the service when loading a service from configuration.

changed milestone to %Tor: 0.3.1.x-final

added component::core tor/tor milestone::Tor: 0.3.1.x-final owner::dgoulet parent::21888 points::0.2 priority::medium prop224 resolution::implemented reviewer::asn severity::normal sponsor::R-must status::closed tor-hs type::enhancement labels

Tor code: ticket21978_031_01 Gitlab: https://gitlab.com/dgoulet/tor/merge_requests/26

Trac:
Status: new to needs_review

Trac:
Reviewer: N/A to asn

Did an initial review but there was lots of moving&editing in the same commit which confused me a bit. I should probably look at it again/

Thanks for review!

I've responded and added a fixup commit.

I made two commits now, one that only creates the validate function (minimal changes to the code) and then a second commit that fixes the duplication in rend_add_service() and make the service configure process use both functions correctly.

See branch: ticket21978_031_02

OK this last branch made the changes slightly clearer.

Changes LGTM.

One thing I did not understand is why the validation function is not called by rend_service_check_dir_and_add(), and instead we have to call both functions in a row.

Replying to asn:

OK this last branch made the changes slightly clearer.

Changes LGTM.

One thing I did not understand is why the validation function is not called by rend_service_check_dir_and_add(), and instead we have to call both functions in a row.

This changes heavily in prop224 refactoring (#21979 (moved)) so it's not in that function... Would you prefer having it there and then modified in the refactoring?

Replying to dgoulet:

Replying to asn:

OK this last branch made the changes slightly clearer.

Changes LGTM.

One thing I did not understand is why the validation function is not called by rend_service_check_dir_and_add(), and instead we have to call both functions in a row.

This changes heavily in prop224 refactoring (#21979 (moved)) so it's not in that function... Would you prefer having it there and then modified in the refactoring?

Nah, I think it's fine. It's just two occurrences for now.

Trac:
Status: needs_review to merge_ready

Sure; merged!

Question -- how's coverage here?

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

Replying to nickm:

Sure; merged!

Question -- how's coverage here?

Very low... but the good news is that I'm almost at 100% with prop224 for loading/configure service which uses heavily those functions.

closed

changed time estimate to 1h 36m

mentioned in issue #21979 (moved)

moved to tpo/core/tor#21978 (closed)