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

[Dominik Csapak]

On 4/22/24 13:34, Fiona Ebner wrote:
> 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.

i have to apologize, it seems my previous message was wrong.
after re-checking and re-testing a variant i had before changing it
to not returning any format for ova/ovfs it seems to work fine.

not sure where the error i got came from (maybe i added some extra check and
forgot about/missed it?)

anyway, yes, i can send a v3 where i return ova/ovf as format for those
files, and even use 'ova+vmdk' etc. for the files contained in ovas
which makes checking for extraction a bit better

does sound ok for you?