| Message ID | 20230319193803.97453-5-donald.hunter@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ynl: add support for user headers and struct attrs | expand |
On Sun, 19 Mar 2023 19:38:01 +0000 Donald Hunter wrote: > enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, > - string, nest, array-nest, nest-type-value ] > + string, nest, array-nest, nest-type-value, struct ] I wonder if we should also only allow struct as a subtype of binary? Structs can technically grow with newer kernels (i.e. new members can be added at the end). So I think for languages like C we will still need to expose to the user the original length of the attribute. And binary comes with a length so codgen reuse fits nicely. Either way - docs need to be updated.
Jakub Kicinski <kuba@kernel.org> writes: > On Sun, 19 Mar 2023 19:38:01 +0000 Donald Hunter wrote: >> enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, >> - string, nest, array-nest, nest-type-value ] >> + string, nest, array-nest, nest-type-value, struct ] > > I wonder if we should also only allow struct as a subtype of binary? > > Structs can technically grow with newer kernels (i.e. new members can > be added at the end). So I think for languages like C we will still > need to expose to the user the original length of the attribute. > And binary comes with a length so codgen reuse fits nicely. > > Either way - docs need to be updated. Yep, as I was replying to your previous comment, I started to think about making struct a subtype of binary. That would make a struct attr something like: - name: stats type: binary sub-type: struct struct: vport-stats I originally chose 'struct' as the attr name, following the pattern that 'enum' is used for enum names but I'm not sure it's clear enough. Maybe 'sub-type-name' would be better? I will update the documentation for this.
On Wed, 22 Mar 2023 11:48:12 +0000 Donald Hunter wrote: > > On Sun, 19 Mar 2023 19:38:01 +0000 Donald Hunter wrote: > >> enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, > >> - string, nest, array-nest, nest-type-value ] > >> + string, nest, array-nest, nest-type-value, struct ] > > > > I wonder if we should also only allow struct as a subtype of binary? > > > > Structs can technically grow with newer kernels (i.e. new members can > > be added at the end). So I think for languages like C we will still > > need to expose to the user the original length of the attribute. > > And binary comes with a length so codgen reuse fits nicely. > > > > Either way - docs need to be updated. > > Yep, as I was replying to your previous comment, I started to think > about making struct a subtype of binary. That would make a struct attr > something like: > > - > name: stats > type: binary > sub-type: struct > struct: vport-stats LGTM! > I originally chose 'struct' as the attr name, following the pattern that > 'enum' is used for enum names but I'm not sure it's clear enough. Maybe > 'sub-type-name' would be better? Agreed, using the sub-type's value as name of another attr is mixing keys and values. But sub-type-name would then also be used for enums (I mean in normal type: u32 enums, not binary arrays)? enums don't have a sub-type so there we'd have sub-type-name and no sub-type. Plus for binary arrays of enums we'd have: - name: stats type: binary sub-type: u32 sub-type-name: vport-stats Doesn't say enum anywhere :S We'd need to assume if sub-type is a scalar the sub-type-name is an enum? Maybe to avoid saying struct twice we should go the enum way and actually ditch the sub-type for structs? Presence of struct: abc implies it's a struct, only use sub-type for scalar types? - name: stats type: binary struct: vport-stats - name: another type: binary sub-type: u32 enum: enums-name
On Wed, 22 Mar 2023 at 18:38, Jakub Kicinski <kuba@kernel.org> wrote: > > Maybe to avoid saying struct twice we should go the enum way and > actually ditch the sub-type for structs? Presence of struct: abc > implies it's a struct, only use sub-type for scalar types? > > - > name: stats > type: binary > struct: vport-stats Yep, this looks good. I'll add this to the docs too. > - > name: another > type: binary > sub-type: u32 > enum: enums-name
diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml index 5dc6f1c07a97..654f40b26beb 100644 --- a/Documentation/netlink/genetlink-legacy.yaml +++ b/Documentation/netlink/genetlink-legacy.yaml @@ -172,7 +172,7 @@ properties: type: string type: &attr-type enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, - string, nest, array-nest, nest-type-value ] + string, nest, array-nest, nest-type-value, struct ] doc: description: Documentation of the attribute. type: string @@ -218,6 +218,11 @@ properties: description: Max length for a string or a binary attribute. $ref: '#/$defs/len-or-define' sub-type: *attr-type + # Start genetlink-legacy + struct: + description: Name of the struct type used for the attribute. + type: string + # End genetlink-legacy # Make sure name-prefix does not appear in subsets (subsets inherit naming) dependencies: diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index 077ba9e8dc98..24f8af3c2b38 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -68,6 +68,11 @@ class Netlink: class NlAttr: + type_formats = { 'u8' : ('B', 1), + 'u16': ('H', 2), + 'u32': ('I', 4), + 'u64': ('Q', 8) } + def __init__(self, raw, offset): self._len, self._type = struct.unpack("HH", raw[offset:offset + 4]) self.type = self._type & ~Netlink.NLA_TYPE_MASK @@ -97,6 +102,16 @@ class NlAttr: format, _ = self.type_formats[type] return list({ x[0] for x in struct.iter_unpack(format, self.raw) }) + def as_struct(self, members): + value = dict() + offset = 0 + for m in members: + format, size = self.type_formats[m.type] + decoded = struct.unpack_from(format, self.raw, offset) + offset += size + value[m.name] = decoded[0] + return value + def __repr__(self): return f"[type:{self.type} len:{self._len}] {self.raw}" @@ -385,6 +400,9 @@ class YnlFamily(SpecFamily): decoded = attr.as_bin() elif attr_spec["type"] == 'flag': decoded = True + elif attr_spec["type"] == 'struct': + s = attr_spec['struct'] + decoded = attr.as_struct(self.consts[s]) elif attr_spec["type"] == 'array-nest': decoded = attr.as_array(attr_spec["sub-type"]) else:
Add support for decoding attributes that contain C structs. Signed-off-by: Donald Hunter <donald.hunter@gmail.com> --- Documentation/netlink/genetlink-legacy.yaml | 7 ++++++- tools/net/ynl/lib/ynl.py | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-)