| Message ID | 20231130214959.27377-5-donald.hunter@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tools/net/ynl: Add 'sub-message' support to ynl | expand |
On Thu, 30 Nov 2023 21:49:56 +0000 Donald Hunter wrote: > The tc netlink-raw family needs binary and pad types for several > qopt C structs. Add support for them to ynl. Nice reuse of the concept of "pad", I don't see why not: Reviewed-by: Jakub Kicinski <kuba@kernel.org> > + value = msg.raw[offset:offset+m.len] What does Python style guide say about spaces around '+' here? I tend to use C style, no idea if it's right.
Jakub Kicinski <kuba@kernel.org> writes: > On Thu, 30 Nov 2023 21:49:56 +0000 Donald Hunter wrote: >> The tc netlink-raw family needs binary and pad types for several >> qopt C structs. Add support for them to ynl. > > Nice reuse of the concept of "pad", I don't see why not: > > Reviewed-by: Jakub Kicinski <kuba@kernel.org> > >> + value = msg.raw[offset:offset+m.len] > > What does Python style guide say about spaces around '+' here? > I tend to use C style, no idea if it's right. The relevant section seems to be this: However, in a slice the colon acts like a binary operator, and should have equal amounts on either side (treating it as the operator with the lowest priority). In an extended slice, both colons must have the same amount of spacing applied. Exception: when a slice parameter is omitted, the space is omitted: # Correct: ham[1:9], ham[1:9:3], ham[:9:3], ham[1::3], ham[1:9:] ham[lower:upper], ham[lower:upper:], ham[lower::step] ham[lower+offset : upper+offset] ham[: upper_fn(x) : step_fn(x)], ham[:: step_fn(x)] ham[lower + offset : upper + offset] # Wrong: ham[lower + offset:upper + offset] ham[1: 9], ham[1 :9], ham[1:9 :3] ham[lower : : step] ham[ : upper] On that basis I could change it to: (a) value = msg.raw[offset : offset+m.len] or: (b) value = msg.raw[offset : offset + m.len] But the existing convention in the code is a mix of these styles: raw[offset:offset + 4] raw[offset:offset+m['len']] Happy to go with whatever preference, though maximising whitespace per (b) follows python style _and_ C style? Also happy to make it consistent across the file (in a separate patch)?
On Mon, 04 Dec 2023 16:18:14 +0000 Donald Hunter wrote: > (b) value = msg.raw[offset : offset + m.len] > > Happy to go with whatever preference, though maximising whitespace per > (b) follows python style _and_ C style? Yup, style (b) does look the least surprising to my C-accustomed eyes, so +1 on using that. > Also happy to make it consistent across the file (in a separate patch)? Follow up cleanup sounds good!
diff --git a/Documentation/netlink/netlink-raw.yaml b/Documentation/netlink/netlink-raw.yaml index 26203282422f..dc3d4eeb67bb 100644 --- a/Documentation/netlink/netlink-raw.yaml +++ b/Documentation/netlink/netlink-raw.yaml @@ -127,7 +127,7 @@ properties: type: string type: description: The netlink attribute type - enum: [ u8, u16, u32, u64, s8, s16, s32, s64, string, binary ] + enum: [ u8, u16, u32, u64, s8, s16, s32, s64, string, binary, pad ] len: $ref: '#/$defs/len-or-define' byte-order: diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index 886ecef5319e..4f1c1e51845e 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -670,8 +670,11 @@ class YnlFamily(SpecFamily): fixed_header_members = self.consts[name].members size = 0 for m in fixed_header_members: - format = NlAttr.get_format(m.type, m.byte_order) - size += format.size + if m.type in ['pad', 'binary']: + size += m.len + else: + format = NlAttr.get_format(m.type, m.byte_order) + size += format.size return size else: return 0 @@ -681,12 +684,20 @@ class YnlFamily(SpecFamily): fixed_header_attrs = dict() offset = 0 for m in fixed_header_members: - format = NlAttr.get_format(m.type, m.byte_order) - [ value ] = format.unpack_from(msg.raw, offset) - offset += format.size - if m.enum: - value = self._decode_enum(value, m) - fixed_header_attrs[m.name] = value + value = None + if m.type == 'pad': + offset += m.len + elif m.type == 'binary': + value = msg.raw[offset:offset+m.len] + offset += m.len + else: + format = NlAttr.get_format(m.type, m.byte_order) + [ value ] = format.unpack_from(msg.raw, offset) + offset += format.size + if value is not None: + if m.enum: + value = self._decode_enum(value, m) + fixed_header_attrs[m.name] = value return fixed_header_attrs def handle_ntf(self, decoded): @@ -753,8 +764,13 @@ class YnlFamily(SpecFamily): fixed_header_members = self.consts[op.fixed_header].members for m in fixed_header_members: value = vals.pop(m.name) if m.name in vals else 0 - format = NlAttr.get_format(m.type, m.byte_order) - msg += format.pack(value) + if m.type == 'pad': + msg += bytearray(m.len) + elif m.type == 'binary': + msg += bytes.fromhex(value) + else: + format = NlAttr.get_format(m.type, m.byte_order) + msg += format.pack(value) for name, value in vals.items(): msg += self._add_attr(op.attr_set.name, name, value) msg = _genl_msg_finalize(msg)
The tc netlink-raw family needs binary and pad types for several qopt C structs. Add support for them to ynl. Signed-off-by: Donald Hunter <donald.hunter@gmail.com> --- Documentation/netlink/netlink-raw.yaml | 2 +- tools/net/ynl/lib/ynl.py | 36 +++++++++++++++++++------- 2 files changed, 27 insertions(+), 11 deletions(-)