Skip to content

Friendly messages discard type information? #294

@twpayne

Description

@twpayne

Firstly, thank you for this library!

I might be holding something wrong here, but it seems to me that the conversion of netlink.Message to the friendly LinkMessage, AddressMessage, etc. here:

rtnetlink/conn.go

Lines 174 to 193 in 61f4d31

var m Message
switch nm.Header.Type {
case unix.RTM_GETLINK, unix.RTM_NEWLINK, unix.RTM_DELLINK:
m = &LinkMessage{filtered: (nm.Header.Flags&netlink.DumpFiltered != 0)}
case unix.RTM_GETADDR, unix.RTM_NEWADDR, unix.RTM_DELADDR:
m = &AddressMessage{}
case unix.RTM_GETROUTE, unix.RTM_NEWROUTE, unix.RTM_DELROUTE:
m = &RouteMessage{}
case unix.RTM_GETNEIGH, unix.RTM_NEWNEIGH, unix.RTM_DELNEIGH:
m = &NeighMessage{}
case unix.RTM_GETRULE, unix.RTM_NEWRULE, unix.RTM_DELRULE:
m = &RuleMessage{}
default:
continue
}
if err := (m).UnmarshalBinary(nm.Data); err != nil {
return nil, err
}
lmsgs = append(lmsgs, m)

discards the information whether the message is an RTM_GET*, RTM_NEW*, or RTM_DEL* message. The only use of nm.Header.Type is determining whether the message is an RTM_*LINK, RTM_*ADDR, etc., and the call to UnmarshalBinary only receives nm.Data, which is not sufficient to determine whether it is a RTM_GET*, RTM_NEW*, or RTM_DEL* message.

Is this understanding correct?

If so, would you accept a PR that adds this information to LinkMessage, AddressMessage, etc.?

One possible design would be to add an Action field (using the word "action" to avoid confusion and conflict with Type):

type Action int

const (
    ActionGet Action = iota
    ActionNew
    ActionDel
)

type LinkMessage struct {
    Action Action
    // ...rest of LinkMessage struct is unchanged
}

and then populate the Action field in the unpackMessages function in conn.go.

Another possible design would be to have separate NewLinkMessage and DelLinkMessage types that embed a LinkMessage, and then allow a type switch to determine the action. A plain LinkMessage would represent a RTM_GETLINK.

A third option would be to include the netlink message type as a field in LinkMessage, but this would leak lower-level details.

Thoughts?

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions