netdev - Re: [PATCH net-next v3 3/8] net: add basic C code generators for Netlink

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: <ddcea8b3cb8c2d218a2747a1e2f566dbaaee8f01.camel@sipsolutions.net>
Date:   Thu, 19 Jan 2023 21:53:12 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
        robh@...nel.org, stephen@...workplumber.org,
        ecree.xilinx@...il.com, sdf@...gle.com, f.fainelli@...il.com,
        fw@...len.de, linux-doc@...r.kernel.org, razor@...ckwall.org,
        nicolas.dichtel@...nd.com
Subject: Re: [PATCH net-next v3 3/8] net: add basic C code generators for
 Netlink

On Wed, 2023-01-18 at 16:36 -0800, Jakub Kicinski wrote:
> 
> +class BaseNlLib:
> +    def __init__(self):
> +        pass


That __init__ seems pointless?

> +        self.name = attr['name'].replace('-', '_')

You have a _lot_ of these ".replace('-', '_')" invocations - maybe make
a function just like you have c_upper()/c_lower()?

> +        if self.presence_type() == 'len':
> +            pfx = '__' if space == 'user' else ''

this is how you define pfx

> +        if member:
> +            ptr = ''
> +            if self. is_multi_val():
> +                ptr = '*'

but this is how you define ptr - feels a bit inconsistent

Also note the extra space after "self." there

> +        if not self.is_multi_val():
> +            ri.cw.p(f"if (ynl_attr_validate(yarg, attr))")
> +            ri.cw.p(f"return MNL_CB_ERROR;")

Those didn't need f-strings.

Does this "ri.cw.p()" do indentation for the code?


> +    def attr_policy(self, cw):
> +        if 'unterminated-ok' in self.checks and self.checks['unterminated-ok']:

maybe

  if self.checks.get('unterminated-ok', 0):

> +class TypeBinary(Type):
> +    def arg_member(self, ri):
> +        return [f"const void *{self.c_name}", 'size_t len']
> +
> +    def presence_type(self):
> +        return 'len'
> +
> +    def struct_member(self, ri):
> +        ri.cw.p(f"void *{self.c_name};")
> +
> +    def _attr_typol(self):
> +        return f'.type = YNL_PT_BINARY,'
> +
> +    def _attr_policy(self, policy):
> +        mem = '{ '
> +        if len(self.checks) == 1 and 'min-len' in self.checks:
> +            mem += '.len = ' + str(self.checks['min-len'])

Why does the len(self.checks) matter?

> +        elif len(self.checks) == 0:
> +            mem += '.type = NLA_BINARY'
> +        else:
> +            raise Exception('Binary type not implemented, yet')

to get to that error messsage? The error messsage seems a bit misleading
too though. It's that 'max-len' isn't implemented, or such, no?

> +    def _complex_member_type(self, ri):
> +        if 'type' not in self.attr or self.attr['type'] == 'nest':

could do

  if self.attr.get('type', 'nest') == 'nest':

again like above

> +            return f"struct {self.nested_render_name}"
> +        elif self.attr['type'] in scalars:
> +            scalar_pfx = '__' if ri.ku_space == 'user' else ''

You also have this

	... = '__' if ... == 'user' else ''

quite a few times, maybe add something like

def qualify_user(value, user):
    if user == 'user':
        return '__' + value
    return value

> +            return scalar_pfx + self.attr['type']

and then this is just

             return qualify_user(self.attr['type'], ri.ku_space)

instead of the two lines

> +    def free_needs_iter(self):
> +        return 'type' not in self.attr or self.attr['type'] == 'nest'
> +
> +    def free(self, ri, var, ref):
> +        if 'type' not in self.attr or self.attr['type'] == 'nest':

two more places that could use the .get trick

but it's really up to you. Just that the line like that seems rather
long to me :-)

There are many more below but I'll stop commenting.

> +        if self.c_name in c_kw:
> +            self.c_name += '_'

You also have this pattern quite a lot. Maybe a "c_safe()" wrapper or
something :)

FWIW, I'm still looking for "c_kw", maybe "pythonically" that should be
"_C_KW" since it's a private global? I think? Haven't seen it yet :)


> +c_kw = {
> +    'do'
> +}


Aha, here it is :-)

> +            if has_ntf:
> +                cw.p('// --------------- Common notification parsing --------------- //')

You said you were using /* */ comments now but this is still there.

> +                print_ntf_parse_prototype(parsed, cw)
> +            cw.nl()
> +        else:
> +            cw.p('// Policies')

and here too, etc.

Whew. I think I skipped some bits ;-)

Doesn't look that bad overall, IMHO. :)

johannes