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

[Martin Kletzander]

On Mon, Jul 18, 2016 at 08:25:40PM +0200, Tomasz Flendrich wrote:
>Thank you for your reply!
>
>> 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.
>I have thought about that before and I agree that a few tests for this
>would be useful. I decided not to add them yet, because of the issue
>I explained in another patch:
>> So far, there is only one test for persistent attachment. I will
>> add more testcases as soon as I hear some feedback on these changes.
>> qemuhotplugtest.c should first be modified anyway, so that the three
>> xmls's filenames (basis domain xml, device xml, expected xml) are stated
>> explicitly instead of being calculated. This will allow for more
>> flexibility in testing and less xml files duplicates.
>link to that patch:
>https://www.redhat.com/archives/libvir-list/2016-July/msg00595.html
>
>Adding tests will be a lot easier and without xml files duplication.
>I can implement it now, but should I do it on top of the patch
>“Test persistent device attachment”, parallelly to it, or wait for the feedback?
>
>
>>> […]
>>
>> vde2 is an address for a partition.  This works?  I don't think it
>> should.
>>
>> [...]
>>
>>> […]
>>
>> This ...
>>
>>> […]
>>
>> ... and this?  This will never work, so that's another bug right here.
>Yay, I am very happy that my tests proved to be useful so fast :)
>
>What exactly is the problem with “vde2”? And is it working properly
>if we change “vde2” to “vdea”?
>

The vde2 is second partition on disk vde.  We just use the vde part.  I
guess we support it just because reasons.

>The second problem is that the device aliases are the same for
>two different devices. Am I correct?
>

Yes, no hypervisor will probably allow you to do that.

>
>>> +      <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.
>I consider reserving/validating addresses as a part of address
>allocation, because they are connected to each other: if some
>address is reserved, it can’t be automatically allocated for another
>device, and vice versa. So I want to test both of them.  But you are
>totally right and thank you :)
>
>In this patch I added a test for both of these mixed together:
>please, take a look at the last three DO_TEST_* in my patch.
>The first two actions (an attachment and a detachment) are
>with explicit addresses, and the last action (an attachment)
>is with an implicit address.

But the problem is that you are not checking any assignment.  The code
is not executed at all.  That's because you specify 'target dev=' from
which we're trying to guess the right address.

Because virDomainDeviceDefPostParseInternal() is ran for the device
definition and it calls virDomainDiskDefAssignAddress() which in turn
calls virDiskNameToIndex().

>There are a lot of possibilities to test, for example the first attachment
>could also be done with an implicit address. I will write
>tests for more cases with pleasure.
>

We don't need to test every single thing, we should keep in mind that
most of the things will be caught anyway and (hopefully) every developer
is running all of them every now and then so it consumes time as
well...  Few parts could get a good scrub, actually.  Anyway, as I said,
there is no need to test every possible combination, I just wanted to
mention that so that you know.

Sorry for the late review, I need to get up to speed.

>Tomasz
>
-------------- 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/20160721/5be8f553/attachment-0001.sig>