Re: [PATCH net-next v2 05/10] tools/net/ynl: Refactor decode_fixed_header into NlMsg

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 18 Aug 2023 at 02:37, Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> On Thu, 17 Aug 2023 16:14:35 +0100 Donald Hunter wrote:
> > > It's a bit of a layering violation that we are futzing with the raw
> > > member of NlMsg inside GenlMsg, no?
> > >
> > > Should we add "fixed hdrs len" argument to NlMsg? Either directly or
> > > pass ynl and let get the expected len from ynl? That way NlMsg can
> > > split itself into hdr, userhdrs and attrs without GenlMsg "fixing it
> > > up"?
> >
> > I agree, it breaks the layering. The issue is that GenlMsg gets created at
> > some point after NlMsg, only when we know the nl_msg is suitable for
> > decoding. The fixed header bit is quite well encapsulated in NlMsg,
> > it's the genl header that needs pulled out and NlMsg shouldn't know
> > anything about it. How about I add a take_bytes(length) method or a
> > generic decode_subheader(format, length) method to NlMsg?
>
> Why do we need to fix up the .raw of NlMsg underlying the GenlMsg
> in the first place? GenlMsg by itself didn't need to do that until now.

Fair point. I will refactor to leave nl_msg.raw untouched.

> Another option to consider which would make things more symmetric
> between raw and genetlink would be to add a wrapper class for old
> families, too. ClassicMsg? CnlMsg? That way we could retain the
> separation of NlMsg is just a raw message which could be a NLM_DONE or
> some other control thing, and higher level class being used to pull
> fixed headers and separate out attrs. Just a thought, not sure it helps.

I _think_ I can avoid doing this. There's an asymmetry to the way
the NlAttrs get created that I need to fix. When I do that, the rest
should be a bit cleaner.




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux