From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60911) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwuDG-00025h-6g for qemu-devel@nongnu.org; Tue, 17 Jun 2014 10:18:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WwuD7-0006HU-OH for qemu-devel@nongnu.org; Tue, 17 Jun 2014 10:18:14 -0400 Received: from cantor2.suse.de ([195.135.220.15]:33213 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwuD7-0006HM-EH for qemu-devel@nongnu.org; Tue, 17 Jun 2014 10:18:05 -0400 Message-ID: <53A04E1C.5080105@suse.de> Date: Tue, 17 Jun 2014 16:18:04 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1402505369-12526-1-git-send-email-pbonzini@redhat.com> <1402505369-12526-2-git-send-email-pbonzini@redhat.com> In-Reply-To: <1402505369-12526-2-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/5] qom: add a generic mechanism to resolve paths List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: mtosatti@redhat.com, stefanha@redhat.com Am 11.06.2014 18:49, schrieb Paolo Bonzini: > It may be desirable to have custom link<> properties that do more > than just store an object. Even the addition of a "check" > function is not enough if setting the link has side effects > or if a non-standard reference counting is preferrable. >=20 > Avoid the assumption that the opaque field of a link<> is a > LinkProperty struct, by adding a generic "resolve" callback > to ObjectProperty. This fixes aliases of link properties. >=20 > Signed-off-by: Paolo Bonzini > --- > include/qom/object.h | 49 ++++++++++++++++++++++++++++++++++++++++++++= ++ > qom/object.c | 55 ++++++++++++++++++++++++++++++++++----------= -------- > 2 files changed, 85 insertions(+), 19 deletions(-) After reading through multiple times, this does seem to make sense... ;) > diff --git a/include/qom/object.h b/include/qom/object.h > index a641dcd..f8ab845 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -304,6 +304,23 @@ typedef void (ObjectPropertyAccessor)(Object *obj, > Error **errp); > =20 > /** > + * ObjectPropertyResolve: > + * @obj: the object that owns the property > + * @opaque: the opaque registered with the property > + * @part: the name of the property > + * > + * If @path is the path that led to @obj, the function should > + * return the Object corresponding to "@path/@part". If #NULL > + * is returned, "@path/@part" is not a valid object path. > + * > + * The returned object can also be used as a starting point > + * to resolve a relative path starting with "@part". Minor: I would propose something like: * Resolves the #Object corresponding to property @part. * * The returned object can also be used as a starting point * to resolve a relative path starting with "@part". * * Returns: If @path is the path that led to @obj, the function * returns the #Object corresponding to "@path/@part". * If "@path/@part" is not a valid object path, it returns #NULL. I.e. inverting the two sentences to fit into a gtk-doc "Returns:" section, and either Object -> #Object or object. > + */ > +typedef Object *(ObjectPropertyResolve)(Object *obj, > + void *opaque, > + const char *part); > + > +/** > * ObjectPropertyRelease: > * @obj: the object that owns the property > * @name: the name of the property > @@ -321,6 +338,7 @@ typedef struct ObjectProperty > gchar *type; > ObjectPropertyAccessor *get; > ObjectPropertyAccessor *set; > + ObjectPropertyResolve *resolve; > ObjectPropertyRelease *release; > void *opaque; > =20 > @@ -769,6 +787,37 @@ void object_ref(Object *obj); > void object_unref(Object *obj); > =20 > /** > + * object_property_add_full: > + * @obj: the object to add a property to > + * @name: the name of the property. This can contain any character ex= cept for > + * a forward slash. In general, you should use hyphens '-' instead o= f > + * underscores '_' when naming properties. > + * @type: the type name of the property. This namespace is pretty loo= sely > + * defined. Sub namespaces are constructed by using a prefix and th= en > + * to angle brackets. For instance, the type 'virtio-net-pci' in th= e > + * 'link' namespace would be 'link'. > + * @get: The getter to be called to read a property. If this is NULL,= then > + * the property cannot be read. > + * @set: the setter to be called to write a property. If this is NULL= , > + * then the property cannot be written. > + * @resolve: called when the property name is used as part of an objec= t > + * path. This is meant for cases when you want to have custom link > + * properties. If it is NULL, the property name cannot be used as p= art > + * of a valid object path. > + * @release: called when the property is removed from the object. Thi= s is > + * meant to allow a property to free its opaque upon object > + * destruction. This may be NULL. > + * @opaque: an opaque pointer to pass to the callbacks for the propert= y > + * @errp: returns an error if this function fails > + */ > +void object_property_add_full(Object *obj, const char *name, const cha= r *type, > + ObjectPropertyAccessor *get, > + ObjectPropertyAccessor *set, > + ObjectPropertyResolve *resolve, > + ObjectPropertyRelease *release, > + void *opaque, Error **errp); > + > +/** > * object_property_add: > * @obj: the object to add a property to > * @name: the name of the property. This can contain any character ex= cept for I'm a bit concerned about the duplication going on here, e.g. of the forbidden characters. I think we should either just add the new argument to object_property_add() and add NULL arguments at old call sites as done before, or we should (to avoid future _really_full, _really_really_full versions) return the resulting ObjectProperty * for modification by the caller (in this case: ->resolve =3D foo). > diff --git a/qom/object.c b/qom/object.c > index e42b254..fcdd0da 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -355,11 +355,6 @@ static inline bool object_property_is_child(Object= Property *prop) > return strstart(prop->type, "child<", NULL); > } > =20 > -static inline bool object_property_is_link(ObjectProperty *prop) > -{ > - return strstart(prop->type, "link<", NULL); > -} > - (Scratch my earlier comment re macros, this is the function I meant.) > static void object_property_del_all(Object *obj) > { > while (!QTAILQ_EMPTY(&obj->properties)) { > @@ -727,9 +722,10 @@ void object_unref(Object *obj) > } > } > =20 > -void object_property_add(Object *obj, const char *name, const char *ty= pe, > +void object_property_add_full(Object *obj, const char *name, const cha= r *type, > ObjectPropertyAccessor *get, > ObjectPropertyAccessor *set, > + ObjectPropertyResolve *resolve, > ObjectPropertyRelease *release, > void *opaque, Error **errp) > { > @@ -751,12 +747,23 @@ void object_property_add(Object *obj, const char = *name, const char *type, > =20 > prop->get =3D get; > prop->set =3D set; > + prop->resolve =3D resolve; > prop->release =3D release; > prop->opaque =3D opaque; > =20 > QTAILQ_INSERT_TAIL(&obj->properties, prop, node); > } > =20 > +void object_property_add(Object *obj, const char *name, const char *ty= pe, > + ObjectPropertyAccessor *get, > + ObjectPropertyAccessor *set, > + ObjectPropertyRelease *release, > + void *opaque, Error **errp) > +{ > + object_property_add_full(obj, name, type, get, set, NULL, release, > + opaque, errp); > +} > + > ObjectProperty *object_property_find(Object *obj, const char *name, > Error **errp) > { > @@ -993,6 +1000,11 @@ static void object_get_child_property(Object *obj= , Visitor *v, void *opaque, > g_free(path); > } > =20 > +static Object *object_resolve_child_property(Object *parent, void *opa= que, const gchar *part) > +{ > + return opaque; > +} > + > static void object_finalize_child_property(Object *obj, const char *na= me, > void *opaque) > { > @@ -1009,8 +1021,9 @@ void object_property_add_child(Object *obj, const= char *name, > =20 > type =3D g_strdup_printf("child<%s>", object_get_typename(OBJECT(c= hild))); > =20 > - object_property_add(obj, name, type, object_get_child_property, NU= LL, > - object_finalize_child_property, child, &local_= err); > + object_property_add_full(obj, name, type, object_get_child_propert= y, NULL, > + object_resolve_child_property, > + object_finalize_child_property, child, &l= ocal_err); > if (local_err) { > error_propagate(errp, local_err); > goto out; > @@ -1128,6 +1141,12 @@ static void object_set_link_property(Object *obj= , Visitor *v, void *opaque, > } > } > =20 > +static Object *object_resolve_link_property(Object *parent, void *opaq= ue, const gchar *part) > +{ > + LinkProperty *lprop =3D opaque; > + return *lprop->child; > +} > + > static void object_release_link_property(Object *obj, const char *name= , > void *opaque) > { > @@ -1156,12 +1175,13 @@ void object_property_add_link(Object *obj, cons= t char *name, > =20 > full_type =3D g_strdup_printf("link<%s>", type); > =20 > - object_property_add(obj, name, full_type, > - object_get_link_property, > - check ? object_set_link_property : NULL, > - object_release_link_property, > - prop, > - &local_err); > + object_property_add_full(obj, name, full_type, > + object_get_link_property, > + check ? object_set_link_property : NULL, > + object_resolve_link_property, > + object_release_link_property, > + prop, > + &local_err); > if (local_err) { > error_propagate(errp, local_err); > g_free(prop); > @@ -1225,11 +1245,8 @@ Object *object_resolve_path_component(Object *pa= rent, const gchar *part) > return NULL; > } > =20 > - if (object_property_is_link(prop)) { > - LinkProperty *lprop =3D prop->opaque; > - return *lprop->child; > - } else if (object_property_is_child(prop)) { > - return prop->opaque; > + if (prop->resolve) { > + return prop->resolve(parent, prop->opaque, part); > } else { > return NULL; > } The _add vs. _add_full issue aside, the implementation looks good. Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg