Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address

[Het Gala <het.gala@nutanix.com>]

[-- Attachment #1: Type: text/plain, Size: 6145 bytes --]


On 18/03/24 7:46 pm, Fabiano Rosas wrote:
> Het Gala<het.gala@nutanix.com>  writes:
>
>> On 15/03/24 6:28 pm, Fabiano Rosas wrote:
>>> Het Gala<het.gala@nutanix.com>   writes:
>>>
>>>> Refactor migrate_get_socket_address to internally utilize 'socket-address'
>>>> parameter, reducing redundancy in the function definition.
>>>>
>>>> migrate_get_socket_address implicitly converts SocketAddress into str.
>>>> Move migrate_get_socket_address inside migrate_get_connect_uri which
>>>> should return the uri string instead.
>>>>
>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>> Suggested-by: Fabiano Rosas<farosas@suse.de>
>>>> Reviewed-by: Fabiano Rosas<farosas@suse.de>
>>>> ---
>>>>    tests/qtest/migration-helpers.c | 29 +++++++++++++++++++----------
>>>>    1 file changed, 19 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>>>> index 3e8c19c4de..8806dc841e 100644
>>>> --- a/tests/qtest/migration-helpers.c
>>>> +++ b/tests/qtest/migration-helpers.c
>>>> @@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
>>>>        }
>>>>    }
>>>>    
>>>> -static char *
>>>> -migrate_get_socket_address(QTestState *who, const char *parameter)
>>>> +static SocketAddress *migrate_get_socket_address(QTestState *who)
>>>>    {
>>>>        QDict *rsp;
>>>> -    char *result;
>>>>        SocketAddressList *addrs;
>>>> +    SocketAddress *addr;
>>>>        Visitor *iv = NULL;
>>>>        QObject *object;
>>>>    
>>>>        rsp = migrate_query(who);
>>>> -    object = qdict_get(rsp, parameter);
>>>> +    object = qdict_get(rsp, "socket-address");
>>> Just a heads up, none of what I'm about to say applies to current
>>> master.
>>>
>>> This can return NULL if there is no socket-address, such as with a file
>>> migration. Then the visitor code below just barfs. It would be nice if
>>> we touched this up eventually.
>> Yes. I agree this is not full proof solution and covers for all the cases.
>> It would only for 'socket-address'. Could you elaborate on what other than
>> socket-address the QObject can have ?
> I can just not have the socket-address, AFAICS. We'd just need to not
> crash if that's the case.

value: {
     "status": "setup",
     "socket-address": [
         {
             "port": "46213",
             "ipv4": true,
             "host": "127.0.0.1",
             "type": "inet"
         }
     ]
}

Okay, I understood your ask here. This is what gets printed from the QDict.
Let me raise a patch to return with a message if the QDict does not have key
with 'socket-address'. This would prevent the crash later on.

I wanted to know what other than "socket-address" key can he QDict give us
because, if that's the case, for other than socket migration, then we can
make this function more generic rather than having it as 
'migrate_get_socket_address'
>>> I only noticed this because I was fiddling with the file migration API
>>> and this series helped me a lot to test my changes. So thanks for that,
>>> Het.
>>>
>>> Another point is: we really need to encourage people to write tests
>>> using the new channels API. I added the FileMigrationArgs with the
>>> 'offset' as a required parameter, not even knowing optional parameters
>>> were a thing. So it's obviously not enough to write support for the new
>>> API if no tests ever touch it.
>> Yes, definitely we need more tests with the new channels API to test other
>> than just tcp connection. I could give a try for vsock and unix with the
>> new QAPI syntax, and add some tests.
>>
>> I also wanted to bring in attention that, this solution I what i feel is
>> also
>> not complete. If we are using new channel syntax for migrate_qmp, then the
>> same syntax should also be used for migrate_qmp_incoming. But we haven't
>> touch that, and it still prints the old syntax. We might need a lot changes
>> in design maybe to incorporate that too in new tests totally with the new
>> syntax.
> Adding migrate_qmp_incoming support should be relatively simple. You had
> patches for that in another version, no?
No Fabiano, what I meant was, in migration-test.c, change in 
migrate_incoming_qmp
would need to change the callback function and ultimately change all the 
callback
handlers ? In that sense, it would require a big change ?
Inside the migrate_incoming_qmp function, adding implementation for 
channels is
same as other migrate_* function.
>> Another thing that you also noted down while discussing on the patches that
>> we should have a standard pattern on how to define the migration tests. Even
>> that would be helpful for the users, on how to add new tests, where to add
>> new tests in the file, and which test is needed to run if a specific change
>> needs to be tested.
>>
>>>>    
>>>>        iv = qobject_input_visitor_new(object);
>>>>        visit_type_SocketAddressList(iv, NULL, &addrs, &error_abort);
>>>> +    addr = addrs->value;
>>>>        visit_free(iv);
>>>>    
>>>> -    /* we are only using a single address */
>>>> -    result = SocketAddress_to_str(addrs->value);
>>>> -
>>>> -    qapi_free_SocketAddressList(addrs);
>>>>        qobject_unref(rsp);
>>>> -    return result;
>>>> +    return addr;
>>>> +}
>>>> +
>>>> +static char *
>>>> +migrate_get_connect_uri(QTestState *who)
>>>> +{
>>>> +    SocketAddress *addrs;
>>>> +    char *connect_uri;
>>>> +
>>>> +    addrs = migrate_get_socket_address(who);
>>>> +    connect_uri = SocketAddress_to_str(addrs);
>>>> +
>>>> +    qapi_free_SocketAddress(addrs);
>>>> +    return connect_uri;
>>>>    }
>>>>    
>>>>    bool migrate_watch_for_events(QTestState *who, const char *name,
>>>> @@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
>>>>    
>>>>        g_assert(!qdict_haskey(args, "uri"));
>>>>        if (!uri) {
>>>> -        connect_uri = migrate_get_socket_address(to, "socket-address");
>>>> +        connect_uri = migrate_get_connect_uri(to);
>>>>        }
>>>>        qdict_put_str(args, "uri", uri ? uri : connect_uri);
>> Regards,
>> Het Gala
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 8747 bytes --]