Re: [PATCH net-next 2/2] ethtool: netlink: convert commands to common SET

[Paolo Abeni <pabeni@redhat.com>]

Hi,

On Fri, 2023-01-20 at 21:44 -0800, Jakub Kicinski wrote:
> @@ -241,49 +229,41 @@ const struct nla_policy ethnl_coalesce_set_policy[] = {
>  	[ETHTOOL_A_COALESCE_TX_AGGR_TIME_USECS] = { .type = NLA_U32 },
>  };
>  
> -int ethnl_set_coalesce(struct sk_buff *skb, struct genl_info *info)
> +static int
> +ethnl_set_coalesce_validate(struct ethnl_req_info *req_info,
> +			    struct genl_info *info)
>  {
> -	struct kernel_ethtool_coalesce kernel_coalesce = {};
> -	struct ethtool_coalesce coalesce = {};
> -	struct ethnl_req_info req_info = {};
> +	const struct ethtool_ops *ops = req_info->dev->ethtool_ops;
>  	struct nlattr **tb = info->attrs;
> -	const struct ethtool_ops *ops;
> -	struct net_device *dev;
>  	u32 supported_params;
> -	bool mod = false;
> -	int ret;
>  	u16 a;
>  
> -	ret = ethnl_parse_header_dev_get(&req_info,
> -					 tb[ETHTOOL_A_COALESCE_HEADER],
> -					 genl_info_net(info), info->extack,
> -					 true);
> -	if (ret < 0)
> -		return ret;
> -	dev = req_info.dev;
> -	ops = dev->ethtool_ops;
> -	ret = -EOPNOTSUPP;
> -	if (!ops->get_coalesce || !ops->set_coalesce)
> -		goto out_dev;
> -
>  	/* make sure that only supported parameters are present */
>  	supported_params = ops->supported_coalesce_params;
>  	for (a = ETHTOOL_A_COALESCE_RX_USECS; a < __ETHTOOL_A_COALESCE_CNT; a++)
>  		if (tb[a] && !(supported_params & attr_to_mask(a))) {
> -			ret = -EINVAL;
>  			NL_SET_ERR_MSG_ATTR(info->extack, tb[a],
>  					    "cannot modify an unsupported parameter");
> -			goto out_dev;
> +			return -EINVAL;
>  		}
>  
> -	rtnl_lock();
> -	ret = ethnl_ops_begin(dev);
> -	if (ret < 0)
> -		goto out_rtnl;
> -	ret = ops->get_coalesce(dev, &coalesce, &kernel_coalesce,
> -				info->extack);
> +	return ops->get_coalesce && ops->set_coalesce ? 1 : -EOPNOTSUPP;

The above changes the error code in case a coalesce op is missing (and
likely supported_coalesce_params is zero) from EOPNOTSUPP to EINVAL.

I guess it's not a big deal but perhaps we can preserve the same error
code as prior to this patch? 

Not a big deal, I see other commands check the oops last.

Thanks,

Paolo