Hi
On Wed, Oct 2, 2024 at 10:21 AM Markus Armbruster <arm...@redhat.com> wrote:
>
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > object_resolve_path_type() didn't always set *ambiguousp.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>
> Fixes: 81c48dd79655 (hw/i386/acpi: Add object_resolve_type_unambiguous to
> improve modularity)
>
ok
> > ---
> > qom/object.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index 28c5b66eab..bdc8a2c666 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -2201,6 +2201,9 @@ Object *object_resolve_path_type(const char *path,
> > const char *typename,
> > }
> > } else {
> > obj = object_resolve_abs_path(object_get_root(), parts + 1,
> > typename);
> > + if (ambiguousp) {
> > + *ambiguousp = false;
> > + }
> > }
> >
> > g_strfreev(parts);
> > @@ -2226,7 +2229,7 @@ Object *object_resolve_path_at(Object *parent, const
> > char *path)
> >
> > Object *object_resolve_type_unambiguous(const char *typename, Error **errp)
> > {
> > - bool ambig;
> > + bool ambig = false;
> > Object *o = object_resolve_path_type("", typename, &ambig);
> >
> > if (ambig) {
>
> Contract:
>
> /**
> * object_resolve_path_type:
> * @path: the path to resolve
> * @typename: the type to look for.
> * @ambiguous: returns true if the path resolution failed because of an
> * ambiguous match
> *
> * This is similar to object_resolve_path. However, when looking for a
> * partial path only matches that implement the given type are considered.
> * This restricts the search and avoids spuriously flagging matches as
> * ambiguous.
> *
> * For both partial and absolute paths, the return value goes through
> * a dynamic cast to @typename. This is important if either the link,
> * or the typename itself are of interface types.
> *
> * Returns: The matched object or NULL on path lookup failure.
> */
>
> Note the parameter is called @ambiguous here, but @ambiguousp in the
> definition. Bad practice.
hmm
>
> All the contract promises is that true will be stored in the variable
> passed to @ambiguous when the function fails in a certain way. For that
> to work, the variable must be initialized to false.
>
> You found a caller that doesn't: object_resolve_type_unambiguous().
> This is a bug. There might be more. Impact is not obvious.
>
> Two ways to fix:
>
> 1. Find all callers that don't, and fix them. Your first hunk is then
> superfluous. Your second hunk fixes the one you already found.
>
Imho, that's not a good API, it's easy to get wrong.
> 2. Change the contract so callers don't have to initialize. Your second
> hunk is then superfluous. The update to the contract is missing.
>
I prefer that it always set the variable. I also prefer that caller
initializes variables. So all are good practices imho, even if it's a
bit redundant.
> While there: the contract fails to specify that @ambiguous may be null.
> Needs fixing, too.
>
> Same for object_resolve_path().
>
ok
