netdev - Re: [patch net-next v1 1/2] tools: ynl: Use dict of predefined Structs to decode scalar types

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <m2pm6rlapr.fsf@gmail.com>
Date: Tue, 23 May 2023 09:21:20 +0100
From: Donald Hunter <donald.hunter@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org,  "David S. Miller" <davem@...emloft.net>,  Eric
 Dumazet <edumazet@...gle.com>,  Paolo Abeni <pabeni@...hat.com>,  Jonathan
 Corbet <corbet@....net>,  linux-doc@...r.kernel.org,
  donald.hunter@...hat.com
Subject: Re: [patch net-next v1 1/2] tools: ynl: Use dict of predefined
 Structs to decode scalar types

Jakub Kicinski <kuba@...nel.org> writes:

> On Sun, 21 May 2023 18:07:32 +0100 Donald Hunter wrote:
>> Use a dict of predefined Struct() objects to decode scalar types in native,
>> big or little endian format. This removes the repetitive code for the
>> scalar variants and ensures all the signed variants are supported.
>
>> @@ -115,17 +116,17 @@ class NlAttr:
>>          return self.raw
>>  
>>      def as_c_array(self, type):
>> -        format, _ = self.type_formats[type]
>> -        return list({ x[0] for x in struct.iter_unpack(format, self.raw) })
>> +        format = self.get_format(type)
>> +        return list({ x[0] for x in format.iter_unpack(self.raw) })
>
> I probably asked about this before, and maybe not the question 
> for this series but - why list({ ... }) and not [...]?

It looks like I cargo-culted something there, and it's just plain
wrong. Reading it now, it's clearly a set comprehension coerced into a
list, which could well change ordering.

I'll fix this in the next version.

>>          else:
>> -            raise Exception(f'Unknown type at {space} {name} {value} {attr["type"]}')
>> +            try:
>> +                format = NlAttr.get_format(attr['type'], attr.byte_order)
>> +                attr_payload = format.pack(int(value))
>> +            except:
>> +                raise Exception(f'Unknown type at {space} {name} {value} {attr["type"]}')
>
> Could we do:
>
> 	elif attr["type"] in NlAttr.type_formats:
>
> instead? Maybe my C brain treats exceptions as too exceptional..

Good suggestion, that's much cleaner.

>> +            elif attr_spec["type"]:
>> +                try:
>> +                    decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order)
>> +                except:
>> +                    raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
>
> Same here.

Ack.

> Nice cleanup!