Re: [Qemu-devel] [PATCH 3/5] ide: Generate BLOCK_IO_ERROR QMP event

[Luiz Capitulino]

On Wed, 03 Feb 2010 10:10:59 +0100
Kevin Wolf <address@hidden> 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 <address@hidden>
> >> ---
> >>  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.