On 02/02/2016 05:57 AM, Daniel P. Berrange wrote: > The QMP monitor code has two helper methods object_add > and qmp_object_del that are called from several places > in the code (QMP, HMP and main emulator startup). > > The HMP and main emulator startup code also share > further logic that extracts the qom-type & id > values from a qdict. > > We soon need to use this logic from qemu-img, qemu-io > and qemu-nbd too, but don't want those to depend on > the monitor, nor do we want to duplicate the code.
Yay - merge conflicts with my work pending on Markus' qapi-next branch: https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00254.html > > To avoid this, move some code out of qmp.c and hmp.c > adding new methods to qom/object_interfaces.c > > - user_creatable_add - takes a QDict holding a full > object definition & instantiates it > - user_creatable_add_type - takes an ID, type name, > and QDict holding object properties & instantiates > it > - user_creatable_add_opts - takes a QemuOpts holding > a full object definition & instantiates it > - user_creatable_add_opts_foreach - variant on > user_creatable_add_opts which can be directly used > in conjunction with qemu_opts_foreach. > - user_creatable_del - takes an ID and deletes the > corresponding object > > The existing code is updated to use these new methods. > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > --- > hmp.c | 52 +++--------- > include/monitor/monitor.h | 3 - > include/qom/object_interfaces.h | 92 ++++++++++++++++++++ > qmp.c | 76 ++--------------- > qom/object_interfaces.c | 180 > ++++++++++++++++++++++++++++++++++++++++ > vl.c | 66 ++------------- > 6 files changed, 296 insertions(+), 173 deletions(-) diffstat is misleading, overall I think this is a nice cleanup. > > diff --git a/hmp.c b/hmp.c > index 54f2620..95930b0 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -29,6 +29,7 @@ > #include "qapi/string-output-visitor.h" > #include "qapi/util.h" > #include "qapi-visit.h" > +#include "qom/object_interfaces.h" > #include "ui/console.h" > #include "block/qapi.h" > #include "qemu-io.h" Any reason to name the file with _ instead of -? It looks funny in relation to the other three files in just this hunk... > +++ b/include/qom/object_interfaces.h > @@ -2,6 +2,8 @@ > #define OBJECT_INTERFACES_H > > #include "qom/object.h" > +#include "qapi/qmp/qdict.h" > +#include "qapi/visitor.h" ...Oh - the file is pre-existing. If we want to rename it now, it should be an independent patch. > +/** > + * user_creatable_add: > + * @qdict: the object definition > + * @v: the visitor > + * @errp: if an error occurs, a pointer to an area to store the error > + * > + * Create an instance of the user creatable object whose type, s/type,/type/ > + * is defined in @qdict by the 'qom-type' field, placing it > + * in the object composition tree with name provided by the > + * 'id' field. The remaining fields in @qdict are used to > + * initialize the object properties. > + * > + * Returns: the newly created object or NULL on error > + */ > +Object *user_creatable_add(const QDict *qdict, > + Visitor *v, Error **errp); > + > +/** > + * user_creatable_add_type: > + * @type: the object type name > + * @id: the unique ID for the object > + * @qdict: the object properties > + * @v: the visitor > + * @errp: if an error occurs, a pointer to an area to store the error > + * > + * Create an instance of the user creatable object @type, placing > + * it in the object composition tree with name @id, initializing > + * it with properties from @qdict Not the first time that I've noticed that you're not a fan of trailing '.' in paragraphs :) I won't hold it against you > + * > + * Returns: the newly created object or NULL on error > + */ > +Object *user_creatable_add_type(const char *type, const char *id, > + const QDict *qdict, > + Visitor *v, Error **errp); > + > +/** > + * user_creatable_add_opts: > + * @opts: the object definition > + * @errp: if an error occurs, a pointer to an area to store the error > + * > + * Create an instance of the user creatable object whose type, s/type,/type/ > @@ -701,27 +649,17 @@ void qmp_object_add(const char *type, const char *id, > } > > qiv = qmp_input_visitor_new(props); > - object_add(type, id, pdict, qmp_input_get_visitor(qiv), errp); > + obj = user_creatable_add_type(type, id, pdict, > + qmp_input_get_visitor(qiv), errp); > qmp_input_visitor_cleanup(qiv); > + if (obj) { > + object_unref(obj); > + } 'if' is redundant; object_unref(NULL) is safe. > +++ b/qom/object_interfaces.c > @@ -1,5 +1,8 @@ > #include "qom/object_interfaces.h" > #include "qemu/module.h" > +#include "qapi-visit.h" > +#include "qapi/qmp-output-visitor.h" > +#include "qapi/opts-visitor.h" > > void user_creatable_complete(Object *obj, Error **errp) > { > @@ -30,6 +33,183 @@ bool user_creatable_can_be_deleted(UserCreatable *uc, > Error **errp) > } > } > > + > +Object *user_creatable_add(const QDict *qdict, > + Visitor *v, Error **errp) > +{ > + char *type = NULL; > + char *id = NULL; > + Object *obj = NULL; > + Error *local_err = NULL, *end_err = NULL; > + QDict *pdict; > + > + pdict = qdict_clone_shallow(qdict); > + > + visit_start_struct(v, NULL, NULL, NULL, 0, &local_err); Yep, conflicts with my pending patches. One less NULL needed if mine land first. > + if (local_err) { > + goto out; > + } > + > + qdict_del(pdict, "qom-type"); > + visit_type_str(v, &type, "qom-type", &local_err); > + if (local_err) { > + goto out_visit; > + } > + > + qdict_del(pdict, "id"); > + visit_type_str(v, &id, "id", &local_err); > + if (local_err) { > + goto out_visit; > + } > + > + obj = user_creatable_add_type(type, id, pdict, v, &local_err); > + if (local_err) { > + goto out_visit; > + } > + > + out_visit: > + visit_end_struct(v, &end_err); > + if (end_err) { > + if (!local_err) { > + error_propagate(&local_err, end_err); > + } else { > + error_free(end_err); > + } This 'if/else' can be simplified to: error_propagate(&local_err, end_err); > + if (obj) { > + user_creatable_del(id, NULL); > + } > + goto out; > + } > + > +out: > + QDECREF(pdict); > + g_free(id); > + g_free(type); > + if (local_err) { > + error_propagate(errp, local_err); > + if (obj) { > + object_unref(obj); > + } > + return NULL; > + } > + return obj; > +} > + > + > +Object *user_creatable_add_opts(QemuOpts *opts, Error **errp) > +{ > + OptsVisitor *ov; > + QDict *pdict; > + Object *obj = NULL; > + > + ov = opts_visitor_new(opts); > + pdict = qemu_opts_to_qdict(opts, NULL); > + > + obj = user_creatable_add(pdict, opts_get_visitor(ov), errp); > + opts_visitor_cleanup(ov); > + QDECREF(pdict); > + return obj; > +} Nice way to share code. > +void user_creatable_del(const char *id, Error **errp) > +{ > + Object *container; > + Object *obj; > + > + container = object_get_objects_root(); > + obj = object_resolve_path_component(container, id); > + if (!obj) { > + error_setg(errp, "object id not found"); Might be worth a "%s",id in there somewhere. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
