qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Steven Sistare
Subject: Re: [PATCH V4 16/19] tests/qtest: defer connection
Date: Thu, 19 Dec 2024 17:33:49 -0500
User-agent: Mozilla Thunderbird

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






reply via email to

[Prev in Thread] Current Thread [Next in Thread]