Re: [pve-devel] [PATCH qemu-server v6 5/6] api: create: add 'import-extraction-storage' parameter

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 15.11.24 um 16:17 schrieb Dominik Csapak:
> this is to override the target extraction storage for the option disk
> extraction for 'import-from'. This way if the storage does not
> supports the content type 'images', one can give an alternative  one.
> 

looks OK to me besides some styling/naming things I commented inline

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 46 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 1aa42585..58aaabbe 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -174,9 +174,18 @@ my $check_storage_access = sub {
>  		    if $vtype ne 'images' && $vtype ne 'import';
>  
>  		if (PVE::QemuServer::Helpers::needs_extraction($vtype, $fmt)) {
> -		    raise_param_exc({ $ds => "$src_image is not on an storage with 'images' content type."})
> -			if !$scfg->{content}->{images};
> -		    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
> +		    if (defined($extraction_storage)) {
> +			my $extraction_scfg = PVE::Storage::storage_config($storecfg, $extraction_storage);
> +			raise_param_exc({ 'import-extraction-storage' => "$extraction_storage does not support"
> +				." 'images' content type or is not file based."})

wrapping two things at once (string and the object/method call parenthesis) is not
ideal readability wise.

> +			if !$extraction_scfg->{content}->{images} || !$extraction_scfg->{path};

please avoid post-ifs on expressions wrapping already multiple lines.

Above could look something like:

if (!$extraction_scfg->{content}->{images} || !$extraction_scfg->{path}) {
    raise_param_exc({
        'import-extraction-storage' => "$extraction_storage does not support"
            ." 'images' content type or is not file based.",
    });
}

> +			$rpcenv->check($authuser, "/storage/$extraction_storage", ['Datastore.AllocateSpace']);
> +		    } else {
> +			raise_param_exc({ $ds => "$src_image is not on an storage with 'images'"
> +			    ." content type and no 'import-extraction-storage' was given."})
> +			    if !$scfg->{content}->{images};

same here, in general you could unfiy the code paths more, they're basically the same,
just need upfront 

my $extraction_scfg = defined($extraction_storage)
    ? PVE::Storage::storage_config($storecfg, $extraction_storage)
    : $scfg;

and
 
$rpcenv->check($authuser, "/storage/" . ($extraction_storage // $storeid), ['Datastore.AllocateSpace']);

IMO it's more gained by more easily seeing that the same things are checked
compared to explicit branches for making loading the extra storage config a bit
more explicit.

> +			$rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
> +		    }
>  		}
>  	    }
>  

> @@ -973,6 +983,12 @@ __PACKAGE__->register_method({
>  		    default => 0,
>  		    description => "Start VM after it was created successfully.",
>  		},
> +		'import-extraction-storage' => get_standard_option('pve-storage-id', {


Would prefer:
import-working-storage

> +		    description => "Storage for temporarily extracted images 'import-from' image"

"images 'import-from' image files" seem like it misses some words in between.

> +			." files (default: import source storage)",

btw. why not prefer the target storage if applicable, i.e. it's a file-storage, and
only fallback to the import one if not? we can change that later though.

But in any way it might be nice to mention that the storage needs to be a file-based
one I think.

Maybe something like:

"A file-based storage with 'images' content-type enabled, into which images are extracted during import as an intermediate step for further processing. Defaults to ..."


> +		    optional => 1,
> +		    completion => \&PVE::QemuServer::complete_storage,
> +		}),
>  	    },
>  	    1, # with_disk_alloc
>  	),

> @@ -2213,6 +2235,12 @@ __PACKAGE__->register_method({
>  		    maximum => 30,
>  		    optional => 1,
>  		},
> +		'import-extraction-storage' => get_standard_option('pve-storage-id', {
> +		    description => "Storage for temporarily extracted images 'import-from' image"
> +			." files (default: import source storage)",

same here as above

> +		    optional => 1,
> +		    completion => \&PVE::QemuServer::complete_storage,
> +		}),
>  	    },
>  	    1, # with_disk_alloc
>  	),



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel