Re: [PATCH V4 16/19] tests/qtest: defer connection

[Steven Sistare <steven.sistare@oracle.com>]

On 12/19/2024 10:46 AM, Peter Xu wrote:
> On Mon, Dec 02, 2024 at 05:20:08AM -0800, Steve Sistare wrote:
>> Add an option to defer making the connecting to the monitor and qtest
>> sockets when calling qtest_init_with_env.  The client makes the connection
>> later by calling qtest_connect_deferred and qtest_qmp_handshake.
>>
>> A test cannot specify port=0 for a deferred connection, because qmp_migrate
>> cannot query for the assigned port, because the monitor is not connected
>> yet.  However, even if the test does not specify port=0, qmp_migrate ->
>> migrate_set_ports unconditionally queries connection parameters.
>> Modify migrate_set_ports to only query when port=0.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> 
> Looks mostly good, nitpicks inline..
> 
>> ---
>>   tests/qtest/libqtest.c          | 80 +++++++++++++++++++++++++----------------
>>   tests/qtest/libqtest.h          | 19 +++++++++-
>>   tests/qtest/migration-helpers.c | 19 +++++-----
>>   tests/qtest/migration-test.c    |  4 +--
>>   4 files changed, 80 insertions(+), 42 deletions(-)
>>
>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
>> index 817fd7a..31c4032 100644
>> --- a/tests/qtest/libqtest.c
>> +++ b/tests/qtest/libqtest.c
>> @@ -75,6 +75,8 @@ struct QTestState
>>   {
>>       int fd;
>>       int qmp_fd;
>> +    int sock;
>> +    int qmpsock;
>>       pid_t qemu_pid;  /* our child QEMU process */
>>       int wstatus;
>>   #ifdef _WIN32
>> @@ -442,18 +444,19 @@ static QTestState *G_GNUC_PRINTF(2, 3) qtest_spawn_qemu(const char *qemu_bin,
>>       return s;
>>   }
>>   
>> +static char *qtest_socket_path(const char *suffix)
>> +{
>> +    return g_strdup_printf("%s/qtest-%d.%s", g_get_tmp_dir(), getpid(), suffix);
>> +}
>> +
>>   static QTestState *qtest_init_internal(const char *qemu_bin,
>> -                                       const char *extra_args)
>> +                                       const char *extra_args,
>> +                                       bool defer_connect)
> 
> Suggest to stick with positive logic naming.
> 
> That is, s/defer_connect/do_connect/ or similar, then invert the values in
> callers.

Will do, and will rename qtest_connect_deferred -> qtest_connect.

Thanks for reviewing these last few patches.

