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

One more comment below

01/09/2017 00:04, Thomas Monjalon:

Hi,

25/08/2017 15:47, David Harton:

quoted

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

quoted

--- 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.

quoted

--- 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".

quoted

-	(*dev->dev_ops->vlan_offload_set)(dev, mask);
+	ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);

Which error codes can be returned by this op?
It is adding new error codes to rte_eth_dev_set_vlan_offload(),
and they must be documented in the doxygen.

quoted

+	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?

Thanks