From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:38696) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rz5zx-0002af-Bn for qemu-devel@nongnu.org; Sun, 19 Feb 2012 07:36:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rz5zv-0003t3-O7 for qemu-devel@nongnu.org; Sun, 19 Feb 2012 07:36:13 -0500 Received: from relay1.mentorg.com ([192.94.38.131]:51572) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rz5zv-0003sl-Dz for qemu-devel@nongnu.org; Sun, 19 Feb 2012 07:36:11 -0500 Message-ID: <4F40ECC6.8000603@mentor.com> Date: Sun, 19 Feb 2012 14:36:22 +0200 From: Alexander Barabash MIME-Version: 1.0 References: <4F3D3F67.9050506@mentor.com> <4F3E2945.3000603@redhat.com> In-Reply-To: <4F3E2945.3000603@redhat.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] Add object_property_get_child(). List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel On 02/17/2012 12:17 PM, Paolo Bonzini wrote: > Besides the non-triviality of the patch, how is this different from > object_property_get_link? Perhaps we should just rename that one to > object_property_get_obj, or add a function that is just a synonym. As for the patch's (non-)triviality, I just wanted to check what counts as trivial here. I agree that was a bit over-reaching. The proposed object_property_get_child() may return either the direct child with the specified name in the composition tree, or the value of the link with the specified name, as object_property_get_link() indeed does. It has exactly the same semantics as: Object *object_property_get_child(Object *obj, const char *name, struct Error **errp) { Object * result; bool ambigious; gchar *child_path; /* If name contains a '/' in it, report an error and return NULL. */ child_path = g_strdup_printf("%s/%s", object_get_canonical_path(obj), name); result = object_resolve_path(child_path, &ambigious); g_free(child_path); if (result == NULL) { /* Report an error. */ return NULL; } if (ambigious) { /* Report an error. */ return NULL; } return result; } but does it in a more straightforward way. > >> + */ >> +Object *object_property_get_child(Object *obj, const char *name, >> + struct Error **errp); >> + >> +/** >> * object_property_set: >> * @obj: the object >> * @v: the visitor that will be used to write the property value. This >> should >> diff --git a/qom/object.c b/qom/object.c >> index 2de6eaf..61e775b 100644 >> --- a/qom/object.c >> +++ b/qom/object.c >> @@ -297,12 +297,86 @@ static void object_property_del_all(Object *obj) >> } >> } >> >> +/* >> + * To ensure correct format checking, >> + * this function should be used only via REPORT_OBJECT_ERROR() macro. >> + * >> + * The first argument after 'obj' should be of type 'const char *'. >> + * It is ignored, and replaced by the canonical path of 'obj'. >> + */ >> +static void report_object_error(Error **errp, const char *fmt, Object >> *obj, ...) >> + GCC_FMT_ATTR(2, 4); >> +static void report_object_error(Error **errp, const char *fmt, Object >> *obj, ...) >> +{ >> + gchar *path; >> + va_list ap; >> + >> + if (errp != NULL) { >> + path = object_get_canonical_path(obj); >> + va_start(ap, obj); >> + va_arg(ap, const char *); /* Ignore the dummy string. */ > Why do you need it at all? I think this is needed to avoid replicating code and have an easy tool to report object's path in errors. > >> + error_set(errp, fmt, path,&ap); >> + va_end(ap); >> + g_free(path); >> + } >> +} >> +#define REPORT_OBJECT_ERROR(errp, fmt, obj, ...) \ >> + do { \ >> + report_object_error(errp, fmt, obj, "", ## __VA_ARGS__); \ >> + } while (0) >> +#define CHILD_PROPERTY_TYPE_PREFIX "child<" >> +#define CHILD_PROPERTY_TYPE_SUFFIX ">" >> +#define LINK_PROPERTY_TYPE_PREFIX "link<" >> +#define LINK_PROPERTY_TYPE_SUFFIX ">" >> + >> +static bool object_property_is_child(ObjectProperty *prop) >> +{ >> + return (strstart(prop->type, CHILD_PROPERTY_TYPE_PREFIX, NULL) != 0); >> +} >> + >> +static bool object_property_is_link(ObjectProperty *prop) >> +{ >> + return (strstart(prop->type, LINK_PROPERTY_TYPE_PREFIX, NULL) != 0); >> +} >> + >> +/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to >> FOO. */ >> +static gchar *link_type_to_type(const gchar *type) >> +{ >> + return g_strndup(&type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1], >> + strlen(type) >> + - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1) >> + - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1)); >> +} >> + >> +static Object *object_property_extract_child(ObjectProperty *prop, >> + bool *p_is_invalid_type) >> +{ >> + if (p_is_invalid_type != NULL) { >> + *p_is_invalid_type = false; >> + } >> + if (object_property_is_child(prop)) { >> + return (Object *)prop->opaque; >> + } else if (object_property_is_link(prop)) { >> + if (prop->opaque != NULL) { >> + return *(Object **)prop->opaque; >> + } else { >> + return NULL; >> + } >> + } else { >> + if (p_is_invalid_type != NULL) { >> + *p_is_invalid_type = true; >> + } >> + return NULL; >> + } >> +} >> + > object_property_get_child should never be on a fast path. We should > avoid as much as possible peeking in the opaque. Instead, you should > just use a visitor like object_property_get_link does. I am not sure what do you mean by "should never be on a fast path". Anyway, I did not add any peeking in the opaque. I just moved the code from object_resolve_abs_path() into a subroutine. Fairly enough, we could avoid looking into the link property's opaque by calling object_property_get_link() instead. I'll prepare a revision of the patch to this end. > > Everything starting here should be a separate patch. But if you remove > object_property_extract_child, I doubt it makes as much sense to > refactor this. Obviously, the refactoring was needed to add code. If we don't, no refactoring is required. > >> static void object_property_del_child(Object *obj, Object *child, Error >> **errp) >> { >> ObjectProperty *prop; >> >> QTAILQ_FOREACH(prop,&obj->properties, node) { >> - if (!strstart(prop->type, "child<", NULL)) { >> + if (!object_property_is_child(prop)) { >> continue; >> } >> >> @@ -799,6 +873,27 @@ Object *object_get_root(void) >> return root; >> } >> >> +Object *object_property_get_child(Object *obj, const char *name, >> + struct Error **errp) { >> + Object *result = NULL; >> + ObjectProperty *prop = object_property_find(obj, name); >> + bool is_invalid_type; >> + >> + if (prop == NULL) { >> + REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_NOT_FOUND, obj, >> name); >> + return NULL; >> + } >> + >> + result = object_property_extract_child(prop,&is_invalid_type); >> + if (is_invalid_type) { >> + REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_INVALID_TYPE, >> + obj, name, "child"); >> + return NULL; >> + } >> + >> + return result; >> +} >> + >> static void object_get_child_property(Object *obj, Visitor *v, void >> *opaque, >> const char *name, Error **errp) >> { >> @@ -829,7 +924,10 @@ void object_property_add_child(Object *obj, const >> char *name, >> */ >> assert(!object_is_type(obj, type_interface)); >> >> - type = g_strdup_printf("child<%s>", >> object_get_typename(OBJECT(child))); >> + type = g_strdup_printf(CHILD_PROPERTY_TYPE_PREFIX >> + "%s" >> + CHILD_PROPERTY_TYPE_SUFFIX, >> + object_get_typename(OBJECT(child))); >> >> object_property_add(obj, name, type, object_get_child_property, >> NULL, object_finalize_child_property, child, >> errp); >> @@ -878,8 +976,7 @@ static void object_set_link_property(Object *obj, >> Visitor *v, void *opaque, >> if (strcmp(path, "") != 0) { >> Object *target; >> >> - /* Go from link to FOO. */ >> - target_type = g_strndup(&type[5], strlen(type) - 6); >> + target_type = link_type_to_type(type); >> target = object_resolve_path_type(path, target_type,&ambiguous); >> >> if (ambiguous) { >> @@ -907,7 +1004,9 @@ void object_property_add_link(Object *obj, const >> char *name, >> { >> gchar *full_type; >> >> - full_type = g_strdup_printf("link<%s>", type); >> + full_type = g_strdup_printf(LINK_PROPERTY_TYPE_PREFIX >> + "%s" >> + LINK_PROPERTY_TYPE_SUFFIX, type); >> >> object_property_add(obj, name, full_type, >> object_get_link_property, >> @@ -932,7 +1031,7 @@ gchar *object_get_canonical_path(Object *obj) >> g_assert(obj->parent != NULL); >> >> QTAILQ_FOREACH(prop,&obj->parent->properties, node) { >> - if (!strstart(prop->type, "child<", NULL)) { >> + if (!object_property_is_child(prop)) { >> continue; >> } >> >> @@ -980,15 +1079,7 @@ static Object *object_resolve_abs_path(Object >> *parent, >> return NULL; >> } >> >> - child = NULL; >> - if (strstart(prop->type, "link<", NULL)) { >> - Object **pchild = prop->opaque; >> - if (*pchild) { >> - child = *pchild; >> - } >> - } else if (strstart(prop->type, "child<", NULL)) { >> - child = prop->opaque; >> - } >> + child = object_property_extract_child(prop, NULL); >> >> if (!child) { >> return NULL; >> @@ -1010,7 +1101,7 @@ static Object *object_resolve_partial_path(Object >> *parent, >> QTAILQ_FOREACH(prop,&parent->properties, node) { >> Object *found; >> >> - if (!strstart(prop->type, "child<", NULL)) { >> + if (!object_property_is_child(prop)) { >> continue; >> } >> >> >> >> > Paolo Alex