Add support for parsing some FreeBSD specific attributes - #221
Add support for parsing some FreeBSD specific attributes#221bigsaltyfishes wants to merge 16 commits into
Conversation
Summary of ChangesHello @bigsaltyfishes, 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 significantly enhances the Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request adds support for parsing FreeBSD-specific netlink attributes for network addresses and routes. The changes are well-structured, using conditional compilation to isolate the FreeBSD-specific logic. I've identified one critical issue that would break compilation on non-FreeBSD platforms, and a few medium-severity issues related to code style and potential compiler warnings. Addressing these points will improve the robustness and clarity of the code.
a1ddf7d to
a6f5446
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #221 +/- ##
==========================================
- Coverage 68.10% 67.92% -0.18%
==========================================
Files 144 140 -4
Lines 10103 9959 -144
==========================================
- Hits 6881 6765 -116
+ Misses 3222 3194 -28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cathay4t
left a comment
There was a problem hiding this comment.
- Rename the
FreeBSDto more specific name. - The
Nla::kindis wrong. - Do not use magic number. Hard to maintain and verify in BSD kernel code.
- Unit test is mandatory. Google says
tcpdump -i lo0 -n 'netlink'can capture netlink message in FreeBSD. Without emit and parse test, I cannot tell whether you are doing correct or not. - Please include short commit message indicate which FreeBSD kernel file is handling these attribute so developer could review.
|
|
||
| #[non_exhaustive] | ||
| #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] | ||
| pub enum FreeBSD { |
There was a problem hiding this comment.
How about FreeBsdAddressAttribute to be more specific?
| let value = parse_u32(buf.value()) | ||
| .context("failed to parse IFA_FREEBSD attribute value")?; | ||
| match buf.value_type() { | ||
| 1 => Ok(FreeBSD::Vhid(value)), |
There was a problem hiding this comment.
Please change magic number to constant.
|
|
||
| impl Nla for FreeBSD { | ||
| fn kind(&self) -> u16 { | ||
| 11 |
There was a problem hiding this comment.
The kind here is wrong.
It should be kind of VhId, IfaFlags and etc.
| #[non_exhaustive] | ||
| #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] | ||
| pub enum FreeBSD { | ||
| Unspecified, |
There was a problem hiding this comment.
Remove this unless freebsd code is using this unspec in real environment instead of place holder.
| #[non_exhaustive] | ||
| #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] | ||
| pub enum FreeBSD { | ||
| Unspecified, |
There was a problem hiding this comment.
Remove unspec if it is a place holder.
| cap_bit_size: u32, | ||
| supported_caps: (IfCapFlags, IfCap2Flags), | ||
| active_caps: (IfCapFlags, IfCap2Flags), | ||
| }, |
There was a problem hiding this comment.
Add Other(DefaultNla) for future FreeBSD kernel.
| IfCaps { | ||
| cap_bit_size: u32, | ||
| supported_caps: (IfCapFlags, IfCap2Flags), | ||
| active_caps: (IfCapFlags, IfCap2Flags), |
| } | ||
|
|
||
| match buf.value_type() { | ||
| 2 => { |
There was a problem hiding this comment.
Use constant instead of magic number.
|
|
||
| impl Nla for FreeBSD { | ||
| fn kind(&self) -> u16 { | ||
| 64 |
There was a problem hiding this comment.
It should be kind of FreeBSD::IfCaps and etc.
6066615 to
7119fa5
Compare
|
I have modified and added unit tests. Because some constant and structure definitions in FreeBSD differ significantly from those in Linux, I have added the missing constants and different structures. |
b71a221 to
26328c8
Compare
|
@bigsaltyfishes Could you include an example code into |
|
@bigsaltyfishes any comments? |
|
Added Examples and rebased, need to be tested on Linux. |
|
@cathay4t Examples added, since rust's libc does not support netlink on FreeBSD, I added a ffi for examples. |
|
Great. Let me install a FreeBSD and try it out. |
|
@bigsaltyfishes I got failure when running this command in FreeBSD 15: Error is: ::: examples/freebsd/mod.rs:13:1
|
13 | pub struct NetlinkSocket(OwnedFd);
| ------------------------ method `write_all` not found for this struct
--> examples/dump_packet_links.rs:105:12
|
105 | socket.write_all(&buf[..]).unwrap();
| ^^^^^^^^^
|
= help: items from traits can only be used if the trait is in scope
help: trait `Write` which provides `write_all` is implemented but not in scope; perhaps you want to import it
|
3 + use std::io::Write;
|
help: there is a method `write` with a similar name
|
105 - socket.write_all(&buf[..]).unwrap();
105 + socket.write(&buf[..]).unwrap();
|
error[E0599]: no method named `read` found for struct `NetlinkSocket` in the current scope
--> examples/dump_packet_links.rs:113:27
|
113 | let size = socket.read(&mut &mut receive_buffer[..]).unwrap();
| ^^^^ method not found in `NetlinkSocket`
|
::: /usr/local/lib/rustlib/src/rust/library/std/src/io/mod.rs:813:8
|
813 | fn read(&mut self, buf: &mut [u8]) -> Result<usize>;
| ---- the method is available for `NetlinkSocket` here
|
::: examples/freebsd/mod.rs:13:1
|
13 | pub struct NetlinkSocket(OwnedFd);
| ------------------------ method `read` not found for this struct
|
= help: items from traits can only be used if the trait is in scope
help: trait `Read` which provides `read` is implemented but not in scope; perhaps you want to import it
|
3 + use std::io::Read; |
Due to the difference of contant and support features, some tests may failed. Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
- flags from: https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/net/if.h#L139-L173 - layer type from: https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/net/if_types.h#L44-L258 (ARPHRD_NETROM is not defined in kernel, but as metioned in https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/tests/sys/netlink/test_rtnl_iface.py#L307-L354, just defined it in layer type like Linux.) - info kinds comes from drivers, attach to list via `ifc_attach_cloner`, for wireless device is "wlan", and for loopback is "lo". (Related: https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/net80211/ieee80211_freebsd.c#L1397, https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/net/if_loop.c#L150, https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netlink/route/iface.c#L364) Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
- `IFA_FREEBSD` and its nested value type are defined at https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netlink/route/ifaddrs.h#L63-L73, handled at https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netlink/route/iface.c#L1007-L1023 - `IfaFlags` from: https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netinet6/in6_var.h#L490-L503 Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
They are FreeBSD specific flags - All of them are defined at https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netlink/route/route.h#L145-L181, handled at https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netlink/route/rt.c#L220-L260 - `RtFlags` from: https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/net/route.h#L166-L197 Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
As defined at https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netlink/route/interface.h#L209-L235, there are no `rx_otherhost_dropped` filed in `Stats64`. Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
- `IFLA_FREEBSD` and its nested value types are defined at https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netlink/route/interface.h#L145-L156, handled at https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netlink/route/iface.c#L350-L357 Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
- `NDA_FREEBSD` and its nested value type are defined at https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netlink/route/neigh.h#L66-L75, handled at https://github.com/freebsd/freebsd-src/blob/release/15.0.0-p1/sys/netlink/route/neigh.c#L193-L198 Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
Add missing kinds and disable Linux-specific ones for FreeBSD. Currently, only Vlan kind supports `IFLA_INFO_DATA`. Reference: https://github.com/freebsd/freebsd-src/blob/main/sys/net/if_vlan.c#L1213-L1296 Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
* Unsupported methods: https://github.com/freebsd/freebsd-src/blob/release/15.0.0/sys/netlink/route/common.h#L50-L86 Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
Signed-off-by: bigsaltyfishes <bigsaltyfishes@gmail.com>
Signed-off-by: bigsaltyfishes <bigsaltyfishes@gmail.com>
…FI example Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
…tests - Some test data is not supported on FreeBSD, since the tests consistently fail on this platform, just disable them for FreeBSD. Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
Signed-off-by: molyuu <bigsaltyfishes@gmail.com>
|
@cathay4t Okay, my fault during testing on Linux. I've revert changes. Should work now. |
IFA_FREEBSD(Conficts withIFA_PROTOin FreeBSD)RTA_RTFLAGS,RTA_KNH_IDandRTA_WEIGHTIFLA_FREEBSDNDA_FREEBSDSTATS64(Structure is different)LinkLayerTypeInfoKind.