On Thu, 29 Aug 2024 17:16:57 +0200 Paolo Abeni wrote:
> Allow grouping multiple leaves shaper under the given root.
> The root and the leaves shapers are created, if needed, otherwise
> the existing shapers are re-linked as requested.
>
> Try hard to pre-allocated the needed resources, to avoid non
> trivial H/W configuration rollbacks in case of any failure.
Need to s/root/parent/ the commit message?
> +static int __net_shaper_group(struct net_shaper_binding *binding,
> + int leaves_count,
> + const struct net_shaper_handle *leaves_handles,
> + struct net_shaper_info *leaves,
> + struct net_shaper_handle *node_handle,
> + struct net_shaper_info *node,
> + struct netlink_ext_ack *extack)
> +{
> + const struct net_shaper_ops *ops = net_shaper_binding_ops(binding);
> + struct net_shaper_info *parent = NULL;
> + struct net_shaper_handle leaf_handle;
> + int i, ret;
> +
> + if (node_handle->scope == NET_SHAPER_SCOPE_NODE) {
> + if (node_handle->id != NET_SHAPER_ID_UNSPEC &&
> + !net_shaper_cache_lookup(binding, node_handle)) {
> + NL_SET_ERR_MSG_FMT(extack, "Node shaper %d:%d does not
> exists",
> + node_handle->scope, node_handle->id);
BAD_ATTR would do?
> + return -ENOENT;
> + }
> +
> + /* When unspecified, the node parent scope is inherited from
> + * the leaves.
> + */
> + if (node->parent.scope == NET_SHAPER_SCOPE_UNSPEC) {
> + for (i = 1; i < leaves_count; ++i) {
> + if (leaves[i].parent.scope !=
> + leaves[0].parent.scope ||
> + leaves[i].parent.id !=
> + leaves[0].parent.id) {
memcmp() ? put a BUILD_BUG_ON(sizeof() != 8) to make sure we double
check it if the struct grows?
> + NL_SET_ERR_MSG_FMT(extack, "All the
> leaves shapers must have the same old parent");
> + return -EINVAL;
5 indents is too many indents :( maybe make the for loop a helper?
> + }
> + }
> +
> + if (leaves_count > 0)
how can we get here and not have leaves? :o
> + node->parent = leaves[0].parent;
> + }
> +
> + } else {
> + net_shaper_default_parent(node_handle, &node->parent);
> + }
> +static int net_shaper_group_send_reply(struct genl_info *info,
> + struct net_shaper_handle *handle)
> +{
> + struct net_shaper_binding *binding = info->user_ptr[0];
> + struct sk_buff *msg;
> + int ret = -EMSGSIZE;
> + void *hdr;
> +
> + /* Prepare the msg reply in advance, to avoid device operation
> + * rollback.
> + */
> + msg = genlmsg_new(net_shaper_handle_size(), GFP_KERNEL);
> + if (!msg)
> + return ret;
return -ENOMEM;
> +
> + hdr = genlmsg_iput(msg, info);
> + if (!hdr)
> + goto free_msg;
> +
> + if (net_shaper_fill_binding(msg, binding, NET_SHAPER_A_IFINDEX))
> + goto free_msg;
> +
> + if (net_shaper_fill_handle(msg, handle, NET_SHAPER_A_HANDLE))
you can combine the two fill ifs into one with ||
> + goto free_msg;
> +
> + genlmsg_end(msg, hdr);
> +
> + ret = genlmsg_reply(msg, info);
> + if (ret)
> + goto free_msg;
reply always eats the skb, just:
return genlmsg_reply(msg, info);
> +
> + return ret;
> +
> +free_msg:
> + nlmsg_free(msg);
> + return ret;
return -EMSGSIZE;
> +}
> +
> +int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct net_shaper_handle *leaves_handles, node_handle;
> + struct net_shaper_info *leaves, node;
> + struct net_shaper_binding *binding;
> + int i, ret, rem, leaves_count;
> + struct nlattr *attr;
> +
> + if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_LEAVES) ||
> + GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_NODE))
> + return -EINVAL;
> +
> + binding = net_shaper_binding_from_ctx(info->user_ptr[0]);
> + leaves_count = net_shaper_list_len(info, NET_SHAPER_A_LEAVES);
> + leaves = kcalloc(leaves_count, sizeof(struct net_shaper_info) +
> + sizeof(struct net_shaper_handle), GFP_KERNEL);
> + if (!leaves) {
> + GENL_SET_ERR_MSG_FMT(info, "Can't allocate memory for %d leaves
> shapers",
> + leaves_count);
> + return -ENOMEM;
> + }
> + leaves_handles = (struct net_shaper_handle *)&leaves[leaves_count];
> +
> + ret = net_shaper_parse_node(binding, info->attrs[NET_SHAPER_A_NODE],
> + info, &node_handle, &node);
> + if (ret)
> + goto free_shapers;
> +
> + i = 0;
> + nla_for_each_attr_type(attr, NET_SHAPER_A_LEAVES,
> + genlmsg_data(info->genlhdr),
> + genlmsg_len(info->genlhdr), rem) {
> + if (WARN_ON_ONCE(i >= leaves_count))
> + goto free_shapers;
> +
> + ret = net_shaper_parse_info_nest(binding, attr, info,
> + NET_SHAPER_SCOPE_QUEUE,
> + &leaves_handles[i],
Wouldn't it be convenient to store the handle in the "info" object?
AFAIU the handle is forever for an info, so no risk of it being out
of sync...
> + &leaves[i]);
> + if (ret)
> + goto free_shapers;
> + i++;
> + }
> +
> + ret = net_shaper_group(binding, leaves_count, leaves_handles, leaves,
> + &node_handle, &node, info->extack);
...and it'd be nice if group had 5 rather than 7 params
> + if (ret < 0)
> + goto free_shapers;
> +
> + ret = net_shaper_group_send_reply(info, &node_handle);
> + if (ret) {
> + /* Error on reply is not fatal to avoid rollback a successful
> + * configuration.
Slight issues with the grammar here, but I think it should be fatal.
The sender will most likely block until they get a response.
Not to mention that the caller will not know what the handle
we allocated is.
> + */
> + GENL_SET_ERR_MSG_FMT(info, "Can't send reply %d", ret);
> + ret = 0;
> + }
> +
> +free_shapers:
> + kfree(leaves);
> + return ret;
> +}
