[RFC net-next v1] tools: ynl: Add an strace rendering mode to ynl-gen

[RFC net-next v1] tools: ynl: Add an strace rendering mode to ynl-gen

[Donald Hunter]

Add --mode strace to ynl-gen-c.py to generate source files for strace
that teach it to understand how to decode genetlink messages defined
in the spec. I successfully used this to add openvswitch message
decoding to strace as I described in:

https://donaldh.wtf/2023/06/teaching-strace-new-tricks/

It successfully generated ovs_datapath and ovs_vport but ovs_flow
needed manual fixes to fix code ordering and forward declarations.

Limitations:

- Uses a crude mechanism to try and emit functions in the right order
  which fails for ovs_flow
- Outputs all strace sources to stdout or a single file
- Does not use the right semantic strace decoders for e.g. IP or MAC
  addresses because there is no schema information to say what the
  domain type is.

This seems like a useful tool to have as part of the ynl suite since
it lowers the cost of getting good strace support for new netlink
families. But I realise that the generated format is dependent on an
out of tree project. If there is interest in having this in-tree then
I can clean it up and address some of the limitations before
submission.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/ynl-gen-c.py | 286 +++++++++++++++++++++++++++++++++++++
 1 file changed, 286 insertions(+)

diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 71c5e79e877f..efd87d8463ed 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -2268,6 +2268,288 @@ def render_user_family(family, cw, prototype):
     cw.block_end(line=';')
 
 
+def render_strace(family, cw):
+
+    xlat_headers = []
+
+    # xlat for definitions
+
+    defines = []
+    for const in family['definitions']:
+        if const['type'] != 'const':
+            cw.writes_defines(defines)
+            defines = []
+            cw.nl()
+
+        if const['type'] == 'enum' or const['type'] == 'flags':
+            enum = family.consts[const['name']]
+
+            xlat_name = f"{family.name}_{c_lower(const['name'])}"
+            xlat_headers.append(xlat_name)
+
+            cw.p(f"// For src/xlat/{xlat_name}.in")
+            cw.p('#unconditional')
+            if const['type'] == 'enum':
+                cw.p('#value_indexed')
+
+            name_pfx = const.get('name-prefix', f"{family.name}-{const['name']}-")
+            for entry in enum.entries.values():
+                cw.p(entry.c_name)
+
+            cw.nl()
+        elif const['type'] == 'const':
+            defines.append([c_upper(family.get('c-define-name',
+                                               f"{family.name}-{const['name']}")),
+                            const['value']])
+
+    if defines:
+        cw.writes_defines(defines)
+        cw.nl()
+
+    # xlat for attrs
+
+    for _, attr_set in family.attr_sets.items():
+        if attr_set.subset_of:
+            continue
+
+        xlat_name = c_lower(attr_set.yaml['enum-name'])
+        xlat_headers.append(xlat_name)
+
+        cw.p(f"// For src/xlat/{xlat_name}.in")
+        cw.p('#unconditional')
+        cw.p('#value_indexed')
+
+        for _, attr in attr_set.items():
+            cw.p(attr.enum_name)
+        cw.nl()
+
+    # xlat for commands
+
+    separate_ntf = 'async-prefix' in family['operations']
+
+    xlat_name = f"{family.name}_cmds"
+    xlat_headers.append(xlat_name)
+
+    cw.p(f"// For src/xlat/{xlat_name}.in")
+    cw.p('#unconditional')
+    cw.p('#value_indexed')
+
+    for op in family.msgs.values():
+        if separate_ntf and ('notify' in op or 'event' in op):
+            continue
+
+        cw.p(op.enum_name)
+    cw.nl()
+
+    if separate_ntf:
+        uapi_enum_start(family, cw, family['operations'], enum_name='async-enum')
+        for op in family.msgs.values():
+            if separate_ntf and not ('notify' in op or 'event' in op):
+                continue
+
+            suffix = ','
+            if 'value' in op:
+                suffix = f" = {op['value']},"
+            cw.p(op.enum_name + suffix)
+        cw.block_end(line=';')
+        cw.nl()
+
+    cw.nl()
+    if defines:
+        cw.writes_defines(defines)
+        cw.nl()
+
+    # Bind into netlink_generic.(c|h)
+
+    cw.p('// Add to src/netlink_generic.h')
+    cw.p(f"extern DECL_NETLINK_GENERIC_DECODER(decode_{family.name}_msg);")
+    cw.nl()
+
+    cw.p('// Add to src/netlink_generic.c in genl_decoders[]')
+    cw.p(f"""\t{{ "{family.name}", decode_{family.name}_msg }},""")
+    cw.nl()
+
+    # strace Makefile
+
+    cw.p('// Add to src/Makefile.am in libstrace_a_SOURCES')
+    cw.p(f"\t{family.name}.c \\")
+    cw.nl()
+
+    # Start of C source file
+
+    cw.p(f"// For src/{family.name}.c")
+    cw.nl()
+
+    cw.p('#include "defs.h"')
+    cw.p('#include "netlink.h"')
+    cw.p('#include "nlattr.h"')
+    cw.p('#include <linux/genetlink.h>')
+    cw.p(f"#include <{family['uapi-header']}>")
+    cw.p('#include "netlink_generic.h"')
+    for h in xlat_headers:
+        cw.p(f"#include \"xlat/{h}.h\"")
+    cw.nl()
+
+    # C code for flags, enum and struct decoders
+
+    for defn in family['definitions']:
+        if defn['type'] in [ 'flags', 'enum' ]:
+            prefix = defn.get('name-prefix', f"{family.name}-{defn['name']}-")
+
+            cw.p('static bool')
+            cw.p(f"decode_{c_lower(defn['name'])}(struct tcb *const tcp,")
+            cw.p("\t\tconst kernel_ulong_t addr,")
+            cw.p("\t\tconst unsigned int len,")
+            cw.p("\t\tconst void *const opaque_data)")
+            cw.block_start()
+            cw.block_start("static const struct decode_nla_xlat_opts opts =")
+            cw.p(f"""{family.name}_{c_lower(defn['name'])}, "{c_upper(prefix)}???", .size = 4""")
+            cw.block_end(';')
+            decoder = 'xval' if defn['type'] == 'enum' else 'flags'
+            cw.p(f"return decode_nla_{decoder}(tcp, addr, len, &opts);")
+            cw.block_end()
+
+        elif defn['type'] == 'struct':
+            struct_name = c_lower(defn['enum-name'] if 'enum-name' in defn else defn['name'])
+            var_name = c_lower(defn['name'])
+
+            cw.p('static bool')
+            cw.p(f"decode_{struct_name}(struct tcb *const tcp,")
+            cw.p("\t\tconst kernel_ulong_t addr,")
+            cw.p("\t\tconst unsigned int len,")
+            cw.p("\t\tconst void *const opaque_data)")
+            cw.block_start()
+
+            cw.p(f"struct {struct_name} {var_name};")
+            cw.p(f"umove_or_printaddr(tcp, addr, &{var_name});")
+            cw.nl()
+
+            for m in defn['members']:
+                if m['name'].startswith('pad'):
+                    continue
+                cw.p(f"PRINT_FIELD_U({var_name}, {c_lower(m['name'])});")
+                cw.p('tprint_struct_next();')
+
+            cw.p('return true;')
+            cw.block_end()
+
+        cw.nl()
+
+    # C code for attibute set decoders
+
+    for _, attr_set in family.attr_sets.items():
+        if attr_set.subset_of:
+            continue
+
+        # Emit nested attr decoders before referencing them
+
+        for _, attr in attr_set.items():
+            if type(attr) in [ TypeNest, TypeArrayNest ]:
+                decoder = f"decode_{c_lower(attr.enum_name)}"
+                nested_set = family.attr_sets[attr['nested-attributes']]
+                nested_attrs = f"{c_lower(nested_set.yaml['enum-name'])}"
+                name_prefix = nested_set.yaml.get('name-prefix',
+                                                  f"{family.name}-{nested_set.name}-")
+                attr_prefix = f"{c_upper(name_prefix)}"
+                decoder_array = f"{c_lower(nested_set.name)}_attr_decoders"
+                array_nest = "_item" if type(attr) == TypeArrayNest else ""
+
+                cw.p('static bool')
+                cw.p(f"{decoder}{array_nest}(struct tcb *const tcp,")
+                cw.p("\tconst kernel_ulong_t addr,")
+                cw.p("\tconst unsigned int len,")
+                cw.p("\tconst void *const opaque_data)")
+                cw.block_start()
+                cw.p(f"decode_nlattr(tcp, addr, len, {nested_attrs},")
+                cw.p(f"\t\"{attr_prefix}???\",")
+                cw.p(f"\tARRSZ_PAIR({decoder_array}),")
+                cw.p("\tNULL);")
+                cw.p('return true;')
+                cw.block_end()
+                cw.nl()
+
+            if type(attr) == TypeArrayNest:
+                cw.p('static bool')
+                cw.p(f"{decoder}(struct tcb *const tcp,")
+                cw.p("\tconst kernel_ulong_t addr,")
+                cw.p("\tconst unsigned int len,")
+                cw.p("\tconst void *const opaque_data)")
+                cw.block_start()
+                cw.p(f"nla_decoder_t decoder = &{decoder}_item;")
+                cw.p('decode_nlattr(tcp, addr, len, NULL, NULL, &decoder, 0, NULL);')
+                cw.p('return true;')
+                cw.block_end()
+                cw.nl()
+
+        # Then emit the decoders array
+
+        cw.block_start(f"static const nla_decoder_t {c_lower(attr_set.name)}_attr_decoders[] =")
+        for _, attr in attr_set.items():
+            if type(attr) in [ TypeUnused, TypeFlag ]:
+                decoder = 'NULL'
+            elif type(attr) == TypeString:
+                decoder = 'decode_nla_str'
+            elif type(attr) == TypeBinary:
+                decoder = 'NULL'
+                if 'struct' in attr.yaml:
+                    defn = family.consts[attr.yaml['struct']]
+                    enum_name = c_lower(defn.get('enum-name', defn.name))
+                    decoder = f"decode_{enum_name}"
+            elif type(attr) == TypeNest:
+                decoder = f"decode_{c_lower(attr.enum_name)}"
+            elif type(attr) == TypeScalar and 'enum' in attr:
+                decoder = f"decode_{c_lower(attr['enum'])}"
+            else:
+                decoder = f"decode_nla_{attr.type}"
+
+            cw.p(f"[{attr.enum_name}] = {decoder},")
+        cw.block_end(';')
+        cw.nl()
+
+    # C code for top-level decoder
+
+    for op in family.msgs.values():
+        cmd_prefix = c_upper(family.yaml['operations']['name-prefix'])
+        attr_set_name = op.yaml['attribute-set']
+        attr_set = family.attr_sets[attr_set_name]
+        name_prefix = c_upper(attr_set.yaml.get('name-prefix', attr_set_name))
+        enum_name = c_lower(attr_set.yaml['enum-name'])
+
+        cw.block_start(f"DECL_NETLINK_GENERIC_DECODER(decode_{family.name}_msg)")
+
+        if op.fixed_header:
+            defn = family.consts[op.fixed_header]
+            header_name = c_lower(defn['name'])
+            cw.p(f"struct {header_name} header;")
+            cw.p(f"size_t offset = sizeof(struct {header_name});")
+            cw.nl()
+
+        cw.p('tprint_struct_begin();')
+        cw.p(f"""PRINT_FIELD_XVAL(*genl, cmd, {family.name}_cmds, "{cmd_prefix}???");""");
+        cw.p('tprint_struct_next();')
+        cw.p('PRINT_FIELD_U(*genl, version);')
+        cw.p('tprint_struct_next();')
+
+        if op.fixed_header:
+            cw.p('if (umove_or_printaddr(tcp, addr, &header))')
+            cw.p('return;')
+            for m in defn.members:
+                cw.p(f"PRINT_FIELD_U(header, {c_lower(m.name)});")
+                cw.p('tprint_struct_next();')
+            cw.nl()
+            cw.p(f"decode_nlattr(tcp, addr + offset, len - offset,");
+        else:
+            cw.p(f"decode_nlattr(tcp, addr, len,");
+
+        cw.p(f"\t{enum_name},");
+        cw.p(f"\t\"{name_prefix}???\",");
+        cw.p(f"\tARRSZ_PAIR({c_lower(attr_set_name)}_attr_decoders),");
+        cw.p("\tNULL);")
+        cw.p('tprint_struct_end();')
+        cw.block_end()
+        break
+
+
 def find_kernel_root(full_path):
     sub_path = ''
     while True:
