[Qemu-devel] [PATCH 0/2]: QMP: expose errno in the BLOCK_IO_ERROR event

[Qemu-devel] [PATCH 0/2]: QMP: expose errno in the BLOCK_IO_ERROR event

[Luiz Capitulino]

 Details in the patches.

[Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Luiz Capitulino]

We need to expose errno in QMP, for three reasons:

  1. Some error handling functions print errno codes to the user,
     while it's debatable whether this is good or not from a user
     perspective, sometimes it's the best we can do because it's
     what system calls and libraries return

  2. Some events (eg. BLOCK_IO_ERROR) will be made even more
     complete with errno information

  3. It's very good for debugging

So, we need a way to expose those codes in QMP. We can't just use
the codes themselfs because they may vary between systems.

The best solution I can think of is to return the string
representation of the name. For example, EIO becomes "EIO".

This is what get_errno_name() does.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-error.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-error.h |    1 +
 2 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/qemu-error.c b/qemu-error.c
index 5a35e7c..7035417 100644
--- a/qemu-error.c
+++ b/qemu-error.c
@@ -207,3 +207,88 @@ void error_report(const char *fmt, ...)
     va_end(ap);
     error_printf("\n");
 }
+
+/*
+ * Probably only useful for QMP
+ */
+const char *get_errno_name(int err)
+{
+    switch (abs(err)) {
+    case EPERM:
+        return "EPERM";
+    case ENOENT:
+        return "ENOENT";
+    case ESRCH:
+        return "ESRCH";
+    case EINTR:
+        return "EINTR";
+    case EIO:
+        return "EIO";
+    case ENXIO:
+        return "ENXIO";
+    case E2BIG:
+        return "E2BIG";
+    case ENOEXEC:
+        return "ENOEXEC";
+    case EBADF:
+        return "EBADF";
+    case ECHILD:
+        return "ECHILD";
+    case EAGAIN:
+        return "EAGAIN";
+    case ENOMEM:
+        return "ENOMEM";
+    case EACCES:
+        return "EACCES";
+    case EFAULT:
+        return "EFAULT";
+    case ENOTBLK:
+        return "ENOTBLK";
+    case EBUSY:
+        return "EBUSY";
+    case EEXIST:
+        return "EEXIST";
+    case EXDEV:
+        return "EXDEV";
+    case ENODEV:
+        return "ENODEV";
+    case ENOTDIR:
+        return "ENOTDIR";
+    case EISDIR:
+        return "EISDIR";
+    case EINVAL:
+        return "EINVAL";
+    case ENFILE:
+        return "ENFILE";
+    case EMFILE:
+        return "EMFILE";
+    case ENOTTY:
+        return "ENOTTY";
+    case ETXTBSY:
+        return "ETXTBSY";
+    case EFBIG:
+        return "EFBIG";
+    case ENOSPC:
+        return "ENOSPC";
+    case ESPIPE:
+        return "ESPIPE";
+    case EROFS:
+        return "EROFS";
+    case EMLINK:
+        return "EMLINK";
+    case EPIPE:
+        return "EPIPE";
+    case EDOM:
+        return "EDOM";
+    case ERANGE:
+        return "ERANGE";
+    case ENOMEDIUM:
+        return "ENOMEDIUM";
+    case ENOTSUP:
+        return "ENOTSUP";
+    default:
+        return "unknown";
+    }
+
+    abort();
+}
diff --git a/qemu-error.h b/qemu-error.h
index a45609f..c5e39b5 100644
--- a/qemu-error.h
+++ b/qemu-error.h
@@ -38,4 +38,5 @@ void error_print_loc(void);
 void error_set_progname(const char *argv0);
 void error_report(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
 
+const char *get_errno_name(int err);
 #endif
-- 
1.7.1.rc1.12.ga6018

[Qemu-devel] [PATCH 2/2] QMP: Add 'reason' member to the BLOCK_IO_ERROR event

[Luiz Capitulino]

It's a parsable errno string representation, this is needed
because some management tools want to base their action on
the error cause.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt |    4 +++-
 block.c            |    8 +++++---
 block.h            |    2 +-
 hw/ide/core.c      |    6 +++---
 hw/scsi-disk.c     |    6 +++---
 hw/virtio-blk.c    |    6 +++---
 6 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 01ec85f..47097fd 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -14,13 +14,15 @@ Data:
     "ignore": error has been ignored
     "report": error has been reported to the device
     "stop": error caused VM to be stopped
+- "reason": errno string representation (json-string)
 
 Example:
 
 { "event": "BLOCK_IO_ERROR",
     "data": { "device": "ide0-hd1",
               "operation": "write",
-              "action": "stop" },
+              "action": "stop",
+              "reason": "EINTR" },
     "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 
 Note: If action is "stop", a STOP event will eventually follow the
diff --git a/block.c b/block.c
index 7974215..1179b1c 100644
--- a/block.c
+++ b/block.c
@@ -1236,7 +1236,7 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 }
 
 void bdrv_mon_event(const BlockDriverState *bdrv,
-                    BlockMonEventAction action, int is_read)
+                    BlockMonEventAction action, int error, int is_read)
 {
     QObject *data;
     const char *action_str;
@@ -1255,10 +1255,12 @@ void bdrv_mon_event(const BlockDriverState *bdrv,
         abort();
     }
 
-    data = qobject_from_jsonf("{ 'device': %s, 'action': %s, 'operation': %s }",
+    data = qobject_from_jsonf("{ 'device': %s, 'action': %s, "
+                              "'operation': %s, 'reason': %s }",
                               bdrv->device_name,
                               action_str,
-                              is_read ? "read" : "write");
+                              is_read ? "read" : "write",
+                              get_errno_name(errno));
     monitor_protocol_event(QEVENT_BLOCK_IO_ERROR, data);
 
     qobject_decref(data);
diff --git a/block.h b/block.h
index 05ad572..9696081 100644
--- a/block.h
+++ b/block.h
@@ -45,7 +45,7 @@ typedef enum {
 } BlockMonEventAction;
 
 void bdrv_mon_event(const BlockDriverState *bdrv,
-                    BlockMonEventAction action, int is_read);
+                    BlockMonEventAction action, int error, int is_read);
 void bdrv_info_print(Monitor *mon, const QObject *data);
 void bdrv_info(Monitor *mon, QObject **ret_data);
 void bdrv_stats_print(Monitor *mon, const QObject *data);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0757528..1ce0cdd 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -484,7 +484,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
     BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
 
     if (action == BLOCK_ERR_IGNORE) {
-        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
+        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, error, is_read);
         return 0;
     }
 
@@ -492,7 +492,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
             || action == BLOCK_ERR_STOP_ANY) {
         s->bus->bmdma->unit = s->unit;
         s->bus->bmdma->status |= op;
-        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
+        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, error, is_read);
         vm_stop(0);
     } else {
         if (op & BM_STATUS_DMA_RETRY) {
@@ -501,7 +501,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
         } else {
             ide_rw_error(s);
         }
-        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
+        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, error, is_read);
     }
 
     return 1;
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 77cb1da..1df9552 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -182,19 +182,19 @@ static int scsi_handle_write_error(SCSIDiskReq *r, int error)
     BlockInterfaceErrorAction action = drive_get_on_error(s->bs, 0);
 
     if (action == BLOCK_ERR_IGNORE) {
-        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, 0);
+        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, error, 0);
         return 0;
     }
 
     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
         r->status |= SCSI_REQ_STATUS_RETRY;
-        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, 0);
+        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, error, 0);
         vm_stop(0);
     } else {
         scsi_command_complete(r, CHECK_CONDITION,
                 HARDWARE_ERROR);
-        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, 0);
+        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, error, 0);
     }
 
     return 1;
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index b05d15e..b0c504e 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -65,7 +65,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
     VirtIOBlock *s = req->dev;
 
     if (action == BLOCK_ERR_IGNORE) {
-        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
+        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, error, is_read);
         return 0;
     }
 
@@ -73,11 +73,11 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
             || action == BLOCK_ERR_STOP_ANY) {
         req->next = s->rq;
         s->rq = req;
-        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
+        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, error, is_read);
         vm_stop(0);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
-        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
+        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, error, is_read);
     }
 
     return 1;
-- 
1.7.1.rc1.12.ga6018

Re: [Qemu-devel] [PATCH 2/2] QMP: Add 'reason' member to the BLOCK_IO_ERROR event

[Anthony Liguori]

On 04/28/2010 03:32 PM, Luiz Capitulino wrote:
> It's a parsable errno string representation, this is needed
> because some management tools want to base their action on
> the error cause.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>    

Does anyone differentiate beyond ENOSPC and EIO?

Regards,

Anthony Liguori

> ---
>   QMP/qmp-events.txt |    4 +++-
>   block.c            |    8 +++++---
>   block.h            |    2 +-
>   hw/ide/core.c      |    6 +++---
>   hw/scsi-disk.c     |    6 +++---
>   hw/virtio-blk.c    |    6 +++---
>   6 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index 01ec85f..47097fd 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -14,13 +14,15 @@ Data:
>       "ignore": error has been ignored
>       "report": error has been reported to the device
>       "stop": error caused VM to be stopped
> +- "reason": errno string representation (json-string)
>
>   Example:
>
>   { "event": "BLOCK_IO_ERROR",
>       "data": { "device": "ide0-hd1",
>                 "operation": "write",
> -              "action": "stop" },
> +              "action": "stop",
> +              "reason": "EINTR" },
>       "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>
>   Note: If action is "stop", a STOP event will eventually follow the
> diff --git a/block.c b/block.c
> index 7974215..1179b1c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1236,7 +1236,7 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>   }
>
>   void bdrv_mon_event(const BlockDriverState *bdrv,
> -                    BlockMonEventAction action, int is_read)
> +                    BlockMonEventAction action, int error, int is_read)
>   {
>       QObject *data;
>       const char *action_str;
> @@ -1255,10 +1255,12 @@ void bdrv_mon_event(const BlockDriverState *bdrv,
>           abort();
>       }
>
> -    data = qobject_from_jsonf("{ 'device': %s, 'action': %s, 'operation': %s }",
> +    data = qobject_from_jsonf("{ 'device': %s, 'action': %s, "
> +                              "'operation': %s, 'reason': %s }",
>                                 bdrv->device_name,
>                                 action_str,
> -                              is_read ? "read" : "write");
> +                              is_read ? "read" : "write",
> +                              get_errno_name(errno));
>       monitor_protocol_event(QEVENT_BLOCK_IO_ERROR, data);
>
>       qobject_decref(data);
> diff --git a/block.h b/block.h
> index 05ad572..9696081 100644
> --- a/block.h
> +++ b/block.h
> @@ -45,7 +45,7 @@ typedef enum {
>   } BlockMonEventAction;
>
>   void bdrv_mon_event(const BlockDriverState *bdrv,
> -                    BlockMonEventAction action, int is_read);
> +                    BlockMonEventAction action, int error, int is_read);
>   void bdrv_info_print(Monitor *mon, const QObject *data);
>   void bdrv_info(Monitor *mon, QObject **ret_data);
>   void bdrv_stats_print(Monitor *mon, const QObject *data);
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 0757528..1ce0cdd 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -484,7 +484,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>       BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
>
>       if (action == BLOCK_ERR_IGNORE) {
> -        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, error, is_read);
>           return 0;
>       }
>
> @@ -492,7 +492,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>               || action == BLOCK_ERR_STOP_ANY) {
>           s->bus->bmdma->unit = s->unit;
>           s->bus->bmdma->status |= op;
> -        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, error, is_read);
>           vm_stop(0);
>       } else {
>           if (op&  BM_STATUS_DMA_RETRY) {
> @@ -501,7 +501,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>           } else {
>               ide_rw_error(s);
>           }
> -        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, error, is_read);
>       }
>
>       return 1;
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 77cb1da..1df9552 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -182,19 +182,19 @@ static int scsi_handle_write_error(SCSIDiskReq *r, int error)
>       BlockInterfaceErrorAction action = drive_get_on_error(s->bs, 0);
>
>       if (action == BLOCK_ERR_IGNORE) {
> -        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, 0);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, error, 0);
>           return 0;
>       }
>
>       if ((error == ENOSPC&&  action == BLOCK_ERR_STOP_ENOSPC)
>               || action == BLOCK_ERR_STOP_ANY) {
>           r->status |= SCSI_REQ_STATUS_RETRY;
> -        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, 0);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, error, 0);
>           vm_stop(0);
>       } else {
>           scsi_command_complete(r, CHECK_CONDITION,
>                   HARDWARE_ERROR);
> -        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, 0);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, error, 0);
>       }
>
>       return 1;
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index b05d15e..b0c504e 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -65,7 +65,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
>       VirtIOBlock *s = req->dev;
>
>       if (action == BLOCK_ERR_IGNORE) {
> -        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, error, is_read);
>           return 0;
>       }
>
> @@ -73,11 +73,11 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
>               || action == BLOCK_ERR_STOP_ANY) {
>           req->next = s->rq;
>           s->rq = req;
> -        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, error, is_read);
>           vm_stop(0);
>       } else {
>           virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> -        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, error, is_read);
>       }
>
>       return 1;
>    

Re: [Qemu-devel] [PATCH 2/2] QMP: Add 'reason' member to the BLOCK_IO_ERROR event

[Luiz Capitulino]

On Wed, 28 Apr 2010 18:24:29 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 04/28/2010 03:32 PM, Luiz Capitulino wrote:
> > It's a parsable errno string representation, this is needed
> > because some management tools want to base their action on
> > the error cause.
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >    
> 
> Does anyone differentiate beyond ENOSPC and EIO?

 For this specific case, I guess not.

 But this general solution is also needed in other places like
savevm, loadvm and delvm I was working (and sent) an RFC last week,
also I think other events and errors might need this in the near future.

 IOW, I expect this to be a QMP idiom.

Re: [Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> We need to expose errno in QMP, for three reasons:
>
>   1. Some error handling functions print errno codes to the user,
>      while it's debatable whether this is good or not from a user
>      perspective, sometimes it's the best we can do because it's
>      what system calls and libraries return
>
>   2. Some events (eg. BLOCK_IO_ERROR) will be made even more
>      complete with errno information
>
>   3. It's very good for debugging
>
> So, we need a way to expose those codes in QMP. We can't just use
> the codes themselfs because they may vary between systems.
>
> The best solution I can think of is to return the string
> representation of the name. For example, EIO becomes "EIO".
>
> This is what get_errno_name() does.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-error.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-error.h |    1 +
>  2 files changed, 86 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-error.c b/qemu-error.c
> index 5a35e7c..7035417 100644
> --- a/qemu-error.c
> +++ b/qemu-error.c
> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...)
>      va_end(ap);
>      error_printf("\n");
>  }
> +
> +/*
> + * Probably only useful for QMP
> + */
> +const char *get_errno_name(int err)
> +{
> +    switch (abs(err)) {
> +    case EPERM:
> +        return "EPERM";
> +    case ENOENT:
> +        return "ENOENT";
[...]
> +    case EDOM:
> +        return "EDOM";
> +    case ERANGE:
> +        return "ERANGE";
> +    case ENOMEDIUM:
> +        return "ENOMEDIUM";
> +    case ENOTSUP:
> +        return "ENOTSUP";
> +    default:
> +        return "unknown";

How did you choose the codes to implement?  POSIX has many more...

> +    }
> +
> +    abort();
> +}
> diff --git a/qemu-error.h b/qemu-error.h
> index a45609f..c5e39b5 100644
> --- a/qemu-error.h
> +++ b/qemu-error.h
> @@ -38,4 +38,5 @@ void error_print_loc(void);
>  void error_set_progname(const char *argv0);
>  void error_report(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
>  
> +const char *get_errno_name(int err);
>  #endif

Re: [Qemu-devel] [PATCH 2/2] QMP: Add 'reason' member to the BLOCK_IO_ERROR event

[Anthony Liguori]

On 04/29/2010 12:30 PM, Luiz Capitulino wrote:
> On Wed, 28 Apr 2010 18:24:29 -0500
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 04/28/2010 03:32 PM, Luiz Capitulino wrote:
>>      
>>> It's a parsable errno string representation, this is needed
>>> because some management tools want to base their action on
>>> the error cause.
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>>
>>>        
>> Does anyone differentiate beyond ENOSPC and EIO?
>>      
>   For this specific case, I guess not.
>
>   But this general solution is also needed in other places like
> savevm, loadvm and delvm I was working (and sent) an RFC last week,
> also I think other events and errors might need this in the near future.
>
>   IOW, I expect this to be a QMP idiom.
>    

I think that's a bit of a crutch.  What's the point of QError if it's 
just encapsulating errno's?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Anthony Liguori]

On 05/03/2010 08:06 AM, Markus Armbruster wrote:
> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>
>    
>> We need to expose errno in QMP, for three reasons:
>>
>>    1. Some error handling functions print errno codes to the user,
>>       while it's debatable whether this is good or not from a user
>>       perspective, sometimes it's the best we can do because it's
>>       what system calls and libraries return
>>
>>    2. Some events (eg. BLOCK_IO_ERROR) will be made even more
>>       complete with errno information
>>
>>    3. It's very good for debugging
>>
>> So, we need a way to expose those codes in QMP. We can't just use
>> the codes themselfs because they may vary between systems.
>>
>> The best solution I can think of is to return the string
>> representation of the name. For example, EIO becomes "EIO".
>>
>> This is what get_errno_name() does.
>>
>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>> ---
>>   qemu-error.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qemu-error.h |    1 +
>>   2 files changed, 86 insertions(+), 0 deletions(-)
>>
>> diff --git a/qemu-error.c b/qemu-error.c
>> index 5a35e7c..7035417 100644
>> --- a/qemu-error.c
>> +++ b/qemu-error.c
>> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...)
>>       va_end(ap);
>>       error_printf("\n");
>>   }
>> +
>> +/*
>> + * Probably only useful for QMP
>> + */
>> +const char *get_errno_name(int err)
>> +{
>> +    switch (abs(err)) {
>> +    case EPERM:
>> +        return "EPERM";
>> +    case ENOENT:
>> +        return "ENOENT";
>>      
> [...]
>    
>> +    case EDOM:
>> +        return "EDOM";
>> +    case ERANGE:
>> +        return "ERANGE";
>> +    case ENOMEDIUM:
>> +        return "ENOMEDIUM";
>> +    case ENOTSUP:
>> +        return "ENOTSUP";
>> +    default:
>> +        return "unknown";
>>      
> How did you choose the codes to implement?  POSIX has many more...
>    

Let me say another way why I think this is a bad path to go down.

In generally, we could never just pass errno through down.  Different 
host platforms are going to generate different errno values so we really 
need to filter and send reliable errno values so that clients don't have 
to have special code for when they're on Linux vs. AIX vs. Solaris.

If we're white listing errno values, we should be able to trivially 
convert errnos to QError types via a table just like you have above.

Regards,

Anthony Liguori

>> +    }
>> +
>> +    abort();
>> +}
>> diff --git a/qemu-error.h b/qemu-error.h
>> index a45609f..c5e39b5 100644
>> --- a/qemu-error.h
>> +++ b/qemu-error.h
>> @@ -38,4 +38,5 @@ void error_print_loc(void);
>>   void error_set_progname(const char *argv0);
>>   void error_report(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
>>
>> +const char *get_errno_name(int err);
>>   #endif
>>      
>
>    

Re: [Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Luiz Capitulino]

On Mon, 03 May 2010 08:16:35 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/03/2010 08:06 AM, Markus Armbruster wrote:
> > Luiz Capitulino<lcapitulino@redhat.com>  writes:
> >
> >    
> >> We need to expose errno in QMP, for three reasons:
> >>
> >>    1. Some error handling functions print errno codes to the user,
> >>       while it's debatable whether this is good or not from a user
> >>       perspective, sometimes it's the best we can do because it's
> >>       what system calls and libraries return
> >>
> >>    2. Some events (eg. BLOCK_IO_ERROR) will be made even more
> >>       complete with errno information
> >>
> >>    3. It's very good for debugging
> >>
> >> So, we need a way to expose those codes in QMP. We can't just use
> >> the codes themselfs because they may vary between systems.
> >>
> >> The best solution I can think of is to return the string
> >> representation of the name. For example, EIO becomes "EIO".
> >>
> >> This is what get_errno_name() does.
> >>
> >> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >> ---
> >>   qemu-error.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   qemu-error.h |    1 +
> >>   2 files changed, 86 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/qemu-error.c b/qemu-error.c
> >> index 5a35e7c..7035417 100644
> >> --- a/qemu-error.c
> >> +++ b/qemu-error.c
> >> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...)
> >>       va_end(ap);
> >>       error_printf("\n");
> >>   }
> >> +
> >> +/*
> >> + * Probably only useful for QMP
> >> + */
> >> +const char *get_errno_name(int err)
> >> +{
> >> +    switch (abs(err)) {
> >> +    case EPERM:
> >> +        return "EPERM";
> >> +    case ENOENT:
> >> +        return "ENOENT";
> >>      
> > [...]
> >    
> >> +    case EDOM:
> >> +        return "EDOM";
> >> +    case ERANGE:
> >> +        return "ERANGE";
> >> +    case ENOMEDIUM:
> >> +        return "ENOMEDIUM";
> >> +    case ENOTSUP:
> >> +        return "ENOTSUP";
> >> +    default:
> >> +        return "unknown";
> >>      
> > How did you choose the codes to implement?  POSIX has many more...

 I just ran an awk script on the linux's base errno header file, my
idea is just to have the common names, anything 'new' will hit the
default clause and we can add it later.

> Let me say another way why I think this is a bad path to go down.
> 
> In generally, we could never just pass errno through down.  Different 
> host platforms are going to generate different errno values so we really 
> need to filter and send reliable errno values so that clients don't have 
> to have special code for when they're on Linux vs. AIX vs. Solaris.

 Sorry for the potential stupid question, but what would a 'reliable'
errno be? Or, what's an unreliable errno?

 We're not sending plain integers to the clients, so the only problem
I can see is if different unices return different errnos for the
same error. Is that the problem you're seeing?

> If we're white listing errno values, we should be able to trivially 
> convert errnos to QError types via a table just like you have above.

 Having a direct errno -> QError mapping doesn't seem good for the
current use cases.

 For example, do_savevm() (not merged yet) has a StateVmSaveFailed
error which also has a 'reason' member, which would look like this
on the wire:

"{ 'class': 'StateVmSaveFailed', 'data': { 'reason': "EIO" } }"

 So, the QError class says what has failed and the errno part says
why it has failed.

 I'd be happy to implement a different solution that satisfies this
basic requirement.

Re: [Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Anthony Liguori]

On 05/04/2010 08:56 AM, Luiz Capitulino wrote:
> On Mon, 03 May 2010 08:16:35 -0500
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 05/03/2010 08:06 AM, Markus Armbruster wrote:
>>      
>>> Luiz Capitulino<lcapitulino@redhat.com>   writes:
>>>
>>>
>>>        
>>>> We need to expose errno in QMP, for three reasons:
>>>>
>>>>     1. Some error handling functions print errno codes to the user,
>>>>        while it's debatable whether this is good or not from a user
>>>>        perspective, sometimes it's the best we can do because it's
>>>>        what system calls and libraries return
>>>>
>>>>     2. Some events (eg. BLOCK_IO_ERROR) will be made even more
>>>>        complete with errno information
>>>>
>>>>     3. It's very good for debugging
>>>>
>>>> So, we need a way to expose those codes in QMP. We can't just use
>>>> the codes themselfs because they may vary between systems.
>>>>
>>>> The best solution I can think of is to return the string
>>>> representation of the name. For example, EIO becomes "EIO".
>>>>
>>>> This is what get_errno_name() does.
>>>>
>>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>>> ---
>>>>    qemu-error.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    qemu-error.h |    1 +
>>>>    2 files changed, 86 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/qemu-error.c b/qemu-error.c
>>>> index 5a35e7c..7035417 100644
>>>> --- a/qemu-error.c
>>>> +++ b/qemu-error.c
>>>> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...)
>>>>        va_end(ap);
>>>>        error_printf("\n");
>>>>    }
>>>> +
>>>> +/*
>>>> + * Probably only useful for QMP
>>>> + */
>>>> +const char *get_errno_name(int err)
>>>> +{
>>>> +    switch (abs(err)) {
>>>> +    case EPERM:
>>>> +        return "EPERM";
>>>> +    case ENOENT:
>>>> +        return "ENOENT";
>>>>
>>>>          
>>> [...]
>>>
>>>        
>>>> +    case EDOM:
>>>> +        return "EDOM";
>>>> +    case ERANGE:
>>>> +        return "ERANGE";
>>>> +    case ENOMEDIUM:
>>>> +        return "ENOMEDIUM";
>>>> +    case ENOTSUP:
>>>> +        return "ENOTSUP";
>>>> +    default:
>>>> +        return "unknown";
>>>>
>>>>          
>>> How did you choose the codes to implement?  POSIX has many more...
>>>        
>   I just ran an awk script on the linux's base errno header file, my
> idea is just to have the common names, anything 'new' will hit the
> default clause and we can add it later.
>
>    
>> Let me say another way why I think this is a bad path to go down.
>>
>> In generally, we could never just pass errno through down.  Different
>> host platforms are going to generate different errno values so we really
>> need to filter and send reliable errno values so that clients don't have
>> to have special code for when they're on Linux vs. AIX vs. Solaris.
>>      
>   Sorry for the potential stupid question, but what would a 'reliable'
> errno be? Or, what's an unreliable errno?
>
>   We're not sending plain integers to the clients, so the only problem
> I can see is if different unices return different errnos for the
> same error. Is that the problem you're seeing?
>    

Different types of platforms return different errno values for the same 
error type.

As an example, on Linux, connect() returns EINPROGRESS when you set the 
socket non-blocking.  On Windows, it returns EWOULDBLOCK.

If you just pass through errno, then all QMP clients are going to have 
to know the different between QEMU on Windows and Linux and handle the 
errnos appropriately.

>> If we're white listing errno values, we should be able to trivially
>> convert errnos to QError types via a table just like you have above.
>>      
>   Having a direct errno ->  QError mapping doesn't seem good for the
> current use cases.
>
>   For example, do_savevm() (not merged yet) has a StateVmSaveFailed
> error which also has a 'reason' member, which would look like this
> on the wire:
>
> "{ 'class': 'StateVmSaveFailed', 'data': { 'reason': "EIO" } }"
>
>   So, the QError class says what has failed and the errno part says
> why it has failed.
>
>   I'd be happy to implement a different solution that satisfies this
> basic requirement.
>    

We need to map errnos to some QMP defined error type.  However, as I've 
said before, "reason" is usually an indicator that an error is bad.

A better error would be:

{ 'class': 'SocketIOError': 'data' : { 'source': 'migration' }}

Or something like that.  You don't want to have command and then 
CommandFailed as the error.  You want the errors to be the 'reason's for 
the commands failure.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Luiz Capitulino]

On Tue, 04 May 2010 09:03:47 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/04/2010 08:56 AM, Luiz Capitulino wrote:
> > On Mon, 03 May 2010 08:16:35 -0500
> > Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >
> >    
> >> On 05/03/2010 08:06 AM, Markus Armbruster wrote:
> >>      
> >>> Luiz Capitulino<lcapitulino@redhat.com>   writes:
> >>>
> >>>
> >>>        
> >>>> We need to expose errno in QMP, for three reasons:
> >>>>
> >>>>     1. Some error handling functions print errno codes to the user,
> >>>>        while it's debatable whether this is good or not from a user
> >>>>        perspective, sometimes it's the best we can do because it's
> >>>>        what system calls and libraries return
> >>>>
> >>>>     2. Some events (eg. BLOCK_IO_ERROR) will be made even more
> >>>>        complete with errno information
> >>>>
> >>>>     3. It's very good for debugging
> >>>>
> >>>> So, we need a way to expose those codes in QMP. We can't just use
> >>>> the codes themselfs because they may vary between systems.
> >>>>
> >>>> The best solution I can think of is to return the string
> >>>> representation of the name. For example, EIO becomes "EIO".
> >>>>
> >>>> This is what get_errno_name() does.
> >>>>
> >>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >>>> ---
> >>>>    qemu-error.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>    qemu-error.h |    1 +
> >>>>    2 files changed, 86 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/qemu-error.c b/qemu-error.c
> >>>> index 5a35e7c..7035417 100644
> >>>> --- a/qemu-error.c
> >>>> +++ b/qemu-error.c
> >>>> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...)
> >>>>        va_end(ap);
> >>>>        error_printf("\n");
> >>>>    }
> >>>> +
> >>>> +/*
> >>>> + * Probably only useful for QMP
> >>>> + */
> >>>> +const char *get_errno_name(int err)
> >>>> +{
> >>>> +    switch (abs(err)) {
> >>>> +    case EPERM:
> >>>> +        return "EPERM";
> >>>> +    case ENOENT:
> >>>> +        return "ENOENT";
> >>>>
> >>>>          
> >>> [...]
> >>>
> >>>        
> >>>> +    case EDOM:
> >>>> +        return "EDOM";
> >>>> +    case ERANGE:
> >>>> +        return "ERANGE";
> >>>> +    case ENOMEDIUM:
> >>>> +        return "ENOMEDIUM";
> >>>> +    case ENOTSUP:
> >>>> +        return "ENOTSUP";
> >>>> +    default:
> >>>> +        return "unknown";
> >>>>
> >>>>          
> >>> How did you choose the codes to implement?  POSIX has many more...
> >>>        
> >   I just ran an awk script on the linux's base errno header file, my
> > idea is just to have the common names, anything 'new' will hit the
> > default clause and we can add it later.
> >
> >    
> >> Let me say another way why I think this is a bad path to go down.
> >>
> >> In generally, we could never just pass errno through down.  Different
> >> host platforms are going to generate different errno values so we really
> >> need to filter and send reliable errno values so that clients don't have
> >> to have special code for when they're on Linux vs. AIX vs. Solaris.
> >>      
> >   Sorry for the potential stupid question, but what would a 'reliable'
> > errno be? Or, what's an unreliable errno?
> >
> >   We're not sending plain integers to the clients, so the only problem
> > I can see is if different unices return different errnos for the
> > same error. Is that the problem you're seeing?
> >    
> 
> Different types of platforms return different errno values for the same 
> error type.
> 
> As an example, on Linux, connect() returns EINPROGRESS when you set the 
> socket non-blocking.  On Windows, it returns EWOULDBLOCK.

 Wonderful.

> If you just pass through errno, then all QMP clients are going to have 
> to know the different between QEMU on Windows and Linux and handle the 
> errnos appropriately.

 Right.

> >> If we're white listing errno values, we should be able to trivially
> >> convert errnos to QError types via a table just like you have above.
> >>      
> >   Having a direct errno ->  QError mapping doesn't seem good for the
> > current use cases.
> >
> >   For example, do_savevm() (not merged yet) has a StateVmSaveFailed
> > error which also has a 'reason' member, which would look like this
> > on the wire:
> >
> > "{ 'class': 'StateVmSaveFailed', 'data': { 'reason': "EIO" } }"
> >
> >   So, the QError class says what has failed and the errno part says
> > why it has failed.
> >
> >   I'd be happy to implement a different solution that satisfies this
> > basic requirement.
> >    
> 
> We need to map errnos to some QMP defined error type.  However, as I've 
> said before, "reason" is usually an indicator that an error is bad.
> 
> A better error would be:
> 
> { 'class': 'SocketIOError': 'data' : { 'source': 'migration' }}
> 
> Or something like that.  You don't want to have command and then 
> CommandFailed as the error.  You want the errors to be the 'reason's for 
> the commands failure.

 StateVmSaveFailed is not like CommandFailed, there are five errors
in do_savevm() and StateVmSaveFailed happens to be one of them.

 But I understand what you mean and I have considered doing something
like it, one of the problems though is that I'm not sure 'source' is
enough to determine where the error has happened.

 Consider do_savevm() again. We have three 'operations' that might
fail: delete an existing snapshot, save the VM state and create the
snapshot. All those operations can return -EIO as an error.

 So, the first question is: would you map EIO to an QError? Just like
you did for SocketIOError? If so, how would you know which operation
has failed? Would you put its name in source? Or have an additional
'operation' key?

 A related problem is not to degrade the current set of error messages
we offer to the users. For do_savevm()'s 'save state' operation, current
message is:

   "Error -7 while writing VM"

 With StateVmSaveFailed it becomes:

   "Failed to save VM state ("EIO")"

 Given the current implementation of QError, I'm not sure how we can have
such a good error message if our QErrors are not 'operation based'.

Re: [Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Anthony Liguori]

On 05/04/2010 03:30 PM, Luiz Capitulino wrote:
>
>   StateVmSaveFailed is not like CommandFailed, there are five errors
> in do_savevm() and StateVmSaveFailed happens to be one of them.
>
>   But I understand what you mean and I have considered doing something
> like it, one of the problems though is that I'm not sure 'source' is
> enough to determine where the error has happened.
>
>   Consider do_savevm() again. We have three 'operations' that might
> fail: delete an existing snapshot, save the VM state and create the
> snapshot. All those operations can return -EIO as an error.
>    

Maybe those three operations should return distinct errnos?

That way, we can make more useful QErrors.

>   So, the first question is: would you map EIO to an QError? Just like
> you did for SocketIOError? If so, how would you know which operation
> has failed? Would you put its name in source? Or have an additional
> 'operation' key?
>
>   A related problem is not to degrade the current set of error messages
> we offer to the users. For do_savevm()'s 'save state' operation, current
> message is:
>
>     "Error -7 while writing VM"
>
>   With StateVmSaveFailed it becomes:
>
>     "Failed to save VM state ("EIO")"
>    

I don't think this is that useful to preserve because as you said, it 
could be one of three things.

Regards,

Anthony Liguori

>   Given the current implementation of QError, I'm not sure how we can have
> such a good error message if our QErrors are not 'operation based'.
>    

Re: [Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Luiz Capitulino]

On Tue, 04 May 2010 16:56:19 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/04/2010 03:30 PM, Luiz Capitulino wrote:
> >
> >   StateVmSaveFailed is not like CommandFailed, there are five errors
> > in do_savevm() and StateVmSaveFailed happens to be one of them.
> >
> >   But I understand what you mean and I have considered doing something
> > like it, one of the problems though is that I'm not sure 'source' is
> > enough to determine where the error has happened.
> >
> >   Consider do_savevm() again. We have three 'operations' that might
> > fail: delete an existing snapshot, save the VM state and create the
> > snapshot. All those operations can return -EIO as an error.
> >    
> 
> Maybe those three operations should return distinct errnos?

 I don't think this is possible, as we would have to guarantee that no
function called by a handler return the same errno.

 Taking the block layer as an example. Most block drivers handlers check
if the driver really exist (-ENOMEDIUM) and if the driver supports the
operation being requested (-ENOTSUP).

 How can we have unique errnos in this case?

 Also remember that we're only talking about the surface. The call
chain is deep. We have almost a hundred handlers, they use functions
from almost all subsystems.

> That way, we can make more useful QErrors.

 Perhaps, the question boils down to how QErrors should be done.

 Today, we're doing it like this, consider handler foo(), it does the following:

      1. connect somewhere
      2. send some data
      3. close

 All operations performed can fail and we want to report that. Currently,
afaiu we're doing the following (Markus correct me if I'm wrong):

      1. ConnectFailed
      2. SendFailed
      3. CloseFailed

 An obvious problem is that we don't say why it has failed. But this is
what errno is for and I thought we could use it someway. The advantage
of this approach is that, errors are high-level. It's easy to identify
what went wrong and we can have very good error messages for them.

 Now, if I got it right, you're suggesting we should do:

      1. BadFileDescriptor, Interrupted, NoPermission ...
         (or anything connect() may return)
      2. IOError ...
      3. IOError, BadFileDescriptor

 This makes sense, but if I'm a user (or a QMP client) I don't want this:

(qemu) savevm foobar
Bad file descriptor

 I'd prefer this instead:

(qemu) savevm foobar
Can't delete 'foobar': Bad file descriptor

 Or:

(qemu) savevm foobar
Can't save VM state: I/O Error

> >   So, the first question is: would you map EIO to an QError? Just like
> > you did for SocketIOError? If so, how would you know which operation
> > has failed? Would you put its name in source? Or have an additional
> > 'operation' key?
> >
> >   A related problem is not to degrade the current set of error messages
> > we offer to the users. For do_savevm()'s 'save state' operation, current
> > message is:
> >
> >     "Error -7 while writing VM"
> >
> >   With StateVmSaveFailed it becomes:
> >
> >     "Failed to save VM state ("EIO")"
> >    
> 
> I don't think this is that useful to preserve because as you said, it 
> could be one of three things.

 But as I said above, we have to say which operation has failed, otherwise
it's very difficult to diagnose the problem.

Re: [Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 05/03/2010 08:06 AM, Markus Armbruster wrote:
>> Luiz Capitulino<lcapitulino@redhat.com>  writes:
>>
>>    
>>> We need to expose errno in QMP, for three reasons:
>>>
>>>    1. Some error handling functions print errno codes to the user,
>>>       while it's debatable whether this is good or not from a user
>>>       perspective, sometimes it's the best we can do because it's
>>>       what system calls and libraries return
>>>
>>>    2. Some events (eg. BLOCK_IO_ERROR) will be made even more
>>>       complete with errno information
>>>
>>>    3. It's very good for debugging
>>>
>>> So, we need a way to expose those codes in QMP. We can't just use
>>> the codes themselfs because they may vary between systems.
>>>
>>> The best solution I can think of is to return the string
>>> representation of the name. For example, EIO becomes "EIO".
>>>
>>> This is what get_errno_name() does.
>>>
>>> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>>> ---
>>>   qemu-error.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   qemu-error.h |    1 +
>>>   2 files changed, 86 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/qemu-error.c b/qemu-error.c
>>> index 5a35e7c..7035417 100644
>>> --- a/qemu-error.c
>>> +++ b/qemu-error.c
>>> @@ -207,3 +207,88 @@ void error_report(const char *fmt, ...)
>>>       va_end(ap);
>>>       error_printf("\n");
>>>   }
>>> +
>>> +/*
>>> + * Probably only useful for QMP
>>> + */
>>> +const char *get_errno_name(int err)
>>> +{
>>> +    switch (abs(err)) {
>>> +    case EPERM:
>>> +        return "EPERM";
>>> +    case ENOENT:
>>> +        return "ENOENT";
>>>      
>> [...]
>>    
>>> +    case EDOM:
>>> +        return "EDOM";
>>> +    case ERANGE:
>>> +        return "ERANGE";
>>> +    case ENOMEDIUM:
>>> +        return "ENOMEDIUM";
>>> +    case ENOTSUP:
>>> +        return "ENOTSUP";
>>> +    default:
>>> +        return "unknown";
>>>      
>> How did you choose the codes to implement?  POSIX has many more...
>>    
>
> Let me say another way why I think this is a bad path to go down.
>
> In generally, we could never just pass errno through down.  Different
> host platforms are going to generate different errno values so we
> really need to filter and send reliable errno values so that clients
> don't have to have special code for when they're on Linux vs. AIX
> vs. Solaris.

Most instances are covered by POSIX.  The best way to deal with systems
that violate POSIX is to provide conforming replacements for the
offenders.  See gnulib (I just wish it wasn't joined at the hip to
automake).

> If we're white listing errno values, we should be able to trivially
> convert errnos to QError types via a table just like you have above.

Yes, but you'll get many tables.

Luiz encodes the information as event name + errno for events, and as
error class + errno for errors.

The same errno can mean different things for different events / errors.
Thus, if you flatten the two into a single error, you need in the order
of one table per event / error.

Re: [Qemu-devel] [PATCH 1/2] qemu-error: Introduce get_errno_name()

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 04 May 2010 16:56:19 -0500
> Anthony Liguori <anthony@codemonkey.ws> wrote:
>
>> On 05/04/2010 03:30 PM, Luiz Capitulino wrote:
>> >
>> >   StateVmSaveFailed is not like CommandFailed, there are five errors
>> > in do_savevm() and StateVmSaveFailed happens to be one of them.
>> >
>> >   But I understand what you mean and I have considered doing something
>> > like it, one of the problems though is that I'm not sure 'source' is
>> > enough to determine where the error has happened.
>> >
>> >   Consider do_savevm() again. We have three 'operations' that might
>> > fail: delete an existing snapshot, save the VM state and create the
>> > snapshot. All those operations can return -EIO as an error.
>> >    
>> 
>> Maybe those three operations should return distinct errnos?
>
>  I don't think this is possible, as we would have to guarantee that no
> function called by a handler return the same errno.
>
>  Taking the block layer as an example. Most block drivers handlers check
> if the driver really exist (-ENOMEDIUM) and if the driver supports the
> operation being requested (-ENOTSUP).
>
>  How can we have unique errnos in this case?
>
>  Also remember that we're only talking about the surface. The call
> chain is deep. We have almost a hundred handlers, they use functions
> from almost all subsystems.
>
>> That way, we can make more useful QErrors.
>
>  Perhaps, the question boils down to how QErrors should be done.
>
>  Today, we're doing it like this, consider handler foo(), it does the following:
>
>       1. connect somewhere
>       2. send some data
>       3. close
>
>  All operations performed can fail and we want to report that. Currently,
> afaiu we're doing the following (Markus correct me if I'm wrong):
>
>       1. ConnectFailed
>       2. SendFailed
>       3. CloseFailed
>
>  An obvious problem is that we don't say why it has failed. But this is
> what errno is for and I thought we could use it someway. The advantage
> of this approach is that, errors are high-level. It's easy to identify
> what went wrong and we can have very good error messages for them.
>
>  Now, if I got it right, you're suggesting we should do:
>
>       1. BadFileDescriptor, Interrupted, NoPermission ...
>          (or anything connect() may return)
>       2. IOError ...
>       3. IOError, BadFileDescriptor
>
>  This makes sense, but if I'm a user (or a QMP client) I don't want this:
>
> (qemu) savevm foobar
> Bad file descriptor
>
>  I'd prefer this instead:
>
> (qemu) savevm foobar
> Can't delete 'foobar': Bad file descriptor
>
>  Or:
>
> (qemu) savevm foobar
> Can't save VM state: I/O Error

These are of the form "what failed: how it failed".

Aside: not mentioned, because it's obvious for monitor commands: where
it failed.  error_report() adds that information, but only when it's not
obvious, see error_print_loc().

We keep these parts separate because squashing them together into a
single QError class multiplies the classes for no good reason, and makes
it needlessly hard to group errors.  E.g. sometimes we care only about
"what failed".  Then a squashed class is most unwelcome, because to
recognize the "what failed" part you have to know all the squashed
classes, hence all possible how-it-faileds, including the ones that will
be invented after your program ships.

[...]