[PATCH net-next 3/7] ynl: support directional specs in ynl-gen-c.py

Stanislav Fomichev posted 7 patches 1 week, 2 days ago
There is a newer version of this series
[PATCH net-next 3/7] ynl: support directional specs in ynl-gen-c.py
Posted by Stanislav Fomichev 1 week, 2 days ago
The intent is to generate ethtool uapi headers. For now, some of the
things are hard-coded:
- <FAMILY>_MSG_{USER,KERNEL}_MAX
- the split between USER and KERNEL messages

Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/net/ynl/ynl-gen-c.py | 116 +++++++++++++++++++++++++++----------
 1 file changed, 85 insertions(+), 31 deletions(-)

diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 0de918c7f18d..a7b7aea1159b 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -2416,6 +2416,87 @@ _C_KW = {
     cw.block_start(line=start_line)
 
 
+def render_uapi_unified(family, cw, max_by_define, separate_ntf):
+    max_name = c_upper(family.get('cmd-max-name', f"{family.op_prefix}MAX"))
+    cnt_name = c_upper(family.get('cmd-cnt-name', f"__{family.op_prefix}MAX"))
+    max_value = f"({cnt_name} - 1)"
+
+    uapi_enum_start(family, cw, family['operations'], 'enum-name')
+    val = 0
+    for op in family.msgs.values():
+        if separate_ntf and ('notify' in op or 'event' in op):
+            continue
+
+        suffix = ','
+        if op.value != val:
+            suffix = f" = {op.value},"
+            val = op.value
+        cw.p(op.enum_name + suffix)
+        val += 1
+    cw.nl()
+    cw.p(cnt_name + ('' if max_by_define else ','))
+    if not max_by_define:
+        cw.p(f"{max_name} = {max_value}")
+    cw.block_end(line=';')
+    if max_by_define:
+        cw.p(f"#define {max_name} {max_value}")
+    cw.nl()
+
+
+def render_uapi_directional(family, cw, max_by_define):
+    max_name = f"{family.op_prefix}USER_MAX"
+    cnt_name = f"__{family.op_prefix}USER_CNT"
+    max_value = f"({cnt_name} - 1)"
+
+    cw.block_start(line='enum')
+    cw.p(c_upper(f'{family.name}_MSG_USER_NONE = 0,'))
+    val = 0
+    for op in family.msgs.values():
+        if 'do' in op and 'event' not in op:
+            suffix = ','
+            if op.value and op.value != val:
+                suffix = f" = {op.value},"
+                val = op.value
+            cw.p(op.enum_name + suffix)
+            val += 1
+    cw.nl()
+    cw.p(cnt_name + ('' if max_by_define else ','))
+    if not max_by_define:
+        cw.p(f"{max_name} = {max_value}")
+    cw.block_end(line=';')
+    if max_by_define:
+        cw.p(f"#define {max_name} {max_value}")
+    cw.nl()
+
+    max_name = f"{family.op_prefix}KERNEL_MAX"
+    cnt_name = f"__{family.op_prefix}KERNEL_CNT"
+    max_value = f"({cnt_name} - 1)"
+
+    cw.block_start(line='enum')
+    cw.p(c_upper(f'{family.name}_MSG_KERNEL_NONE = 0,'))
+    val = 0
+    for op in family.msgs.values():
+        if ('do' in op and 'reply' in op['do']) or 'notify' in op or 'event' in op:
+            enum_name = op.enum_name
+            if 'event' not in op and 'notify' not in op:
+                enum_name = f'{enum_name}_REPLY'
+
+            suffix = ','
+            if op.value and op.value != val:
+                suffix = f" = {op.value},"
+                val = op.value
+            cw.p(enum_name + suffix)
+            val += 1
+    cw.nl()
+    cw.p(cnt_name + ('' if max_by_define else ','))
+    if not max_by_define:
+        cw.p(f"{max_name} = {max_value}")
+    cw.block_end(line=';')
+    if max_by_define:
+        cw.p(f"#define {max_name} {max_value}")
+    cw.nl()
+
+
 def render_uapi(family, cw):
     hdr_prot = f"_UAPI_LINUX_{c_upper(family.uapi_header_name)}_H"
     cw.p('#ifndef ' + hdr_prot)
@@ -2519,30 +2600,10 @@ _C_KW = {
     # Commands
     separate_ntf = 'async-prefix' in family['operations']
 
-    max_name = c_upper(family.get('cmd-max-name', f"{family.op_prefix}MAX"))
-    cnt_name = c_upper(family.get('cmd-cnt-name', f"__{family.op_prefix}MAX"))
-    max_value = f"({cnt_name} - 1)"
-
-    uapi_enum_start(family, cw, family['operations'], 'enum-name')
-    val = 0
-    for op in family.msgs.values():
-        if separate_ntf and ('notify' in op or 'event' in op):
-            continue
-
-        suffix = ','
-        if op.value != val:
-            suffix = f" = {op.value},"
-            val = op.value
-        cw.p(op.enum_name + suffix)
-        val += 1
-    cw.nl()
-    cw.p(cnt_name + ('' if max_by_define else ','))
-    if not max_by_define:
-        cw.p(f"{max_name} = {max_value}")
-    cw.block_end(line=';')
-    if max_by_define:
-        cw.p(f"#define {max_name} {max_value}")
-    cw.nl()
+    if family.msg_id_model == 'unified':
+        render_uapi_unified(family, cw, max_by_define, separate_ntf)
+    else:
+        render_uapi_directional(family, cw, max_by_define)
 
     if separate_ntf:
         uapi_enum_start(family, cw, family['operations'], enum_name='async-enum')
@@ -2666,13 +2727,6 @@ _C_KW = {
         os.sys.exit(1)
         return
 
-    supported_models = ['unified']
-    if args.mode in ['user', 'kernel']:
-        supported_models += ['directional']
-    if parsed.msg_id_model not in supported_models:
-        print(f'Message enum-model {parsed.msg_id_model} not supported for {args.mode} generation')
-        os.sys.exit(1)
-
     cw = CodeWriter(BaseNlLib(), args.out_file, overwrite=(not args.cmp_out))
 
     _, spec_kernel = find_kernel_root(args.spec)
-- 
2.47.0
Re: [PATCH net-next 3/7] ynl: support directional specs in ynl-gen-c.py
Posted by Jakub Kicinski 1 week, 2 days ago
On Wed, 13 Nov 2024 10:10:19 -0800 Stanislav Fomichev wrote:
> -    supported_models = ['unified']
> -    if args.mode in ['user', 'kernel']:
> -        supported_models += ['directional']
> -    if parsed.msg_id_model not in supported_models:
> -        print(f'Message enum-model {parsed.msg_id_model} not supported for {args.mode} generation')
> -        os.sys.exit(1)

Don't we still need to validate that it's one of the two options?
Re: [PATCH net-next 3/7] ynl: support directional specs in ynl-gen-c.py
Posted by Stanislav Fomichev 1 week, 2 days ago
On 11/13, Jakub Kicinski wrote:
> On Wed, 13 Nov 2024 10:10:19 -0800 Stanislav Fomichev wrote:
> > -    supported_models = ['unified']
> > -    if args.mode in ['user', 'kernel']:
> > -        supported_models += ['directional']
> > -    if parsed.msg_id_model not in supported_models:
> > -        print(f'Message enum-model {parsed.msg_id_model} not supported for {args.mode} generation')
> > -        os.sys.exit(1)
> 
> Don't we still need to validate that it's one of the two options?

I removed it because I'm assuming only two modes exist (and we support
them both now). Are you suggesting it's better to future-proof it and
still keep the check in case we add some new modes in the future? (or
running against some rogue specs?)
Re: [PATCH net-next 3/7] ynl: support directional specs in ynl-gen-c.py
Posted by Jakub Kicinski 1 week, 2 days ago
On Wed, 13 Nov 2024 15:45:21 -0800 Stanislav Fomichev wrote:
> On 11/13, Jakub Kicinski wrote:
> > On Wed, 13 Nov 2024 10:10:19 -0800 Stanislav Fomichev wrote:  
> > > -    supported_models = ['unified']
> > > -    if args.mode in ['user', 'kernel']:
> > > -        supported_models += ['directional']
> > > -    if parsed.msg_id_model not in supported_models:
> > > -        print(f'Message enum-model {parsed.msg_id_model} not supported for {args.mode} generation')
> > > -        os.sys.exit(1)  
> > 
> > Don't we still need to validate that it's one of the two options?  
> 
> I removed it because I'm assuming only two modes exist (and we support
> them both now). Are you suggesting it's better to future-proof it and
> still keep the check in case we add some new modes in the future? (or
> running against some rogue specs?)

TBH I don't remember how much precedent there is for C codegen
depending on jsonschema for spec input validation. My gut tells
me to do:

+    if family.msg_id_model == 'unified':
+        render_uapi_unified(family, cw, max_by_define, separate_ntf)
+    elif family.msg_id_model == 'directional':
+        render_uapi_directional(family, cw, max_by_define)
+    else:
+        raise ..

and then we can indeed drop the validation of the arg directly