@@ -2335,6 +2617,10 @@ def main():
         render_uapi(parsed, cw)
         return
 
+    if args.mode == 'strace':
+        render_strace(parsed, cw)
+        return
+
     hdr_prot = f"_LINUX_{parsed.name.upper()}_GEN_H"
     if args.header:
         cw.p('#ifndef ' + hdr_prot)
-- 
2.39.0

Re: [RFC net-next v1] tools: ynl: Add an strace rendering mode to ynl-gen

[Simon Horman]

On Thu, Jun 15, 2023 at 04:13:36PM +0100, Donald Hunter wrote:
> Add --mode strace to ynl-gen-c.py to generate source files for strace
> that teach it to understand how to decode genetlink messages defined
> in the spec. I successfully used this to add openvswitch message
> decoding to strace as I described in:
> 
> https://donaldh.wtf/2023/06/teaching-strace-new-tricks/
> 
> It successfully generated ovs_datapath and ovs_vport but ovs_flow
> needed manual fixes to fix code ordering and forward declarations.
> 
> Limitations:
> 
> - Uses a crude mechanism to try and emit functions in the right order
>   which fails for ovs_flow
> - Outputs all strace sources to stdout or a single file
> - Does not use the right semantic strace decoders for e.g. IP or MAC
>   addresses because there is no schema information to say what the
>   domain type is.
> 
> This seems like a useful tool to have as part of the ynl suite since
> it lowers the cost of getting good strace support for new netlink
> families. But I realise that the generated format is dependent on an
> out of tree project. If there is interest in having this in-tree then
> I can clean it up and address some of the limitations before
> submission.
> 
> Signed-off-by: Donald Hunter <donald.hunter@gmail.com>

