Re: [PATCH V7 23/24] migration-test: cpr-transfer

[Steven Sistare]

On 1/16/2025 3:02 PM, Fabiano Rosas wrote:
> Steven Sistare <steven.sistare@oracle.com> writes:
>
> > On 1/16/2025 2:06 PM, Fabiano Rosas wrote:
> > > Steve Sistare <steven.sistare@oracle.com> writes:
> > [...]
> > > > +    /*
> > > > +     * The cpr channel must be included in outgoing channels, but not in
> > > > +     * migrate-incoming channels.
> > > > +     */
> > > >        if (args->connect_channels) {
> > > > +        in_channels = qobject_from_json(args->connect_channels, &error_abort);
> > > >            out_channels = qobject_from_json(args->connect_channels, 
> > > > &error_abort);
> > > > +
> > > > +        if (args->cpr_channel) {
> > > > +            QList *channels_list = qobject_to(QList, out_channels);
> > > > +            QObject *obj = migrate_str_to_channel(args->cpr_channel);
> > > > +
> > > > +            qlist_append(channels_list, obj);
> > > > +        }
> > > >        }
> > > >    
> > > >        if (args->result == MIG_TEST_QMP_ERROR) {
> > > > @@ -735,6 +751,9 @@ void test_precopy_common(MigrateCommon *args)
> > > >        if (args->start.defer_target_connect) {
> > > >            qtest_connect(to);
> > > >            qtest_qmp_handshake(to);
> > > > +        if (!strcmp(args->listen_uri, "defer")) {
> > > > +            migrate_incoming_qmp(to, args->connect_uri, in_channels, "{}");
> > > > +        }
> > >
> > > Paths that don't call migrate_incoming_qmp() never free
> > > in_channels. We'll need something like this, let me know if I can squash
> > > it in or you want to do it differently:
> > >
> > > -- >8 --
> > >   From 62d60c39b3e5d38cac20241e63b9d023bd296d2f Mon Sep 17 00:00:00 2001
> > > From: Fabiano Rosas <farosas@suse.de>
> > > Date: Thu, 16 Jan 2025 15:40:22 -0300
> > > Subject: [PATCH] fixup! migration-test: cpr-transfer
> > >
> > > ---
> > >    tests/qtest/migration/framework.c | 5 +++++
> > >    1 file changed, 5 insertions(+)
> > >
> > > diff --git a/tests/qtest/migration/framework.c 
> > > b/tests/qtest/migration/framework.c
> > > index 699bedae69..1d5918d922 100644
> > > --- a/tests/qtest/migration/framework.c
> > > +++ b/tests/qtest/migration/framework.c
> > > @@ -753,9 +753,14 @@ void test_precopy_common(MigrateCommon *args)
> > >            qtest_qmp_handshake(to);
> > >            if (!strcmp(args->listen_uri, "defer")) {
> > >                migrate_incoming_qmp(to, args->connect_uri, in_channels, "{}");
> > > +            in_channels = NULL;
> > >            }
> > >        }
> > >    
> > > +    if (in_channels) {
> > > +        qobject_unref(in_channels);
> > > +    }
> > > +
> > >        if (args->result != MIG_TEST_SUCCEED) {
> > >            bool allow_active = args->result == MIG_TEST_FAIL;
> > >            wait_for_migration_fail(from, allow_active);
> >
> > Thank-you, though it would be more direct to avoid creating in_channels when
> > not needed:
> >
> >       if (args->connect_channels) {
> >           if (args->start.defer_target_connect) {
> >               in_channels = qobject_from_json(args->connect_channels,
> >                                               &error_abort);
> >           }
> >           out_channels = qobject_from_json(args->connect_channels, 
> > &error_abort);
>
> That's better, but still needs one unref for the listen_uri != defer path.

OK, then:

    if (args->connect_channels) {
        if (args->start.defer_target_connect &&
            !strcmp(args->listen_uri, "defer")) {
            in_channels = qobject_from_json(args->connect_channels,
                                            &error_abort);
        }
        out_channels = qobject_from_json(args->connect_channels, &error_abort);

Or keep your fix.  I have no preference.

- Steve