[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 06/11] block: make 'top' argument to block-co
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 06/11] block: make 'top' argument to block-commit optional |
Date: |
Tue, 27 May 2014 11:20:48 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
On 05/27/2014 08:28 AM, Jeff Cody wrote:
> Now that active layer block-commit is supported, the 'top' argument
> no longer needs to be mandatory.
>
> Change it to optional, with the default being the active layer in the
> device chain.
>
> Reviewed-by: Eric Blake <address@hidden>
> Reviewed-by: Benoit Canet <address@hidden>
> Signed-off-by: Jeff Cody <address@hidden>
> ---
> blockdev.c | 3 ++-
> qapi-schema.json | 7 ++++---
> qmp-commands.hx | 5 +++--
> tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
> 4 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 9a9bdec..b37ace7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1912,7 +1912,8 @@ void qmp_block_stream(const char *device, bool has_base,
> }
>
> void qmp_block_commit(const char *device,
> - bool has_base, const char *base, const char *top,
> + bool has_base, const char *base,
> + bool has_top, const char *top,
> bool has_speed, int64_t speed,
> Error **errp)
> {
Hmm. Later in this function we have:
if (top) {
if (strcmp(bs->filename, top) != 0) {
top_bs = bdrv_find_backing_image(bs, top);
}
}
which is not ideal; it means we are DEPENDING on qapi to NULL-initialize
a pointer when has_top is false.
Although we have (finally!) made that guarantee (see commit fc13d937), I
worry that backporting this patch but not that one may cause a use of
undefined memory, unless you add an explicit:
if (!has_top) {
top = NULL;
}
In other words, I'm a bit reluctant to use default initialization values
without documenting and testing that we can rely on them.
On the other hand, prior to this commit, the 'if (top)' condition was
dead code (since QMP doesn't allow "arguments":{"top":null}, there was
previously no way for a caller to pass a NULL 'top' parameter - so the
fact that the code was already checking for a NULL top implies that we
had planned ages ago to make top a conditional parameter). So on that
grounds, we are merely doing what we'd been planning all along. I can
live with keeping my R-b without a respin, even though it feels a bit
dirty to not be checking has_top.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 01/11] block: Auto-generate node_names for each BDS entry, (continued)
- [Qemu-devel] [PATCH v2 01/11] block: Auto-generate node_names for each BDS entry, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 02/11] block: add helper function to determine if a BDS is in a chain, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 03/11] block: Add overlay BDS pointer into the BlockDriverState struct, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 04/11] block: add helper function to find the active layer of any BDS, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 05/11] block: simplify bdrv_find_base(), Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 06/11] block: make 'top' argument to block-commit optional, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 08/11] block: extend block-commit to accept a string for the backing file, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 07/11] block: Accept node-name arguments for block-commit, Jeff Cody, 2014/05/27
- [Qemu-devel] [PATCH v2 09/11] block: add ability for block-stream to use node-name, Jeff Cody, 2014/05/27