Re: [Qemu-devel] [RFC PATCH 1/2] block: allow blockdev-backup from any source

[John Snow]

On 06/26/2018 09:38 PM, Eric Blake wrote:
> On 06/26/2018 05:22 PM, John Snow wrote:
>> In the case of image fleecing, the node we choose as the source
>> for a blockdev-backup is going to be both a root node AND the
>> backing node for the exported image. It does not qualify as a root
>> image in this case.
>>
>> Loosen the restriction.
> 
> Did we regress (and if so, when), or has this never worked?  But you are
> right: visually, we are starting with:

It's actually a regression, because I had a variant of this test in my
local files that used to work. We never really advertised or tested this
feature, though, so ...

> 
>                   Device
> Base (backing) <- Top (active)
> 
> then want to add nodes for an NBD export:
>                   Device    NBD
> Base (backing) <- Top    <- Tmp

          [device] --> [active]
                          ^
[nbd] --> [fleecing]------'


So active here has two parents: the device root, and the temporary
fleecing node. This disqualifies it from being the source in
blockdev_backup, but there's no real reason to.

If the caller of blockdev_backup chooses a dumb source node they'll get
dumb data. GIGO.

> 
> with a blockdev-backup "sync":"none" from Top to Tmp (any writes from
> the Device first copy the old data to Tmp; the NBD export sees a
> read-only view of Tmp that is unchanging from the time the backup job
> started, regardless of what the Device does in the meantime).
> 
> Then when the fleece job ends, the NBD export is stopped, the
> blockdev-backup job is canceled, and Tmp is thrown away as unneeded.
> 

Yup, highlighted a little better in the next patch.

>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   blockdev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 58d7570932..526f8b60be 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3517,7 +3517,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup
>> *backup, JobTxn *txn,
>>           backup->compress = false;
>>       }
>>   -    bs = qmp_get_root_bs(backup->device, errp);
>> +    bs = bdrv_lookup_bs(backup->device, backup->device, errp);
> 
> Reviewed-by: Eric Blake <address@hidden>
> 
>>       if (!bs) {
>>           return NULL;
>>       }
>>
>