[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] Don't exit() in config_error()
From: |
Mark McLoughlin |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] Don't exit() in config_error() |
Date: |
Wed, 30 Sep 2009 14:27:47 +0100 |
On Wed, 2009-09-30 at 15:24 +0200, Markus Armbruster wrote:
> Mark McLoughlin <address@hidden> 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 <address@hidden>
> >> ---
> >> 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.