[pve-devel] [PATCH storage/qemu-server/container v3 00/27] consistent assertions for volume's content types

[pve-devel] [PATCH storage/qemu-server/container v3 00/27] consistent assertions for volume's content types

[Daniel Kral]

== Changelog since v2 ==

Thanks @Fiona for the feedback and help on this!

v1: https://lore.proxmox.com/pve-devel/20240916163839.236908-1-d.kral@proxmox.com/
v2: https://lore.proxmox.com/pve-devel/20250211160825.254167-1-d.kral@proxmox.com/

- improve some commit messages and clarify some non-trivial changes
- rename helpers from `assert_*_supported` to `assert_*_available`
- make helpers also assert whether storages are enabled (local/remote)
- make $fmt a required parameter for vdisk_alloc again
- make $vtype a required parameter for vdisk_alloc
- other smaller changes are inline in patches

== Description ==

There were (at least) four missing assertions whether the underlying
storage supports the content type that is about the be allocated, but
will fail to start with a volume not being on a storage, which supports
VM images (see `config_to_command` for the existing check):

- when moving disks (e.g. `qm disk move <vmid> --storage ...`)
- when cloning vms (e.g. `qm clone <vmid> <new-vmid> --storage ...`)
- when importing OVF manifests (e.g. `qm importovf ... --storage ...`)
- when adding cloudinit images (e.g. `qm set <vmid> -ide0
  "noimages:cloudinit"`), and

