Patchwork [3/5] ide: Generate BLOCK_IO_ERROR QMP event

login
register
mail settings
Submitter Luiz Capitulino
Date Feb. 2, 2010, 9:10 p.m.
Message ID <1265145013-23231-4-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/44308/
State New
Headers show

Comments

Luiz Capitulino - Feb. 2, 2010, 9:10 p.m.
Just call bdrv_mon_event() in the right place.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/ide/core.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
Christoph Hellwig - Feb. 3, 2010, 8:31 a.m.
On Tue, Feb 02, 2010 at 07:10:11PM -0200, Luiz Capitulino wrote:
> Just call bdrv_mon_event() in the right place.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hw/ide/core.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index b6643e8..603e537 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -480,14 +480,17 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>      int is_read = (op & BM_STATUS_RETRY_READ);
>      BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
>  
> -    if (action == BLOCK_ERR_IGNORE)
> +    if (action == BLOCK_ERR_IGNORE) {
> +        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
>          return 0;
> +    }
>  
>      if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
>              || action == BLOCK_ERR_STOP_ANY) {
>          s->bus->bmdma->unit = s->unit;
>          s->bus->bmdma->status |= op;
>          vm_stop(0);
> +        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);

Why isn't the event directly sent from drive_get_on_error?  Having
to opencode this in every driver is a sure way to make sure it's
going to be broken somewhere.
Kevin Wolf - Feb. 3, 2010, 9:10 a.m.
Am 03.02.2010 09:31, schrieb Christoph Hellwig:
> On Tue, Feb 02, 2010 at 07:10:11PM -0200, Luiz Capitulino wrote:
>> Just call bdrv_mon_event() in the right place.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>  hw/ide/core.c |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index b6643e8..603e537 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -480,14 +480,17 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>>      int is_read = (op & BM_STATUS_RETRY_READ);
>>      BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
>>  
>> -    if (action == BLOCK_ERR_IGNORE)
>> +    if (action == BLOCK_ERR_IGNORE) {
>> +        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
>>          return 0;
>> +    }
>>  
>>      if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
>>              || action == BLOCK_ERR_STOP_ANY) {
>>          s->bus->bmdma->unit = s->unit;
>>          s->bus->bmdma->status |= op;
>>          vm_stop(0);
>> +        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> 
> Why isn't the event directly sent from drive_get_on_error?  Having
> to opencode this in every driver is a sure way to make sure it's
> going to be broken somewhere.

Because drive_get_on_error isn't an event handler and shouldn't have any
side effects. It might be called anywhere. And it doesn't know the error
code, so it can't even decide if the VM has stopped or not.

Maybe we could look at writing a generic handle_rw_error function for
all block devices. They look pretty much the same in every driver, even
without the monitor event.

Kevin
Luiz Capitulino - Feb. 3, 2010, 11:41 a.m.
On Wed, 03 Feb 2010 10:10:59 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 03.02.2010 09:31, schrieb Christoph Hellwig:
> > On Tue, Feb 02, 2010 at 07:10:11PM -0200, Luiz Capitulino wrote:
> >> Just call bdrv_mon_event() in the right place.
> >>
> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> ---
> >>  hw/ide/core.c |    6 +++++-
> >>  1 files changed, 5 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >> index b6643e8..603e537 100644
> >> --- a/hw/ide/core.c
> >> +++ b/hw/ide/core.c
> >> @@ -480,14 +480,17 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
> >>      int is_read = (op & BM_STATUS_RETRY_READ);
> >>      BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
> >>  
> >> -    if (action == BLOCK_ERR_IGNORE)
> >> +    if (action == BLOCK_ERR_IGNORE) {
> >> +        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
> >>          return 0;
> >> +    }
> >>  
> >>      if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
> >>              || action == BLOCK_ERR_STOP_ANY) {
> >>          s->bus->bmdma->unit = s->unit;
> >>          s->bus->bmdma->status |= op;
> >>          vm_stop(0);
> >> +        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
> > 
> > Why isn't the event directly sent from drive_get_on_error?  Having
> > to opencode this in every driver is a sure way to make sure it's
> > going to be broken somewhere.
> 
> Because drive_get_on_error isn't an event handler and shouldn't have any
> side effects. It might be called anywhere. And it doesn't know the error
> code, so it can't even decide if the VM has stopped or not.
> 
> Maybe we could look at writing a generic handle_rw_error function for
> all block devices. They look pretty much the same in every driver, even
> without the monitor event.

 Any design in mind? I could try this later (preferably after the
event series is merged).

 What if block devices register the following callbacks somewhere:

- handle_rw_error_ignore()
- handle_rw_error_stop()
- handle_rw_error_report()

 But then they'd have to trigger their call by calling, say,
block_handle_rw_error() from the error site.

Patch

diff --git a/hw/ide/core.c b/hw/ide/core.c
index b6643e8..603e537 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -480,14 +480,17 @@  static int ide_handle_rw_error(IDEState *s, int error, int op)
     int is_read = (op & BM_STATUS_RETRY_READ);
     BlockInterfaceErrorAction action = drive_get_on_error(s->bs, is_read);
 
-    if (action == BLOCK_ERR_IGNORE)
+    if (action == BLOCK_ERR_IGNORE) {
+        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
         return 0;
+    }
 
     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
         s->bus->bmdma->unit = s->unit;
         s->bus->bmdma->status |= op;
         vm_stop(0);
+        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
     } else {
         if (op & BM_STATUS_DMA_RETRY) {
             dma_buf_commit(s, 0);
@@ -495,6 +498,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);
     }
 
     return 1;