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?
