Re: [Intel-wired-lan] [PATCH v5 net-next 04/12] net-shapers: implement NL group operation

[Paolo Abeni <pabeni@redhat.com>]

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