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

[Fabiano Rosas <farosas@suse.de>]

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.

>
>> 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?

>
> 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