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

[Markus Armbruster]

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?