Patchwork [2/2] QMP: Add 'reason' member to the BLOCK_IO_ERROR event

login
register
mail settings
Submitter Luiz Capitulino
Date April 28, 2010, 8:32 p.m.
Message ID <1272486729-14771-3-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/51252/
State New
Headers show

Comments

Luiz Capitulino - April 28, 2010, 8:32 p.m.
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(-)
Anthony Liguori - April 28, 2010, 11:24 p.m.
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;
>
Luiz Capitulino - April 29, 2010, 5:30 p.m.
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.
Anthony Liguori - May 3, 2010, 1:14 p.m.
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

Patch

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;