-
Notifications
You must be signed in to change notification settings - Fork 173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFE: investigate Rust bindings #323
Comments
@drakenclimber thoughts on this? If this sounds good I can reach out to polachok to see if they are interested. At the very least I would want to make sure that they are okay with the stuff in CONTRIBUTING.md and MAINTAINER_PROCESS.md; it seems like any project under github.com/seccomp should adopt similar policies. Thoughts? |
Yes, I think rust support would be a very good thing. (Aside - the rust kernel RFC that was recently sent out seems very interesting.)
Sounds great to me. If possible, I would love to credit and use an existing project rather than reinventing a wheel. |
Hello, @pcmoore @drakenclimber I'm sorry to interrupt you, but I'd like to introduce you to our related project. @ManaSugi Could you explain feature and status of https://github.com/ManaSugi/libseccomp-rs? |
@KentaTada Thank you for the introduction! Hello, @pcmoore, @drakenclimber The libseccomp crate wraps libseccomp-sys of low-level bindings in a nice to use, mostly safe API. Currently, we are working on adding seccomp feature to kata-containers using our In addition, the Please let me know if you are interested in our libseccomp crate. |
Thanks @KentaTada and @ManaSugi, this is something worth looking into! Unfortunately I am still new to Rust so it is hard for me to judge things at the moment but I definitely prefer bindings which are generated by hand over automated bindings; they tend to hold up better over the long run. This is one of the reasons why the Python bindings were done by hand using Cython and not automated via SWIG. I also very much like the idea of adopting bindings from people who have been active and contributed patches to the general libseccomp project already. These people are less of an unknown and I feel like I can trust them more than people who have never contributed :) @drakenclimber any thoughts on the projects mentioned above? |
@pcmoore Thank you for your valuable opinion.
I understand the reason. |
I looked through libseccomp-rs. Nice work, @ManaSugi and @KentaTada. It looks really solid to me. I have a handful of thoughts:
|
After looking at the Rust documentation, are you using the |
Thank you very much for the review of our crate, @drakenclimber.
The reason why all calls to the libseccomp C library are surrounded by
Unfortunately, we can not call the libseccomp C library from Rust code without
Yes, we recommend that developers should use libseccomp crate with dynamically linked the libseccomp C library. Therefore, the crate is linked dynamically by default.
Our libseccomp crate includes only the function interface of the libseccomp C library, not the C code itself. However, if the crate is used with statically linked the libseccomp library that is under
I'm interested in sharing automated testing between various languages, but I think it is difficult because of the different function interfaces. Could you tell me your idea about a way to share libseccomp test? |
Makes sense. Thank you. I'm newer to rust as well and hadn't used the FFI portion yet, but did hit the memory checking once.
Sounds good.
As I said originally, I try to stay out of license discussions and leave them to the lawyers :). But what you outlined above is exactly the scenario I was afraid of. It might be good to run these scenarios by your company's legal team.
(Warning - not a fully thought out idea here.) The automated tests within libseccomp do a really good job of covering every major feature from a functional point of view. And the framework supports both C and Python already. I don't think it would be too tough to add support for more languages to the framework. Over time we could add Golang and Rust tests alongside the existing tests. But where should these tests reside in github? In libcgroup, we split the tests into their own repo - which has had some definite pros/cons. We could leave the tests in the seccomp/libseccomp repo, then have the other languages add the tests (and libcgroup library code) as a git submodule. Again, thinking out loud here, that could work pretty well... Ultimately my real goal is to get better functional code coverage for Golang and Rust. I'm open in how we get there. Thanks! |
@drakenclimber, Thank you for the kind reply. I'm sorry, I misunderstood about the automated tests, but now I understand that. First of all, I'll start to create more tests of our libseccomp crate while referencing the existing functional tests. Thanks again. |
Possibly, but honestly I don't really know. I am also the maintainer of libcgroup, and a while back we split our tests into a separate git repo. Overall I have mixed feelings on the split. The tests can grow and change without excess code churn in the main repo, but keeping the two repos in sync is more work than I like. Also, using git submodules is less than ideal. tl;dr - I'm not sure what the right answer is regarding tests for multiple languages and multiple platforms. I don't think there's an easy answer :( |
Edited 2022/01/05 https://github.com/libseccomp-rs/libseccomp-rs In addition, we have started managing the Currently, I'm considering a way to add automated testing to the existing framework that supports C and Python already. In the commit, I added the
The tests can be run by the following commands: $ ./configure --enable-rust
$ cd tests && make check-build
$ ./regression -m rust Could you give me your thoughts about the |
I really like what you've done with these two crates. Hiding the ugly C bindings in the low-level sys crate makes for a clean interface. The interfaces in the user-facing libseccomp crate map nicely to the low-level C while still taking advantage of Rust and its nifty features. Nice work.
Your integration of the Rust code and the existing libcgroup library/tests is creative. Very cool. I have a few questions, but I'll post them inline to the RFC patch you sent. Thanks so much @ManaSugi |
Add the regression tests for the libseccomp crate that is Rust language bindings for the libseccomp library. You can run the tests as follows. ```sh $ cd tests $ make check-build $ ./regression -m rust ``` If you do not want to build Rust test programs, set `RUST_BINDINGS_TEST=no` as follows. ```sh $ make check-build RUST_BINDINGS_TEST=no ``` Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
Thank you for your review. |
Hello, @pcmoore @drakenclimber I've released libseccomp-rs v0.3.0 recently, but I'm sorry for the late update on the test programs for rust. Currently, I prepared for 12 tests for rust, but there are still a lot of tests and it could take some time to create all tests (60 tests). Is it nice to start considering that libseccomp-rs will be migrated under the |
Thanks for the update @ManaSugi |
Hello, @pcmoore @drakenclimber We created 60 tests and confirmed the CI works properly. Some tests still fail because the libseccomp-rs doesn't support yet the latest functions or architectures that will include in the next release, v2.6.0. (we have plan to support them soon.) But, we are ready to happily migrate the libseccomp-rs under your seccomp project. Thank you. |
Thanks, @ManaSugi. You've put a lot of time into this work; thank you. I briefly looked through your changes, and they seem like a reasonable patchset to start the discussion. I recommend you open a pull request. A couple questions off the top of my head:
|
Add the regression tests (1-60) for the libseccomp crate that is Rust language bindings for the libseccomp library. You can run the tests as follows: ```sh $ sed -i "/^AC_INIT/ s/0.0.0/9.9.9/" configure.ac $ ./autogen.sh $ ./configure --prefix=$(pwd)/src/.libs --enable-rust $ make && make install $ make check-build $ cd tests && ./regression -m rust ``` Based on: seccomp#323 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com> Signed-off-by: mayank <mayank.mrinal@sony.com>
@drakenclimber, @pcmoore Thanks for the review and I'm glad to start the disucussion.
I removed the
I also think merging the Rust bindings into the main repo seems to be powerful, but some problems are caused. Merging the Rust bidings into the main repo:
|
Add the regression tests (1-60) for the libseccomp crate that is Rust language bindings for the libseccomp library. You can run the tests as follows: ```sh $ sed -i "/^AC_INIT/ s/0.0.0/9.9.9/" configure.ac $ ./autogen.sh $ ./configure --prefix=$(pwd)/src/.libs --enable-rust $ make && make install $ make check-build $ cd tests && ./regression -m rust ``` Based on: seccomp#323 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com> Signed-off-by: mayank <mayank.mrinal@sony.com>
Add the regression tests (1-60) for the libseccomp crate that is Rust language bindings for the libseccomp library. You can run the tests as follows: ```sh $ sed -i "/^AC_INIT/ s/0.0.0/9.9.9/" configure.ac $ ./autogen.sh $ ./configure --prefix=$(pwd)/src/.libs --enable-rust $ make && make install $ make check-build $ cd tests && ./regression -m rust ``` Based on: seccomp#323 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com> Signed-off-by: mayank <mayank.mrinal@sony.com>
Add the regression tests (1-60) for the libseccomp crate that is Rust language bindings for the libseccomp library. You can run the tests as follows: ```sh $ sed -i "/^AC_INIT/ s/0.0.0/9.9.9/" configure.ac $ ./autogen.sh $ ./configure --prefix=$(pwd)/src/.libs --enable-rust $ make && make install $ make check-build $ cd tests && ./regression -m rust ``` Based on: seccomp#323 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com> Signed-off-by: mayank <mayank.mrinal@sony.com>
Add the regression tests (1-60) for the libseccomp crate that is Rust language bindings for the libseccomp library. You can run the tests as follows: ```sh $ sed -i "/^AC_INIT/ s/0.0.0/9.9.9/" configure.ac $ ./autogen.sh $ ./configure --prefix=$(pwd)/src/.libs --enable-rust $ make && make install $ make check-build $ cd tests && ./regression -m rust ``` Based on: seccomp#323 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com> Signed-off-by: mayank <mayank.mrinal@sony.com>
Add the regression tests (1-60) for the libseccomp crate that is Rust language bindings for the libseccomp library. You can run the tests as follows: ```sh $ sed -i "/^AC_INIT/ s/0.0.0/9.9.9/" configure.ac $ ./autogen.sh $ ./configure --prefix=$(pwd)/src/.libs --enable-rust $ make && make install $ make check-build $ cd tests && ./regression -m rust ``` Based on: seccomp#323 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com> Signed-off-by: mayank <mayank.mrinal@sony.com>
I think it is time to revisit this discussion @drakenclimber and @ManaSugi - back when this issue was created in 2021 I both had more time to devote to libseccomp as well as hopes to learn Rust in the near future. Now, three (almost four) years later, I still have not had the time to learn Rust and the amount of time I am able to allot to libseccomp feature development has decreased, or at least been de-prioritized. This is a long way of saying that I'm not certain I would be able to provide the necessary back-up support for including libseccomp Rust bindings in the main GH seccomp organization at this point in time. @drakenclimber, how about you? Do you both have the cycles and a level of familiarity with Rust to back-up @ManaSugi for inclusion of Rust bindings for libseccomp? |
I am in a similar position. My goal of growing my rust knowledge remains a goal :(. And as has been apparent the last few months, I also have had limited time for libseccomp. With that said, I think libseccomp rust bindings are important and would be a tremendous asset to the community. Given that @pcmoore and I are struggling to find time for libseccomp, I'm now wondering if a separate libseccomp/rust repository is the best solution. I'm definitely open to ideas here, though. |
My only concern with bringing the Rust bindings into this GH org is that if the current devs and maintainers go off to work on other things @drakenclimber and I are going to be responsible for maintaining the bindings which is not something I can agree to support at this point in time. The good news is that there is no harm caused by the Rust bindings living elsewhere. |
@pcmoore @drakenclimber Thank you so much for your comment! I exactly understand your current situation and opinions. From the perspective of the libseccomp-rs side, bringing the Rust bindings into the GH org can help itself to keep up with the same quality of the original C library and the Python bindings by leveraging the common test framework added in the PR #411, and both @rusty-snake (the other maintainer of the Rust binding) and at least I can take so enough time to maintain it as usual even after it is moved to the GH org. Besides, as @pcmoore mentioned on the first comment in this issue, I also feel that there is value in having the bindings in one central location like the libseccomp-golang, which helps it's discoverability. But, I don't have a strong opinion about that and I also agree with the fact that there is no harm caused by Rust bindings living elsewhere. @rusty-snake Could you tell me your thoughts? |
@rusty-snake Do you have any comments? |
I've no clear preference between the repo being in the seccomp or libseccomp-rs org. Test suites can run it both. Only merging in one repo (i.e. one PR for changes) would make a difference. seccomp seems to still have some stricter, arguable DCO policy. Visibility/visible as official might be increased, but you can find it on crates.io (search for a (lib)seccomp crate for rust vs. search for language bindings of libseccomp) and it would be possible to link from here. |
Given the growing popularity of Rust for system development, it seems like it might be a good idea to support a set of libseccomp Rust bindings. We could develop our own, but it seems that a developer has already created a set of bindings for Rust:
I'm not a fan of reimplementing perfectly good code, so if the bindings above seem reasonable we may want to offer the developer a chance to migrate those bindings under https://github.com/seccomp (assuming they would want to do this). Of course, if the developer doesn't want to move their repo, and we agree that their bindings seem reasonable, we can just close this out and point people at the appropriate developers repo.
I mention this as I feel that there is value in having the bindings in one central location; not only does it help discoverability, I feel it helps keep the bindings current.
The text was updated successfully, but these errors were encountered: