On Fri, 20 Jan 2023 18:16:50 +0100 Lorenzo Bianconi wrote:
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
>
> Add a Netlink spec-compatible family for netdevs.
> This is a very simple implementation without much
> thought going into it.
>
> It allows us to reap all the benefits of Netlink specs,
> one can use the generic client to issue the commands:
>
> $ ./gen.py --spec netdev.yaml --do dev_get --json='{"ifindex": 2}'
> {'ifindex': 2, 'xdp-features': 31}
>
> $ ./gen.py --spec netdev.yaml --dump dev_get
> [{'ifindex': 1, 'xdp-features': 0}, {'ifindex': 2, 'xdp-features': 31}]
In the meantime I added support for rendering enums in Python.
So you can show names in the example. eg:
$ ./cli.py --spec netdev.yaml --dump dev_get
[{'ifindex': 1, 'xdp-features': set()},
{'ifindex': 2,
'xdp-features': {'ndo-xmit', 'pass', 'redirect', 'aborted', 'drop'}},
{'ifindex': 3, 'xdp-features': {'rx-sg'}}]
> the generic python library does not have flags-by-name
> support, yet, but we also don't have to carry strings
> in the messages, as user space can get the names from
> the spec.
>
> Co-developed-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> Co-developed-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> Co-developed-by: Marek Majtyka <alardam@xxxxxxxxx>
> Signed-off-by: Marek Majtyka <alardam@xxxxxxxxx>
> Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>
> ---
> Documentation/netlink/specs/netdev.yaml | 72 ++++++++++
FWIW I'm not 100% sure if we should scope the family to all of netdev
or just xdp. Same for the name of the op, should we call the op dev_get
or dev_xdp_get..
> diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> new file mode 100644
> index 000000000000..254fc336d469
> --- /dev/null
> +++ b/include/uapi/linux/netdev.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/* Do not edit directly, auto-generated from: */
Like this line says, you can't hand edit this file.
Next time someone adds an attribute all your changes will be wiped.
> +/* Documentation/netlink/specs/netdev.yaml */
> +/* YNL-GEN uapi header */
> +
> +#ifndef _UAPI_LINUX_NETDEV_H
> +#define _UAPI_LINUX_NETDEV_H
> +
> +#define NETDEV_FAMILY_NAME "netdev"
> +#define NETDEV_FAMILY_VERSION 1
> +
> +enum netdev_xdp_act {
> + NETDEV_XDP_ACT_ABORTED_BIT,
> + NETDEV_XDP_ACT_DROP_BIT,
> + NETDEV_XDP_ACT_PASS_BIT,
> + NETDEV_XDP_ACT_TX_BIT,
> + NETDEV_XDP_ACT_REDIRECT_BIT,
> + NETDEV_XDP_ACT_NDO_XMIT_BIT,
> + NETDEV_XDP_ACT_XSK_ZEROCOPY_BIT,
> + NETDEV_XDP_ACT_HW_OFFLOAD_BIT,
> + NETDEV_XDP_ACT_RX_SG_BIT,
> + NETDEV_XDP_ACT_NDO_XMIT_SG_BIT
You need to add -bit to all the enum names in the yaml if you want
to have _BIT in the name here.
> +};
> +
> +#define NETDEV_XDP_ACT_ABORTED BIT(NETDEV_XDP_ACT_ABORTED_BIT)
> +#define NETDEV_XDP_ACT_DROP BIT(NETDEV_XDP_ACT_DROP_BIT)
> +#define NETDEV_XDP_ACT_PASS BIT(NETDEV_XDP_ACT_PASS_BIT)
> +#define NETDEV_XDP_ACT_TX BIT(NETDEV_XDP_ACT_TX_BIT)
> +#define NETDEV_XDP_ACT_REDIRECT BIT(NETDEV_XDP_ACT_REDIRECT_BIT)
> +#define NETDEV_XDP_ACT_NDO_XMIT BIT(NETDEV_XDP_ACT_NDO_XMIT_BIT)
> +#define NETDEV_XDP_ACT_XSK_ZEROCOPY BIT(NETDEV_XDP_ACT_XSK_ZEROCOPY_BIT)
> +#define NETDEV_XDP_ACT_HW_OFFLOAD BIT(NETDEV_XDP_ACT_HW_OFFLOAD_BIT)
> +#define NETDEV_XDP_ACT_RX_SG BIT(NETDEV_XDP_ACT_RX_SG_BIT)
> +#define NETDEV_XDP_ACT_NDO_XMIT_SG BIT(NETDEV_XDP_ACT_NDO_XMIT_SG_BIT)
> +
> +#define NETDEV_XDP_ACT_BASIC (NETDEV_XDP_ACT_DROP | \
> + NETDEV_XDP_ACT_PASS | \
> + NETDEV_XDP_ACT_TX | \
> + NETDEV_XDP_ACT_ABORTED)
> +#define NETDEV_XDP_ACT_FULL (NETDEV_XDP_ACT_BASIC | \
> + NETDEV_XDP_ACT_REDIRECT)
> +#define NETDEV_XDP_ACT_ZC (NETDEV_XDP_ACT_FULL | \
> + NETDEV_XDP_ACT_XSK_ZEROCOPY)
These defines don't belong in uAPI. Especially the use of BIT().
> + if (err < 0)
> + break;
> +cont:
> + idx++;
> + }
> + }
> +
> + rtnl_unlock();
> +
> + if (err != -EMSGSIZE)
> + return err;
> +
> + cb->args[1] = idx;
> + cb->args[0] = h;
> + cb->seq = net->dev_base_seq;
> + nl_dump_check_consistent(cb, nlmsg_hdr(skb));
I think that this line can be dropped.
> + return skb->len;
> +}
