[libvirt] [PATCH v2] qemuhotplugtest: Add tests for ccw devices

[Martin Kletzander]

On Tue, Jul 12, 2016 at 03:08:22PM +0200, Tomasz Flendrich wrote:
>These are a few simple and one complex testcases.
>The simple ones test attaching and detaching ccw devices
>with both implicitly and explicitly stated addresses.
>In the complex one, attaching and detaching a device
>should make the address free to reuse.
>
>I have plan to rework the address handling, so testcases
>that verify hotplugging ccw devices will help in avoiding
>regression.
>
>In this commit, some device files are duplicated because
>of the way qemuhotplug.c calculates the expected xml filenames.
>I plan on changing that to explicitly stating the basis domain
>xml, the device xml, and the expected xml.
>---
>
>Changed in v2:
>* Add a newline to the end of qemuhotplug-ccw-virtio-2-explicit.xml,
>  so that syntax-check doesn't complain.
>* Rename two files: remove the "explicit" word where the address
>  is actually implicit.
>
>Link to the previous patch:
>https://www.redhat.com/archives/libvir-list/2016-July/msg00264.html
>
>

[...]

>diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-ccw-virtio-1-explicit.xml b/tests/qemuhotplugtestdevices/qemuhotplug-ccw-virtio-1-explicit.xml
>new file mode 100644
>index 0000000..74bd6a9
>--- /dev/null
>+++ b/tests/qemuhotplugtestdevices/qemuhotplug-ccw-virtio-1-explicit.xml
>@@ -0,0 +1,8 @@
>+<disk type='file' device='disk'>
>+  <driver name='qemu' type='raw' cache='none'/>
>+  <source file='/dev/null'/>
>+  <target dev='vde' bus='virtio'/>
>+  <readonly/>
>+  <shareable/>
>+  <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/>
>+</disk>

I know you weren't aiming for that, but this got me thinking, are we
checking that unplug works with not fully specified XML?  I mean that
when unplugging, you don't need to specify everything.  Whatever
uniquely identifies the device should be enough.  So for example just
address or target, definitely no need for the readonly and shareable
flags.  But as said, that's not meant to be part of this patch.

[...]

>diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-ccw-virtio-2-explicit.xml b/tests/qemuhotplugtestdevices/qemuhotplug-ccw-virtio-2-explicit.xml
>new file mode 100644
>index 0000000..b2cb161
>--- /dev/null
>+++ b/tests/qemuhotplugtestdevices/qemuhotplug-ccw-virtio-2-explicit.xml
>@@ -0,0 +1,8 @@
>+<disk type='file' device='disk'>
>+  <driver name='qemu' type='raw' cache='none'/>
>+  <source file='/dev/null'/>
>+  <target dev='vde2' bus='virtio'/>

vde2 is an address for a partition.  This works?  I don't think it
should.

[...]

>diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-explicit.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-explicit.xml
>new file mode 100644
>index 0000000..ff94b6c
>--- /dev/null
>+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-ccw-live-with-2-ccw-virtio+ccw-virtio-1-explicit.xml
>@@ -0,0 +1,73 @@
>+<domain type='kvm' id='7'>
>+  <name>hotplug</name>
>+  <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid>
>+  <memory unit='KiB'>4194304</memory>
>+  <currentMemory unit='KiB'>4194304</currentMemory>
>+  <vcpu placement='static'>4</vcpu>
>+  <os>
>+    <type arch='s390x' machine='s390-ccw'>hvm</type>
>+    <boot dev='hd'/>
>+  </os>
>+  <features>
>+    <acpi/>
>+    <apic/>
>+    <pae/>
>+  </features>
>+  <clock offset='utc'/>
>+  <on_poweroff>destroy</on_poweroff>
>+  <on_reboot>restart</on_reboot>
>+  <on_crash>restart</on_crash>
>+  <devices>
>+    <emulator>/usr/libexec/qemu-kvm</emulator>
>+    <disk type='file' device='disk'>
>+      <driver name='qemu' type='raw' cache='none'/>
>+      <source file='/dev/null'/>
>+      <backingStore/>
>+      <target dev='vde' bus='virtio'/>
>+      <readonly/>
>+      <shareable/>
>+      <alias name='virtio-disk4'/>

This ...

>+      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/>
>+    </disk>
>+    <disk type='file' device='disk'>
>+      <driver name='qemu' type='raw' cache='none'/>
>+      <source file='/dev/null'/>
>+      <backingStore/>
>+      <target dev='vde2' bus='virtio'/>
>+      <readonly/>
>+      <shareable/>
>+      <alias name='virtio-disk4'/>

... and this?  This will never work, so that's another bug right here.

>+      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0001'/>

By the way, attaching a device without explicit address and without
target dev= specified (only bus='virtio')  Should work and that would
actually test address allocation.  By allocating disk with target
dev='XX' the address gets calculated from that device, so no allocation
is being tested.

Other than these problems the approach is fine.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160718/17bc4ff4/attachment-0001.sig>