[Qemu-devel] [PATCH 1/4] qom: split object_property_set_link()

[Stefan Hajnoczi]

The path resolution logic in object_property_set_link() should be a
separate function.  This makes the code easier to read and maintain.

More importantly, the error behavior of object_property_set_link() is
dangerous.  It sets the link property object to NULL if an error occurs.
A setter function should either succeed or fail, it shouldn't leave the
value NULL on failure.

This patch splits the code and fixes the error case so the old link
property object is left in place on failure.

Signed-off-by: Stefan Hajnoczi <address@hidden>
---
 qom/object.c | 79 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 31 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 660859c..c11ea1e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1039,48 +1039,65 @@ static void object_get_link_property(Object *obj, 
Visitor *v, void *opaque,
     }
 }
 
-static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
-                                     const char *name, Error **errp)
+/* object_resolve_link:
+ *
+ * Lookup an object and ensure its type matches the link property type.  This
+ * is similar to object_resolve_path() except type verification against the
+ * link property is performed.
+ *
+ * Returns: The matched object or NULL on path lookup failures.
+ */
+static Object *object_resolve_link(Object *obj, const char *name,
+                                   const char *path, Error **errp)
 {
-    Object **child = opaque;
-    Object *old_target;
-    bool ambiguous = false;
     const char *type;
-    char *path;
     gchar *target_type;
+    bool ambiguous = false;
+    Object *target;
 
+    /* Go from link<FOO> to FOO.  */
     type = object_property_get_type(obj, name, NULL);
-
-    visit_type_str(v, &path, name, errp);
-
-    old_target = *child;
-    *child = NULL;
-
-    if (strcmp(path, "") != 0) {
-        Object *target;
-
-        /* Go from link<FOO> to FOO.  */
-        target_type = g_strndup(&type[5], strlen(type) - 6);
-        target = object_resolve_path_type(path, target_type, &ambiguous);
-
-        if (ambiguous) {
-            error_set(errp, QERR_AMBIGUOUS_PATH, path);
-        } else if (target) {
-            object_ref(target);
-            *child = target;
+    target_type = g_strndup(&type[5], strlen(type) - 6);
+    target = object_resolve_path_type(path, target_type, &ambiguous);
+
+    if (ambiguous) {
+        error_set(errp, QERR_AMBIGUOUS_PATH, path);
+    } else if (!target) {
+        target = object_resolve_path(path, &ambiguous);
+        if (target || ambiguous) {
+            error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, target_type);
         } else {
-            target = object_resolve_path(path, &ambiguous);
-            if (target || ambiguous) {
-                error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, 
target_type);
-            } else {
-                error_set(errp, QERR_DEVICE_NOT_FOUND, path);
-            }
+            error_set(errp, QERR_DEVICE_NOT_FOUND, path);
         }
-        g_free(target_type);
     }
+    g_free(target_type);
 
+    return target;
+}
+
+static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
+                                     const char *name, Error **errp)
+{
+    Error *local_err = NULL;
+    Object **child = opaque;
+    Object *old_target = *child;
+    Object *new_target = NULL;
+    char *path = NULL;
+
+    visit_type_str(v, &path, name, &local_err);
+    if (!local_err && strcmp(path, "") != 0) {
+        new_target = object_resolve_link(obj, name, path, &local_err);
+    }
     g_free(path);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
+    if (new_target) {
+        object_ref(new_target);
+    }
+    *child = new_target;
     if (old_target != NULL) {
         object_unref(old_target);
     }
-- 
1.8.5.3