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

[Wen Congyang <wency@cn.fujitsu.com>]

On 09/03/2015 02:50 AM, Eric Blake wrote:
> 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.

Sorry, this graph is wrong. virtio-blk dev 'Y' can not use the BDS Y if it
is referenced by X. That is why I need patch 1 and 2.

Thanks
Wen Congyang

> 
>>
>> 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?
>