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

[Thomas Lamprecht]

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.

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