Re: [Qemu-devel] [PATCH 04/16] block: Allow references for backing files

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2732 bytes --]

On 09/02/2015 02:51 AM, Wen Congyang wrote:
> Usage:
> -drive file=xxx,id=Y, \
> -drive file=xxxx,id=X,backing.backing_reference=Y
> 
> It will create such backing chain:
>                {virtio-blk dev 'Y'}      {virtio-blk dev 'X'}
>                          |                         |
>                          |                         |
>                          v                         v
> 
>     [base] <- [mid] <- ( Y )  <----------------- ( X )

This makes any changes to 'Y' have unspecified effects on 'X'.  While we
may have a valid reason to use a backing BDS in more than one chain, I
seriously doubt anyone will ever want to have two guest-visible -drive's
that are both read-write where one can corrupt the other.  I can totally
see the point of having BDS 'Y' exist for checkpoints or some other
non-guest-visible action, so I'm not saying this patch is wrong, just
that the commit message is picking a poor example of how it would be used.

> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  block.c               | 39 +++++++++++++++++++++++++++++++++++----
>  include/block/block.h |  1 +
>  2 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index d02d9e9..f93c50d 100644
> --- a/block.c
> +++ b/block.c
> @@ -1179,6 +1179,7 @@ out:
>  }
>  
>  #define ALLOW_WRITE_BACKING_FILE    "allow-write-backing-file"
> +#define BACKING_REFERENCE           "backing_reference"

Why the inconsistency in '-' vs. '_'?  I'd stick with dash here.

>  static QemuOptsList backing_file_opts = {
>      .name = "backing_file",
>      .head = QTAILQ_HEAD_INITIALIZER(backing_file_opts.head),
> @@ -1188,6 +1189,11 @@ static QemuOptsList backing_file_opts = {
>              .type = QEMU_OPT_BOOL,
>              .help = "allow write to backing file",
>          },
> +        {
> +            .name = BACKING_REFERENCE,
> +            .type = QEMU_OPT_STRING,
> +            .help = "reference to the exsiting BDS",

s/exsiting/existing/

But why do we need this? In qapi, BlockdevOptionsGenericCOWFormat
already has '*backing':'BlockdevRef', and BlockdevRef already has a
choice between 'definition' (object) and 'reference' (string).  Or is
this just a matter of teaching the command line to do what QMP can
already do?  In which case, wouldn't:

-drive file=xxx,id=Y, -drive file=xxxx,id=X,backing=Y

be the natural mapping of 'backing' being a string rather than a dictionary?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]