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

[Anthony Liguori]

On 03/26/2012 10:34 AM, Kevin Wolf wrote:
> Am 26.03.2012 17:14, schrieb Anthony Liguori:
> > On 03/26/2012 10:08 AM, Kevin Wolf wrote:
> > > Am 26.03.2012 15:37, schrieb Anthony Liguori:
> > > > On 03/26/2012 03:39 AM, Kevin Wolf 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"}}}
> > > >
> > > > This is not QMP's fault.  This is the block layers.  Specifically, you're 
> > > > missing:
> > > >
> > > > diff --git a/blockdev.c b/blockdev.c
> > > > index 1a500b8..04c3a39 100644
> > > > --- a/blockdev.c
> > > > +++ b/blockdev.c
> > > > @@ -777,7 +777,11 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **
> > > >                                       states->old_bs->drv->format_name,
> > > >                                       NULL, -1, flags);
> > > >                 if (ret) {
> > > > -                error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> > > > +                if (ret == -EPERM) {
> > > > +                    error_set(errp, QERR_PERMISSION_DENIED);
> > > > +                } else {
> > > > +                    error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> > > > +                }
> > > >                     goto delete_and_fail;
> > > >                 }
> > > >             }
> > > >
> > > > Which is handling:
> > > >
> > > >                ret = bdrv_img_create(new_image_file, format,
> > > >                                      states->old_bs->filename,
> > > >                                      states->old_bs->drv->format_name,
> > > >                                      NULL, -1, flags);
> > >
> > > It really should be something like this:
> > >
> > > -    error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> > > +    error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file, -ret);
> > >
> > > And QERR_OPEN_FILE_FAILED would contain a conversion specifier for
> > > errnos in qobject_from_jsonv().
> >
> > No, it really shouldn't be.
> >
> > Errors are verbs, not knows, you're treating the error as a noun "the operation
> > open file" and looking to use errno as the verb.  This is wrong.  The noun is
> > implied in the operation.
> >
> > You could use error_set_from_errno(errp, -ret) which doesn't exist, but could.
> > But errno on it's own lacks a lot of useful information so I wouldn't suggest
> > always using such a function.
>
> I couldn't care less about nouns and verbs and stuff.
>
> I want to transfer the information that a "permission denied" error has
> happened and on which file it has happened. The existing OpenFileFailed
> error doesn't allow to specify that the missing permission was the
> problem, and a hypothetical PermissionDenied error wouldn't allow me to
> specify the file name because it would be too generic.
>
> This is my problem, and nothing else.

Then extend PermissionDenied to include a filename.  Problem solved.

> > > Yes, but that's a completely independent problem.
> >
> > It's not really.  If you want high quality errors, you have to push the error
> > handling up the stack.  That's the reason we have Error--to introduce a common
> > error handling framework capable of generating high quality error information.
>
> Yes, but if there is no appropriate error, then even if I added Error
> support to the Linux syscalls they couldn't generate the right error
> message. This is why I still think it's completely independent.

Than add/improve the existing errors.

> > > > QMP provides all the infrastructure you need.   You just have to use it.
> > >
> > > It doesn't provide the portable way of reporting errno yet.
> >
> > I think what you'll find is that 90% of the time, the errno is being generated
> > somewhere within QEMU code or that there's a system call that returns on one
> > errno that we care about.  If you push error handling down to the source of the
> > error, I'm sure you'll find that you almost never have to switch on errno.
>
> I'm looking for a solution that works now and not only in five years
> when all of qemu has been rewritten. I'm also not quite sure if we
> really want to drag Errors through coroutines and AIO code in the block
> layer...

You can bridge this all today.  I showed you in patches how to do it.  It's not 
difficult.

> > Having an error_set_from_errno() would be a stop-gap to help bridge unconverted
> > code, but if you want high quality errors, the right answer is to convert the
> > existing code to use the Error infrastructure properly.
>
> Only if it can be used properly. That is, if I can somehow create an
> error message that contains _both_ the file name and the error cause.

You can add parameters to Errors in a fully compatible fashion, so just add an 
filename parameter to PermissionDenied.  Problem solved.

Regards,

Anthony Liguori