Date:   Fri, 12 Aug 2022 15:48:50 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com, jacob.e.keller@...el.com, vadfed@...com,
        johannes@...solutions.net, jiri@...nulli.us, dsahern@...nel.org,
        stephen@...workplumber.org, fw@...len.de, linux-doc@...r.kernel.org
Subject: Re: [RFC net-next 4/4] ynl: add a sample user for ethtool

On Fri, 12 Aug 2022 09:26:13 -0700 Stanislav Fomichev wrote:
> On Thu, Aug 11, 2022 at 4:31 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > Let me think about it some more. My main motivation is people writing
> > new families, I haven't sent too much time worrying about the existing
> > ones with all their quirks. It's entirely possible that the uAPI quirks
> > can just go and we won't generate uAPI for existing families as it
> > doesn't buy us anything.  
> 
> Ack. Although, we have to have some existing examples for people to
> write new families. So you might still have to convert something :-)

We do seem to have at least a couple on the horizon for 6.1.

Another thought I had yesterday was that maybe we want to have two
schemas - one "full" with all the historical baggage. And one "blessed"
for new families which narrows the options?

> > Still not convinced about messages, as it makes me think that every
> > "space" is then a definition of a message rather than just container
> > for field definitions with independent ID spaces.
> >
> > Attribute-sets sounds good, happy to rename.
> >
> > Another thought I just had was to call it something like "data-types"
> > or "field-types" or "type-spaces". To indicate the split into "data"
> > and "actions"/"operations"?  
> 
> I like attribute-set better than attribute-space :-)

OK!

> > Herm, looking at my commits where I started going with the C codegen
> > (which I haven't posted here) is converting the values to the same
> > format as keys (i.e. YAML/JSON style with dashes). So the codegen does:
> >
> >         c_name = attr['name']
> >         if c_name in c_keywords:
> >                 c_name += '_'
> >         c_name = c_name.replace('-', '_')
> >
> > So the name would be "dev-index", C will make that dev_index, Java will
> > make that devIndex (or whatever) etc.
> >
> > I really don't want people to have to prefix the names because that's
> > creating more work. We can slap a /* header.dev_index */ comment in
> > the generated uAPI, for the grep? Dunno..  
> 
> Yeah, dunno as well, not sure how much of the per-language knowledge
> you should bake into the tool itself..

By "the tool" you mean the codegen? I assumed that's gotta be pretty
language specific. TBH I expected everyone will write their own codegen
or support library. Like I don't think Rust people will want to look at
my nasty Python C generator :S

I'd prefer to keep as much C-ness out of the spec as possible but
some may have to leak in because of the need to gen the uAPI.

> I think my confusion mostly
> comes from the fact that 'name' is mixed together with 'name-prefix'
> and one is 'low_caps' while the other one is 'ALL_CAPS'. Too much
> magic :-)

Fair point, at the very least they should be consistent, I'll fix.

> Thinking out loud: maybe these transformations should all go via some
> extra/separate yaml (or separate sections)? Like:
> 
> fixup-attribute-sets:
>   - name: header
>     canonical-c-name: "ETHTOOL_A_HEADER_{name.upper()}"
>   - name: channels
>     canonical-c-name: "ETHTOOL_A_CHANNELS_{name.upper()}"
> 
> fixup-operations:
>   canonical-c-name: "ETHTOOL_MSG_{name.upper()}"
> 
>   # in case the "generic" catch-all above doesn't work
>   - name: channgels_get
>      canonical-c-name: "ETHTOOL_MSG_CHANNELS_GET"
> 
> We can call it "compatibility" yaml and put all sorts of weird stuff in it.
> New users hopefully don't need to care about it and don't need to
> write any of the -prefix/-suffix stuff.

Let me chew on this for a little bit.

> > Why does Python need to know the length of the string tho?
> > On receive if kernel gives you a longer name - great, no problem.
> > On send the kernel will tell you so also meh.  
> 
> I was thinking that you wanted to do some client size validation as
> well? As in sizeof(rx_buf) > ALTIFNAMSIZ -> panic? But I agree that
> there is really no value in that, the kernel will do the validation
> anyway..

Yup, I don't see the point, the ext_ack handling must be solid anyway,
so the kernel error is as good as local panic for cases which should
"never happen" (famous last words, maybe, we'll see)

Powered by blists - more mailing lists