Het Gala <het.g...@nutanix.com> writes:
> On 18/03/24 7:46 pm, Fabiano Rosas wrote:
>> Het Gala<het.g...@nutanix.com> writes:
>>
>>> On 15/03/24 6:28 pm, Fabiano Rosas wrote:
>>>> Het Gala<het.g...@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.g...@nutanix.com>
>>>>> Suggested-by: Fabiano Rosas<faro...@suse.de>
>>>>> Reviewed-by: Fabiano Rosas<faro...@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
