[PATCH 0/1] tests/qtest: add testing coverage of -readconfig

[PATCH 0/1] tests/qtest: add testing coverage of -readconfig

[Daniel P. Berrangé]

I reviewing Paolo's latest fix for -readconfig I noticed that we have
absolutely zero coverage of -readconfig under tests/, which makes it
way too easy for regressions to slip in.

This following patch doesn't do much, but it tests the last three
regressions we've had and gives a starting point for more testing
of -readconfig.

Daniel P. Berrangé (1):
  tests/qtest: add scenario for -readconfig handling

 tests/qtest/meson.build       |   1 +
 tests/qtest/readconfig-test.c | 195 ++++++++++++++++++++++++++++++++++
 2 files changed, 196 insertions(+)
 create mode 100644 tests/qtest/readconfig-test.c

-- 
2.37.1

[PATCH 1/1] tests/qtest: add scenario for -readconfig handling

[Daniel P. Berrangé]

This test of -readconfig validates the last three regressions we
have fixed with -readconfig:

 * Interpretation of memory size units as MiB not bytes
 * Allow use of [spice]
 * Allow use of [object]

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qtest/meson.build       |   1 +
 tests/qtest/readconfig-test.c | 195 ++++++++++++++++++++++++++++++++++
 2 files changed, 196 insertions(+)
 create mode 100644 tests/qtest/readconfig-test.c

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 3a474010e4..be4b30dea2 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -26,6 +26,7 @@ qtests_generic = [
   'qom-test',
   'test-hmp',
   'qos-test',
+  'readconfig-test',
 ]
 if config_host.has_key('CONFIG_MODULES')
   qtests_generic += [ 'modules-test' ]
