Re: [RFC PATCH net-next v1 0/6] tools/net/ynl: Add dynamic selector for options attrs

[Donald Hunter <donald.hunter@gmail.com>]

Jakub Kicinski <kuba@kernel.org> writes:

> On Wed, 29 Nov 2023 10:11:53 +0000 Donald Hunter wrote:
>> This patchset adds a dynamic selector mechanism to YNL for kind-specific
>> options attributes. I am sending this as an RFC solicit feedback on a
>> couple of issues before I complete the patchset.
>
> Exciting stuff!
>  
>> I started adding this feature for the rt_link spec which is monomorphic,
>> i.e. the kind-specific 'data' attribute is always a nest. The selector
>> looked like this:
>> 
>>   -
>>     name: data
>>     type: dynamic
>>     selector:
>>       attribute: kind
>>       list:
>>         -
>>           value: bridge
>>           nested-attributes: linkinfo-bridge-attrs
>>         -
>>           value: erspan
>>           nested-attributes: linkinfo-gre-attrs
>
> It's kinda moot given your discovery below :(, but FWIW this is very
> close to what I've been thinking.
>
> After some pondering I thought it'd be better to structure it just
> a bit differently:
>
>  -
>    name: data
>    type: poly-nest
>    selector: kind    # which attr carries the key
>
> that's it for the attr, and then in attr-set I'd add a "key":
>
>  -
>    name: linkinfo-bridge-attrs
>    poly-key: bridge
>
> putting the key on the attr set is worse if we ever need to "key"
> the same attr set with different selectors, but it makes the attr
> definition a lot smaller. And in practice I didn't expect us
> to ever need keying into one attr set with different selectors. 
> If we did - we could complicate it later, but start simple.

rt_link shares attribute-sets between different kinds of link so I think
that rules out putting the key on the attribute-set. I think we may also
see reuse across stats attribute sets in tc.

FWIW I initially considered avoiding a selector list by using a template
to generate the attribute set name, but that broke pretty quickly.

>> ...
>> I now see a few possible ways forward and would like feedback on the
>> preferred approach:
>> 
>> 1. Simplify the current patchset to implement fixed-header & nest
>>    support in the dynamic selector. This would leave existing
>>    fixed-header support for messages unchanged. We could drop the 'type'
>>    field.
>> 
>>    -
>>      value: netem
>>      fixed-header: tc-netem-qopt
>>      nested-attributes: tc-netem-attrs
>> 
>> 2. Keep the 'type' field and support for the 'binary' type which is
>>    useful for specifying nests with unknown attribute spaces. An
>>    alternative would be to default to 'binary' behaviour if there is no
>>    selector entry.
>> 
>> 3. Refactor the existing fixed-header support to be an optional part of
>>    all attribute sets instead of just messages (in legacy and raw specs)
>>    and dynamic attribute nests (in raw specs).
>> 
>>    attribute-sets:
>>      -
>>        name: tc-netem-attrs
>>        fixed-header: tc-netem-qopt
>>        attributes:
>
> Reading this makes me feel like netem wants to be a "sub-message"?
> It has a fixed header followed by attrs, that's quite message-like.

Yeah, I guess we could call it sub-message because it's not a pure nest
and the different name makes it an explicitly netlink-raw concept.

> Something along the lines of 1 makes most sense to me, but can we
> put the "selector ladder" out-of-line? I'm worried that the attr
> definition will get crazy long.

It seems reasonable to pull the selector list out of line because
they do get big, e.g. over 100 lines for tc "options".

My preference is 1, probably including a fallback to "binary" if there
is no selector match.

> attribute-sets:
>   -
>     name: outside-attrs
>     attributes:
>       ...
>       -
>          name: kind
>          type: string
>       -
>          name: options
>          type: sub-message
>          sub-type: inside-msg  # reuse sub-type or new property?
>          selector: kind
>     ...
>   -
>     name: inside-attrs:
>     attributes: 
>       ...
>
> sub-messages:
>   list:
>     -
>       name: inside-msg
>       formats: # not a great name?..
>         -
>           value: some-value
>           fixed-header: struct-name
>         -
>           value: other-value
>           fixed-header: struct-name-two
>           nested-attributes: inside-attrs
>         -
>           value: another-one
>           nested-attributes: inside-attrs
>     -
>       name: different-inside-msg
>       ...
>
> operations:
>   ...
>
> At least that's what comes to my mind after reading the problem
> description. Does it make sense?

I think that once you have broken out to a sub-message, they're no
longer "nested-attributes" and we should maybe reuse "attribute-set".

I don't think we can reuse "sub-type" because the schema for it is the
set of netlink type names, not a free string. Maybe we add "sub-message"
instead? So how about this:

attribute-sets:
  -
    name: outside-attrs
    attributes:
      ...
      -
         name: kind
         type: string
      -
         name: options
         type: sub-message
         sub-message: inside-msg
         selector: kind
    ...
  -
    name: inside-attrs:
    attributes:
      ...

sub-messages:
  -
    name: inside-msg
    formats:
      -
        value: some-value
        fixed-header: struct-name
      -
        value: other-value
        fixed-header: struct-name-two
        attribute-set: inside-attrs
      -
        value: another-one
        attribute-set: inside-attrs
  -
    name: different-inside-msg
    ...

operations:
  ...

I cannot think of a better name than "formats" so happy to go with that.
Did you want an explicit "list:" in the yaml schema?

Thanks,
Donald.