On Mon, Jan 26, 2026 at 04:16:11PM -0300, Fabiano Rosas wrote:
> Peter Xu <[email protected]> writes:
>
> > On Thu, Jan 22, 2026 at 08:24:56PM -0300, Fabiano Rosas wrote:
> >> Fix a couple of leaks detected by Coverity. Both are currently
> >> harmless because the visitor in the setter can never fail and the
> >> whole of the getter is unused, it's only purpose at the moment is to
> >> provide a complete implementation of the StrOrNull property.
> >>
> >> Fixes: CID 1643919
> >> Fixes: CID 1643920
> >> Reported-by: Peter Maydell <[email protected]>
> >> Signed-off-by: Fabiano Rosas <[email protected]>
> >> ---
> >> CI run: https://gitlab.com/farosas/qemu/-/pipelines/2280325023
> >> ---
> >> migration/options.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/migration/options.c b/migration/options.c
> >> index 9a5a39c886..9dc44a3736 100644
> >> --- a/migration/options.c
> >> +++ b/migration/options.c
> >> @@ -225,6 +225,7 @@ static void get_StrOrNull(Object *obj, Visitor *v,
> >> const char *name,
> >> str_or_null = g_new0(StrOrNull, 1);
> >> str_or_null->type = QTYPE_QSTRING;
> >> str_or_null->u.s = g_strdup("");
> >> + *ptr = str_or_null;
> >
> > Yep, fixes the leak.
> >
> > However does it then mean a get() can internally update this Property? It
> > used to be NULL, now it's QTYPE_STRING "".
> >
> > It may make slightly more sense to me to have zero side effect on a get()
> > call. Say, keeping *ptr==NULL after get() returns (as before), while we can
> > use a temp string ("") for the visitor.
> >
> > I randomly checked another get() provider, e.g. get_drive() does that.
> > Should we follow?
> >
>
> Hmm, I'm actually inclined to assert. This is code for the very specific
> purpose of allowing compatibility of -global tls-* options which are
> properties of the s->parameters object.
>
> The only way the property could be NULL in the getter is if the setter
> has failed. Is there a way we could make the setter fail without it
> being a programming error? I don't see it.
>
> - the default for the property is an empty string.
>
> - setting via set-parameters bypasses the qdev code entirely.
>
> - device_add is not reachable because of dc->user_creatable = false.
>
> - object_set_propv and qom_set use a string visitor, so no type mismatch
> to trigger the qobject_to(QString, qobj) cast.
I agree, assert would even be better. Then, maybe let's add a quick
comment for it? Something like:
/*
* Assert for both str_or_null be present and not QNULL.
* Guaranteed because ...
*/
assert(str_or_null && str_or_null->type != QTYPE_QNULL);
--
Peter Xu
