From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 8B69A9E0EE for ; Mon, 30 Oct 2023 14:12:11 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7605FF6A7 for ; Mon, 30 Oct 2023 14:12:11 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 30 Oct 2023 14:12:10 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 89FBC4248E for ; Mon, 30 Oct 2023 14:12:10 +0100 (CET) Date: Mon, 30 Oct 2023 14:12:09 +0100 From: Wolfgang Bumiller To: Filip Schauer Cc: pve-devel@lists.proxmox.com Message-ID: References: <20231024125554.131800-1-f.schauer@proxmox.com> <20231024125554.131800-3-f.schauer@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231024125554.131800-3-f.schauer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.098 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v2 guest-common 1/1] Add foreach_passthrough_device X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Oct 2023 13:12:11 -0000 On Tue, Oct 24, 2023 at 02:55:54PM +0200, Filip Schauer wrote: > Add a function to iterate over passthrough devices of a provided > container config. As container specific code this should be in pve-container. > > Signed-off-by: Filip Schauer > --- > src/PVE/AbstractConfig.pm | 44 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm > index a286b13..ed26e91 100644 > --- a/src/PVE/AbstractConfig.pm > +++ b/src/PVE/AbstractConfig.pm > @@ -484,6 +484,50 @@ sub foreach_volume { > return $class->foreach_volume_full($conf, undef, $func, @param); > } > > +sub foreach_passthrough_device { > + my ($class, $conf, $func, @param) = @_; > + > + foreach my $key (keys %$conf) { > + next if $key !~ m/^dev(\d+)$/; > + > + my $device = $class->parse_device($conf->{$key}); (`parse_device` does not exist in AbstractConfig) > + > + if (defined($device->{path})) { > + die "Device path $device->{path} does not start with /dev/\n" > + if $device->{path} !~ m!^/dev/!; IMO `parse_device()` should be responsible for the above check, and `verify_lxc_dev_string()` already does this, so the above check can just be dropped. > + > + my $sanitized_path = substr($conf->{$key}, 1); > + die "Device /$sanitized_path does not exist\n" unless (-e "/$sanitized_path"); Since we now have a property string and `parse_device()`, it would make more sense to pass `$device` rather than this path. In fact, thinking about this more I'm not sure passing the substring through was the best idea, since it's rather specific to how it'll end up in the lxc config. That's my bad. > + > + $func->($key, $sanitized_path, @param); > + } elsif (defined($device->{usbmapping})) { > + my $mapping = $device->{usbmapping}; > + my $map_devices = PVE::Mapping::USB::find_on_current_node($mapping); > + > + die "USB device mapping not found for '$mapping'\n" > + if !$map_devices || !scalar($map_devices->@*); > + > + die "More than one USB mapping per host not supported\n" > + if scalar($map_devices->@*) > 1; > + > + eval { > + PVE::Mapping::USB::assert_valid($mapping, $map_devices->[0]); > + }; > + if (my $err = $@) { > + die "USB Mapping invalid (hardware probably changed): $err\n"; > + } > + > + my $map_device = $map_devices->[0]; > + my $lsusb_output = `/usr/bin/lsusb -d $map_device->{id}`; > + my ($bus_id, $device_id) = $lsusb_output =~ /Bus (\d+) Device (\d+)/; ^ qx/backticks should be avoided, it'll be interpreted by a shell Also, we don't need this :-) See `PVE::SysFSTools::scan_usb()` which does essentially the same thing without invoking an external binary. > + > + $func->($key, "dev/bus/usb/$bus_id/$device_id", @param); So this will do... something :-) But unfortunately it won't deal with the *interesting* bits. So while I do like the idea of having such mappings, for it to make sense we'd also need to figure out the device specific nodes. Eg. for input devices we'd want to include `/dev/input/eventXY`, for eg. FIDO keys we'd want `/dev/hidraw*` devices. I'm not sure how much work we should put into this in the initial implementation, the question is mainly whether we want to do it like *this* in the first place and how much we plan to support. Or maybe it would make more sense to have specific entries for hidraw, event devices, block devices, ... instead? I'm not sure. > + } else { > + die "Either path or usbmapping has to be defined for $key"; > + } > + } > +} > + > # $volume_map is a hash of 'old_volid' => 'new_volid' pairs. > # This method replaces 'old_volid' by 'new_volid' throughout the config including snapshots, pending > # changes, unused volumes and vmstate volumes. > -- > 2.39.2