From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37920) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTH68-0003IP-9j for qemu-devel@nongnu.org; Wed, 04 Mar 2015 16:44:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YTH65-0004I8-2F for qemu-devel@nongnu.org; Wed, 04 Mar 2015 16:44:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34929) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTH64-0004HY-RJ for qemu-devel@nongnu.org; Wed, 04 Mar 2015 16:44:53 -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 t24LiovX004923 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Mar 2015 16:44:50 -0500 Message-ID: <54F77CD0.4050705@redhat.com> Date: Wed, 04 Mar 2015 16:44:48 -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. > > 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. Because I will not send out another version of this series for the next two weeks, I'll just give an overview of where I'm currently at (for each of the patches you replied to): We need this patch before "block: Make bdrv_is_inserted() recursive", because that patch makes the problems apparent which are fixed in my bdrv_close_all() series (071 fails, because flushing the qcow2 metadata suddenly no longer results in an error; this is because bdrv_is_inserted() now returns false because the file BDS under qcow2 has been closed (because it has a BB), so it's no longer inserted). Therefore, we need patch 2 before said patch to work around the problem (until it's fixed by the respective series, which depends on this one, however). I could probably split up patch 22 and pull the first part up front, along with patch 23, but this would require work, which I don't deem necessary. As I said before, in my opinion this patch does not break any existing valid usage of blockdev-add, it's just that there are some use cases we want to have after this patch that do not work yet. But they will be added in a later patch in this series, so I don't find this an issue. Therefore, for this patch, I'll add a note into the commit message stating that there will be a follow-up patch allowing options that are not parsed by bdrv_open() (like caching) to be given. Max