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

[Fiona Ebner]

Am 22.04.24 um 13:09 schrieb 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.

Hmm, maybe we can lift that limitation? Needs a bit of consideration of
course, but I don't see why the format returned by parse_volname()
should be limited to images, since there already is a vtype to
distinguish. And your series already uses the format for 'import' vtype
too ;)

> 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...
> 

We shouldn't allow that of course, but I don't see verify_format()
called other than for the "pve-storage-format" schema format, so maybe
some other check that fails? Or some use site of "pve-storage-format"
that should first check that it's an image? And I guess we should rename
it to "pve-storage-image-format", because it's used as that to avoid
future confusion.