On Fri, 23 Aug 2024 10:52:04 +0200 Paolo Abeni wrote:

quoted

quoted

+ * 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?  

I had a few back-and-forth between the two options, enforcing only QUEUE 
leaves or allowing even NODE.

I think the first option is general enough - can create arbitrary 
topologies with the same amount of operations - and leads to slightly 
simpler code, but no objections for allow both.

Ah, so we can only "grow the tree from the side of the leaves", 
so to speak? We can't create a group in the middle of the hierarchy?
I have no strong use for groups in between, maybe just mention in
a comment or cover letter.

quoted

quoted

+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?  

Uhmm... should happen only in buggy situation. What about adding adding 
WARN_ON_ONCE() ?

That's better, at least it will express that it's not expected.

quoted

quoted

+	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?  

I don't understand the question.

The root handle has scope NETDEV and id 0, the id will not printed out 
as redundant: there is only a scope NETDEV shaper per struct net_device.

Misread, yes, no id but it does have scope. That's fine, sorry.