From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44525) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKx9c-0007P6-Ec for qemu-devel@nongnu.org; Tue, 04 Mar 2014 16:45:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKx9O-0001zf-1Q for qemu-devel@nongnu.org; Tue, 04 Mar 2014 16:45:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:2783) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKx9N-0001zT-PY for qemu-devel@nongnu.org; Tue, 04 Mar 2014 16:45:21 -0500 From: Stefan Hajnoczi Date: Tue, 4 Mar 2014 22:45:09 +0100 Message-Id: <1393969512-9468-2-git-send-email-stefanha@redhat.com> In-Reply-To: <1393969512-9468-1-git-send-email-stefanha@redhat.com> References: <1393969512-9468-1-git-send-email-stefanha@redhat.com> Subject: [Qemu-devel] [PATCH 1/4] qom: split object_property_set_link() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Peter Maydell , Andreas Faerber , Anthony Liguori , Paolo Bonzini 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 --- 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 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 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