[pve-devel] applied: [RFC PATCH] collect device list for nested pci-bridges

[Wolfgang Bumiller]

applied

On Thu, Apr 12, 2018 at 05:04:56PM +0200, Dominik Csapak wrote:
> when using q35 as machine type, there are nested pci-bridges,
> but we only checked the first layer
> 
> this resulted in not being able to hotplug scsi devices,
> because scsihw0 was deeper in the pci-bridge construct, we did not see
> it and tried to add it (which fails of course)
> 
> this patch checks all bridges, regardless how deeply nested they are
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> i am not sure if it is important to count only
> the devices with an id for the bridge devices, a short glance
> over the code did not reveal anything which would depend on
> this, but maybe someone with a deeper insight could comment on that?

AFAIK we can't add/delete devices without ids, and cannot add/delete to
bridges which have no ID, so there's not much use in checking them, they
shouldn't have any of our added devices to begin with, so duplicate
errors aren't supposed to happen anyway ;-)

> also i know that the obvious solution would be to make a recursive
> sub, but i am not a fan of those
> if someone has a compelling argument for using recursion, i can rewrite it
>  PVE/QemuServer.pm | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 315073f..9225788 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -3601,21 +3601,22 @@ sub vm_devices_list {
>      my ($vmid) = @_;
>  
>      my $res = vm_mon_cmd($vmid, 'query-pci');
> +    my $devices_to_check = [];
>      my $devices = {};
>      foreach my $pcibus (@$res) {
> -	foreach my $device (@{$pcibus->{devices}}) {
> -	    next if !$device->{'qdev_id'};
> -	    if ($device->{'pci_bridge'}) {
> -		$devices->{$device->{'qdev_id'}} = 1;
> -		foreach my $bridge_device (@{$device->{'pci_bridge'}->{devices}}) {
> -		    next if !$bridge_device->{'qdev_id'};
> -		    $devices->{$bridge_device->{'qdev_id'}} = 1;
> -		    $devices->{$device->{'qdev_id'}}++;
> -		}
> -	    } else {
> -		$devices->{$device->{'qdev_id'}} = 1;
> -	    }
> +	push @$devices_to_check, @{$pcibus->{devices}},
> +    }
> +
> +    while (@$devices_to_check) {
> +	my $to_check = [];
> +	for my $d (@$devices_to_check) {
> +	    $devices->{$d->{'qdev_id'}} = 1 if $d->{'qdev_id'};
> +	    next if !$d->{'pci_bridge'};
> +
> +	    $devices->{$d->{'qdev_id'}} += scalar(@{$d->{'pci_bridge'}->{devices}});
> +	    push @$to_check, @{$d->{'pci_bridge'}->{devices}};
>  	}
> +	$devices_to_check = $to_check;
>      }
>  
>      my $resblock = vm_mon_cmd($vmid, 'query-block');
> -- 
> 2.11.0