>>   {
>>       QTestState *s;
>>       int sock, qmpsock, i;
>> -    gchar *socket_path;
>> -    gchar *qmp_socket_path;
>> -
>> -    socket_path = g_strdup_printf("%s/qtest-%d.sock",
>> -                                  g_get_tmp_dir(), getpid());
>> -    qmp_socket_path = g_strdup_printf("%s/qtest-%d.qmp",
>> -                                      g_get_tmp_dir(), getpid());
>> +    g_autofree gchar *socket_path = qtest_socket_path("sock");
>> +    g_autofree gchar *qmp_socket_path = qtest_socket_path("qmp");
>>   
>>       /*
>>        * It's possible that if an earlier test run crashed it might
>> @@ -485,22 +488,17 @@ static QTestState *qtest_init_internal(const char *qemu_bin,
>>       qtest_client_set_rx_handler(s, qtest_client_socket_recv_line);
>>       qtest_client_set_tx_handler(s, qtest_client_socket_send);
>>   
>> -    s->fd = socket_accept(sock);
>> -    if (s->fd >= 0) {
>> -        s->qmp_fd = socket_accept(qmpsock);
>> -    }
>> -    unlink(socket_path);
>> -    unlink(qmp_socket_path);
>> -    g_free(socket_path);
>> -    g_free(qmp_socket_path);
>> -
>> -    g_assert(s->fd >= 0 && s->qmp_fd >= 0);
>> -
>>       s->rx = g_string_new("");
>>       for (i = 0; i < MAX_IRQ; i++) {
>>           s->irq_level[i] = false;
>>       }
>>   
>> +    s->sock = sock;
>> +    s->qmpsock = qmpsock;
>> +    if (!defer_connect) {
>> +        qtest_connect_deferred(s);
> 
> Now qtest_connect_deferred() itself has nothing to do with the "defer"
> concept.. it is the helper to connect the sockets, so maybe better call it
> qtest_connect_socks(), or similar.
> 
>> +    }
>> +
>>       /*
>>        * Stopping QEMU for debugging is not supported on Windows.
>>        *
>> @@ -515,34 +513,54 @@ static QTestState *qtest_init_internal(const char *qemu_bin,
>>       }
>>   #endif
>>   
>> +   return s;
>> +}
>> +
>> +void qtest_connect_deferred(QTestState *s)
>> +{
>> +    g_autofree gchar *socket_path = qtest_socket_path("sock");
>> +    g_autofree gchar *qmp_socket_path = qtest_socket_path("qmp");
>> +
>> +    g_assert(s->sock >= 0 && s->qmpsock >= 0);
>> +    s->fd = socket_accept(s->sock);
>> +    if (s->fd >= 0) {
>> +        s->qmp_fd = socket_accept(s->qmpsock);
>> +    }
>> +    unlink(socket_path);
>> +    unlink(qmp_socket_path);
>> +    g_assert(s->fd >= 0 && s->qmp_fd >= 0);
>> +    s->sock = s->qmpsock = -1;
>>       /* ask endianness of the target */
>> -
>>       s->big_endian = qtest_query_target_endianness(s);
>> -
>> -   return s;
>>   }
>>   
>>   QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
>>   {
>> -    return qtest_init_internal(qtest_qemu_binary(NULL), extra_args);
>> +    return qtest_init_internal(qtest_qemu_binary(NULL), extra_args, false);
>>   }
>>   
>> -QTestState *qtest_init_with_env(const char *var, const char *extra_args)
>> +void qtest_qmp_handshake(QTestState *s)
>>   {
>> -    QTestState *s = qtest_init_internal(qtest_qemu_binary(var), extra_args);
>> -    QDict *greeting;
>> -
>>       /* Read the QMP greeting and then do the handshake */
>> -    greeting = qtest_qmp_receive(s);
>> +    QDict *greeting = qtest_qmp_receive(s);
>>       qobject_unref(greeting);
>>       qobject_unref(qtest_qmp(s, "{ 'execute': 'qmp_capabilities' }"));
>> +}
>>   
>> +QTestState *qtest_init_with_env(const char *var, const char *extra_args,
>> +                                bool defer_connect)
>> +{
>> +    QTestState *s = qtest_init_internal(qtest_qemu_binary(var), extra_args,
>> +                                        defer_connect);
>> +    if (!defer_connect) {
>> +        qtest_qmp_handshake(s);
>> +    }
>>       return s;
>>   }
>>   
>>   QTestState *qtest_init(const char *extra_args)
>>   {
>> -    return qtest_init_with_env(NULL, extra_args);
>> +    return qtest_init_with_env(NULL, extra_args, false);
>>   }
>>   
>>   QTestState *qtest_vinitf(const char *fmt, va_list ap)
>> @@ -1523,7 +1541,7 @@ static struct MachInfo *qtest_get_machines(const char *var)
>>   
>>       silence_spawn_log = !g_test_verbose();
>>   
>> -    qts = qtest_init_with_env(qemu_var, "-machine none");
>> +    qts = qtest_init_with_env(qemu_var, "-machine none", false);
>>       response = qtest_qmp(qts, "{ 'execute': 'query-machines' }");
>>       g_assert(response);
>>       list = qdict_get_qlist(response, "return");
>> @@ -1578,7 +1596,7 @@ static struct CpuModel *qtest_get_cpu_models(void)
>>   
>>       silence_spawn_log = !g_test_verbose();
>>   
>> -    qts = qtest_init_with_env(NULL, "-machine none");
>> +    qts = qtest_init_with_env(NULL, "-machine none", false);
>>       response = qtest_qmp(qts, "{ 'execute': 'query-cpu-definitions' }");
>>       g_assert(response);
>>       list = qdict_get_qlist(response, "return");
>> diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
>> index beb96b1..db76f2c 100644
>> --- a/tests/qtest/libqtest.h
>> +++ b/tests/qtest/libqtest.h
>> @@ -60,13 +60,15 @@ QTestState *qtest_init(const char *extra_args);
>>    * @var: Environment variable from where to take the QEMU binary
>>    * @extra_args: Other arguments to pass to QEMU.  CAUTION: these
>>    * arguments are subject to word splitting and shell evaluation.
>> + * @defer_connect: do not connect to qemu monitor and qtest socket.
>>    *
>>    * Like qtest_init(), but use a different environment variable for the
>>    * QEMU binary.
>>    *
>>    * Returns: #QTestState instance.
>>    */
>> -QTestState *qtest_init_with_env(const char *var, const char *extra_args);
>> +QTestState *qtest_init_with_env(const char *var, const char *extra_args,
>> +                                bool defer_connect);
>>   
>>   /**
>>    * qtest_init_without_qmp_handshake:
>> @@ -78,6 +80,21 @@ QTestState *qtest_init_with_env(const char *var, const char *extra_args);
>>   QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
>>   
>>   /**
>> + * qtest_connect_deferred:
>> + * @s: #QTestState instance to connect
>> + * Connect to qemu monitor and qtest socket, after deferring them in
>> + * qtest_init_with_env.  Does not handshake with the monitor.
>> + */
>> +void qtest_connect_deferred(QTestState *s);
>> +
>> +/**
>> + * qtest_qmp_handshake:
>> + * @s: #QTestState instance to operate on.
>> + * Perform handshake after connecting to qemu monitor.
>> + */
>> +void qtest_qmp_handshake(QTestState *s);
>> +
>> +/**
>>    * qtest_init_with_serial:
>>    * @extra_args: other arguments to pass to QEMU.  CAUTION: these
>>    * arguments are subject to word splitting and shell evaluation.
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index 3f8ba7f..9f39401 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -127,25 +127,28 @@ migrate_get_connect_qdict(QTestState *who)
>>   
>>   static void migrate_set_ports(QTestState *to, QList *channel_list)
>>   {
>> -    QDict *addr;
>> +    g_autoptr(QDict) addr = NULL;
>>       QListEntry *entry;
>>       const char *addr_port = NULL;
>>   
>> -    addr = migrate_get_connect_qdict(to);
>> -
>>       QLIST_FOREACH_ENTRY(channel_list, entry) {
>>           QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
>>           QDict *addrdict = qdict_get_qdict(channel, "addr");
>>   
>> -        if (qdict_haskey(addrdict, "port") &&
>> -            qdict_haskey(addr, "port") &&
>> -            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
>> +        if (!qdict_haskey(addrdict, "port") ||
>> +            strcmp(qdict_get_str(addrdict, "port"), "0")) {
>> +            continue;
>> +        }
>> +
>> +        if (!addr) {
>> +            addr = migrate_get_connect_qdict(to);
> 
> May be good to add a comment above on why the query was done only lazily.
> 
> Meanwhile this chunk of change can be separate; it's relevant to the defer
> idea but still pretty standalone change. Can be one small patch prior to
> this one, IMHO.

Will do both.

> Optional idea, can be for later: if QTestState can have the state showing
> whether the QMP is ready, we could already assert making sure the query
> happens only if the QMP is available.

Good idea, I'll add it now in a small patch.  It already caught bugs as I
flipped defer_connect to do_connect :)

- Steve

>> +        }
>> +
>> +        if (qdict_haskey(addr, "port")) {
>>               addr_port = qdict_get_str(addr, "port");
>>               qdict_put_str(addrdict, "port", addr_port);
>>           }
>>       }
>> -
>> -    qobject_unref(addr);
>>   }
>>   
>>   bool migrate_watch_for_events(QTestState *who, const char *name,
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 64e1c50..b7001b0 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -844,7 +844,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>                                    args->opts_source ? args->opts_source : "",
>>                                    ignore_stderr);
>>       if (!args->only_target) {
>> -        *from = qtest_init_with_env(QEMU_ENV_SRC, cmd_source);
>> +        *from = qtest_init_with_env(QEMU_ENV_SRC, cmd_source, false);
>>           qtest_qmp_set_event_callback(*from,
>>                                        migrate_watch_for_events,
>>                                        &src_state);
>> @@ -865,7 +865,7 @@ static int test_migrate_start(QTestState **from, QTestState **to,
>>                                    shmem_opts ? shmem_opts : "",
>>                                    args->opts_target ? args->opts_target : "",
>>                                    ignore_stderr);
>> -    *to = qtest_init_with_env(QEMU_ENV_DST, cmd_target);
>> +    *to = qtest_init_with_env(QEMU_ENV_DST, cmd_target, false);
>>       qtest_qmp_set_event_callback(*to,
>>                                    migrate_watch_for_events,
>>                                    &dst_state);
>> -- 
>> 1.8.3.1
>>
>