Re: [PATCH v2 21/24] tests/qtest/migration: Take reference when passing %p to qtest_qmp

[Peter Xu <peterx@redhat.com>]

On Mon, Jun 30, 2025 at 04:59:10PM -0300, Fabiano Rosas wrote:
> The documentation of qobject_from_jsonv() states that it takes
> ownership of any %p arguments passed in.
> 
> Next patches will add config-passing to the tests, so take an extra
> reference in the migrate_qmp* functions to ensure the config is not
> freed from under us.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  tests/qtest/migration/migration-qmp.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qtest/migration/migration-qmp.c b/tests/qtest/migration/migration-qmp.c
> index fb59741b2c..d82ac8c750 100644
> --- a/tests/qtest/migration/migration-qmp.c
> +++ b/tests/qtest/migration/migration-qmp.c
> @@ -97,7 +97,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
>      }
>  
>      err = qtest_qmp_assert_failure_ref(
> -        who, "{ 'execute': 'migrate', 'arguments': %p}", args);
> +        who, "{ 'execute': 'migrate', 'arguments': %p}",
> +        qdict_clone_shallow(args));
>  
>      g_assert(qdict_haskey(err, "desc"));
>  
> @@ -136,7 +137,8 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>      }
>  
>      qtest_qmp_assert_success(who,
> -                             "{ 'execute': 'migrate', 'arguments': %p}", args);
> +                             "{ 'execute': 'migrate', 'arguments': %p}",
> +                             qdict_clone_shallow(args));
>  }
>  
>  void migrate_set_capability(QTestState *who, const char *capability,
> @@ -174,7 +176,7 @@ void migrate_incoming_qmp(QTestState *to, const char *uri, QObject *channels,
>      migrate_set_capability(to, "events", true);
>  
>      rsp = qtest_qmp(to, "{ 'execute': 'migrate-incoming', 'arguments': %p}",
> -                    args);
> +                    qdict_clone_shallow(args));

Isn't it intentional to pass over the ownership in the three places here?
I don't see otherwise where args got freed.

OTOH, I saw there're yet another three similar usages of %p in framework.c:

x1:migration [migration-params-caps-no-config]$ git grep -A1 %p framework.c
framework.c:        migrate_qmp_fail(from, args->connect_uri, NULL, "{ 'config': %p }",
framework.c-                         args->start.config);
--
framework.c:    migrate_qmp(from, to, args->connect_uri, NULL, "{ 'config': %p }",
framework.c-                args->start.config);
--
framework.c:    migrate_incoming_qmp(to, args->connect_uri, NULL, "{ 'config': %p }",
framework.c-                         args->start.config);

They seem to be suspecious instead, as they seem to have lost ownership of
args->start.config, so args->start.config can start to point to garbage?

>  
>      if (!qdict_haskey(rsp, "return")) {
>          g_autoptr(GString) s = qobject_to_json_pretty(QOBJECT(rsp), true);
> -- 
> 2.35.3
> 

-- 
Peter Xu