| Message ID | 20240930081458.1926382-22-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | -Werror=maybe-uninitialized fixes | expand |
On 30.09.24 11:14, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > object_resolve_path_type() didn't always set *ambiguousp. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > 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; > + } Doesn't this hunk in isolation fix the issue? With this object_resolve_path_type() should set the pointer on all paths if it is non-null.. Hmm, called object_resolve_partial_path() also doesn't set ambiguous on every path, so this hunk is at lease incomplete. I'm unsure about what semantics expected around ambigous pointers, but it seems to me that it is set only on failure paths, as a reason, why we failed. If this is true, I think, we need only the second hunk, which initializes local "ambig". > } > > 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) {
Hi Vladimir On Tue, Oct 1, 2024 at 6:06 PM Vladimir Sementsov-Ogievskiy < vsementsov@yandex-team.ru> wrote: > On 30.09.24 11:14, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > object_resolve_path_type() didn't always set *ambiguousp. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > 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; > > + } > > Doesn't this hunk in isolation fix the issue? With this > object_resolve_path_type() should set the pointer on all paths if it is > non-null.. > > > Hmm, called object_resolve_partial_path() also doesn't set ambiguous on > every path, so this hunk is at lease incomplete. > yeah, but object_resolve_path_type() initializes it. I'm unsure about what semantics expected around ambigous pointers, but it > seems to me that it is set only on failure paths, as a reason, why we > failed. If this is true, I think, we need only the second hunk, which > initializes local "ambig". > > right, and that seems good enough. Do you ack/rb this change then? qom/object: fix -Werror=maybe-uninitialized object_resolve_path_type() didn't always set *ambiguousp. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> diff --git a/qom/object.c b/qom/object.c index 28c5b66eab..d3d3003541 100644 --- a/qom/object.c +++ b/qom/object.c @@ -2226,7 +2226,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) { thanks! > > } > > > > 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) { > > -- > Best regards, > Vladimir > > >
marcandre.lureau@redhat.com writes: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > object_resolve_path_type() didn't always set *ambiguousp. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Fixes: 81c48dd79655 (hw/i386/acpi: Add object_resolve_type_unambiguous to improve modularity) > --- > 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. 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. 2. Change the contract so callers don't have to initialize. Your second hunk is then superfluous. The update to the contract is missing. While there: the contract fails to specify that @ambiguous may be null. Needs fixing, too. Same for object_resolve_path().
On 01.10.24 18:22, Marc-André Lureau wrote: > Hi Vladimir > > On Tue, Oct 1, 2024 at 6:06 PM Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru <mailto:vsementsov@yandex-team.ru>> wrote: > > On 30.09.24 11:14, marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com> wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com>> > > > > object_resolve_path_type() didn't always set *ambiguousp. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com>> > > --- > > 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; > > + } > > Doesn't this hunk in isolation fix the issue? With this object_resolve_path_type() should set the pointer on all paths if it is non-null.. > > > > Hmm, called object_resolve_partial_path() also doesn't set ambiguous on every path, so this hunk is at lease incomplete. > > > yeah, but object_resolve_path_type() initializes it. > > I'm unsure about what semantics expected around ambigous pointers, but it seems to me that it is set only on failure paths, as a reason, why we failed. If this is true, I think, we need only the second hunk, which initializes local "ambig". > > > right, and that seems good enough. > > Do you ack/rb this change then? > > > qom/object: fix -Werror=maybe-uninitialized > > object_resolve_path_type() didn't always set *ambiguousp. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com>> > > diff --git a/qom/object.c b/qom/object.c > index 28c5b66eab..d3d3003541 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -2226,7 +2226,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) { > > Yes, I think this one in isolation is OK: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > thanks! > > > } > > > > 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) { > > -- > Best regards, > Vladimir > > > > > -- > Marc-André Lureau
Hi On Wed, Oct 2, 2024 at 10:21 AM Markus Armbruster <armbru@redhat.com> wrote: > > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > object_resolve_path_type() didn't always set *ambiguousp. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@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
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Hi > > On Wed, Oct 2, 2024 at 10:21 AM Markus Armbruster <armbru@redhat.com> wrote: [...] >> 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. Since you're doing the work to fix the bug, you get first dibs on how to fix it :) [...]
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) {