From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 92B391FF0B7 for ; Fri, 16 Oct 2026 14:27:14 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 579392164F; Fri, 16 Oct 2026 14:26:54 +0200 (CEST) From: Erik Fastermann To: pve-devel@lists.proxmox.com Subject: [PATCH qemu-server v3 3/6] api: report why drive options are rejected Date: Fri, 16 Oct 2026 14:25:59 +0200 Message-ID: <20261016122602.183911-4-e.fastermann@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261016122602.183911-1-e.fastermann@proxmox.com> References: <20261016122602.183911-1-e.fastermann@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792153609071 X-SPAM-LEVEL: Spam detection results: 0 AWL 1.636 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: GTAHIUT3JNKZ5M6JDOGVNLYAMDPA2MVV X-Message-ID-Hash: GTAHIUT3JNKZ5M6JDOGVNLYAMDPA2MVV X-MailFrom: e.fastermann@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Erik Fastermann X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: parse_drive only warned about invalid combinations of drive options and then returned undef. During an API call these warnings end up in the journal, so the web UI only showed the generic "unable to parse drive options" error without any reason. Collect the problems in parse_drive and hand them to callers that ask for them, while all other callers keep getting warnings as before. The drive parameter check for VM creation and config updates now uses this, so the error states which options conflict. Checks that previously failed without any message now report a reason as well. This covers conflicting total and read/write limits, invalid CD-ROM options, invalid sizes and errors in the drive string itself. For callers that do not ask for the errors, these are now logged as warnings instead of being silently ignored. Signed-off-by: Erik Fastermann --- src/PVE/API2/Qemu.pm | 9 +++-- src/PVE/QemuServer/Drive.pm | 66 ++++++++++++++++++++++++++----------- 2 files changed, 54 insertions(+), 21 deletions(-) diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm index cfbc1f51..740aec67 100644 --- a/src/PVE/API2/Qemu.pm +++ b/src/PVE/API2/Qemu.pm @@ -113,8 +113,13 @@ my $check_drive_param = sub { for my $opt (sort keys $param->%*) { next if !PVE::QemuServer::is_valid_drivename($opt); - my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1); - raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive; + my @drive_errors; + my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1, \@drive_errors); + if (!$drive) { + my $msg = 'unable to parse drive options'; + $msg .= ': ' . join('; ', @drive_errors) if @drive_errors; + raise_param_exc({ $opt => $msg }); + } if ($drive->{'import-from'}) { if ($drive->{file} !~ $PVE::QemuServer::Drive::NEW_DISK_RE || $3 != 0) { diff --git a/src/PVE/QemuServer/Drive.pm b/src/PVE/QemuServer/Drive.pm index bccf0371..9af87ac7 100644 --- a/src/PVE/QemuServer/Drive.pm +++ b/src/PVE/QemuServer/Drive.pm @@ -843,16 +843,20 @@ sub parse_drive_interface { # [,aio=native|threads][,discard=ignore|on][,detect_zeroes=on|off] # [,iothread=on][,serial=serial][,model=model] -sub parse_drive { - my ($key, $data, $with_alloc) = @_; +my sub parse_drive_append_errors { + my ($key, $data, $with_alloc, $errors) = @_; my ($interface, $index) = eval { parse_drive_interface($key) }; - return if $@; + if (my $err = $@) { + chomp $err; + push @$errors, $err; + return; + } my $desc_hash = $with_alloc ? $drivedesc_hash_with_alloc : $drivedesc_hash; if (!defined($desc_hash->{$key})) { - warn "invalid drive key: $key\n"; + push @$errors, "invalid drive key: $key"; return; } @@ -861,16 +865,18 @@ sub parse_drive { my $pps_opts = { skip => $DROPPED_PROPERTIES }; PVE::JSONSchema::parse_property_string($desc, $data, undef, undef, $pps_opts); }; - return if !$res; + if (my $err = $@) { + chomp $err; + push @$errors, $err; + return; + } $res->{interface} = $interface; $res->{index} = $index; - my $error = 0; foreach my $opt (qw(bps bps_rd bps_wr)) { if (defined(my $bps = delete $res->{$opt})) { if (defined($res->{"m$opt"})) { - warn "both $opt and m$opt specified\n"; - ++$error; + push @$errors, "both $opt and m$opt specified"; next; } $res->{"m$opt"} = sprintf("%.3f", $bps / (1024 * 1024.0)); @@ -894,25 +900,47 @@ sub parse_drive { ) { my ($option, $requires) = @$requirement; if ($res->{$option} && !$res->{$requires}) { - warn "$option requires $requires\n"; - ++$error; + push @$errors, "$option requires $requires"; } } - return if $error; - - return if $res->{mbps_rd} && $res->{mbps}; - return if $res->{mbps_wr} && $res->{mbps}; - return if $res->{iops_rd} && $res->{iops}; - return if $res->{iops_wr} && $res->{iops}; + for my $mutually_exclusive_option ( + ['mbps', 'mbps_rd'], + ['mbps', 'mbps_wr'], + ['iops', 'iops_rd'], + ['iops', 'iops_wr'], + ) { + my ($total, $directional) = @$mutually_exclusive_option; + push @$errors, "both $total and $directional specified" + if $res->{$total} && $res->{$directional}; + } if ($res->{media} && ($res->{media} eq 'cdrom')) { - return if $res->{snapshot} || $res->{format}; - return if $res->{interface} eq 'virtio'; + push @$errors, "CD-ROM drives do not support 'snapshot' or 'format'" + if $res->{snapshot} || $res->{format}; + push @$errors, "CD-ROM drives are not supported on VirtIO" + if $res->{interface} eq 'virtio'; } if (my $size = $res->{size}) { - return if !defined($res->{size} = PVE::JSONSchema::parse_size($size)); + push @$errors, "invalid size '$size'" + if !defined($res->{size} = PVE::JSONSchema::parse_size($size)); + } + + return if @$errors; + return $res; +} + +sub parse_drive { + my ($key, $data, $with_alloc, $errors) = @_; + + my @local_errors; + my $res = parse_drive_append_errors($key, $data, $with_alloc, \@local_errors); + + if ($errors) { + push @$errors, @local_errors; + } else { + warn "$_\n" for @local_errors; } return $res; -- 2.47.3