On 8/30/24 04:04, Jakub Kicinski wrote:
+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?
We can reach here from the delete() op (next patch), there will be no paired attribute is such case. Even for the group() operation it will need to push here towards several callers additional context to identify the attribute, it should be quite ugly, can we keep with ERR_MSG_FMT here?
+ 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.
You mean we should return a negative error code, and _not_ that we should additionally attempt a rollback, right? The rollback will be very difficult at best: at this point destructive action have taken place.
Thanks, Paolo