...

> +    # C code for attibute set decoders

Hi Donald,

a minor nit from my side: attibute -> attribute

Re: [RFC net-next v1] tools: ynl: Add an strace rendering mode to ynl-gen

[Jakub Kicinski]

On Thu, 15 Jun 2023 16:13:36 +0100 Donald Hunter wrote:
> Add --mode strace to ynl-gen-c.py to generate source files for strace
> that teach it to understand how to decode genetlink messages defined
> in the spec. I successfully used this to add openvswitch message
> decoding to strace as I described in:
> 
> https://donaldh.wtf/2023/06/teaching-strace-new-tricks/
> 
> It successfully generated ovs_datapath and ovs_vport but ovs_flow
> needed manual fixes to fix code ordering and forward declarations.
> 
> Limitations:
> 
> - Uses a crude mechanism to try and emit functions in the right order
>   which fails for ovs_flow

What's the dependency? I pushed some stuff recently to try to order
types more intelligently but for normal C netlink it still won't deal
with cycles :(

Actually I think that you're using raw family info rather than the
codegen-focused structs, maybe that's why?

> - Outputs all strace sources to stdout or a single file
> - Does not use the right semantic strace decoders for e.g. IP or MAC
>   addresses because there is no schema information to say what the
>   domain type is.

The interpretation depends on another attribute or we expose things 
as binary with no machine-readable indication if its IP addr or MAC etc?

> This seems like a useful tool to have as part of the ynl suite since
> it lowers the cost of getting good strace support for new netlink
> families. But I realise that the generated format is dependent on an
> out of tree project. If there is interest in having this in-tree then
> I can clean it up and address some of the limitations before
> submission.

I think it's fine, we'll have to cross this bridge sooner or later.
I suspect we'll need to split ynl-gen-c once again (like the
tools/net/ynl/lib/nlspec.py, maybe we need another layer for code 
generators? nlcodegen or some such?) before we add codegen for more
languages. I'm not sure you actually need that yet, maybe the strace
generator needs just nlspec.py and it can be a separate script?

Re: [RFC net-next v1] tools: ynl: Add an strace rendering mode to ynl-gen

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Thu, 15 Jun 2023 16:13:36 +0100 Donald Hunter wrote:
>> Add --mode strace to ynl-gen-c.py to generate source files for strace
>> that teach it to understand how to decode genetlink messages defined
>> in the spec. I successfully used this to add openvswitch message
>> decoding to strace as I described in:
>> 
>> https://donaldh.wtf/2023/06/teaching-strace-new-tricks/
>> 
>> It successfully generated ovs_datapath and ovs_vport but ovs_flow
>> needed manual fixes to fix code ordering and forward declarations.
>> 
>> Limitations:
>> 
>> - Uses a crude mechanism to try and emit functions in the right order
>>   which fails for ovs_flow
>
> What's the dependency? I pushed some stuff recently to try to order
> types more intelligently but for normal C netlink it still won't deal
> with cycles :(

For strace I need to emit attr decoder functions before referencing them
in dispatch tables. The crude mechanism I used was to emit decoders for
nested attributes first, which worked okay for e.g. ovs_vport. But
ovs_flow has I think at least 1 cycle.

> Actually I think that you're using raw family info rather than the
> codegen-focused structs, maybe that's why?

Yes, that's a fair point. I'm just walking through the declared
attribute-sets in the order defined in the schema. I can take a look at
what the codegen-focused structs provide.

>> - Outputs all strace sources to stdout or a single file
>> - Does not use the right semantic strace decoders for e.g. IP or MAC
>>   addresses because there is no schema information to say what the
>>   domain type is.
>
> The interpretation depends on another attribute or we expose things 
> as binary with no machine-readable indication if its IP addr or MAC etc?

Yeah, it's the lack of machine-readable indication. I'd suggest adding
something like 'format: ipv4-address' to the schema.

>> This seems like a useful tool to have as part of the ynl suite since
>> it lowers the cost of getting good strace support for new netlink
>> families. But I realise that the generated format is dependent on an
>> out of tree project. If there is interest in having this in-tree then
>> I can clean it up and address some of the limitations before
>> submission.
>
> I think it's fine, we'll have to cross this bridge sooner or later.
> I suspect we'll need to split ynl-gen-c once again (like the
> tools/net/ynl/lib/nlspec.py, maybe we need another layer for code 
> generators? nlcodegen or some such?) before we add codegen for more
> languages. I'm not sure you actually need that yet, maybe the strace
> generator needs just nlspec.py and it can be a separate script?

The strace generator uses CodeWriter and makes partial use of the Type*
classes as well. If we split those out of ynl-gen-c then it could be a
separate script. A first step could be to move all but main() into a
lib?

Re: [RFC net-next v1] tools: ynl: Add an strace rendering mode to ynl-gen

[Jakub Kicinski]

