[Qemu-devel] Ignoring errno makes QMP errors suck

[Qemu-devel] Ignoring errno makes QMP errors suck

[Kevin Wolf]

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)

How should management tools ever be able to provide a helpful error
message to their users if all they get is this useless "something went
wrong" error?

Kevin

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

[Luiz Capitulino]

On Mon, 26 Mar 2012 10:39:50 +0200
Kevin Wolf <kwolf@redhat.com> 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.

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

[Kevin Wolf]

Am 26.03.2012 14:46, schrieb Luiz Capitulino:
> On Mon, 26 Mar 2012 10:39:50 +0200
> Kevin Wolf <kwolf@redhat.com> 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.

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?

Kevin

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

[Luiz Capitulino]

On Mon, 26 Mar 2012 15:13:36 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 26.03.2012 14:46, schrieb Luiz Capitulino:
> > On Mon, 26 Mar 2012 10:39:50 +0200
> > Kevin Wolf <kwolf@redhat.com> 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.

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

[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);

But it would be even better to push Error ** into bdrv_img_create().  There's 
only two callers so it would be trivial to do that.  Then you could do:

diff --git a/block.c b/block.c
index b88ee90..a7bf8a9 100644
--- a/block.c
+++ b/block.c
@@ -3881,7 +3881,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cook

  int bdrv_img_create(const char *filename, const char *fmt,
                      const char *base_filename, const char *base_fmt,
-                    char *options, uint64_t img_size, int flags)
+                    char *options, uint64_t img_size, int flags,
+                    Error **errp)
  {
      QEMUOptionParameter *param = NULL, *create_options = NULL;
      QEMUOptionParameter *backing_fmt, *backing_file, *size;
@@ -3893,14 +3894,14 @@ int bdrv_img_create(const char *filename, const char *fm
      /* Find driver and parse its options */
      drv = bdrv_find_format(fmt);
      if (!drv) {
-        error_report("Unknown file format '%s'", fmt);
+        error_set(errp, QERR_INVALID_BLOCK_FORMAT, fmt);
          ret = -EINVAL;
          goto out;
      }

Etc.

> 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)
>
> How should management tools ever be able to provide a helpful error
> message to their users if all they get is this useless "something went
> wrong" error?

You need to kill off error_report in the block layer and replace it with 
error_set.  The problem with error_report is that while you can understand what 
"Unknown file format 'qcow2'" means, management tools can't.  Responding that 
"the tool can just present that error to the user" implies that the management 
tool only provides an English-language interface which is not terribly friendly.

QMP provides all the infrastructure you need.   You just have to use it.

Regards,

Anthony Liguori

> Kevin

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

[Anthony Liguori]

On 03/26/2012 08:28 AM, Luiz Capitulino wrote:
> On Mon, 26 Mar 2012 15:13:36 +0200
> Kevin Wolf<kwolf@redhat.com>  wrote:
>
>> Am 26.03.2012 14:46, schrieb Luiz Capitulino:
>>> On Mon, 26 Mar 2012 10:39:50 +0200
>>> Kevin Wolf<kwolf@redhat.com>  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.

Note that this whole discussion is somewhat irrelevant.  The error gets 
"converted" in the block code, not in generic QMP code.

If you want a error_set_from_errno() function, that's fairly trivial to add 
although you're missing the opportunity to add more useful information (like the 
name of the file).

Regards,

Anthony Liguori

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

[Kevin Wolf]

Am 26.03.2012 15:28, schrieb Luiz Capitulino:
> On Mon, 26 Mar 2012 15:13:36 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> Am 26.03.2012 14:46, schrieb Luiz Capitulino:
>>> On Mon, 26 Mar 2012 10:39:50 +0200
>>> Kevin Wolf <kwolf@redhat.com> 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.

EACCES in this case, but yes.

> 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

Yes, I know the reasons why we can't use these options and they are good
reasons. If we get something to replace it (so that we can have a
errno_to_qmp()), that part should be solved.

>> 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.

Your PermissionDenied example doesn't really do this. It is a generic
error message that may occur in multiple contexts, right? So in one
context you may need a file name as additional information, in another
context permission for something completely different may be missing
(especially if you include EPERM in the same error).

I think 'OpenFileFailed' is not too bad, it's just missing a field that
gives the detail 'PermissionDenied'. I'm not sure if having
'PermissionDenied' as the top-level error object is the best idea.

Kevin

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

[Luiz Capitulino]

On Mon, 26 Mar 2012 16:04:26 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> >> 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.
> 
> Your PermissionDenied example doesn't really do this. It is a generic
> error message that may occur in multiple contexts, right? So in one
> context you may need a file name as additional information, in another
> context permission for something completely different may be missing
> (especially if you include EPERM in the same error).

