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

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

On 12/30/2010 8:26 AM, Shmulik Ravid wrote:
> Adding a pair of set-get routines to dcbnl for setting the negotiation
> flags of the various DCB features. Conforms to the CEE flavor of DCBX
> The user sets these flags (enable, advertise, willing) for each feature
> to be used by the DCBX engine. The 'get' routine returns which of the
> features is enabled after the negotiation.
> 
> This patch is dependent on the following patches:
> [net-next-2.6 PATCH 1/3] dcbnl: add support for ieee8021Qaz attributes
> [net-next-2.6 PATCH 2/3] dcbnl: add appliction tlv handlers
> [net-next-2.6 PATCH 3/3] net_dcb: add application notifiers
> 
> Signed-off-by: Shmulik Ravid <shmulikr@broadcom.com>
> ---

[...]

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.

Thanks,
John.