diff --git a/tests/qtest/readconfig-test.c b/tests/qtest/readconfig-test.c
new file mode 100644
index 0000000000..2e604d7c2d
--- /dev/null
+++ b/tests/qtest/readconfig-test.c
@@ -0,0 +1,195 @@
+/*
+ * Validate -readconfig
+ *
+ * Copyright (c) 2022 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qapi/error.h"
+#include "qapi/qapi-visit-machine.h"
+#include "qapi/qapi-visit-qom.h"
+#include "qapi/qapi-visit-ui.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qlist.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qmp/qstring.h"
+#include "qemu/units.h"
+
+static QTestState *qtest_init_with_config(const char *cfgdata)
+{
+    GError *error = NULL;
+    g_autofree char *args = NULL;
+    int cfgfd = -1;
+    g_autofree char *cfgpath = NULL;
+    QTestState *qts;
+    ssize_t ret;
+
+    cfgfd = g_file_open_tmp("readconfig-test-XXXXXX", &cfgpath, &error);
+    g_assert_no_error(error);
+    g_assert_cmpint(cfgfd, >=, 0);
+
+    ret = qemu_write_full(cfgfd, cfgdata, strlen(cfgdata));
+    if (ret < 0) {
+        unlink(cfgpath);
+    }
+    g_assert_cmpint(ret, ==, strlen(cfgdata));
+
+    close(cfgfd);
+
+    args = g_strdup_printf("-nodefaults -machine none -readconfig %s", cfgpath);
+
+    qts = qtest_init(args);
+
+    unlink(cfgpath);
+
+    return qts;
+}
+
+static void test_x86_memdev_resp(QObject *res)
+{
+    Visitor *v;
+    g_autoptr(MemdevList) memdevs = NULL;
+    Memdev *memdev;
+
+    g_assert(res);
+    v = qobject_input_visitor_new(res);
+    visit_type_MemdevList(v, NULL, &memdevs, &error_abort);
+
+    g_assert(memdevs);
+    g_assert(memdevs->value);
+    g_assert(!memdevs->next);
+
+    memdev = memdevs->value;
+    g_assert_cmpstr(memdev->id, ==, "ram");
+    g_assert_cmpint(memdev->size, ==, 200 * MiB);
+
+    visit_free(v);
+}
+
+static void test_x86_memdev(void)
+{
+    QDict *resp;
+    QTestState *qts;
+    const char *cfgdata =
+        "[memory]\n"
+        "size = \"200\"";
+
+    qts = qtest_init_with_config(cfgdata);
+   /* Test valid command */
+    resp = qtest_qmp(qts, "{ 'execute': 'query-memdev' }");
+    test_x86_memdev_resp(qdict_get(resp, "return"));
+    qobject_unref(resp);
+
+    qtest_quit(qts);
+}
+
+
+#ifdef CONFIG_SPICE
+static void test_spice_resp(QObject *res)
+{
+    Visitor *v;
+    g_autoptr(SpiceInfo) spice = NULL;
+
+    g_assert(res);
+    v = qobject_input_visitor_new(res);
+    visit_type_SpiceInfo(v, "spcie", &spice, &error_abort);
+
+    g_assert(spice);
+    g_assert(spice->enabled);
+
+    visit_free(v);
+}
+
+static void test_spice(void)
+{
+    QDict *resp;
+    QTestState *qts;
+    const char *cfgdata =
+        "[spice]\n"
+        "disable-ticketing = \"on\"\n"
+        "unix = \"on\"\n";
+
+    qts = qtest_init_with_config(cfgdata);
+   /* Test valid command */
+    resp = qtest_qmp(qts, "{ 'execute': 'query-spice' }");
+    test_spice_resp(qdict_get(resp, "return"));
+    qobject_unref(resp);
+
+    qtest_quit(qts);
+}
+#endif
+
+static void test_object_rng_resp(QObject *res)
+{
+    Visitor *v;
+    g_autoptr(ObjectPropertyInfoList) objs = NULL;
+    ObjectPropertyInfoList *tmp;
+    ObjectPropertyInfo *obj;
+    bool seen_rng = false;
+
+    g_assert(res);
+    v = qobject_input_visitor_new(res);
+    visit_type_ObjectPropertyInfoList(v, NULL, &objs, &error_abort);
+
+    g_assert(objs);
+    tmp = objs;
+    while (tmp) {
+        g_assert(tmp->value);
+
+        obj = tmp->value;
+        if (g_str_equal(obj->name, "rng0") &&
+            g_str_equal(obj->type, "child<rng-builtin>")) {
+            seen_rng = true;
+        }
+
+        tmp = tmp->next;
+    }
+
+    g_assert(seen_rng);
+
+    visit_free(v);
+}
+
+static void test_object_rng(void)
+{
+    QDict *resp;
+    QTestState *qts;
+    const char *cfgdata =
+        "[object]\n"
+        "qom-type = \"rng-builtin\"\n"
+        "id = \"rng0\"\n";
+
+    qts = qtest_init_with_config(cfgdata);
+   /* Test valid command */
+    resp = qtest_qmp(qts,
+                     "{ 'execute': 'qom-list',"
+                     "  'arguments': {'path': '/objects' }}");
+    test_object_rng_resp(qdict_get(resp, "return"));
+    qobject_unref(resp);
+
+    qtest_quit(qts);
+}
+
+int main(int argc, char *argv[])
+{
+    const char *arch;
+    g_test_init(&argc, &argv, NULL);
+
+    arch = qtest_get_arch();
+
+    if (g_str_equal(arch, "i386") ||
+        g_str_equal(arch, "x86_64")) {
+        qtest_add_func("readconfig/x86/memdev", test_x86_memdev);
+    }
+#ifdef CONFIG_SPICE
+    qtest_add_func("readconfig/spice", test_spice);
+#endif
+
+    qtest_add_func("readconfig/object-rng", test_object_rng);
+
+    return g_test_run();
+}
-- 
2.37.1

Re: [PATCH 1/1] tests/qtest: add scenario for -readconfig handling

[Thomas Huth]

On 05/08/2022 13.55, Daniel P. Berrangé wrote:
> This test of -readconfig validates the last three regressions we
> have fixed with -readconfig:
> 
>   * Interpretation of memory size units as MiB not bytes
>   * Allow use of [spice]
>   * Allow use of [object]

