Wrapper types policy?

[daniel-noland]

Several types are recurring in networking code in general.

For example,

• IP addresses (v4 and v6 come from std)
• MAC
• Vlan ID
• MPLS label
• TCP / UDP Port number
• ...

Do we have a policy on when to create wrapper types to describe networking constructs?

For example, netlink messages often use u16 to represent VLAN IDs. However, vlan IDs are actually only 12 bits long. My first instinct is to write:

#[derive(Debug, PartialEq, Eq, Clone, Copy, PartialOrd, Ord, Hash)]
#[repr(transparent)]
pub struct VlanId(u16);

impl VlanId {
    pub fn try_new(id: u16) -> Result<Self, Error> {
        if id >= 4096 {
            return Err(anyhow::anyhow!("VLAN ID must be less than 4096"));
        }
        Ok(Self(id))
    }
}

impl From<VlanId> for u16 {
    fn from(val: VlanId) -> Self {
        val.0
    }
}

impl TryFrom<u16> for VlanId {
    type Error = Error;

    fn try_from(id: u16) -> Result<Self, Self::Error> {
        Self::try_new(id)
    }
}

impl AsRef<u16> for VlanId {
    fn as_ref(&self) -> &u16 {
        &self.0
    }
}

This is a bit of boilerplate, but it does make the API a bit clearer and safer IMO.

1. It improves error handling.
2. It attaches "units" to otherwise non-descript u16 values.
3. You can use traits or inherent impls to add methods to the type (e.g. Mac::is_multicast()).

That said, if the purpose of the library is to provide a mapping to the netlink API, then it might be better to just use u16 directly as has already been done elsewhere. After all, a netlink message containing vlan 7000 is a syntactically legal message even if it is semantically nonsensical. Is it the job of this library to provide guard rails against such nonsense messages?

I personally lean towards the use of wrapper types (although they should be implemented in a different crate and used only in the higher level libraries). I looked at the rest of the code base for guidance on this and found that we seem to prefer the raw types unless the type is compound or more complex.

We seem to have consistently used the ip address types from std, but we don't seem to have wrapped types like MAC or Vlan ID into validated and reusable types.

MAC is represented as [u8; 6] in several places.

In link/link_info/vlan.rs we have:

pub enum InfoVlan {
    Id(u16),
    // ...
}

and in link/sriov/vf_vlan.rs we have:

pub struct VfVlanInfo {
    // ...
    pub vlan_id: u32,
    // ...
}

I'm happy with most approaches, but I do think it would be ideal to have guidelines here.

[cathay4t]

@daniel-noland Yes. It would be ideal to have something written as architecture decision for where and how to place shared types.

You may create pull request to https://github.com/rust-netlink/.github containing files like:

https://github.com/rust-netlink/.github/blob/main/architecture_decisions/ADR-000-adr-template.md

My initial through on this would be: please place shared struct at the parent level of all consumers. For example:

• If struct shared within a protocol(e.g. link, address, tc), the struct will be placed at src/link/foo.rs
• If struct shared within route crate among multiple protocols, the struct will be placed at src/foo.rs
• If struct shared among multiple crates of rust-netlink organization, please place it to netlink-packet-core crate.

In summery, a ADR to https://github.com/rust-netlink/.github would have this design/policy recorded.

Ideally, we should have a CI enforcing this policy, but I know it would be very complex to check the similarity of types.