Re: [Qemu-devel] [PATCH v3 6/7] qdev-monitor-test: add device_add leak test cases

[Andreas Färber]

Am 30.10.2013 14:54, schrieb Stefan Hajnoczi:
> Ensure that the device_add error code path deletes device objects.
> Failure to do so not only leaks the objects but can also keep other
> objects (like drive or netdev) alive due to qdev properties holding
> references.
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  tests/Makefile            |  2 ++
>  tests/qdev-monitor-test.c | 81 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 83 insertions(+)
>  create mode 100644 tests/qdev-monitor-test.c
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index 7863123..2771f92 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -68,6 +68,7 @@ check-qtest-i386-y += tests/rtc-test$(EXESUF)
>  check-qtest-i386-y += tests/i440fx-test$(EXESUF)
>  check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
>  check-qtest-i386-y += tests/blockdev-test$(EXESUF)
> +check-qtest-i386-y += tests/qdev-monitor-test$(EXESUF)
>  check-qtest-x86_64-y = $(check-qtest-i386-y)
>  gcov-files-i386-y += i386-softmmu/hw/mc146818rtc.c

Same comment here about adding to the list.

>  gcov-files-x86_64-y = $(subst 
> i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
> @@ -176,6 +177,7 @@ tests/tmp105-test$(EXESUF): tests/tmp105-test.o 
> $(libqos-omap-obj-y)
>  tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
>  tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
>  tests/blockdev-test$(EXESUF): tests/blockdev-test.o $(libqos-pc-obj-y)
> +tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o 
> $(libqos-pc-obj-y)
>  tests/qemu-iotests/socket_scm_helper$(EXESUF): 
> tests/qemu-iotests/socket_scm_helper.o
>  
>  # QTest rules
> diff --git a/tests/qdev-monitor-test.c b/tests/qdev-monitor-test.c
> new file mode 100644
> index 0000000..d3d6d39
> --- /dev/null
> +++ b/tests/qdev-monitor-test.c
> @@ -0,0 +1,81 @@
> +/*
> + * qdev-monitor.c test cases
> + *
> + * Copyright (C) 2013 Red Hat Inc.
> + *
> + * Authors:
> + *  Stefan Hajnoczi <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> + * See the COPYING.LIB file in the top-level directory.
> + */
> +
> +#include <string.h>
> +#include <glib.h>
> +#include "libqtest.h"
> +#include "qapi/qmp/qjson.h"
> +
> +static void test_device_add(void)
> +{
> +    QDict *response;
> +    QDict *error;
> +
> +    qtest_start("-drive if=none,id=drive0");
> +
> +    /* Make device_add fail.  If this leaks the virtio-blk-pci device then a
> +     * reference to drive0 will also be held (via qdev properties).
> +     */
> +    response = qmp("{\"execute\": \"device_add\","
> +                   " \"arguments\": {"
> +                   "   \"driver\": \"virtio-blk-pci\","
> +                   "   \"drive\": \"drive0\""
> +                   "}}");
> +    g_assert(response);
> +    error = qdict_get_qdict(response, "error");
> +    g_assert(!strcmp(qdict_get_try_str(error, "class") ?: "",
> +                     "GenericError"));
> +    g_assert(!strcmp(qdict_get_try_str(error, "desc") ?: "",
> +                     "Device initialization failed."));
> +    QDECREF(response);
> +
> +    /* Delete the drive */
> +    response = qmp("{\"execute\": \"human-monitor-command\","
> +                   " \"arguments\": {"
> +                   "   \"command-line\": \"drive_del drive0\""
> +                   "}}");
> +    g_assert(response);
> +    g_assert(!strcmp(qdict_get_try_str(response, "return") ?: "(null)", ""));
> +    QDECREF(response);
> +
> +    /* Try to re-add the drive.  This fails with duplicate IDs if a leaked
> +     * virtio-blk-pci exists that holds a reference to the old drive0.
> +     */
> +    response = qmp("{\"execute\": \"human-monitor-command\","
> +                   " \"arguments\": {"
> +                   "   \"command-line\": \"drive_add pci-addr=auto 
> if=none,id=drive0\""
> +                   "}}");
> +    g_assert(response);
> +    g_assert(!strcmp(qdict_get_try_str(response, "return") ?: "",
> +                     "OK\r\n"));
> +    QDECREF(response);
> +
> +    qtest_end();
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    const char *arch = qtest_get_arch();
> +
> +    /* Check architecture */
> +    if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
> +        g_test_message("Skipping test for non-x86\n");
> +        return 0;
> +    }
> +
> +    /* Run the tests */
> +    g_test_init(&argc, &argv, NULL);
> +
> +    qtest_add_func("/qdev/device_add", test_device_add);

What about naming these tests /qmp/...?

Again, test itself looks great on a brief look.

Andreas

> +
> +    return g_test_run();
> +}
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg