[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] tests: Introduce qemuhotplugtest



On Tue, Jun 25, 2013 at 11:38:15AM +0200, Michal Privoznik wrote:
> As my punishment for the break in 7f15ebc7 (fixed in 752596b5dd) I'm
> introducing this test to make sure it won't happen again. Currently,
> only test for <graphics/> is supported.
> ---
>  .gitignore                                         |   1 +
>  tests/Makefile.am                                  |  11 +-
>  tests/qemuhotplugtest.c                            | 208 +++++++++++++++++++++
>  .../qemuhotplug-disk-cdrom-nochange.xml            |   6 +
>  .../qemuhotplug-graphics-spice-listen.xml          |  11 ++
>  .../qemuhotplug-graphics-spice-nochange.xml        |  11 ++
>  ...qemuhotplug-graphics-spice-timeout-nochange.xml |   1 +
>  ...qemuhotplug-graphics-spice-timeout-password.xml |   1 +
>  .../qemuxml2argv-graphics-spice-listen-network.xml |  45 +++++
>  9 files changed, 293 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qemuhotplugtest.c
>  create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-disk-cdrom-nochange.xml
>  create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-graphics-spice-listen.xml
>  create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-graphics-spice-nochange.xml
>  create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-graphics-spice-timeout-nochange.xml
>  create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-graphics-spice-timeout-password.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-listen-network.xml

ACK

