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

[Fabiano Rosas]

Het Gala <het.gala@nutanix.com> writes:

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

For now, there's nothing else. Let's just ignore when socket-address is
missing in the reply so we don't break future tests that use a
non-socket type.

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

You could add the parameter to migrate_incoming_qmp and use NULL when
calling. The callbacks don't need to be changed. When we add more tests
then we'd alter the callbacks accordingly.

I might convert the file tests soon, you can leave that part to me if
you want.

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