Re: [pve-devel] [PATCH qemu-server v3 04/13] PCI: reuse parsed info from print_hostpci_devices

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 20/09/2022 um 14:50 schrieb Dominik Csapak:
> instead of parsing the config again when trying to reserver/prepare the
> pci devices. also split the preparing into non-mdev devices and mdev
> devices, this will come in handy later.

I'd rather factor out the parsing parts, see below.
 

> diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
> index 7406246..b5284ef 100644
> --- a/PVE/QemuServer/PCI.pm
> +++ b/PVE/QemuServer/PCI.pm
> @@ -422,12 +422,17 @@ sub print_hostpci_devices {
>      my $kvm_off = 0;
>      my $gpu_passthrough = 0;
>      my $legacy_igd = 0;
> +    my $parsed_devices = {};
>  
>      my $pciaddr;
>      for (my $i = 0; $i < $MAX_HOSTPCI_DEVICES; $i++)  {
>  	my $id = "hostpci$i";
>  	my $d = parse_hostpci($conf->{$id});
>  	next if !$d;
> +	$parsed_devices->{$i} = {
> +	    device => $d,
> +	    used => [],
> +	};

note that iff we'd do that I'd want a comment on the sub describing the parsed_device
"struct" and its field; if it sounds like I'm really missing rust here you'd be spot
on..

>  
>  	if (my $pcie = $d->{pcie}) {
>  	    die "q35 machine model is not enabled" if !$q35;
> @@ -449,6 +454,7 @@ sub print_hostpci_devices {
>  	}
>  
>  	my $pcidevices = $d->{pciid};
> +	$parsed_devices->{$i}->{used} = $pcidevices;

so, this is already included in $parsed_devices->{$i} which contains the full $d
anyway, what gives?

How about splitting out the looping over all possible "hostpci$i" with the checks
and transformations done with the parsed $d into an parse_hostpci_devices returning
only the hash and pass then that to print_hostpci_devices (which then can probably
drop some parameters), that may be even better outcome in terms of readability and
maintainability than the "03/13 refactor print_pci_device", especially if you don't
plan to reuse that anyway (only skimmed remaining patches), so hopefully a cleaner
separation.

for (my $i = 0; $i < $MAX_HOSTPCI_DEVICES; $i++)  

>  	my $multifunction = @$pcidevices > 1;
>  
>  	if ($d->{'legacy-igd'}) {
> @@ -503,7 +509,7 @@ sub print_hostpci_devices {
>  	}
>      }
>  
> -    return ($kvm_off, $gpu_passthrough, $legacy_igd);
> +    return ($kvm_off, $gpu_passthrough, $legacy_igd, $parsed_devices);
>  }
>  
>  sub prepare_pci_device {