Re: [pve-devel] [PATCH v4 container 1/1] Add device passthrough

[Filip Schauer <f.schauer@proxmox.com>]

Patch v5 available

https://lists.proxmox.com/pipermail/pve-devel/2023-November/060285.html

On 16/11/2023 09:32, Wolfgang Bumiller wrote:
> On Wed, Nov 15, 2023 at 03:14:50PM +0100, Thomas Lamprecht wrote:
>> concept wise this looks pretty much OK, but a few (mostly code-style) comments in line
>>
>> Am 13/11/2023 um 11:30 schrieb Filip Schauer:
>>> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
>>> index 56e1f10..9f325f2 100644
>>> --- a/src/PVE/LXC/Config.pm
>>> +++ b/src/PVE/LXC/Config.pm
>>> @@ -29,6 +29,7 @@ mkdir $lockdir;
>>>   mkdir "/etc/pve/nodes/$nodename/lxc";
>>>   my $MAX_MOUNT_POINTS = 256;
>>>   my $MAX_UNUSED_DISKS = $MAX_MOUNT_POINTS;
>>> +my $MAX_DEVICES = 256;
>>>   
>>>   # BEGIN implemented abstract methods from PVE::AbstractConfig
>>>   
>>> @@ -908,6 +909,71 @@ for (my $i = 0; $i < $MAX_UNUSED_DISKS; $i++) {
>>>       }
>>>   }
>>>   
>>> +PVE::JSONSchema::register_format('pve-lxc-dev-string', \&verify_lxc_dev_string);
>>> +sub verify_lxc_dev_string {
>>> +    my ($dev, $noerr) = @_;
>>> +
>>> +    if (
>>> +	$dev =~ m@/\.\.?/@ ||
>>> +	$dev =~ m@/\.\.?$@ ||
>> could be a single regex:
>>
>> $dev =~ @/\.\.?(?:/|$)@
>>
>> but no hard feelings, all variant are not easily readable and need close
>> checking anyway (iow. like most regexes)
>>
>>> +	$dev !~ m!^/dev/!
>>> +    ) {
>>> +	return undef if $noerr;
>>> +	die "$dev is not a valid device path\n";
>>> +    }
>>> +
>>> +    return $dev;
>>> +}
>>> +
>>> +PVE::JSONSchema::register_format('file-access-mode-string', \&verify_file_access_mode);
>>> +sub verify_file_access_mode {
>>> +    my ($mode, $noerr) = @_;
>>> +
>>> +    if ($mode !~ /^[0-7]*$/) {
>> this would allow an empty mode though? Also, not sure if we want to allow
>> partial modes like 77 ?
> Yeah, an empty mode should not be allowed. Not sure what you mean by
> partial though, other than the missing leading zero.
>
> For octal we should definitely enforce the leading zero.
>
>>> +	return undef if $noerr;
>>> +	die "$mode is not a valid file access mode\n";
>>> +    }
>>> +
>>> +    return $mode;
>>> +}
>>> +
>>> +my $dev_desc = {
>>> +    path => {
>>> +	optional => 1,
>>> +	type => 'string',
>>> +	default_key => 1,
>>> +	format => 'pve-lxc-dev-string',
>>> +	format_description => 'Path',
>>> +	description => 'Device to pass through to the container',
>>> +	verbose_description => 'Path to the device to pass through to the container',
>>> +    },
>>> +    mode => {
>>> +	optional => 1,
>>> +	type => 'integer',
>>> +	format => 'file-access-mode-string',
> ... this should be `type => 'string'` (integer just doesn't enforce a
> format),
> the `format` should be dropped (including the registered sub above), and
> instead use 'pattern' as:
>
>      pattern => '0[0-7]*',
>
> JSONSchema anchors the pattern, so no need to include '^' and '$',
> although I also wouldn't mind using
>
>      pattern => qr/^0[0-7]*$/,
>
>>> +	format_description => 'Octal access mode',
>>> +	description => 'Access mode to be set on the device node',
>>> +    },
>>> +    uid => {
>>> +	optional => 1,
>>> +	type => 'integer',
>>> +	description => 'User ID to be assigned to the device node',
>>> +    },
>>> +    gid => {
>>> +	optional => 1,
>>> +	type => 'integer',
>>> +	description => 'Group ID to be assigned to the device node',
> Add `minimum => 0`, to both uid and gid.