Message-ID: <20240126105055.2200dc36@kernel.org>
Date: Fri, 26 Jan 2024 10:50:55 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Donald Hunter <donald.hunter@...il.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Jonathan
 Corbet <corbet@....net>, linux-doc@...r.kernel.org, Jacob Keller
 <jacob.e.keller@...el.com>, Breno Leitao <leitao@...ian.org>, Jiri Pirko
 <jiri@...nulli.us>, Alessandro Marcolini <alessandromarcolini99@...il.com>,
 donald.hunter@...hat.com
Subject: Re: [PATCH net-next v1 02/12] tools/net/ynl: Support sub-messages
 in nested attribute spaces

On Fri, 26 Jan 2024 12:44:57 +0000 Donald Hunter wrote:
> I was quite pleased with how simple the patch turned out to be when I
> used ChainMap, but it does have this weakness.

It is very neat, no question about it :(

> In practice, the only place this could be a problem is with
> tc-act-attrs which has the same attribute name 'kind' in the nest and
> in tc-attrs at the top level. If you send a create message with ynl,
> you could omit the 'kind' attr in the 'act' nest and ynl would
> incorrectly resolve to the top level 'kind'. The kernel would reject
> the action with a missing 'kind' but the rest of payload would be
> encoded wrongly and/or could break ynl.

We can detect the problem post-fact and throw an exception. I primarily
care about removing the ambiguity.

Is it possible to check at which "level" of the chainmap the key was
found? If so we can also construct a 'chainmap of attr sets' and make
sure that the key level == attr set level. I.e. that we got a hit at
the first level which declares a key of that name.

More crude option - we could construct a list of dicts (the levels
within the chainmap) and keys they can't contain. Once we got a hit 
for a sub-message key at level A, all dicts currently on top of A
are not allowed to add that key. Once we're done with the message we
scan thru the list and make sure the keys haven't appeared?

Another random thought, should we mark the keys which can "descend"
somehow? IDK, put a ~ in front?

	selector: ~kind

or some other char?

> My initial thought is that this might be better handled as input
> validation, e.g. adding 'required: true' to the spec for 'act/kind'.
> After using ynl for a while, I think it would help to specify required
> attributes for messages, nests and sub-messsages. It's very hard to
> discover the required attributes for families that don't provide
> extack responses for errors.

Hah, required attrs. I have been sitting on patches for the kernel for
over a year - https://github.com/kuba-moo/linux/tree/req-args
Not sure if they actually work but for the kernel I was curious if it's
possible to do the validation in constant time (in relation to the
policy size, i.e. without scanning the entire policy at the end to
confirm that all required attrs are present). And that's what I came up
with.

I haven't posted it because I was a tiny bit worried that required args
will cause bugs (people forgetting to null check attrs) and may cause
uAPI breakage down the line (we should clearly state that "required"
status is just advisory, and can go away in future kernel release).
But that was more of a on-the-fence situation. If you find them useful
feel free to move forward!

I do think that's a separate story, tho. For sub-message selector
- isn't the key _implicitly_ required, in the first attr set where 
it is defined? Conversely if the sub-message isn't present the key
isn't required any more either?

Powered by blists - more mailing lists