From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55383) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTACx-0001Gm-VE for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:23:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YTACs-0001SG-W7 for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:23:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34961) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTACs-0001SA-O6 for qemu-devel@nongnu.org; Wed, 04 Mar 2015 09:23:26 -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 t24ENPBO001909 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 4 Mar 2015 09:23:25 -0500 Message-ID: <54F7155A.9040202@redhat.com> Date: Wed, 04 Mar 2015 09:23:22 -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> <54F710DB.8070202@redhat.com> <20150304141519.GS3465@noname.str.redhat.com> In-Reply-To: <20150304141519.GS3465@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 09:15, Kevin Wolf wrote: > Am 04.03.2015 um 15:04 hat Max Reitz geschrieben: >> 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. > No matter what I'll find there (okay, I've cheated and already quickly > looked at it before writing this), this answer tells me that you're > doing things in the wrong order. To cite the cover letter of v1 regarding the patch order, here for patches 1 and 2: > Patches 35 [22] and 36 [23] are kind of a follow-up to these; but > patch 35 [22] depends on patch 34 [21] which is the reason why there > is a large gap between patch 2 and 35 [22]. I remember needing patch 2 before the rest. Maybe I don't which means I can move it back. I'll see. My reasoning is the following: Yes, blockdev-add for BB-less BDS trees is nearly unusable after this. But it's enough for patch 2; and it does not break anything, because trying that simply always threw an error before this patch. Afterwards, only most usages will result in an error, but the ones introduced in patch 2 are fine. And patch 23 then "unlocks" all useful usages (that I can see). So nothing breaks at any point, we're only slowly allowing more use cases (and everything contained within a single series). Max