[pve-devel] [PATCH storage v2 02/10] plugin: dir: implement import content type

[Dominik Csapak]

On 4/22/24 13:00, Fiona Ebner wrote:
> Am 19.04.24 um 11:45 schrieb Dominik Csapak:
>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>> index 22a9729..39a8354 100644
>> --- a/src/PVE/Storage/Plugin.pm
>> +++ b/src/PVE/Storage/Plugin.pm
>> @@ -654,6 +654,10 @@ sub parse_volname {
>>   	return ('backup', $fn);
>>       } elsif ($volname =~ m!^snippets/([^/]+)$!) {
>>   	return ('snippets', $1);
>> +    } elsif ($volname =~ m!^import/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::IMPORT_EXT_RE_1)$!) {
>> +	return ('import', $1);
> 
> Wouldn't it be nicer to return 'ovf' and 'ova' as the $file_format here
> and check for that at the call sites? Currently you rely on the presence
> or absence of $file_format in copy_needs_extraction() and
> get_import_metadata() and then re-match on the ova extension. Having the
> format right away would be a bit cleaner and more future-proof or is
> there a specific reason against doing it?

i explained it in either the cover letter or in some commit message, probably would have fit
in here too:

we currently only support raw/vmdk/qcow2/subvol here and it is intended only for image formats
IIUC. Also we would have to adapt the `verify_format` for that, since it will be
tested by that at least once. (not sure where exactly though, found out by testing)
and that would mean we could set it as 'default' format on the storage, which is not what we want...

> 
>> +    } elsif ($volname =~ m!^import/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+\.(raw|vmdk|qcow2))$!) {
>> +	return ('import', $1, undef, undef, undef, undef, $2);
>>       }
>>   
>>       die "unable to parse directory volume name '$volname'\n";