[pve-devel] [PATCH pve-storage 04/10] rename_volume: add source && target snap

[DERUMIER, Alexandre]

>>we could consider adding a new API method `rename_snapshot` instead:
>>
>>my ($class, $scfg, $storeid, $volname, $source_snap, $target_snap) =
>>@_;
>>
>>for the two plugins here it could easily share most of the
>>implementation
>>with rename_volume, without blowing up the interface for a fairly
>>limited
>>use case?
>>
>>rename_volume right now is used for re-assigning a volume from one
>>owner/vmid to another only, AFAICT, with $target_volname never being
>>actually provided by callers. the new call would then never provide
>>$target_vmid and never provide $target_volname, while existing ones
>>never provide the snapshot parameters. OTOH, just like the existing
>>rename_volume, such a rename_snapshot method would only have a
>>single use case/call site, unless we plan to also add generic
>>snapshot renaming as a feature down the line..

ok, no problem, I'll add it

>>another missing piece here is that for external snapshots, we need
>>to actually rename all the volumes when reassigning, else only the
>>current volume is renamed, and the rest still have the old
owner/name..

ah yes, indeed, I'll fix it

> 
>  
>      my (
>          undef, $source_image, $source_vmid, $base_name, $base_vmid,
> undef, $format,
>      ) = $class->parse_volname($source_volname);
> +
> +    $source_image = $class->get_snap_volname($source_volname,
> $source_snap) if $source_snap;

>>if you flip these two around, this could just overwrite
>>$source_volname, because otherwise
>>you rely on $source_image and $source_volname being identical always
>>which might not be
>>the case in the future?
>> or is this more correct in general, in case we ever add base image
>>support to non-thin LVM?


it was mostly because source_volname is used just after to find
target_volname. 
I'll cleanup this in the dedicated rename_snasphot sub