Open micheledallerive opened 4 months ago
Hi Michele,
thanks a lot for posting this work! So far I've only had time to look at patches 01 and 02, but here are some things that I noticed.
Some general things:
./scripts/checkpatch.pl
can be used to check this).bindgen
generates and 2. your doctests. The changes in bindgen
are quite trivial; for the doctests I'd recommend having a look at the other doctests in the kernel crate. Please note that there are some limitations to what is possible in doctests in the kernel. Below some truncated output of the failing compilation:
error[E0107]: struct takes 1 generic argument but 2 generic arguments were supplied
--> rust/kernel/net/socket.rs:146:19
|
146 | bindings::__BindgenBitfieldUnit::<[u8; 8], u8>::new(self.flags().to_be_bytes())
| ^^^^^^^^^^^^^^^^^^^^^ -- help: remove this generic argument
| |
| expected 1 generic argument
|
note: struct defined here, with 1 generic parameter: `Storage`
--> /mnt/build/rust-for-linux/rust4lx/rust/bindings/bindings_generated.rs:5:12
|
5 | pub struct __BindgenBitfieldUnit<Storage> {
| ^^^^^^^^^^^^^^^^^^^^^ -------
error[E0107]: struct takes 1 generic argument but 2 generic arguments were supplied --> rust/kernel/net/socket.rs:166:23 | 166 | bindings::__BindgenBitfieldUnit::<[u8; 8], u8>::new(self.flags().to_be_bytes()); | ^^^^^^^^^^^^^^^^^^^^^ -- help: remove this generic argument |
---|---|---|---|
expected 1 generic argument | |||
note: struct defined here, with 1 generic parameter: Storage
--> /mnt/build/rust-for-linux/rust4lx/rust/bindings/bindings_generated.rs:5:12
|
5 | pub struct __BindgenBitfieldUnit
error: aborting due to 2 previous errors
For more information about this error, try rustc --explain E0107
.
error: cannot find macro println
in this scope
--> rust/doctests_kernel_generated.rs:3846:4
|
3846 | println!("Flag: {:?}", flag);
| ^^^^^^^
error[E0599]: no function or associated item named from_str found for struct rust_doctest_kernel_build_assert_rs_0::kernel::net::addr::SocketAddrV4 in the current scope
--> rust/doctests_kernel_generated.rs:1534:26 |
1534 | let addr = SocketAddrV4::from_str("1.2.3.4:5678").unwrap(); | ^^^^^^^^ function or associated item not found in SocketAddrV4 |
---|
note: if you're trying to build a new rust_doctest_kernel_build_assert_rs_0::kernel::net::addr::SocketAddrV4 , consider using rust_doctest_kernel_build_assert_rs_0::kernel::net::addr::SocketAddrV4::new which returns rust_doctest_kernel_build_assert_rs_0::kernel::net::addr::SocketAddrV4
--> /mnt/build/rust-for-linux/rust4lx/rust/kernel/net/addr.rs:553:5 |
553 | pub const fn new(addr: Ipv4Addr, port: u16) -> Self { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a use for it: |
---|
3 + use core::str::FromStr; | [...]
error[E0283]: type annotations needed for FlagSet<_>
--> rust/doctests_kernel_generated.rs:3902:5 |
---|---|
3902 | let mut empty_flags = flag_set!(); |
^^^^^^^^^^^^^^^ | |
3903 | assert_eq!(empty_flags.value(), 0); |
----------- ----- required by a bound introduced by this call | |
type must be known at this point | |
= note: cannot satisfy `_: Flag`
= help: the following types implement trait `Flag`:
SendFlag
ReceiveFlag
MessageFlag
note: required by a bound in FlagSet::<T>::value
--> /mnt/build/rust-for-linux/rust4lx/rust/kernel/net/socket/flags.rs:257:9 |
257 | impl |
^^^^ required by this bound in FlagSet::<T>::value
...
341 |
pub fn value(&self) -> isize { | ----- required by a bound in this associated function
help: consider giving empty_flags an explicit type, where the type for type parameter T is specified |
---|---|---|---|---|---|
3902 | let mut emptyflags: FlagSet<> = flag_set!(); | ||||
++++++++++++ |
error[E0277]: the ? operator can only be used in a function that returns Result or Option (or another type that implements FromResidual )
--> rust/doctests_kernel_generated.rs:4206:83 |
4201 | fn main() { #[allow(non_snake_case)] fn _doctest_main_rust_kernel_net_socket_rs_137_0() { | -------------------------------------------------- this function should return Result or Option to accept ?
...
4206 |
let socket = Socket::new(AddressFamily::Inet, SockType::Datagram, IpProtocol::Udp)?; | ^ cannot use the ? operator in a function that returns () |
---|
= help: the trait `FromResidual<core::result::Result<Infallible, rust_doctest_kernel_build_assert_rs_0::kernel::error::Error>>` is not implemented for `()`
Please note that:
- Review comments that are prefixed with "Style:" are things that you do not necessarily have to fix in order to have the changes in merged into `rust-net`, but that would be required in mainline. They usually don't only apply to the place where I have posted them, it's rather a general pattern that also appears at other places. It would still be nice if you could address those, as long as it is not too much work. It might be a good idea to wait with those until we have sorted out the more high-level issues pointed out by Trevor in order to avoid unnecessary work.
Finally, some questions where I'd be interested in the opinion of you and others:
- Currently nothing in this series depends on any kernel config options besides the top-level `CONFIG_NET`. However, many things do not make much sense without, e.g., TCP/IP or IPV6 support, i.e., `CONFIG_INET` or `CONFIG_IPV6`. I'd like to raise the question if it would make sense to put some modules behind an additional `cfg` gate?
- Related to the above point: Would it make sense to introduce new `CONFIG_*`s that toggle the compilation of these abstractions, e.g., like there is the `CONFIG_RUST_PHYLIB_ABSTRACTIONS` for the `phy` module.
@tgross35 @vobst Thanks for the comments, I'm working on the required changes. I am not sure how to commit the changes tho; should I simply commit and then when the PR is ready rebase everything into some patch-like commits or should I already integrate the changes in the previous commits and force-push? If I'm not mistaken the latter could mess up the reviews in Github.
Feel free to either keep pushing commits or squash and force push however you prefer. GH preserves comments across revisions so no concerns :)
Add Rust abstractions for basic network entities, socket and its wrappers (
TcpStream
andUdpSocket
).Specifically, it was added:
in_addr
,in6_addr
,sockaddr_in
,sockaddr_in6
,sockaddr_storage
).Currently, there is no explicit user for these abstractions.
The original patch series was sent as a RFC to the mailing list. The mailing list patches contained some questions about some portions of the code; if it's relevant, I will promptly rewrite them in this PR as well.
I'm not sure how the
rust-net
workflow will be, but I'm super available and happy to discuss changes to the code and improve/correct it. By the way, I am very glad this branch become a thing and hopefully Rust will be able to become more and more used in the net subsystem!