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 <address@hidden>
> ---
>  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é <address@hidden>