The first two were already merged for v2, while the latter two are still
in this revision. Otherwise, this patch series 1) fixes these situations
(qemu-server #1-#3) and introduces 2) assertion helpers and 3) an
optional additional assertion in `vdisk_alloc` in the rest of the patch
series to make the error messages of content type assertions more
consistent and allow for easier grepping where these checks are done.

This patch series also introduces test cases for the `alloc_disk` helper
in pve-container to make the existing function easier to follow and
allow a single call to `vdisk_alloc` instead of four. Where applicable
(e.g. qemu-server #4-#6 for cloudinit images), there are also some
smaller refactorings so that the bug fix ends up not duplicating code.

The only content type assertions at `vdisk_alloc`, which are not
possible are for fleecing backup images and snapshot vm state files,
since both can be currently created without any error, so adding the
assertion would be an API breakage. Therefore, I propose to add this
check for PVE 9.0.

== Standalone patches ==

I also made sure that the bug fixes and introduction of test cases a
factored out at the top, so they can be applied before the refactorings:

- qemu-server #1-#3, and
- pve-container #1-#5.

== Breaking changes ==

As this patch series changes the signature of vdisk_alloc, there needs
to be a versioned break between them, since else the other packages will
fail to call vdisk_alloc with the older parameters.

This seemed like the best option as this already forces any caller to
pass the $vtype now and will only require remaining 'any's to be changed
to the correct content type in PVE 9.

== Building ==

To build this, pve-storage must be built first, as the API of
`vdisk_alloc` is broken. I made sure that there's one/two commit in each
repository to make them compatible with each other, which is pve-storage
#3 and $4, qemu-server #4 and #5, and pve-container #6 and $7.

These two patches each can be squashed in all cases as I only wanted to
separate them for review, but either way is fine for me here. The first
always moves $name to the optional hash and the second always adds the
new required parameter $vtype.

== Testing ==

I tested the same (and some more) scenarios unpatched and patched as for
v2:

- moving disks will only work to supported storages now,
- cloning VMs will only work to supported storages now,
- creating cloudinit images will only work on supported storages now,
  - it is irrelevant if there is a 'media=cdrom' appended or not now,
  - this needs the "Datastore.AllocateSpace" permission now,
- importing OVF manifests will only work on supported storages now,
- creating VMs on supported storages succeed for harddisks, efidisks,
  tpm disks, and cloudinit images,
- creating VMs on unsupported storages (for the above) fails,
- cloning VMs between supported storages locally works as expected,
- cloning VMs between supported storages between nodes works,
- suspending and resuming a VM works fine,
- backups and qmrestores of VMs work fine,
- migrating a VM between two nodes on a supported target storage works,
- migrating a VM to another node on a unsupported target storage fails,
- migrating a VM to another node from a unsupported source storage fails,
- updating the VM config in such a way that a disk needs to be allocated
  works fine as long as the disk to allocate is on a supported storage,
- importing disks works as expected to {un,}supported storages,
- allocating a disk with `pvesm` works as expected,
- backing up a VM with a disk that is on an unsupported storage fails,
- restoring a VM with to an unsupported storage fails.

== Diffstat ==

pve-storage:

Daniel Kral (4):
  introduce helpers for content type assertions of storages and volumes
  tree-wide: make use of content type assertion helper
  vdisk_alloc: factor out optional parameters in options hash argument
  vdisk_alloc: add assertion for volume's content type

 src/PVE/API2/Storage/Content.pm    | 16 +++++-
 src/PVE/API2/Storage/Status.pm     | 10 ++--
 src/PVE/GuestImport.pm             |  3 +-
 src/PVE/Storage.pm                 | 92 +++++++++++++++++++++++++++++-
 src/test/run_test_zfspoolplugin.pl | 12 ++--
 5 files changed, 116 insertions(+), 17 deletions(-)


qemu-server:

Daniel Kral (12):
  fix #5284: cli: importovf: assert content type support for target
    storage
  api: remove unusable default storage parameter in check_storage_access
  fix #5284: api: update-vm: assert content type support for cloudinit
    images
  tree-wide: update vdisk_alloc optional arguments signature
  tree-wide: update vdisk_alloc vtype argument signature
  cfg2cmd: improve error message for invalid volume content type
  api: {clone,move}_vm: use volume content type assertion helpers
  api: {create,update}_vm: use volume content type assertion helpers
  api: import{disk,ovf}: use volume content type assertion helpers
  api: qmrestore: use volume content type assertion helpers
  api: migrate_vm: use volume content type assertion helpers
  tree-wide: add todos for breaking content type assertions

 PVE/API2/Qemu.pm                              | 52 +++++++++----------
 PVE/CLI/qm.pm                                 |  9 ++--
 PVE/QemuConfig.pm                             |  4 +-
 PVE/QemuMigrate.pm                            | 17 +++---
 PVE/QemuServer.pm                             | 52 +++++++------------
 PVE/QemuServer/Cloudinit.pm                   |  3 +-
 PVE/QemuServer/ImportDisk.pm                  |  3 +-
 PVE/VZDump/QemuServer.pm                      | 10 +++-
 qmextract                                     |  4 +-
 test/MigrationTest/QmMock.pm                  |  7 ++-
 .../unsupported-storage-content-type.conf     |  2 +-
 11 files changed, 78 insertions(+), 85 deletions(-)


pve-container:

Daniel Kral (11):
  migration: prepare: factor out common read-only variables
  tests: add tests for expected behavior of alloc_disk wrapper
  alloc disk: fix content type check for ZFS storages
  alloc_disk: factor out common arguments for call to vdisk_alloc
  alloc_disk: simplify storage-specific logic for vdisk_alloc arguments
  alloc_disk: update vdisk_alloc optional arguments signature
  alloc_disk: use volume content type assertion helpers
  api: create: use volume content type assertion helpers
  migration: prepare: use volume content type assertion helpers
  api: update_vm: use volume content type assertion helpers
  mount: raw/iso: use volume content type assertion helpers

 src/PVE/API2/LXC.pm              |  15 +--
 src/PVE/LXC.pm                   |  47 ++++-----
 src/PVE/LXC/Config.pm            |   4 +-
 src/PVE/LXC/Migrate.pm           |  15 ++-
 src/test/Makefile                |   5 +-
 src/test/run_alloc_disk_tests.pl | 157 +++++++++++++++++++++++++++++++
 6 files changed, 198 insertions(+), 45 deletions(-)
 create mode 100755 src/test/run_alloc_disk_tests.pl


Summary over all repositories:
  22 files changed, 392 insertions(+), 147 deletions(-)

-- 
Generated by git-murpp 0.8.0


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

[pve-devel] [PATCH storage v3 1/4] introduce helpers for content type assertions of storages and volumes

[Daniel Kral]

Add subroutines for asserting the content types of storages and volumes
to reduce code duplication, e.g. when implementing preconditions in an
API handler before calling vdisk_alloc().

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * split documentation back to be above each helper
  * changed names of helpers from *_supported to *_available
  * made both helpers also assert whether storage is enabled

 src/PVE/Storage.pm | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index d0a696a..b5a2c8a 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -537,6 +537,53 @@ sub parse_volume_id {
     return PVE::Storage::Plugin::parse_volume_id($volid, $noerr);
 }
 
+=head3 assert_content_type_available($cfg, $storeid, $content_type [, $node])
+
+Asserts whether the storage with the identifier C<$storeid>, which is defined
+in C<$cfg>, is available and supports the content type C<$content_type>.
+
+If C<$node> is set, the assertion is made for the specified C<$node>, else for
+the current node.
+
+If the check fails, the subroutine will C<die> with an error message for either
+the storage being unavailable or the storage not supporting the specified
+content type.
+
+=cut
+
+sub assert_content_type_available : prototype($$$;$) {
+    my ($cfg, $storeid, $content_type, $node) = @_;
+
+    my $scfg = storage_check_enabled($cfg, $storeid, $node);
+
+    die "storage '$storeid' does not support content type '$content_type'\n"
+	if !$scfg->{content}->{$content_type};
+}
+
+=head3 assert_volume_type_available($cfg, $volid [, $node])
+
+Asserts whether the volume with the identifier C<$volid>, which is on a storage
+defined in C<$cfg>, is available and supports the volume's content type
+determined by C<L<< parse_volname()|/parse_volname($cfg, $volid) >>>.
+
+If C<$node> is set, the assertion is made for the specified C<$node>, else for
+the current node.
+
+If the check fails, the subroutine will C<die> with an error message for either
+the volume's storage being unavailable or the storage not supporting the
+volume's content type.
+
+=cut
+
+sub assert_volume_type_available : prototype($$;$) {
+    my ($cfg, $volid, $node) = @_;
+
+    my ($storeid) = parse_volume_id($volid);
+    my ($vtype) = parse_volname($cfg, $volid);
+
+    assert_content_type_available($cfg, $storeid, $vtype, $node);
+}
+
 # test if we have read access to volid
 sub check_volume_access {
     my ($rpcenv, $user, $cfg, $vmid, $volid, $type) = @_;
-- 
2.39.5



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

[pve-devel] [PATCH storage v3 2/4] tree-wide: make use of content type assertion helper

[Daniel Kral]

Make any code path with an existent content type assertion use the newly
introduced content type assertion helper.

Where possible, existing calls to `storage_check_enabled()` can be
lowered to calls to `storage_config()` as the former subroutine is
already called in the helper already.

Also the other code path, where there was no call to
`storage_check_enabled()` before, does not break the existing API,
because it did actions on the storage anyway and would fail on an
unavailable storage.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available
  * lower storage_check_enabled to storage_config where possible due to
    the helper already checking that

 src/PVE/API2/Storage/Status.pm | 10 ++++------
 src/PVE/Storage.pm             |  3 ++-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index 14915ae..3a929d8 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -439,7 +439,7 @@ __PACKAGE__->register_method ({
 	my $cfg = PVE::Storage::config();
 
 	my ($node, $storage) = $param->@{qw(node storage)};
-	my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
+	my $scfg = PVE::Storage::storage_config($cfg, $storage);
 
 	die "can't upload to storage type '$scfg->{type}'\n"
 	    if !defined($scfg->{path});
@@ -485,8 +485,7 @@ __PACKAGE__->register_method ({
 	    raise_param_exc({ content => "upload content type '$content' not allowed" });
 	}
 
-	die "storage '$storage' does not support '$content' content\n"
-	    if !$scfg->{content}->{$content};
+	PVE::Storage::assert_content_type_available($cfg, $storage, $content, $node);
 
 	my $dest = "$path/$filename";
 	my $dirname = dirname($dest);
@@ -663,15 +662,14 @@ __PACKAGE__->register_method({
 	my $cfg = PVE::Storage::config();
 
 	my ($node, $storage, $compression) = $param->@{qw(node storage compression)};
-	my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
+	my $scfg = PVE::Storage::storage_config($cfg, $storage);
 
 	die "can't upload to storage type '$scfg->{type}', not a file based storage!\n"
 	    if !defined($scfg->{path});
 
 	my ($content, $url) = $param->@{'content', 'url'};
 
-	die "storage '$storage' is not configured for content-type '$content'\n"
-	    if !$scfg->{content}->{$content};
+	PVE::Storage::assert_content_type_available($cfg, $storage, $content, $node);
 
 	my $filename = PVE::Storage::normalize_content_filename($param->{filename});
 
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index b5a2c8a..eea51e9 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1842,7 +1842,8 @@ sub prune_backups {
     my ($cfg, $storeid, $keep, $vmid, $type, $dryrun, $logfunc) = @_;
 
     my $scfg = storage_config($cfg, $storeid);
-    die "storage '$storeid' does not support backups\n" if !$scfg->{content}->{backup};
+
+    PVE::Storage::assert_content_type_available($cfg, $storeid, 'backup');
 
     if (!defined($keep)) {
 	die "no prune-backups options configured for storage '$storeid'\n"
-- 
2.39.5



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

[pve-devel] [PATCH storage v3 3/4] vdisk_alloc: factor out optional parameters in options hash argument

[Daniel Kral]

Move the optional parameter `name` into an optional hash argument at the
end of the subroutine signature of vdisk_alloc().

This allows to add more optional arguments in the future and makes the
function call easier to follow when a name is not required.

This requires a versioned break for packages that call vdisk_alloc(),
which are currently qemu-server and pve-container.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * wrap 100+ char lines correctly
  * add $fmt back as a required parameter as it didn't make sense to
    have it be optional because of only a few places

 src/PVE/API2/Storage/Content.pm    |  5 ++---
 src/PVE/GuestImport.pm             |  2 +-
 src/PVE/Storage.pm                 | 32 ++++++++++++++++++++++++++++--
 src/test/run_test_zfspoolplugin.pl |  8 ++++----
 4 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/src/PVE/API2/Storage/Content.pm b/src/PVE/API2/Storage/Content.pm
index 8fbfbe9..64ed56d 100644
--- a/src/PVE/API2/Storage/Content.pm
+++ b/src/PVE/API2/Storage/Content.pm
@@ -218,9 +218,8 @@ __PACKAGE__->register_method ({
 
 	my $cfg = PVE::Storage::config();
 
-	my $volid = PVE::Storage::vdisk_alloc ($cfg, $storeid, $param->{vmid},
-					       $param->{format},
-					       $name, $size);
+	my $volid = PVE::Storage::vdisk_alloc(
+	    $cfg, $storeid, $param->{vmid}, $param->{format}, $size, { name => $name });
 
 	return $volid;
     }});
diff --git a/src/PVE/GuestImport.pm b/src/PVE/GuestImport.pm
index 16099ca..76ebc9d 100644
--- a/src/PVE/GuestImport.pm
+++ b/src/PVE/GuestImport.pm
@@ -69,7 +69,7 @@ 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, undef, 1024);
+	$target_volid = PVE::Storage::vdisk_alloc($cfg, $target_storeid, $vmid, $inner_fmt, 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 eea51e9..ab4dcdd 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -1056,8 +1056,36 @@ sub unmap_volume {
     return $plugin->unmap_volume($storeid, $scfg, $volname, $snapname);
 }
 
-sub vdisk_alloc {
-    my ($cfg, $storeid, $vmid, $fmt, $name, $size) = @_;
+=head3 vdisk_alloc($cfg, $storeid, $vmid, $fmt, $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 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>.
+
+Optionally, the following key-value pairs are available for C<%opts>:
+
+=over
+
+=item * C<< $name => $string >>
+
+Specifies the name of the new volume.
+
+If undefined, the name will be automatically generated.
+
+=back
+
+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) = @_;
+
+    my $name = $opts->{name};
 
     die "no storage ID specified\n" if !$storeid;
 
diff --git a/src/test/run_test_zfspoolplugin.pl b/src/test/run_test_zfspoolplugin.pl
index 095ccb3..7dd0a2b 100755
--- a/src/test/run_test_zfspoolplugin.pl
+++ b/src/test/run_test_zfspoolplugin.pl
@@ -565,7 +565,7 @@ my $test13 = sub {
     print "\nrun test13 \"vdisk_alloc\"\n";
 
     eval {
-	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "112", "raw", undef ,1024 * 1024);
+	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "112", "raw", 1024 * 1024);
 
 	if ($tmp_volid ne "$storagename:vm-112-disk-0") {
 	    die "volname:$tmp_volid don't match\n";
@@ -589,7 +589,7 @@ my $test13 = sub {
     }
 
     eval {
-	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "112", "raw", undef ,2048 * 1024);
+	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "112", "raw", 2048 * 1024);
 
 	if ($tmp_volid ne "$storagename:vm-112-disk-1") {
 	    die "volname:$tmp_volid don't match\n";
@@ -613,7 +613,7 @@ my $test13 = sub {
     }
 
     eval {
-	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "113", "subvol", undef ,1024 * 1024);
+	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "113", "subvol", 1024 * 1024);
 
 	if ($tmp_volid ne "$storagename:subvol-113-disk-0") {
 	    die "volname:$tmp_volid  don't match\n";
@@ -637,7 +637,7 @@ my $test13 = sub {
     }
 
     eval {
-	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "113", "subvol", undef ,2048 * 1024);
+	my $tmp_volid = PVE::Storage::vdisk_alloc($cfg, $storagename, "113", "subvol", 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

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

[Daniel Kral]

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

[pve-devel] [PATCH qemu-server v3 01/12] fix #5284: cli: importovf: assert content type support for target storage

[Daniel Kral]

Asserts whether the target storage supports storing VM images before
importing a OVF manifest as a VM to the target storage.

Without the check in place, a VM volume can be imported to a storage,
which does not support VM images, but won't be able to start since any
attached volume must be stored on a supported storage.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * none
  * moved up now, as this change can be applied independently from the
    cloudinit patch (the two after this one)

 PVE/CLI/qm.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 3e3a4c91..afcc0243 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -756,7 +756,9 @@ __PACKAGE__->register_method ({
 
 	die "$ovf_file: non-existent or non-regular file\n" if (! -f $ovf_file);
 	my $storecfg = PVE::Storage::config();
-	PVE::Storage::storage_check_enabled($storecfg, $storeid);
+	my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storeid);
+	raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
+	   if !$scfg->{content}->{images};
 
 	my $parsed = PVE::GuestImport::OVF::parse_ovf($ovf_file);
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 02/12] api: remove unusable default storage parameter in check_storage_access

[Daniel Kral]

Since 0541eeb8 ("use property strings for drive options") the user input
of a volume with allocation support must be a pair of a PVE-managed
storage and an "arbitrary" string, even though only a volume name or the
size of a new disk in GB is supported [0].

This means the $volid must always be the string
"$storeid:$volname_or_size" for cloudinit images and new disks.
Therefore, the $default_storage parameter cannot be set and 'cloudinit'
is an invalid value for $volid.

Remove them as they are rejected by `verify_volume_id_or_qm_path` for
allocatable disk drives before calling this subroutine anyway, which is
used by both API handlers, i.e. `create_vm` and `update_vm`, that call
the subroutine.

[0] except the special cases "none", "cdrom" and absolute paths, which
    were introduced some time later with `pve-volume-id-or-absolute-path`
    and `pve-volume-id-or-qm-path`.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * improve structure and reasoning in commit message
  * add $is_new_disk variable and always capture $storeid from it
  * add removal of ($volid eq 'cloudinit') reason to commit message

 PVE/API2/Qemu.pm | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 626cce45..693667d5 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -148,7 +148,7 @@ my $check_drive_param = sub {
 };
 
 my $check_storage_access = sub {
-   my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage, $extraction_storage) = @_;
+   my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $extraction_storage) = @_;
 
    $foreach_volume_with_alloc->($settings, sub {
 	my ($ds, $drive) = @_;
@@ -158,13 +158,15 @@ my $check_storage_access = sub {
 	my $volid = $drive->{file};
 	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
 
-	if (!$volid || ($volid eq 'none' || $volid eq 'cloudinit' || (defined($volname) && $volname eq 'cloudinit'))) {
+	my $is_new_disk = $volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE;
+	# the NEW_DISK_RE captures the correct storeid of the new disk
+	$storeid = $2 if $is_new_disk;
+
+	if (!$volid || ($volid eq 'none' || (defined($volname) && $volname eq 'cloudinit'))) {
 	    # nothing to check
 	} elsif ($isCDROM && ($volid eq 'cdrom')) {
 	    $rpcenv->check($authuser, "/", ['Sys.Console']);
-	} elsif (!$isCDROM && ($volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE)) {
-	    my $storeid = $2 || $default_storage;
-	    die "no storage ID specified (and no default storage)\n" if !$storeid;
+	} elsif (!$isCDROM && $is_new_disk) {
 	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
 	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
 	    raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
@@ -1187,8 +1189,7 @@ __PACKAGE__->register_method({
 	if (scalar(keys $param->%*) > 0) {
 	    &$resolve_cdrom_alias($param);
 
-	    &$check_storage_access(
-		$rpcenv, $authuser, $storecfg, $vmid, $param, $storage, $extraction_storage);
+	    &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $extraction_storage);
 
 	    &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ keys %$param]);
 
@@ -1966,7 +1967,7 @@ my $update_vm_api  = sub {
 
     &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, undef, [keys %$param]);
 
-    &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, undef, $extraction_storage);
+    &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $extraction_storage);
 
     PVE::QemuServer::check_bridge_access($rpcenv, $authuser, $param);
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 03/12] fix #5284: api: update-vm: assert content type support for cloudinit images

[Daniel Kral]

Asserts whether the target storage supports storing cloudinit images,
i.e. VM images, before creating a cloudinit image on the target storage.

Without the check in place, a cloudinit image can be created on the
storage, which does not support VM images, but won't be able to start
since any attached volume must be stored on a supported storage.

This enforces that allocating a new cloudinit image requires the user to
have the `Datastore.AllocateSpace` permission, which was not required
before. This does not change the behavior for re-generating them.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rebase untop changes of last patch
  * ignore cloudinit images for $isCDROM in drive_is_cdrom(...)
  * the above change fixes that cloudinit images created with
    ',media=cdrom' appended are also checked with the new_disk branch
  * make check a little more readable

 PVE/API2/Qemu.pm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 693667d5..3bdbcfab 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -153,20 +153,21 @@ my $check_storage_access = sub {
    $foreach_volume_with_alloc->($settings, sub {
 	my ($ds, $drive) = @_;
 
-	my $isCDROM = PVE::QemuServer::drive_is_cdrom($drive);
+	my $isCDROM = PVE::QemuServer::drive_is_cdrom($drive, 1);
 
 	my $volid = $drive->{file};
 	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
+	my $is_cloudinit = defined($volname) && $volname eq 'cloudinit';
 
 	my $is_new_disk = $volid =~ $PVE::QemuServer::Drive::NEW_DISK_RE;
 	# the NEW_DISK_RE captures the correct storeid of the new disk
 	$storeid = $2 if $is_new_disk;
 
-	if (!$volid || ($volid eq 'none' || (defined($volname) && $volname eq 'cloudinit'))) {
+	if (!$volid || $volid eq 'none') {
 	    # nothing to check
 	} elsif ($isCDROM && ($volid eq 'cdrom')) {
 	    $rpcenv->check($authuser, "/", ['Sys.Console']);
-	} elsif (!$isCDROM && $is_new_disk) {
+	} elsif (!$isCDROM && ($is_new_disk || $is_cloudinit)) {
 	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
 	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
 	    raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 04/12] tree-wide: update vdisk_alloc optional arguments signature

[Daniel Kral]

Update any callers of `PVE::Storage::vdisk_alloc` to the updated
function signature, which moves the optional argument `$name` to the
optional hash at the end of the argument list.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * fix lines which are 100+ chars
  * fix some wrappings to meet the styleguide
  * do not change " to '

 PVE/API2/Qemu.pm             | 7 ++++---
 PVE/QemuConfig.pm            | 3 ++-
 PVE/QemuServer.pm            | 9 ++++-----
 PVE/QemuServer/Cloudinit.pm  | 3 ++-
 PVE/QemuServer/ImportDisk.pm | 3 ++-
 PVE/VZDump/QemuServer.pm     | 2 +-
 qmextract                    | 3 +--
 test/MigrationTest/QmMock.pm | 4 ++--
 8 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 3bdbcfab..93b0bd4d 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -433,7 +433,8 @@ my sub create_disks : prototype($$$$$$$$$$$) {
 
 	    # Initial disk created with 4 MB and aligned to 4MB on regeneration
 	    my $ci_size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
-	    my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $name, $ci_size/1024);
+	    my $volid = PVE::Storage::vdisk_alloc(
+		$storecfg, $storeid, $vmid, $fmt, $ci_size/1024, { name => $name });
 	    $disk->{file} = $volid;
 	    $disk->{media} = 'cdrom';
 	    push @$vollist, $volid;
@@ -578,9 +579,9 @@ my sub create_disks : prototype($$$$$$$$$$$) {
 		} elsif ($ds eq 'tpmstate0') {
 		    # swtpm can only use raw volumes, and uses a fixed size
 		    $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
-		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size);
+		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", $size);
 		} else {
-		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
+		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $size);
 		}
 
 		# change created disk to a base volume in case the VM is a template
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 2609542c..5c052dbb 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -229,7 +229,8 @@ sub __snapshot_save_vmstate {
     my $name = "vm-$vmid-state-$snapname";
     $name .= ".raw" if $scfg->{path}; # add filename extension for file base storage
 
-    my $statefile = PVE::Storage::vdisk_alloc($storecfg, $target, $vmid, 'raw', $name, $size*1024);
+    my $statefile = PVE::Storage::vdisk_alloc(
+	$storecfg, $target, $vmid, 'raw', $size*1024, { name => $name });
     my $runningmachine = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
 
     # get current QEMU -cpu argument to ensure consistency of custom CPU models
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index ccdceedc..6f19c5d1 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5527,7 +5527,7 @@ sub vm_migrate_alloc_nbd_disks {
 	$format = $defFormat if !$format || !grep { $format eq $_ } $validFormats->@*;
 
 	my $size = $drive->{size} / 1024;
-	my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, undef, $size);
+	my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $size);
 	my $newdrive = $drive;
 	$newdrive->{format} = $format;
 	$newdrive->{file} = $newvolid;
@@ -6771,7 +6771,7 @@ my $restore_allocate_devices = sub {
 	}
 
 	my $volid = PVE::Storage::vdisk_alloc(
-	    $storecfg, $storeid, $vmid, $d->{format}, $name, $alloc_size);
+	    $storecfg, $storeid, $vmid, $d->{format}, $alloc_size, { name => $name });
 
 	print STDERR "new volume ID is '$volid'\n";
 	$d->{volid} = $volid;
@@ -8489,8 +8489,7 @@ sub clone_disk {
 	    $size = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 10);
 	}
 	$newvolid = PVE::Storage::vdisk_alloc(
-	    $storecfg, $storeid, $newvmid, $dst_format, $name, ($size/1024)
-	);
+	    $storecfg, $storeid, $newvmid, $dst_format, ($size/1024), { name => $name });
 	push @$newvollist, $newvolid;
 
 	PVE::Storage::activate_volumes($storecfg, [$newvolid]);
@@ -8619,7 +8618,7 @@ sub create_efidisk($$$$$$$$) {
 
     my $vars_size_b = -s $ovmf_vars;
     my $vars_size = PVE::Tools::convert_size($vars_size_b, 'b' => 'kb');
-    my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $vars_size);
+    my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $vars_size);
     PVE::Storage::activate_volumes($storecfg, [$volid]);
 
     qemu_img_convert($ovmf_vars, $volid, $vars_size_b);
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index 001022e6..6e2c706b 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -44,7 +44,8 @@ sub commit_cloudinit_disk {
 	$volname =~ m/(vm-$vmid-cloudinit(.\Q$format\E)?)/;
 	my $name = $1;
 	$size = 4 * 1024;
-	PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $name, $size);
+	PVE::Storage::vdisk_alloc(
+	    $storecfg, $storeid, $vmid, $format, $size, { name => $name });
 	$size *= 1024; # vdisk alloc takes KB, qemu-img dd's osize takes byte
     }
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
index 75b5e0de..d369a5e5 100755
--- a/PVE/QemuServer/ImportDisk.pm
+++ b/PVE/QemuServer/ImportDisk.pm
@@ -28,7 +28,8 @@ sub do_import {
     warn "format '$format' is not supported by the target storage - using '$dst_format' instead\n"
 	if $format && $format ne $dst_format;
 
-    my $dst_volid = PVE::Storage::vdisk_alloc($storecfg, $storage_id, $vmid, $dst_format, undef, $src_size / 1024);
+    my $dst_volid = PVE::Storage::vdisk_alloc(
+	$storecfg, $storage_id, $vmid, $dst_format, $src_size / 1024);
 
     my $zeroinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid);
 
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 4860798e..650b6643 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -593,7 +593,7 @@ my sub allocate_fleecing_images {
 		}
 
 		$di->{'fleece-volid'} = PVE::Storage::vdisk_alloc(
-		    $self->{storecfg}, $fleecing_storeid, $vmid, $format, $name, $size);
+		    $self->{storecfg}, $fleecing_storeid, $vmid, $format, $size, { name => $name });
 
 		push $fleece_volids->@*, $di->{'fleece-volid'};
 
diff --git a/qmextract b/qmextract
index 1466a586..0500c0b8 100755
--- a/qmextract
+++ b/qmextract
@@ -174,8 +174,7 @@ sub extract_archive {
 		if $format ne 'raw';
 	}
 
-	my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid,
-					      $format, undef, $alloc_size);
+	my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid, $format, $alloc_size);
 
 	print STDERR "new volume ID is '$volid'\n";
 
diff --git a/test/MigrationTest/QmMock.pm b/test/MigrationTest/QmMock.pm
index fb94c58b..a3e6cd1a 100644
--- a/test/MigrationTest/QmMock.pm
+++ b/test/MigrationTest/QmMock.pm
@@ -84,10 +84,10 @@ my $disk_counter = 10;
 
 $MigrationTest::Shared::storage_module->mock(
     vdisk_alloc => sub {
-	my ($cfg, $storeid, $vmid, $fmt, $name, $size) = @_;
+	my ($cfg, $storeid, $vmid, $fmt, $size, $opts) = @_;
 
 	die "vdisk_alloc (mocked) - name is not expected to be set - implement me\n"
-	    if defined($name);
+	    if defined($opts->{name});
 
 	my $name_without_extension = "vm-${vmid}-disk-${disk_counter}";
 	$disk_counter++;
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 05/12] tree-wide: update vdisk_alloc vtype argument signature

[Daniel Kral]

Update any callers of `PVE::Storage::vdisk_alloc` to the updated
function signature, which adds the required parameter `$vtype` that
asserts whether the underlying storage is available and supports the
content type.

Make all callers assert for 'any' volume type, i.e. skip the check for
now, so that the assertions can be added case-by-case.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * new - the parameter was optional before and could be added
    per-patch, but now needs to be added in a single patch to not break
    API inbetween

 PVE/API2/Qemu.pm             | 8 +++++---
 PVE/QemuConfig.pm            | 2 +-
 PVE/QemuServer.pm            | 8 ++++----
 PVE/QemuServer/Cloudinit.pm  | 2 +-
 PVE/QemuServer/ImportDisk.pm | 2 +-
 PVE/VZDump/QemuServer.pm     | 9 ++++++++-
 qmextract                    | 2 +-
 test/MigrationTest/QmMock.pm | 5 ++++-
 8 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 93b0bd4d..9dc40e42 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -434,7 +434,7 @@ my sub create_disks : prototype($$$$$$$$$$$) {
 	    # Initial disk created with 4 MB and aligned to 4MB on regeneration
 	    my $ci_size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
 	    my $volid = PVE::Storage::vdisk_alloc(
-		$storecfg, $storeid, $vmid, $fmt, $ci_size/1024, { name => $name });
+		$storecfg, $storeid, $vmid, $fmt, 'any', $ci_size/1024, { name => $name });
 	    $disk->{file} = $volid;
 	    $disk->{media} = 'cdrom';
 	    push @$vollist, $volid;
@@ -579,9 +579,11 @@ my sub create_disks : prototype($$$$$$$$$$$) {
 		} elsif ($ds eq 'tpmstate0') {
 		    # swtpm can only use raw volumes, and uses a fixed size
 		    $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
-		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", $size);
+		    $volid = PVE::Storage::vdisk_alloc(
+			$storecfg, $storeid, $vmid, "raw", "any", $size);
 		} else {
-		    $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $size);
+		    $volid = PVE::Storage::vdisk_alloc(
+			$storecfg, $storeid, $vmid, $fmt, "any", $size);
 		}
 
 		# change created disk to a base volume in case the VM is a template
diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 5c052dbb..19d9785e 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -230,7 +230,7 @@ sub __snapshot_save_vmstate {
     $name .= ".raw" if $scfg->{path}; # add filename extension for file base storage
 
     my $statefile = PVE::Storage::vdisk_alloc(
-	$storecfg, $target, $vmid, 'raw', $size*1024, { name => $name });
+	$storecfg, $target, $vmid, 'raw', 'any', $size*1024, { name => $name });
     my $runningmachine = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
 
     # get current QEMU -cpu argument to ensure consistency of custom CPU models
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 6f19c5d1..8e6ab92f 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5527,7 +5527,7 @@ sub vm_migrate_alloc_nbd_disks {
 	$format = $defFormat if !$format || !grep { $format eq $_ } $validFormats->@*;
 
 	my $size = $drive->{size} / 1024;
-	my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, $size);
+	my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, 'any', $size);
 	my $newdrive = $drive;
 	$newdrive->{format} = $format;
 	$newdrive->{file} = $newvolid;
@@ -6771,7 +6771,7 @@ my $restore_allocate_devices = sub {
 	}
 
 	my $volid = PVE::Storage::vdisk_alloc(
-	    $storecfg, $storeid, $vmid, $d->{format}, $alloc_size, { name => $name });
+	    $storecfg, $storeid, $vmid, $d->{format}, 'any', $alloc_size, { name => $name });
 
 	print STDERR "new volume ID is '$volid'\n";
 	$d->{volid} = $volid;
@@ -8489,7 +8489,7 @@ sub clone_disk {
 	    $size = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 10);
 	}
 	$newvolid = PVE::Storage::vdisk_alloc(
-	    $storecfg, $storeid, $newvmid, $dst_format, ($size/1024), { name => $name });
+	    $storecfg, $storeid, $newvmid, $dst_format, 'any', ($size/1024), { name => $name });
 	push @$newvollist, $newvolid;
 
 	PVE::Storage::activate_volumes($storecfg, [$newvolid]);
@@ -8618,7 +8618,7 @@ sub create_efidisk($$$$$$$$) {
 
     my $vars_size_b = -s $ovmf_vars;
     my $vars_size = PVE::Tools::convert_size($vars_size_b, 'b' => 'kb');
-    my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, $vars_size);
+    my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, 'any', $vars_size);
     PVE::Storage::activate_volumes($storecfg, [$volid]);
 
     qemu_img_convert($ovmf_vars, $volid, $vars_size_b);
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index 6e2c706b..3c44b8fb 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -45,7 +45,7 @@ sub commit_cloudinit_disk {
 	my $name = $1;
 	$size = 4 * 1024;
 	PVE::Storage::vdisk_alloc(
-	    $storecfg, $storeid, $vmid, $format, $size, { name => $name });
+	    $storecfg, $storeid, $vmid, $format, 'any', $size, { name => $name });
 	$size *= 1024; # vdisk alloc takes KB, qemu-img dd's osize takes byte
     }
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
index d369a5e5..7dc25626 100755
--- a/PVE/QemuServer/ImportDisk.pm
+++ b/PVE/QemuServer/ImportDisk.pm
@@ -29,7 +29,7 @@ sub do_import {
 	if $format && $format ne $dst_format;
 
     my $dst_volid = PVE::Storage::vdisk_alloc(
-	$storecfg, $storage_id, $vmid, $dst_format, $src_size / 1024);
+	$storecfg, $storage_id, $vmid, $dst_format, 'any', $src_size / 1024);
 
     my $zeroinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid);
 
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index 650b6643..b4bac3f9 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -593,7 +593,14 @@ my sub allocate_fleecing_images {
 		}
 
 		$di->{'fleece-volid'} = PVE::Storage::vdisk_alloc(
-		    $self->{storecfg}, $fleecing_storeid, $vmid, $format, $size, { name => $name });
+		    $self->{storecfg},
+		    $fleecing_storeid,
+		    $vmid,
+		    $format,
+		    'any',
+		    $size,
+		    { name => $name }
+		);
 
 		push $fleece_volids->@*, $di->{'fleece-volid'};
 