On Fri, 16 Jun 2023 11:17:25 +0100 Donald Hunter wrote:
> Jakub Kicinski <kuba@kernel.org> writes:
> 
> > On Thu, 15 Jun 2023 16:13:36 +0100 Donald Hunter wrote:  
> >> Add --mode strace to ynl-gen-c.py to generate source files for strace
> >> that teach it to understand how to decode genetlink messages defined
> >> in the spec. I successfully used this to add openvswitch message
> >> decoding to strace as I described in:
> >> 
> >> https://donaldh.wtf/2023/06/teaching-strace-new-tricks/
> >> 
> >> It successfully generated ovs_datapath and ovs_vport but ovs_flow
> >> needed manual fixes to fix code ordering and forward declarations.
> >> 
> >> Limitations:
> >> 
> >> - Uses a crude mechanism to try and emit functions in the right order
> >>   which fails for ovs_flow  
> >
> > What's the dependency? I pushed some stuff recently to try to order
> > types more intelligently but for normal C netlink it still won't deal
> > with cycles :(  
> 
> For strace I need to emit attr decoder functions before referencing them
> in dispatch tables. The crude mechanism I used was to emit decoders for
> nested attributes first, which worked okay for e.g. ovs_vport. But
> ovs_flow has I think at least 1 cycle.
> 
> > Actually I think that you're using raw family info rather than the
> > codegen-focused structs, maybe that's why?  
> 
> Yes, that's a fair point. I'm just walking through the declared
> attribute-sets in the order defined in the schema. I can take a look at
> what the codegen-focused structs provide.
> 
> >> - Outputs all strace sources to stdout or a single file
> >> - Does not use the right semantic strace decoders for e.g. IP or MAC
> >>   addresses because there is no schema information to say what the
> >>   domain type is.  
> >
> > The interpretation depends on another attribute or we expose things 
> > as binary with no machine-readable indication if its IP addr or MAC etc?  
> 
> Yeah, it's the lack of machine-readable indication. I'd suggest adding
> something like 'format: ipv4-address' to the schema.

I'd prefer to avoid defining our own names, too much thinking :)
Two ideas which come to mind are to either add a struct reference
(struct in_addr, in this case) or use the printk formats
Documentation/core-api/printk-formats.rst (%pI4).

> >> This seems like a useful tool to have as part of the ynl suite since
> >> it lowers the cost of getting good strace support for new netlink
> >> families. But I realise that the generated format is dependent on an
> >> out of tree project. If there is interest in having this in-tree then
> >> I can clean it up and address some of the limitations before
> >> submission.  
> >
> > I think it's fine, we'll have to cross this bridge sooner or later.
> > I suspect we'll need to split ynl-gen-c once again (like the
> > tools/net/ynl/lib/nlspec.py, maybe we need another layer for code 
> > generators? nlcodegen or some such?) before we add codegen for more
> > languages. I'm not sure you actually need that yet, maybe the strace
> > generator needs just nlspec.py and it can be a separate script?  
> 
> The strace generator uses CodeWriter and makes partial use of the Type*
> classes as well. If we split those out of ynl-gen-c then it could be a
> separate script. A first step could be to move all but main() into a
> lib?

Hm, my instinct was the opposite, move as little as possible while
avoiding duplication. I was thinking about the split in context of
C++ and Rust, there's a lot of C intermixed with the code currently
in ynl-gen-c. But you need the C, AFAIU.

You shouldn't need all the print_ stuff, tho, do you? So we could split
more or less around where _C_KW is defined? Anything above it would be
shared among C codegens?

Re: [RFC net-next v1] tools: ynl: Add an strace rendering mode to ynl-gen

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Fri, 16 Jun 2023 11:17:25 +0100 Donald Hunter wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>> 
>> > On Thu, 15 Jun 2023 16:13:36 +0100 Donald Hunter wrote:  
>> >
>> > The interpretation depends on another attribute or we expose things 
>> > as binary with no machine-readable indication if its IP addr or MAC etc?  
>> 
>> Yeah, it's the lack of machine-readable indication. I'd suggest adding
>> something like 'format: ipv4-address' to the schema.
>
> I'd prefer to avoid defining our own names, too much thinking :)
> Two ideas which come to mind are to either add a struct reference
> (struct in_addr, in this case) or use the printk formats
> Documentation/core-api/printk-formats.rst (%pI4).

I tried these suggestions out and they seem a bit problematic. For
struct references I don't see a way to validate them, when it's not C
codegen. Non C consumers will need to enumarete the struct references
they 'understand'. The printk formats are meaningful in kernel, but not
directly usable elsewhere, without writing a parser for them.

It seems desirable to have schema validation for the values and I tried
using the %p printk formats as the enumeration. Using this format, the
values need to be quoted everywhere. See diff below.

The printk formats also carry specific opinions about formatting details
such as the case and separator to be used for output. This seems
orthogonal to a type annotation about meaning.

Perhaps the middle ground is to derive a list of format specificer
enumerations from the printk formats, but that's maybe not much
different from defining our own?

I currently have "%pI4", "%pI6", "%pM", "%pMF", "%pU", "%ph", which
could be represented as ipv4, ipv6, mac, fddi, uuid, hex. From the
printk formats documentation, the only other one I can see is bluetooth.
The other formats all look like they cover composite values.

