Fwd: [BusyBox] [PATCH] 1.0-rc1 ifupdown fixes

[Mike Snitzer]

doh, forgot to reply all... 

---------- Forwarded message ----------
From: Mike Snitzer <snitzer at gmail.com>
Date: Wed, 21 Jul 2004 12:08:52 -0600
Subject: Re: [BusyBox] [PATCH] 1.0-rc1 ifupdown fixes
To: Glenn McGrath <bug1 at iinet.net.au>

On Wed, 21 Jul 2004 22:22:02 +1000, Glenn McGrath <bug1 at iinet.net.au> wrote:
> On Tue, 20 Jul 2004 15:07:28 -0600
> Mike Snitzer <snitzer at gmail.com> wrote:
>
> > The attached patch dates back to 1.0-pre3 but I just applied and
> > rediff'd against 1.0-rc1.  I have a need to _really_ know if the
> > interface was properly configured via ifup so I made busybox's
> > ifupdown pass the return codes through rather than dropping them on
> > the floor.
...
> Im going to have to have a second look at the other hunks, im having
> trouble following these return/exit codes right now.
>
> The return codes do look like they need work.

Yes, they definitely do.  I just traced the code pretty closely.

All the functions in ifupdown.c return 1 on success and 0 on failure
(which happens to the opposite of standard practices but whatever).
So it is important for all these functions to not blindly return 1.

1st hunk:
With the execute() function anything other than a return code of 1 is
an error.  The hunk in my patch exit()'s on != 1, I had it return(ret)
but I could've left it as return(1).  The problem with blindly
returning ret, even if it is != 1, is the callers expect a 0 or 1 and
accumulate the return codes.  So a function that makes 3 calls to
execute will have a value of 3 accumulated.  That value of 1 (success)
was almost always returned even if 1 of the commands in the command
sequence failed.  The attached patch fixes the lack of checking to
verify thar result == expected_reult.

I had a "fun" time tracing execute back though static_up and the like
and then to the methods stucture, and finally to addr_fams struct;
get_address_family determines what type of interface initialization to
use; static, dhcp, etc.  The matching address_family has an associated
function to be ran via execute(); get_method() returns that callable
function.  iface_up() and iface_down() do the work of calling
execute(); they are called in ifupdown_main() through the line: if
(cmds(currif) == -1) {...  So really all this tracing of the code
leads us back to iface_up() and iface_down().  These both use
execute_all() which uses doit().  Long story short; I've come to
realize that exit(ret) in execute() is not the right fix.  It should
be if (ret != 1) return(0).

3rd hunk:
No longer needed.  After going through the excercise of explaining
hunk#1 it is clear that doit() should return(0) in the face of
failure.

5th hunk:
No longer needed. the any_failures counter I've added handles at least
acknowledging that there was a failure with a return(1).

So all this said, I've created and heavily tested the attached patch
(minus the 2 hunks you've already applied from the previous patch; so
it _should_ apply cleanly to cvs).  Its now provably correct.

regards,
Mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.00-rc1_ifupdown_r2.patch
Type: application/octet-stream
Size: 6489 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20040721/3fc4dee1/attachment.obj