diff --git a/qmextract b/qmextract
index 0500c0b8..b8f874f9 100755
--- a/qmextract
+++ b/qmextract
@@ -174,7 +174,7 @@ sub extract_archive {
 		if $format ne 'raw';
 	}
 
-	my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid, $format, $alloc_size);
+	my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid, $format, 'any', $alloc_size);
 
 	print STDERR "new volume ID is '$volid'\n";
 
diff --git a/test/MigrationTest/QmMock.pm b/test/MigrationTest/QmMock.pm
index a3e6cd1a..dfe06667 100644
--- a/test/MigrationTest/QmMock.pm
+++ b/test/MigrationTest/QmMock.pm
@@ -84,11 +84,14 @@ my $disk_counter = 10;
 
 $MigrationTest::Shared::storage_module->mock(
     vdisk_alloc => sub {
-	my ($cfg, $storeid, $vmid, $fmt, $size, $opts) = @_;
+	my ($cfg, $storeid, $vmid, $fmt, $vtype, $size, $opts) = @_;
 
 	die "vdisk_alloc (mocked) - name is not expected to be set - implement me\n"
 	    if defined($opts->{name});
 
+	die "vdisk_alloc (mocked) - vtype is expected to be either any or images\n"
+	    if $vtype !~ m/(any|images)/;
+
 	my $name_without_extension = "vm-${vmid}-disk-${disk_counter}";
 	$disk_counter++;
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 06/12] cfg2cmd: improve error message for invalid volume content type

[Daniel Kral]

Improve the error message when a VM start fails due to a volume storage
not supporting the volume's required content type by prefixing it with
the disk handle (e.g. scsi0) and using the new helper.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available

 PVE/QemuServer.pm                               | 17 +++--------------
 .../unsupported-storage-content-type.conf       |  2 +-
 2 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 8e6ab92f..b6d8cae2 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3898,7 +3898,9 @@ sub config_to_command {
 	my ($ds, $drive) = @_;
 
 	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
-	    check_volume_storage_type($storecfg, $drive->{file});
+	    eval { PVE::Storage::assert_volume_type_available($storecfg, $drive->{file}) };
+	    die "$ds: $@" if $@;
+
 	    push @$vollist, $drive->{file};
 	}
 
@@ -8957,19 +8959,6 @@ sub vm_is_paused {
     );
 }
 
