Message ID | 1481822644-617-2-git-send-email-mdroth@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
I notice this pair of patches doesn't seem to have gone anywhere. WHile it's labelled as a monitor fix, it's all QOM stuff, so I don't think it should be going via me. Dave * Michael Roth (mdroth@linux.vnet.ibm.com) 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 > 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 include a FIXME stub to test > for this case, which will then be resolved in a subsequent patch. > > Suggested-by: Daniel Berrange <berrange@redhat.com> > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Cc: Daniel Berrange <berrange@redhat.com> > Cc: qemu-stable@nongnu.org > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > --- > tests/check-qom-proplist.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c > index a16cefc..e3f56ca 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 }, > + { } > + } > }; > > > @@ -320,6 +327,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 +403,45 @@ 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 need to > + * check for this in user_creatable_del() and remove the QemuOpts if > + * it is present. > + * > + * FIXME: add an assert to verify that the QemuOpts is cleaned up > + * once the corresponding cleanup code is added. > + */ > +} > + > static void test_dummy_badenum(void) > { > Error *err = NULL; > @@ -525,6 +579,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); > -- > 1.9.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > I notice this pair of patches doesn't seem to have gone anywhere. > WHile it's labelled as a monitor fix, it's all QOM stuff, so I don't > think it should be going via me. It's somewhere between QOM and QemuOpts. Andreas, please have a look. Feel free to ask me to take the patch through my tree. > Dave > > * Michael Roth (mdroth@linux.vnet.ibm.com) 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 >> 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 include a FIXME stub to test >> for this case, which will then be resolved in a subsequent patch. >> >> Suggested-by: Daniel Berrange <berrange@redhat.com> >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> Cc: Markus Armbruster <armbru@redhat.com> >> Cc: Eric Blake <eblake@redhat.com> >> Cc: Daniel Berrange <berrange@redhat.com> >> Cc: qemu-stable@nongnu.org >> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> --- >> tests/check-qom-proplist.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 55 insertions(+) >> >> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c >> index a16cefc..e3f56ca 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 }, >> + { } >> + } >> }; >> >> >> @@ -320,6 +327,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 +403,45 @@ 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 need to >> + * check for this in user_creatable_del() and remove the QemuOpts if >> + * it is present. >> + * >> + * FIXME: add an assert to verify that the QemuOpts is cleaned up >> + * once the corresponding cleanup code is added. >> + */ >> +} >> + >> static void test_dummy_badenum(void) >> { >> Error *err = NULL; >> @@ -525,6 +579,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); >> -- >> 1.9.1 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Markus Armbruster <armbru@redhat.com> writes: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > >> I notice this pair of patches doesn't seem to have gone anywhere. >> WHile it's labelled as a monitor fix, it's all QOM stuff, so I don't >> think it should be going via me. > > It's somewhere between QOM and QemuOpts. Andreas, please have a look. > Feel free to ask me to take the patch through my tree. I considered applying the series to qapi-next, and realized I'm not sure what the latest version is. I could peruse the archives, but that's work. Mike, would you mind resending your latest version?
Quoting Markus Armbruster (2017-05-31 12:05:16) > Markus Armbruster <armbru@redhat.com> writes: > > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > > >> I notice this pair of patches doesn't seem to have gone anywhere. > >> WHile it's labelled as a monitor fix, it's all QOM stuff, so I don't > >> think it should be going via me. > > > > It's somewhere between QOM and QemuOpts. Andreas, please have a look. > > Feel free to ask me to take the patch through my tree. > > I considered applying the series to qapi-next, and realized I'm not sure > what the latest version is. I could peruse the archives, but that's > work. Mike, would you mind resending your latest version? This version (v4) is the latest, but I've rebased/re-tested on master and re-submitted those as v5. Thanks!
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c index a16cefc..e3f56ca 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 }, + { } + } }; @@ -320,6 +327,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 +403,45 @@ 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 need to + * check for this in user_creatable_del() and remove the QemuOpts if + * it is present. + * + * FIXME: add an assert to verify that the QemuOpts is cleaned up + * once the corresponding cleanup code is added. + */ +} + static void test_dummy_badenum(void) { Error *err = NULL; @@ -525,6 +579,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);