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

["Daniel P. Berrangé" <berrange@redhat.com>]

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.
> 
> Link: 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.
> + */

The docs are a welcome addition, but at the same time the docs won't get
read most of the time.

With this in mind, IMHO, it is a conceptually *terrible* design for us to
have a method called "get" which magically *creates* stuff as a side-effect
of its calling. We'd be well served by fixing that design problem.

If I look in the code at what calls we have to container_get, and more
specifically what "path" values we pass, there are not actually that many:

  /objects
  /chardevs
  /unattached
  /machine
  /peripheral
  /peripheral-anon
  /backend
  /dr-connector
  

Ignoring the last one, those other 7 containers are things we expect
to exist in *every* system emulator.

Second, every single one of them is a single level deep. IOW, the for()
loop in container_get is effectively pointless.

We can fix this by having a single method:

 void container_create_builtin(Object *root)

which creates the 7 built-in standard containers we expect
everywhere, with open coded object_new + add_child calls. 

Then all current users of container_get() can switch over
to object_resolve_path, and container_get() can be eliminated.

The 'dr-connector' creation can just be open-coded using
object_new() in the spapr code.

>  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
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|