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