netdev - Re: [RFC net-next 2/4] ynl: add the schema for the schemas

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220816085316.65fda789@kernel.org>
Date:   Tue, 16 Aug 2022 08:53:16 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com, sdf@...gle.com, jacob.e.keller@...el.com,
        vadfed@...com, jiri@...nulli.us, dsahern@...nel.org,
        stephen@...workplumber.org, fw@...len.de, linux-doc@...r.kernel.org
Subject: Re: [RFC net-next 2/4] ynl: add the schema for the schemas

On Tue, 16 Aug 2022 09:21:27 +0200 Johannes Berg wrote:
> On Mon, 2022-08-15 at 17:47 -0700, Jakub Kicinski wrote:
> > On Mon, 15 Aug 2022 22:09:11 +0200 Johannes Berg wrote:  
> > It's the incredibly inventive nesting format used in genetlink policy
> > dumps where the type of the sub-attr(s there are actually two levels)
> > carry a value (index of the policy and attribute) rather than denoting
> > a type :S :S :S  
> 
> Hmm, OK, in the policy dump (not specific to genetlink, btw, can be used
> for any policy, but is only generically hooked up for genetlink), we
> have
> 
> [policy_idx] = {
>   [attr_idx] = {
>     [NL_POLICY_TYPE_ATTR_...] = ...
>   }
> }
> 
> Is that what you mean?

Yes.

> I guess I never really thought about this format much from a description
> POV, no need to have a policy since you simply iterate (for_each_attr)
> when reading it, and don't really need to care about the attribute
> index, at least.
> 
> For future reference, how would you suggest to have done this instead?

My guess was that some of the wrapping was for ease of canceling here
(cancel is used both on skip and on error). What I think we should push
for is multi-attr, so the same attribute happens multiple times.

[msg]
 [ATTR1]
 [ATTR2] // elem 1
   [SubATTR1]
   [SubATTR2]
 [ATTR2] // elem 2
   [SubATTR1]
   [SubATTR2]
 [ATTR2] // elem 3
   [SubATTR1]
   [SubATTR2]
 [ATTR3]
 [ATTR4]

Instead of wrapping into an array and then elements.

As Michal pointed out a number of times - the wrapping ends up limiting 
the size of the array to U16_MAX, and I have a suspicion that most of
wrapping is done because we tend to parse into a pointer array, which
makes multi-attr a little tricky. But we shouldn't let one parsing
technique in a relatively uncommon language like C dictate the format :)

> > Slightly guessing but I think I know what you mean -> the value of the
> > array is a nest with index as the type and then inside that is the
> > entry of the array with its attributes <- and that's where the space is
> > applied, not at the first nest level?  
> 
> Right.
> 
> > Right, I should probably put that in the docs rather than the schema,
> > array-nests are expected to strip one layer of nesting and put the
> > value taken from the type (:D) into an @idx member of the struct
> > representing the values of the array. Or at least that's what I do in
> > the C codegen.  
> 
> Well mostly you're not supposed to care about the 'value'/'type', I
> guess?

Fair, I wasn't sure if it's ever used, but I figured if I have to parse
it out why not save the values. Same for array-next indexes.

I'm leaning heavily towards defining a subset of the YAML spec as 
"the way to do things in new family" which will allow only one form 
of arrays.