Opened 3 years ago

Closed 2 years ago

#19024 closed enhancement (fixed)

prop224: Refactor rend_data_t so be able to use multiple HS version

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, prop224, TorCoreTeam201609, nickm-deferred-20161005
Cc: Actual Points:
Parent ID: #17238 Points: 4
Reviewer: asn Sponsor: SponsorR-must

Description

Break rend_data_t into something that could looks like this (or maybe without a union...):

struct rend_data_t {
  uint32_t version; /* XXX: Maybe not necessary if our code flow doesn't
                     * require us to learn the version through that data struct. */
  union {
    hs_data_v2_t v2;
    hs_data_v3_t v3;
  } hs;
};

Once we have such construction, we can use v3 with that data structure more cleanly.

Child Tickets

Change History (20)

comment:1 Changed 2 years ago by dgoulet

Points: 1
Status: newaccepted

comment:2 Changed 2 years ago by dgoulet

Points: 14
Status: acceptedneeds_review

This is the _first_ building block for the entire HSDir support. I'm submitting a request for review so if we are satisfied with it, it will be the first commit of the upcoming branch for the entire implementation (#17238). It might change slightly as we add more commits but the bulk of the work is here:

https://gitlab.com/dgoulet/tor/merge_requests/8

comment:3 in reply to:  2 ; Changed 2 years ago by asn

Replying to dgoulet:

This is the _first_ building block for the entire HSDir support. I'm submitting a request for review so if we are satisfied with it, it will be the first commit of the upcoming branch for the entire implementation (#17238). It might change slightly as we add more commits but the bulk of the work is here:

https://gitlab.com/dgoulet/tor/merge_requests/8

I did an initial review here just to keep things rolling.

I will do more review here in the future.

comment:4 Changed 2 years ago by asn

Reviewer: asn

comment:5 Changed 2 years ago by nickm

Keywords: review-group-6 added

comment:6 in reply to:  3 ; Changed 2 years ago by dgoulet

Replying to asn:

Replying to dgoulet:

https://gitlab.com/dgoulet/tor/merge_requests/8

I did an initial review here just to keep things rolling.

I will do more review here in the future.

Thanks for this. I've addressed your comments. See 3 new commits on top I pushed to the merge request. In particular, the addition of a new file for rend_data_t stuff.

comment:7 Changed 2 years ago by dgoulet

Keywords: review-group-6 removed

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

Replying to dgoulet:

Replying to asn:

Replying to dgoulet:

https://gitlab.com/dgoulet/tor/merge_requests/8

I did an initial review here just to keep things rolling.

I will do more review here in the future.

Thanks for this. I've addressed your comments. See 3 new commits on top I pushed to the merge request. In particular, the addition of a new file for rend_data_t stuff.

Great stuff. I did another review on that same merge request. The merge request has been polluted with various unrelated changes from master, but hopefully it's still readable.

Code looks good here! I guess the next step here would be testing it with the current HS system.

comment:9 Changed 2 years ago by dgoulet

Keywords: TorCoreTeam201608 added

comment:10 Changed 2 years ago by nickm

Keywords: review-group-7 added

comment:11 Changed 2 years ago by dgoulet

Keywords: review-group-7 removed

Removing this from the review group because this ticket patch is part of the big #17238.

comment:12 Changed 2 years ago by dgoulet

Keywords: TorCoreTeam201609 added; TorCoreTeam201608 removed

Review and merge will happen in September.

comment:13 Changed 2 years ago by nickm

Keywords: review-group-8 added

comment:14 Changed 2 years ago by nickm

Status: needs_reviewneeds_revision

see parent

comment:15 Changed 2 years ago by nickm

Keywords: review-group-9 added; review-group-8 removed

comment:16 Changed 2 years ago by nickm

Keywords: nickm-deferred-20161005 added
Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final

Deferring big/risky-feature things (even the ones I really love!) to 0.3.0. Please argue if I'm wrong.

comment:17 Changed 2 years ago by nickm

These have sat in needs_revision for a few weeks at least. Removing from review-group-9, not adding to review-group-10.

comment:18 Changed 2 years ago by dgoulet

Status: needs_revisionaccepted

The implementation is in parent ticket which is being reviewed so putting this one in Accepted state and once parent is merged, we can simply close.

comment:19 Changed 2 years ago by nickm

Keywords: review-group-9 removed

comment:20 Changed 2 years ago by dgoulet

Resolution: fixed
Status: acceptedclosed

Merged as part of #17238 in commit 8293356a.

Note: See TracTickets for help on using tickets.