[pve-devel] [PATCH storage v3 03/10] plugin: dir: handle ova files for import

[Dominik Csapak]

On 5/23/24 14:25, Fabian Grünbichler wrote:
> On May 23, 2024 12:40 pm, Dominik Csapak wrote:
>> On 5/22/24 12:08, Fabian Grünbichler wrote:
>>> On April 29, 2024 1:21 pm, Dominik Csapak wrote:
[snip]
>>>> +	$target_volname = "$vmid/" . $target_diskname;
>>>
>>> this encodes a fact about volname semantics that might not be a given
>>> for external, dir-based plugins (not sure if we want to worry about that
>>> though, or how to avoid it ;)).
>>
>> i mean we could call 'alloc' with a very small size instead
>> and simply "overwrite" it? then we'd also get around things like
>> mkpath and imagedir etc.
> 
> that might actually be nice(r) than the current approach since it avoids
> the volname format issue entirely. the only downside is that we then
> briefly have a "wrong" disk visible, but since the VM has to be locked
> at that point there shouldn't be too much harm in that?
> 

we also have a 'wrong' disk after extraction before the import step though
so yes I do think that approach should work fine

>>>> +	$target_path = $target_plugin->filesystem_path($target_scfg, $target_volname);
>>>
>>> this should be equivalent to PVE::Storage::path for DirPlugin based
>>> storages?
>>>
>>>> +
>>>> +	print "renaming $source_path to $target_path\n";
>>>> +	my $imagedir = $target_plugin->get_subdir($target_scfg, 'images');
>>>
>>> we already did this above, but see comment there ;)
>>
>> true ;)
>>
>>>
>>>> +	mkpath "$imagedir/$vmid";
>>>> +
>>>> +	rename($source_path, $target_path) or die "unable to move - $!\n";
>>>> +    };
>>>> +    if (my $err = $@) {
>>>> +	unlink $source_path;
>>>> +	unlink $target_path if defined($target_path);
>>>
>>> isn't this pretty much impossible to happen? the last thing we do in the
>>> eval block is the rename - if that failed, $target_path can't exist yet.
>>> if it didn't fail, we can't end up here?
>>
>> that probably depends on the underlying filesystem no? not
>> every fs has posix rename semantics i guess?
> 
> I think we can assume an intra-FS rename to either work and have an
> effect, or not work and not have an effect on anything we want to
> support as dir storage? :)
> 
>> in that case we'd cleanup the file, and if it does not exists, it doesn't hurt
>> but
> 
> sure, but error handling tends to get more complicated over time, so not
> having nops in there reduces the complexity somewhat IMHO.

fine with me :)