Re: [PATCH v3 6/6] net/failsafe: fix removed device handling

[Matan Azrad <matan@mellanox.com>]

Hi Gaetan

From: Gaëtan Rivet, Monday, January 8, 2018 3:47 PM
> On Mon, Jan 08, 2018 at 12:55:49PM +0000, Matan Azrad wrote:
> > Hi Gaetan
> >
> > From: Gaëtan Rivet, Monday, January 8, 2018 12:58 PM
> > > Hi Matan,
> > >
> > > Sorry for the delay on this.
> > >
> >
> > It's OK in spite of I need to fetch it back :)
> >
> > > On Wed, Dec 20, 2017 at 10:58:29AM +0000, Matan Azrad wrote:
<snip>
> > > And this kind of code-flow is not unusual, or even unwanted.
> > > I dislike having this kind of implicit rule derived from using a
> > > helper such as fs_is_error().
> > >
> > > The alternative
> > >
> > >     if ((err = fs_err(sdev, err))) {
> > >             ERROR("xxxx");
> > >             return err;
> > >     }
> > >
> > > Forces the value err to be set to the correct one.
> > >
> > Good point, will change it.
> >
> > > This mistake can already be found in your patch:
> > >
> > > > @@ -150,7 +150,7 @@
> > > >                         continue;
> > > >                 local_ret = rte_flow_destroy(PORT_ID(sdev),
> > > >                                 flow->flows[i], error);
> > > > -               if (local_ret) {
> > > > +               if (fs_is_error(sdev, local_ret)) {
> > > >                         ERROR("Failed to destroy flow on sub_device %d: %d",
> > > >                                         i, local_ret);
> > > >                         if (ret == 0)
> > >
> >
> > Sorry, I can't see any issue here.
> >
> 
> You're right, actually the code would still be correct.
> I checked again the rest of the edit, there shouldn't be any issue, usually "0"
> is explicitly returned.
> 
> Still, the point stands.
> 
Yes.

> > > Your environment does not include the function, but this is within
> > > fs_flow_destroy (please update to include the context by the way it
> > > helps a lot the review :). Afterward, line 162 ret is directly used as return
> value.
> > >
> > I don't understand what do you mean.
> >
> > > Also, fs_err() would need to transform rte_errno when relevant
> > > (mostly in failsafe_flow.c I think).
> > >
> > Your suggestion is always to update rte_errno to 0 in case the error is
> because of removal?
> >
> 
> If the error is indeed due to the device being absent, then rte_errno should
> be set back to its previous value I think.
So, I think it will require old rte_errno save before each device command...
Why not to set it to 0 in the special case(removal) by the new internal API?

> 
> --
> Gaëtan Rivet
> 6WIND