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.
> Then I started working on tc and found that the 'options' attribute is
> poymorphic. It is typically either a C struct or a nest. So I extended the
> dynamic selector to include a 'type' field and type-specific sub-fields:
>
> -
> name: options
> type: dynamic
> selector:
> attribute: kind
> list:
> -
> value: bfifo
> type: binary
> struct: tc-fifo-qopt
> -
> value: cake
> type: nest
> nested-attributes: tc-cake-attrs
> -
> value: cbs
> type: nest
> nested-attributes: tc-cbs-attrs
>
> Then I encountered 'netem' which has a nest with a C struct header. I
> realised that maybe my mental model had been wrong and that all cases
> could be supported by a nest type with an optional fixed-header followed
> by zero or more nlattrs.
>
> -
> value: netem
> type: nest
> fixed-header: tc-netem-qopt
> nested-attributes: tc-netem-attrs
>
> Perhaps it is attribute-sets in general that should have an optional
> fixed-header, which would also work for fixed-headers at the start of
> genetlink messages. I originally added fixed-header support to
> operations for genetlink, but fixed headers on attribute sets would work
> for all these cases.
>
> 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.
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.
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?
