Question about the design of `LinkMessage` attributes

[JKearnsl]

Hello! I would like to clarify whether attributes can be repeated for a single interface.

Is the use of Vec<LinkAttribute> related to the fact that attributes can be repeated, or is it done solely to save memory by not using a struct with Option<T>?

netlink-packet-route/src/link/message.rs

Lines 11 to 18 in 420d181

	#[derive(Debug, PartialEq, Eq, Clone, Default)]
	#[non_exhaustive]
	pub struct LinkMessage {
	pub header: LinkHeader,
	pub attributes: Vec<LinkAttribute>,
	}

[cathay4t]

rust-netlink is merely abstract Linux kernel data formats into Rust native types. So I do not have plan on abstract it into a Strcut. I recall AfSpec can be multiple, not 100% sure. I have some idea for indexing of these attributes because I also suffer from searching desired attributes.

[cathay4t]

No. AfSpec is not multiple. In iproute code ip/iplink.c, they have

	if (tb[IFLA_MTU])
		print_int(PRINT_ANY,
			  "mtu", "mtu %u ",
			  rta_getattr_u32(tb[IFLA_MTU]));

This indicate they are indexing the netlink attributes using its kind.

But the kernel never claims whether a attribute can duplicate or not, but treat it array by appending.

So to make our life easier for searching, how about we introduce enum LinkAttributeKind, then user can do:

    let attr_indexed: HashMap<LinkAttributeKind, LinkAttribute> = link_msg
        .attributes
        .drain()
        .map(|attr| (attr.kind_enum(), attr))
        .collect();

    let mtu = attr_indexed.get(&LinkAttributeKind::Mtu).and_then(|attr| {
        if let LinkAttribute::Mtu(d) = attr {
            Some(d)
        } else {
            None
        }
    });

I think creating struct LinkAttributes with mtu: Option<u32> should be considered as another crate instead of netlink-packet-route, maybe rtnetlink is better place where we wrap stuff in seek of easy to use way. Something like:

pub struct Link {
  index: u32,
  if_name: Option<String>,
  mtu: Option<u32>,
}

impl From<LinkMessage> for LinkAttributes {
// do the parse there
}

impl LinkHandle {
    pub fn get_parsed(&self) -> LinkGetParsedRequest {
        // The LinkGetParsedRequest::execute will emit `struct Link` instead of
        // `LinkMessage`.
    }
}

Concerns:

1. Do we have user for this? To create big picture of network status, you need multiple calls to netlink (e.g. link, address, route, bridge AfSpec, etc). People (for me, I create nispor crate) still need to create their own crate to wrap these netlink messages into fancy Rust struct. my use case actually prefer to get precise netlink attribute instead of parsed one.
2. This dramatically increase the effort of rtnetlink crate and will slow us down on the road to release 1.0 with API stable.

So, I refused to create struct with Option<> in netlink-packet-route. And not willing to do so in rtnetlink. Solution to improve user experience is introduce LinkAttributeKind and many other XxxxKind allowing user to index on their own.

[JKearnsl]

Hmm, indeed, if we consider this project as a wrapper, then all sorts of Option<T> and other modifications can confuse/harm (memory)/be unnecessary (the developer himself can parse what he needs)

For example, introducing the same LinkAttributeKind will be a kind of duplicate of LinkAttribute only without a body, which already adds complexity to working with the library ,and as you already said, the developer will in any case parse this into his high-level objects (as me also did 🙂)

Perhaps the best solution would be to leave it as is 🤔

[cathay4t]

Without LinkAttributeKind, even user can index LinkAttribute using LinkAttribute::kind() -> u16 call. It still require user of this index map something like indexed.get(IFLA_MTU) which is against of rust-netlink design (saving people from reading kernel code or C headers).

Let me try a bit, the vec search performance is worse than indexed map. So I am actually considering to improve this in my own project also.

[cathay4t]

But this LinkAttributeKind is not a good design. I am working on doing this Option<T> smart parsing stuff in rtnetlink crate now.

[cathay4t]

I am doing experiment for new crate of wireguard https://github.com/rust-netlink/nl-wireguard

The design is:

impl WireguardHandle {
    pub(crate) fn new(handle: GenetlinkHandle) -> Self {
        WireguardHandle { handle }
    }

    pub async fn get_by_name(
        &mut self,
        iface_name: &str,
    ) -> Result<WireguardParsed, WireguardError> {
        todo!()
    }

    pub async fn set(
        &mut self,
        setting: WireguardParsed,
    ) -> Result<(), WireguardError> {
        todo!()
    }

    /// Sending arbitrary [WireguardMessage] message and manually handle
    /// [WireguardMessage] reply from kernel.
    pub async fn request(
        &mut self,
        nl_header_flags: u16,
        message: WireguardMessage,
    ) -> Result<
        impl Stream<Item = Result<WireguardMessage, WireguardError>>,
        WireguardError,
    > {
    }

The get() and set() are using WireguardParsed struct like:

pub struct WireguardParsed {
    pub iface_name: Option<String>,
    pub iface_index: Option<u16>,
}

impl From<WireguardMessage> for WireguardParsed {
    fn from(msg: WireguardMessage) -> Self {
        todo!()
    }
}

impl WireguardParsed {
    /// Build [WireguardMessage]
    pub fn build(&self, cmd: WireguardCmd) -> WireguardMessage {
    }
}

With this, normal user just use WireguardHandle::get() and WireguardHandle::set() without worry the detail of netlink stuff.
Corner case user can use WireguardHandle::request to play with WireguardMessage directly.

I haven't finished yet, but look promising.

If this works well, I will add rtnetlink::LinkHandle::get_parsed() and change_with_parsed() to do smart wrapping of NetlinkMessages.

Let me know if this is not meet your expectation.

[cathay4t]

During coding, I think we should NOT do this in netlink-packet-route, reasons:

1. We will lose netlink attribute order when parse it into fancy Rust flat struct.
2. The Rust flat struct cannot differentiate for message RTM_GETLINK vs RTM_DELLINK. Try to create new data type to differentiate them only make the design worse than current kernel like layout.

But it fits into rtnetlink crate, because:

1. Save from user do parsing works.
2. Does not need to be lose-free serialize and deserialize between LinkParsed and LinkMessage.
3. This is might be better than current LinkMessage builder work flow.
4. Saving people from handling Stream. Instead of LinkHandle::get() -> impl Stream, it is easier for Rust new learner to use LinkHandle::get_parsed_with_filter(file: FooFilter) -> Result<Vec<LinkParsed>, RtnetlinkError>.

[cathay4t]

@JKearnsl I have finished my work in https://github.com/rust-netlink/nl-wireguard . It is using the new Parsed design.

Any comment on doing the same for LinkMessage in rtnetlink crate?

[JKearnsl]

I looked at the nl-wireguard repository.

Am I correct in understanding that in rtnetlink I'll be able to access the desired field something like this:

let links: Vec<LinkParsed> = netlink_handle.link().get().__somefilters__.parsed().await?;
let mtu: Option<u32> = links[0].mtu;

?

[cathay4t]

Yes. I think I will just expand LinkGetRequest to:

impl LinkGetRequest {
    pub async fn execute_parsed(self) => Result<Vec<LinkParsed>, Error> {
        todo!()
    }
}

With this, we can reuse existing filters of LinkGetRequest.

For user of this parsed rtnetlink, they should not need to play with any netlink attribute or Stream stuff any more.

Or maybe redesign a new set of API as LinkParsedHandler for user-friendly API.

[cathay4t]

Let's move the discussion to rust-netlink/rtnetlink#151