[Qemu-devel] [PATCH] qom: support orphan objects in object_get_canonical_path

[Qemu-devel] [PATCH] qom: support orphan objects in object_get_canonical_path

[Paolo Bonzini]

Mostly a rewrite, in order to keep the loop simple.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 0fc972030e..4f30431ae3 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1669,25 +1669,28 @@ gchar *object_get_canonical_path(Object *obj)
     Object *root = object_get_root();
     char *newpath, *path = NULL;
 
-    while (obj != root) {
+    if (obj == root) {
+        return g_strdup("/");
+    }
+
+    do {
         char *component = object_get_canonical_path_component(obj);
 
-        if (path) {
-            newpath = g_strdup_printf("%s/%s", component, path);
-            g_free(component);
+        if (!component) {
+            /* A canonical path must be complete, so discard what was
+             * collected so far.
+             */
             g_free(path);
-            path = newpath;
-        } else {
-            path = component;
+            return NULL;
         }
 
-        obj = obj->parent;
-    }
-
-    newpath = g_strdup_printf("/%s", path ? path : "");
-    g_free(path);
+        newpath = g_strdup_printf("/%s%s", component, path ? path : "");
+        g_free(path);
+        g_free(component);
+        path = newpath;
+    } while ((obj = obj->parent) != root);
 
-    return newpath;
+    return path;
 }
 
 Object *object_resolve_path_component(Object *parent, const gchar *part)
-- 
2.17.0

Re: [Qemu-devel] [PATCH] qom: support orphan objects in object_get_canonical_path

[Philippe Mathieu-Daudé]

On 05/30/2018 01:23 PM, Paolo Bonzini wrote:
> Mostly a rewrite, in order to keep the loop simple.

Thus easier to review using "git difftool -y -x sdiff" (or whichever
tool you prefer, such meld).  "git format-patch" doesn't provide a way
to generate side-by-side diffs applicable.

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qom/object.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 0fc972030e..4f30431ae3 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1669,25 +1669,28 @@ gchar *object_get_canonical_path(Object *obj)
>      Object *root = object_get_root();
>      char *newpath, *path = NULL;
>  
> -    while (obj != root) {
> +    if (obj == root) {
> +        return g_strdup("/");
> +    }
> +
> +    do {
>          char *component = object_get_canonical_path_component(obj);
>  
> -        if (path) {
> -            newpath = g_strdup_printf("%s/%s", component, path);
> -            g_free(component);
> +        if (!component) {
> +            /* A canonical path must be complete, so discard what was
> +             * collected so far.
> +             */
>              g_free(path);
> -            path = newpath;
> -        } else {
> -            path = component;
> +            return NULL;

This function now matches his documentation!

>          }
>  
> -        obj = obj->parent;
> -    }
> -
> -    newpath = g_strdup_printf("/%s", path ? path : "");
> -    g_free(path);
> +        newpath = g_strdup_printf("/%s%s", component, path ? path : "");
> +        g_free(path);
> +        g_free(component);
> +        path = newpath;

Easier to read in 2 lines:

           obj = obj->parent;
       } while (obj != root);

> +    } while ((obj = obj->parent) != root);
>  
> -    return newpath;
> +    return path;
>  }
>  
>  Object *object_resolve_path_component(Object *parent, const gchar *part)
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH] qom: support orphan objects in object_get_canonical_path

[Alexey Kardashevskiy]

On 31/5/18 2:23 am, Paolo Bonzini wrote:
> Mostly a rewrite, in order to keep the loop simple.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qom/object.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 0fc972030e..4f30431ae3 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1669,25 +1669,28 @@ gchar *object_get_canonical_path(Object *obj)
>      Object *root = object_get_root();
>      char *newpath, *path = NULL;
>  
> -    while (obj != root) {
> +    if (obj == root) {
> +        return g_strdup("/");
> +    }
> +
> +    do {
>          char *component = object_get_canonical_path_component(obj);
>  
> -        if (path) {
> -            newpath = g_strdup_printf("%s/%s", component, path);
> -            g_free(component);
> +        if (!component) {
> +            /* A canonical path must be complete, so discard what was
> +             * collected so far.
> +             */

Well, this is correct indeed for the normal case when the result is used
for internal business but for my task (show the owner of an MR or at least
give a clue what to grep for) it will discard a partial path.

I guess I could print a typename if object_get_canonical_path() returns
NULL, I'll repost v4.


>              g_free(path);
> -            path = newpath;
> -        } else {
> -            path = component;
> +            return NULL;
>          }
>  
> -        obj = obj->parent;
> -    }
> -
> -    newpath = g_strdup_printf("/%s", path ? path : "");
> -    g_free(path);
> +        newpath = g_strdup_printf("/%s%s", component, path ? path : "");
> +        g_free(path);
> +        g_free(component);
> +        path = newpath;
> +    } while ((obj = obj->parent) != root);
>  
> -    return newpath;
> +    return path;
>  }
>  
>  Object *object_resolve_path_component(Object *parent, const gchar *part)
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH] qom: support orphan objects in object_get_canonical_path

[Paolo Bonzini]

On 31/05/2018 05:45, Alexey Kardashevskiy wrote:
> Well, this is correct indeed for the normal case when the result is used
> for internal business but for my task (show the owner of an MR or at least
> give a clue what to grep for) it will discard a partial path.
> 
> I guess I could print a typename if object_get_canonical_path() returns
> NULL, I'll repost v4.

Very good!

Paolo