Re: [net-next-2.6 PATCH v2 2/4] dcbnl: adding DCBX feature flags get-set

[John Fastabend <john.r.fastabend@intel.com>]

On 1/2/2011 8:01 AM, Shmulik Ravid wrote:
> 
>> One more nit ;)
>>
>>> +
>>> +	ret = nla_parse_nested(data, DCB_FEATCFG_ATTR_MAX, tb[DCB_ATTR_FEATCFG],
>>> +			       dcbnl_featcfg_nest);
>>> +	if (ret) {
>>> +		ret = -EINVAL;
>>> +		goto err_out;
>>> +	}
>>
>> Why do you set EINVAL here if you use the returned error code from nla_parse_nested you get a more descriptive error. See ./lib/nlattr.c:nla_parse()/validate_nla().
>>
>> [...]
>>
>>> +static int dcbnl_setfeatcfg(struct net_device *netdev, struct nlattr **tb,
>>> +			    u32 pid, u32 seq, u16 flags)
>>> +{
>>> +	struct nlattr *data[DCB_FEATCFG_ATTR_MAX + 1];
>>> +	int ret = -EINVAL;
>>> +	u8 value;
>>> +	int i;
>>> +
>>> +	if (!tb[DCB_ATTR_FEATCFG] || !netdev->dcbnl_ops->setfeatcfg)
>>> +		return ret;
>>> +
>>> +	ret = nla_parse_nested(data, DCB_FEATCFG_ATTR_MAX, tb[DCB_ATTR_FEATCFG],
>>> +			       dcbnl_featcfg_nest);
>>> +
>>> +	if (ret) {
>>> +		ret = -EINVAL;
>>> +		goto err;
>>> +	}
>>
>> Same here.
>>
> 
> I'll send a patch with the improved return values for the the new dcbnl
> routines. While I'm at it, is it safe to fix on the same lines the
> older already established dcbnl routines?

This should be safe I would not expect using more accurate error values could hurt any existing applications. Be sure to make it a separate patch though.

John