From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49259) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwWBb-0003G4-9p for qemu-devel@nongnu.org; Wed, 01 Aug 2012 06:29:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwWBV-0004YY-EJ for qemu-devel@nongnu.org; Wed, 01 Aug 2012 06:29:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33617) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwWBV-0004YU-5S for qemu-devel@nongnu.org; Wed, 01 Aug 2012 06:29:45 -0400 Message-ID: <50190513.8090604@redhat.com> Date: Wed, 01 Aug 2012 12:29:39 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1343127865-16608-1-git-send-email-pbonzini@redhat.com> <1343127865-16608-15-git-send-email-pbonzini@redhat.com> In-Reply-To: <1343127865-16608-15-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 14/47] stream: add on-error argument List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: jcody@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com Am 24.07.2012 13:03, schrieb Paolo Bonzini: > This patch adds support for error management to streaming. > > Signed-off-by: Paolo Bonzini > --- > 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