Re: [Qemu-devel] [PATCH for-2.8 2/2] tests: check-qom-proplist: add checks for cmdline-created objects

[Michael Roth]

Quoting Markus Armbruster (2016-12-07 05:10:47)
> "Daniel P. Berrange" <address@hidden> writes:
> 
> > On Tue, Dec 06, 2016 at 02:15:00PM -0600, Michael Roth wrote:
> >> check-qom-proplist originally added tests for verifying that
> >> object-creation helpers object_new_with_{props,propv} behaved in
> >> similar fashion to the "traditional" method involving setting each
> >> individual property separately after object creation rather than
> >> in via a single call.
> >> 
> >> Another similar "helper" for creating Objects exists in the form of
> >> objects specified via -object command-line parameters. By that
> >> rationale, we extend check-qom-proplist to include similar checks
> >> for command-line-created objects by employing the same
> >> qemu_opts_parse()-based parsing the vl.c employs.
> >> 
> >> This parser has a side-effect of parsing the object's options into
> >> a QemuOpt structure and registering this in the global QemuOptsList
> >> using the Object's ID. This can conflict with future Object instances
> >> that attempt to use the same ID if we don't ensure this is cleaned
> >> up as part of Object finalization, so we add specific checks for this
> >> scenario in addition to the normal sanity checks.
> >> 
> >> Suggested-by: Daniel Berrange <address@hidden>
> >> Cc: "Dr. David Alan Gilbert" <address@hidden>
> >> Cc: Markus Armbruster <address@hidden>
> >> Cc: Eric Blake <address@hidden>
> >> Cc: Daniel Berrange <address@hidden>
> >> Signed-off-by: Michael Roth <address@hidden>
> >> ---
> >>  tests/check-qom-proplist.c | 54 
> >> +++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 53 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> >> index a16cefc..087ce21 100644
> >> --- a/tests/check-qom-proplist.c
> >> +++ b/tests/check-qom-proplist.c
> >> @@ -23,6 +23,9 @@
> >>  #include "qapi/error.h"
> >>  #include "qom/object.h"
> >>  #include "qemu/module.h"
> >> +#include "qemu/option.h"
> >> +#include "qemu/config-file.h"
> >> +#include "qom/object_interfaces.h"
> >>  
> >>  
> >>  #define TYPE_DUMMY "qemu-dummy"
> >> @@ -162,6 +165,10 @@ static const TypeInfo dummy_info = {
> >>      .instance_finalize = dummy_finalize,
> >>      .class_size = sizeof(DummyObjectClass),
> >>      .class_init = dummy_class_init,
> >> +    .interfaces = (InterfaceInfo[]) {
> >> +        { TYPE_USER_CREATABLE },
> >> +        { }
> >> +    }
> >>  };
> >>  
> >>  
> >> @@ -291,7 +298,6 @@ static void dummy_backend_init(Object *obj)
> >>  {
> >>  }
> >>  
> >> -
> >>  static const TypeInfo dummy_dev_info = {
> >>      .name          = TYPE_DUMMY_DEV,
> >>      .parent        = TYPE_OBJECT,
> >> @@ -320,6 +326,14 @@ static const TypeInfo dummy_backend_info = {
> >>      .class_size = sizeof(DummyBackendClass),
> >>  };
> >>  
> >> +static QemuOptsList qemu_object_opts = {
> >> +    .name = "object",
> >> +    .implied_opt_name = "qom-type",
> >> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
> >> +    .desc = {
> >> +        { }
> >> +    },
> >> +};
> >>  
> >>  
> >>  static void test_dummy_createv(void)
> >> @@ -388,6 +402,43 @@ static void test_dummy_createlist(void)
> >>      object_unparent(OBJECT(dobj));
> >>  }
> >>  
> >> +static void test_dummy_createcmdl(void)
> >> +{
> >> +    QemuOpts *opts;
> >> +    DummyObject *dobj;
> >> +    Error *err = NULL;
> >> +    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);
> >> +    g_assert(err == NULL);
> >> +    g_assert(opts);
> >> +
> >> +    dobj = DUMMY_OBJECT(user_creatable_add_opts(opts, &err));
> >> +    g_assert(err == NULL);
> >> +    g_assert(dobj);
> >> +    g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
> >> +    g_assert(dobj->bv == true);
> >> +    g_assert(dobj->av == DUMMY_PLATYPUS);
> >> +
> >> +    user_creatable_del("dev0", &err);
> >> +    g_assert(err == NULL);
> >> +    error_free(err);
> >> +
> >> +    /* 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 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(qemu_opts_find(&qemu_object_opts, "dev0") == NULL);
> >> +}
> >> +
> >>  static void test_dummy_badenum(void)
> >>  {
> >>      Error *err = NULL;
> >> @@ -525,6 +576,7 @@ int main(int argc, char **argv)
> >>  
> >>      g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
> >>      g_test_add_func("/qom/proplist/createv", test_dummy_createv);
> >> +    g_test_add_func("/qom/proplist/createcmdline", test_dummy_createcmdl);
> >>      g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
> >>      g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
> >>      g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
> >
> > Reviewed-by: Daniel P. Berrange <address@hidden>
> >
> >
> > except that this should be squashed into the previous commit - tests should
> > always be in the same commit as the new code that they're verifying.
> 
> Not sure how particular we are about that.
> 
> When neither test nor fix is trivial, I personally like to commit the
> test first, defanged so it doesn't blow up make check, with a suitable
> FIXME comment.  The fix then resolves the FIXME.

I think this approach also makes sense here due to the fact that the
bulk of the checks added are pertaining to general sanity checks.
There's really only one assert here that checks the fix in question.
I'll switch it around and replace that check with a FIXME, then squash
the check into the fix patch.

>