[pve-devel] [PATCH qemu-server 1/2] cfg2cmd test: add tests for multifunction devices

[Dominik Csapak]

On 12/7/19 12:25 PM, Thomas Lamprecht wrote:
> On 12/6/19 2:29 PM, Dominik Csapak wrote:
>> by mocking the lspci call
>>
>> the mocked lspci code is basically the same as the real one,
>> only difference is the source of the devices and
>> there is no verbose flag
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>>   .../q35-linux-hostpci-multifunction.conf      | 16 +++++++
>>   .../q35-linux-hostpci-multifunction.conf.cmd  | 34 +++++++++++++++
>>   test/run_config2command_tests.pl              | 42 +++++++++++++++++++
>>   3 files changed, 92 insertions(+)
>>   create mode 100644 test/cfg2cmd/q35-linux-hostpci-multifunction.conf
>>   create mode 100644 test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
>>
>> diff --git a/test/cfg2cmd/q35-linux-hostpci-multifunction.conf b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf
>> new file mode 100644
>> index 0000000..5f1a3ff
>> --- /dev/null
>> +++ b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf
>> @@ -0,0 +1,16 @@
>> +# TEST: Config with q35, NUMA, hostpci passthrough, EFI & Linux
>> +bios: ovmf
>> +bootdisk: scsi0
>> +cores: 1
>> +efidisk0: local:100/vm-100-disk-1.qcow2,size=128K
>> +hostpci0: f0:43
>> +hostpci1: 1234:f0:43
>> +machine: q35
>> +memory: 512
>> +net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
>> +numa: 1
>> +ostype: l26
>> +scsihw: virtio-scsi-pci
>> +smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
>> +sockets: 2
>> +vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
>> diff --git a/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
>> new file mode 100644
>> index 0000000..833f37b
>> --- /dev/null
>> +++ b/test/cfg2cmd/q35-linux-hostpci-multifunction.conf.cmd
>> @@ -0,0 +1,34 @@
>> +/usr/bin/kvm \
>> +  -id 8006 \
>> +  -name vm8006 \
>> +  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server,nowait' \
>> +  -mon 'chardev=qmp,mode=control' \
>> +  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
>> +  -mon 'chardev=qmp-event,mode=control' \
>> +  -pidfile /var/run/qemu-server/8006.pid \
>> +  -daemonize \
>> +  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
>> +  -drive 'if=pflash,unit=0,format=raw,readonly,file=/usr/share/pve-edk2-firmware//OVMF_CODE.fd' \
>> +  -drive 'if=pflash,unit=1,format=qcow2,id=drive-efidisk0,file=/var/lib/vz/images/100/vm-100-disk-1.qcow2' \
>> +  -smp '2,sockets=2,cores=1,maxcpus=2' \
>> +  -nodefaults \
>> +  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
>> +  -vnc unix:/var/run/qemu-server/8006.vnc,password \
>> +  -cpu kvm64,+lahf_lm,+sep,+kvm_pv_unhalt,+kvm_pv_eoi,enforce \
>> +  -m 512 \
>> +  -object 'memory-backend-ram,id=ram-node0,size=256M' \
>> +  -numa 'node,nodeid=0,cpus=0,memdev=ram-node0' \
>> +  -object 'memory-backend-ram,id=ram-node1,size=256M' \
>> +  -numa 'node,nodeid=1,cpus=1,memdev=ram-node1' \
>> +  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
>> +  -readconfig /usr/share/qemu-server/pve-q35-4.0.cfg \
>> +  -device 'usb-tablet,id=tablet,bus=ehci.0,port=1' \
>> +  -device 'vfio-pci,host=0000:f0:43.0,id=hostpci0.0,bus=pci.0,addr=0x10.0,multifunction=on' \
>> +  -device 'vfio-pci,host=0000:f0:43.1,id=hostpci0.1,bus=pci.0,addr=0x10.1' \
>> +  -device 'vfio-pci,host=1234:f0:43.1,id=hostpci1,bus=pci.0,addr=0x11' \
>> +  -device 'VGA,id=vga,bus=pcie.0,addr=0x1' \
>> +  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3' \
>> +  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
>> +  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
>> +  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,bootindex=300' \
>> +  -machine 'type=q35+pve1'
>> diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
>> index 9b0b3bb..520b7e7 100755
>> --- a/test/run_config2command_tests.pl
>> +++ b/test/run_config2command_tests.pl
>> @@ -59,6 +59,20 @@ my $base_env = {
>>       real_qemu_version => PVE::QemuServer::kvm_user_version(), # not yet mocked
>>   };
>>   
>> +my $pci_devs = [
>> +    "0000:00:43.1",
>> +    "0000:00:f4.0",
>> +    "0000:00:ff.1",
>> +    "0000:0f:f2.0",
>> +    "0000:d0:13.0",
>> +    "0000:d0:15.1",
>> +    "0000:d0:17.0",
>> +    "0000:f0:42.0",
>> +    "0000:f0:43.0",
>> +    "0000:f0:43.1",
>> +    "1234:f0:43.1",
>> +];
>> +
>>   my $current_test; # = {
>>   #   description => 'Test description', # if available
>>   #   qemu_version => '2.12',
>> @@ -146,6 +160,34 @@ $pve_common_tools->mock(
>>       },
>>   );
>>   
>> +my $pve_common_sysfstools;
>> +$pve_common_sysfstools = Test::MockModule->new('PVE::SysFSTools');
>> +$pve_common_sysfstools->mock(
>> +    lspci => sub {
>> +	my ($filter, $verbose) = @_;
>> +	my $devices = [];
>> +	foreach my $id (@$pci_devs) {
>> +	    if (defined($filter) && !ref($filter) && $id !~ m/^(0000:)?\Q$filter\E/) {
>> +		next;
>> +	    }
>> +
>> +	    my $res = {
>> +		id => $id
>> +	    };
>> +
>> +	    if (defined($filter) && ref($filter) eq 'CODE' && !$filter->($res)) {
>> +		next;
>> +	    }
> 
> maybe just:
> next if defined($filter) && !ref($filter) && $id !~ m/^(0000:)?\Q$filter\E/;
> next if defined($filter) && ref($filter) eq 'CODE' && !$filter->($res);
> 
> or slightly nicer to read:
> if (defined($filter)) {
>      next if !ref($filter) && $id !~ m/^(0000:)?\Q$filter\E/;
>      next if ref($filter) eq 'CODE' && !$filter->($res);
> }
> 
>> +
>> +	    push @$devices, $res;
> 
> 
> push @$devices, { id => $id };
> 
>> +	}
>> +
>> +	$devices = [ sort { $a->{id} cmp $b->{id} } @$devices ];
> 
> just do the sort in the for loop above already?
> 
> I.e., all in all:
> 
> my $devices = [];
> for my $id (sort @$pci_devs) {
>      if (defined($filter)) {
> 	next if !ref($filter) && $id !~ m/^(0000:)?\Q$filter\E/;
> 	next if ref($filter) eq 'CODE' && !$filter->($id);
>      }
>      push @$devices, { id => $id };
> }
> return $devices;
> 
> seems quite shorter and still expressive (at least IMO), which would be nice
> to not bloat the test code to much - that's why I'm nitpicking here :)
> 
> Playing code golf one could also do something like the following for
> the whole sub:
> 
> return [ grep {
>      !defined($filter)
>      || (!ref($filter) && $_ =~ m/^(0000:)?\Q$filter\E/)
>      || (ref($filter) eq 'CODE' && $filter->($_))
> } sort @$pci_devs ];
> 
> this time even tested :P
> 
> As said, a bit nit-picky, but even the code-golfy example is still
> really expressive enough to quickly understand (if having at least
> some perl knowlegde), and it has 8 lines, yours 23 - so roughly 3/4
> lines reduced.. I'd even say having many lines for "return sorted
> filtered array" makes it harder to understand, as one expects that
> it does more.

sure, will send a v2 :)

> 
>> +
>> +	return $devices;
>> +    },
>> +);
>> +
>>   sub diff($$) {
>>       my ($a, $b) = @_;
>>       return if $a eq $b;
>>
>