Re: [PATCH net-next] netlink: specs: add ctnetlink dump and stats dump support

[Donald Hunter <donald.hunter@gmail.com>]

Florian Westphal <fw@strlen.de> writes:

> This adds support to dump the connection tracking table
> ("conntrack -L") and the conntrack statistics, ("conntrack -S").
>
> Example conntrack dump:
> tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/ctnetlink.yaml --dump ctnetlink-get
> [{'id': 59489769,
>   'mark': 0,
>   'nfgen-family': 2,
>   'protoinfo': {'protoinfo-tcp': {'tcp-flags-original': {'flags': {'maxack',
>                                                                    'sack-perm',
>                                                                    'window-scale'},
>                                                          'mask': set()},
>                                   'tcp-flags-reply': {'flags': {'maxack',
>                                                                 'sack-perm',
>                                                                 'window-scale'},
>                                                       'mask': set()},
>                                   'tcp-state': 'established',
>                                   'tcp-wscale-original': 7,
>                                   'tcp-wscale-reply': 8}},
>   'res-id': 0,
>   'secctx': {'secctx-name': 'system_u:object_r:unlabeled_t:s0'},
>   'status': {'assured',
>              'confirmed',
>              'dst-nat-done',
>              'seen-reply',
>              'src-nat-done'},
>   'timeout': 431949,
>   'tuple-orig': {'tuple-ip': {'ip-v4-dst': '34.107.243.93',
>                               'ip-v4-src': '192.168.0.114'},
>                  'tuple-proto': {'proto-dst-port': 443,
>                                  'proto-num': 6,
>                                  'proto-src-port': 37104}},
>   'tuple-reply': {'tuple-ip': {'ip-v4-dst': '192.168.0.114',
>                                'ip-v4-src': '34.107.243.93'},
>                   'tuple-proto': {'proto-dst-port': 37104,
>                                   'proto-num': 6,
>                                   'proto-src-port': 443}},
>   'use': 1,
>   'version': 0},
>  {'id': 3402229480,
>
> Example stats dump:
> tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/ctnetlink.yaml --dump ctnetlink-stats-get
> [{'chain-toolong': 0,
>   'clash-resolve': 3,
>   'drop': 0,
>  ....
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Documentation/netlink/specs/ctnetlink.yaml | 582 +++++++++++++++++++++
>  1 file changed, 582 insertions(+)
>  create mode 100644 Documentation/netlink/specs/ctnetlink.yaml

Can you change the filename to conntrack.yaml so that it matches the
family name. This helps ./tools/net/ynl/pyynl/cli.py --list-families
which is based on the filenames. It's also redundant to say netlink in
the filename since it is in the netlink/specs directory.

...

> +attribute-sets:
> +  -
> +    name: ctnetlink-counter-attrs

Not sure the ctnetlink- prefix is needed in all the attribute-set names.
I'd suggest keeping a prefix only for the toplevel attriubtes but change
the prefix to conntrack- for consistency.

...

> +  -
> +    name: ctnetlink-attrs
> +    attributes:

...

> +operations:
> +  enum-model: directional
> +  list:
> +    -
> +      name: ctnetlink-get

My preference is to drop the ctnetlink- prefix from the op names, to be
consistent with the other netlink specs.

> +      doc: get / dump entries
> +      attribute-set: ctnetlink-attrs
> +      fixed-header: nfgenmsg
> +      do:
> +        request:
> +          value: 0x101
> +          attributes:
> +            - name
> +        reply:
> +          value: 0x100
> +          attributes:
> +            - name

The usage is not specified correctly. You give a dump example so there
should be a dump: definition. The reply attributes should be enumerated.
If do: is supported then the request attributes should be enumerated.

Same for stats-get below.

> +    -
> +      name: ctnetlink-stats-get
> +      doc: dump pcpu conntrack stats
> +      attribute-set: ctnetlink-stats-attrs
> +      fixed-header: nfgenmsg
> +      do:
> +        request:
> +          value: 0x104
> +          attributes:
> +            - name
> +        reply:
> +          value: 0x104
> +          attributes:
> +            - name
> +

Thanks,
Donald.