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