Re: [pve-devel] [PATCH v2 guest-common 1/1] Add foreach_passthrough_device

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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 <f.schauer@proxmox.com>
> ---
>  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