[pve-devel] [PATCH v2 storage 27/28] Add volname_for_storage and print_volname helper

[Fabian Ebner]

On 2/25/20 12:34 PM, Fabian Grünbichler wrote:
> On February 24, 2020 1:44 pm, Fabian Ebner wrote:
>> Allows to convert a volume ID from the source storage to
>> a valid volume name for the target storage. The original name is
>> preserved, except for a possible extension and it's also checked
>> whether the format is valid for the target storage.
>> Example: mylvm:vm-123-disk-4 <-> mydir:123/vm-123-disk-4.raw
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>   PVE/Storage.pm        | 17 +++++++++++++++--
>>   PVE/Storage/Plugin.pm | 19 +++++++++++++++++++
>>   2 files changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index 8969d2e..7a76b2c 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -39,11 +39,11 @@ use PVE::Storage::DRBDPlugin;
>>   use PVE::Storage::PBSPlugin;
>>   
>>   # Storage API version. Icrement it on changes in storage API interface.
>> -use constant APIVER => 4;
>> +use constant APIVER => 5;
>>   # Age is the number of versions we're backward compatible with.
>>   # This is like having 'current=APIVER' and age='APIAGE' in libtool,
>>   # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
>> -use constant APIAGE => 3;
>> +use constant APIAGE => 4;
>>   
>>   # load standard plugins
>>   PVE::Storage::DirPlugin->register();
>> @@ -392,6 +392,19 @@ sub parse_volname {
>>       return $plugin->parse_volname($volname);
>>   }
>>   
>> +# returns a valid volume name for the specified target storage
>> +# using an existing volume ID on the source storage
>> +sub volname_for_storage {
>> +    my ($cfg, $volid, $target_storeid) = @_;
>> +
>> +    my $target_scfg = storage_config($cfg, $target_storeid);
>> +    my $target_plugin = PVE::Storage::Plugin->lookup($target_scfg->{type});
>> +
>> +    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) = parse_volname($cfg, $volid);
>> +
>> +    return $target_plugin->print_volname($target_scfg, $vtype, $name, $vmid, $basename, $basevmid, $isBase, $format);
>> +}
>> +
>>   sub parse_volume_id {
>>       my ($volid, $noerr) = @_;
>>   
>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>> index 59660d9..6ce73bb 100644
>> --- a/PVE/Storage/Plugin.pm
>> +++ b/PVE/Storage/Plugin.pm
>> @@ -445,6 +445,25 @@ sub parse_volname {
>>       die "unable to parse directory volume name '$volname'\n";
>>   }
> 
> a comment here that this is only used and tested for volname_for_storage
> usage might be a good idea (e.g., it's not working for base images, and
> I am not sure whether all the shared exotic storages like iSCSI do the
> right thing?)
> 

I only focused on the plugins which can alloc_image, but it's true that 
such a general method should handle all cases.

> also, this is technically not really sure to work for external plugins
> derived from PVE::Storage::Plugin.pm - they might override
> parse_volname, and now get a print_volname with a default
> implementation. we could double check this by re-parsing before
> returning, if the assumption that parse_volname(print_volname($cfg, @param))
> should return @param again is true ;)

I'm aware of this and described scenarios in the commit message of #28,
but it's true that re-parsing could be done in volname_for_storage 
directly and not give the burden to check and a possibly invalid volname 
to the callers.

Would a general print_volname be very useful? Otherwise this "guessing a 
valid ID" could also be done in (a helper for) storage_migrate without 
any API changes.

>> +sub print_volname {
>> +    my ($class, $scfg, $vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) = @_;
>> +
>> +    die "print_volname is not implemented for type '$vtype'\n" if $vtype ne 'images' && $vtype ne 'rootdir';
>> +    die "print_volname is not implemented for base images or linked clones\n" if defined($basename) || $isBase;
>> +
>> +    my (undef, $valid_formats) = default_format($scfg);
>> +    my $format_is_valid = grep {$_ eq $format } @$valid_formats;
>> +    die "print_volname: unsupported format '$format' for storage type $scfg->{type}\n" if !$format_is_valid;
>> +
>> +    (my $name_without_extension = $name) =~ s/\.$format$//;
>> +
>> +    if ($scfg->{path}) {
>> +	return "$vmid/$name_without_extension.$format";
>> +    } else {
>> +	return "$name_without_extension";
>> +    }
>> +}
>> +
>>   my $vtype_subdirs = {
>>       images => 'images',
>>       rootdir => 'private',
>> -- 
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>