From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49202) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YT9uO-0002zl-09 for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:04:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YT9uJ-0001YE-DD for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:04:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52254) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YT9uJ-0001Y3-6l for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:04:15 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t24E4DIY022425 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Mar 2015 09:04:14 -0500 Message-ID: <54F710DB.8070202@redhat.com> Date: Wed, 04 Mar 2015 09:04:11 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423501897-30410-1-git-send-email-mreitz@redhat.com> <1423501897-30410-2-git-send-email-mreitz@redhat.com> <20150304133901.GO3465@noname.str.redhat.com> In-Reply-To: <20150304133901.GO3465@noname.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Kevin Wolf Cc: John Snow , qemu-devel@nongnu.org, Stefan Hajnoczi , Markus Armbruster On 2015-03-04 at 08:39, Kevin Wolf wrote: > 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. See patch 23. Max > > 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