From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44264) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1awA5E-0005Wp-8C for qemu-devel@nongnu.org; Fri, 29 Apr 2016 11:12:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1awA52-0007Uf-E9 for qemu-devel@nongnu.org; Fri, 29 Apr 2016 11:11:50 -0400 Date: Fri, 29 Apr 2016 17:11:07 +0200 From: Kevin Wolf Message-ID: <20160429151107.GL4350@noname.redhat.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v9 07/11] block: Add QMP support for streaming to an intermediate layer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz , Eric Blake , Stefan Hajnoczi Am 04.04.2016 um 15:43 hat Alberto Garcia geschrieben: > This patch makes the 'device' parameter of the 'block-stream' command > accept a node name as well as a device name. > > In addition to that, operation blockers will be checked in all > intermediate nodes between the top and the base node. > > Since qmp_block_stream() now uses the error from bdrv_lookup_bs() and > no longer returns DeviceNotFound, iotest 030 is updated to expect > GenericError instead. > > Signed-off-by: Alberto Garcia > --- > blockdev.c | 31 +++++++++++++++++++++++-------- > qapi/block-core.json | 10 +++++++--- > tests/qemu-iotests/030 | 2 +- > 3 files changed, 31 insertions(+), 12 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 2e7712e..bfdc0e3 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2989,6 +2989,7 @@ void qmp_block_stream(const char *device, > BlockBackend *blk; > BlockDriverState *bs; > BlockDriverState *base_bs = NULL; > + BlockDriverState *active; > AioContext *aio_context; > Error *local_err = NULL; > const char *base_name = NULL; > @@ -2997,21 +2998,19 @@ void qmp_block_stream(const char *device, > on_error = BLOCKDEV_ON_ERROR_REPORT; > } > > - blk = blk_by_name(device); > - if (!blk) { > - error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > - "Device '%s' not found", device); > + bs = bdrv_lookup_bs(device, device, errp); > + if (!bs) { > return; > } > > - aio_context = blk_get_aio_context(blk); > + aio_context = bdrv_get_aio_context(bs); > aio_context_acquire(aio_context); > > - if (!blk_is_available(blk)) { > + blk = blk_by_name(device); > + if (blk && !blk_is_available(blk)) { > error_setg(errp, "Device '%s' has no medium", device); > goto out; > } > - bs = blk_bs(blk); > > if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) { > goto out; > @@ -3027,6 +3026,22 @@ void qmp_block_stream(const char *device, > base_name = base; > } > > + /* Look for the top-level node that contains 'bs' in its chain */ > + active = NULL; > + do { > + active = bdrv_next(active); > + } while (active && !bdrv_chain_contains(active, bs)); > + > + if (active == NULL) { > + error_setg(errp, "Cannot find top level node for '%s'", device); > + goto out; > + } Hm... On the one hand, I really like that you don't expect the user to provide the active layer in QMP. This allows us to remove this wart once we have the new op blockers. On the other hand, this code assumes that there is only a single top-level node. This isn't necessarily true any more these days. Do we need to set blockers on _all_ root nodes that have the node in their backing chain? Kevin