qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/5] qom: add a generic mechanism to resolve pat


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH 1/5] qom: add a generic mechanism to resolve paths
Date: Tue, 17 Jun 2014 17:07:57 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0

Il 17/06/2014 16:18, Andreas Färber 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 char *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 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 = foo).

The reason I went with "_full" is that the new argument is really needed only in a minority of cases. There are ~50 occurrences right now, and I expect only a handful of them to need a ->resolve callback (and so far all of them would be in qom/object.c).

There are many examples in glib's GSource (g_timeout_add_full, g_main_context_invoke_full, etc.) or elsewhere in glib (g_format_size_full).

Since we do not have an ABI to follow, we could add arguments to the _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 *type,
+void object_property_add_full(Object *obj, const char *name, const char *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 = get;
     prop->set = set;
+    prop->resolve = resolve;
     prop->release = release;
     prop->opaque = opaque;

     QTAILQ_INSERT_TAIL(&obj->properties, prop, node);
 }

+void object_property_add(Object *obj, const char *name, const char *type,
+                         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);
 }

+static Object *object_resolve_child_property(Object *parent, void *opaque, 
const gchar *part)
+{
+    return opaque;
+}
+
 static void object_finalize_child_property(Object *obj, const char *name,
                                            void *opaque)
 {
@@ -1009,8 +1021,9 @@ void object_property_add_child(Object *obj, const char 
*name,

     type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));

-    object_property_add(obj, name, type, object_get_child_property, NULL,
-                        object_finalize_child_property, child, &local_err);
+    object_property_add_full(obj, name, type, object_get_child_property, 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 *obj, 
Visitor *v, void *opaque,
     }
 }

+static Object *object_resolve_link_property(Object *parent, void *opaque, 
const gchar *part)
+{
+    LinkProperty *lprop = 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, const char 
*name,

     full_type = 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 *parent, 
const gchar *part)
         return NULL;
     }

-    if (object_property_is_link(prop)) {
-        LinkProperty *lprop = 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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]