Date:   Thu, 8 Feb 2018 15:11:03 +0100
From:   Phil Sutter <phil@....cc>
To:     Élie Bouttier <elie@...ttier.eu>
Cc:     Stephen Hemminger <stephen@...workplumber.org>,
        netdev@...r.kernel.org
Subject: Re: [iproute PATCH] ip-route: Propagate errors from parse_one_nh()

Hi,

On Thu, Feb 08, 2018 at 02:26:05PM +0100, Élie Bouttier wrote:
> On 24/01/2018 16:44, Stephen Hemminger wrote:
> > On Wed, 24 Jan 2018 10:19:24 +0100
> > Phil Sutter <phil@....cc> wrote:
> >> On Tue, Jan 23, 2018 at 02:44:42PM -0800, Stephen Hemminger wrote:
> >>> The original commit 2f406f2d0b4ef ("ip route: replace exits with returns")
> >>> looks like well intentioned but suspect. Most of the errors in ip route
> >>> indicate real issues where continuing is not a good plan.  
> >>
> >> You're right, the use of invarg() for any other error effectively
> >> prevents what said commit tried to achieve, so my fix is pretty
> >> pointless in that regard. Yet I wonder why we still have 'ip -batch
> >> -force' given that it's not useful. Maybe Élie is able to provide some
> >> details about the use-case said commit tried to fix?
> >>
> >> Meanwhile I'll prepare some patches to address the shortcomings you
> >> mentioned above.
> > 
> > The use case for batch (and force) is that there may be a large set of routes
> > or qdisc operations where it is ok for some of them to fail because of responses
> > from the kernel failing.  I don't think batch should ever just continue if handed
> > invalid syntax for device or address. There are some borderline cases, for example
> > if a tunnel device could not be created and later steps depend on that name.
> > 
> > Agree, lets get some real data on why the original patch was done.
> 
> If I remember correctly, I made this commit for a batch of "route get"
> and not stop on inexistent routes. But my patch was not limited to this
> use case and I tried to fix others potential situations where we should
> not stop. The change I made in parse_one_nh is not directly related to
> my use care, sorry for the introduced regression, I should have been
> more careful.
> 
> Ihmo, if a tunnel device could not be created, later steps depending on
> it will fail too, but we potentially still want subsequent operations
> (for instance the creation of a second tunnel) to be done. I you don't
> want it, don't use -force or make two batch files. From this point of
> view, Phil patch is better than invarg() but I'm fine with invarg() too.

OK, that 'route get' case seems pretty valid for me. Also, the situation
I was fixing for in the first place was caused by non-existing
interface, which may very well be caused by a previous attempt at
creating it which had failed. So not really a case of "invalid syntax",
either.

I guess the best approach to satisfy all considerations would be to make
a clear distinction between syntax errors and others (including
non-existing interface e.g.) and not allowing for the latter to exit the
program but propagate errors up to a non-zero return code in do_cmd().
Might be quite some work given that '-batch -force' seems to be a rarely
used combination.

I had a quick idea of changing batch mode to fork and exec in order to
avoid the diligent work needed, but a quick comparison between batch
mode and separate ip invocations for about 64k commands each showed a
slowdown of over 20 times, so really not an alternative here.

My guess is that distinguishing between syntax error and "regular" ones
won't be very clear in all cases and trying to maintain it might be
error-prone. So I'd personally just not make that distinction at all but
try to let -force continue at all times, no matter what. Especially
since the user explicitly requested this behaviour.

Stephen, I think this is a maintainer's decision now. :)

Cheers, Phil

Powered by blists - more mailing lists