[pve-devel] [PATCH qemu-server 12/14] qemu_img convert : add external snapshot support

[DERUMIER, Alexandre]

Hi,
>  
>      my $cmd = [];
>      push @$cmd, '/usr/bin/qemu-img', 'convert', '-p', '-n';
> -    push @$cmd, '-l', "snapshot.name=$snapname"
> - if $snapname && $src_format && $src_format eq "qcow2";
> +    push @$cmd, '-l', "snapshot.name=$snapname" if $snapname &&
> $snapshot_type == 2;

>>this breaks backwards compatibility with external plugins that
>>override volume_has_features, and
>>thus still return '1' while using internal qcow2 snapshots.. I think
>>we need to detect that and
>>fallback here? or we need to differentiate all four cases:

>>1: plugin not changed, fallback to format-based decision
>>2: storage snapshot, no need to pass snapshot name to qemu-img
>>3: qcow2-internal snapshot, need to pass snapshot name to qemu-img
>>4: qcow2-external snapshot, no need to pass snapshot name to qemu-img
>>
>>?
>>
>>at that point, it might make more sense to actually name those
>>values:
>>
>>1: plugin not changed, 'legacy' snapshot support
>>'storage': same semantics as '1', but plugin was updated/doesn't
>>override volume_has_features
>>'file-internal': same as 3 above
>>'file-external': same as 4 above

>> or if we want to keep volume_has_features the same, we could
>>introduce a new helper that
>>returns the snapshot type?


I was working on this today (I have send a test patch for missing rbd
&& btrfs test).

Maybe it could be simplier indeed.

has_features->{snapshot}->{raw} = 1 :  it's always a storage snapshot
(assuming we don't want to implement external qcow2 snapshot with raw
backing)

has_features->{snapshot}->{qcow2|other_format_than_raw)} = 1 :  qemu
internal snapshot
has_features->{snapshot}->{qcow2} = 2 :  qemu external snapshot


and could be call like

if ($fmt eq 'raw') {
     type = 'storage'
} else {
   if (has_features->{snapshot}->{$fmt} == 2) {
       type = 'file-external'
   } else {
       type = 'file-internal'
   }
} 

what do you think ? Should not break current plugins, and it should
work for any plugin



or we can add a new helper like

PVE::Storage::snapshot_type($storecfg, $volid)

and in default plugin something like

sub snapshot_type {
....
die "missing snapshot feature" if !has_features->{snapshot}->{$fmt};
if ($fmt eq 'raw') {
       type = 'storage'
} elseif ($fmt eq 'qcow2') {
       type = 'internal'
} else {
   die "unsupported format";
}


with add specific 'external' type for dirplug with snapext && lvm with
qcow2