[PATCH v3 0/7] chardev: postpone connect

[PATCH v3 0/7] chardev: postpone connect

[Vladimir Sementsov-Ogievskiy]

Hi all!

That's only a preparation for backend-transfer migration of
vhost-user-blk, and introduced DEFINE_PROP_CHR_NO_CONNECT()
is unused now.

v2 of "vhost-user-blk: live-backend local migration" is coming
soon, and will be based on this series (and will use
DEFINE_PROP_CHR_NO_CONNECT of course).

If the design gets general approval, I'll try to update other
chardev backends, to avoid supporting two different initialization
APIs in future.


Changes in v3:

01: add r-b by Marc-André
02: - rename function and save comment
    - add r-b by Marc-André
03: fix local_err to errp
04-06: add r-b by Marc-André
07: reworked to introduce DEFINE_PROP_CHR_NO_CONNECT


For backend-transfer migration we are going to pass
vhost-user-blk fds, including backend chardev fd to the target
in migration stream (backed by UNIX domain socket).

So, on the target, we want to know, should we call connect(),
or is it a backend-transfer migration, and we should wait for
incoming fd.

But at initialization time we can't know it: user may setup
migration parameters (enabling backend-transfer) later.

So, let's postpone chardev open/connect phase up to attaching
to frontend. At this point we can split the behavior:

If frontend is a device, which define chardev property
using DEFINE_PROP_CHR_NO_CONNECT (at least, vhost-user-blk will
behave this way soon), then do not connect(), let the device decide
when to do connect().

Otherwise (basic DEFINE_PROP_CHR, or other calls to qemu_chr_fe_init()),
do connect() at point of attaching frontend.

The series is based on
  [PATCH 0/2] remove deprecated 'reconnect' options
Based-on: <20250924133309.334631-1-vsementsov@yandex-team.ru>

Vladimir Sementsov-Ogievskiy (7):
  chardev/char-socket: simplify reconnect-ms handling
  chardev/char: split chardev_init_logfd() out of qemu_char_open()
  chardev/char: qemu_char_open(): add return value
  chardev/char: move filename and be_opened handling to qemu_char_open()
  chardev/char: introduce .init() + .connect() initialization interface
  chardev/char-socket: move to .init + .connect api
  chardev: introduce DEFINE_PROP_CHR_NO_CONNECT

 chardev/char-fe.c                   |  12 ++-
 chardev/char-socket.c               |  64 ++++++++-------
 chardev/char.c                      | 118 ++++++++++++++++++++--------
 hw/core/qdev-properties-system.c    |  26 +++++-
 include/chardev/char-fe.h           |   6 +-
 include/chardev/char-socket.h       |   1 +
 include/chardev/char.h              |  22 +++++-
 include/hw/qdev-properties-system.h |   3 +
 tests/unit/test-char.c              |  14 ++--
 ui/dbus-chardev.c                   |  12 ++-
 10 files changed, 203 insertions(+), 75 deletions(-)

-- 
2.48.1

[PATCH v3 1/7] chardev/char-socket: simplify reconnect-ms handling

[Vladimir Sementsov-Ogievskiy]

We pass it to qmp_chardev_open_socket_client() only to write
to s->reconnect_time_ms. Let's simply set this field earlier,
together with other options.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-socket.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 62852e3caf..f3bc6290d2 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1274,18 +1274,16 @@ skip_listen:
 
 
 static int qmp_chardev_open_socket_client(Chardev *chr,
-                                          int64_t reconnect_ms,
                                           Error **errp)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
-    if (reconnect_ms > 0) {
-        s->reconnect_time_ms = reconnect_ms;
+    if (s->reconnect_time_ms > 0) {
         tcp_chr_connect_client_async(chr);
         return 0;
-    } else {
-        return tcp_chr_connect_client_sync(chr, errp);
     }
+
+    return tcp_chr_connect_client_sync(chr, errp);
 }
 
 
@@ -1378,7 +1376,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
     bool is_tn3270      = sock->has_tn3270  ? sock->tn3270  : false;
     bool is_waitconnect = sock->has_wait    ? sock->wait    : false;
     bool is_websock     = sock->has_websocket ? sock->websocket : false;
-    int64_t reconnect_ms = sock->has_reconnect_ms ? sock->reconnect_ms : 0;
     SocketAddress *addr;
 
     s->is_listen = is_listen;
