From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43004) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YT9W1-0006KU-Ny for qemu-devel@nongnu.org; Wed, 04 Mar 2015 08:39:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YT9Vw-0004mO-22 for qemu-devel@nongnu.org; Wed, 04 Mar 2015 08:39:09 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52051) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YT9Vv-0004mD-QO for qemu-devel@nongnu.org; Wed, 04 Mar 2015 08:39:03 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t24Dd33q010448 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Mar 2015 08:39:03 -0500 Date: Wed, 4 Mar 2015 14:39:01 +0100 From: Kevin Wolf Message-ID: <20150304133901.GO3465@noname.str.redhat.com> References: <1423501897-30410-1-git-send-email-mreitz@redhat.com> <1423501897-30410-2-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1423501897-30410-2-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 01/37] blockdev: Allow creation of BDS trees without BB List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: John Snow , qemu-devel@nongnu.org, Stefan Hajnoczi , Markus Armbruster Am 09.02.2015 um 18:11 hat Max Reitz geschrieben: > If the "id" field is missing from the options given to blockdev-add, > just omit the BlockBackend and create the BlockDriverState tree alone. > > However, if "id" is missing, "node-name" must be specified; otherwise, > the BDS tree would no longer be accessible. > > Signed-off-by: Max Reitz > --- > blockdev.c | 44 +++++++++++++++++++++++++++++++------------- > qapi/block-core.json | 13 +++++++++---- > tests/qemu-iotests/087 | 2 +- > tests/qemu-iotests/087.out | 4 ++-- > 4 files changed, 43 insertions(+), 20 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 6eedcf5..6d67c80 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2822,17 +2822,12 @@ out: > void qmp_blockdev_add(BlockdevOptions *options, Error **errp) > { > QmpOutputVisitor *ov = qmp_output_visitor_new(); > - BlockBackend *blk; > + BlockDriverState *bs; > + BlockBackend *blk = NULL; > QObject *obj; > QDict *qdict; > Error *local_err = NULL; > > - /* Require an ID in the top level */ > - if (!options->has_id) { > - error_setg(errp, "Block device needs an ID"); > - goto fail; > - } > - > /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with > * cache.direct=false instead of silently switching to aio=threads, except > * when called from drive_new(). > @@ -2860,14 +2855,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) > > qdict_flatten(qdict); > > - blk = blockdev_init(NULL, qdict, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - goto fail; > + if (options->has_id) { > + blk = blockdev_init(NULL, qdict, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + goto fail; > + } > + > + bs = blk_bs(blk); > + } else { > + int ret; > + > + if (!qdict_get_try_str(qdict, "node-name")) { > + error_setg(errp, "'id' and/or 'node-name' need to be specified for " > + "the root node"); > + goto fail; > + } > + > + bs = NULL; > + ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB, > + NULL, errp); Now all the qdict entries that aren't parsed by bdrv_open() but converted into flags by blockdev_init() are broken if you don't give an id. This includes read-only, discard, the cache options - in other words, enough to make this "support" completely useless. I guess I need to dig out my cache mode series and get it ready to be merged. Once this is in, the parsing of the other necessary options should be easy to move into bdrv_open(). Unless you have a good reason why we should merge this patch in its hardly functional state now, I would consider those conversions a dependency of this one. Kevin