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

[Steven Sistare <steven.sistare@oracle.com>]

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