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.
To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information
Child items
...
Show closed items
Linked items
0
Link issues together to show that they're related.
Learn more.
See git@github.com:chelseakomlo/tor_patches.git, branch protover-rust-impl
Some considerations:
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
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.
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.
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.
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.
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.
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?
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:
Using tor_malloc for allocating in Rust (see /src/rust/tor_allocate), which enables us to later free in C. In the future we can use tor's allocator by default, so this is a temporary solution.
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 (moved)
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.
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!
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.
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.
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.
Trac: Status: needs_review to closed Resolution: N/Ato implemented