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

[Thomas Monjalon <thomas@monjalon.net>]

01/09/2017 02:40, David Harton (dharton):
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > --- 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.

I mean you must take care of the number of blank lines to have
before and after a title (2 blank lines before a title).

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

Sorry I overlooked the function. I thought these data were set by the PMD,
that's why I was telling the cleanup should be done in the PMD.
You can forget this comment :)