[3/5] ide: Generate BLOCK_IO_ERROR QMP event

Submitted by Luiz Capitulino on Feb. 2, 2010, 9:10 p.m.

Details

Message ID 1265145013-23231-4-git-send-email-lcapitulino@redhat.com
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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;