Opened 3 years ago

Closed 3 years ago

#20568 closed enhancement (implemented)

Move encode_cert() from hs_descriptor.c into torcert.c

Reported by: dgoulet Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, easy, refactoring, review-group-12
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor: SponsorR-can

Description

Give it a better name, maybe improve the code to be a bit more generic and start using it in router.c at minimum.

Child Tickets

Attachments (4)

0001-Move-encode_cert-to-torcert.c-from-hs_descriptor.c.patch (4.8 KB) - added by neel 3 years ago.
Patch to move encode_cert() to torcert.c
tor_check_output.txt (1.1 KB) - added by neel 3 years ago.
Unit testing (done with "make check")
mode_encode_cert.patch (9.8 KB) - added by neel 3 years ago.
Updated patch for encode_cert (now tor_cert_encode_ed22519()) move to torcert.c
0001-Move-encode_cert-to-torcert.c-and-rename-it-to-tor_c.patch (6.4 KB) - added by neel 3 years ago.
Revised patch for encode_cert()

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by neel

Patch to move encode_cert() to torcert.c

Changed 3 years ago by neel

Attachment: tor_check_output.txt added

Unit testing (done with "make check")

comment:1 Changed 3 years ago by neel

I have attempted to do this refactoring, and please tell me what you think about this code.

Thanks,
Neel Chauhan

comment:2 Changed 3 years ago by dgoulet

Status: newneeds_revision

Thanks for this neel!

  • DG1: I would rename the function to something more meaningful as this function encoded an ed25519 certificate which is pretty specific and also should follow the torcert.c namespace. Maybe tor_cert_encode_ed22519() ?
  • DG2: You can remove it from hs_descriptor.h instead of commenting it out.
  • DG3: We have to change this log message to be a bit more generic as it's very specific to HS right now.
        log_err(LD_BUG, "Couldn't base64-encode descriptor signing key cert!");
    

Changed 3 years ago by neel

Attachment: mode_encode_cert.patch added

Updated patch for encode_cert (now tor_cert_encode_ed22519()) move to torcert.c

comment:3 Changed 3 years ago by neel

I have made an updated patch for this. Keep in mind that it is just additions to the first patch, but the changes you requested were made.

Changed 3 years ago by neel

Revised patch for encode_cert()

comment:4 Changed 3 years ago by neel

I created a revised patch (0001-Move-encode_cert-to-torcert.c-and-rename-it-to-tor_c.patch​) which merges all the changes as one commit.

comment:5 Changed 3 years ago by dgoulet

Status: needs_revisionneeds_review

Thanks neel! lgtm! I only fixed one thing which is that I put back this comment in the code ;).

-/* === ENCODING === */

I've put the patch in a branch and added an extra fixup commit for the above: ticket20568_030_01

NOTE: this patch doesn't make router.c use this function, it's only use by the HS subsystem so far. So maybe a second ticket once this merged on actually using this interface in the code.

comment:6 Changed 3 years ago by nickm

Keywords: review-group-12 added

comment:7 Changed 3 years ago by nickm

Status: needs_reviewmerge_ready

comment:8 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.