[BusyBox] [PATCH] 1.0-rc1 ifupdown fixes

Glenn McGrath bug1 at iinet.net.au
Wed Jul 21 23:58:39 UTC 2004


On Wed, 21 Jul 2004 12:08:52 -0600
Mike Snitzer <snitzer at gmail.com> wrote:

> On Wed, 21 Jul 2004 22:22:02 +1000, Glenn McGrath <bug1 at iinet.net.au>
> wrote:
> > 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.

Thanks, that made it much easier to understand, ive applied your patch
with some minor alterations, we can get away without declaring a few
variables.

I think the internal return values should be standardised, but that can
wait untill after 1.0



Glenn






More information about the busybox mailing list