[pve-devel] [PATCH storage v3 4/4] vdisk_alloc: add assertion for volume's content type

[Daniel Kral <d.kral@proxmox.com>]

Require a caller of vdisk_alloc() to specify the volume's intended
content type and assert whether the specified content type may be stored
on the specified storage before allocating any volume.

The 'any' volume type is for backwards compatibility of external
callers, whose API would break if content type assertions would be
added.

The two affected pve-storage code paths of this change are the API
handler for the `pvesm alloc` endpoint, which fallbacks to 'images' or
'rootdir' depending which is supported for the storage and errors out if
neither is available, and the extract_disk_from_import_file subroutine,
which is assumed to always create a VM image for now, as its only caller
is for VM images currently.

This requires a versioned break for packages where vdisk_alloc() is
called from, which are currently qemu-server and pve-container.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * make $vtype a required parameter now
  * add better documentation about the $vtype's possible values
  * do only allow $vtype to be 'images', 'rootdir', 'import' and 'any'
  * add the 'any' content type for backwards compatibility until PVE 9
  * add commit message description about 'any' content type and changes
    do existing code paths
  * add fallback logic in `pvesm alloc` for retrieving a content type

 src/PVE/API2/Storage/Content.pm    | 13 ++++++++++++-
 src/PVE/GuestImport.pm             |  3 ++-
 src/PVE/Storage.pm                 | 20 +++++++++++++++-----
 src/test/run_test_zfspoolplugin.pl | 12 ++++++++----
 4 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
index 64ed56d..7cfb9c1 100644
--- a/src/PVE/API2/Storage/Content.pm
+++ b/src/PVE/API2/Storage/Content.pm
@@ -217,9 +217,20 @@ __PACKAGE__->register_method ({
 	}
 
 	my $cfg = PVE::Storage::config();
+	my $scfg = PVE::Storage::storage_config($cfg, $storeid);
+
+	# FIXME Replace fallback logic with scoped volume names
+	my $vtype;
+	$vtype = 'images' if defined($scfg->{content}->{images});
+	$vtype = 'rootdir' if defined($scfg->{content}->{rootdir});
+	if (!$vtype) {
+	    raise_param_exc({
+		    storage => "'$storeid' does neither support content type 'images' nor 'rootdir'"
+	    });
+	}
 
 	my $volid = PVE::Storage::vdisk_alloc(
-	    $cfg, $storeid, $param->{vmid}, $param->{format}, $size, { name => $name });
+	    $cfg, $storeid, $param->{vmid}, $param->{format}, $vtype, $size, { name => $name });
 
 	return $volid;
     }});
diff --git a/src/PVE/GuestImport.pm b/src/PVE/GuestImport.pm
index 76ebc9d..a285043 100644
--- a/src/PVE/GuestImport.pm
+++ b/src/PVE/GuestImport.pm
@@ -69,7 +69,8 @@ sub extract_disk_from_import_file {
 
 	# create temporary 1M image that will get overwritten by the rename
 	# to reserve the filename and take care of locking
-	$target_volid = PVE::Storage::vdisk_alloc($cfg, $target_storeid, $vmid, $inner_fmt, 1024);
+	$target_volid = PVE::Storage::vdisk_alloc(
+	    $cfg, $target_storeid, $vmid, $inner_fmt, 'images', 1024);
 	$target_path = PVE::Storage::path($cfg, $target_volid);
 
 	print "renaming $source_path to $target_path\n";
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index ab4dcdd..f089777 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1056,16 +1056,20 @@ sub unmap_volume {
     return $plugin->unmap_volume($storeid, $scfg, $volname, $snapname);
 }
 
-=head3 vdisk_alloc($cfg, $storeid, $vmid, $fmt, $size [, %opts])
+=head3 vdisk_alloc($cfg, $storeid, $vmid, $fmt, $vtype, $size [, %opts])
 
 Allocates a volume on the storage with the identifier C<$storeid> (defined in
-the storage config C<$cfg>) for the VM with the id C<$vmid> with format C<$fmt>
-and a size of C<$size> kilobytes.
+the storage config C<$cfg>) for the VM with the id C<$vmid> with format C<$fmt>,
+volume type C<$vtype> and a size of C<$size> kilobytes.
 
 The format C<$fmt> can be C<'raw'>, C<'qcow2'>, C<'subvol'> or C<'vmdk'>. If
 C<$fmt> is left undefined, it will fallback on the default format of the storage
 type of C<$storeid>.
 
+The volume type C<$vtype> can be C<'images'>, C<'rootdir'>, C<'import'>, or
+C<'any'>, where the last allows the allocation of any volume type. C<'any'> will
+be removed in PVE 9, so do not rely on this value for new code.
+
 Optionally, the following key-value pairs are available for C<%opts>:
 
 =over
@@ -1082,8 +1086,8 @@ Returns the identifier for the new volume in the format C<"$storeid:$volname">.
 
 =cut
 
-sub vdisk_alloc : prototype($$$$$;%) {
-    my ($cfg, $storeid, $vmid, $fmt, $size, $opts) = @_;
+sub vdisk_alloc : prototype($$$$$$;%) {
+    my ($cfg, $storeid, $vmid, $fmt, $vtype, $size, $opts) = @_;
 
     my $name = $opts->{name};
 
@@ -1103,6 +1107,12 @@ sub vdisk_alloc : prototype($$$$$;%) {
 
     activate_storage($cfg, $storeid);
 
+    die "no volume type specified\n" if !$vtype;
+    die "$vtype is not a valid volume type for vdisk_alloc\n"
+	if $vtype !~ m/(any|images|import|rootdir)/;
+
+    assert_content_type_available($cfg, $storeid, $vtype) if $vtype ne 'any';
+
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 
     # lock shared storage
diff --git a/src/test/run_test_zfspoolplugin.pl b/src/test/run_test_zfspoolplugin.pl
index 7dd0a2b..c85ee2c 100755
--- a/src/test/run_test_zfspoolplugin.pl
+++ b/src/test/run_test_zfspoolplugin.pl
@@ -565,7 +565,8 @@ my $test13 = sub {
     print "\nrun test13 \"vdisk_alloc\"\n";
 
     eval {
-	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "112", "raw", 1024 * 1024);
+	my $tmp_volid = PVE::Storage::vdisk_alloc(
+	    $cfg, $storagename, "112", "raw", "images", 1024 * 1024);
 
 	if ($tmp_volid ne "$storagename:vm-112-disk-0") {
 	    die "volname:$tmp_volid don't match\n";
@@ -589,7 +590,8 @@ my $test13 = sub {
     }
 
     eval {
-	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "112", "raw", 2048 * 1024);
+	my $tmp_volid = PVE::Storage::vdisk_alloc(
+	    $cfg, $storagename, "112", "raw", "images", 2048 * 1024);
 
 	if ($tmp_volid ne "$storagename:vm-112-disk-1") {
 	    die "volname:$tmp_volid don't match\n";
@@ -613,7 +615,8 @@ my $test13 = sub {
     }
 
     eval {
-	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "113", "subvol", 1024 * 1024);
+	my $tmp_volid = PVE::Storage::vdisk_alloc(
+	    $cfg, $storagename, "113", "subvol", "images", 1024 * 1024);
 
 	if ($tmp_volid ne "$storagename:subvol-113-disk-0") {
 	    die "volname:$tmp_volid  don't match\n";
@@ -637,7 +640,8 @@ my $test13 = sub {
     }
 
     eval {
-	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "113", "subvol", 2048 * 1024);
+	my $tmp_volid = PVE::Storage::vdisk_alloc(
+	    $cfg, $storagename, "113", "subvol", "images", 2048 * 1024);
 
 	if ($tmp_volid ne "$storagename:subvol-113-disk-1") {
 	    die "volname:$tmp_volid  don't match\n";
-- 
2.39.5



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