Yes, but it's not a severe problem. Not yet. Because most of the time the
error context can be inferred from the command's context. Say, you fail to
create a file where only one file could be created.

But yes, we need to add a way for errors to accept optional parameters.

I have a series that's almost done that moves all errors to a qapi-error.json
file and auto-generates the QErrors tables. Maybe that's a first step towards
more flexible errors.

> I think 'OpenFileFailed' is not too bad, it's just missing a field that
> gives the detail 'PermissionDenied'. I'm not sure if having
> 'PermissionDenied' as the top-level error object is the best idea.

I think the end result is the same, but I prefer PermissionDenied as the
top level because it's slightly simpler.

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

[Kevin Wolf]

Am 26.03.2012 16:33, schrieb Luiz Capitulino:
> On Mon, 26 Mar 2012 16:04:26 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>>>> 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.
>>
>> Your PermissionDenied example doesn't really do this. It is a generic
>> error message that may occur in multiple contexts, right? So in one
>> context you may need a file name as additional information, in another
>> context permission for something completely different may be missing
>> (especially if you include EPERM in the same error).
> 
> Yes, but it's not a severe problem. Not yet. Because most of the time the
> error context can be inferred from the command's context. Say, you fail to
> create a file where only one file could be created.

So how do you like the 'transaction' command? :-)

But even if you could infer the context, the possible error details may
still differ in the possible contexts. With -blockdev this will become
even more fun, because even for bdrv_open you may or may not have a
filename. Maybe you rather have a hostname, port and sheepdog volume ID.
Or directory name, FAT type and stuff.

> But yes, we need to add a way for errors to accept optional parameters.
> 
> I have a series that's almost done that moves all errors to a qapi-error.json
> file and auto-generates the QErrors tables. Maybe that's a first step towards
> more flexible errors.
> 
>> I think 'OpenFileFailed' is not too bad, it's just missing a field that
>> gives the detail 'PermissionDenied'. I'm not sure if having
>> 'PermissionDenied' as the top-level error object is the best idea.
> 
> I think the end result is the same, but I prefer PermissionDenied as the
> top level because it's slightly simpler.

How would you do the conversion in a compatible way?

Kevin

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

[Luiz Capitulino]

On Mon, 26 Mar 2012 16:47:51 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 26.03.2012 16:33, schrieb Luiz Capitulino:
> > On Mon, 26 Mar 2012 16:04:26 +0200
> > Kevin Wolf <kwolf@redhat.com> wrote:
> > 
> >>>> 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.
> >>
> >> Your PermissionDenied example doesn't really do this. It is a generic
> >> error message that may occur in multiple contexts, right? So in one
> >> context you may need a file name as additional information, in another
> >> context permission for something completely different may be missing
> >> (especially if you include EPERM in the same error).
> > 
> > Yes, but it's not a severe problem. Not yet. Because most of the time the
> > error context can be inferred from the command's context. Say, you fail to
> > create a file where only one file could be created.
> 
> So how do you like the 'transaction' command? :-)

That's the only exception I think. If this is hurting you, then we have two
options: think of a way to add optional parameters to errors right now or
add new QERR_s macros that accept the filename argument (note that we wouldn't
add a new error, just a macro that accepts a new argument).

> But even if you could infer the context, the possible error details may
> still differ in the possible contexts. With -blockdev this will become
> even more fun, because even for bdrv_open you may or may not have a
> filename. Maybe you rather have a hostname, port and sheepdog volume ID.
> Or directory name, FAT type and stuff.

Right, and the solution for this is really extending the error infra to
accept optional parameters. Although I don't know how to support different
error messages.

> > But yes, we need to add a way for errors to accept optional parameters.
> > 
> > I have a series that's almost done that moves all errors to a qapi-error.json
> > file and auto-generates the QErrors tables. Maybe that's a first step towards
> > more flexible errors.
> > 
> >> I think 'OpenFileFailed' is not too bad, it's just missing a field that
> >> gives the detail 'PermissionDenied'. I'm not sure if having
> >> 'PermissionDenied' as the top-level error object is the best idea.
> > 
> > I think the end result is the same, but I prefer PermissionDenied as the
> > top level because it's slightly simpler.
> 
> How would you do the conversion in a compatible way?

Returning additional errors is not incompatible.

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

[Kevin Wolf]

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().

