Conversation
… commands and the documentation here https://docs.kernel.org/networking/ethtool-netlink.html#linkstate-get tested with cargo run --example dump_link_state
Summary of ChangesHello @laundrei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for get_link_state, following the existing patterns in the crate. A new link_state module is introduced, along with an example to demonstrate its usage. The changes are well-structured. I've identified a few areas for improvement related to code clarity, efficiency, and robustness, and have provided suggestions to address them.
| fn emit_value(&self, buffer: &mut [u8]) { | ||
| match self { | ||
| Self::Header(ref nlas) => nlas.as_slice().emit(buffer), | ||
| Self::Other(ref attr) => attr.emit(buffer), | ||
| _ => todo!("Does not support changing link state"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Using todo! here will cause a panic if any code attempts to serialize EthtoolLinkStateAttr variants other than Header or Other. While this is not the case for get_link_state, it could be an unexpected trap for future developers using this crate, especially if set functionality is added. It would be better to use unimplemented! which is more idiomatic for this situation. This makes the intent clearer that this path is not yet supported for emission.
| fn emit_value(&self, buffer: &mut [u8]) { | |
| match self { | |
| Self::Header(ref nlas) => nlas.as_slice().emit(buffer), | |
| Self::Other(ref attr) => attr.emit(buffer), | |
| _ => todo!("Does not support changing link state"), | |
| } | |
| } | |
| fn emit_value(&self, buffer: &mut [u8]) { | |
| match self { | |
| Self::Header(ref nlas) => nlas.as_slice().emit(buffer), | |
| Self::Other(ref attr) => attr.emit(buffer), | |
| _ => unimplemented!("Does not support changing link state"), | |
| } | |
| } |
| for nla in NlasIterator::new(buffer) { | ||
| let error_msg = format!( | ||
| "Failed to parse ethtool link state message attribute {nla:?}" | ||
| ); | ||
| let nla = &nla.context(error_msg.clone())?; | ||
| let parsed = EthtoolLinkStateAttr::parse(nla).context(error_msg)?; | ||
| nlas.push(EthtoolAttr::LinkState(parsed)); | ||
| } |
There was a problem hiding this comment.
The current implementation for parsing NLAs is a bit inefficient and the error message can be confusing:
- It calls
format!andclone()on each iteration, which is not performant. - The
nlavariable informat!is aResult, so the error message will containOk(...)which is not ideal.
A cleaner and more performant approach would be to handle the Result from the iterator first, and then parse the NlaBuffer. This avoids unnecessary allocations and provides clearer error messages.
for nla in NlasIterator::new(buffer) {
let nla = nla.context("Failed to parse link state NLA header")?;
let parsed = EthtoolLinkStateAttr::parse(&nla)
.context(format!("Failed to parse link state NLA: {nla:?}"))?;
nlas.push(EthtoolAttr::LinkState(parsed));
}
src/message.rs
Outdated
| let nlas = match iface_name { | ||
| Some(s) => { | ||
| vec![EthtoolAttr::LinkState(EthtoolLinkStateAttr::Header( | ||
| vec![EthtoolHeader::DevName(s.to_string())], | ||
| ))] | ||
| } | ||
| None => { | ||
| vec![EthtoolAttr::LinkState(EthtoolLinkStateAttr::Header( | ||
| vec![], | ||
| ))] | ||
| } | ||
| }; |
There was a problem hiding this comment.
The match statement contains duplicated code. This can be simplified to be more concise and easier to maintain by handling the Option more idiomatically.
let nlas = vec![EthtoolAttr::LinkState(EthtoolLinkStateAttr::Header(
iface_name
.map(|s| vec![EthtoolHeader::DevName(s.to_string())])
.unwrap_or_default(),
))];…L_FLAG_STATS to get additional stats from LINKSTATE_GET
1 participant
followed the example of the other get commands and the documentation here
https://docs.kernel.org/networking/ethtool-netlink.html#linkstate-get
tested with cargo run --example dump_link_state