Opened 3 months ago

Last modified 3 days ago

#22840 needs_review enhancement

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
Cc: isis, Sebastian, nickm, alexcrichton, manishearth@… Actual Points:
Parent ID: Points: 3
Reviewer: Sponsor: SponsorZ

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 (8)

comment:1 Changed 3 months ago by chelseakomlo

Owner: set to chelseakomlo
Status: newassigned

comment:2 Changed 3 months ago by isis

Keywords: rust added
Points: 3
Sponsor: SponsorZ

comment:3 Changed 3 months ago by nickm

Keywords: protover rust-pilot added

comment:4 Changed 3 weeks 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 weeks ago by chelseakomlo

Cc: nickm added
Status: assignedneeds_review

comment:6 Changed 3 weeks ago by chelseakomlo

Cc: alexcrichton added

comment:7 Changed 3 weeks ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.3.x-final

comment:8 Changed 3 days ago by manish.earth

Cc: manishearth@… added
Note: See TracTickets for help on using tickets.