Reorganize the code for detail show#10
Conversation
Summary of ChangesHello @cathay4t, 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 focuses on enhancing the internal structure and maintainability of the network interface detail display functionality. By extracting interface-specific logic into new, dedicated modules, the changes improve code organization and prepare the codebase for easier expansion and maintenance of different network interface types. The core detail handling has been streamlined to leverage these new modular components. 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
|
Signed-off-by: Gris Ge <fge@redhat.com>
![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 introduces a significant and well-executed refactoring of the code for displaying link details. By moving interface-specific logic into a new ifaces module, the code is now much cleaner and easier to maintain. I've found a few issues in the new bridge.rs file, mainly related to data representation and formatting, including one that causes incorrect output for bridge IDs compared to iproute2.
| /// Format bridge ID to match iproute2's format: | ||
| /// Priority is 4 hex digits, MAC address bytes use minimal formatting (no | ||
| /// leading zeros for bytes < 0x10) | ||
| fn format_bridge_id(priority: u16, mac_bytes: [u8; 6]) -> String { | ||
| format!( | ||
| "{:04x}.{:x}:{:x}:{:x}:{:x}:{:x}:{:x}", | ||
| priority, | ||
| mac_bytes[0], | ||
| mac_bytes[1], | ||
| mac_bytes[2], | ||
| mac_bytes[3], | ||
| mac_bytes[4], | ||
| mac_bytes[5] | ||
| ) | ||
| } |
There was a problem hiding this comment.
The format_bridge_id implementation and its documentation do not match the output of iproute2. iproute2 formats the bridge ID as priority.mac_address without colons in the MAC and with zero-padding for each byte (e.g., 8000.001122334455). The current implementation includes colons and does not pad all MAC address bytes, leading to incorrect output.
The documentation is also misleading regarding the formatting.
/// Format bridge ID to match iproute2's format: `8000.001122334455` where
/// `8000` is the priority and `001122334455` is the MAC address.
fn format_bridge_id(priority: u16, mac_bytes: [u8; 6]) -> String {
format!(
"{:04x}.{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}",
priority,
mac_bytes[0],
mac_bytes[1],
mac_bytes[2],
mac_bytes[3],
mac_bytes[4],
mac_bytes[5]
)
}| vlan_stats_enabled: Option<u8>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| vlan_stats_per_port: Option<u8>, | ||
| group_fwd_mask: String, |
There was a problem hiding this comment.
The group_fwd_mask field is stored as a String and then parsed back to a number in the Display implementation. This is inefficient. It would be better to store it as a u16 and format it directly when displaying.
This would involve:
- Changing the field type to
u16. - Updating the
fromimplementation to assign theu16value directly. - Updating the
Displayimplementation to format theu16value.
| group_fwd_mask: String, | |
| group_fwd_mask: u16, |
| group_fwd_mask: String, | ||
| group_fwd_mask_str: String, |
There was a problem hiding this comment.
The fields group_fwd_mask and group_fwd_mask_str are redundant and lead to confusing output. The Display implementation prints both a decimal and a hex representation with labels that are not user-friendly (group_fwd_mask_str).
To simplify and fix the output, I suggest storing only the raw u16 value and formatting it as hex for display. This will also improve the JSON output by serializing it as a number.
You would need to:
- Replace both fields with a single
group_fwd_mask: u16. - Update the
fromimplementation to assign theu16value. - Update the
Displayimplementation to print only onegroup_fwd_maskformatted as hex (e.g.,group_fwd_mask {:#x}).
| group_fwd_mask: String, | |
| group_fwd_mask_str: String, | |
| group_fwd_mask: u16, |
1 participant
No description provided.