Opened 5 months ago

Closed 7 weeks ago

#22840 closed enhancement (implemented)

Implement protover in Rust

Reported by: chelseakomlo Owned by: chelseakomlo
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rust, protover, rust-pilot, review-group-24
Cc: isis, Sebastian, nickm, alexcrichton, manishearth@…, catalyst Actual Points:
Parent ID: Points: 3
Reviewer: Sponsor: SponsorV

Description

As part of the effort to move toward supporting Rust as a first-level language in Tor, one objective is to implement an existing non-trivial tor submodule in Rust.

We have identified protover as a good candidate, as it is relatively small, has few external dependencies, and has good test coverage.

Child Tickets

Change History (18)

comment:1 Changed 5 months ago by chelseakomlo

Owner: set to chelseakomlo
Status: newassigned

comment:2 Changed 5 months ago by isis

Keywords: rust added
Points: 3
Sponsor: SponsorZ

comment:3 Changed 5 months ago by nickm

Keywords: protover rust-pilot added

comment:4 Changed 3 months ago by chelseakomlo

See git@github.com:chelseakomlo/tor_patches.git, branch protover-rust-impl

Some considerations:

  1. Changes to protover.c

src/or/protover.c was rearranged in order to have functionality defined based on conditional definitions. These fall into two categories: 1) functions that should only be defined when Rust is not enabled are guarded by HAVE_RUST, and 2) functions that are used in src/test/test_protover.c are guarded by HAVE_RUST or TOR_UNIT_TESTS (so they are defined only for testing or when Rust is not enabled)

Other than rearranging for ifdef guards, logic in protover.c was not changed

  1. Why src/or/protover_rust.c is needed

Currently, it is important to keep the C code stable as we test feasibility of introducing Rust. In the future, we can change C code to better enable adding Rust functionality, but in the short term, changing C as little as possible will both allow us to merge Rust code, and worry less about introducing bugs in the meantime. Adding wrappers for translation in src/or/protover_rust.c is a temporary measure and is not meant as a long-term solution.

  1. Naming/similar functionality between src/common/compat_rust and src/commonrust_types

As we are currently experimenting with Rust, I introduced another method of handling strings between the Rust/C ABI, which made implementing protover in Rust simpler. I propose that we discuss in another ticket how to move forward with only one method, and merge this patch with both enabled.

This patch introduces a less heavyweight manner to handle strings passed from Rust to C, to better allow Rust to assign string values to pointers passed as function arguments.

  1. String handling/copying

In this patch, strings are passed to C and then immediately copied. The string allocated in Rust is immediately freed. In the future, we can discuss implementations that avoid this extra copy, but I propose merging this patch without that enhancement.

  1. Extending src/rust/smartlist

In the future, we should talk about how to better handle smartlists that are passed between C and Rust, as currently only smartlists of strings is enabled.

  1. Documentation and tests in src/rust

Please let me know if more/less documentation would be helpful, or if certain things can be better documented and/or tested.

  1. Logging

This patch does not implement logging to tor's logger from Rust. Maybe this functionality can be added separately, and then logging can be added here afterward?

comment:5 Changed 3 months ago by chelseakomlo

Cc: nickm added
Status: assignedneeds_review

comment:6 Changed 3 months ago by chelseakomlo

Cc: alexcrichton added

comment:7 Changed 3 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.3.x-final

comment:8 Changed 2 months ago by manish.earth

Cc: manishearth@… added

comment:9 Changed 8 weeks ago by nickm

Keywords: review-group-24 added

review-group-24 is now open.

comment:10 Changed 8 weeks ago by nickm

Sponsor: SponsorZSponsorV

comment:11 Changed 8 weeks ago by chelseakomlo

See latest updates based on feedback at Tor Dev at git@github.com:chelseakomlo/tor_patches.git, branch protover-rust-impl. I can also rebase onto master and squash these commits after review if that would be helpful.

Notable changes include:

Future improvements:

  • The module to translate smartlists into Rust vectors (/src/rust/smartlist) can be improved to use iterators to avoid an extra copy. I plan on doing this in a separate issue.
  • Logging should be added. This is tracked in #23881

One thing that I think is less than ideal in this patch is the rearrangement of protover.c. Please let me know any recommendations of how to do this more cleanly.

If anyone wants to talk through this on irc, please let me know! Looking forward to feedback and anything that can be can improved upon.

comment:12 Changed 8 weeks ago by chelseakomlo

Cc: catalyst added

comment:13 Changed 8 weeks ago by nickm

Looks cool! I'd like to get this into Tor soon, so I hope we can postpone any issues we identify into separate tickets.

Since this is a big patch, it might be more ergonomic to review it on oniongit. Can you make a merge request there, or should I?

comment:14 Changed 8 weeks ago by chelseakomlo

Hey! Sounds great. I haven't used oniongit before, but getting set up there is a good idea for future work as well.

Totally up to you if you want to wait for a review for that- I can get set up this week.

As a side note- I pushed one commit this morning for cargo fmt fixes. Looking forward to getting automated linting in as a next step :)

Last edited 8 weeks ago by chelseakomlo (previous) (diff)

comment:15 Changed 7 weeks ago by nickm

Okay -- I made an oniongit merge_request at https://oniongit.eu/nickm/tor/merge_requests/9 . If I can, I'll start commenting there today on any line-by-line issues. Also, let's talk on IRC tomorrow!

comment:16 Changed 7 weeks ago by nickm

So, I've spent a while looking over the branch, and while I think I'll have some suggestions, I think it might make sense to do a merge first, and open a bunch of tickets to work on afterwards. I'll add more comments on oniongit as I go along.

One thing I didn't understand: What's going on with the reorganization of protover.c ? I don't understand the rationale there.

comment:17 Changed 7 weeks ago by chelseakomlo

Hey Nick- either way. I'm happy to fix up small things immediately as well, especially for C fixes.

I pushed up commit 31a2dce30762fb12a00745af56a93784f8907a78 this morning to git@github.com:chelseakomlo/tor_patches.git branch protover-rust-impl. This has some cargo fmt fixes, if you wouldn't mind merging this as well. Sorry for pushing that so late!

The reorganization of protover.c is because some of the unit tests in test_protover.c use static methods defined in protover.c.

See line 34 and and 251 in protover.c in the patch. For example, test_protover_parse in test_protover.c uses parse_protocol_list.

I also thought about copying private functions used by protover tests into test_protover.c, but then we have duplicated code. That might be a better solution in the short term instead of rearranging protover.c- I'm curious to hear your thoughts.

Another thing I wanted to call out is that src/rust/tor_allocate borrowed heavily from Manish's earlier proof of concept. See https://github.com/Manishearth/freestring, from_bytes_unchecked.

See you on IRC tomorrow!

comment:18 Changed 7 weeks ago by nickm

Resolution: implemented
Status: needs_reviewclosed

As discussed on IRC: I've revised protover.c to have a smaller diff, disabled the tests that only looked at static functions inside protover.c, and added copyright notices. With that, it's all merged to master! I'll be opening tickets for other issues/questions/concerns.

Note: See TracTickets for help on using tickets.