Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Peter Xu]

On Mon, Nov 18, 2024 at 05:13:30PM -0500, Peter Xu wrote:
> When used incorrectly, container_get() can silently create containers even
> if the caller may not intend to do so.  Add a rich document describing the
> helper, as container_get() should only be used in path lookups.
> 
> Add one object_dynamic_cast() check to make sure whatever objects the
> helper walks will be a container object (including the one to be returned).
> It is a programming error otherwise, hence assert that.
> 
> It may make container_get() tiny slower than before, but the hope is the
> change is neglictable, as object_class_dynamic_cast() has a fast path just
> for similar leaf use case.

Just a heads up: out of curiosity, I tried to see whether the fast path hit
that I mentioned here (mostly, commit 793c96b54032 of Paolo's), and it
didn't..

It's fundamentally because all TypeImpl was allocated dynamically from
heap, including its type->name.  While typename should normally be const
strings that locates on RODATA sections, hence they should mostly never
hit when compare with pointers..

I was thinking whether we could add a strcmp() there too for the fast path,
but then I noticed that QEMU could have some pretty long type->name... so
that strcmp() idea may not be good if that's the case. E.g.:

  virtio-net-pci-non-transitional::conventional-pci-device

Which has 55 chars..

I don't have good idea to make the fast path hit here, so I'll at least
remove this paragraph if I'm going to repost.. I hope it's not a huge deal
to still do the sanity check here, as the container type is so special and
small, so that check should be fast regardless.

> 
> Link: 87pln6ds8q.fsf@pond.sub.org">https://lore.kernel.org/r/87pln6ds8q.fsf@pond.sub.org
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qom/container.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/qom/container.c b/qom/container.c
> index cfec92a944..ff6e35f837 100644
> --- a/qom/container.c
> +++ b/qom/container.c
> @@ -24,6 +24,20 @@ static void container_register_types(void)
>      type_register_static(&container_info);
>  }
>  
> +/**
> + * container_get(): Get the container object under specific path
> + *
> + * @root: The root path object to start walking from.  When starting from
> + *        root, one can pass in object_get_root().
> + * @path: The sub-path to lookup, must be an non-empty string starts with 
> "/".
> + *
> + * Returns: The container object specified by @path.
> + *
> + * NOTE: the function may impplicitly create internal containers when the
> + * whole path is not yet created.  It's the caller's responsibility to make
> + * sure the path specified is always used as object containers, rather than
> + * any other type of objects.
> + */
>  Object *container_get(Object *root, const char *path)
>  {
>      Object *obj, *child;
> @@ -31,6 +45,7 @@ Object *container_get(Object *root, const char *path)
>      int i;
>  
>      parts = g_strsplit(path, "/", 0);
> +    /* "path" must be an non-empty string starting with "/" */
>      assert(parts != NULL && parts[0] != NULL && !parts[0][0]);
>      obj = root;
>  
> @@ -40,6 +55,12 @@ Object *container_get(Object *root, const char *path)
>              child = object_new(TYPE_CONTAINER);
>              object_property_add_child(obj, parts[i], child);
>              object_unref(child);
> +        } else {
> +            /*
> +             * Each object within the path must be a container object
> +             * itself, including the object to be returned.
> +             */
> +            assert(object_dynamic_cast(child, TYPE_CONTAINER));
>          }
>      }
>  
> -- 
> 2.45.0
> 

-- 
Peter Xu