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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id B0665A1BBA for ; Fri, 16 Jun 2023 09:50:52 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 90C662F2F4 for ; Fri, 16 Jun 2023 09:50:22 +0200 (CEST) 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 ; Fri, 16 Jun 2023 09:50:21 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4B8F745A72 for ; Fri, 16 Jun 2023 09:50:21 +0200 (CEST) Date: Fri, 16 Jun 2023 09:50:14 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230614084622.1446211-1-d.csapak@proxmox.com> <20230614084622.1446211-9-d.csapak@proxmox.com> In-Reply-To: <20230614084622.1446211-9-d.csapak@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1686900251.1zunohpk4l.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.078 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 POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH manager v6 02/15] api: add resource map api endpoints for PCI and USB 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: Fri, 16 Jun 2023 07:50:52 -0000 On June 14, 2023 10:46 am, Dominik Csapak wrote: > this adds the typical section config crud API calls for > USB and PCI resource mapping to /cluster/resource/{TYPE} >=20 > the only special thing that this series does is the list call > for both has a special 'check-node' parameter that uses the > 'proxyto_callback' to reroute the api call to the given node > so that it can check the validity of the mapping for that node >=20 > in the future when we e.g. broadcast the lspci output via pmxcfs > we drop the proxyto_callback and directly use the info from > pmxcfs (or we drop the parameter and always check all nodes) >=20 > Signed-off-by: Dominik Csapak > --- > PVE/API2/Cluster.pm | 8 + > PVE/API2/Cluster/Makefile | 5 + > PVE/API2/Cluster/Mapping.pm | 53 ++++++ > PVE/API2/Cluster/Mapping/Makefile | 18 ++ > PVE/API2/Cluster/Mapping/PCI.pm | 300 ++++++++++++++++++++++++++++++ > PVE/API2/Cluster/Mapping/USB.pm | 295 +++++++++++++++++++++++++++++ > PVE/API2/Hardware.pm | 1 - > PVE/API2/Nodes.pm | 1 + > 8 files changed, 680 insertions(+), 1 deletion(-) > create mode 100644 PVE/API2/Cluster/Mapping.pm > create mode 100644 PVE/API2/Cluster/Mapping/Makefile > create mode 100644 PVE/API2/Cluster/Mapping/PCI.pm > create mode 100644 PVE/API2/Cluster/Mapping/USB.pm [..] > diff --git a/PVE/API2/Cluster/Mapping/PCI.pm b/PVE/API2/Cluster/Mapping/P= CI.pm > new file mode 100644 > index 00000000..9fe20bea > --- /dev/null > +++ b/PVE/API2/Cluster/Mapping/PCI.pm > @@ -0,0 +1,300 @@ > +package PVE::API2::Cluster::Mapping::PCI; > + > +use strict; > +use warnings; > + > +use Storable qw(dclone); > + > +use PVE::Cluster qw(cfs_lock_file); > +use PVE::Mapping::PCI; > +use PVE::JSONSchema qw(get_standard_option); > +use PVE::Tools qw(extract_param); > + > +use PVE::RESTHandler; > + > +use base qw(PVE::RESTHandler); > + > +__PACKAGE__->register_method ({ > + name =3D> 'index', > + path =3D> '', > + method =3D> 'GET', > + # only proxy if we give the 'check-node' parameter > + proxyto_callback =3D> sub { > + my ($rpcenv, $proxyto, $param) =3D @_; > + return $param->{'check-node'} // 'localhost'; > + }, > + description =3D> "List PCI Hardware Mapping", > + permissions =3D> { > + description =3D> "Only lists entries where you have 'Mapping.Modi= fy', 'Mapping.Use' or". > + " 'Mapping.Audit' permissions on '/mapping/pci/'.", nit: the schema/parameters call it 'id', not 'name', repeated a few times below.. if I create a mapping and then query this API endpoint with pvesh, I get a wrong result: $ pvesh ls /cluster/mapping/usb --output-format json Use of uninitialized value $c in concatenation (.) or string at /usr/share/= perl5/PVE/CLI/pvesh.pm line 364. [{"capabilities":"Dr-c-","name":null}] not sure whether the issue is with pvesh or here, but could be related to > + user =3D> 'all', > + }, > + parameters =3D> { > + additionalProperties =3D> 0, > + properties =3D> { > + 'check-node' =3D> get_standard_option('pve-node', { > + description =3D> "If given, checks the configurations on = the given node for ". > + "correctness, and adds relevant errors to the devices= .", > + optional =3D> 1, > + }), > + }, > + }, > + returns =3D> { > + type =3D> 'array', > + items =3D> { > + type =3D> "object", > + properties =3D> { > + id =3D> { > + type =3D> 'string', > + description =3D> "The logical ID of the mapping." > + }, > + map =3D> { > + type =3D> 'array', > + description =3D> "The entries of the mapping.", > + items =3D> { > + type =3D> 'string', > + description =3D> "A mapping for a node.", > + }, > + }, > + description =3D> { > + type =3D> 'string', > + description =3D> "A description of the logical mappin= g.", > + }, > + error =3D> { > + description =3D> "A list of errors when 'check_node' = is given.", > + items =3D> { > + type =3D> 'object', > + properties =3D> { > + severity =3D> { > + type =3D> "string", > + description =3D> "The severity of the err= or", > + }, > + message =3D> { > + type =3D> "string", > + description =3D> "The message of the erro= r", > + }, > + }, > + } > + }, > + }, > + }, > + links =3D> [ { rel =3D> 'child', href =3D> "{name}" } ], this part here, where it tries to link children using 'name', although the return value only contains 'id' as property.. > + }, > + code =3D> sub { > + my ($param) =3D @_; > + > + my $rpcenv =3D PVE::RPCEnvironment::get(); > + my $authuser =3D $rpcenv->get_user(); > + my $node =3D $param->{'check-node'}; > + > + die "Wrong node to check\n" > + if defined($node) && $node ne 'localhost' && $node ne PVE::IN= otify::nodename(); > + > + my $cfg =3D PVE::Mapping::PCI::config(); > + > + my $res =3D []; > + > + my $privs =3D ['Mapping.Modify', 'Mapping.Use', 'Mapping.Audit']; > + > + for my $id (keys $cfg->{ids}->%*) { > + next if !$rpcenv->check_full($authuser, "/mapping/pci/$id", $= privs, 1, 1); > + next if !$cfg->{ids}->{$id}; > + > + my $entry =3D dclone($cfg->{ids}->{$id}); > + $entry->{id} =3D $id; > + $entry->{digest} =3D $cfg->{digest}; > + > + if (defined($node)) { > + $entry->{errors} =3D []; > + if (my $mappings =3D PVE::Mapping::PCI::get_node_mapping(= $cfg, $id, $node)) { > + if (!scalar($mappings->@*)) { > + push $entry->{errors}->@*, { > + severity =3D> 'warning', > + message =3D> "No mapping for node $node.", > + }; > + } > + for my $mapping ($mappings->@*) { > + eval { > + PVE::Mapping::PCI::assert_valid($id, $mapping= ); > + }; > + if (my $err =3D $@) { > + push $entry->{errors}->@*, { > + severity =3D> 'error', > + message =3D> "Invalid configuration: $err= ", > + }; > + } > + } > + } > + } > + > + push @$res, $entry; this adds the full entry to the returned value, and the permission check allows it with Mappings.* > + } > + > + return $res; > + }, > +}); > + > +__PACKAGE__->register_method ({ > + name =3D> 'get', > + protected =3D> 1, > + path =3D> '{id}', > + method =3D> 'GET', > + description =3D> "Get PCI Mapping.", > + permissions =3D> { > + check =3D>['or', > + ['perm', '/mapping/pci/{name}', ['Mapping.Use']], > + ['perm', '/mapping/pci/{name}', ['Mapping.Modify']], but this here doesn't allow Mapping.Audit? either the index call needs to return a limited view, or this should be allowed for Audit as well I think? > + ], > + }, > + parameters =3D> { > + additionalProperties =3D> 0, > + properties =3D> { > + id =3D> { > + type =3D> 'string', > + format =3D> 'pve-configid', > + }, > + } > + }, > + returns =3D> { type =3D> 'object' }, > + code =3D> sub { > + my ($param) =3D @_; > + > + my $cfg =3D PVE::Mapping::PCI::config(); > + my $id =3D $param->{id}; > + > + my $entry =3D $cfg->{ids}->{$id}; > + die "mapping '$param->{id}' not found\n" if !defined($entry); > + > + my $data =3D dclone($entry); > + > + $data->{digest} =3D $cfg->{digest}; > + > + return $data; > + }}); > + > +__PACKAGE__->register_method ({ > + name =3D> 'create', > + protected =3D> 1, > + path =3D> '', > + method =3D> 'POST', > + description =3D> "Create a new hardware mapping.", > + permissions =3D> { > + check =3D> ['perm', '/mapping/pci/{name}', ['Mapping.Modify']], we usually use the higher level path for creating (/mapping/pci) - although the priv here is 'Modify' and not 'Allocate', we are still creating a new entry ;) > + }, > + # todo parameters ? ;) > + parameters =3D> PVE::Mapping::PCI->createSchema(1), > + returns =3D> { > + type =3D> 'null', > + }, > + code =3D> sub { > + my ($param) =3D @_; > + > + my $id =3D extract_param($param, 'id'); > + > + my $plugin =3D PVE::Mapping::PCI->lookup('pci'); > + my $opts =3D $plugin->check_config($id, $param, 1, 1); > + > + PVE::Mapping::PCI::lock_pci_config(sub { > + my $cfg =3D PVE::Mapping::PCI::config(); > + > + die "pci ID '$id' already defined\n" if defined($cfg->{ids}->{$id})= ; > + > + $cfg->{ids}->{$id} =3D $opts; > + > + PVE::Mapping::PCI::write_pci_config($cfg); > + > + }, "create hardware mapping failed"); > + > + return; > + }, > +}); > + > +__PACKAGE__->register_method ({ > + name =3D> 'update', > + protected =3D> 1, > + path =3D> '{id}', > + method =3D> 'PUT', > + description =3D> "Update a hardware mapping.", > + permissions =3D> { > + check =3D> ['perm', '/mapping/pci/{id}', ['Mapping.Modify']], this one is in line with our usual scheme > + }, > + parameters =3D> PVE::Mapping::PCI->updateSchema(), > + returns =3D> { > + type =3D> 'null', > + }, > + code =3D> sub { > + my ($param) =3D @_; > + > + my $digest =3D extract_param($param, 'digest'); > + my $delete =3D extract_param($param, 'delete'); > + my $id =3D extract_param($param, 'id'); > + > + if ($delete) { > + $delete =3D [ PVE::Tools::split_list($delete) ]; > + } > + > + PVE::Mapping::PCI::lock_pci_config(sub { > + my $cfg =3D PVE::Mapping::PCI::config(); > + > + PVE::Tools::assert_if_modified($cfg->{digest}, $digest) if defined(= $digest); > + > + die "pci ID '$id' does not exist\n" if !defined($cfg->{ids}->{$id})= ; > + > + my $plugin =3D PVE::Mapping::PCI->lookup('pci'); > + my $opts =3D $plugin->check_config($id, $param, 1, 1); > + > + my $data =3D $cfg->{ids}->{$id}; > + > + my $options =3D $plugin->private()->{options}->{pci}; > + PVE::SectionConfig::delete_from_config($data, $options, $opts, $del= ete); > + > + $data->{$_} =3D $opts->{$_} for keys $opts->%*; > + > + PVE::Mapping::PCI::write_pci_config($cfg); > + > + }, "update hardware mapping failed"); > + > + return; > + }, > +}); > + > +__PACKAGE__->register_method ({ > + name =3D> 'delete', > + protected =3D> 1, > + path =3D> '{id}', > + method =3D> 'DELETE', > + description =3D> "Remove Hardware Mapping.", > + permissions =3D> { > + check =3D> [ 'perm', '/mapping/pci/{id}', ['Mapping.Modify']], this one should be changed if the create one is changed - they should match.. > + }, > + parameters =3D> { > + additionalProperties =3D> 0, > + properties =3D> { > + id =3D> { > + type =3D> 'string', > + format =3D> 'pve-configid', > + }, > + } > + }, > + returns =3D> { type =3D> 'null' }, > + code =3D> sub { > + my ($param) =3D @_; > + > + my $id =3D $param->{id}; > + > + PVE::Mapping::PCI::lock_pci_config(sub { > + my $cfg =3D PVE::Mapping::PCI::config(); > + > + if ($cfg->{ids}->{$id}) { > + delete $cfg->{ids}->{$id}; > + } > + > + PVE::Mapping::PCI::write_pci_config($cfg); > + > + }, "delete pci mapping failed"); > + > + return; > + } > +}); > + > +1; > diff --git a/PVE/API2/Cluster/Mapping/USB.pm b/PVE/API2/Cluster/Mapping/U= SB.pm and pretty much everything mentioned above applies to this one as well ;) > [..]