>> >> This seems like a useful tool to have as part of the ynl suite since
>> >> it lowers the cost of getting good strace support for new netlink
>> >> families. But I realise that the generated format is dependent on an
>> >> out of tree project. If there is interest in having this in-tree then
>> >> I can clean it up and address some of the limitations before
>> >> submission.  
>> >
>> > I think it's fine, we'll have to cross this bridge sooner or later.
>> > I suspect we'll need to split ynl-gen-c once again (like the
>> > tools/net/ynl/lib/nlspec.py, maybe we need another layer for code 
>> > generators? nlcodegen or some such?) before we add codegen for more
>> > languages. I'm not sure you actually need that yet, maybe the strace
>> > generator needs just nlspec.py and it can be a separate script?  
>> 
>> The strace generator uses CodeWriter and makes partial use of the Type*
>> classes as well. If we split those out of ynl-gen-c then it could be a
>> separate script. A first step could be to move all but main() into a
>> lib?
>
> Hm, my instinct was the opposite, move as little as possible while
> avoiding duplication. I was thinking about the split in context of
> C++ and Rust, there's a lot of C intermixed with the code currently
> in ynl-gen-c. But you need the C, AFAIU.
>
> You shouldn't need all the print_ stuff, tho, do you? So we could split
> more or less around where _C_KW is defined? Anything above it would be
> shared among C codegens?

Yeah, that makes sense - all the top-level defs after _C_KW are specific
to main() so shouldn't be moved to a lib.

I will do an initial refactor and see how it works out.

Thanks.

diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index b474889b49ff..f3ecdeb7c38c 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -119,7 +119,11 @@ properties:
               name:
                 type: string
               type:
                 enum: [ u8, u16, u32, u64, s8, s16, s32, s64, string ]
+              format-specifier: &format-specifier
+                description: Optional format specifier for an attribute
+                enum: [ "%pI4", "%pI6", "%pM", "%pMF", "%pU", "%ph" ]
               len:
                 $ref: '#/$defs/len-or-define'
               byte-order:
@@ -179,8 +183,10 @@ properties:
               name:
                 type: string
               type: &attr-type
+                description: The netlink attribute type
                 enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64,
                         string, nest, array-nest, nest-type-value ]
+              format-specifier: *format-specifier
               doc:
                 description: Documentation of the attribute.
                 type: string
diff --git a/Documentation/netlink/specs/ovs_flow.yaml b/Documentation/netlink/specs/ovs_flow.yaml
index 1ecbcd117385..08b2918baa27 100644
--- a/Documentation/netlink/specs/ovs_flow.yaml
+++ b/Documentation/netlink/specs/ovs_flow.yaml
@@ -33,6 +33,20 @@ definitions:
         name: n-bytes
         type: u64
         doc: Number of matched bytes.
+  -
+    name: ovs-key-ethernet
+    type: struct
+    members:
+      -
+        name: eth-src
+        type: binary
+        len: 6
+        format-specifier: "%pM"
+      -
+        name: eth-dst
+        type: binary
+        len: 6
+        format-specifier: "%pM"
   -
     name: ovs-key-mpls
     type: struct
@@ -49,10 +63,12 @@ definitions:
         name: ipv4-src
         type: u32
         byte-order: big-endian
+        format-specifier: "%pI4"
       -
         name: ipv4-dst
         type: u32
         byte-order: big-endian
+        format-specifier: "%pI4"
       -
         name: ipv4-proto
         type: u8

Re: [RFC net-next v1] tools: ynl: Add an strace rendering mode to ynl-gen

[Jakub Kicinski]

On Mon, 19 Jun 2023 11:04:11 +0100 Donald Hunter wrote:
> I tried these suggestions out and they seem a bit problematic. For
> struct references I don't see a way to validate them, when it's not C
> codegen. Non C consumers will need to enumarete the struct references
> they 'understand'. The printk formats are meaningful in kernel, but not
> directly usable elsewhere, without writing a parser for them.
> 
> It seems desirable to have schema validation for the values and I tried
> using the %p printk formats as the enumeration. Using this format, the
> values need to be quoted everywhere. See diff below.
> 
> The printk formats also carry specific opinions about formatting details
> such as the case and separator to be used for output. This seems
> orthogonal to a type annotation about meaning.
> 
> Perhaps the middle ground is to derive a list of format specificer
> enumerations from the printk formats, but that's maybe not much
> different from defining our own?

