| Message ID | 1265145013-23231-4-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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;
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(-)