Re: [Qemu-devel] Ignoring errno makes QMP errors suck

[Luiz Capitulino]

On Mon, 26 Mar 2012 15:13:36 +0200
Kevin Wolf <address@hidden> wrote:

> Am 26.03.2012 14:46, schrieb Luiz Capitulino:
> > On Mon, 26 Mar 2012 10:39:50 +0200
> > Kevin Wolf <address@hidden> wrote:
> > 
> >> Hi,
> >>
> >> I keep getting reports of problems, with nice error descriptions that
> >> usually look very similar to what I produced here:
> >>
> >> {"execute":"blockdev-snapshot-sync","arguments":{"device":"ide0-hd0","snapshot-file":"/tmp/backing.qcow2"}}
> >> {"error": {"class": "OpenFileFailed", "desc": "Could not open
> >> '/tmp/backing.qcow2'", "data": {"filename": "/tmp/backing.qcow2"}}}
> >>
> >> Who can tell me what has happened here? Oh, yes, the command failed, I
> >> would have guessed that from the "error" key. But the actual error
> >> description is as useless as it gets. It doesn't tell me anything about
> >> _why_ the snapshot couldn't be created. ("Permission denied" would have
> >> been the helpful additional information in this case)
> > 
> > There's a function called qemu_fopen_err() in the screendump conversion 
> > series
> > that return more specific errors. It will be trivial to add qemu_open_err()
> > as soon as qemu_fopen_err() is merged.
> > 
> > We're adding a bunch of more precise errors (some map directly to errno). 
> > That's
> > the easy part. The hard part is to convert everything to use them.
> > 
> > Note that while it's true that this shouldn't have leaked to QMP, good error
> > reporting is a general problem in QEMU.
> 
> I guess my point is that we're actually moving backwards here. In HMP
> things like this do print the right error message (using error_report).
> And the return code is passed all the way down to where the QMP error is
> generated, it's just ignored there.
> 
> The last time I checked there was no easy way to handle it there because
> errno and strerror(-errno) aren't things allowed in QMP messages. This
> is the problem for which I wanted to get some attention.

What we're doing now is to add QErrors that map to errno. So, for example,
we have PermissionDenied for EPERM.

I think this is exactly the same thing, except:

 1. We don't use the errno number, because this may differ among OSs
 2. We don't use the sterrror() string to allow for internationalization,
    but we have our own string that should have the same end result

> Does the patch that you mentioned add a generic way for adding an
> (converted) errno to QMP errors? Or does it split up existing errors
> into more and finer grained errors?

The latter. The QMP errors have to be added manually. But it's just a matter
of time to get the most used errnos added.