Fair point. Our own names would be easier to understand -- OTOH I like
how the print formats almost forcefully drive the point that these are
supposed to be used exclusively for printing. 

If someone needs to interpret the data they should add a struct.

But I guess a big fat warning above the documentation and calling the
attribute "print-format" / "print-hint" could work as well? Up to you.

Hope this makes sense.

> I currently have "%pI4", "%pI6", "%pM", "%pMF", "%pU", "%ph", which
> could be represented as ipv4, ipv6, mac, fddi, uuid, hex. From the
> printk formats documentation, the only other one I can see is bluetooth.
> The other formats all look like they cover composite values.

> diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
> index b474889b49ff..f3ecdeb7c38c 100644
> --- a/Documentation/netlink/genetlink-legacy.yaml
> +++ b/Documentation/netlink/genetlink-legacy.yaml

If we're only talking about printing we will want to extend the support
to new families as well.

Re: [RFC net-next v1] tools: ynl: Add an strace rendering mode to ynl-gen

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Mon, 19 Jun 2023 11:04:11 +0100 Donald Hunter wrote:
>> I tried these suggestions out and they seem a bit problematic. For
>> struct references I don't see a way to validate them, when it's not C
>> codegen. Non C consumers will need to enumarete the struct references
>> they 'understand'. The printk formats are meaningful in kernel, but not
>> directly usable elsewhere, without writing a parser for them.
>> 
>> It seems desirable to have schema validation for the values and I tried
>> using the %p printk formats as the enumeration. Using this format, the
>> values need to be quoted everywhere. See diff below.
>> 
>> The printk formats also carry specific opinions about formatting details
>> such as the case and separator to be used for output. This seems
>> orthogonal to a type annotation about meaning.
>> 
>> Perhaps the middle ground is to derive a list of format specificer
>> enumerations from the printk formats, but that's maybe not much
>> different from defining our own?
>
> Fair point. Our own names would be easier to understand -- OTOH I like
> how the print formats almost forcefully drive the point that these are
> supposed to be used exclusively for printing. 
>
> If someone needs to interpret the data they should add a struct.
>
> But I guess a big fat warning above the documentation and calling the
> attribute "print-format" / "print-hint" could work as well? Up to you.
>
> Hope this makes sense.

Does "display-hint" sound okay? Maybe me being a bit fussy vs
"print-hint" but it feels more appropriate to me.

>> I currently have "%pI4", "%pI6", "%pM", "%pMF", "%pU", "%ph", which
>> could be represented as ipv4, ipv6, mac, fddi, uuid, hex. From the
>> printk formats documentation, the only other one I can see is bluetooth.
>> The other formats all look like they cover composite values.
>
>> diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
>> index b474889b49ff..f3ecdeb7c38c 100644
>> --- a/Documentation/netlink/genetlink-legacy.yaml
>> +++ b/Documentation/netlink/genetlink-legacy.yaml
>
> If we're only talking about printing we will want to extend the support
> to new families as well.

Yep, makes sense. Is there any magic/scripted way of keeping the
different schemas in sync or do they just get modified independently?

Re: [RFC net-next v1] tools: ynl: Add an strace rendering mode to ynl-gen

[Jakub Kicinski]

On Fri, 23 Jun 2023 13:04:32 +0100 Donald Hunter wrote:
> > Fair point. Our own names would be easier to understand -- OTOH I like
> > how the print formats almost forcefully drive the point that these are
> > supposed to be used exclusively for printing. 
> >
> > If someone needs to interpret the data they should add a struct.
> >
> > But I guess a big fat warning above the documentation and calling the
> > attribute "print-format" / "print-hint" could work as well? Up to you.
> >
> > Hope this makes sense.  
> 
> Does "display-hint" sound okay? Maybe me being a bit fussy vs
> "print-hint" but it feels more appropriate to me.

Sounds good.

> > If we're only talking about printing we will want to extend the support
> > to new families as well.  
> 
> Yep, makes sense. Is there any magic/scripted way of keeping the
> different schemas in sync or do they just get modified independently?

Nope :(