add set_bitrate method #50
Conversation
|
Wow, thanks so much! I've wanted to get more of the nl interface implemented, but haven't had the time to get into it. I have a few questions and comments inline, but either way, as long as this works to set the bitrate, I'll get it merged right away. Also. the things you marked as "missing from libc"... did you PR them to libc to try to get them included there? If not, that's something we should do to get them in their proper place. |
| /// | ||
| #[repr(C)] | ||
| #[allow(non_camel_case_types)] | ||
| #[derive(Default, Clone)] |
There was a problem hiding this comment.
It's nice to derive Debug as well if there are not problems doing so.
There was a problem hiding this comment.
The struct can_bittiming is only used to make some very simple settings, so I don't think it's necessary to derive Debug.
There was a problem hiding this comment.
But when you write a library, it's better not to limit what you think users might need or want to do. Someone learning the library might want to simply dump the data to see what it is, or use the data structures in a larger collection that they want to dump. A single object that doesn't implement debug would prevent a user from simply deriving their own Debug.
| #[allow(non_camel_case_types)] | ||
| #[derive(Default, Clone)] | ||
| pub struct can_bittiming { | ||
| pub(crate) bitrate: u32, /* Bit-rate in bits/second */ |
There was a problem hiding this comment.
These can all just be pub - instead of pub(crate) - like they do in libc.
| /// | ||
| /// Note: Cannot use repr(C) here, as this will not actually make the underlying type a u32 | ||
| /// as it would be in C (which checks which data type is necessary, notices that i32 does not | ||
| /// work in this case, and goes for u32 next). |
There was a problem hiding this comment.
Sorry, I'm confused. What do we want for the underlying type? It looks like we convert to and from u16. So shouldn't we ask for that as the underlying type?
#[repr(u16)]
It probably doesn't matter, though, since we're just using these on the Rust side; as long as we can convert to and from the underlying type. But it's a nice reminder of what the underlying type should be.
| /// work in this case, and goes for u32 next). | ||
| /// | ||
| pub enum IflaCan { | ||
| Unspec = 0, |
There was a problem hiding this comment.
Nit: We probably don't need to specify all the discriminants if they just use the defaults, but I guess it doesn't hurt.
There was a problem hiding this comment.
In order to correspond to the netlink API, it's better to specify it so that it's easier to understand.
| /// as it would be in C (which checks which data type is necessary, notices that i32 does not | ||
| /// work in this case, and goes for u32 next). | ||
| /// | ||
| pub enum IflaCan { |
There was a problem hiding this comment.
For purely numeric enumeratrions like this, it's nice to make them Copy and derive the comparison traits for equality and ordering:
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
|
Actually, I have some time to work on this today, so I'll just merge and clean up those nits... Thanks, again. This is awesome. |
add set_bitrate method using netlink API