-
Notifications
You must be signed in to change notification settings - Fork 1
nameif: add command implementation #2
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
Conversation
f805bbb to
808e385
Compare
d2b2889 to
cff2270
Compare
lvoytek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, just a few extra comments :)
|
|
||
| /// Parse command-line interface/MAC pairs | ||
| fn parse_cli_pairs(pairs: &[String]) -> Result<Vec<InterfaceChange>> { | ||
| if !pairs.len().is_multiple_of(2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows compilation on stable toolchains
| if !pairs.len().is_multiple_of(2) { | |
| if pairs.len() % 2 != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, no idea why I initially thought of this function 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, actually, turned out that this is what cargo clippy prefers:
https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_multiple_of
a.is_multiple_of(b) is a clearer way to check for divisibility of a by b. This expression can never panic.
src/nameif/mod.rs
Outdated
| // SAFETY: Reading ifru_hwaddr which was just populated by the successful ioctl | ||
| let mac = unsafe { | ||
| let sa_data = &ifreq.ifr_ifru.ifru_hwaddr.sa_data; | ||
| sa_data[..MAC_MAX_LEN].iter().map(|&b| b as u8).collect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return a 6-byte address instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original implementation does copy all 14 bytes, not just 6:
https://github.com/ecki/net-tools/blob/fc3a1338e84f7387b105550499a164921ce286c9/nameif.c#L104
memcpy(mac, ifr.ifr_hwaddr.sa_data, MAC_ADDRESS_MAX_LENGTH);#define MAC_ADDRESS_MAX_LENGTH (sizeof(((struct ifreq *)0)->ifr_hwaddr.sa_data))AFAIU, this macro calculates the size of sa_data field at compile time, which equals 14 bytes
cff2270 to
09532d7
Compare
This PR adds a Rust implementation of the nameif command from net-tools, which renames network interfaces based on MAC addresses
Like the original
nameifcommand, the implemented solution supports reading from the configuration file (-c/--config-fileoption, default/etc/mactab) and from CLI arguments, e.g.nameif testif0 00:11:22:33:44:55The
-s/--syslogoption from the original is not implemented for the sake of the PR size and will be added in a follow-up PRThe PR also adds compliance tests for the
nameifcommand to verify that its behaviour matches the originalnameifcommand one