On June 12, 2025 4:02 pm, Fiona Ebner wrote:
> With '-blockdev', it is necessary to activate the volumes to generate
> the command line, because it can be necessary to check whether the
> volume is a block device or a regular file.
>
> Signed-off-by: Fiona Ebner <f.eb...@proxmox.com>
> ---
> PVE/QemuServer.pm | 61 +++++++++++++++++++++++---------
> test/run_config2command_tests.pl | 10 ++++++
> 2 files changed, 55 insertions(+), 16 deletions(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 934adf60..82304096 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3844,7 +3844,6 @@ sub config_to_command {
> push @$devices, '-watchdog-action', $wdopts->{action} if
> $wdopts->{action};
> }
>
> - my $vollist = [];
> my $scsicontroller = {};
> my $ahcicontroller = {};
> my $scsihw = defined($conf->{scsihw}) ? $conf->{scsihw} :
> $defaults->{scsihw};
> @@ -3857,11 +3856,6 @@ sub config_to_command {
> PVE::QemuConfig->foreach_volume($conf, sub {
> my ($ds, $drive) = @_;
>
> - if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
> - check_volume_storage_type($storecfg, $drive->{file});
> - push @$vollist, $drive->{file};
> - }
> -
> # ignore efidisk here, already added in bios/fw handling code above
> return if $drive->{interface} eq 'efidisk';
> # similar for TPM
> @@ -4035,7 +4029,6 @@ sub config_to_command {
>
> if (my $vmstate = $conf->{vmstate}) {
> my $statepath = PVE::Storage::path($storecfg, $vmstate);
> - push @$vollist, $vmstate;
> push @$cmd, '-loadstate', $statepath;
> print "activating and using '$vmstate' as vmstate\n";
> }
> @@ -4051,7 +4044,7 @@ sub config_to_command {
> push @$cmd, @$aa;
> }
>
> - return wantarray ? ($cmd, $vollist, $spice_port, $pci_devices, $conf) :
> $cmd;
> + return wantarray ? ($cmd, $spice_port, $pci_devices, $conf) : $cmd;
> }
>
> sub spice_port {
> @@ -5649,12 +5642,18 @@ sub vm_start_nolock {
> my $memory = get_current_memory($conf->{memory});
> my $start_timeout = $params->{timeout} // config_aware_timeout($conf,
> $memory, $resume);
>
> - my ($cmd, $vollist, $spice_port);
> + my $vollist = get_current_vm_volumes($storecfg, $conf);
> + push $vollist->@*, $statefile if $statefile_is_a_volume;
> +
> + my ($cmd, $spice_port);
> my $pci_reserve_list = [];
> eval {
> + # With -blockdev, it is necessary to activate the volumes before
> generating the command line
> + PVE::Storage::activate_volumes($storecfg, $vollist);
> +
> # Note that for certain cases like templates, the configuration is
> minimized, so need to ensure
> # the rest of the function here uses the same configuration that was
> used to build the command
> - ($cmd, $vollist, $spice_port, my $pci_devices, $conf) =
> config_to_command(
> + ($cmd, $spice_port, my $pci_devices, $conf) = config_to_command(
> $storecfg,
> $vmid,
> $conf,
> @@ -5665,7 +5664,6 @@ sub vm_start_nolock {
> );
>
> push $cmd->@*, $state_cmdline->@*;
> - push @$vollist, $statefile if $statefile_is_a_volume;
>
> for my $device (values $pci_devices->%*) {
> next if $device->{mdev}; # we don't reserve for mdev devices
> @@ -5707,14 +5705,13 @@ sub vm_start_nolock {
> push @$cmd, '-uuid', $uuid if defined($uuid);
> };
> if (my $err = $@) {
> + eval { PVE::Storage::deactivate_volumes($storecfg, $vollist); };
> + warn $@ if $@;
> eval { cleanup_pci_devices($vmid, $conf) };
> warn $@ if $@;
> die $err;
> }
>
> - PVE::Storage::activate_volumes($storecfg, $vollist);
> -
> -
> my %silence_std_outs = (outfunc => sub {}, errfunc => sub {});
> eval { run_command(['/bin/systemctl', 'reset-failed', "$vmid.scope"],
> %silence_std_outs) };
> eval { run_command(['/bin/systemctl', 'stop', "$vmid.scope"],
> %silence_std_outs) };
> @@ -5982,14 +5979,25 @@ sub vm_commandline {
>
> my $defaults = load_defaults();
>
> + my $running = PVE::QemuServer::Helpers::vm_running_locally($vmid);
> + my $volumes = [];
> +
> + # With -blockdev, it is necessary to activate the volumes before
> generating the command line
> + if (!$running) {
> + $volumes = get_current_vm_volumes($storecfg, $conf);
> + PVE::Storage::activate_volumes($storecfg, $volumes);
> + }
> +
> my $cmd;
> eval {
> $cmd = config_to_command($storecfg, $vmid, $conf, $defaults,
> $forcemachine, $forcecpu);
> };
> my $err = $@;
>
> - # if the vm is not running, we need to clean up the reserved/created
> devices
> - if (!PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
> + # if the vm is not running, need to clean up the reserved/created
> devices and activated volumes
> + if (!$running) {
> + eval { PVE::Storage::deactivate_volumes($storecfg, $volumes); };
I don't think we are allowed to do that here - `qm showcmd` can run
concurrently with other tasks (move disk, offline migration, ..) that
might be at a point in time where they've just activated but not yet
started using the volume..
in general, volume deactivation is tricky, and should often be avoided
unless it's 100% clear cut that it's safe to do (e.g., freshly allocated
volume while holding a lock, freeing a volume, ..) or required
(migration).
> + warn $@ if $@;
> eval { cleanup_pci_devices($vmid, $conf) };
and this here might be dangerous as well, given that we don't take a
lock and the running state might have changed already since we checked?
should `qm showcmd` take a lock? or should we avoid doing the actual PCI
reservation if we are in the "just show command" code path?
> warn $@ if $@;
> }
> @@ -6030,6 +6038,27 @@ sub get_vm_volumes {
> return $vollist;
> }
>
> +# Get volumes defined in the current VM configuration, including the VM
> state file.
> +sub get_current_vm_volumes {
> + my ($storecfg, $conf) = @_;
> +
> + my $volumes = [];
> +
> + PVE::QemuConfig->foreach_volume($conf, sub {
> + my ($ds, $drive) = @_;
> +
> + if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
> + check_volume_storage_type($storecfg, $drive->{file});
> + push $volumes->@*, $drive->{file};
> + }
> + });
> + if (my $vmstate = $conf->{vmstate}) {
> + push $volumes->@*, $vmstate;
> + }
> +
> + return $volumes;
> +}
> +
> sub cleanup_pci_devices {
> my ($vmid, $conf) = @_;
>
> diff --git a/test/run_config2command_tests.pl
> b/test/run_config2command_tests.pl
> index 7c581ad2..5217a020 100755
> --- a/test/run_config2command_tests.pl
> +++ b/test/run_config2command_tests.pl
> @@ -255,6 +255,16 @@ $qemu_server_module->mock(
> },
> );
>
> +my $storage_module = Test::MockModule->new("PVE::Storage");
> +$storage_module->mock(
> + activate_volumes => sub {
> + return;
> + },
> + deactivate_volumes => sub {
> + return;
> + },
> +);
> +
> my $zfsplugin_module = Test::MockModule->new("PVE::Storage::ZFSPlugin");
> $zfsplugin_module->mock(
> zfs_get_lu_name => sub {
> --
> 2.39.5
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
