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.
