[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [PATCH] Add object_property_get_child().
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-trivial] [PATCH] Add object_property_get_child(). |
Date: |
Fri, 17 Feb 2012 11:17:41 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20120131 Thunderbird/10.0 |
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.
> + */
> +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?
> + 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.
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.
> 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<FOO> 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