| Message ID | 20240219172525.71406-4-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netlink: specs: devlink: add the rest of missing attribute definitions | expand |
On Mon, 19 Feb 2024 18:25:19 +0100 Jiri Pirko wrote:
> + def _get_scalar(self, attr_spec, value):
It'd be cleaner to make it more symmetric with _decode_enum(), and call
it _encode_enum().
On Mon, 19 Feb 2024 18:25:19 +0100 Jiri Pirko wrote: > + if enum.type == 'flags' or attr_spec.get('enum-as-flags', False): > + scalar = 0 > + for single_value in value: > + scalar += enum.entries[single_value].user_value(as_flags = True) If the user mistakenly passes a single value for a flag, rather than a set, this is going to generate a hard to understand error. How about we check isinstance(, str) and handle that directly, whether a flag or not.
Mon, Feb 19, 2024 at 09:49:14PM CET, kuba@kernel.org wrote: >On Mon, 19 Feb 2024 18:25:19 +0100 Jiri Pirko wrote: >> + def _get_scalar(self, attr_spec, value): > >It'd be cleaner to make it more symmetric with _decode_enum(), and call >it _encode_enum(). That is misleading name, as it does not have to be enum.
Mon, Feb 19, 2024 at 09:51:00PM CET, kuba@kernel.org wrote: >On Mon, 19 Feb 2024 18:25:19 +0100 Jiri Pirko wrote: >> + if enum.type == 'flags' or attr_spec.get('enum-as-flags', False): >> + scalar = 0 >> + for single_value in value: >> + scalar += enum.entries[single_value].user_value(as_flags = True) > >If the user mistakenly passes a single value for a flag, rather than >a set, this is going to generate a hard to understand error. >How about we check isinstance(, str) and handle that directly, >whether a flag or not. Yeah, I was thinking about that as well. But as the flag output is always list, here we expect also always list. I can either do what you suggest of Errout with some sane message in case of the variable is not a list. I didn't find ynl to be particularly forgiving in case of input and error messages, that is why I didn't bother here.
On Tue, 20 Feb 2024 08:25:14 +0100 Jiri Pirko wrote: > >It'd be cleaner to make it more symmetric with _decode_enum(), and call > >it _encode_enum(). > > That is misleading name, as it does not have to be enum. It's a variant of a encode function using enum metadata.
On Tue, 20 Feb 2024 08:27:57 +0100 Jiri Pirko wrote: > >If the user mistakenly passes a single value for a flag, rather than > >a set, this is going to generate a hard to understand error. > >How about we check isinstance(, str) and handle that directly, > >whether a flag or not. > > Yeah, I was thinking about that as well. But as the flag output is > always list, here we expect also always list. I can either do what you > suggest of Errout with some sane message in case of the variable is not > a list. I didn't find ynl to be particularly forgiving in case of input > and error messages, that is why I didn't bother here. It's not the same thing, but (without looking at the code IIRC) for multi-attr we do accept both a list and direct value. Here we don't have to be lenient in what we accept. Clear error message would be good enough. Some of the sharp edges in Python YNL are because I very much anticipated the pyroute2 maintainer to do a proper implementation, and this tool was just a very crude PoC :D
On Wed, 21 Feb 2024 at 01:59, Jakub Kicinski <kuba@kernel.org> wrote: > > Some of the sharp edges in Python YNL are because I very much > anticipated the pyroute2 maintainer to do a proper implementation, > and this tool was just a very crude PoC :D Hah yeah, I looked at pyroute2 a while back and thought there was a bit of an impedance mismatch between the dynamic schema driven approach of ynl and the declarative / procedural code in iproute2. I think a code generator would be the way to target iproute2. https://github.com/svinota/pyroute2/blob/34d0768f89fd232126c49e2f7c94e6da6582795b/pyroute2/netlink/rtnl/rtmsg.py#L102-L139 I find ynl to be a very useful tool when writing and testing spec files and have been happy to contribute to it for that purpose. I think we have started to remove some of the sharp edges, but there is more to do :-)
Wed, Feb 21, 2024 at 02:55:56AM CET, kuba@kernel.org wrote: >On Tue, 20 Feb 2024 08:25:14 +0100 Jiri Pirko wrote: >> >It'd be cleaner to make it more symmetric with _decode_enum(), and call >> >it _encode_enum(). >> >> That is misleading name, as it does not have to be enum. > >It's a variant of a encode function using enum metadata. Not really. "decode_enum()" is exactly that, it only works with enum case. Here, the enum is optional. If user passes scalar directly or if the attribute does not have "enum" defined, this has nothing to do with enum. But, as you wish.
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index ccdb2f1e7379..d2ea1571d00c 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -438,6 +438,21 @@ class YnlFamily(SpecFamily): self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_ADD_MEMBERSHIP, mcast_id) + def _get_scalar(self, attr_spec, value): + try: + return int(value) + except (ValueError, TypeError) as e: + if 'enum' not in attr_spec: + raise e + enum = self.consts[attr_spec['enum']] + if enum.type == 'flags' or attr_spec.get('enum-as-flags', False): + scalar = 0 + for single_value in value: + scalar += enum.entries[single_value].user_value(as_flags = True) + return scalar + else: + return enum.entries[value].user_value() + def _add_attr(self, space, name, value, search_attrs): try: attr = self.attr_sets[space][name] @@ -474,7 +489,7 @@ class YnlFamily(SpecFamily): else: raise Exception(f'Unknown type for binary attribute, value: {value}') elif attr['type'] in NlAttr.type_formats or attr.is_auto_scalar: - scalar = int(value) + scalar = self._get_scalar(attr, value) if attr.is_auto_scalar: attr_type = attr["type"][0] + ('32' if scalar.bit_length() <= 32 else '64') else: @@ -482,7 +497,9 @@ class YnlFamily(SpecFamily): format = NlAttr.get_format(attr_type, attr.byte_order) attr_payload = format.pack(scalar) elif attr['type'] in "bitfield32": - attr_payload = struct.pack("II", int(value["value"]), int(value["selector"])) + scalar_value = self._get_scalar(attr, value["value"]) + scalar_selector = self._get_scalar(attr, value["selector"]) + attr_payload = struct.pack("II", scalar_value, scalar_selector) elif attr['type'] == 'sub-message': msg_format = self._resolve_selector(attr, search_attrs) attr_payload = b''