From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37426) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQkWr-0007C4-I1 for qemu-devel@nongnu.org; Tue, 02 Feb 2016 18:38:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQkWp-00081Z-Ug for qemu-devel@nongnu.org; Tue, 02 Feb 2016 18:38:37 -0500 References: <1454417864-18774-1-git-send-email-berrange@redhat.com> <1454417864-18774-2-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <56B13DF4.5010006@redhat.com> Date: Tue, 2 Feb 2016 16:38:28 -0700 MIME-Version: 1.0 In-Reply-To: <1454417864-18774-2-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="R1hwAkTlQ5ravgtdxNi0iLvSUrp5LIsLO" Subject: Re: [Qemu-devel] [PATCH v5 01/10] qom: add helpers for UserCreatable object types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Kevin Wolf , Paolo Bonzini , Markus Armbruster , qemu-block@nongnu.org, =?UTF-8?Q?Andreas_F=c3=a4rber?= This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --R1hwAkTlQ5ravgtdxNi0iLvSUrp5LIsLO Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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). >=20 > The HMP and main emulator startup code also share > further logic that extracts the qom-type & id > values from a qdict. >=20 > 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 >=20 > To avoid this, move some code out of qmp.c and hmp.c > adding new methods to qom/object_interfaces.c >=20 > - 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 >=20 > The existing code is updated to use these new methods. >=20 > Signed-off-by: Daniel P. Berrange > --- > 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. >=20 > 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 > =20 > #include "qom/object.h" > +#include "qapi/qmp/qdict.h" > +#include "qapi/visitor.h" =2E..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, > } > =20 > qiv =3D qmp_input_visitor_new(props); > - object_add(type, id, pdict, qmp_input_get_visitor(qiv), errp); > + obj =3D 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" > =20 > void user_creatable_complete(Object *obj, Error **errp) > { > @@ -30,6 +33,183 @@ bool user_creatable_can_be_deleted(UserCreatable *u= c, Error **errp) > } > } > =20 > + > +Object *user_creatable_add(const QDict *qdict, > + Visitor *v, Error **errp) > +{ > + char *type =3D NULL; > + char *id =3D NULL; > + Object *obj =3D NULL; > + Error *local_err =3D NULL, *end_err =3D NULL; > + QDict *pdict; > + > + pdict =3D 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 =3D 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 =3D NULL; > + > + ov =3D opts_visitor_new(opts); > + pdict =3D qemu_opts_to_qdict(opts, NULL); > + > + obj =3D 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 =3D object_get_objects_root(); > + obj =3D object_resolve_path_component(container, id); > + if (!obj) { > + error_setg(errp, "object id not found"); Might be worth a "%s",id in there somewhere. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --R1hwAkTlQ5ravgtdxNi0iLvSUrp5LIsLO Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWsT30AAoJEKeha0olJ0NqK2YIAIzthJQc0UW+gX5nW9bXh37S uyoaxV/i5EfZDEbR0RQ9CPKQSWt+cAmjwRJkeIsTVWjE1w2AFozcGWUYKz2N7Jj4 ooklP7R9jEAZW8GaVwinm7+IPsCYdaIVeq94Aa/SPjragfYi9Rb2WT49B6FY7C/3 js1aAS83jb+9JL0aoR7xwDS66S1Fo8NhG5248jawcMXnR9HYR2aIPb4jfWGy2VbN yqNhCyxNRc8TinD4M2QTZJYfi1pJm0GXWC/lHAYSSiDI+fwFm7xz0AKzJGGmZ2n0 F00s/PC+W/05GQCuOgYmapjJnIqxSxMl1O3/UB83YWa7Aw6Fx9+hHpXmuAfnPDg= =hVIH -----END PGP SIGNATURE----- --R1hwAkTlQ5ravgtdxNi0iLvSUrp5LIsLO--