Re: [PATCH 06/25] tests: convert check-qom-proplist to keyval

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> The command-line creation test is using QemuOpts.  Switch it to keyval,
> since all the -object command line options will follow
> qemu-storage-daemon and do the same.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

$ gdb tests/check-qom-proplist
[...]
(gdb) r
[...]
# random seed: R02S802948119789b5481f33f9842e3b5d1b
1..9
# Start of qom tests
# Start of proplist tests
ok 1 /qom/proplist/createlist
ok 2 /qom/proplist/createv
Unexpected error in find_list() at ../util/qemu-config.c:24:
There is no option group 'object'

Thread 1 "check-qom-propl" received signal SIGABRT, Aborted.
0x00007ffff7b839e5 in raise () from /lib64/libc.so.6
Missing separate debuginfos, use: dnf debuginfo-install glib2-2.64.6-1.fc32.x86_64 pcre-8.44-2.fc32.x86_64
(gdb) bt
#0  0x00007ffff7b839e5 in raise () from /lib64/libc.so.6
#1  0x00007ffff7b6c895 in abort () from /lib64/libc.so.6
#2  0x000055555557fe08 in error_handle_fatal (
    errp=0x5555555aa300 <error_abort>, err=0x5555555b9580)
    at ../util/error.c:40
#3  0x000055555557ff38 in error_setv (errp=0x5555555aa300 <error_abort>, 
    src=0x5555555914f1 "../util/qemu-config.c", line=24, 
    func=0x555555591ad0 <__func__.5> "find_list", 
    err_class=ERROR_CLASS_GENERIC_ERROR, 
    fmt=0x5555555914d3 "There is no option group '%s'", ap=0x7fffffffd690, 
    suffix=0x0) at ../util/error.c:73
#4  0x0000555555580116 in error_setg_internal (
    errp=0x5555555aa300 <error_abort>, 
    src=0x5555555914f1 "../util/qemu-config.c", line=24, 
    func=0x555555591ad0 <__func__.5> "find_list", 
    fmt=0x5555555914d3 "There is no option group '%s'") at ../util/error.c:97
#5  0x000055555556fdb4 in find_list (lists=0x5555555aa060 <vm_config_groups>, 
    group=0x55555558e97a "object", errp=0x5555555aa300 <error_abort>)
    at ../util/qemu-config.c:24
#6  0x0000555555570426 in qemu_find_opts_err (group=0x55555558e97a "object", 
    errp=0x5555555aa300 <error_abort>) at ../util/qemu-config.c:275
#7  0x000055555555f8bd in user_creatable_del (id=0x55555558e233 "dev0", 
    errp=0x5555555aa300 <error_abort>) at ../qom/object_interfaces.c:312
#8  0x000055555555dc8e in test_dummy_createcmdl ()
    at ../tests/check-qom-proplist.c:439
#9  0x00007ffff7ef429e in g_test_run_suite_internal ()
   from /lib64/libglib-2.0.so.0
#10 0x00007ffff7ef409b in g_test_run_suite_internal ()
   from /lib64/libglib-2.0.so.0
#11 0x00007ffff7ef409b in g_test_run_suite_internal ()
   from /lib64/libglib-2.0.so.0
#12 0x00007ffff7ef478a in g_test_run_suite () from /lib64/libglib-2.0.so.0
#13 0x00007ffff7ef47a5 in g_test_run () from /lib64/libglib-2.0.so.0
#14 0x000055555555e98c in main (argc=1, argv=0x7fffffffdcf8)
    at ../tests/check-qom-proplist.c:655

> ---
>  tests/check-qom-proplist.c | 58 +++++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index 1b76581980..8dba26fb3c 100644
> --- a/tests/check-qom-proplist.c
> +++ b/tests/check-qom-proplist.c
> @@ -21,6 +21,8 @@
>  #include "qemu/osdep.h"
>  
>  #include "qapi/error.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qobject.h"
>  #include "qom/object.h"

