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