On Thu, Aug 11, 2022 at 4:31 PM Jakub Kicinski [off-list ref] wrote:

On Thu, 11 Aug 2022 15:55:44 -0700 Stanislav Fomichev wrote:

quoted

quoted

We could, I guess. To be clear this controls the count, IOW:

enum {
        PREFIX_A_BLA_ATTR = 1,
        PREFIX_A_ANOTHER_ATTR,
        PREFIX_A_AND_ONEMORE,
        __PFREIX_A_CNT, // <--- This thing
};
#define PREFIX_A_MAX (__PFREIX_A_CNT - 1)

It's not used in the generated code, only if we codegen the uAPI,
AFAIR. So we'd need a way to tell the generator of the uAPI about
the situation, anyway. I could be misremembering.

My worry is that we'll have more hacks like these and it's hard, as a
spec reader/writer, to figure out that they exist..
So I was wondering if it's "easier" (from the spec reader/writer pov)
to have some c-header-fixup: section where we can have plain
c-preprocessor hacks like these (where we need to redefine something
to match the old behavior).

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 :-)

quoted

Coming from stubby/grpc, I was expecting to see words like
message/field/struct. The question is what's more confusing: sticking
with netlink naming or trying to map grpc/thrift concepts on top of
what we have. (I'm assuming more people know about grpc/thrift than
netlink)

messages: # or maybe 'attribute-sets' ?
  - name: channels
    ...

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 :-)

quoted

operations:
  - name: channel_get
    message: channels
    do:
      request:
        fields:
        - header
        - rx_max

Or maybe all we really need is a section in the doc called 'Netlink
for gRPC/Thrift users' where we map these concepts:
- attribute-spaces (attribute-sets?) -> messages
- attributes -> fields

Excellent idea!

quoted

quoted

Dunno, that'd mean that the Python method is called
ETHTOOL_MSG_CHANNELS_GET rather than just channels_get.
I don't want to force all languages to use the C naming.
The C naming just leads to silly copy'n'paste issues like
f329a0ebeab.

Can we have 'name:' and 'long-name:' or 'c-name:' or 'full-name' ?

- name: header
   attributes:
    - name: dev_index
      full-name: ETHTOOL_A_HEADER_DEV_INDEX
      val:
      type:

Suppose I'm rewriting my c application from uapi to some generated (in
the future) python-like channels_get() method. If I can grep for
ETHTOOL_MSG_CHANNELS_GET, that would save me a bunch of time figuring
out what the new canonical wrapper is.

Also, maybe, at some point we'll have:
- name: dev_index
  c-name: ETHTOOL_A_HEADER_DEV_INDEX
  java-name: headerDevIndex

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.. 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 :-)

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.

quoted

quoted

Good catch, I'm aware. I was planning to add a "header constants"
section or some such. A section in "headers" which defines the
constants which C code will get from the headers.

Define as in 're-define' or define as in 'you need to include some
other header for this to work'?

const:
  - name: ALTIFNAMSIZ
    val: 128

This one. In most cases the constant is defined in the same uAPI header
as the proto so we're good. But there's IFNAMSIZ and friends which are
shared.

quoted

which then does

#ifndef
#define ALTIFNAMSIZ 128
#else
static_assert(ALTIFNAMSIZ == 128)
#endif

?

or:

external-const:
  - name: ALTIFNAMSIZ
    header: include/uapi/linux/if.h

which then might generate the following:

#include <include/uapi/linux/if.h>
#ifndef ALTIFNAMSIZ
#error "include/uapi/linux/if.h does not define ALTIFNAMSIZ"
#endif

quoted

For Python it does not matter, as we don't have to size arrays.

Hm, I was expecting the situation to be the opposite :-) Because if
you really have to know this len in python, how do you resolve
ALTIFNAMSIZ?

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..

In C the struct has a char bla[FIXED_SIZE] so if we get an oversized
string we're pooped, that's my point, dunno what other practical use
the string sizing has.

quoted

The simplest thing to do might be to require these headers to be
hermetic (i.e., redefine all consts the spec cares about internally)?

That's what I'm thinking if they are actually needed. But it only C
cares we can just slap the right includes and not worry. Dunno if other
languages are similarly string-challenged.