qemu-devel
[Top][All Lists]
Advanced

[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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]