[PATCH qemu-server v3 3/6] api: report why drive options are rejected

[Erik Fastermann <e.fastermann@proxmox.com>]

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 <e.fastermann@proxmox.com>
---
 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