From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47192) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wwuza-0004ZP-6q for qemu-devel@nongnu.org; Tue, 17 Jun 2014 11:08:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WwuzS-0000TK-0C for qemu-devel@nongnu.org; Tue, 17 Jun 2014 11:08:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2099) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwuzR-0000T5-OS for qemu-devel@nongnu.org; Tue, 17 Jun 2014 11:08:01 -0400 Message-ID: <53A059CD.7050807@redhat.com> Date: Tue, 17 Jun 2014 17:07:57 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1402505369-12526-1-git-send-email-pbonzini@redhat.com> <1402505369-12526-2-git-send-email-pbonzini@redhat.com> <53A04E1C.5080105@suse.de> In-Reply-To: <53A04E1C.5080105@suse.de> Content-Type: text/plain; charset=ISO-8859-15; format=flowed 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: =?ISO-8859-15?Q?Andreas_F=E4rber?= , qemu-devel@nongnu.org Cc: mtosatti@redhat.com, stefanha@redhat.com Il 17/06/2014 16:18, Andreas F=E4rber ha scritto: >> /** >> + * 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. Good suggestion. >> +void object_property_add_full(Object *obj, const char *name, const ch= ar *type, >> + ObjectPropertyAccessor *get, >> + ObjectPropertyAccessor *set, >> + ObjectPropertyResolve *resolve, >> + ObjectPropertyRelease *release, >> + void *opaque, Error **errp); > > 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 argumen= t > 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). The reason I went with "_full" is that the new argument is really needed=20 only in a minority of cases. There are ~50 occurrences right now, and I=20 expect only a handful of them to need a ->resolve callback (and so far=20 all of them would be in qom/object.c). There are many examples in glib's GSource (g_timeout_add_full,=20 g_main_context_invoke_full, etc.) or elsewhere in glib (g_format_size_ful= l). Since we do not have an ABI to follow, we could add arguments to the=20 _full routine while keeping the shorthand version as is. I can change the 50 occurrences if you want though. Paolo >> -void object_property_add(Object *obj, const char *name, const char *t= ype, >> +void object_property_add_full(Object *obj, const char *name, const ch= ar *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, >> >> prop->get =3D get; >> prop->set =3D set; >> + prop->resolve =3D resolve; >> prop->release =3D release; >> prop->opaque =3D opaque; >> >> QTAILQ_INSERT_TAIL(&obj->properties, prop, node); >> } >> >> +void object_property_add(Object *obj, const char *name, const char *t= ype, >> + 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 *ob= j, Visitor *v, void *opaque, >> g_free(path); >> } >> >> +static Object *object_resolve_child_property(Object *parent, void *op= aque, const gchar *part) >> +{ >> + return opaque; >> +} >> + >> static void object_finalize_child_property(Object *obj, const char *n= ame, >> void *opaque) >> { >> @@ -1009,8 +1021,9 @@ void object_property_add_child(Object *obj, cons= t char *name, >> >> type =3D g_strdup_printf("child<%s>", object_get_typename(OBJECT(= child))); >> >> - object_property_add(obj, name, type, object_get_child_property, N= ULL, >> - object_finalize_child_property, child, &local= _err); >> + object_property_add_full(obj, name, type, object_get_child_proper= ty, NULL, >> + object_resolve_child_property, >> + object_finalize_child_property, child, &= local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> goto out; >> @@ -1128,6 +1141,12 @@ static void object_set_link_property(Object *ob= j, Visitor *v, void *opaque, >> } >> } >> >> +static Object *object_resolve_link_property(Object *parent, void *opa= que, const gchar *part) >> +{ >> + LinkProperty *lprop =3D opaque; >> + return *lprop->child; >> +} >> + >> static void object_release_link_property(Object *obj, const char *nam= e, >> void *opaque) >> { >> @@ -1156,12 +1175,13 @@ void object_property_add_link(Object *obj, con= st char *name, >> >> full_type =3D g_strdup_printf("link<%s>", type); >> >> - 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 *p= arent, const gchar *part) >> return NULL; >> } >> >> - 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 >