Re: [Qemu-devel] [PATCH 14/47] stream: add on-error argument

[Kevin Wolf]

Am 24.07.2012 13:03, schrieb Paolo Bonzini:
> This patch adds support for error management to streaming.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  block/stream.c   |   28 +++++++++++++++++++++++++++-
>  block_int.h      |    3 ++-
>  blockdev.c       |   11 ++++++++---
>  hmp.c            |    3 ++-
>  qapi-schema.json |   10 ++++++++--
>  qmp-commands.hx  |    2 +-
>  6 files changed, 48 insertions(+), 9 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index b3ede44..03cae14 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -31,6 +31,7 @@ typedef struct StreamBlockJob {
>      BlockJob common;
>      RateLimit limit;
>      BlockDriverState *base;
> +    BlockdevOnError on_error;
>      char backing_file_id[1024];
>  } StreamBlockJob;
>  
> @@ -78,6 +79,7 @@ static void coroutine_fn stream_run(void *opaque)
>      BlockDriverState *bs = s->common.bs;
>      BlockDriverState *base = s->base;
>      int64_t sector_num, end;
> +    int error = 0;
>      int ret = 0;
>      int n = 0;
>      void *buf;
> @@ -136,7 +138,19 @@ wait:
>              ret = stream_populate(bs, sector_num, n, buf);
>          }
>          if (ret < 0) {
> -            break;
> +            BlockErrorAction action =
> +                block_job_error_action(&s->common, s->common.bs, s->on_error,
> +                                       true, -ret);
> +            if (action == BDRV_ACTION_STOP) {
> +                n = 0;
> +                continue;
> +            }
> +            if (error == 0) {
> +                error = ret;
> +            }

I'm not sure we should return an error at the end of the job for
BDRV_ACTION_IGNORE. If it's used for guest block devices, there's no
sign of an error either (except the guest reading garbage, of course).
But it's hard to discuss a feature for which there is no clear use case
anyway... Maybe it's something you could use to rescue what's still
accessible on a corrupted image or so.

In any case we should document it somewhere. Your commit message in
patch 13 probably belongs somewhere in the docs.

>  void stream_start(BlockDriverState *bs, BlockDriverState *base,
>                    const char *base_id, int64_t speed,
> +                  BlockdevOnError on_error,
>                    BlockDriverCompletionFunc *cb,
>                    void *opaque, Error **errp)
>  {
>      StreamBlockJob *s;
>  
> +    if ((on_error == BLOCKDEV_ON_ERROR_STOP ||
> +         on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
> +        !bdrv_iostatus_is_enabled(bs)) {
> +        error_set(errp, QERR_INVALID_PARAMETER, "on-error");
> +        return;
> +    }

Hm, this is an interesting one. bdrv_iostatus_is_enabled() returns true
for a block device that is (or was once) attached to virtio-blk, IDE or
scsi-disk. Which made sense so far because only these devices would
actually set the status.

Now with block jobs, we have other places that can set the status. And
we have images that don't belong to any device, but can still get errors
(mirror target). Maybe it would make sense to just enable the iostatus
here instead of failing?

Kevin