| Message ID | 20240326063728.2369353-2-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ynl: rename array-nest to indexed-array | expand |
On Tue, 26 Mar 2024 14:37:27 +0800 Hangbin Liu wrote: > diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py > index 5fa7957f6e0f..7239e673a28a 100644 > --- a/tools/net/ynl/lib/ynl.py > +++ b/tools/net/ynl/lib/ynl.py > @@ -686,8 +686,9 @@ class YnlFamily(SpecFamily): > decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order) > if 'enum' in attr_spec: > decoded = self._decode_enum(decoded, attr_spec) > - elif attr_spec["type"] == 'array-nest': > - decoded = self._decode_array_nest(attr, attr_spec) > + elif attr_spec["type"] == 'indexed-array' and 'sub-type' in attr_spec: > + if attr_spec["sub-type"] == 'nest': > + decoded = self._decode_array_nest(attr, attr_spec) We need to make sure somehow cleanly that we treat unknown subtype the same we would treat unknown type. In this elif ladder we have: else: if not self.process_unknown: raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}') So we should raise an exception if sub-type != nest. > elif attr_spec["type"] == 'bitfield32': > value, selector = struct.unpack("II", attr.raw) > if 'enum' in attr_spec: > diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py > index 6b7eb2d2aaf1..8d5ec5449648 100755 > --- a/tools/net/ynl/ynl-gen-c.py > +++ b/tools/net/ynl/ynl-gen-c.py > @@ -838,8 +838,9 @@ class AttrSet(SpecAttrSet): > t = TypeBitfield32(self.family, self, elem, value) > elif elem['type'] == 'nest': > t = TypeNest(self.family, self, elem, value) > - elif elem['type'] == 'array-nest': > - t = TypeArrayNest(self.family, self, elem, value) > + elif elem['type'] == 'indexed-array' and 'sub-type' in elem: > + if elem["sub-type"] == 'nest': > + t = TypeArrayNest(self.family, self, elem, value) same here > elif elem['type'] == 'nest-type-value': > t = TypeNestTypeValue(self.family, self, elem, value) > else: > @@ -1052,8 +1053,9 @@ class Family(SpecFamily): > if nested in self.root_sets: > raise Exception("Inheriting members to a space used as root not supported") > inherit.update(set(spec['type-value'])) > - elif spec['type'] == 'array-nest': > - inherit.add('idx') > + elif spec['type'] == 'indexed-array' and 'sub-type' in spec: > + if spec["sub-type"] == 'nest': > + inherit.add('idx') Here you don't have to match on sub-type, all indexed-arrays will have an idx (index) member. > self.pure_nested_structs[nested].set_inherited(inherit) > > for root_set, rs_members in self.root_sets.items(): > @@ -1616,9 +1618,10 @@ def _multi_parse(ri, struct, init_lines, local_vars): > multi_attrs = set() > needs_parg = False > for arg, aspec in struct.member_list(): > - if aspec['type'] == 'array-nest': > - local_vars.append(f'const struct nlattr *attr_{aspec.c_name};') > - array_nests.add(arg) > + if aspec['type'] == 'indexed-array' and 'sub-type' in aspec: > + if aspec["sub-type"] == 'nest': > + local_vars.append(f'const struct nlattr *attr_{aspec.c_name};') > + array_nests.add(arg) Please also update the info about nested-array in Documentation/userspace-api/netlink/genetlink-legacy.rst
On Tue, Mar 26, 2024 at 08:46:10PM -0700, Jakub Kicinski wrote: > On Tue, 26 Mar 2024 14:37:27 +0800 Hangbin Liu wrote: > > diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py > > index 5fa7957f6e0f..7239e673a28a 100644 > > --- a/tools/net/ynl/lib/ynl.py > > +++ b/tools/net/ynl/lib/ynl.py > > @@ -686,8 +686,9 @@ class YnlFamily(SpecFamily): > > decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order) > > if 'enum' in attr_spec: > > decoded = self._decode_enum(decoded, attr_spec) > > - elif attr_spec["type"] == 'array-nest': > > - decoded = self._decode_array_nest(attr, attr_spec) > > + elif attr_spec["type"] == 'indexed-array' and 'sub-type' in attr_spec: > > + if attr_spec["sub-type"] == 'nest': > > + decoded = self._decode_array_nest(attr, attr_spec) > > We need to make sure somehow cleanly that we treat unknown subtype the > same we would treat unknown type. In this elif ladder we have: > > else: > if not self.process_unknown: > raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}') > > So we should raise an exception if sub-type != nest. I agree we need raise exception when only support nest sub-type. But what about after adding other sub-types in patch 2/2. e.g. if attr_spec["sub-type"] == 'nest': decoded = self._decode_array_nest(attr, attr_spec) else: decoded = self._decode_index_array(attr, attr_spec) Should we remove the exception in patch 2? Thanks Hangbin
On Thu, 28 Mar 2024 15:50:35 +0800 Hangbin Liu wrote: > I agree we need raise exception when only support nest sub-type. But > what about after adding other sub-types in patch 2/2. e.g. > > if attr_spec["sub-type"] == 'nest': > decoded = self._decode_array_nest(attr, attr_spec) > else: > decoded = self._decode_index_array(attr, attr_spec) > > Should we remove the exception in patch 2? Looking a bit closer you should probably have: - elif attr_spec["type"] == 'array-nest': - decoded = self._decode_array_nest(attr, attr_spec) + elif attr_spec["type"] == 'indexed-array': + decoded = self._decode_array_attr(attr, attr_spec) elif attr_spec["type"] == 'bitfield32': and do the sub-type handling inside (now renamed) _decode_array_attr() Throw the exception there appropriately.
diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml index 4dfd899a1661..4f803eaac6d8 100644 --- a/Documentation/netlink/genetlink-c.yaml +++ b/Documentation/netlink/genetlink-c.yaml @@ -158,7 +158,7 @@ properties: type: &attr-type enum: [ unused, pad, flag, binary, uint, sint, u8, u16, u32, u64, s32, s64, - string, nest, array-nest, nest-type-value ] + string, nest, indexed-array, nest-type-value ] doc: description: Documentation of the attribute. type: string diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml index b48ad3b1cc32..8db0e22fa72c 100644 --- a/Documentation/netlink/genetlink-legacy.yaml +++ b/Documentation/netlink/genetlink-legacy.yaml @@ -201,7 +201,7 @@ properties: description: The netlink attribute type enum: [ unused, pad, flag, binary, bitfield32, uint, sint, u8, u16, u32, u64, s32, s64, - string, nest, array-nest, nest-type-value ] + string, nest, indexed-array, nest-type-value ] doc: description: Documentation of the attribute. type: string diff --git a/Documentation/netlink/genetlink.yaml b/Documentation/netlink/genetlink.yaml index ebd6ee743fcc..b036227b46f1 100644 --- a/Documentation/netlink/genetlink.yaml +++ b/Documentation/netlink/genetlink.yaml @@ -124,7 +124,7 @@ properties: type: &attr-type enum: [ unused, pad, flag, binary, uint, sint, u8, u16, u32, u64, s32, s64, - string, nest, array-nest, nest-type-value ] + string, nest, indexed-array, nest-type-value ] doc: description: Documentation of the attribute. type: string diff --git a/Documentation/netlink/netlink-raw.yaml b/Documentation/netlink/netlink-raw.yaml index a76e54cbadbc..914aa1c0a273 100644 --- a/Documentation/netlink/netlink-raw.yaml +++ b/Documentation/netlink/netlink-raw.yaml @@ -222,7 +222,7 @@ properties: description: The netlink attribute type enum: [ unused, pad, flag, binary, bitfield32, u8, u16, u32, u64, s8, s16, s32, s64, - string, nest, array-nest, nest-type-value, + string, nest, indexed-array, nest-type-value, sub-message ] doc: description: Documentation of the attribute. diff --git a/Documentation/netlink/specs/nlctrl.yaml b/Documentation/netlink/specs/nlctrl.yaml index b1632b95f725..a36535350bdb 100644 --- a/Documentation/netlink/specs/nlctrl.yaml +++ b/Documentation/netlink/specs/nlctrl.yaml @@ -65,11 +65,13 @@ attribute-sets: type: u32 - name: ops - type: array-nest + type: indexed-array + sub-type: nest nested-attributes: op-attrs - name: mcast-groups - type: array-nest + type: indexed-array + sub-type: nest nested-attributes: mcast-group-attrs - name: policy diff --git a/Documentation/netlink/specs/rt_link.yaml b/Documentation/netlink/specs/rt_link.yaml index 8e4d19adee8c..08bec2537a63 100644 --- a/Documentation/netlink/specs/rt_link.yaml +++ b/Documentation/netlink/specs/rt_link.yaml @@ -1617,7 +1617,8 @@ attribute-sets: type: binary - name: hw-s-info - type: array-nest + type: indexed-array + sub-type: nest nested-attributes: hw-s-info-one - name: l3-stats diff --git a/Documentation/netlink/specs/tc.yaml b/Documentation/netlink/specs/tc.yaml index 324fa182cd14..dbcf19e494ec 100644 --- a/Documentation/netlink/specs/tc.yaml +++ b/Documentation/netlink/specs/tc.yaml @@ -1937,7 +1937,8 @@ attribute-sets: nested-attributes: tc-ematch-attrs - name: act - type: array-nest + type: indexed-array + sub-type: nest nested-attributes: tc-act-attrs - name: police @@ -2077,7 +2078,8 @@ attribute-sets: type: u32 - name: tin-stats - type: array-nest + type: indexed-array + sub-type: nest nested-attributes: tc-cake-tin-stats-attrs - name: deficit @@ -2297,7 +2299,8 @@ attribute-sets: type: string - name: act - type: array-nest + type: indexed-array + sub-type: nest nested-attributes: tc-act-attrs - name: key-eth-dst @@ -2798,7 +2801,8 @@ attribute-sets: type: string - name: act - type: array-nest + type: indexed-array + sub-type: nest nested-attributes: tc-act-attrs - name: mask @@ -2951,7 +2955,8 @@ attribute-sets: type: u32 - name: act - type: array-nest + type: indexed-array + sub-type: nest nested-attributes: tc-act-attrs - name: flags @@ -3324,7 +3329,8 @@ attribute-sets: nested-attributes: tc-police-attrs - name: act - type: array-nest + type: indexed-array + sub-type: nest nested-attributes: tc-act-attrs - name: tc-taprio-attrs @@ -3542,7 +3548,8 @@ attribute-sets: nested-attributes: tc-police-attrs - name: act - type: array-nest + type: indexed-array + sub-type: nest nested-attributes: tc-act-attrs - name: indev diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index 5fa7957f6e0f..7239e673a28a 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -686,8 +686,9 @@ class YnlFamily(SpecFamily): decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order) if 'enum' in attr_spec: decoded = self._decode_enum(decoded, attr_spec) - elif attr_spec["type"] == 'array-nest': - decoded = self._decode_array_nest(attr, attr_spec) + elif attr_spec["type"] == 'indexed-array' and 'sub-type' in attr_spec: + if attr_spec["sub-type"] == 'nest': + decoded = self._decode_array_nest(attr, attr_spec) elif attr_spec["type"] == 'bitfield32': value, selector = struct.unpack("II", attr.raw) if 'enum' in attr_spec: diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py index 6b7eb2d2aaf1..8d5ec5449648 100755 --- a/tools/net/ynl/ynl-gen-c.py +++ b/tools/net/ynl/ynl-gen-c.py @@ -838,8 +838,9 @@ class AttrSet(SpecAttrSet): t = TypeBitfield32(self.family, self, elem, value) elif elem['type'] == 'nest': t = TypeNest(self.family, self, elem, value) - elif elem['type'] == 'array-nest': - t = TypeArrayNest(self.family, self, elem, value) + elif elem['type'] == 'indexed-array' and 'sub-type' in elem: + if elem["sub-type"] == 'nest': + t = TypeArrayNest(self.family, self, elem, value) elif elem['type'] == 'nest-type-value': t = TypeNestTypeValue(self.family, self, elem, value) else: @@ -1052,8 +1053,9 @@ class Family(SpecFamily): if nested in self.root_sets: raise Exception("Inheriting members to a space used as root not supported") inherit.update(set(spec['type-value'])) - elif spec['type'] == 'array-nest': - inherit.add('idx') + elif spec['type'] == 'indexed-array' and 'sub-type' in spec: + if spec["sub-type"] == 'nest': + inherit.add('idx') self.pure_nested_structs[nested].set_inherited(inherit) for root_set, rs_members in self.root_sets.items(): @@ -1616,9 +1618,10 @@ def _multi_parse(ri, struct, init_lines, local_vars): multi_attrs = set() needs_parg = False for arg, aspec in struct.member_list(): - if aspec['type'] == 'array-nest': - local_vars.append(f'const struct nlattr *attr_{aspec.c_name};') - array_nests.add(arg) + if aspec['type'] == 'indexed-array' and 'sub-type' in aspec: + if aspec["sub-type"] == 'nest': + local_vars.append(f'const struct nlattr *attr_{aspec.c_name};') + array_nests.add(arg) if 'multi-attr' in aspec: multi_attrs.add(arg) needs_parg |= 'nested-attributes' in aspec
Some implementations, like bonding, has nest array with same attr type. To support all kinds of entries under one nest array. As disscussed[1], let's rename array-nest to indexed-array, and assuming the value is a nest by passing the type via sub-type. [1] https://lore.kernel.org/netdev/20240312100105.16a59086@kernel.org/ Suggested-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- Documentation/netlink/genetlink-c.yaml | 2 +- Documentation/netlink/genetlink-legacy.yaml | 2 +- Documentation/netlink/genetlink.yaml | 2 +- Documentation/netlink/netlink-raw.yaml | 2 +- Documentation/netlink/specs/nlctrl.yaml | 6 ++++-- Documentation/netlink/specs/rt_link.yaml | 3 ++- Documentation/netlink/specs/tc.yaml | 21 ++++++++++++++------- tools/net/ynl/lib/ynl.py | 5 +++-- tools/net/ynl/ynl-gen-c.py | 17 ++++++++++------- 9 files changed, 37 insertions(+), 23 deletions(-)