Good idea!

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qtest/meson.build       |   1 +
>   tests/qtest/readconfig-test.c | 195 ++++++++++++++++++++++++++++++++++
>   2 files changed, 196 insertions(+)
>   create mode 100644 tests/qtest/readconfig-test.c
> 
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 3a474010e4..be4b30dea2 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -26,6 +26,7 @@ qtests_generic = [
>     'qom-test',
>     'test-hmp',
>     'qos-test',
> +  'readconfig-test',
>   ]
>   if config_host.has_key('CONFIG_MODULES')
>     qtests_generic += [ 'modules-test' ]
> diff --git a/tests/qtest/readconfig-test.c b/tests/qtest/readconfig-test.c
> new file mode 100644
> index 0000000000..2e604d7c2d
> --- /dev/null
> +++ b/tests/qtest/readconfig-test.c
> @@ -0,0 +1,195 @@
> +/*
> + * Validate -readconfig
> + *
> + * Copyright (c) 2022 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-visit-machine.h"
> +#include "qapi/qapi-visit-qom.h"
> +#include "qapi/qapi-visit-ui.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qlist.h"
> +#include "qapi/qobject-input-visitor.h"
> +#include "qapi/qmp/qstring.h"
> +#include "qemu/units.h"
> +
> +static QTestState *qtest_init_with_config(const char *cfgdata)
> +{
> +    GError *error = NULL;
> +    g_autofree char *args = NULL;
> +    int cfgfd = -1;
> +    g_autofree char *cfgpath = NULL;
> +    QTestState *qts;
> +    ssize_t ret;
> +
> +    cfgfd = g_file_open_tmp("readconfig-test-XXXXXX", &cfgpath, &error);
> +    g_assert_no_error(error);
> +    g_assert_cmpint(cfgfd, >=, 0);
> +
> +    ret = qemu_write_full(cfgfd, cfgdata, strlen(cfgdata));
> +    if (ret < 0) {
> +        unlink(cfgpath);
> +    }
> +    g_assert_cmpint(ret, ==, strlen(cfgdata));
> +
> +    close(cfgfd);

Maybe move the close() before the "if (ret < 0)"

> +    args = g_strdup_printf("-nodefaults -machine none -readconfig %s", cfgpath);
> +
> +    qts = qtest_init(args);
> +
> +    unlink(cfgpath);
> +
> +    return qts;
> +}
> +
> +static void test_x86_memdev_resp(QObject *res)
> +{
> +    Visitor *v;
> +    g_autoptr(MemdevList) memdevs = NULL;
> +    Memdev *memdev;
> +
> +    g_assert(res);
> +    v = qobject_input_visitor_new(res);
> +    visit_type_MemdevList(v, NULL, &memdevs, &error_abort);
> +
> +    g_assert(memdevs);
> +    g_assert(memdevs->value);
> +    g_assert(!memdevs->next);
> +
> +    memdev = memdevs->value;
> +    g_assert_cmpstr(memdev->id, ==, "ram");
> +    g_assert_cmpint(memdev->size, ==, 200 * MiB);
> +
> +    visit_free(v);
> +}
> +
> +static void test_x86_memdev(void)
> +{
> +    QDict *resp;
> +    QTestState *qts;
> +    const char *cfgdata =
> +        "[memory]\n"
> +        "size = \"200\"";
> +
> +    qts = qtest_init_with_config(cfgdata);
> +   /* Test valid command */

Comment has bad indentation.

> +    resp = qtest_qmp(qts, "{ 'execute': 'query-memdev' }");
> +    test_x86_memdev_resp(qdict_get(resp, "return"));
> +    qobject_unref(resp);
> +
> +    qtest_quit(qts);
> +}
> +
> +
> +#ifdef CONFIG_SPICE
> +static void test_spice_resp(QObject *res)
> +{
> +    Visitor *v;
> +    g_autoptr(SpiceInfo) spice = NULL;
> +
> +    g_assert(res);
> +    v = qobject_input_visitor_new(res);
> +    visit_type_SpiceInfo(v, "spcie", &spice, &error_abort);

That "spcie" looks like a typo?

> +    g_assert(spice);
> +    g_assert(spice->enabled);
> +
> +    visit_free(v);
> +}
> +
> +static void test_spice(void)
> +{
> +    QDict *resp;
> +    QTestState *qts;
> +    const char *cfgdata =
> +        "[spice]\n"
> +        "disable-ticketing = \"on\"\n"
> +        "unix = \"on\"\n";
> +
> +    qts = qtest_init_with_config(cfgdata);
> +   /* Test valid command */

Bad indentation again.

> +    resp = qtest_qmp(qts, "{ 'execute': 'query-spice' }");
> +    test_spice_resp(qdict_get(resp, "return"));
> +    qobject_unref(resp);
> +
> +    qtest_quit(qts);
> +}
> +#endif
> +
> +static void test_object_rng_resp(QObject *res)
> +{
> +    Visitor *v;
> +    g_autoptr(ObjectPropertyInfoList) objs = NULL;
> +    ObjectPropertyInfoList *tmp;
> +    ObjectPropertyInfo *obj;
> +    bool seen_rng = false;
> +
> +    g_assert(res);
> +    v = qobject_input_visitor_new(res);
> +    visit_type_ObjectPropertyInfoList(v, NULL, &objs, &error_abort);
> +
> +    g_assert(objs);
> +    tmp = objs;
> +    while (tmp) {
> +        g_assert(tmp->value);
> +
> +        obj = tmp->value;
> +        if (g_str_equal(obj->name, "rng0") &&
> +            g_str_equal(obj->type, "child<rng-builtin>")) {
> +            seen_rng = true;

Do a "break;" here to speed things up?

> +        }
> +
> +        tmp = tmp->next;
> +    }
> +
> +    g_assert(seen_rng);
> +
> +    visit_free(v);
> +}
> +
> +static void test_object_rng(void)
> +{
> +    QDict *resp;
> +    QTestState *qts;
> +    const char *cfgdata =
> +        "[object]\n"
> +        "qom-type = \"rng-builtin\"\n"
> +        "id = \"rng0\"\n";
> +
> +    qts = qtest_init_with_config(cfgdata);
> +   /* Test valid command */

Bad indentation again.

> +    resp = qtest_qmp(qts,
> +                     "{ 'execute': 'qom-list',"
> +                     "  'arguments': {'path': '/objects' }}");
> +    test_object_rng_resp(qdict_get(resp, "return"));
> +    qobject_unref(resp);
> +
> +    qtest_quit(qts);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    const char *arch;
> +    g_test_init(&argc, &argv, NULL);
> +
> +    arch = qtest_get_arch();
> +
> +    if (g_str_equal(arch, "i386") ||
> +        g_str_equal(arch, "x86_64")) {
> +        qtest_add_func("readconfig/x86/memdev", test_x86_memdev);
> +    }
> +#ifdef CONFIG_SPICE
> +    qtest_add_func("readconfig/spice", test_spice);
> +#endif
> +
> +    qtest_add_func("readconfig/object-rng", test_object_rng);
> +
> +    return g_test_run();
> +}

  Thomas

