Re: [pve-devel] [PATCH manager 1/1] api/ui: improve mdev listing for pci mappings

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

On 10/24/24 18:53, Thomas Lamprecht wrote:
> Am 06/08/2024 um 14:22 schrieb Dominik Csapak:
>> currently when we have a pci resource mapping, we manually check only
>> the available models for the first pci entry. This often works, but not
>> always, since one could have completely different devices in one
>> mapping, or with the new nvidia sysfs api we don't get the generally
>> available models.
>>
>> To improve this, extend the 'pciid' regex to include pciids or mapping
>> names, and for mappings, iterate over all local pci devices in it and
>> extract the mdev types.
>>
>> This also vastly simplifies the ui code, since we only have to give the
>> mapping to the selector instead of an (arbitrarily selected) pci id.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/API2/Hardware/PCI.pm     | 45 +++++++++++++++++++++++++++++-------
>>   www/manager6/qemu/PCIEdit.js | 12 +---------
>>   2 files changed, 38 insertions(+), 19 deletions(-)
>>
>> diff --git a/PVE/API2/Hardware/PCI.pm b/PVE/API2/Hardware/PCI.pm
>> index a3a689bf..7135a605 100644
>> --- a/PVE/API2/Hardware/PCI.pm
>> +++ b/PVE/API2/Hardware/PCI.pm
>> @@ -135,7 +135,7 @@ __PACKAGE__->register_method ({
>>   
>>   __PACKAGE__->register_method ({
>>       name => 'pciindex',
>> -    path => '{pciid}',
>> +    path => '{pciid-or-mapping}',
>>       method => 'GET',
>>       description => "Index of available pci methods",
>>       permissions => {
>> @@ -145,9 +145,9 @@ __PACKAGE__->register_method ({
>>   	additionalProperties => 0,
>>   	properties => {
>>   	    node => get_standard_option('pve-node'),
>> -	    pciid => {
>> +	    'pciid-or-mapping' => {
> 
> While in general this should be fine w.r.t. breaking changes for the API itself,
> as the name of those template-variables from the API path can be basically seen
> as internal detail, it would IMO be warranted to state that explicitly in the
> commit message, as FWICT this is basically the only time where changing the
> parameter name mid-release is legit without some backward compat handling.


Yeah, you're right, i'll send a v2 of this with the commit message extended,
but see below for an alternative.

> 
> And just to be sure: you are certain that this cannot be passed anyhow else,
> e.g. in some CLI (pvesh?) or other way?

I did not find a way, and AFAICT this parameter is always part of the path,
where we don't can give explicit names

Of course, we can leave the name of the parameter the same if you'd
prefer that (with a TODO notice for 9.x?), and just change the regex, then
it's definitely backwards compatible, even if i overlooked some case
where one can give the parameter name


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel