Re: [PATCH v3 04/12] net-shapers: implement NL set and delete operations

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 30 Jul 2024 22:39:47 +0200 Paolo Abeni wrote:
> +static int net_shaper_delete(struct net_device *dev, u32 handle,
> +			     struct netlink_ext_ack *extack)
> +{
> +	struct net_shaper_info *parent, *shaper = sc_lookup(dev, handle);
> +	struct xarray *xa = __sc_container(dev);
> +	enum net_shaper_scope pscope;
> +	u32 parent_handle;
> +	int ret;
> +
> +	if (!xa || !shaper) {
> +		NL_SET_ERR_MSG_FMT(extack, "Shaper %x not found", handle);

below the print format for shaper id is %d

also just point at the attribute with NL_SET_BAD_ATTR(),
we shouldn't bloat the kernel with strings for trivial errors

> +		return -EINVAL;

ENOENT

> +	}
> +
> +	if (is_detached(handle) && shaper->children > 0) {
> +		NL_SET_ERR_MSG_FMT(extack, "Can't delete detached shaper %d with %d child nodes",
> +				   handle, shaper->children);
> +		return -EINVAL;

I'd say EBUSY

> +	}
> +
> +	while (shaper) {
> +		parent_handle = shaper->parent;
> +		pscope = net_shaper_handle_scope(parent_handle);
> +
> +		ret = dev->netdev_ops->net_shaper_ops->delete(dev, handle,
> +							      extack);
> +		if (ret < 0)
> +			return ret;
> +
> +		xa_lock(xa);
> +		__xa_erase(xa, handle);
> +		if (is_detached(handle))
> +			idr_remove(&dev->net_shaper_data->detached_ids,
> +				   net_shaper_handle_id(handle));
> +		xa_unlock(xa);
> +		kfree(shaper);
> +		shaper = NULL;

IIUC child is the input / ingress node? (The parentage terminology 
is a bit ambiguous in this case, although I must admit I keep catching
myself trying to use it, too).
Does "deleting a queue" return it to the "implicit mux" at the 
global level? If we look at the delegation use case - when queue
is "deleted" from a container-controlled mux it should go back to
the group created by the orchestrator, not "escpate" to global scope,
right?

> +		if (pscope == NET_SHAPER_SCOPE_DETACHED) {
> +			parent = sc_lookup(dev, parent_handle);
> +			if (parent && !--parent->children) {
> +				shaper = parent;
> +				handle = parent_handle;
> +			}
> +		}
> +	}
> +	return 0;
>  }
>  
>  int net_shaper_nl_delete_doit(struct sk_buff *skb, struct genl_info *info)
>  {
> -	return -EOPNOTSUPP;
> +	struct net_device *dev;
> +	u32 handle;
> +	int ret;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_HANDLE))
> +		return -EINVAL;
> +
> +	ret = fetch_dev(info, &dev);
> +	if (ret)
> +		return ret;

Possibly a candidate for a "pre" handler, since multiple ops call it?