Re: [PATCH v4 net-next 03/12] net-shapers: implement NL get operation

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 20 Aug 2024 17:12:24 +0200 Paolo Abeni wrote:
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -81,6 +81,8 @@ struct xdp_frame;
>  struct xdp_metadata_ops;
>  struct xdp_md;
>  struct ethtool_netdev_state;
> +struct net_shaper_ops;
> +struct net_shaper_data;

no need, forward declarations are only needed for function declarations

> + * struct net_shaper_ops - Operations on device H/W shapers
> + *
> + * The initial shaping configuration at device initialization is empty:
> + * does not constraint the rate in any way.
> + * The network core keeps track of the applied user-configuration in
> + * the net_device structure.
> + * The operations are serialized via a per network device lock.
> + *
> + * Each shaper is uniquely identified within the device with an 'handle'

a handle

> + * comprising the shaper scope and a scope-specific id.
> + */
> +struct net_shaper_ops {
> +	/**
> +	 * @group: create the specified shapers scheduling group
> +	 *
> +	 * Nest the @leaves shapers identified by @leaves_handles under the
> +	 * @root shaper identified by @root_handle. All the shapers belong
> +	 * to the network device @dev. The @leaves and @leaves_handles shaper
> +	 * arrays size is specified by @leaves_count.
> +	 * Create either the @leaves and the @root shaper; or if they already
> +	 * exists, links them together in the desired way.
> +	 * @leaves scope must be NET_SHAPER_SCOPE_QUEUE.

Or SCOPE_NODE, no?

> +	 * Returns 0 on group successfully created, otherwise an negative
> +	 * error value and set @extack to describe the failure's reason.

the return and extack lines are pretty obvious, you can drop

> +	 */
> +	int (*group)(struct net_device *dev, int leaves_count,
> +		     const struct net_shaper_handle *leaves_handles,
> +		     const struct net_shaper_info *leaves,
> +		     const struct net_shaper_handle *root_handle,
> +		     const struct net_shaper_info *root,
> +		     struct netlink_ext_ack *extack);

> +#endif
> +

ooh, here's one of the trailing whitespace git was mentioning :)

>  #include <linux/kernel.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/idr.h>
> +#include <linux/netdevice.h>
> +#include <linux/netlink.h>
>  #include <linux/skbuff.h>
> +#include <linux/xarray.h>
> +#include <net/net_shaper.h>

kernel.h between idr.h and netdevice.h

> +static int net_shaper_fill_handle(struct sk_buff *msg,
> +				  const struct net_shaper_handle *handle,
> +				  u32 type, const struct genl_info *info)
> +{
> +	struct nlattr *handle_attr;
> +
> +	if (handle->scope == NET_SHAPER_SCOPE_UNSPEC)
> +		return 0;

In what context can we try to fill handle with scope unspec?

> +	handle_attr = nla_nest_start_noflag(msg, type);
> +	if (!handle_attr)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_u32(msg, NET_SHAPER_A_SCOPE, handle->scope) ||
> +	    (handle->scope >= NET_SHAPER_SCOPE_QUEUE &&
> +	     nla_put_u32(msg, NET_SHAPER_A_ID, handle->id)))
> +		goto handle_nest_cancel;

So netdev root has no id and no scope?

> +	nla_nest_end(msg, handle_attr);
> +	return 0;
> +
> +handle_nest_cancel:
> +	nla_nest_cancel(msg, handle_attr);
> +	return -EMSGSIZE;
> +}

> +/* On success sets pdev to the relevant device and acquires a reference
> + * to it.
> + */
> +static int net_shaper_fetch_dev(const struct genl_info *info,
> +				struct net_device **pdev)
> +{
> +	struct net *ns = genl_info_net(info);
> +	struct net_device *dev;
> +	int ifindex;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_IFINDEX))
> +		return -EINVAL;
> +
> +	ifindex = nla_get_u32(info->attrs[NET_SHAPER_A_IFINDEX]);
> +	dev = dev_get_by_index(ns, ifindex);

netdev_get_by_index()

> +	if (!dev) {
> +		GENL_SET_ERR_MSG_FMT(info, "device %d not found", ifindex);

Point to the IFINDEX attribute, return -ENOENT.
Please only use string errors when there's no way of expressing 
the error with machine readable attrs.

> +		return -EINVAL;
> +	}
> +
> +	if (!dev->netdev_ops->net_shaper_ops) {
> +		GENL_SET_ERR_MSG_FMT(info, "device %s does not support H/W shaper",
> +				     dev->name);

same as a above, point at device, -EOPNOTSUPP

> +		netdev_put(dev, NULL);

I appears someone is coding to patchwork checks 🧐️