Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tor Trac #25417]: HSFETCH support for v3 Hidden Services #646

Merged
merged 1 commit into from Feb 24, 2019

Conversation

Projects
None yet
4 participants
@neelchauhan
Copy link
Contributor

commented Jan 14, 2019

For the bug here.

@@ -443,7 +443,7 @@ pick_hsdir_v3(const ed25519_public_key_t *onion_identity_pk)
*
* On success, HS_CLIENT_FETCH_LAUNCHED is returned. Otherwise, an error from
* hs_client_fetch_status_t is returned. */
MOCK_IMPL(STATIC hs_client_fetch_status_t,
MOCK_IMPL(hs_client_fetch_status_t,
fetch_v3_desc, (const ed25519_public_key_t *onion_identity_pk))

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Jan 23, 2019

Contributor

Making this one public then we need to change its name with hs_client_.

@neelchauhan neelchauhan force-pushed the neelchauhan:b25417 branch from 247acb4 to 6dcd93f Jan 23, 2019

@coveralls

This comment has been minimized.

Copy link

commented Jan 23, 2019

Pull Request Test Coverage Report for Build 3694

  • 0 of 22 (0.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 61.73%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/feature/hs/hs_control.c 0 3 0.0%
src/feature/hs/hs_client.c 0 7 0.0%
src/feature/control/control.c 0 12 0.0%
Totals Coverage Status
Change from base Build 3677: -0.01%
Covered Lines: 45258
Relevant Lines: 73316

💛 - Coveralls
if (hsdir_rs != NULL) {
hs_client_launch_v3_desc_fetch(&v3_pk, hsdir_rs);
} else {
hs_client_fetch_v3_desc(&v3_pk);

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Jan 23, 2019

Contributor

Can't we simply have one function that would accept NULL for the hsdir_rs ?

static hs_client_fetch_status_t
directory_launch_v3_desc_fetch(const ed25519_public_key_t *onion_identity_pk,
hs_client_fetch_status_t
hs_client_launch_v3_desc_fetch(const ed25519_public_key_t *onion_identity_pk,
const routerstatus_t *hsdir)

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Jan 23, 2019

Contributor

I would really rather keep that one private and add a function to the public API that calls this one if hsdir is non NULL or else call fetch_v3_desc() (keeping it private also).

So we have one single entry point into the HS subsystem. Actually, more annoying, I think hs_control.c is the better place. We would have something like hs_control_hsfetch_command(...) which would then use the new call I'm talking about above which we need public but only used within the HS subsystem though.

Hope that is clear/makes sense?

@neelchauhan neelchauhan force-pushed the neelchauhan:b25417 branch 3 times, most recently from cd0e984 to 23bc488 Jan 23, 2019

hs_client_launch_v3_desc_fetch(const ed25519_public_key_t *onion_identity_pk,
const smartlist_t *hsdirs)
{
tor_assert(hsdirs);

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Jan 24, 2019

Contributor

We can't assert here.

hs_control_hsfetch_command(const ed25519_public_key_t *onion_identity_pk,
const smartlist_t *hsdirs)
{
tor_assert(hsdirs);

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Jan 24, 2019

Contributor

Can't assert.

@@ -259,3 +260,15 @@ hs_control_hspost_command(const char *body, const char *onion_address,
smartlist_free(hsdirs);
return ret;
}

/* With a given <b>onion_identity_pk</b>, fetch its descriptor. This function
* calls hs_client_launch_v3_desc_fetch(). */

This comment has been minimized.

Copy link
@dgoulet-tor

dgoulet-tor Jan 24, 2019

Contributor

We need to document hsdirs especially that it can be NULL.

@neelchauhan neelchauhan force-pushed the neelchauhan:b25417 branch from 23bc488 to c985940 Jan 24, 2019

@torproject-pusher torproject-pusher merged commit c985940 into torproject:master Feb 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.