Re: [Qemu-devel] [PATCH v4 05/10] block: Accept node-name arguments for block-commit

[Eric Blake]

On 06/04/2014 07:51 AM, Jeff Cody wrote:
> This modifies the block operation block-commit so that it will
> accept node-name arguments for either 'top' or 'base' BDS.
> 
> The filename and node-name are mutually exclusive to each other;
> i.e.:
>     "top" and "top-node-name" are mutually exclusive (enforced)
>     "base" and "base-node-name" are mutually exclusive (enforced)
> 
> The "device" argument now becomes optional as well, because with
> a node-name we can identify the block device chain.  It is only
> optional if a node-name is not specified.

Stale paragraph; just delete it.  [We may later want to add patches to
make device optional, if we can figure out a solution for what to do
when a BDS has more than one overlay; but that doesn't need to stall
this series]

> 
> The preferred and recommended method now of using block-commit
> is to specify the BDS to operate on via their node-name arguments.
> 
> This also adds an explicit check that base_bs is in the chain of
> top_bs, because if they are referenced by their unique node-name
> arguments, the discovery process does not intrinsically verify
> they are in the same chain.
> 
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  blockdev.c       | 55 +++++++++++++++++++++++++++++++++++++++++++++++--------
>  qapi-schema.json | 37 +++++++++++++++++++++++++++----------
>  qmp-commands.hx  | 31 +++++++++++++++++++++++--------
>  3 files changed, 97 insertions(+), 26 deletions(-)
> 

> -    bs = bdrv_find(device);
> -    if (!bs) {
> -        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +    /* argument combination validation */
> +    if (has_base && has_base_node_name) {
> +        error_setg(errp, "'base' and 'base-node-name' are mutually 
> exclusive");
> +        return;
> +    }

An alternative would have been to validate that both 'base' and
'base-node-name' resolve to the same BDS, but I like the simplicity of
erroring out here.

> +
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> +        return;
> +    }

[unrelated to this commit - are we sure that the semantics of the commit
blocker are correct? If we implement BDS reuse among multiple chains,
consider what happens if I have:
       / sn1 <- sn2
base <
       \ file3

Even if there is no other block job operating on sn2 or its chain, we
really want to block an attempt to merge file3 into base, as modifying
base would corrupt both sn1 and sn2.  That is, a BDS being a common
backing between more than one chain should automatically behave as an
op-blocker for commits - where the block is what you are committing
into, not where you are committing from.]

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature