| Message ID | 20241118221330.3480246-6-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | QOM: Enforce container_get() to operate on containers only | expand |
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: 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 >
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)); } }
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(+)