> But it would be even better to push Error ** into bdrv_img_create().  There's 
> only two callers so it would be trivial to do that.  Then you could do:
> 
> diff --git a/block.c b/block.c
> index b88ee90..a7bf8a9 100644
> --- a/block.c
> +++ b/block.c
> @@ -3881,7 +3881,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cook
> 
>   int bdrv_img_create(const char *filename, const char *fmt,
>                       const char *base_filename, const char *base_fmt,
> -                    char *options, uint64_t img_size, int flags)
> +                    char *options, uint64_t img_size, int flags,
> +                    Error **errp)
>   {
>       QEMUOptionParameter *param = NULL, *create_options = NULL;
>       QEMUOptionParameter *backing_fmt, *backing_file, *size;
> @@ -3893,14 +3894,14 @@ int bdrv_img_create(const char *filename, const char *fm
>       /* Find driver and parse its options */
>       drv = bdrv_find_format(fmt);
>       if (!drv) {
> -        error_report("Unknown file format '%s'", fmt);
> +        error_set(errp, QERR_INVALID_BLOCK_FORMAT, fmt);
>           ret = -EINVAL;
>           goto out;
>       }
> 
> Etc.

Yes, but that's a completely independent problem.

>> 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)
>>
>> How should management tools ever be able to provide a helpful error
>> message to their users if all they get is this useless "something went
>> wrong" error?
> 
> You need to kill off error_report in the block layer and replace it with 
> error_set.  The problem with error_report is that while you can understand what 
> "Unknown file format 'qcow2'" means, management tools can't.  Responding that 
> "the tool can just present that error to the user" implies that the management 
> tool only provides an English-language interface which is not terribly friendly.
> 
> 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 could add
tests for specific errors (like you suggested above) in every single
place that sets an error, but I'd rather not. It would make the code
verbose and the error reporting probably inconsistent, if not buggy.

Kevin

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

[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.

>
>> But it would be even better to push Error ** into bdrv_img_create().  There's
>> only two callers so it would be trivial to do that.  Then you could do:
>>
>> diff --git a/block.c b/block.c
>> index b88ee90..a7bf8a9 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3881,7 +3881,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cook
>>
>>    int bdrv_img_create(const char *filename, const char *fmt,
>>                        const char *base_filename, const char *base_fmt,
>> -                    char *options, uint64_t img_size, int flags)
>> +                    char *options, uint64_t img_size, int flags,
>> +                    Error **errp)
>>    {
>>        QEMUOptionParameter *param = NULL, *create_options = NULL;
>>        QEMUOptionParameter *backing_fmt, *backing_file, *size;
>> @@ -3893,14 +3894,14 @@ int bdrv_img_create(const char *filename, const char *fm
>>        /* Find driver and parse its options */
>>        drv = bdrv_find_format(fmt);
>>        if (!drv) {
>> -        error_report("Unknown file format '%s'", fmt);
>> +        error_set(errp, QERR_INVALID_BLOCK_FORMAT, fmt);
>>            ret = -EINVAL;
>>            goto out;
>>        }
>>
>> Etc.
>
> 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.

>>> 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)
>>>
>>> How should management tools ever be able to provide a helpful error
>>> message to their users if all they get is this useless "something went
>>> wrong" error?
>>
>> You need to kill off error_report in the block layer and replace it with
>> error_set.  The problem with error_report is that while you can understand what
>> "Unknown file format 'qcow2'" means, management tools can't.  Responding that
>> "the tool can just present that error to the user" implies that the management
>> tool only provides an English-language interface which is not terribly friendly.
>>
>> 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.

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.

> I could add
> tests for specific errors (like you suggested above) in every single
> place that sets an error, but I'd rather not. It would make the code
> verbose and the error reporting probably inconsistent, if not buggy.

We have a lot of:

error_report("Some english string\n");
return -ERANDOMERRORCODE;

This idiom does not make for good on the wire errors.  You can replace these 
lines with a single error_set() call.  There's no need for switching.

Regards,

Anthony Liguori

> Kevin

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

[Kevin Wolf]

Am 26.03.2012 16:54, schrieb Luiz Capitulino:
> On Mon, 26 Mar 2012 16:47:51 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> Am 26.03.2012 16:33, schrieb Luiz Capitulino:
>>> On Mon, 26 Mar 2012 16:04:26 +0200
>>> Kevin Wolf <kwolf@redhat.com> wrote:
>>>
>>>>>> 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.
>>>>
>>>> Your PermissionDenied example doesn't really do this. It is a generic
>>>> error message that may occur in multiple contexts, right? So in one
>>>> context you may need a file name as additional information, in another
>>>> context permission for something completely different may be missing
>>>> (especially if you include EPERM in the same error).
>>>
>>> Yes, but it's not a severe problem. Not yet. Because most of the time the
>>> error context can be inferred from the command's context. Say, you fail to
>>> create a file where only one file could be created.
>>
>> So how do you like the 'transaction' command? :-)
> 
> That's the only exception I think. If this is hurting you, then we have two
> options: think of a way to add optional parameters to errors right now or
> add new QERR_s macros that accept the filename argument (note that we wouldn't
> add a new error, just a macro that accepts a new argument).

It isn't hurting anyone yet, but if we don't design it in, I'm pretty
sure that it will hurt us badly at some point.

>> But even if you could infer the context, the possible error details may
>> still differ in the possible contexts. With -blockdev this will become
>> even more fun, because even for bdrv_open you may or may not have a
>> filename. Maybe you rather have a hostname, port and sheepdog volume ID.
>> Or directory name, FAT type and stuff.
> 
> Right, and the solution for this is really extending the error infra to
> accept optional parameters. Although I don't know how to support different
> error messages.

Does this really solve the problem? Wouldn't you end up with one Error
object that has optional parameters for the sheepdog volume name,
network backend, port number of the tcp chardev, etc.? Would this be any
better than just having a QDict with arbitrary content?

This is why I believe that having a specific error is much better, there
are just too many types of PermissionDenied or NotFound.

Or maybe we need inheritance. Wait, I didn't say that...

Kevin

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

[Kevin Wolf]

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.

>> 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.

>>>> 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)
>>>>
>>>> How should management tools ever be able to provide a helpful error
>>>> message to their users if all they get is this useless "something went
>>>> wrong" error?
>>>
>>> You need to kill off error_report in the block layer and replace it with
>>> error_set.  The problem with error_report is that while you can understand what
>>> "Unknown file format 'qcow2'" means, management tools can't.  Responding that
>>> "the tool can just present that error to the user" implies that the management
>>> tool only provides an English-language interface which is not terribly friendly.
>>>
>>> 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...

