Date: Mon, 19 Jun 2023 11:04:11 +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>,
  donald.hunter@...hat.com
Subject: Re: [RFC net-next v1] tools: ynl: Add an strace rendering mode to
 ynl-gen

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

> On Fri, 16 Jun 2023 11:17:25 +0100 Donald Hunter wrote:
>> Jakub Kicinski <kuba@...nel.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

Powered by blists - more mailing lists