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

["David Harton (dharton)" <dharton@cisco.com>]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, August 31, 2017 6:04 PM
> To: David Harton (dharton) <dharton@cisco.com>
> Cc: dev@dpdk.org; ferruh.yigit@intel.com; ajit.khaparde@broadcom.com; John
> Daley (johndale) <johndale@cisco.com>; konstantin.ananyev@intel.com;
> jingjing.wu@intel.com; beilei.xing@intel.com; jing.d.chen@intel.com;
> adrien.mazarguil@6wind.com; nelio.laranjeiro@6wind.com;
> alejandro.lucero@netronome.com; hemant.agrawal@nxp.com;
> rasesh.mody@cavium.com; harish.patil@cavium.com; skhare@vmware.com;
> yliu@fridaylinux.org; maxime.coquelin@redhat.com;
> allain.legacy@windriver.com
> Subject: Re: [dpdk-dev] [PATCH v3] ethdev: modifiy vlan_offload_set_t to
> return int
> 
> 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

Sure.

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

Sure I'll move it to API.

Line spacing?  You mean 80 char width?  I followed an example from a previous
release so I'm not sure what you mean.

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

Ok.

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

Will do.

> 
> > -	(*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?

By setting the dev_conf.rxmode.hw_vlan_* fields this function is claiming 
ownership of that data and therefore it should be the one to reset it.

> Do we want to document this behaviour with the ops prototype?

Why?  Shouldn't any function that doesn't do what it was asked to do 
leave the system in its original state and in fact if it doesn't that 
is when it should be documented what the behavior is?  Otherwise every 
caller has to implement some cleanup code and would have to be given 
information to know how to perform that cleanup as well which seems 
more complicated.

Thanks,
Dave