> 
> diff --git a/.gitignore b/.gitignore
> index 7a28941..3efc2e4 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -167,6 +167,7 @@
>  /tests/openvzutilstest
>  /tests/qemuargv2xmltest
>  /tests/qemuhelptest
> +/tests/qemuhotplugtest
>  /tests/qemumonitorjsontest
>  /tests/qemumonitortest
>  /tests/qemuxmlnstest
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 9c9c802..a019eb9 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -155,7 +155,7 @@ endif
>  if WITH_QEMU
>  test_programs += qemuxml2argvtest qemuxml2xmltest qemuxmlnstest \
>  	qemuargv2xmltest qemuhelptest domainsnapshotxml2xmltest \
> -	qemumonitortest qemumonitorjsontest
> +	qemumonitortest qemumonitorjsontest qemuhotplugtest
>  endif
>  
>  if WITH_LXC
> @@ -405,6 +405,13 @@ qemumonitorjsontest_SOURCES = \
>  	$(NULL)
>  qemumonitorjsontest_LDADD = $(qemu_LDADDS) libqemumonitortestutils.la
>  
> +qemuhotplugtest_SOURCES = \
> +	qemuhotplugtest.c \
> +	testutils.c testutils.h \
> +	testutilsqemu.c testutilsqemu.h \
> +	$(NULL)
> +qemuhotplugtest_LDADD = $(qemu_LDADDS) libqemumonitortestutils.la
> +
>  domainsnapshotxml2xmltest_SOURCES = \
>  	domainsnapshotxml2xmltest.c testutilsqemu.c testutilsqemu.h \
>  	testutils.c testutils.h
> @@ -413,7 +420,7 @@ else
>  EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \
>  	qemuxmlnstest.c qemuhelptest.c domainsnapshotxml2xmltest.c \
>  	qemumonitortest.c testutilsqemu.c testutilsqemu.h \
> -	qemumonitorjsontest.c \
> +	qemumonitorjsontest.c qemuhotplugtest.c \
>  	$(QEMUMONITORTESTUTILS_SOURCES)
>  endif
>  
> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
> new file mode 100644
> index 0000000..ed3ca7f
> --- /dev/null
> +++ b/tests/qemuhotplugtest.c
> @@ -0,0 +1,208 @@
> +/*
> + * Copyright (C) 2011-2013 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <config.h>
> +
> +#include "qemu/qemu_conf.h"
> +#include "qemu/qemu_hotplug.h"
> +#include "qemumonitortestutils.h"
> +#include "testutils.h"
> +#include "testutilsqemu.h"
> +#include "virerror.h"
> +#include "virstring.h"
> +#include "virthread.h"
> +#include "virfile.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +static virQEMUDriver driver;
> +
> +struct qemuHotplugTestData {
> +    const char *domain_filename;
> +    const char *device_filename;
> +    bool fail;
> +    const char *const *mon;
> +};
> +
> +static int
> +qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
> +                         virDomainObjPtr *vm,
> +                         const char *filename)
> +{
> +    int ret = -1;
> +
> +    if (!(*vm = virDomainObjNew(xmlopt)))
> +        goto cleanup;
> +
> +    if (!((*vm)->def = virDomainDefParseFile(filename,
> +                                             driver.caps,
> +                                             driver.xmlopt,
> +                                             QEMU_EXPECTED_VIRT_TYPES,
> +                                             0)))
> +        goto cleanup;
> +
> +    ret = 0;
> +cleanup:
> +    return ret;
> +}
> +
> +static int
> +testQemuHotplug(const void *data)
> +{
> +    int ret = -1;
> +    struct qemuHotplugTestData *test = (struct qemuHotplugTestData *) data;
> +    char *domain_filename = NULL;
> +    char *device_filename = NULL;
> +    char *device_xml = NULL;
> +    const char *const *tmp;
> +    bool fail = test->fail;
> +    virDomainObjPtr vm = NULL;
> +    virDomainDeviceDefPtr dev = NULL;
> +    virCapsPtr caps = NULL;
> +    qemuMonitorTestPtr test_mon = NULL;
> +    qemuDomainObjPrivatePtr priv;
> +
> +    if (virAsprintf(&domain_filename, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml",
> +                    abs_srcdir, test->domain_filename) < 0 ||
> +        virAsprintf(&device_filename, "%s/qemuhotplugtestdata/qemuhotplug-%s.xml",
> +                    abs_srcdir, test->device_filename) < 0)
> +        goto cleanup;
> +
> +    if (!(caps = virQEMUDriverGetCapabilities(&driver, false)))
> +        goto cleanup;
> +
> +    if (qemuHotplugCreateObjects(driver.xmlopt, &vm, domain_filename) < 0)
> +        goto cleanup;
> +
> +    priv = vm->privateData;
> +
> +    if (virtTestLoadFile(device_filename, &device_xml) < 0)
> +        goto cleanup;
> +
> +    if (!(dev = virDomainDeviceDefParse(device_xml, vm->def,
> +                                        caps, driver.xmlopt,
> +                                        VIR_DOMAIN_XML_INACTIVE)))
> +        goto cleanup;
> +
> +    /* Now is the best time to feed the spoofed monitor with predefined
> +     * replies. */
> +    if (!(test_mon = qemuMonitorTestNew(true, driver.xmlopt)))
> +        goto cleanup;
> +
> +    tmp = test->mon;
> +    while (tmp && *tmp) {
> +        const char *command_name;
> +        const char *response;
> +
> +        if (!(command_name = *tmp++) ||
> +            !(response = *tmp++))
> +            break;
> +        if (qemuMonitorTestAddItem(test_mon, command_name, response) < 0)
> +            goto cleanup;
> +    }
> +
> +    priv->mon = qemuMonitorTestGetMonitor(test_mon);
> +    priv->monJSON = true;
> +
> +    /* XXX We need to unlock the monitor here, as
> +     * qemuDomainObjEnterMonitorInternal (called from qemuDomainChangeGraphics)
> +     * tries to lock it again */
> +    virObjectUnlock(priv->mon);
> +
> +    /* XXX Ideally, we would call qemuDomainUpdateDeviceLive here.  But that
> +     * would require us to provide virConnectPtr and virDomainPtr (they're used
> +     * in case of updating a disk device. So for now, we will proceed with
> +     * breaking the function into pieces. If we ever learn how to fake those
> +     * required object, we can replace this code then. */
> +    switch (dev->type) {
> +    case VIR_DOMAIN_DEVICE_GRAPHICS:
> +        ret = qemuDomainChangeGraphics(&driver, vm, dev->data.graphics);
> +        break;
> +    default:
> +        if (virTestGetVerbose())
> +            fprintf(stderr, "device type '%s' cannot be updated",
> +                    virDomainDeviceTypeToString(dev->type));
> +        break;
> +    }
> +
> +cleanup:
> +    VIR_FREE(domain_filename);
> +    VIR_FREE(device_filename);
> +    VIR_FREE(device_xml);
> +    /* don't dispose test monitor with VM */
> +    priv->mon = NULL;
> +    virObjectUnref(vm);
> +    virDomainDeviceDefFree(dev);
> +    virObjectUnref(caps);
> +    qemuMonitorTestFree(test_mon);
> +    return ((ret < 0 && fail) || (!ret && !fail)) ? 0 : -1;
> +}
> +
> +static int
> +mymain(void)
> +{
> +    int ret = 0;
> +
> +#if !WITH_YAJL
> +    fputs("libvirt not compiled with yajl, skipping this test\n", stderr);
> +    return EXIT_AM_SKIP;
> +#endif
> +
> +    if (virThreadInitialize() < 0 ||
> +        !(driver.caps = testQemuCapsInit()) ||
> +        !(driver.xmlopt = virQEMUDriverCreateXMLConf(&driver)))
> +        return EXIT_FAILURE;
> +
> +    virEventRegisterDefaultImpl();
> +
> +    driver.config = virQEMUDriverConfigNew(false);
> +    VIR_FREE(driver.config->spiceListen);
> +    VIR_FREE(driver.config->vncListen);
> +
> +    /* some dummy values from 'config file' */
> +    if (VIR_STRDUP_QUIET(driver.config->spicePassword, "123456") < 0)
> +        return EXIT_FAILURE;
> +
> +#define DO_TEST(file, dev, fial, ...) \
> +    do { \
> +        const char *my_mon[] = { __VA_ARGS__, NULL}; \
> +        struct qemuHotplugTestData data =                                    \
> +            {.domain_filename = file, .device_filename = dev, .fail = fial,  \
> +             .mon = my_mon}; \
> +        if (virtTestRun(#file, 1, testQemuHotplug, &data) < 0)               \
> +            ret = -1;                                                        \
> +    } while (0)

What's with the 'fail' parameter you're passing across test cases.
AFAICT, no test needs to be aware of the fail status of any earlier
test. You're re-creating the fake monitor for each test case so
no state is shared between tests. Just setting the 'ret = -1' here
is sufficient

> +
> +    DO_TEST("graphics-spice", "graphics-spice-nochange", false, NULL);
> +    DO_TEST("graphics-spice-timeout", "graphics-spice-timeout-nochange", false,
> +            "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}");
> +    DO_TEST("graphics-spice-timeout", "graphics-spice-timeout-password", false,
> +            "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}");
> +    DO_TEST("graphics-spice", "graphics-spice-listen", true, NULL);
> +    DO_TEST("graphics-spice-listen-network", "graphics-spice-listen-network", false,
> +            "set_password", "{\"return\":{}}", "expire_password", "{\"return\":{}}");
> +    /* Strange huh? Currently, only graphics can be testet :-P */
> +    DO_TEST("disk-cdrom", "disk-cdrom-nochange", true, NULL);
> +
> +    virObjectUnref(driver.caps);
> +    virObjectUnref(driver.xmlopt);
> +    return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
> +}


Everything looks good, aside from the 'fail' flag there. ACK if that
is just removed.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]