@@ -1386,6 +1383,8 @@ static void qmp_chardev_open_socket(Chardev *chr,
     s->is_tn3270 = is_tn3270;
     s->is_websock = is_websock;
     s->do_nodelay = do_nodelay;
+    s->reconnect_time_ms = sock->has_reconnect_ms ? sock->reconnect_ms : 0;
+
     if (sock->tls_creds) {
         Object *creds;
         creds = object_resolve_path_component(
@@ -1450,7 +1449,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
             return;
         }
     } else {
-        if (qmp_chardev_open_socket_client(chr, reconnect_ms, errp) < 0) {
+        if (qmp_chardev_open_socket_client(chr, errp) < 0) {
             return;
         }
     }
-- 
2.48.1

[PATCH v3 2/7] chardev/char: split chardev_init_logfd() out of qemu_char_open()

[Vladimir Sementsov-Ogievskiy]

We are going to share new chardev_init_logfd() with further
alternative initialization interface. Let qemu_char_open() be
a wrapper for .open(), and its artifacts (handle be_opened if
was not set to false by backend, and filename).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char.c | 50 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index a43b7e5481..4a531265c1 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -250,22 +250,6 @@ static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
                            bool *be_opened, Error **errp)
 {
     ChardevClass *cc = CHARDEV_GET_CLASS(chr);
-    /* Any ChardevCommon member would work */
-    ChardevCommon *common = backend ? backend->u.null.data : NULL;
-
-    if (common && common->logfile) {
-        int flags = O_WRONLY;
-        if (common->has_logappend &&
-            common->logappend) {
-            flags |= O_APPEND;
-        } else {
-            flags |= O_TRUNC;
-        }
-        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
-        if (chr->logfd < 0) {
-            return;
-        }
-    }
 
     if (cc->open) {
         cc->open(chr, backend, be_opened, errp);
@@ -1000,6 +984,29 @@ void qemu_chr_set_feature(Chardev *chr,
     return set_bit(feature, chr->features);
 }
 
+static bool chardev_init_common(Chardev *chr, ChardevBackend *backend,
+                                Error **errp)
+{
+    /* Any ChardevCommon member would work */
+    ChardevCommon *common = backend ? backend->u.null.data : NULL;
+
+    if (common && common->logfile) {
+        int flags = O_WRONLY;
+        if (common->has_logappend &&
+            common->logappend) {
+            flags |= O_APPEND;
+        } else {
+            flags |= O_TRUNC;
+        }
+        chr->logfd = qemu_create(common->logfile, flags, 0666, errp);
+        if (chr->logfd < 0) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 static Chardev *chardev_new(const char *id, const char *typename,
                             ChardevBackend *backend,
                             GMainContext *gcontext,
@@ -1020,11 +1027,14 @@ static Chardev *chardev_new(const char *id, const char *typename,
     chr->label = g_strdup(id);
     chr->gcontext = gcontext;
 
+    if (!chardev_init_common(chr, backend, errp)) {
+        goto fail;
+    }
+
     qemu_char_open(chr, backend, &be_opened, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        object_unref(obj);
-        return NULL;
+        goto fail;
     }
 
     if (!chr->filename) {
@@ -1035,6 +1045,10 @@ static Chardev *chardev_new(const char *id, const char *typename,
     }
 
     return chr;
+
+fail:
+    object_unref(obj);
+    return NULL;
 }
 
 Chardev *qemu_chardev_new(const char *id, const char *typename,
-- 
2.48.1

[PATCH v3 3/7] chardev/char: qemu_char_open(): add return value

[Vladimir Sementsov-Ogievskiy]

Accordingly with recommendations in include/qapi/error.h accompany
errp by boolean return value and get rid of error propagation.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 chardev/char.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 4a531265c1..c874a2d31e 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -246,14 +246,20 @@ int qemu_chr_add_client(Chardev *s, int fd)
         CHARDEV_GET_CLASS(s)->chr_add_client(s, fd) : -1;
 }
 
-static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
+static bool qemu_char_open(Chardev *chr, ChardevBackend *backend,
                            bool *be_opened, Error **errp)
 {
+    ERRP_GUARD();
     ChardevClass *cc = CHARDEV_GET_CLASS(chr);
 
     if (cc->open) {
         cc->open(chr, backend, be_opened, errp);
+        if (*errp) {
+            return false;
+        }
     }
+
+    return true;
 }
 
 static void char_init(Object *obj)
@@ -1015,7 +1021,6 @@ static Chardev *chardev_new(const char *id, const char *typename,
 {
     Object *obj;
     Chardev *chr = NULL;
-    Error *local_err = NULL;
     bool be_opened = true;
 
     assert(g_str_has_prefix(typename, "chardev-"));
@@ -1031,9 +1036,7 @@ static Chardev *chardev_new(const char *id, const char *typename,
         goto fail;
     }
 
-    qemu_char_open(chr, backend, &be_opened, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!qemu_char_open(chr, backend, &be_opened, errp)) {
         goto fail;
     }
 
-- 
2.48.1

[PATCH v3 4/7] chardev/char: move filename and be_opened handling to qemu_char_open()

[Vladimir Sementsov-Ogievskiy]

Absent filename and necessity to send CHR_EVENT_OPENED are artifacts
of .open(). Handle them in qemu_char_open().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index c874a2d31e..27290e26fb 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -247,18 +247,27 @@ int qemu_chr_add_client(Chardev *s, int fd)
 }
 
 static bool qemu_char_open(Chardev *chr, ChardevBackend *backend,
-                           bool *be_opened, Error **errp)
+                           const char *default_filename, Error **errp)
 {
     ERRP_GUARD();
     ChardevClass *cc = CHARDEV_GET_CLASS(chr);
+    bool be_opened = true;
 
     if (cc->open) {
-        cc->open(chr, backend, be_opened, errp);
+        cc->open(chr, backend, &be_opened, errp);
         if (*errp) {
             return false;
         }
     }
 
+    if (!chr->filename) {
+        chr->filename = g_strdup(default_filename);
+    }
+
+    if (be_opened) {
+        qemu_chr_be_event(chr, CHR_EVENT_OPENED);
+    }
+
     return true;
 }
 
@@ -1021,7 +1030,6 @@ static Chardev *chardev_new(const char *id, const char *typename,
 {
     Object *obj;
     Chardev *chr = NULL;
-    bool be_opened = true;
 
     assert(g_str_has_prefix(typename, "chardev-"));
     assert(id);
@@ -1036,17 +1044,10 @@ static Chardev *chardev_new(const char *id, const char *typename,
         goto fail;
     }
 
-    if (!qemu_char_open(chr, backend, &be_opened, errp)) {
+    if (!qemu_char_open(chr, backend, typename + 8, errp)) {
         goto fail;
     }
 
-    if (!chr->filename) {
-        chr->filename = g_strdup(typename + 8);
-    }
-    if (be_opened) {
-        qemu_chr_be_event(chr, CHR_EVENT_OPENED);
-    }
-
     return chr;
 
 fail:
-- 
2.48.1

[PATCH v3 5/7] chardev/char: introduce .init() + .connect() initialization interface

[Vladimir Sementsov-Ogievskiy]

We'll need a possibility to postpone connect step to later point in
time to implement backend-transfer migration feature for vhost-user-blk
in further commits. Let's start with new char interface for backends.

.init() takes QAPI parameters and should parse them, called early

.connect() should actually establish a connection, and postponed to
the point of attaching to frontend. Called at later point, either
at time of attaching frontend, either from qemu_chr_wait_connected().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-fe.c      |  4 ++++
 chardev/char.c         | 39 +++++++++++++++++++++++++++++++++++++--
 include/chardev/char.h | 22 +++++++++++++++++++++-
 3 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 158a5f4f55..973fed5bea 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -193,6 +193,10 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
 {
     unsigned int tag = 0;
 
+    if (!qemu_chr_connect(s, errp)) {
+        return false;
+    }
+
     if (s) {
         if (CHARDEV_IS_MUX(s)) {
             MuxChardev *d = MUX_CHARDEV(s);
diff --git a/chardev/char.c b/chardev/char.c
index 27290e26fb..409f3aac1c 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -33,6 +33,7 @@
 #include "qapi/error.h"
 #include "qapi/qapi-commands-char.h"
 #include "qapi/qmp/qerror.h"
+#include "qom/object.h"
 #include "system/replay.h"
 #include "qemu/help_option.h"
 #include "qemu/module.h"
@@ -338,10 +339,29 @@ static bool qemu_chr_is_busy(Chardev *s)
     }
 }
 
+bool qemu_chr_connect(Chardev *chr, Error **errp)
+{
+    ChardevClass *cc = CHARDEV_GET_CLASS(chr);
+
+    if (chr->connect_postponed) {
+        assert(cc->connect);
+        chr->connect_postponed = false;
+        if (!cc->connect(chr, errp)) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 int qemu_chr_wait_connected(Chardev *chr, Error **errp)
 {
     ChardevClass *cc = CHARDEV_GET_CLASS(chr);
 
+    if (!qemu_chr_connect(chr, errp)) {
+        return -1;
+    }
+
     if (cc->chr_wait_connected) {
         return cc->chr_wait_connected(chr, errp);
     }
@@ -1030,6 +1050,7 @@ static Chardev *chardev_new(const char *id, const char *typename,
 {
     Object *obj;
     Chardev *chr = NULL;
+    ChardevClass *cc;
 
     assert(g_str_has_prefix(typename, "chardev-"));
     assert(id);
@@ -1044,8 +1065,22 @@ static Chardev *chardev_new(const char *id, const char *typename,
         goto fail;
     }
 
-    if (!qemu_char_open(chr, backend, typename + 8, errp)) {
-        goto fail;
+    cc = CHARDEV_GET_CLASS(chr);
+
+    if (cc->init) {
+        assert(!cc->open);
+        assert(cc->connect);
+
+        if (!cc->init(chr, backend, errp)) {
+            goto fail;
+        }
+        assert(chr->filename);
+
+        chr->connect_postponed = true;
+    } else {
+        if (!qemu_char_open(chr, backend, typename + 8, errp)) {
+            goto fail;
+        }
     }
 
     return chr;
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 429852f8d9..ebadaf3482 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -63,6 +63,7 @@ struct Chardev {
     CharBackend *be;
     char *label;
     char *filename;
+    bool connect_postponed;
     int logfd;
     int be_open;
     /* used to coordinate the chardev-change special-case: */
@@ -225,6 +226,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
                                 bool permit_mux_mon);
 int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
 #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
+bool qemu_chr_connect(Chardev *chr, Error **errp);
 int qemu_chr_wait_connected(Chardev *chr, Error **errp);
 
 #define TYPE_CHARDEV "chardev"
@@ -259,10 +261,28 @@ struct ChardevClass {
     /* parse command line options and populate QAPI @backend */
     void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
 
-    /* called after construction, open/starts the backend */
+    /*
+     * Called after construction, create and open/starts the backend,
+     * mutual exclusive with .init. .connect must not be defined when
+     * .open is defined.
+     */
     void (*open)(Chardev *chr, ChardevBackend *backend,
                  bool *be_opened, Error **errp);
 
+    /*
+     * Called after construction, create the backend, mutual exclusive
+     * with .open, and must be accompanied by .connect.
+     * Must set chr-filename.
+     */
+    bool (*init)(Chardev *chr, ChardevBackend *backend,
+                 Error **errp);
+
+    /*
+     * Called after .init(), open/starts the backend, mutual exclusive
+     * with .open. Must send CHR_EVENT_OPENED.
+     */
+    bool (*connect)(Chardev *chr, Error **errp);
+
     /* write buf to the backend */
     int (*chr_write)(Chardev *s, const uint8_t *buf, int len);
 
-- 
2.48.1

[PATCH v3 6/7] chardev/char-socket: move to .init + .connect api

[Vladimir Sementsov-Ogievskiy]

Move char-socket to new API. This will help to realize backend-transfer
feature for vhost-user-blk.

With this commit qemu_chr_fe_init() starts to do connecting, so we
should handle its errors instead of passing &error_abort.

Also, move qemu_chr_fe_init() in test-char.c, to trigger connect
before trying to get address.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-socket.c         | 55 ++++++++++++++++++++---------------
 chardev/char.c                |  7 +++--
 include/chardev/char-socket.h |  1 +
 tests/unit/test-char.c        | 14 ++++-----
 ui/dbus-chardev.c             | 12 ++++++--
 5 files changed, 54 insertions(+), 35 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index f3bc6290d2..0a5738c158 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1287,6 +1287,25 @@ static int qmp_chardev_open_socket_client(Chardev *chr,
 }
 
 
+static bool char_socket_connect(Chardev *chr, Error **errp)
+{
+    SocketChardev *s = SOCKET_CHARDEV(chr);
+
+    if (s->is_listen) {
+        if (qmp_chardev_open_socket_server(chr, s->is_telnet || s->is_tn3270,
+                                           s->is_waitconnect, errp) < 0) {
+            return false;
+        }
+    } else {
+        if (qmp_chardev_open_socket_client(chr, errp) < 0) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
+
 static bool qmp_chardev_validate_socket(ChardevSocket *sock,
                                         SocketAddress *addr,
                                         Error **errp)
@@ -1363,10 +1382,9 @@ static bool qmp_chardev_validate_socket(ChardevSocket *sock,
 }
 
 
-static void qmp_chardev_open_socket(Chardev *chr,
-                                    ChardevBackend *backend,
-                                    bool *be_opened,
-                                    Error **errp)
+static bool char_socket_init(Chardev *chr,
+                             ChardevBackend *backend,
+                             Error **errp)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
     ChardevSocket *sock = backend->u.socket.data;
@@ -1374,7 +1392,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
     bool is_listen      = sock->has_server  ? sock->server  : true;
     bool is_telnet      = sock->has_telnet  ? sock->telnet  : false;
     bool is_tn3270      = sock->has_tn3270  ? sock->tn3270  : false;
-    bool is_waitconnect = sock->has_wait    ? sock->wait    : false;
     bool is_websock     = sock->has_websocket ? sock->websocket : false;
     SocketAddress *addr;
 
@@ -1383,6 +1400,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
     s->is_tn3270 = is_tn3270;
     s->is_websock = is_websock;
     s->do_nodelay = do_nodelay;
+    s->is_waitconnect = sock->has_wait ? sock->wait : false;
     s->reconnect_time_ms = sock->has_reconnect_ms ? sock->reconnect_ms : 0;
 
     if (sock->tls_creds) {
@@ -1392,7 +1410,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
         if (!creds) {
             error_setg(errp, "No TLS credentials with id '%s'",
                        sock->tls_creds);
-            return;
+            return false;
         }
         s->tls_creds = (QCryptoTLSCreds *)
             object_dynamic_cast(creds,
@@ -1400,7 +1418,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
         if (!s->tls_creds) {
             error_setg(errp, "Object with id '%s' is not TLS credentials",
                        sock->tls_creds);
-            return;
+            return false;
         }
         object_ref(OBJECT(s->tls_creds));
         if (!qcrypto_tls_creds_check_endpoint(s->tls_creds,
@@ -1408,7 +1426,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
                                           ? QCRYPTO_TLS_CREDS_ENDPOINT_SERVER
                                           : QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
                                           errp)) {
-            return;
+            return false;
         }
     }
     s->tls_authz = g_strdup(sock->tls_authz);
@@ -1416,7 +1434,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
     s->addr = addr = socket_address_flatten(sock->addr);
 
     if (!qmp_chardev_validate_socket(sock, addr, errp)) {
-        return;
+        return false;
     }
 
     qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
@@ -1433,26 +1451,14 @@ static void qmp_chardev_open_socket(Chardev *chr,
      */
     if (!chr->handover_yank_instance) {
         if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), errp)) {
-            return;
+            return false;
         }
     }
     s->registered_yank = true;
 
-    /* be isn't opened until we get a connection */
-    *be_opened = false;
-
     update_disconnected_filename(s);
 
-    if (s->is_listen) {
-        if (qmp_chardev_open_socket_server(chr, is_telnet || is_tn3270,
-                                           is_waitconnect, errp) < 0) {
-            return;
-        }
-    } else {
-        if (qmp_chardev_open_socket_client(chr, errp) < 0) {
-            return;
-        }
-    }
+    return true;
 }
 
 static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
@@ -1576,7 +1582,8 @@ static void char_socket_class_init(ObjectClass *oc, const void *data)
     cc->supports_yank = true;
 
     cc->parse = qemu_chr_parse_socket;
-    cc->open = qmp_chardev_open_socket;
+    cc->init = char_socket_init;
+    cc->connect = char_socket_connect;
     cc->chr_wait_connected = tcp_chr_wait_connected;
     cc->chr_write = tcp_chr_write;
     cc->chr_sync_read = tcp_chr_sync_read;
diff --git a/chardev/char.c b/chardev/char.c
index 409f3aac1c..b68d44e394 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1222,12 +1222,15 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
     }
 
     chr->be = NULL;
-    qemu_chr_fe_init(be, chr_new, &error_abort);
+    if (!qemu_chr_fe_init(be, chr_new, errp)) {
+        object_unref(OBJECT(chr_new));
+        return NULL;
+    }
 
     if (be->chr_be_change(be->opaque) < 0) {
         error_setg(errp, "Chardev '%s' change failed", chr_new->label);
         chr_new->be = NULL;
-        qemu_chr_fe_init(be, chr, &error_abort);
+        qemu_chr_fe_init(be, chr, NULL);
         if (closed_sent) {
             qemu_chr_be_event(chr, CHR_EVENT_OPENED);
         }
diff --git a/include/chardev/char-socket.h b/include/chardev/char-socket.h
index d6d13ad37f..0109727eaa 100644
--- a/include/chardev/char-socket.h
+++ b/include/chardev/char-socket.h
@@ -68,6 +68,7 @@ struct SocketChardev {
     bool is_listen;
     bool is_telnet;
     bool is_tn3270;
+    bool is_waitconnect;
     GSource *telnet_source;
     TCPChardevTelnetInit *telnet_init;
 
diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
index f30a39f61f..5c9482a478 100644
--- a/tests/unit/test-char.c
+++ b/tests/unit/test-char.c
@@ -845,6 +845,7 @@ static void char_websock_test(void)
                               0xef, 0xaa, 0xc5, 0x97, /* Masking key */
                               0xec, 0x42              /* Status code */ };
 
+    qemu_chr_fe_init(&be, chr, &error_abort);
     addr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
     qdict = qobject_to(QDict, addr);
     port = qdict_get_str(qdict, "port");
@@ -852,7 +853,6 @@ static void char_websock_test(void)
     handshake_port = g_strdup_printf(handshake, port, port);
     qobject_unref(qdict);
 
-    qemu_chr_fe_init(&be, chr, &error_abort);
     qemu_chr_fe_set_handlers(&be, websock_server_can_read, websock_server_read,
                              NULL, NULL, chr, NULL, true);
 
@@ -1216,6 +1216,8 @@ static void char_socket_server_test(gconstpointer opaque)
     g_assert_nonnull(chr);
     g_assert(!object_property_get_bool(OBJECT(chr), "connected", &error_abort));
 
+    qemu_chr_fe_init(&be, chr, &error_abort);
+
     qaddr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
     g_assert_nonnull(qaddr);
 
@@ -1224,8 +1226,6 @@ static void char_socket_server_test(gconstpointer opaque)
     visit_free(v);
     qobject_unref(qaddr);
 
-    qemu_chr_fe_init(&be, chr, &error_abort);
-
  reconnect:
     data.event = -1;
     data.be = &be;
@@ -1417,6 +1417,8 @@ static void char_socket_client_test(gconstpointer opaque)
     qemu_opts_del(opts);
     g_assert_nonnull(chr);
 
+    qemu_chr_fe_init(&be, chr, &error_abort);
+
     if (config->reconnect) {
         /*
          * If reconnect is set, the connection will be
@@ -1431,8 +1433,6 @@ static void char_socket_client_test(gconstpointer opaque)
                                           &error_abort));
     }
 
-    qemu_chr_fe_init(&be, chr, &error_abort);
-
  reconnect:
     data.event = -1;
     data.be = &be;
@@ -1550,6 +1550,8 @@ static void char_socket_server_two_clients_test(gconstpointer opaque)
     g_assert_nonnull(chr);
     g_assert(!object_property_get_bool(OBJECT(chr), "connected", &error_abort));
 
+    qemu_chr_fe_init(&be, chr, &error_abort);
+
     qaddr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
     g_assert_nonnull(qaddr);
 
@@ -1558,8 +1560,6 @@ static void char_socket_server_two_clients_test(gconstpointer opaque)
     visit_free(v);
     qobject_unref(qaddr);
 
-    qemu_chr_fe_init(&be, chr, &error_abort);
-
     qemu_chr_fe_set_handlers(&be, char_socket_can_read, char_socket_discard_read,
                              count_closed_event, NULL,
                              &closed, NULL, true);
diff --git a/ui/dbus-chardev.c b/ui/dbus-chardev.c
index d05dddaf81..23cf9d6ee9 100644
--- a/ui/dbus-chardev.c
+++ b/ui/dbus-chardev.c
@@ -210,8 +210,14 @@ dbus_chr_open(Chardev *chr, ChardevBackend *backend,
     if (*errp) {
         return;
     }
-    CHARDEV_CLASS(object_class_by_name(TYPE_CHARDEV_SOCKET))->open(
-        chr, be, be_opened, errp);
+    if (!CHARDEV_CLASS(object_class_by_name(TYPE_CHARDEV_SOCKET))->init(
+        chr, be, errp)) {
+        return;
+    }
+    if (!CHARDEV_CLASS(object_class_by_name(TYPE_CHARDEV_SOCKET))->connect(
+        chr, errp)) {
+        return;
+    }
 }
 
 static void
@@ -276,6 +282,8 @@ char_dbus_class_init(ObjectClass *oc, const void *data)
 
     cc->parse = dbus_chr_parse;
     cc->open = dbus_chr_open;
+    cc->init = NULL;
+    cc->connect = NULL;
     cc->chr_set_fe_open = dbus_chr_set_fe_open;
     cc->chr_set_echo = dbus_chr_set_echo;
     klass->parent_chr_be_event = cc->chr_be_event;
-- 
2.48.1

[PATCH v3 7/7] chardev: introduce DEFINE_PROP_CHR_NO_CONNECT

[Vladimir Sementsov-Ogievskiy]

For further vhost-user-blk backend-transfer migration realization we
want to give it (vhost-user-blk) a possibility (and responsibility) to
decide when do connect.

For incoming migration we'll need to postpone connect at least until
early stage of migrate-incoming command, when we already know all
migration parameters and can decide, are we going to do incoming
backend-transfer (and get chardev fd from incoming stream), or we
finally need to connect.

With this patch, we only provide new macro, to define chardev property,
later it will be used in vhost-user-blk instead of DEFINE_PROP_CHR.
Then, vhost-user-blk will call qemu_chr_connect() by hand when needed
(for example through qemu_chr_fe_wait_connected(), which is already
called in vhost_user_blk_realize_connect()).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 chardev/char-fe.c                   | 10 ++++++++--
 hw/core/qdev-properties-system.c    | 26 +++++++++++++++++++++++---
 include/chardev/char-fe.h           |  6 +++++-
 include/hw/qdev-properties-system.h |  3 +++
 4 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 973fed5bea..a0218393e4 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -189,11 +189,12 @@ bool qemu_chr_fe_backend_open(CharBackend *be)
     return be->chr && be->chr->be_open;
 }
 
-bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
+bool qemu_chr_fe_init_ex(CharBackend *b, Chardev *s, bool connect,
+                         Error **errp)
 {
     unsigned int tag = 0;
 
-    if (!qemu_chr_connect(s, errp)) {
+    if (connect && !qemu_chr_connect(s, errp)) {
         return false;
     }
 
@@ -218,6 +219,11 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
     return true;
 }
 
+bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
+{
+    return qemu_chr_fe_init_ex(b, s, true, errp);
+}
+
 void qemu_chr_fe_deinit(CharBackend *b, bool del)
 {
     assert(b);
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 1f810b7ddf..6a0572ca03 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -266,8 +266,8 @@ static void get_chr(Object *obj, Visitor *v, const char *name, void *opaque,
     g_free(p);
 }
 
-static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque,
-                    Error **errp)
+static void do_set_chr(Object *obj, Visitor *v, const char *name, void *opaque,
+                       bool connect, Error **errp)
 {
     ERRP_GUARD();
     const Property *prop = opaque;
@@ -297,13 +297,25 @@ static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque,
     if (s == NULL) {
         error_setg(errp, "Property '%s.%s' can't find value '%s'",
                    object_get_typename(obj), name, str);
-    } else if (!qemu_chr_fe_init(be, s, errp)) {
+    } else if (!qemu_chr_fe_init_ex(be, s, connect, errp)) {
         error_prepend(errp, "Property '%s.%s' can't take value '%s': ",
                       object_get_typename(obj), name, str);
     }
     g_free(str);
 }
 
+static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque,
+                    Error **errp)
+{
+    do_set_chr(obj, v, name, opaque, true, errp);
+}
+
+static void set_chr_no_connect(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    do_set_chr(obj, v, name, opaque, false, errp);
+}
+
 static void release_chr(Object *obj, const char *name, void *opaque)
 {
     const Property *prop = opaque;
@@ -320,6 +332,14 @@ const PropertyInfo qdev_prop_chr = {
     .release = release_chr,
 };
 
+const PropertyInfo qdev_prop_chr_no_connect = {
+    .type  = "str",
+    .description = "ID of a chardev to use as a backend",
+    .get   = get_chr,
+    .set   = set_chr_no_connect,
+    .release = release_chr,
+};
+
 /* --- mac address --- */
 
 /*
diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index 8ef05b3dd0..32013623b3 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -25,15 +25,19 @@ struct CharBackend {
 };
 
 /**
- * qemu_chr_fe_init:
+ * qemu_chr_fe_init(_ex):
  *
  * Initializes a front end for the given CharBackend and
  * Chardev. Call qemu_chr_fe_deinit() to remove the association and
  * release the driver.
+ * Call qemu_chr_connect(), except for the case when connect=false
+ * parameter set for _ex() version.
  *
  * Returns: false on error.
  */
 bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp);
+bool qemu_chr_fe_init_ex(CharBackend *b, Chardev *s, bool connect,
+                         Error **errp);
 
 /**
  * qemu_chr_fe_deinit:
diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
index 9601a11a09..41f68f60b9 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -7,6 +7,7 @@ bool qdev_prop_sanitize_s390x_loadparm(uint8_t *loadparm, const char *str,
                                        Error **errp);
 
 extern const PropertyInfo qdev_prop_chr;
+extern const PropertyInfo qdev_prop_chr_no_connect;
 extern const PropertyInfo qdev_prop_macaddr;
 extern const PropertyInfo qdev_prop_reserved_region;
 extern const PropertyInfo qdev_prop_multifd_compression;
@@ -39,6 +40,8 @@ extern const PropertyInfo qdev_prop_virtio_gpu_output_list;
 
 #define DEFINE_PROP_CHR(_n, _s, _f)             \
     DEFINE_PROP(_n, _s, _f, qdev_prop_chr, CharBackend)
+#define DEFINE_PROP_CHR_NO_CONNECT(_n, _s, _f) \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_chr_no_connect, CharBackend)
 #define DEFINE_PROP_NETDEV(_n, _s, _f)             \
     DEFINE_PROP(_n, _s, _f, qdev_prop_netdev, NICPeers)
 #define DEFINE_PROP_DRIVE(_n, _s, _f) \
-- 
2.48.1

Re: [PATCH v3 2/7] chardev/char: split chardev_init_logfd() out of qemu_char_open()

[Markus Armbruster]

The subject says "split chardev_init_logfd() out of qemu_char_open()".
What the actually does is factor chardev_init_common() out of
qemu_char_open().  Has the commit message gone stale?

Re: [PATCH v3 3/7] chardev/char: qemu_char_open(): add return value

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Accordingly with recommendations in include/qapi/error.h accompany
> errp by boolean return value and get rid of error propagation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  chardev/char.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 4a531265c1..c874a2d31e 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -246,14 +246,20 @@ int qemu_chr_add_client(Chardev *s, int fd)
>          CHARDEV_GET_CLASS(s)->chr_add_client(s, fd) : -1;
>  }
>  
> -static void qemu_char_open(Chardev *chr, ChardevBackend *backend,
> +static bool qemu_char_open(Chardev *chr, ChardevBackend *backend,
>                             bool *be_opened, Error **errp)
>  {
> +    ERRP_GUARD();
>      ChardevClass *cc = CHARDEV_GET_CLASS(chr);
>  
>      if (cc->open) {
>          cc->open(chr, backend, be_opened, errp);
> +        if (*errp) {
> +            return false;
> +        }
>      }
> +
> +    return true;
>  }
>  
>  static void char_init(Object *obj)
> @@ -1015,7 +1021,6 @@ static Chardev *chardev_new(const char *id, const char *typename,
>  {
>      Object *obj;
>      Chardev *chr = NULL;
> -    Error *local_err = NULL;
>      bool be_opened = true;
>  
>      assert(g_str_has_prefix(typename, "chardev-"));
> @@ -1031,9 +1036,7 @@ static Chardev *chardev_new(const char *id, const char *typename,
>          goto fail;
>      }
>  
> -    qemu_char_open(chr, backend, &be_opened, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!qemu_char_open(chr, backend, &be_opened, errp)) {
>          goto fail;
>      }

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v3 2/7] chardev/char: split chardev_init_logfd() out of qemu_char_open()

[Vladimir Sementsov-Ogievskiy]

On 15.10.25 09:35, Markus Armbruster wrote:
> The subject says "split chardev_init_logfd() out of qemu_char_open()".
> What the actually does is factor chardev_init_common() out of
> qemu_char_open().  Has the commit message gone stale?
> 

Oh, right, I've renamed the function in v3.

-- 
Best regards,
Vladimir

Re: [PATCH v3 5/7] chardev/char: introduce .init() + .connect() initialization interface

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> We'll need a possibility to postpone connect step to later point in
> time to implement backend-transfer migration feature for vhost-user-blk
> in further commits. Let's start with new char interface for backends.
>
> .init() takes QAPI parameters and should parse them, called early
>
> .connect() should actually establish a connection, and postponed to
> the point of attaching to frontend. Called at later point, either
> at time of attaching frontend, either from qemu_chr_wait_connected().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

[...]

> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 429852f8d9..ebadaf3482 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -63,6 +63,7 @@ struct Chardev {
>      CharBackend *be;
>      char *label;
>      char *filename;
> +    bool connect_postponed;
>      int logfd;
>      int be_open;
>      /* used to coordinate the chardev-change special-case: */
> @@ -225,6 +226,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
>                                  bool permit_mux_mon);
>  int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
>  #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
> +bool qemu_chr_connect(Chardev *chr, Error **errp);
>  int qemu_chr_wait_connected(Chardev *chr, Error **errp);
>  
>  #define TYPE_CHARDEV "chardev"
> @@ -259,10 +261,28 @@ struct ChardevClass {
>      /* parse command line options and populate QAPI @backend */
>      void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
>  
> -    /* called after construction, open/starts the backend */
> +    /*
> +     * Called after construction, create and open/starts the backend,

What to do mean by "create and open/starts"?

Maybe "create and start"?

> +     * mutual exclusive with .init. .connect must not be defined when

mutually

> +     * .open is defined.
> +     */

Suggest to use @name to refer to a member name.  We do that elsewhere,
and it's easier on the eyes than dots.

>      void (*open)(Chardev *chr, ChardevBackend *backend,
>                   bool *be_opened, Error **errp);
>  
> +    /*
> +     * Called after construction, create the backend, mutual exclusive

mutually

> +     * with .open, and must be accompanied by .connect.

Is it okay to destroy after init() without connect()?

If yes, "must" is misleading.

> +     * Must set chr-filename.

What's chr-filename?

> +     */
> +    bool (*init)(Chardev *chr, ChardevBackend *backend,
> +                 Error **errp);
> +
> +    /*
> +     * Called after .init(), open/starts the backend, mutual exclusive

mutually

> +     * with .open. Must send CHR_EVENT_OPENED.

Must send CHR_EVENT_OPENED when it succeeds, I guess.

> +     */
> +    bool (*connect)(Chardev *chr, Error **errp);
> +
>      /* write buf to the backend */
>      int (*chr_write)(Chardev *s, const uint8_t *buf, int len);

So, a ChardevClass either provides methods init() and connect(), or
their fusion open().  Correct?

Perhaps documentation becomes simpler if you put init() and connect()
before open().  You could then say open() needs to do the work of both.

Re: [PATCH v3 7/7] chardev: introduce DEFINE_PROP_CHR_NO_CONNECT

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> For further vhost-user-blk backend-transfer migration realization we
> want to give it (vhost-user-blk) a possibility (and responsibility) to
> decide when do connect.
>
> For incoming migration we'll need to postpone connect at least until
> early stage of migrate-incoming command, when we already know all
> migration parameters and can decide, are we going to do incoming
> backend-transfer (and get chardev fd from incoming stream), or we
> finally need to connect.
>
> With this patch, we only provide new macro, to define chardev property,
> later it will be used in vhost-user-blk instead of DEFINE_PROP_CHR.

There is no "later" in this series.

The new macro is called DEFINE_PROP_CHR_NO_CONNECT().

> Then, vhost-user-blk will call qemu_chr_connect() by hand when needed
> (for example through qemu_chr_fe_wait_connected(), which is already
> called in vhost_user_blk_realize_connect()).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Excuse my quick & ignorant questions...

I understand ChardevClass provides either methods init() and connect(),
or method open().

Is a ChardevClass providing open() usable with
DEFINE_PROP_CHR_NO_CONNECT()?

Is a ChardevClass providing init() and connect() usable with
DEFINE_PROP_CHR()?

Could the code do the right thing based on presence of open() vs. init()
and connect() instead of DEFINE_PROP_CHR()
vs. DEFINE_PROP_CHR_NO_CONNECT()?

Re: [PATCH v3 5/7] chardev/char: introduce .init() + .connect() initialization interface

[Vladimir Sementsov-Ogievskiy]

On 15.10.25 09:50, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> We'll need a possibility to postpone connect step to later point in
>> time to implement backend-transfer migration feature for vhost-user-blk
>> in further commits. Let's start with new char interface for backends.
>>
>> .init() takes QAPI parameters and should parse them, called early
>>
>> .connect() should actually establish a connection, and postponed to
>> the point of attaching to frontend. Called at later point, either
>> at time of attaching frontend, either from qemu_chr_wait_connected().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> [...]
> 
>> diff --git a/include/chardev/char.h b/include/chardev/char.h
>> index 429852f8d9..ebadaf3482 100644
>> --- a/include/chardev/char.h
>> +++ b/include/chardev/char.h
>> @@ -63,6 +63,7 @@ struct Chardev {
>>       CharBackend *be;
>>       char *label;
>>       char *filename;
>> +    bool connect_postponed;
>>       int logfd;
>>       int be_open;
>>       /* used to coordinate the chardev-change special-case: */
>> @@ -225,6 +226,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
>>                                   bool permit_mux_mon);
>>   int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
>>   #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
>> +bool qemu_chr_connect(Chardev *chr, Error **errp);
>>   int qemu_chr_wait_connected(Chardev *chr, Error **errp);
>>   
>>   #define TYPE_CHARDEV "chardev"
>> @@ -259,10 +261,28 @@ struct ChardevClass {
>>       /* parse command line options and populate QAPI @backend */
>>       void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
>>   
>> -    /* called after construction, open/starts the backend */
>> +    /*
>> +     * Called after construction, create and open/starts the backend,
> 
> What to do mean by "create and open/starts"?
> 
> Maybe "create and start"?

Yes, sounds good.

> 
>> +     * mutual exclusive with .init. .connect must not be defined when
> 
> mutually
> 
>> +     * .open is defined.
>> +     */
> 
> Suggest to use @name to refer to a member name.  We do that elsewhere,
> and it's easier on the eyes than dots.
> 
>>       void (*open)(Chardev *chr, ChardevBackend *backend,
>>                    bool *be_opened, Error **errp);
>>   
>> +    /*
>> +     * Called after construction, create the backend, mutual exclusive
> 
> mutually
> 
>> +     * with .open, and must be accompanied by .connect.
> 
> Is it okay to destroy after init() without connect()?
> 
> If yes, "must" is misleading.

Hmm. "should" is OK then?

> 
>> +     * Must set chr-filename.
> 
> What's chr-filename?

Type. That should be chr->filename. Or better @chr->filename ?

> 
>> +     */
>> +    bool (*init)(Chardev *chr, ChardevBackend *backend,
>> +                 Error **errp);
>> +
>> +    /*
>> +     * Called after .init(), open/starts the backend, mutual exclusive
> 
> mutually
> 
>> +     * with .open. Must send CHR_EVENT_OPENED.
> 
> Must send CHR_EVENT_OPENED when it succeeds, I guess.

Yes, will add.

> 
>> +     */
>> +    bool (*connect)(Chardev *chr, Error **errp);
>> +
>>       /* write buf to the backend */
>>       int (*chr_write)(Chardev *s, const uint8_t *buf, int len);
> 
> So, a ChardevClass either provides methods init() and connect(), or
> their fusion open().  Correct?

Yes

> 
> Perhaps documentation becomes simpler if you put init() and connect()
> before open().  You could then say open() needs to do the work of both.
> 

Agree, will do.

Thanks for reviewing!

-- 
Best regards,
Vladimir

Re: [PATCH v3 7/7] chardev: introduce DEFINE_PROP_CHR_NO_CONNECT

[Vladimir Sementsov-Ogievskiy]

On 15.10.25 09:56, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> For further vhost-user-blk backend-transfer migration realization we
>> want to give it (vhost-user-blk) a possibility (and responsibility) to
>> decide when do connect.
>>
>> For incoming migration we'll need to postpone connect at least until
>> early stage of migrate-incoming command, when we already know all
>> migration parameters and can decide, are we going to do incoming
>> backend-transfer (and get chardev fd from incoming stream), or we
>> finally need to connect.
>>
>> With this patch, we only provide new macro, to define chardev property,
>> later it will be used in vhost-user-blk instead of DEFINE_PROP_CHR.
> 
> There is no "later" in this series.
> 
> The new macro is called DEFINE_PROP_CHR_NO_CONNECT().
> 
>> Then, vhost-user-blk will call qemu_chr_connect() by hand when needed
>> (for example through qemu_chr_fe_wait_connected(), which is already
>> called in vhost_user_blk_realize_connect()).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Excuse my quick & ignorant questions...
> 
> I understand ChardevClass provides either methods init() and connect(),
> or method open().
> 
> Is a ChardevClass providing open() usable with
> DEFINE_PROP_CHR_NO_CONNECT()?

Good question. It's usable, but it will work like simple DEFINE_PROP_CHR.
I should improve it somehow. Better is to fail than go unexpected way.

> 
> Is a ChardevClass providing init() and connect() usable with
> DEFINE_PROP_CHR()?

Yes, and works correctly.

> 
> Could the code do the right thing based on presence of open() vs. init()
> and connect() instead of DEFINE_PROP_CHR()
> vs. DEFINE_PROP_CHR_NO_CONNECT()?
> 

No, because, the frontend should be prepared to work with new _NO_CONNECT (e.g.,
call _connect() by hand when needed). There are a lot of frontends, which
expect already connected backend, updating them all would be big (and
unnecessary) work.

-- 
Best regards,
Vladimir

Re: [PATCH v3 5/7] chardev/char: introduce .init() + .connect() initialization interface

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 15.10.25 09:50, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> We'll need a possibility to postpone connect step to later point in
>>> time to implement backend-transfer migration feature for vhost-user-blk
>>> in further commits. Let's start with new char interface for backends.
>>>
>>> .init() takes QAPI parameters and should parse them, called early
>>>
>>> .connect() should actually establish a connection, and postponed to
>>> the point of attaching to frontend. Called at later point, either
>>> at time of attaching frontend, either from qemu_chr_wait_connected().
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> [...]
>> 
>>> diff --git a/include/chardev/char.h b/include/chardev/char.h
>>> index 429852f8d9..ebadaf3482 100644
>>> --- a/include/chardev/char.h
>>> +++ b/include/chardev/char.h
>>> @@ -63,6 +63,7 @@ struct Chardev {
>>>      CharBackend *be;
>>>      char *label;
>>>      char *filename;
>>> +    bool connect_postponed;
>>>      int logfd;
>>>      int be_open;
>>>      /* used to coordinate the chardev-change special-case: */
>>> @@ -225,6 +226,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
>>>                                  bool permit_mux_mon);
>>>  int qemu_chr_write(Chardev *s, const uint8_t *buf, int len, bool write_all);
>>>  #define qemu_chr_write_all(s, buf, len) qemu_chr_write(s, buf, len, true)
>>> +bool qemu_chr_connect(Chardev *chr, Error **errp);
>>>  int qemu_chr_wait_connected(Chardev *chr, Error **errp);
>>>
>>>  #define TYPE_CHARDEV "chardev"
>>> @@ -259,10 +261,28 @@ struct ChardevClass {
>>>      /* parse command line options and populate QAPI @backend */
>>>      void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
>>>
>>> -    /* called after construction, open/starts the backend */
>>> +    /*
>>> +     * Called after construction, create and open/starts the backend,
>>
>> What to do mean by "create and open/starts"?
>> Maybe "create and start"?
>
> Yes, sounds good.
>
>>> +     * mutual exclusive with .init. .connect must not be defined when
>>
>> mutually
>> 
>>> +     * .open is defined.
>>> +     */
>>
>> Suggest to use @name to refer to a member name.  We do that elsewhere,
>> and it's easier on the eyes than dots.
>> 
>>>      void (*open)(Chardev *chr, ChardevBackend *backend,
>>>                   bool *be_opened, Error **errp);
>>> +    /*
>>> +     * Called after construction, create the backend, mutual exclusive
>>
>> mutually
>> 
>>> +     * with .open, and must be accompanied by .connect.
>>
>> Is it okay to destroy after init() without connect()?
>> If yes, "must" is misleading.
>
> Hmm. "should" is OK then?

"Should be followed by connect()" would work.

Or something like "The backend still needs to be started with init()."

>>> +     * Must set chr-filename.
>>
>> What's chr-filename?
>
> Type. That should be chr->filename. Or better @chr->filename ?

There is no member @chr.  I figure it's CharBackend member @chr.
Perhaps "the CharBackend's chr->filename". 

[...]

> Thanks for reviewing!

You're welcome!

Re: [PATCH v3 7/7] chardev: introduce DEFINE_PROP_CHR_NO_CONNECT

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 15.10.25 09:56, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> For further vhost-user-blk backend-transfer migration realization we
>>> want to give it (vhost-user-blk) a possibility (and responsibility) to
>>> decide when do connect.
>>>
>>> For incoming migration we'll need to postpone connect at least until
>>> early stage of migrate-incoming command, when we already know all
>>> migration parameters and can decide, are we going to do incoming
>>> backend-transfer (and get chardev fd from incoming stream), or we
>>> finally need to connect.
>>>
>>> With this patch, we only provide new macro, to define chardev property,
>>> later it will be used in vhost-user-blk instead of DEFINE_PROP_CHR.
>>
>> There is no "later" in this series.
>> The new macro is called DEFINE_PROP_CHR_NO_CONNECT().
>> 
>>> Then, vhost-user-blk will call qemu_chr_connect() by hand when needed
>>> (for example through qemu_chr_fe_wait_connected(), which is already
>>> called in vhost_user_blk_realize_connect()).
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>
>> Excuse my quick & ignorant questions...
>>
>> I understand ChardevClass provides either methods init() and connect(),
>> or method open().
>>
>> Is a ChardevClass providing open() usable with
>> DEFINE_PROP_CHR_NO_CONNECT()?
>
> Good question. It's usable, but it will work like simple DEFINE_PROP_CHR.
> I should improve it somehow. Better is to fail than go unexpected way.
>
>> Is a ChardevClass providing init() and connect() usable with
>> DEFINE_PROP_CHR()?
>
> Yes, and works correctly.
>
>> Could the code do the right thing based on presence of open() vs. init()
>> and connect() instead of DEFINE_PROP_CHR()
>> vs. DEFINE_PROP_CHR_NO_CONNECT()?
>> 
>
> No, because, the frontend should be prepared to work with new _NO_CONNECT (e.g.,
> call _connect() by hand when needed). There are a lot of frontends, which
> expect already connected backend, updating them all would be big (and
> unnecessary) work.

QMP command

    {"execute": "qom-list-types", "arguments": {"implements": "chardev"}}

shows me 23 subtypes of "chardev".  Could miss a few not in this build.

Converting them all would be work.  It's not a prohibitive amount of
work, though.  Whether it's worth our while is not for me to judge.

Re: [PATCH v3 7/7] chardev: introduce DEFINE_PROP_CHR_NO_CONNECT

[Vladimir Sementsov-Ogievskiy]

On 15.10.25 10:55, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 15.10.25 09:56, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>
>>>> For further vhost-user-blk backend-transfer migration realization we
>>>> want to give it (vhost-user-blk) a possibility (and responsibility) to
>>>> decide when do connect.
>>>>
>>>> For incoming migration we'll need to postpone connect at least until
>>>> early stage of migrate-incoming command, when we already know all
>>>> migration parameters and can decide, are we going to do incoming
>>>> backend-transfer (and get chardev fd from incoming stream), or we
>>>> finally need to connect.
>>>>
>>>> With this patch, we only provide new macro, to define chardev property,
>>>> later it will be used in vhost-user-blk instead of DEFINE_PROP_CHR.
>>>
>>> There is no "later" in this series.
>>> The new macro is called DEFINE_PROP_CHR_NO_CONNECT().
>>>
>>>> Then, vhost-user-blk will call qemu_chr_connect() by hand when needed
>>>> (for example through qemu_chr_fe_wait_connected(), which is already
>>>> called in vhost_user_blk_realize_connect()).
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>
>>> Excuse my quick & ignorant questions...
>>>
>>> I understand ChardevClass provides either methods init() and connect(),
>>> or method open().
>>>
>>> Is a ChardevClass providing open() usable with
>>> DEFINE_PROP_CHR_NO_CONNECT()?
>>
>> Good question. It's usable, but it will work like simple DEFINE_PROP_CHR.
>> I should improve it somehow. Better is to fail than go unexpected way.
>>
>>> Is a ChardevClass providing init() and connect() usable with
>>> DEFINE_PROP_CHR()?
>>
>> Yes, and works correctly.
>>
>>> Could the code do the right thing based on presence of open() vs. init()
>>> and connect() instead of DEFINE_PROP_CHR()
>>> vs. DEFINE_PROP_CHR_NO_CONNECT()?
>>>
>>
>> No, because, the frontend should be prepared to work with new _NO_CONNECT (e.g.,
>> call _connect() by hand when needed). There are a lot of frontends, which
>> expect already connected backend, updating them all would be big (and
>> unnecessary) work.
> 
> QMP command
> 
>      {"execute": "qom-list-types", "arguments": {"implements": "chardev"}}
> 
> shows me 23 subtypes of "chardev".  Could miss a few not in this build.
> 
> Converting them all would be work.  It's not a prohibitive amount of
> work, though.  Whether it's worth our while is not for me to judge.
> 

I'm not sure we talk about the same thing.

Converting all chardevs to new API .init + .connect is feasible, and I say
in cover letter:

> If the design gets general approval, I'll try to update other
> chardev backends, to avoid supporting two different initialization
> APIs in future.

But converting all chardev users to DEFINE_PROP_CHR_NO_CONNECT is another
thing:

git grep DEFINE_PROP_CHR | wc -l
71

- it would be a huge work, and no benefits. Having a default of "already
connected chardev" seems reasonable. No reason to teach these 70 frontends
to call _connect() by hand.

-

frontend/backend is always misleading terminology for me, especially when
we have more than two components in the system :/


-- 
Best regards,
Vladimir