On Tue, 20 Aug 2024 17:12:24 +0200 Paolo Abeni wrote:

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 🧐️