Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#21978 closed enhancement (implemented)

hs: Decouple adding and validating a service

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224
Cc: Actual Points:
Parent ID: #21888 Points: 0.2
Reviewer: asn Sponsor: SponsorR-must

Description

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.

Child Tickets

Change History (10)

comment:1 Changed 2 years ago by dgoulet

Status: newneeds_review

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

comment:2 Changed 2 years ago by asn

Reviewer: asn

comment:3 Changed 2 years ago by 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/

comment:4 Changed 2 years ago by dgoulet

Thanks for review!

I've responded and added a fixup commit.

comment:5 Changed 2 years ago by dgoulet

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

comment:6 Changed 2 years ago by 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.

comment:7 in reply to:  6 ; Changed 2 years ago by 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) so it's not in that function... Would you prefer having it there and then modified in the refactoring?

comment:8 in reply to:  7 Changed 2 years ago by asn

Status: needs_reviewmerge_ready

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) 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.

comment:9 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Sure; merged!

Question -- how's coverage here?

comment:10 in reply to:  9 Changed 2 years ago by dgoulet

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.

Note: See TracTickets for help on using tickets.