Re: [PATCH 06/24] tests: hostdev: Group test cases

[Laine Stump <laine@xxxxxxxxx>]

On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
> Instead of considering each single step its own test case, create
> high level test cases that reproduce a certain scenario.
> ---
>   tests/virhostdevtest.c | 119 ++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 97 insertions(+), 22 deletions(-)

I like the idea, just have two comments:

1) would it maybe be useful to keep the individual tests, in order to 
make it simpler to determine which piece had broken in the case of a 
regression?

2) It looks like this only tests for legacy kvm (i.e. 
attaching/detaching pci-stub). It would be good to have it test for vfio 
as well (although maybe that can be saved for after we add support for 
using individual devices' driver_override to attach different drivers).

ACK as is, the comments are food for thought.

> diff --git a/tests/virhostdevtest.c b/tests/virhostdevtest.c
> index 8dc92f6..c0ab044 100644
> --- a/tests/virhostdevtest.c
> +++ b/tests/virhostdevtest.c
> @@ -160,7 +160,7 @@ virHostdevHostSupportsPassthroughKVM(void)
>   # endif
>   
>   static int
> -testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED)
> +testVirHostdevPreparePCIHostdevs_unmanaged(void)
>   {
>       int ret = -1;
>       size_t active_count, inactive_count, i;
> @@ -219,7 +219,7 @@ testVirHostdevPreparePCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED)
>   }
>   
>   static int
> -testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED)
> +testVirHostdevReAttachPCIHostdevs_unmanaged(void)
>   {
>       int ret = -1;
>       size_t active_count, inactive_count, i;
> @@ -253,7 +253,7 @@ testVirHostdevReAttachPCIHostdevs_unmanaged(const void *opaque ATTRIBUTE_UNUSED)
>   }
>   
>   static int
> -testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED)
> +testVirHostdevPreparePCIHostdevs_managed(void)
>   {
>       int ret = -1;
>       size_t active_count, inactive_count, i;
> @@ -304,7 +304,7 @@ testVirHostdevPreparePCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED)
>   }
>   
>   static int
> -testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED)
> +testVirHostdevReAttachPCIHostdevs_managed(void)
>   {
>       int ret = -1;
>       size_t active_count, inactive_count, i;
> @@ -338,7 +338,7 @@ testVirHostdevReAttachPCIHostdevs_managed(const void *opaque ATTRIBUTE_UNUSED)
>   }
>   
>   static int
> -testVirHostdevDetachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED)
> +testVirHostdevDetachPCINodeDevice(void)
>   {
>       int ret = -1;
>       size_t active_count, inactive_count, i;
> @@ -357,8 +357,9 @@ testVirHostdevDetachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED)
>    cleanup:
>       return ret;
>   }
> +
>   static int
> -testVirHostdevResetPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED)
> +testVirHostdevResetPCINodeDevice(void)
>   {
>       int ret = -1;
>       size_t active_count, inactive_count, i;
> @@ -380,7 +381,7 @@ testVirHostdevResetPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED)
>   }
>   
>   static int
> -testVirHostdevReAttachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED)
> +testVirHostdevReAttachPCINodeDevice(void)
>   {
>       int ret = -1;
>       size_t active_count, inactive_count, i;
> @@ -402,7 +403,7 @@ testVirHostdevReAttachPCINodeDevice(const void *opaque ATTRIBUTE_UNUSED)
>   }
>   
>   static int
> -testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED)
> +testVirHostdevUpdateActivePCIHostdevs(void)
>   {
>       int ret = -1;
>       size_t active_count, inactive_count;
> @@ -430,6 +431,91 @@ testVirHostdevUpdateActivePCIHostdevs(const void *opaque ATTRIBUTE_UNUSED)
>       return ret;
>   }
>   
> +/**
> + * testVirHostdevRoundtripUnmanaged:
> + * @opaque: unused
> + *
> + * Perform a roundtrip with unmanaged devices.
> + *
> + *   1. Detach devices from the host
> + *   2. Attach devices to the guest as unmanaged
> + *   3. Detach devices from the guest as unmanaged
> + *   4. Reattach devices to the host
> + */
> +static int
> +testVirHostdevRoundtripUnmanaged(const void *opaque ATTRIBUTE_UNUSED)
> +{
> +    int ret = -1;
> +
> +    if (testVirHostdevDetachPCINodeDevice() < 0)
> +        goto out;
> +    if (virHostdevHostSupportsPassthroughKVM()) {
> +        if (testVirHostdevPreparePCIHostdevs_unmanaged() < 0)
> +            goto out;
> +        if (testVirHostdevReAttachPCIHostdevs_unmanaged() < 0)
> +            goto out;
> +    }
> +    if (testVirHostdevReAttachPCINodeDevice() < 0)
> +        goto out;
> +
> +    ret = 0;
> +
> + out:
> +    return ret;
> +}
> +
> +/**
> + * testVirHostdevRoundtripManaged:
> + * @opaque: unused
> + *
> + * Perform a roundtrip with managed devices.
> + *
> + *   1. Attach devices to the guest as managed
> + *   2. Detach devices from the guest as managed
> + */
> +static int
> +testVirHostdevRoundtripManaged(const void *opaque ATTRIBUTE_UNUSED)
> +{
> +    int ret = -1;
> +
> +    if (virHostdevHostSupportsPassthroughKVM()) {
> +        if (testVirHostdevPreparePCIHostdevs_managed() < 0)
> +            goto out;
> +        if (testVirHostdevReAttachPCIHostdevs_managed() < 0)
> +            goto out;
> +    }
> +
> +    ret = 0;
> +
> + out:
> +    return ret;
> +}
> +
> +/**
> + * testVirHostdevOther:
> + * @opaque: unused
> + *
> + * Perform other operations on devices.
> + *
> + *   1. Reset devices
> + *   2. Update list of active devices
> + */
> +static int
> +testVirHostdevOther(const void *opaque ATTRIBUTE_UNUSED)
> +{
> +    int ret = -1;
> +
> +    if (testVirHostdevResetPCINodeDevice() < 0)
> +        goto out;
> +    if (testVirHostdevUpdateActivePCIHostdevs() < 0)
> +        goto out;
> +
> +    ret = 0;
> +
> + out:
> +    return ret;
> +}
> +
>   # define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX"
>   
>   static int
> @@ -460,20 +546,9 @@ mymain(void)
>       if (myInit() < 0)
>           fprintf(stderr, "Init data structures failed.");
>   
> -    DO_TEST(testVirHostdevDetachPCINodeDevice);
> -    if (virHostdevHostSupportsPassthroughKVM()) {
> -        /* following tests would check KVM support */
> -        DO_TEST(testVirHostdevPreparePCIHostdevs_unmanaged);
> -        DO_TEST(testVirHostdevReAttachPCIHostdevs_unmanaged);
> -    }
> -    DO_TEST(testVirHostdevResetPCINodeDevice);
> -    DO_TEST(testVirHostdevReAttachPCINodeDevice);
> -    if (virHostdevHostSupportsPassthroughKVM()) {
> -        /* following tests would check KVM support */
> -        DO_TEST(testVirHostdevPreparePCIHostdevs_managed);
> -        DO_TEST(testVirHostdevReAttachPCIHostdevs_managed);
> -    }
> -    DO_TEST(testVirHostdevUpdateActivePCIHostdevs);
> +    DO_TEST(testVirHostdevRoundtripUnmanaged);
> +    DO_TEST(testVirHostdevRoundtripManaged);
> +    DO_TEST(testVirHostdevOther);
>   
>       myCleanup();
>   

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list