Re: [dpdk-dev] [PATCH v3] ethdev: modifiy vlan_offload_set_t to return int

[Thomas Monjalon <thomas@monjalon.net>]

Hi,

25/08/2017 15:47, David Harton:
> Some devices may not support or fail setting VLAN offload
> configuration based on dynamic circurmstances so the
> vlan_offload_set_t vector is modified to return an int so
> the caller can determine success or not.

I agree with allowing to return an error.

Comments on details below.

The title could be changed to better reflect the purpose:
	ethdev: allow returning error on VLAN configuration

> --- a/doc/guides/rel_notes/release_17_11.rst
> +++ b/doc/guides/rel_notes/release_17_11.rst
> @@ -124,7 +124,7 @@ ABI Changes
>     Also, make sure to start the actual text at the margin.
>     =========================================================
>  
> -
> +* Changed return type of ``vlan_offload_set_t`` from ``void`` to ``int``.

It should be referenced as an API change (instead of ABI change).
(and line spacing must be kept)

We also need to bump the library version but it can be done with
bigger changes in ethdev.

> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2049,10 +2049,16 @@ struct rte_eth_dev *
>  	int ret = 0;
>  	int mask = 0;
>  	int cur, org = 0;
> +	uint8_t org_strip, org_filter, org_extend;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	dev = &rte_eth_devices[port_id];
>  
> +	/* save original values in case of failure */
> +	org_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
> +	org_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
> +	org_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;

Please waste one more char to write "orig" instead of "org".

> -	(*dev->dev_ops->vlan_offload_set)(dev, mask);
> +	ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
> +	if (ret) {
> +		/* hit an error restore  original values */
> +		dev->data->dev_conf.rxmode.hw_vlan_strip = org_strip;
> +		dev->data->dev_conf.rxmode.hw_vlan_filter = org_filter;
> +		dev->data->dev_conf.rxmode.hw_vlan_extend = org_extend;
> +	}

Isn't it the responsibility of the PMD to restore values in case of error?
I understand it is there to factorize error handling code, right?
Do we want to document this behaviour with the ops prototype?