Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> On 02/07/2012 08:09 AM, Markus Armbruster wrote:
>> Convert to error_report().  This adds error locations to the messages,
>> which is particularly important when the location is buried in a
>> configuration file.  Moreover, we'll need this when we create a
>> monitor command to add character devices, so its errors actually
>> appear in the monitor, not stderr.
>>
>> Also clean up the messages, and get rid of some that look like errors,
>> but aren't.
>>
>> Improves user-hostile messages like this one for "-chardev
>> socket,id=foo,host=blackfin,port=1,server"
>>
>>      inet_listen_opts: bind(ipv4,192.168.2.9,1): Permission denied
>>      chardev: opening backend "socket" failed
>>
>> to
>>
>>      qemu-system-x86_64: -chardev socket,id=foo,host=blackfin,port=1,server: 
>> Can't bind port blackfin:1: Permission denied
>>      chardev: opening backend "socket" failed
>>
>> and this one for "-chardev udp,id=foo,localport=1,port=1"
>>
>>      inet_dgram_opts: bind(ipv4,0.0.0.0,1): OK
>>      inet_dgram_opts failed
>>      chardev: opening backend "udp" failed
>>
>> to
>>
>>      qemu-system-x86_64: -chardev udp,id=foo,localport=1,port=1: Can't bind 
>> port :1: Permission denied
>>      chardev: opening backend "udp" failed
>>
>> You got to love the "OK" part.
>>
>> The uninformative extra "opening backend failed" message will be
>> cleaned up shortly.
>>
>> Signed-off-by: Markus Armbruster<address@hidden>
>> ---
>>   qemu-char.c    |    1 -
>>   qemu-sockets.c |  154 
>> +++++++++++++++++++++++++------------------------------
>>   2 files changed, 70 insertions(+), 85 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index bb9e3f5..d591f70 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2101,7 +2101,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts 
>> *opts)
>>
>>       fd = inet_dgram_opts(opts);
>>       if (fd<  0) {
>> -        fprintf(stderr, "inet_dgram_opts failed\n");
>>           goto return_err;
>>       }
>
> Let's add an Error ** argument here.
>
> It's easy to bridge errors to error_report (qerror_report_err) so it's
> conducive to incremental refactoring.  Plus it starts getting us away
> from terminal errors and into proper erroro propagation.
[...]
>> @@ -518,26 +499,31 @@ int unix_connect_opts(QemuOpts *opts)
>>       int sock;
>>
>>       if (NULL == path) {
>> -        fprintf(stderr, "unix connect: no path specified\n");
>> +        error_report("unix socket character device requires parameter 
>> path");
>>           return -1;
>>       }
>>
>>       sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>>       if (sock<  0) {
>> -        perror("socket(unix)");
>> -        return -1;
>> +        goto err;
>>       }
>>
>>       memset(&un, 0, sizeof(un));
>>       un.sun_family = AF_UNIX;
>>       snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
>>       if (connect(sock, (struct sockaddr*)&un, sizeof(un))<  0) {
>> -        fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
>> -        close(sock);
>> -    return -1;
>> +        goto err;
>>       }
>>
>>       return sock;
>> +
>> +err:
>> +    error_report("Can't connect to socket %s: %s",
>> +                 un.sun_path, strerror(errno));
>> +    if (sock>= 0) {
>> +        close(sock);
>> +    }
>> +    return -1;
>>   }
>
> Basically, same thing here and the remaining functions.  Let's not
> introduce additional uses of error_report().
>
> That said, I imagine you don't want to introduce a bunch of error
> types for these different things and that's probably not productive
> anyway.

You're imagining correctly.  I've learned the hard way that replacing
nicely crafted error messages by error types frequently degrades the
error messages, which first makes me sad, and then makes me surf the web
rather than work.

You'd need somebody with a higher tolerance for bad error messages, or a
lower propensity to indulge in wasting time rather than deliver shoddy
work ;)

> So let's compromise and introduce a generic QERR_INTERNAL_ERROR that
> takes a single human readable string as an argument.  We can have a
> wrapper for it that also records location information in the error
> object.

This series goes from stderr to error_report().  That's a relatively
simple step, which makes it relatively easy to review.  I'm afraid
moving all the way to error.h in one step wouldn't be as easy.  Kevin
suggests to do it in a follow-up series, and I agree.

Can you point to an existing conversion from error_report() to error.h,
to give us an idea how it's supposed to be done?