Allow access to Ack messages containing data

[mraerino]

When implementing the W1 Netlink adapter (uses NETLINK_CONNECTOR) i ran into this issue:

Responses to requests are send with the message_type set to 0x3. Therefore as responses arrive, the NetlinkPayload enum will be set to Done and any payload attached to the message is thrown away.

Do you think there is a good way to model this behaviour in the library?
I think i could also make a custom codec and re-implement NetlinkMessage in a different way, but i'd rather solved this upstream if possible.

My (very rough) code is here: https://github.com/kalkspace/w1-netlink-rs/blob/main/examples/lowlevel.rs

This is one of the payloads coming back in that example code:

[
    // netlink header
    0x34, 0x00, 0x00, 0x00, // len
    0x03, 0x00, // type: Done
    0x00, 0x00, // flags
    0x00, 0x00, 0x00, 0x00, // seq
    0x00, 0x00, 0x00, 0x00, // pid

    // netlink connector header
    0x03, 0x00, 0x00, 0x00, // idx
    0x01, 0x00, 0x00, 0x00, // val
    0x00, 0x00, 0x00, 0x00, // seq
    0x01, 0x00, 0x00, 0x00, // ack
    0x10, 0x00, // len (excl header)
    0x61, 0x38, // flags

    // w1 message header
    0x06, // type: W1_LIST_MASTERS
    0x00, // status: no error
    0x04, 0x00, // len of data
    0x30, 0x20, 0x2D, 0x2D, 0x2D, 0x70, 0x20, 0x30, // device id
    // data
    0x01, 0x00, 0x00, 0x00, // bus id
]

[mraerino]

@little-dude any guidance here?

[little-dude]

Hello @mraerino :)

To be honest I don't know, really. I didn't expect these messages to have a payload.

Option 1

Make the payload of Done messages a Vec<u8>:

#[derive(Debug, PartialEq, Eq, Clone)]
pub enum NetlinkPayload<I> {
    Done(Vec<u8>),
    Error(ErrorMessage),
    Ack(AckMessage),
    Noop,
    Overrun(Vec<u8>),
    InnerMessage(I),
}

But then users have to do the parsing themself. Not ideal

Option 2

We could make the messages generic over the payload of the Done message:

#[derive(Debug, PartialEq, Eq, Clone)]
pub enum NetlinkPayload<I, D> {
    Done(D),
    Error(ErrorMessage),
    Ack(AckMessage),
    Noop,
    Overrun(Vec<u8>),
    InnerMessage(I),
}

This is a very drastic API change though...

Option 3

Have a generic payload for Done messages, but make it the same than the InnerMessage payload

#[derive(Debug, PartialEq, Eq, Clone)]
pub enum NetlinkPayload<I> {
    Done(I),
    Error(ErrorMessage),
    Ack(AckMessage),
    Noop,
    Overrun(Vec<u8>),
    InnerMessage(I),
}

This means that the inner message payload need to have an additional variant though. For instance for RouteMessage in netlink-packet-route:

#[derive(Debug, PartialEq, Eq, Clone)]
pub enum RtnlMessage {
    Done, // <- new variant
    NewLink(LinkMessage),
    DelLink(LinkMessage),
    GetLink(LinkMessage),
    SetLink(LinkMessage),
    NewLinkProp(LinkMessage),
    DelLinkProp(LinkMessage),
    NewAddress(AddressMessage),
    // ...
}

But this doesn't feel right at all...

Perhaps as a first step I'd prefer to go with the least invasive option, which is to parse the payload as a Vec<u8>.

---

Another change we'll need, is for netlink-proto to forward the Done message, because currently we're not doing that:

netlink/netlink-proto/src/connection.rs

Line 233 in bda893b

Noop | Done | Ack(_) => {

But this should not be too much of a problem.

[stbuehler]

How about option 2 with a default for D that ignores all data, i.e. a #[derive(Debug, PartialEq, Eq, Clone)] struct IgnoreDropData; and pub enum NetlinkPayload<I, D=IgnoreDropData> {...}? I think (or at least hope) that wouldn't even break the API.

[mraerino]

good idea. i have it working with a breaking change, but your suggestion seems sensible

[stbuehler]

Actually, modifying the Done variant still will break the API, but with a default for the type parameter it should be easier to migrate - basically Done(_) in patterns and Done(Default::default()) when creating the variant. And my own code doesn't seem to use the variant anyway, so maybe most users won't need to change anything.

I don't like Option 3 (although it would have a similar "get it to compile" pattern), because it will produce DecodeError errors instead of messages if parsing the data fails - as it probably will most of the time given there won't be any data but the (inner) message expects some.