[pve-devel] [PATCH qemu-server v7 1/7] usb: refactor usb code and move some into USB module

[Dominik Csapak <d.csapak@proxmox.com>]

similar to how we handle the PCI module and format. This makes the
'verify_usb_device' method and format unnecessary since
we simply check the format with a regex.

while doing tihs, i noticed that we don't correctly check for the
case-insensitive variant for 'spice' during hotplug, so fix that too

With this we can also remove some parameters from the get_usb_devices
and get_usb_controllers functions

while were at it, refactor the permission checks for the usb config too
and use the new 'my sub' style for the functions

also make print_usbdevice_full parse the device itself, so we don't have
to do it in multiple places (especially in places where we don't see
that this is needed)

No functional change intended

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v7
 PVE/API2/Qemu.pm      | 40 ++++++++++--------
 PVE/QemuServer.pm     | 72 ++++---------------------------
 PVE/QemuServer/USB.pm | 98 +++++++++++++++++++++++++++++++------------
 3 files changed, 103 insertions(+), 107 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index c92734a6..c6a4cd2a 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -583,19 +583,29 @@ my $check_vm_create_serial_perm = sub {
     return 1;
 };
 
-my $check_vm_create_usb_perm = sub {
+my sub check_usb_perm {
+    my ($rpcenv, $authuser, $vmid, $pool, $opt, $value) = @_;
+
+    return 1 if $authuser eq 'root@pam';
+
+    my $device = PVE::JSONSchema::parse_property_string('pve-qm-usb', $value);
+    if ($device->{host} =~ m/^spice$/i) {
+	$rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
+    } else {
+	die "only root can set '$opt' config for real devices\n";
+    }
+
+    return 1;
+}
+
+my sub check_vm_create_usb_perm {
     my ($rpcenv, $authuser, $vmid, $pool, $param) = @_;
 
     return 1 if $authuser eq 'root@pam';
 
     foreach my $opt (keys %{$param}) {
 	next if $opt !~ m/^usb\d+$/;
-
-	if ($param->{$opt} =~ m/spice/) {
-	    $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.HWType']);
-	} else {
-	    die "only root can set '$opt' config for real devices\n";
-	}
+	check_usb_perm($rpcenv, $authuser, $vmid, $pool, $opt, $param->{$opt});
     }
 
     return 1;
@@ -878,7 +888,8 @@ __PACKAGE__->register_method({
 	    &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ keys %$param]);
 
 	    &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param);
-	    &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param);
+	    check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param);
+
 	    PVE::QemuServer::check_bridge_access($rpcenv, $authuser, $param);
 	    &$check_cpu_model_access($rpcenv, $authuser, $param);
 