I expected this one to go.

>  #include "qemu/module.h"
>  #include "qemu/option.h"
> @@ -400,42 +402,58 @@ static void test_dummy_createlist(void)
>  
>  static void test_dummy_createcmdl(void)
>  {
> -    QemuOpts *opts;
> +    QDict *qdict;
>      DummyObject *dobj;
>      Error *err = NULL;
> +    bool help;
>      const char *params = TYPE_DUMMY \
>                           ",id=dev0," \
>                           "bv=yes,sv=Hiss hiss hiss,av=platypus";
>  
> -    qemu_add_opts(&qemu_object_opts);
> -    opts = qemu_opts_parse(&qemu_object_opts, params, true, &err);
> +    qdict = keyval_parse(params, "qom-type", &help, &err);

Removes the only use of qemu_object_opts.  You should remove
qemu_object_opts, too.

>      g_assert(err == NULL);
> -    g_assert(opts);
> +    g_assert(qdict);
> +    g_assert(!help);
>  
> -    dobj = DUMMY_OBJECT(user_creatable_add_opts(opts, &err));
> +    g_assert(user_creatable_add_dict(qdict, true, &err));
>      g_assert(err == NULL);
> +    qobject_unref(qdict);
> +
> +    dobj = DUMMY_OBJECT(object_resolve_path_component(object_get_objects_root(),
> +                                                      "dev0"));

Why does user_creatable_add_opts() return the object on success, null on
failure, but user_creatable_add_dict() only a rather less useful bool?

>      g_assert(dobj);
>      g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
>      g_assert(dobj->bv == true);
>      g_assert(dobj->av == DUMMY_PLATYPUS);
>  
> +    qdict = keyval_parse(params, "qom-type", &help, &err);

Why parse again?

> +    g_assert(!user_creatable_add_dict(qdict, true, &err));
> +    g_assert(err);

Use error_free_or_abort(&err) instead, and ...

> +    g_assert(object_resolve_path_component(object_get_objects_root(), "dev0")
> +             == OBJECT(dobj));
> +    qobject_unref(qdict);

... drop the next two lines:

> +    error_free(err);
> +    err = NULL;
> +
> +    qdict = keyval_parse(params, "qom-type", &help, &err);

And again?

>      user_creatable_del("dev0", &error_abort);
> +    g_assert(object_resolve_path_component(object_get_objects_root(), "dev0")
> +             == NULL);
>  
> -    object_unref(OBJECT(dobj));
> -
> -    /*
> -     * cmdline-parsing via qemu_opts_parse() results in a QemuOpts entry
> -     * corresponding to the Object's ID to be added to the QemuOptsList
> -     * for objects. To avoid having this entry conflict with future
> -     * Objects using the same ID (which can happen in cases where
> -     * qemu_opts_parse() is used to parse the object params, such as
> -     * with hmp_object_add() at the time of this comment), we need to
> -     * check for this in user_creatable_del() and remove the QemuOpts if
> -     * it is present.
> -     *
> -     * The below check ensures this works as expected.
> -     */
> -    g_assert_null(qemu_opts_find(&qemu_object_opts, "dev0"));
> +    g_assert(user_creatable_add_dict(qdict, true, &err));

Am I confused, or are you going from two to three creates?  Should this
be in a separate patch?

> +    g_assert(err == NULL);
> +    qobject_unref(qdict);
> +
> +    dobj = DUMMY_OBJECT(object_resolve_path_component(object_get_objects_root(),
> +                                                      "dev0"));
> +    g_assert(dobj);
> +    g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
> +    g_assert(dobj->bv == true);
> +    g_assert(dobj->av == DUMMY_PLATYPUS);
> +    g_assert(object_resolve_path_component(object_get_objects_root(), "dev0")
> +             == OBJECT(dobj));
> +
> +    object_unparent(OBJECT(dobj));
>  }
>  
>  static void test_dummy_badenum(void)