-sub check_volume_storage_type {
-    my ($storecfg, $vol) = @_;
-
-    my ($storeid, $volname) = PVE::Storage::parse_volume_id($vol);
-    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
-    my ($vtype) = PVE::Storage::parse_volname($storecfg, $vol);
-
-    die "storage '$storeid' does not support content-type '$vtype'\n"
-	if !$scfg->{content}->{$vtype};
-
-    return 1;
-}
-
 sub add_nets_bridge_fdb {
     my ($conf, $vmid) = @_;
 
diff --git a/test/cfg2cmd/unsupported-storage-content-type.conf b/test/cfg2cmd/unsupported-storage-content-type.conf
index e33165a8..e0903eff 100644
--- a/test/cfg2cmd/unsupported-storage-content-type.conf
+++ b/test/cfg2cmd/unsupported-storage-content-type.conf
@@ -1,3 +1,3 @@
 # TEST: Unsupported storage content type in a volume disk
-# EXPECT_ERROR: storage 'noimages' does not support content-type 'images'
+# EXPECT_ERROR: scsi0: storage 'noimages' does not support content type 'images'
 scsi0: noimages:8006/vm-8006-disk-0.raw,iothread=1,size=32G
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 07/12] api: {clone, move}_vm: use volume content type assertion helpers

[Daniel Kral]

Make use of the newly introduced content type assertion helpers in the
preconditions check paths and the call to `vdisk_alloc` in the API
handlers for cloning VMs to another storage and moving a VM disk to
another storage with the newly introduced helper.

Since the preconditions existed before, adding a content type safeguard
at `vdisk_alloc` does not break the API.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available
  * lower storage_check_enabled to storage_config where possible due to
    the helper already checking that
  * change of vdisk_alloc signature with required $vtype

 PVE/API2/Qemu.pm  | 11 +++++------
 PVE/QemuServer.pm |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9dc40e42..e9eb2b2e 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3969,9 +3969,9 @@ __PACKAGE__->register_method({
 
 	    if ($storage) {
 		# check if storage is enabled on local node and supports vm images
-		my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storage);
-		raise_param_exc({ storage => "storage '$storage' does not support vm images" })
-		    if !$scfg->{content}->{images};
+		my $scfg = PVE::Storage::storage_config($storecfg, $storage);
+		eval { PVE::Storage::assert_content_type_available($storecfg, $storage, 'images') };
+		raise_param_exc({ storage => "$@" }) if $@;
 
 		if ($target) {
 		    # check if storage is available on target node
@@ -4352,9 +4352,8 @@ __PACKAGE__->register_method({
 	    die "you can't move to the same storage with same format\n"
 		if $oldstoreid eq $storeid && (!$format || !$oldfmt || $oldfmt eq $format);
 
-	    my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storeid);
-	    raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
-		if !$scfg->{content}->{images};
+	    eval { PVE::Storage::assert_content_type_available($storecfg, $storeid, 'images') };
+	    raise_param_exc({ storage => "$@" }) if $@;
 
 	    # this only checks snapshots because $disk is passed!
 	    my $snapshotted = PVE::QemuServer::Drive::is_volume_in_use(
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b6d8cae2..4fb8bff0 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8491,7 +8491,7 @@ sub clone_disk {
 	    $size = PVE::Storage::volume_size_info($storecfg, $drive->{file}, 10);
 	}
 	$newvolid = PVE::Storage::vdisk_alloc(
-	    $storecfg, $storeid, $newvmid, $dst_format, 'any', ($size/1024), { name => $name });
+	    $storecfg, $storeid, $newvmid, $dst_format, 'images', ($size/1024), { name => $name });
 	push @$newvollist, $newvolid;
 
 	PVE::Storage::activate_volumes($storecfg, [$newvolid]);
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 08/12] api: {create, update}_vm: use volume content type assertion helpers

[Daniel Kral]

Make use of the newly introduced content type assertion helpers in the
preconditions check paths and the call to `vdisk_alloc` in the API
handlers for creating a VM or updating a VM with newly allocated VM,
EFI, TPM and/or cloudinit images.

Since the preconditions existed before, adding a content type safeguard
at `vdisk_alloc` does not break the API.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available
  * lower storage_check_enabled to storage_config where possible due to
    the helper already checking that
  * change of vdisk_alloc signature with required $vtype

 PVE/API2/Qemu.pm            | 11 +++++------
 PVE/QemuServer.pm           |  2 +-
 PVE/QemuServer/Cloudinit.pm |  2 +-
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index e9eb2b2e..6931aa18 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -169,9 +169,8 @@ my $check_storage_access = sub {
 	    $rpcenv->check($authuser, "/", ['Sys.Console']);
 	} elsif (!$isCDROM && ($is_new_disk || $is_cloudinit)) {
 	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
-	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
-	    raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
-		if !$scfg->{content}->{images};
+	    eval { PVE::Storage::assert_content_type_available($storecfg, $storeid, 'images') };
+	    raise_param_exc({ storage => "$@" }) if $@;
 	} else {
 	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
 	    if ($storeid) {
@@ -434,7 +433,7 @@ my sub create_disks : prototype($$$$$$$$$$$) {
 	    # Initial disk created with 4 MB and aligned to 4MB on regeneration
 	    my $ci_size = PVE::QemuServer::Cloudinit::CLOUDINIT_DISK_SIZE;
 	    my $volid = PVE::Storage::vdisk_alloc(
-		$storecfg, $storeid, $vmid, $fmt, 'any', $ci_size/1024, { name => $name });
+		$storecfg, $storeid, $vmid, $fmt, 'images', $ci_size/1024, { name => $name });
 	    $disk->{file} = $volid;
 	    $disk->{media} = 'cdrom';
 	    push @$vollist, $volid;
@@ -580,10 +579,10 @@ my sub create_disks : prototype($$$$$$$$$$$) {
 		    # swtpm can only use raw volumes, and uses a fixed size
 		    $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb');
 		    $volid = PVE::Storage::vdisk_alloc(
-			$storecfg, $storeid, $vmid, "raw", "any", $size);
+			$storecfg, $storeid, $vmid, "raw", "images", $size);
 		} else {
 		    $volid = PVE::Storage::vdisk_alloc(
-			$storecfg, $storeid, $vmid, $fmt, "any", $size);
+			$storecfg, $storeid, $vmid, $fmt, "images", $size);
 		}
 
 		# change created disk to a base volume in case the VM is a template
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 4fb8bff0..060c2b5b 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8620,7 +8620,7 @@ sub create_efidisk($$$$$$$$) {
 
     my $vars_size_b = -s $ovmf_vars;
     my $vars_size = PVE::Tools::convert_size($vars_size_b, 'b' => 'kb');
-    my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, 'any', $vars_size);
+    my $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, 'images', $vars_size);
     PVE::Storage::activate_volumes($storecfg, [$volid]);
 
     qemu_img_convert($ovmf_vars, $volid, $vars_size_b);
diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
index 3c44b8fb..4f416835 100644
--- a/PVE/QemuServer/Cloudinit.pm
+++ b/PVE/QemuServer/Cloudinit.pm
@@ -45,7 +45,7 @@ sub commit_cloudinit_disk {
 	my $name = $1;
 	$size = 4 * 1024;
 	PVE::Storage::vdisk_alloc(
-	    $storecfg, $storeid, $vmid, $format, 'any', $size, { name => $name });
+	    $storecfg, $storeid, $vmid, $format, 'images', $size, { name => $name });
 	$size *= 1024; # vdisk alloc takes KB, qemu-img dd's osize takes byte
     }
     my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 09/12] api: import{disk, ovf}: use volume content type assertion helpers

[Daniel Kral]

Make use of the newly introduced content type assertion helpers in the
preconditions check paths and the call to `vdisk_alloc` in the API
handlers for importing VM disks or importing OVF manifests as VMs.