@@ -1719,11 +1730,7 @@ my $update_vm_api  = sub {
 		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
 		    PVE::QemuConfig->write_config($vmid, $conf);
 		} elsif ($opt =~ m/^usb\d+$/) {
-		    if ($val =~ m/spice/) {
-			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
-		    } elsif ($authuser ne 'root@pam') {
-			die "only root can delete '$opt' config for real devices\n";
-		    }
+		    check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $val);
 		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
 		    PVE::QemuConfig->write_config($vmid, $conf);
 		} elsif ($opt eq 'tags') {
@@ -1784,11 +1791,10 @@ my $update_vm_api  = sub {
 		    }
 		    $conf->{pending}->{$opt} = $param->{$opt};
 		} elsif ($opt =~ m/^usb\d+/) {
-		    if ((!defined($conf->{$opt}) || $conf->{$opt} =~ m/spice/) && $param->{$opt} =~ m/spice/) {
-			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.HWType']);
-		    } elsif ($authuser ne 'root@pam') {
-			die "only root can modify '$opt' config for real devices\n";
+		    if (my $olddevice = $conf->{$opt}) {
+			check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $conf->{$opt});
 		    }
+		    check_usb_perm($rpcenv, $authuser, $vmid, undef, $opt, $param->{$opt});
 		    $conf->{pending}->{$opt} = $param->{$opt};
 		} elsif ($opt eq 'tags') {
 		    assert_tag_permissions($vmid, $conf->{$opt}, $param->{$opt}, $rpcenv, $authuser);
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 6cbaf878..38200fea 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -56,7 +56,7 @@ use PVE::QemuServer::Machine;
 use PVE::QemuServer::Memory;
 use PVE::QemuServer::Monitor qw(mon_cmd);
 use PVE::QemuServer::PCI qw(print_pci_addr print_pcie_addr print_pcie_root_port parse_hostpci);
-use PVE::QemuServer::USB qw(parse_usb_device);
+use PVE::QemuServer::USB;
 
 my $have_sdn;
 eval {
@@ -835,7 +835,6 @@ while (my ($k, $v) = each %$confdesc) {
     PVE::JSONSchema::register_standard_option("pve-qm-$k", $v);
 }
 
-my $MAX_USB_DEVICES = 14;
 my $MAX_NETS = 32;
 my $MAX_SERIAL_PORTS = 4;
 my $MAX_PARALLEL_PORTS = 3;
@@ -1081,44 +1080,6 @@ sub verify_volume_id_or_absolute_path {
     return $volid;
 }
 
-my $usb_fmt = {
-    host => {
-	default_key => 1,
-	type => 'string', format => 'pve-qm-usb-device',
-	format_description => 'HOSTUSBDEVICE|spice',
-        description => <<EODESCR,
-The Host USB device or port or the value 'spice'. HOSTUSBDEVICE syntax is:
-
- 'bus-port(.port)*' (decimal numbers) or
- 'vendor_id:product_id' (hexadeciaml numbers) or
- 'spice'
-
-You can use the 'lsusb -t' command to list existing usb devices.
-
-NOTE: This option allows direct access to host hardware. So it is no longer possible to migrate such
-machines - use with special care.
-
-The value 'spice' can be used to add a usb redirection devices for spice.
-EODESCR
-    },
-    usb3 => {
-	optional => 1,
-	type => 'boolean',
-	description => "Specifies whether if given host option is a USB3 device or port."
-	    ." For modern guests (machine version >= 7.1 and ostype l26 and windows > 7), this flag"
-	    ." is irrelevant (all devices are plugged into a xhci controller).",
-        default => 0,
-    },
-};
-
-my $usbdesc = {
-    optional => 1,
-    type => 'string', format => $usb_fmt,
-    description => "Configure an USB device (n is 0 to 4, for machine version >= 7.1 and ostype"
-	." l26 or windows > 7, n can be up to 14).",
-};
-PVE::JSONSchema::register_standard_option("pve-qm-usb", $usbdesc);
-
 my $serialdesc = {
 	optional => 1,
 	type => 'string',
@@ -1167,8 +1128,8 @@ for my $key (keys %{$PVE::QemuServer::Drive::drivedesc_hash}) {
     $confdesc->{$key} = $PVE::QemuServer::Drive::drivedesc_hash->{$key};
 }
 
-for (my $i = 0; $i < $MAX_USB_DEVICES; $i++)  {
-    $confdesc->{"usb$i"} = $usbdesc;
+for (my $i = 0; $i < $PVE::QemuServer::USB::MAX_USB_DEVICES; $i++)  {
+    $confdesc->{"usb$i"} = $PVE::QemuServer::USB::usbdesc;
 }
 
 my $boot_fmt = {
@@ -2244,17 +2205,6 @@ sub qemu_created_version_fixups {
     return;
 }
 
-PVE::JSONSchema::register_format('pve-qm-usb-device', \&verify_usb_device);
-sub verify_usb_device {
-    my ($value, $noerr) = @_;
-
-    return $value if parse_usb_device($value);
-
-    return if $noerr;
-
-    die "unable to parse usb device\n";
-}
-
 # add JSON properties for create and set function
 sub json_config_properties {
     my ($prop, $with_disk_alloc) = @_;
@@ -3740,7 +3690,7 @@ sub config_to_command {
 
     # add usb controllers
     my @usbcontrollers = PVE::QemuServer::USB::get_usb_controllers(
-	$conf, $bridges, $arch, $machine_type, $usbdesc->{format}, $MAX_USB_DEVICES, $machine_version);
+	$conf, $bridges, $arch, $machine_type, $machine_version);
     push @$devices, @usbcontrollers if @usbcontrollers;
     my $vga = parse_vga($conf->{vga});
 
@@ -3782,7 +3732,7 @@ sub config_to_command {
     $usb_dev_features->{spice_usb3} = 1 if min_version($machine_version, 4, 0);
 
     my @usbdevices = PVE::QemuServer::USB::get_usb_devices(
-	$conf, $usbdesc->{format}, $MAX_USB_DEVICES, $usb_dev_features, $bootorder, $machine_version);
+	$conf, $usb_dev_features, $bootorder, $machine_version);
     push @$devices, @usbdevices if @usbdevices;
 
     # serial devices
@@ -4653,12 +4603,8 @@ sub qemu_usb_hotplug {
 	qemu_deviceadd($vmid, PVE::QemuServer::USB::print_qemu_xhci_controller($pciaddr));
     }
 
-    # print_usbdevice_full expects the parsed device
-    my $d = parse_usb_device($device->{host});
-    $d->{usb3} = $device->{usb3};
-
     # add the new one
-    vm_deviceplug($storecfg, $conf, $vmid, $deviceid, $d, $arch, $machine_type);
+    vm_deviceplug($storecfg, $conf, $vmid, $deviceid, $device, $arch, $machine_type);
 }
 
 sub qemu_cpu_hotplug {
@@ -5120,9 +5066,9 @@ sub vmconfig_hotplug_pending {
 	    } elsif ($opt =~ m/^usb(\d+)$/) {
 		my $index = $1;
 		die "skip\n" if !$usb_hotplug;
-		my $d = eval { parse_property_string($usbdesc->{format}, $value) };
+		my $d = eval { parse_property_string('pve-qm-usb', $value) };
 		my $id = $opt;
-		if ($d->{host} eq 'spice')  {
+		if ($d->{host} =~ m/^spice$/i)  {
 		    $id = "usbredirdev$index";
 		}
 		qemu_usb_hotplug($storecfg, $conf, $vmid, $id, $d, $arch, $machine_type);
@@ -5199,7 +5145,7 @@ sub vmconfig_hotplug_pending {
     # unplug xhci controller if no usb device is left
     if ($usb_hotplug) {
 	my $has_usb = 0;
-	for (my $i = 0; $i < $MAX_USB_DEVICES; $i++) {
+	for (my $i = 0; $i < $PVE::QemuServer::USB::MAX_USB_DEVICES; $i++) {
 	    next if !defined($conf->{"usb$i"});
 	    $has_usb = 1;
 	    last;
diff --git a/PVE/QemuServer/USB.pm b/PVE/QemuServer/USB.pm
index 686461cc..fe541c1f 100644
--- a/PVE/QemuServer/USB.pm
+++ b/PVE/QemuServer/USB.pm
@@ -15,6 +15,52 @@ get_usb_devices
 );
 
 my $OLD_MAX_USB = 5;
+our $MAX_USB_DEVICES = 14;
+
+
+my $USB_ID_RE = qr/(0x)?([0-9A-Fa-f]{4}):(0x)?([0-9A-Fa-f]{4})/;
+my $USB_PATH_RE = qr/(\d+)\-(\d+(\.\d+)*)/;
+
+my $usb_fmt = {
+    host => {
+	default_key => 1,
+	type => 'string',
+	pattern => qr/(?:(?:$USB_ID_RE)|(?:$USB_PATH_RE)|[Ss][Pp][Ii][Cc][Ee])/,
+	format_description => 'HOSTUSBDEVICE|spice',
+        description => <<EODESCR,
+The Host USB device or port or the value 'spice'. HOSTUSBDEVICE syntax is:
+
+ 'bus-port(.port)*' (decimal numbers) or
+ 'vendor_id:product_id' (hexadeciaml numbers) or
+ 'spice'
+
+You can use the 'lsusb -t' command to list existing usb devices.
+
+NOTE: This option allows direct access to host hardware. So it is no longer possible to migrate such
+machines - use with special care.
+
+The value 'spice' can be used to add a usb redirection devices for spice.
+EODESCR
+    },
+    usb3 => {
+	optional => 1,
+	type => 'boolean',
+	description => "Specifies whether if given host option is a USB3 device or port."
+	    ." For modern guests (machine version >= 7.1 and ostype l26 and windows > 7), this flag"
+	    ." is irrelevant (all devices are plugged into a xhci controller).",
+        default => 0,
+    },
+};
+
+PVE::JSONSchema::register_format('pve-qm-usb', $usb_fmt);
+
+our $usbdesc = {
+    optional => 1,
+    type => 'string', format => $usb_fmt,
+    description => "Configure an USB device (n is 0 to 4, for machine version >= 7.1 and ostype"
+	." l26 or windows > 7, n can be up to 14).",
+};
+PVE::JSONSchema::register_standard_option("pve-qm-usb", $usbdesc);
 
 sub parse_usb_device {
     my ($value) = @_;
@@ -22,10 +68,10 @@ sub parse_usb_device {
     return if !$value;
 
     my $res = {};
-    if ($value =~ m/^(0x)?([0-9A-Fa-f]{4}):(0x)?([0-9A-Fa-f]{4})$/) {
+    if ($value =~ m/^$USB_ID_RE$/) {
 	$res->{vendorid} = $2;
 	$res->{productid} = $4;
-    } elsif ($value =~ m/^(\d+)\-(\d+(\.\d+)*)$/) {
+    } elsif ($value =~ m/^$USB_PATH_RE$/) {
 	$res->{hostbus} = $1;
 	$res->{hostport} = $2;
     } elsif ($value =~ m/^spice$/i) {
@@ -47,7 +93,7 @@ my sub assert_usb_index_is_useable {
 }
 
 sub get_usb_controllers {
-    my ($conf, $bridges, $arch, $machine, $format, $max_usb_devices, $machine_version) = @_;
+    my ($conf, $bridges, $arch, $machine, $machine_version) = @_;
 
     my $devices = [];
     my $pciaddr = "";
@@ -68,10 +114,10 @@ sub get_usb_controllers {
 
     my ($use_usb2, $use_usb3) = 0;
     my $any_usb = 0;
-    for (my $i = 0; $i < $max_usb_devices; $i++)  {
+    for (my $i = 0; $i < $MAX_USB_DEVICES; $i++)  {
 	next if !$conf->{"usb$i"};
 	assert_usb_index_is_useable($i, $use_qemu_xhci);
-	my $d = eval { PVE::JSONSchema::parse_property_string($format,$conf->{"usb$i"}) } or next;
+	my $d = eval { PVE::JSONSchema::parse_property_string($usb_fmt, $conf->{"usb$i"}) } or next;
 	$any_usb = 1;
 	$use_usb3 = 1 if $d->{usb3};
 	$use_usb2 = 1 if !$d->{usb3};
@@ -93,7 +139,7 @@ sub get_usb_controllers {
 }
 
 sub get_usb_devices {
-    my ($conf, $format, $max_usb_devices, $features, $bootorder, $machine_version) = @_;
+    my ($conf, $features, $bootorder, $machine_version) = @_;
 
     my $devices = [];
 
@@ -101,30 +147,26 @@ sub get_usb_devices {
     my $use_qemu_xhci = min_version($machine_version, 7, 1)
 	&& defined($ostype) && ($ostype eq 'l26' || windows_version($ostype) > 7);
 
-    for (my $i = 0; $i < $max_usb_devices; $i++)  {
+    for (my $i = 0; $i < $MAX_USB_DEVICES; $i++)  {
 	my $devname = "usb$i";
 	next if !$conf->{$devname};
 	assert_usb_index_is_useable($i, $use_qemu_xhci);
-	my $d = eval { PVE::JSONSchema::parse_property_string($format,$conf->{$devname}) };
+	my $d = eval { PVE::JSONSchema::parse_property_string($usb_fmt, $conf->{$devname}) };
 	next if !$d;
 
 	my $port = $use_qemu_xhci ? $i + 1 : undef;
 
-	if (defined($d->{host})) {
-	    my $hostdevice = parse_usb_device($d->{host});
-	    $hostdevice->{usb3} = $d->{usb3};
-	    if ($hostdevice->{spice}) {
-		# usb redir support for spice
-		my $bus = 'ehci';
-		$bus = 'xhci' if ($hostdevice->{usb3} && $features->{spice_usb3}) || $use_qemu_xhci;
-
-		push @$devices, '-chardev', "spicevmc,id=usbredirchardev$i,name=usbredir";
-		push @$devices, '-device', print_spice_usbdevice($i, $bus, $port);
-
-		warn "warning: spice usb port set as bootdevice, ignoring\n" if $bootorder->{$devname};
-	    } else {
-		push @$devices, '-device', print_usbdevice_full($conf, $devname, $hostdevice, $bootorder, $port);
-	    }
+	if ($d->{host} && $d->{host} =~ m/^spice$/) {
+	    # usb redir support for spice
+	    my $bus = 'ehci';
+	    $bus = 'xhci' if ($d->{usb3} && $features->{spice_usb3}) || $use_qemu_xhci;
+
+	    push @$devices, '-chardev', "spicevmc,id=usbredirchardev$i,name=usbredir";
+	    push @$devices, '-device', print_spice_usbdevice($i, $bus, $port);
+
+	    warn "warning: spice usb port set as bootdevice, ignoring\n" if $bootorder->{$devname};
+	} else {
+	    push @$devices, '-device', print_usbdevice_full($conf, $devname, $d, $bootorder, $port);
 	}
     }
 
@@ -157,10 +199,12 @@ sub print_usbdevice_full {
 	$usbdevice .= ",port=$port" if defined($port);
     }
 
-    if (defined($device->{vendorid}) && defined($device->{productid})) {
-	$usbdevice .= ",vendorid=0x$device->{vendorid},productid=0x$device->{productid}";
-    } elsif (defined($device->{hostbus}) && defined($device->{hostport})) {
-	$usbdevice .= ",hostbus=$device->{hostbus},hostport=$device->{hostport}";
+    my $parsed = parse_usb_device($device->{host});
+
+    if (defined($parsed->{vendorid}) && defined($parsed->{productid})) {
+	$usbdevice .= ",vendorid=0x$parsed->{vendorid},productid=0x$parsed->{productid}";
+    } elsif (defined($parsed->{hostbus}) && defined($parsed->{hostport})) {
+	$usbdevice .= ",hostbus=$parsed->{hostbus},hostport=$parsed->{hostport}";
     } else {
 	die "no usb id or path given\n";
     }
-- 
2.30.2