Message-ID: <CAD4GDZw=CoHXbTn_AR1h2YUnn92K_JVj+ACAKH670PWRrJ+_pA@mail.gmail.com>
Date: Wed, 26 Jul 2023 23:01:12 +0100
From: Donald Hunter <donald.hunter@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, donald.hunter@...hat.com
Subject: Re: [PATCH net-next v1 2/3] tools/net/ynl: Add support for
 netlink-raw families

On Wed, 26 Jul 2023 at 22:37, Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue, 25 Jul 2023 17:22:04 +0100 Donald Hunter wrote:
> > Refactor the ynl code to encapsulate protocol-family specifics into
> > NetlinkProtocolFamily and GenlProtocolFamily.
>
> > +class SpecMcastGroup(SpecElement):
> > +    """Netlink Multicast Group
> > +
> > +    Information about a multicast group.
> > +
> > +    Attributes:
> > +        id        numerical id of this multicast group for netlink-raw
> > +        yaml      raw spec as loaded from the spec file
> > +    """
> > +    def __init__(self, family, yaml):
> > +        super().__init__(family, yaml)
> > +        self.id = self.yaml.get('id')
>
> name, too?

Ack.

> > +        mcgs = self.yaml.get('mcast-groups')
> > +        if mcgs:
> > +            for elem in mcgs['list']:
> > +                mcg = self.new_mcast_group(elem)
> > +                self.mcast_groups[elem['name']] = mcg
>
> Could you factor out the mcgroup changes to a separate patch?

Will do.

> > diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> > index 3e2ade2194cd..7e877c43e10f 100644
> > --- a/tools/net/ynl/lib/ynl.py
> > +++ b/tools/net/ynl/lib/ynl.py
> > @@ -25,6 +25,7 @@ class Netlink:
> >      NETLINK_ADD_MEMBERSHIP = 1
> >      NETLINK_CAP_ACK = 10
> >      NETLINK_EXT_ACK = 11
> > +    NETLINK_GET_STRICT_CHK = 12
> >
> >      # Netlink message
> >      NLMSG_ERROR = 2
> > @@ -153,6 +154,21 @@ class NlAttr:
> >              value[m.name] = decoded
> >          return value
> >
> > +    @classmethod
> > +    def decode_enum(cls, raw, attr_spec, consts):
> > +        enum = consts[attr_spec['enum']]
> > +        if 'enum-as-flags' in attr_spec and attr_spec['enum-as-flags']:
> > +            i = 0
> > +            value = set()
> > +            while raw:
> > +                if raw & 1:
> > +                    value.add(enum.entries_by_val[i].name)
> > +                raw >>= 1
> > +                i += 1
> > +        else:
> > +            value = enum.entries_by_val[raw].name
> > +        return value
>
> This doesn't always operates on netlink attributes, technically,
> so how about we make it a standalone function, not a member of NlAttr?
> Or should we move it to a parent class, NetlinkProtocolFamily?

Fair point. I'll maybe go for standalone just now but will think on
it some more first.

>
> > +    def decode_fixed_header(self, consts, op):
> > +        fixed_header_members = consts[op.fixed_header].members
> > +        self.fixed_header_attrs = dict()
> > +        offset = 0
> > +        for m in fixed_header_members:
> > +            format = NlAttr.get_format(m.type, m.byte_order)
> > +            [ value ] = format.unpack_from(self.raw, offset)
> > +            offset += format.size
> > +
> > +            if m.enum:
> > +                value = NlAttr.decode_enum(value, m, consts)
> > +
> > +            self.fixed_header_attrs[m.name] = value
> > +        self.raw = self.raw[offset:]
> > +
> > +    def cmd(self):
> > +        return self.nl_type
>
> And perhaps the pure code moves could be a separate patch for ease
> of review?

Ack.

> >      def __repr__(self):
> >          msg = f"nl_len = {self.nl_len} ({len(self.raw)}) nl_flags = 0x{self.nl_flags:x} nl_type = {self.nl_type}\n"
> >          if self.error:
> > @@ -318,23 +353,21 @@ def _genl_load_families():
> >
> >
> >  class GenlMsg:
> > -    def __init__(self, nl_msg, fixed_header_members=[]):
> > -        self.nl = nl_msg
> > +    def __init__(self, nl_msg, ynl=None):
> > +        self.genl_cmd, self.genl_version, _ = struct.unpack_from("BBH", nl_msg.raw, 0)
> > +        nl_msg.raw = nl_msg.raw[4:]
> >
> > -        self.hdr = nl_msg.raw[0:4]
> > -        offset = 4
> > +        if ynl:
> > +            op = ynl.rsp_by_value[self.genl_cmd]
>
> Took me a while to figure out why ynl gets passed here :S
> I'm not sure what the best structure of inheritance is but
> I think we should at the very least *not* call genl vs raw-nl
> "family".
>
> NetlinkProtocolFamily -> NetlinkProtocol
> GenlProtocolFamily -> GenlProtocol

Yeah, agreed. "Family" is way too overloaded already :-)

> and store them in YnlFamily to self.nlproto or self.protocol
> or some such.

Ack. Just a note that I have been wondering about refactoring this
from "YnlFamily is a Spec" to "Ynl has n Specs" so that we could do
multi spec notification handling. If we did this, then passing a
SpecContext around would look more natural maybe.

> > +            if op.fixed_header:
> > +                nl_msg.decode_fixed_header(ynl.consts, op)
>
> >      def _decode_binary(self, attr, attr_spec):
> >          if attr_spec.struct_name:
> >              members = self.consts[attr_spec.struct_name]
> > -            decoded = attr.as_struct(members)
> > +            decoded = attr.as_struct(members, self.consts)
>
> I applied the series on top of Arkadiusz's fixes and this line throws
> an "as_struct takes 2 arguments, 3 given" exception.

Ah, my bad. Looks like I missed a fix for that from the patchset.

> >              for m in members:
> >                  if m.enum:
> >                       decoded[m.name] = self._decode_enum(decoded[m.name], m)
>

Powered by blists - more mailing lists