Since the preconditions existed before, adding a content type safeguard
at `vdisk_alloc` does not break the API.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available
  * lower/remove storage_check_enabled to storage_config where possible
    due to the helper already checking that
  * change of vdisk_alloc signature with required $vtype
  * keep raise_param_exc for importovf, which was accidentally "lowered"
    to a die before

 PVE/CLI/qm.pm                | 11 +++--------
 PVE/QemuServer/ImportDisk.pm |  2 +-
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index afcc0243..0ecd5fcb 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -606,11 +606,7 @@ __PACKAGE__->register_method ({
 	die "$source: non-existent or non-regular file\n" if (! -f $source);
 
 	my $storecfg = PVE::Storage::config();
-	PVE::Storage::storage_check_enabled($storecfg, $storeid);
-
-	my $target_storage_config =  PVE::Storage::storage_config($storecfg, $storeid);
-	die "storage $storeid does not support vm images\n"
-	    if !$target_storage_config->{content}->{images};
+	PVE::Storage::assert_content_type_available($storecfg, $storeid, 'images');
 
 	print "importing disk '$source' to VM $vmid ...\n";
 
@@ -756,9 +752,8 @@ __PACKAGE__->register_method ({
 
 	die "$ovf_file: non-existent or non-regular file\n" if (! -f $ovf_file);
 	my $storecfg = PVE::Storage::config();
-	my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storeid);
-	raise_param_exc({ storage => "storage '$storeid' does not support vm images" })
-	   if !$scfg->{content}->{images};
+	eval { PVE::Storage::assert_content_type_available($storecfg, $storeid, 'images') };
+	raise_param_exc({ storage => "$@" }) if $@;
 
 	my $parsed = PVE::GuestImport::OVF::parse_ovf($ovf_file);
 
diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
index 7dc25626..365f21d2 100755
--- a/PVE/QemuServer/ImportDisk.pm
+++ b/PVE/QemuServer/ImportDisk.pm
@@ -29,7 +29,7 @@ sub do_import {
 	if $format && $format ne $dst_format;
 
     my $dst_volid = PVE::Storage::vdisk_alloc(
-	$storecfg, $storage_id, $vmid, $dst_format, 'any', $src_size / 1024);
+	$storecfg, $storage_id, $vmid, $dst_format, 'images', $src_size / 1024);
 
     my $zeroinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid);
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 10/12] api: qmrestore: use volume content type assertion helpers

[Daniel Kral]

Make use of the newly introduced content type assertion helpers in the
preconditions check paths and the call to `vdisk_alloc` in the API and
CLI handler for restoring VMs to a specified target storage.

Since the preconditions existed before, adding a content type safeguard
at `vdisk_alloc` does not break the API.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available
  * lower storage_check_enabled to storage_config where possible due to
    the helper already checking that
  * change of vdisk_alloc signature with required $vtype

 PVE/QemuServer.pm | 13 +++++--------
 qmextract         |  3 ++-
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 060c2b5b..ed207192 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6687,11 +6687,10 @@ my $parse_backup_hints = sub {
     my ($rpcenv, $user, $storecfg, $fh, $devinfo, $options) = @_;
 
     my $check_storage = sub { # assert if an image can be allocate
-	my ($storeid, $scfg) = @_;
-	die "Content type 'images' is not available on storage '$storeid'\n"
-	    if !$scfg->{content}->{images};
+	my ($storeid) = @_;
 	$rpcenv->check($user, "/storage/$storeid", ['Datastore.AllocateSpace'])
 	    if $user ne 'root@pam';
+	PVE::Storage::assert_content_type_available($storecfg, $storeid, 'images');
     };
 
     my $virtdev_hash = {};
@@ -6712,8 +6711,7 @@ my $parse_backup_hints = sub {
 	    $devinfo->{$devname}->{format} = $format;
 	    $devinfo->{$devname}->{storeid} = $storeid;
 
-	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
-	    $check_storage->($storeid, $scfg); # permission and content type check
+	    $check_storage->($storeid); # permission and content type check
 
 	    $virtdev_hash->{$virtdev} = $devinfo->{$devname};
 	} elsif ($line =~ m/^((?:ide|sata|scsi)\d+):\s*(.*)\s*$/) {
@@ -6723,10 +6721,9 @@ my $parse_backup_hints = sub {
 	    if (drive_is_cloudinit($drive)) {
 		my ($storeid, $volname) = PVE::Storage::parse_volume_id($drive->{file});
 		$storeid = $options->{storage} if defined ($options->{storage});
-		my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
 		my $format = eval { checked_volume_format($storecfg, $drive->{file}) } // 'raw';
 
-		$check_storage->($storeid, $scfg); # permission and content type check
+		$check_storage->($storeid); # permission and content type check
 
 		$virtdev_hash->{$virtdev} = {
 		    format => $format,
@@ -6773,7 +6770,7 @@ my $restore_allocate_devices = sub {
 	}
 
 	my $volid = PVE::Storage::vdisk_alloc(
-	    $storecfg, $storeid, $vmid, $d->{format}, 'any', $alloc_size, { name => $name });
+	    $storecfg, $storeid, $vmid, $d->{format}, 'images', $alloc_size, { name => $name });
 
 	print STDERR "new volume ID is '$volid'\n";
 	$d->{volid} = $volid;
diff --git a/qmextract b/qmextract
index b8f874f9..762fea0b 100755
--- a/qmextract
+++ b/qmextract
@@ -174,7 +174,8 @@ sub extract_archive {
 		if $format ne 'raw';
 	}
 
-	my $volid = PVE::Storage::vdisk_alloc($cfg, $storeid, $vmid, $format, 'any', $alloc_size);
+	my $volid = PVE::Storage::vdisk_alloc(
+	    $cfg, $storeid, $vmid, $format, 'images', $alloc_size);
 
 	print STDERR "new volume ID is '$volid'\n";
 
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 11/12] api: migrate_vm: use volume content type assertion helpers

[Daniel Kral]

Make use of the newly introduced content type assertion helpers in the
preconditions check paths and the call to `vdisk_alloc` in the API
handler for migrating VMs (and its subcommands and `mtunnel` endpoint).

Since the preconditions existed before, adding a content type safeguard
at `vdisk_alloc` does not break the API.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available
  * lower/remove storage_check_enabled to storage_config where possible
    due to the helper already checking that
  * change of vdisk_alloc signature with required $vtype
  * add comment about why target_storage_check_available cannot use
    assert_volume_type_available(...)
  * change back mock in QmMock to only expect 'images' now

 PVE/API2/Qemu.pm             |  7 +------
 PVE/QemuMigrate.pm           | 17 ++++++++---------
 PVE/QemuServer.pm            | 17 ++++++++---------
 test/MigrationTest/QmMock.pm |  4 ++--
 4 files changed, 19 insertions(+), 26 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 6931aa18..6a882400 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -263,13 +263,8 @@ my $check_storage_access_clone = sub {
 my $check_storage_access_migrate = sub {
     my ($rpcenv, $authuser, $storecfg, $storage, $node) = @_;
 
-    PVE::Storage::storage_check_enabled($storecfg, $storage, $node);
-
     $rpcenv->check($authuser, "/storage/$storage", ['Datastore.AllocateSpace']);
-
-    my $scfg = PVE::Storage::storage_config($storecfg, $storage);
-    die "storage '$storage' does not support vm images\n"
-	if !$scfg->{content}->{images};
+    PVE::Storage::assert_content_type_available($storecfg, $storage, 'images', $node);
 };
 
 my $import_from_volid = sub {
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index b7bf2aa3..fca53a45 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -159,15 +159,14 @@ sub target_storage_check_available {
     my ($self, $storecfg, $targetsid, $volid) = @_;
 
     if (!$self->{opts}->{remote}) {
-	# check if storage is available on target node
-	my $target_scfg = PVE::Storage::storage_check_enabled(
-	    $storecfg,
-	    $targetsid,
-	    $self->{node},
-	);
-	my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
-	die "$volid: content type '$vtype' is not available on storage '$targetsid'\n"
-	    if !$target_scfg->{content}->{$vtype};
+	# check if storage is available on target node and target supports the volume's content type
+	# cannot use assert_volume_type_available(), because it checks the volume's current storage
+	eval {
+	    my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
+	    PVE::Storage::assert_content_type_available(
+		$storecfg, $targetsid, $vtype, $self->{node});
+	};
+	die "$volid: $@" if $@;
     }
 }
 
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index ed207192..f3ce3892 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2578,14 +2578,12 @@ sub check_storage_availability {
 	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
 	return if !$sid;
 
-	# check if storage is available on both nodes
-	my $scfg = PVE::Storage::storage_check_enabled($storecfg, $sid);
-	PVE::Storage::storage_check_enabled($storecfg, $sid, $node);
-
-	my ($vtype) = PVE::Storage::parse_volname($storecfg, $volid);
-
-	die "$volid: content type '$vtype' is not available on storage '$sid'\n"
-	    if !$scfg->{content}->{$vtype};
+	# check if storage is available on both nodes and target supports the volume's content type
+	eval {
+	    PVE::Storage::storage_check_enabled($storecfg, $sid);
+	    PVE::Storage::assert_volume_type_available($storecfg, $volid, $node);
+	};
+	die "$volid: $@" if $@;
    });
 }
 
@@ -5529,7 +5527,8 @@ sub vm_migrate_alloc_nbd_disks {
 	$format = $defFormat if !$format || !grep { $format eq $_ } $validFormats->@*;
 
 	my $size = $drive->{size} / 1024;
-	my $newvolid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $format, 'any', $size);
+	my $newvolid = PVE::Storage::vdisk_alloc(
+	    $storecfg, $storeid, $vmid, $format, 'images', $size);
 	my $newdrive = $drive;
 	$newdrive->{format} = $format;
 	$newdrive->{file} = $newvolid;
diff --git a/test/MigrationTest/QmMock.pm b/test/MigrationTest/QmMock.pm
index dfe06667..bea0c33e 100644
--- a/test/MigrationTest/QmMock.pm
+++ b/test/MigrationTest/QmMock.pm
@@ -89,8 +89,8 @@ $MigrationTest::Shared::storage_module->mock(
 	die "vdisk_alloc (mocked) - name is not expected to be set - implement me\n"
 	    if defined($opts->{name});
 
-	die "vdisk_alloc (mocked) - vtype is expected to be either any or images\n"
-	    if $vtype !~ m/(any|images)/;
+	die "vdisk_alloc (mocked) - vtype is expected to be images\n"
+	    if $vtype ne 'images';
 
 	my $name_without_extension = "vm-${vmid}-disk-${disk_counter}";
 	$disk_counter++;
-- 
2.39.5



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

[pve-devel] [PATCH qemu-server v3 12/12] tree-wide: add todos for breaking content type assertions

[Daniel Kral]

Snapshot state files and fleecing images are currently allowed to be put
on storages, which do not support VM images. For consistency's sake,
these should also only be created on storages supporting VM images, but
this would break the API for users now, so postpone this until PVE 9.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * none, except rebase

 PVE/QemuConfig.pm        | 1 +
 PVE/VZDump/QemuServer.pm | 1 +
 2 files changed, 2 insertions(+)

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 19d9785e..e92bb165 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -229,6 +229,7 @@ sub __snapshot_save_vmstate {
     my $name = "vm-$vmid-state-$snapname";
     $name .= ".raw" if $scfg->{path}; # add filename extension for file base storage
 
+    # TODO PVE 9: assert for supported content type 'images' here
     my $statefile = PVE::Storage::vdisk_alloc(
 	$storecfg, $target, $vmid, 'raw', 'any', $size*1024, { name => $name });
     my $runningmachine = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index b4bac3f9..ad83d405 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -592,6 +592,7 @@ my sub allocate_fleecing_images {
 		    $size = PVE::Tools::convert_size($di->{'block-node-size'}, 'b' => 'kb');
 		}
 
+		# TODO PVE 9: assert for supported content type 'images' here
 		$di->{'fleece-volid'} = PVE::Storage::vdisk_alloc(
 		    $self->{storecfg},
 		    $fleecing_storeid,
-- 
2.39.5



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

[pve-devel] [PATCH container v3 01/11] migration: prepare: factor out common read-only variables

[Daniel Kral]

Factor out the common read-only variables to allow the second call to
`storage_check_enabled` to be below 100 characters.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * none

 src/PVE/LXC/Migrate.pm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index e1e6cab..f06a10a 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -71,7 +71,8 @@ sub prepare {
 	die "can't determine assigned storage for mount point '$ms'\n" if !$storage;
 
 	# check if storage is available on both nodes
-	my $scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $storage);
+	my ($storecfg, $node) = $self->@{qw(storecfg node)};
+	my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storage);
 
 	my $targetsid = $storage;
 
@@ -92,7 +93,7 @@ sub prepare {
 	}
 
 	if (!$remote) {
-	    my $target_scfg = PVE::Storage::storage_check_enabled($self->{storecfg}, $targetsid, $self->{node});
+	    my $target_scfg = PVE::Storage::storage_check_enabled($storecfg, $targetsid, $node);
 
 	    die "$volid: content type 'rootdir' is not available on storage '$targetsid'\n"
 		if !$target_scfg->{content}->{rootdir};
-- 
2.39.5



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

[pve-devel] [PATCH container v3 02/11] tests: add tests for expected behavior of alloc_disk wrapper

[Daniel Kral]

Adds test cases for the alloc_disk wrapper subroutine to ensure that:

- zero-sized volumes are allocated as subvols on path-based storages
- non-zero-sized volumes are allocated as raw on path-based storages
- volumes are allocated as raw on btrfs storages without quotas
- volumes are allocated as subvols on btrfs storages with quotas
- volumes are allocated as subvols on zfs storages
- volumes cannot be allocated on storages that do not support rootdir

These test cases should allow to catch regressions in following changes
to the alloc_disk wrapper.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * added a content type (images) to the 'norootdirs' test storage
  * destructure @result to $volid and $needs_chown
  * simplify logic for $should_fail

 src/test/Makefile                |   5 +-
 src/test/run_alloc_disk_tests.pl | 157 +++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+), 1 deletion(-)
 create mode 100755 src/test/run_alloc_disk_tests.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index 91ae6ff..729f981 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -2,7 +2,7 @@ RUN_USERNS := lxc-usernsexec -m "u:0:`id -u`:1" -m "g:0:`id -g`:1" --
 
 all: test
 
-test: test_setup test_snapshot test_bindmount test_idmap
+test: test_setup test_snapshot test_bindmount test_idmap test_alloc_disk
 
 test_setup: run_setup_tests.pl
 	if test -e /run/lock/sbuild; then \
@@ -24,5 +24,8 @@ test_bindmount: bindmount_test.pl
 test_idmap: run_idmap_tests.pl
 	./run_idmap_tests.pl
 
+test_alloc_disk: run_alloc_disk_tests.pl
+	./run_alloc_disk_tests.pl
+
 clean:
 	rm -rf tmprootfs
diff --git a/src/test/run_alloc_disk_tests.pl b/src/test/run_alloc_disk_tests.pl
new file mode 100755
index 0000000..2af64e6
--- /dev/null
+++ b/src/test/run_alloc_disk_tests.pl
@@ -0,0 +1,157 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use PVE::Tools;
+
+use Test::More;
+use Test::MockModule;
+
+use PVE::LXC;
+
+my $test_vmid = 100;
+my $test_root_uid = 100000;
+my $test_root_gid = 100000;
+
+my $storage_config = {
+    ids => {
+	local => {
+	    content => {
+		rootdir => 1,
+	    },
+	    path => "/var/lib/vz",
+	    type => "dir",
+	    shared => 0,
+	},
+	norootdirs => {
+	    content => {
+		images => 1,
+	    },
+	    path => '/var/lib/vz',
+	    type => 'dir',
+	},
+	btrfsstore => {
+	    content => {
+		rootdir => 1,
+	    },
+	    path => '/butter/bread',
+	    type => 'btrfs',
+	},
+	btrfsquotas => {
+	    content => {
+		rootdir => 1,
+	    },
+	    path => '/butter/bread',
+	    type => 'btrfs',
+	    quotas => 1,
+	},
+	zfspool0 => {
+	    type => 'zfspool',
+	    content => {
+		rootdir => 1,
+	    },
+	    pool => 'rpool0',
+	    mountpoint => '/zfspool0',
+	},
+    },
+};
+
+my $storage_module = Test::MockModule->new("PVE::Storage");
+$storage_module->redefine(
+    vdisk_alloc => sub {
+	my ($storecfg, $storage, $vmid, $fmt, $name, $size_kb) = @_;
+
+	$fmt //= '';
+	my $prefix = ($fmt eq 'subvol') ? 'subvol' : 'vm';
+
+	return "$storage:$prefix-$vmid-disk-0";
+    },
+);
+
+my $format_disk_called = 0;
+
+my $lxc_module = Test::MockModule->new("PVE::LXC");
+$lxc_module->redefine(
+    format_disk => sub {
+	$format_disk_called = 1;
+    },
+);
+
+my $tests = [
+    {
+	description => 'allocate zero-sized volume on path-based storage',
+	storage => 'local',
+	size_kb => 0,
+	result => ["local:subvol-$test_vmid-disk-0", 1],
+    },
+    {
+	description => 'allocate non-zero-sized volume on path-based storage',
+	should_format_disk => 1,
+	storage => 'local',
+	size_kb => 1024 * 1024,
+	result => ["local:vm-$test_vmid-disk-0", 0],
+    },
+    {
+	description => 'allocate volume on btrfs with quotas disabled',
+	should_format_disk => 1,
+	storage => 'btrfsstore',
+	size_kb => 1024 * 1024,
+	result => ["btrfsstore:vm-$test_vmid-disk-0", 0],
+    },
+    {
+	description => 'allocate volume on btrfs with quotas enabled',
+	storage => 'btrfsquotas',
+	size_kb => 1024 * 1024,
+	result => ["btrfsquotas:subvol-$test_vmid-disk-0", 1],
+    },
+    {
+	description => 'allocate volume on zfspool',
+	storage => 'zfspool0',
+	size_kb => 1024 * 1024,
+	result => ["zfspool0:subvol-$test_vmid-disk-0", 1],
+    },
+    {
+	description => 'allocate volume on storage without rootdir content type support',
+	should_fail => 1,
+	storage => 'norootdirs',
+	size_kb => 1024 * 1024,
+    },
+];
+
+# multiply by 2 because of format_disk test
+plan(tests => 2 * scalar($tests->@*));
+
+for my $case ($tests->@*) {
+    my $should_format_disk = exists($case->{should_format_disk}) ? $case->{should_format_disk} : 0;
+    $format_disk_called = 0;
+
+    my ($volid, $needs_chown) = eval {
+	PVE::LXC::alloc_disk(
+	    $storage_config,
+	    $test_vmid,
+	    $case->{storage},
+	    $case->{size_kb},
+	    $test_root_uid,
+	    $test_root_gid
+	)
+    };
+    if (my $err = $@) {
+	my $should_fail = exists($case->{should_fail}) ? $case->{should_fail} : 0;
+
+	if ($should_fail) {
+	    pass("should fail: $case->{description}");
+	} else {
+	    diag explain $err;
+	    fail("should fail: $case->{description}");
+	}
+    } else {
+	is_deeply([$volid, $needs_chown], $case->{result}, $case->{description});
+    }
+
+    is($format_disk_called, $should_format_disk, "should format_disk: $case->{description}");
+}
+
+done_testing();
-- 
2.39.5



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

[pve-devel] [PATCH container v3 03/11] alloc disk: fix content type check for ZFS storages

[Daniel Kral]

There were existing assertions whether the underlying storage supports
the content type 'rootdir' for the case where volumes are created on ZFS
storages in the create_vm API handler and update_pct_config().

This assertion was missing from the case where volumes are copied with
copy_volume(), so always assert the support of rootdirs in storages when
allocating a volume with alloc_disk().

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * change commit summary and message the change for zfs pools

 src/PVE/LXC.pm | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index a58c997..8581da6 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2171,7 +2171,11 @@ sub alloc_disk {
 
     eval {
 	my $do_format = 0;
-	if ($scfg->{content}->{rootdir} && $scfg->{path}) {
+
+	die "storage '$storage' does not support content type 'rootdir'\n"
+	    if !$scfg->{content}->{rootdir};
+
+	if ($scfg->{path}) {
 	    if ($size_kb > 0 && !($scfg->{type} eq 'btrfs' && $scfg->{quotas})) {
 		$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
 		$do_format = 1;
@@ -2182,11 +2186,9 @@ sub alloc_disk {
 	} elsif ($scfg->{type} eq 'zfspool') {
 	    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb);
 	    $needs_chown = 1;
-	} elsif ($scfg->{content}->{rootdir}) {
+	} else {
 	    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
 	    $do_format = 1;
-	} else {
-	    die "content type 'rootdir' is not available or configured on storage '$storage'\n";
 	}
 	format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
     };
-- 
2.39.5



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

[pve-devel] [PATCH container v3 04/11] alloc_disk: factor out common arguments for call to vdisk_alloc

[Daniel Kral]

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * none

 src/PVE/LXC.pm | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 8581da6..780a0d4 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2170,6 +2170,7 @@ sub alloc_disk {
     # fixme: use better naming ct-$vmid-disk-X.raw?
 
     eval {
+	my $format = 'raw';
 	my $do_format = 0;
 
 	die "storage '$storage' does not support content type 'rootdir'\n"
@@ -2177,19 +2178,20 @@ sub alloc_disk {
 
 	if ($scfg->{path}) {
 	    if ($size_kb > 0 && !($scfg->{type} eq 'btrfs' && $scfg->{quotas})) {
-		$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
 		$do_format = 1;
 	    } else {
-		$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb);
+		$format = 'subvol';
 		$needs_chown = 1;
 	    }
 	} elsif ($scfg->{type} eq 'zfspool') {
-	    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'subvol', undef, $size_kb);
+	    $format = 'subvol';
 	    $needs_chown = 1;
 	} else {
-	    $volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, 'raw', undef, $size_kb);
 	    $do_format = 1;
 	}
+
+	$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, undef, $size_kb);
+
 	format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
     };
     if (my $err = $@) {
-- 
2.39.5



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

[pve-devel] [PATCH container v3 05/11] alloc_disk: simplify storage-specific logic for vdisk_alloc arguments

[Daniel Kral]

Since there are only two distinct outcomes, i.e. raw format + do_format
and subvol format + needs_chown, break down the conditions to reduce the
branching depth.

Drop the check for $scfg->{path} for BTRFS and ZFS pool storages, since
BTRFS must always have a path and ZFS pool can never have a path.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * drop $scfg->{path} check for btrfs and ZFS pool and add the
    reasoning for that to the commit message

 src/PVE/LXC.pm | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 780a0d4..97c0fa6 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2176,14 +2176,11 @@ sub alloc_disk {
 	die "storage '$storage' does not support content type 'rootdir'\n"
 	    if !$scfg->{content}->{rootdir};
 
-	if ($scfg->{path}) {
-	    if ($size_kb > 0 && !($scfg->{type} eq 'btrfs' && $scfg->{quotas})) {
-		$do_format = 1;
-	    } else {
-		$format = 'subvol';
-		$needs_chown = 1;
-	    }
-	} elsif ($scfg->{type} eq 'zfspool') {
+	my $is_unsized_on_path = $scfg->{path} && $size_kb <= 0;
+	my $is_btrfs_quotas = $scfg->{type} eq 'btrfs' && $scfg->{quotas};
+	my $is_zfs_pool = $scfg->{type} eq 'zfspool';
+
+	if ($is_unsized_on_path || $is_btrfs_quotas || $is_zfs_pool) {
 	    $format = 'subvol';
 	    $needs_chown = 1;
 	} else {
-- 
2.39.5



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

[pve-devel] [PATCH container v3 06/11] alloc_disk: update vdisk_alloc optional arguments signature

[Daniel Kral]

Update the call to `PVE::Storage::vdisk_alloc` to the updated function
signature, which moves the optional argument `$name` (as it may be
passed as undefined), to the optional hash argument at the end of the
argument list.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available
  * lower/remove storage_check_enabled to storage_config where possible
    due to the helper already checking that
  * change of vdisk_alloc signature with required $vtype

 src/PVE/LXC.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 97c0fa6..4209233 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2187,7 +2187,7 @@ sub alloc_disk {
 	    $do_format = 1;
 	}
 
-	$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, undef, $size_kb);
+	$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, $size_kb);
 
 	format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
     };
-- 
2.39.5



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

[pve-devel] [PATCH container v3 07/11] alloc_disk: use volume content type assertion helpers

[Daniel Kral]

Make use of the newly introduced content type assertion helpers in the
precondition check path and the call to `vdisk_alloc` in the wrapper for
allocating container's disks.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available
  * lower/remove storage_check_enabled to storage_config where possible
    due to the helper already checking that
  * change of vdisk_alloc signature with required $vtype

 src/PVE/LXC.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 4209233..799aa9a 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -2173,8 +2173,7 @@ sub alloc_disk {
 	my $format = 'raw';
 	my $do_format = 0;
 
-	die "storage '$storage' does not support content type 'rootdir'\n"
-	    if !$scfg->{content}->{rootdir};
+	PVE::Storage::assert_content_type_available($storecfg, $storage, 'rootdir');
 
 	my $is_unsized_on_path = $scfg->{path} && $size_kb <= 0;
 	my $is_btrfs_quotas = $scfg->{type} eq 'btrfs' && $scfg->{quotas};
@@ -2187,7 +2186,8 @@ sub alloc_disk {
 	    $do_format = 1;
 	}
 
-	$volid = PVE::Storage::vdisk_alloc($storecfg, $storage, $vmid, $format, $size_kb);
+	$volid = PVE::Storage::vdisk_alloc(
+	    $storecfg, $storage, $vmid, $format, 'rootdir', $size_kb);
 
 	format_disk($storecfg, $volid, $root_uid, $root_gid) if $do_format;
     };
-- 
2.39.5



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

[pve-devel] [PATCH container v3 08/11] api: create: use volume content type assertion helpers

[Daniel Kral]

Make use of newly introduced content type assertion helpers in the
preconditions check path in the API handler for creating a container.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available
  * lower/remove storage_check_enabled to storage_config where possible
    due to the helper already checking that

 src/PVE/API2/LXC.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 5c6ee57..947021a 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -317,10 +317,10 @@ __PACKAGE__->register_method({
 	my $check_and_activate_storage = sub {
 	    my ($sid) = @_;
 
-	    my $scfg = PVE::Storage::storage_check_enabled($storage_cfg, $sid, $node);
-
-	    raise_param_exc({ storage => "storage '$sid' does not support container directories"})
-		if !$scfg->{content}->{rootdir};
+	    eval {
+		PVE::Storage::assert_content_type_available($storage_cfg, $sid, 'rootdir', $node)
+	    };
+	    raise_param_exc({ storage => "$@" }) if $@;
 
 	    $rpcenv->check($authuser, "/storage/$sid", ['Datastore.AllocateSpace']);
 
-- 
2.39.5



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

[pve-devel] [PATCH container v3 09/11] migration: prepare: use volume content type assertion helpers

[Daniel Kral]

Make use of newly introduced content type assertion helpers in the
preconditions check and preparation path of the migration API handlers.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available
  * lower/remove storage_check_enabled to storage_config where possible
    due to the helper already checking that

 src/PVE/API2/LXC.pm    |  7 +------
 src/PVE/LXC/Migrate.pm | 14 ++++++--------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 947021a..73e1bf4 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -54,13 +54,8 @@ my sub assert_not_restore_from_external {
 my $check_storage_access_migrate = sub {
     my ($rpcenv, $authuser, $storecfg, $storage, $node) = @_;
 
-    PVE::Storage::storage_check_enabled($storecfg, $storage, $node);
-
     $rpcenv->check($authuser, "/storage/$storage", ['Datastore.AllocateSpace']);
-
-    my $scfg = PVE::Storage::storage_config($storecfg, $storage);
-    die "storage '$storage' does not support CT rootdirs\n"
-	if !$scfg->{content}->{rootdir};
+    PVE::Storage::assert_content_type_available($storecfg, $storage, 'rootdir', $node);
 };
 
 __PACKAGE__->register_method ({
diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index f06a10a..adb8fad 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -72,13 +72,11 @@ sub prepare {
 
 	# check if storage is available on both nodes
 	my ($storecfg, $node) = $self->@{qw(storecfg node)};
-	my $scfg = PVE::Storage::storage_check_enabled($storecfg, $storage);
+	my $scfg = PVE::Storage::storage_config($storecfg, $storage);
+	PVE::Storage::assert_content_type_available($storecfg, $storage, 'rootdir');
 
 	my $targetsid = $storage;
 
-	die "content type 'rootdir' is not available on storage '$storage'\n"
-	    if !$scfg->{content}->{rootdir};
-
 	if ($scfg->{shared} && !$remote) {
 	    # PVE::Storage::activate_storage checks this for non-shared storages
 	    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
@@ -93,10 +91,10 @@ sub prepare {
 	}
 
 	if (!$remote) {
-	    my $target_scfg = PVE::Storage::storage_check_enabled($storecfg, $targetsid, $node);
-
-	    die "$volid: content type 'rootdir' is not available on storage '$targetsid'\n"
-		if !$target_scfg->{content}->{rootdir};
+	    eval {
+		PVE::Storage::assert_content_type_available($storecfg, $targetsid, 'rootdir', $node)
+	    };
+	    die "$volid: $@" if $@;
 	}
 
 	$storages->{$targetsid} = 1;
-- 
2.39.5



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

[pve-devel] [PATCH container v3 10/11] api: update_vm: use volume content type assertion helpers

[Daniel Kral]

Make use of the newly introduced content type assertion helpers at
verifying the added and/or changed container config values, which is
called e.g. in the API handler for updating the container config.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available

 src/PVE/LXC/Config.pm | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 0740e8c..cedf061 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1176,9 +1176,7 @@ sub update_pct_config {
     my $check_content_type = sub {
 	my ($mp) = @_;
 	my $sid = PVE::Storage::parse_volume_id($mp->{volume});
-	my $storage_config = PVE::Storage::storage_config($storage_cfg, $sid);
-	die "storage '$sid' does not allow content type 'rootdir' (Container)\n"
-	    if !$storage_config->{content}->{rootdir};
+	PVE::Storage::assert_content_type_available($storage_cfg, $sid, 'rootdir');
     };
 
     foreach my $opt (sort keys %$param) { # add/change
-- 
2.39.5



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

[pve-devel] [PATCH container v3 11/11] mount: raw/iso: use volume content type assertion helpers

[Daniel Kral]

Make use of the newly introduced content type assertion helpers at
verifying the content type when mounting mountpoints, which have the
format "raw" or "iso".

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v2:
  * rename helper from *_supported to *_available

I already pointed that out in a response to a v2 patch, but this already
existing check in __mountpoint_mount will make it impossible AFAICS for
users to move out of the error state of a root disk / mountpoint being
on a storage that doesn't support the content type 'rootdir'. But this
would be a follow-up if we want to change it.

Also there is no check for subvols made here, i.e. container root disks
/ mountpoints on a ZFS pool can be started fine, while ones on e.g.
directory storages cannot (because they are 'raw').

And on another note: The error message from this is rather cryptic for
an end user and is only a little bit clarified in the output of `pct
start ... --debug`, but else the user must guess:

run_buffer: 571 Script exited with status 25
lxc_init: 845 Failed to run lxc.hook.pre-start for container "101"
__lxc_start: 2034 Failed to initialize container "101"
TASK ERROR: startup for container '101' failed

I skipped adding a patch for this now, as it's out of scope of this
already long patch series.

 src/PVE/LXC.pm | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 799aa9a..0beb49c 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -1978,17 +1978,17 @@ sub __mountpoint_mount {
 		}
 	    };
 	    my $use_loopdev = 0;
-	    if ($scfg->{content}->{rootdir}) {
-		if ($scfg->{path}) {
-		    $mounted_dev = run_with_loopdev($domount, $path, $readonly);
-		    $use_loopdev = 1;
-		} else {
-		    $mounted_dev = $path;
-		    &$domount($path);
-		}
+
+	    PVE::Storage::assert_content_type_available($storage_cfg, $storage, 'rootdir');
+
+	    if ($scfg->{path}) {
+		$mounted_dev = run_with_loopdev($domount, $path, $readonly);
+		$use_loopdev = 1;
 	    } else {
-		die "storage '$storage' does not support containers\n";
+		$mounted_dev = $path;
+		&$domount($path);
 	    }
+
 	    return wantarray ? ($path, $use_loopdev, $mounted_dev) : $path;
 	} else {
 	    die "unsupported image format '$format'\n";
-- 
2.39.5



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