> 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.

>> I could add
>> tests for specific errors (like you suggested above) in every single
>> place that sets an error, but I'd rather not. It would make the code
>> verbose and the error reporting probably inconsistent, if not buggy.
> 
> We have a lot of:
> 
> error_report("Some english string\n");
> return -ERANDOMERRORCODE;
> 
> This idiom does not make for good on the wire errors.  You can replace these 
> lines with a single error_set() call.  There's no need for switching.

But this is not the case I have asked for.

Kevin

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

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

[Kevin Wolf]

Am 26.03.2012 17:38, schrieb 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.

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

So your error types will end up accumulating optional parameters for all
contexts in which they can occur?

In the long run, PermissionDenied would have an optional file name (for
raw), optional host name, port, sheepdog volume ID (for sheepdog),
optional source and destination block devices (for blkmirror), remote
host and port, local address and port (for UDP chardevs)...

I could go on forever. Does this really make sense?

Kevin

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

[Anthony Liguori]

On 03/26/2012 10:59 AM, Kevin Wolf wrote:
> Am 26.03.2012 17:38, schrieb Anthony Liguori:
>> You can add parameters to Errors in a fully compatible fashion, so just add an
>> filename parameter to PermissionDenied.  Problem solved.
>
> So your error types will end up accumulating optional parameters for all
> contexts in which they can occur?
>
> In the long run, PermissionDenied would have an optional file name (for
> raw), optional host name, port, sheepdog volume ID (for sheepdog),
> optional source and destination block devices (for blkmirror), remote
> host and port, local address and port (for UDP chardevs)...

I don't see the generalization.

>
> I could go on forever. Does this really make sense?

What's the alternative proposal?  If you just add errno to OPEN_FILE_FAILED, 
then you end up with errors for SHEEP_DOG_ATTACH_VOLUME_FAILED, 
NBD_CONNECT_FAILED, etc.

The proper way to design an error is to make it a verb, and have parameters that 
correspond to the direct object and/or indirect object.  The subject is implied 
by the command itself.

So in the case of block_snapshot_sync, the failure is:

The block_snapshot_sync command failed due to insufficient permission when 
creating foo.img.

So the error is "PERMISSION_DENIED", with the data "filename=foo.img"

In the case of NBD, the error would be:

The block_snapshot_sync command failed because the host localhost:42 could not 
be contacted.

The error is "CONNECTION_REFUSED", with the data "hostname=localhost,port=42".

It the block layer returns both of these as EACCESS today, then this is a good 
argument to refactor the block layer to take an Error object instead of 
overloading the meaning of EACCESS.

Regards,

Anthony Liguori

>
> Kevin