Re: [Qemu-devel] [PATCH 2/3] Don't exit() in config_error()

[Mark McLoughlin <markmc@redhat.com>]

On Wed, 2009-09-30 at 15:24 +0200, Markus Armbruster wrote:
> Mark McLoughlin <markmc@redhat.com> writes:
> 
> > On Mon, 2009-09-28 at 21:11 +0200, Markus Armbruster wrote:
> >> Propagating errors up the call chain is tedious.  In startup code, we
> >> can take a shortcut: terminate the program.  This is wrong elsewhere,
> >> the monitor in particular.
> >> 
> >> config_error() tries to cater for both customers: it terminates the
> >> program unless its mon parameter tells it it's working for the
> >> monitor.
> >> 
> >> Its users need to return status anyway (unless passing a null mon
> >> argument, which none do), which their users need to check.  So this
> >> automatic exit buys us exactly nothing useful.  Only the dangerous
> >> delusion that we can get away without returning status.  Some of its
> >> users fell for that.  Their callers continue executing after failure
> >> when working for the monitor.
> >> 
> >> This bites monitor command host_net_add in two places:
> >> 
> >> * net_slirp_init() continues after slirp_hostfwd(), slirp_guestfwd(),
> >>   or slirp_smb() failed, and may end up reporting success.  This
> >>   happens for "host_net_add user guestfwd=foo": it complains about the
> >>   invalid guest forwarding rule, then happily creates the user network
> >>   without guest forwarding.
> >> 
> >> * net_client_init() can't detect slirp_guestfwd() failure, and gets
> >>   fooled by net_slirp_init() lying about success.  Suppresses its
> >>   "Could not initialize device" message.
> >> 
> >> Add the missing error reporting, make sure errors are checked, and
> >> drop the exit() from config_error().
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  net.c |   83 ++++++++++++++++++++++++++++++++++++----------------------------
> >>  net.h |    4 +-
> >>  vl.c  |    9 ++++--
> >>  3 files changed, 55 insertions(+), 41 deletions(-)
> >> 
> > ...
> >> diff --git a/vl.c b/vl.c
> >> index 7bfd415..96e4312 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> > ...
> >> @@ -5766,7 +5768,8 @@ int main(int argc, char **argv, char **envp)
> >>  
> >>      /* init USB devices */
> >>      if (usb_enabled) {
> >> -        foreach_device_config(DEV_USB, usb_parse);
> >> +        if (foreach_device_config(DEV_USB, usb_parse) < 0)
> >> +            exit(1);
> >>      }
> >>  
> >>      /* init generic devices */
> >
> > This hunk appears to be unrelated
> 
> net_client_init() returns failure through usb_device_add(), usb_parse(),
> foreach_device_config() to main().  Without this hunk, we ignore the
> error and continue.
> 
> Before patch: net_client_init() or one of its callees terminates the
> program on failure, by calling config_error().
> 
> Makes sense?

Yes, it does - I shouldn't have doubted you :-)

Re-instated it again in my tree

Thanks,
Mark.