Re: [PATCH 1/1] tests/qtest: add scenario for -readconfig handling

[Daniel P. Berrangé]

On Mon, Aug 08, 2022 at 07:55:50AM +0200, Thomas Huth wrote:
> On 05/08/2022 13.55, Daniel P. Berrangé wrote:
> > This test of -readconfig validates the last three regressions we
> > have fixed with -readconfig:
> > 
> >   * Interpretation of memory size units as MiB not bytes
> >   * Allow use of [spice]
> >   * Allow use of [object]
> 
> Good idea!
> 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/qtest/meson.build       |   1 +
> >   tests/qtest/readconfig-test.c | 195 ++++++++++++++++++++++++++++++++++
> >   2 files changed, 196 insertions(+)
> >   create mode 100644 tests/qtest/readconfig-test.c
> > 
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> > index 3a474010e4..be4b30dea2 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -26,6 +26,7 @@ qtests_generic = [
> >     'qom-test',
> >     'test-hmp',
> >     'qos-test',
> > +  'readconfig-test',
> >   ]
> >   if config_host.has_key('CONFIG_MODULES')
> >     qtests_generic += [ 'modules-test' ]
> > diff --git a/tests/qtest/readconfig-test.c b/tests/qtest/readconfig-test.c
> > new file mode 100644
> > index 0000000000..2e604d7c2d
> > --- /dev/null
> > +++ b/tests/qtest/readconfig-test.c
> > @@ -0,0 +1,195 @@
> > +/*
> > + * Validate -readconfig
> > + *
> > + * Copyright (c) 2022 Red Hat, Inc.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "libqtest.h"
> > +#include "qapi/error.h"
> > +#include "qapi/qapi-visit-machine.h"
> > +#include "qapi/qapi-visit-qom.h"
> > +#include "qapi/qapi-visit-ui.h"
> > +#include "qapi/qmp/qdict.h"
> > +#include "qapi/qmp/qlist.h"
> > +#include "qapi/qobject-input-visitor.h"
> > +#include "qapi/qmp/qstring.h"
> > +#include "qemu/units.h"
> > +
> > +static QTestState *qtest_init_with_config(const char *cfgdata)
> > +{
> > +    GError *error = NULL;
> > +    g_autofree char *args = NULL;
> > +    int cfgfd = -1;
> > +    g_autofree char *cfgpath = NULL;
> > +    QTestState *qts;
> > +    ssize_t ret;
> > +
> > +    cfgfd = g_file_open_tmp("readconfig-test-XXXXXX", &cfgpath, &error);
> > +    g_assert_no_error(error);
> > +    g_assert_cmpint(cfgfd, >=, 0);
> > +
> > +    ret = qemu_write_full(cfgfd, cfgdata, strlen(cfgdata));
> > +    if (ret < 0) {
> > +        unlink(cfgpath);
> > +    }
> > +    g_assert_cmpint(ret, ==, strlen(cfgdata));
> > +
> > +    close(cfgfd);
> 
> Maybe move the close() before the "if (ret < 0)"
> 
> > +    args = g_strdup_printf("-nodefaults -machine none -readconfig %s", cfgpath);
> > +
> > +    qts = qtest_init(args);
> > +
> > +    unlink(cfgpath);
> > +
> > +    return qts;
> > +}
> > +
> > +static void test_x86_memdev_resp(QObject *res)
> > +{
> > +    Visitor *v;
> > +    g_autoptr(MemdevList) memdevs = NULL;
> > +    Memdev *memdev;
> > +
> > +    g_assert(res);
> > +    v = qobject_input_visitor_new(res);
> > +    visit_type_MemdevList(v, NULL, &memdevs, &error_abort);
> > +
> > +    g_assert(memdevs);
> > +    g_assert(memdevs->value);
> > +    g_assert(!memdevs->next);
> > +
> > +    memdev = memdevs->value;
> > +    g_assert_cmpstr(memdev->id, ==, "ram");
> > +    g_assert_cmpint(memdev->size, ==, 200 * MiB);
> > +
> > +    visit_free(v);
> > +}
> > +
> > +static void test_x86_memdev(void)
> > +{
> > +    QDict *resp;
> > +    QTestState *qts;
> > +    const char *cfgdata =
> > +        "[memory]\n"
> > +        "size = \"200\"";
> > +
> > +    qts = qtest_init_with_config(cfgdata);
> > +   /* Test valid command */
> 
> Comment has bad indentation.
> 
> > +    resp = qtest_qmp(qts, "{ 'execute': 'query-memdev' }");
> > +    test_x86_memdev_resp(qdict_get(resp, "return"));
> > +    qobject_unref(resp);
> > +
> > +    qtest_quit(qts);
> > +}
> > +
> > +
> > +#ifdef CONFIG_SPICE
> > +static void test_spice_resp(QObject *res)
> > +{
> > +    Visitor *v;
> > +    g_autoptr(SpiceInfo) spice = NULL;
> > +
> > +    g_assert(res);
> > +    v = qobject_input_visitor_new(res);
> > +    visit_type_SpiceInfo(v, "spcie", &spice, &error_abort);
> 
> That "spcie" looks like a typo?

Yes, but I believe this argument is ignored by the visitor in this
scenario - it didn't fail for me at least.

Regardless, sHould be fixed of course


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/1] tests/qtest: add scenario for -readconfig handling

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Aug 08, 2022 at 07:55:50AM +0200, Thomas Huth wrote:
>> On 05/08/2022 13.55, Daniel P. Berrangé wrote:
>> > This test of -readconfig validates the last three regressions we
>> > have fixed with -readconfig:
>> > 
>> >   * Interpretation of memory size units as MiB not bytes
>> >   * Allow use of [spice]
>> >   * Allow use of [object]
>> 
>> Good idea!
>> 
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

