| Message ID | 20241203130655.45293-5-donald.hunter@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netlink: specs: add a spec for nl80211 wiphy | expand |
On Tue, 3 Dec 2024 13:06:52 +0000 Donald Hunter wrote: > + def _from_string(self, string, display_hint, type): Any reason not to pass attr_spec instead of the members one by one? > + if display_hint in ['ipv4', 'ipv6']: > + ip = ipaddress.ip_address(string) > + if type == 'binary': > + raw = ip.packed > + else: > + raw = int(ip) > + else: I wonder if we should raise in this case? Especially if type is binary passing the string back will just blow up later, right? We could instead rise with a nice clear error message here. > + raw = string > + return raw
On Wed, 4 Dec 2024 at 02:07, Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 3 Dec 2024 13:06:52 +0000 Donald Hunter wrote: > > + def _from_string(self, string, display_hint, type): > > Any reason not to pass attr_spec instead of the members one by one? I thought it would be better to keep the low-level helpers decoupled from attr_spec and mostly just following existing helpers that only have the display_hint parameter. > > + if display_hint in ['ipv4', 'ipv6']: > > + ip = ipaddress.ip_address(string) > > + if type == 'binary': > > + raw = ip.packed > > + else: > > + raw = int(ip) > > + else: > > I wonder if we should raise in this case? > Especially if type is binary passing the string back will just blow up > later, right? We could instead rise with a nice clear error message > here. It's actually a bit misleading that the attr is called 'string' because it could be a binary input if it was supplied from python code, i.e. not parsed from JSON on the command-line. But you highlight the bigger point that our input validation is quite weak and will need to be improved. In this case it would be desirable to defensively check if the input is already binary before checking for a display-hint. Then for input that can't be handled, we can raise an error message.
On Wed, 4 Dec 2024 at 13:37, Donald Hunter <donald.hunter@gmail.com> wrote: > > On Wed, 4 Dec 2024 at 02:07, Jakub Kicinski <kuba@kernel.org> wrote: > > > > > + if display_hint in ['ipv4', 'ipv6']: > > > + ip = ipaddress.ip_address(string) > > > + if type == 'binary': > > > + raw = ip.packed > > > + else: > > > + raw = int(ip) > > > + else: > > > > I wonder if we should raise in this case? > > Especially if type is binary passing the string back will just blow up > > later, right? We could instead rise with a nice clear error message > > here. > > It's actually a bit misleading that the attr is called 'string' > because it could be a binary input if it was supplied from python > code, i.e. not parsed from JSON on the command-line. I was wrong, the binary input is handled already by the caller, so this should just raise an exception.
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index f07a8404f71a..c861c1a7d933 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -536,9 +536,11 @@ class YnlFamily(SpecFamily): try: return int(value) except (ValueError, TypeError) as e: - if 'enum' not in attr_spec: - raise e - return self._encode_enum(attr_spec, value) + if 'enum' in attr_spec: + return self._encode_enum(attr_spec, value) + if attr_spec.display_hint: + return self._from_string(value, attr_spec.display_hint, attr_spec['type']) + raise e def _add_attr(self, space, name, value, search_attrs): try: @@ -571,7 +573,10 @@ class YnlFamily(SpecFamily): if isinstance(value, bytes): attr_payload = value elif isinstance(value, str): - attr_payload = bytes.fromhex(value) + if attr.display_hint: + attr_payload = self._from_string(value, attr.display_hint, attr['type']) + else: + attr_payload = bytes.fromhex(value) elif isinstance(value, dict) and attr.struct_name: attr_payload = self._encode_struct(attr.struct_name, value) else: @@ -899,6 +904,17 @@ class YnlFamily(SpecFamily): formatted = raw return formatted + def _from_string(self, string, display_hint, type): + if display_hint in ['ipv4', 'ipv6']: + ip = ipaddress.ip_address(string) + if type == 'binary': + raw = ip.packed + else: + raw = int(ip) + else: + raw = string + return raw + def handle_ntf(self, decoded): msg = dict() if self.include_raw:
The ynl tool uses display-hint to know when to format IP addresses in printed output, but not to parse IP addresses from --json input. Add support for parsing ipv4 and ipv6 strings. Signed-off-by: Donald Hunter <donald.hunter@gmail.com> --- tools/net/ynl/lib/ynl.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)