[pve-devel] [PATCH qemu-server v3 03/13] PCI: refactor print_pci_device

[Thomas Lamprecht]

Am 20/09/2022 um 14:50 schrieb Dominik Csapak:
> into a private sub. This makes the 'print_hostpci_devices' function more
> easier to read

looks OK, some style nits inline

> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/QemuServer/PCI.pm | 57 +++++++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
> index 1b82aca..7406246 100644
> --- a/PVE/QemuServer/PCI.pm
> +++ b/PVE/QemuServer/PCI.pm
> @@ -393,6 +393,29 @@ sub parse_hostpci {
>      return $res;
>  }
>  
> +my $print_pci_device = sub {

nit: could use a `my sub` here or? 

> +    my ($device, $id, $hostdevice, $pciaddr, $xvga, $bootindex, $function) = @_;

could use the opportunity to switch to a more consistent naming style, e.g., using snake_case

> +
> +    my $devicestr = "vfio-pci,$hostdevice";
> +
> +    my $mf_addr = defined($function) ? ".$function" : '';

s/mf/multi_function/ 
> +
> +    $devicestr .= ",id=${id}${mf_addr}${pciaddr}${mf_addr}";
> +
> +    if (!defined($function) || $function == 0) {
> +	$devicestr .= ',rombar=0' if defined($device->{rombar}) && !$device->{rombar};
> +	$devicestr .= "$xvga";
> +	$devicestr .= ",multifunction=on" if defined($function);
> +	$devicestr .= ",romfile=/usr/share/kvm/$device->{romfile}" if $device->{romfile};
> +	$devicestr .= ",bootindex=$bootindex" if defined($bootindex);
> +	for my $option (qw(vendor-id device-id sub-vendor-id sub-device-id)) {
> +	    $devicestr .= ",x-pci-$option=$device->{$option}" if $device->{$option};
> +	}
> +    }
> +
> +    return $devicestr;
> +};
> +
>  sub print_hostpci_devices {
>      my ($vmid, $conf, $devices, $vga, $winversion, $q35, $bridges, $arch, $machine_type, $bootorder) = @_;
>  
> @@ -457,37 +480,23 @@ sub print_hostpci_devices {
>  	    $gpu_passthrough = 1;
>  	}
>  
> -	my $sysfspath;
> -	if ($d->{mdev} && scalar(@$pcidevices) == 1) {
> +	my $bootindex = $bootorder->{$id};
> +
> +	if ($d->{mdev} && !$multifunction) {
>  	    my $uuid = generate_mdev_uuid($vmid, $i);
> -	    $sysfspath = "/sys/bus/mdev/devices/$uuid";
> +	    my $sysfspath = "sysfsdev=/sys/bus/mdev/devices/$uuid";
> +	    my $devicestr = $print_pci_device->($d, $id, $sysfspath, $pciaddr, $xvga, $bootindex);
> +	    push @$devices, '-device', $devicestr;

maybe add a short comment or empty line here to make this `next` stand out a bit more, almost
overlooked it

> +	    next;
>  	} elsif ($d->{mdev}) {
>  	    warn "ignoring mediated device '$id' with multifunction device\n";
>  	}
>  
>  	my $j = 0;
>  	foreach my $pcidevice (@$pcidevices) {
> -	    my $devicestr = "vfio-pci";
> -
> -	    if ($sysfspath) {
> -		$devicestr .= ",sysfsdev=$sysfspath";
> -	    } else {
> -		$devicestr .= ",host=$pcidevice->{id}";
> -	    }
> -
> -	    my $mf_addr = $multifunction ? ".$j" : '';
> -	    $devicestr .= ",id=${id}${mf_addr}${pciaddr}${mf_addr}";
> -
> -	    if ($j == 0) {
> -		$devicestr .= ',rombar=0' if defined($d->{rombar}) && !$d->{rombar};
> -		$devicestr .= "$xvga";
> -		$devicestr .= ",multifunction=on" if $multifunction;
> -		$devicestr .= ",romfile=/usr/share/kvm/$d->{romfile}" if $d->{romfile};
> -		$devicestr .= ",bootindex=$bootorder->{$id}" if $bootorder->{$id};
> -		for my $option (qw(vendor-id device-id sub-vendor-id sub-device-id)) {
> -		    $devicestr .= ",x-pci-$option=$d->{$option}" if $d->{$option};
> -		}
> -	    }
> +	    my $host =  "host=$pcidevice->{id}";

extra white space after =

> +	    my $func = $multifunction ? $j : undef;
> +	    my $devicestr = $print_pci_device->($d, $id, $host, $pciaddr, $xvga, $bootindex, $func);
>  
>  	    push @$devices, '-device', $devicestr;
>  	    $j++;