[...]

>> > diff --git a/tests/qtest/readconfig-test.c b/tests/qtest/readconfig-test.c
>> > new file mode 100644
>> > index 0000000000..2e604d7c2d
>> > --- /dev/null
>> > +++ b/tests/qtest/readconfig-test.c

[...]

>> > +#ifdef CONFIG_SPICE
>> > +static void test_spice_resp(QObject *res)
>> > +{
>> > +    Visitor *v;
>> > +    g_autoptr(SpiceInfo) spice = NULL;
>> > +
>> > +    g_assert(res);
>> > +    v = qobject_input_visitor_new(res);
>> > +    visit_type_SpiceInfo(v, "spcie", &spice, &error_abort);
>> 
>> That "spcie" looks like a typo?
>
> Yes, but I believe this argument is ignored by the visitor in this
> scenario - it didn't fail for me at least.

include/qapi/visitor.h:

 * The @name parameter of visit_type_FOO() describes the relation
 * between this QAPI value and its parent container.  When visiting
 * the root of a tree, @name is ignored; when visiting a member of an
 * object, @name is the key associated with the value; when visiting a
 * member of a list, @name is NULL; and when visiting the member of an
 * alternate, @name should equal the name used for visiting the
 * alternate.

> Regardless, sHould be fixed of course
>
>
> With regards,
> Daniel