Jakub Kicinski [off-list ref] writes:

On Thu, 17 Aug 2023 10:10:35 +0100 Donald Hunter wrote:

quoted

quoted

Looks good, but do we also need some extra plumbing to decode extack
for classic netlink correctly?  Basically shouldn't _decode_extack()
also move to proto? Or we can parameterize it? All we really need there
is to teach it how much of fixed headers parser needs to skip to get to
attributes, really (which, BTW is already kinda buggy for genl families
with fixed headers).  

I have been working on the assumption that extack responses don't
include any fixed headers. I have seen extack messages decoded correctly
for classic netlink, here with RTM_NEWROUTE:

lib.ynl.NlError: Netlink error: Invalid argument
nl_len = 80 (64) nl_flags = 0x300 nl_type = 2
  error: -22  extack: {'msg': 'Invalid prefix for given prefix length'}

Is there something I am missing?

I'm thinking of extack messages carrying offsets in addition to the 
textual error message. NLMSGERR_ATTR_OFFS or NLMSGERR_ATTR_MISS_NEST.

In that case ynl will try to re-parse its own message via
_decode_extack_path() to resolve from the offset to what attribute
was there. See the commit message on a552bfa16:

    lib.ynl.NlError: Netlink error: Numerical result out of range
    nl_len = 108 (92) nl_flags = 0x300 nl_type = 2
            error: -34      extack: {'msg': 'integer out of range',...
                                     'bad-attr': '.ifindex'}

I mean the "bad-attr" thing.

I think it works out of sheer luck here, we happen to skip over 
the fixed header because it looks like a 0-length attribute?

You're right, sheer luck, and maybe only for some values of dp-ifindex.
When I tried to reproduce your test in commit a552bfa16, with a value of
dp-ifindex = 5, then ynl goes into an infinite loop trying to read a
zero length nlattr.

As you say, I'll need to rework the extack handling to account for fixed
headers. At a minimum _decode_extack will need to use nlproto.decode()
and needs to learn to skip the fixed header.

Apologies for being slow to catch up with you on this. Failing to grok
that _decode_extack is decoding the request, not the response.