* [PATCH v2 1/7] chardev/char-socket: simplify reconnect-ms handling
2025-10-13 13:38 [PATCH v2 0/7] chardev: postpone connect Vladimir Sementsov-Ogievskiy
@ 2025-10-13 13:38 ` Vladimir Sementsov-Ogievskiy
2025-10-14 7:31 ` Marc-André Lureau
2025-10-13 13:38 ` [PATCH v2 2/7] chardev/char: split chardev_init_logfd() out of qemu_char_open() Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-13 13:38 UTC (permalink / raw)
To: marcandre.lureau
Cc: pbonzini, berrange, eduardo, qemu-devel, vsementsov, raphael,
armbru, yc-core, d-tatianin
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>
---
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 1/7] chardev/char-socket: simplify reconnect-ms handling
2025-10-13 13:38 ` [PATCH v2 1/7] chardev/char-socket: simplify reconnect-ms handling Vladimir Sementsov-Ogievskiy
@ 2025-10-14 7:31 ` Marc-André Lureau
0 siblings, 0 replies; 21+ messages in thread
From: Marc-André Lureau @ 2025-10-14 7:31 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
On Mon, Oct 13, 2025 at 5:39 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> 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
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 2/7] chardev/char: split chardev_init_logfd() out of qemu_char_open()
2025-10-13 13:38 [PATCH v2 0/7] chardev: postpone connect Vladimir Sementsov-Ogievskiy
2025-10-13 13:38 ` [PATCH v2 1/7] chardev/char-socket: simplify reconnect-ms handling Vladimir Sementsov-Ogievskiy
@ 2025-10-13 13:38 ` Vladimir Sementsov-Ogievskiy
2025-10-14 7:30 ` Marc-André Lureau
2025-10-13 13:38 ` [PATCH v2 3/7] chardev/char: qemu_char_open(): add return value Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-13 13:38 UTC (permalink / raw)
To: marcandre.lureau
Cc: pbonzini, berrange, eduardo, qemu-devel, vsementsov, raphael,
armbru, yc-core, d-tatianin
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>
---
chardev/char.c | 49 +++++++++++++++++++++++++++++++------------------
1 file changed, 31 insertions(+), 18 deletions(-)
diff --git a/chardev/char.c b/chardev/char.c
index a43b7e5481..d5a2533e8e 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,28 @@ void qemu_chr_set_feature(Chardev *chr,
return set_bit(feature, chr->features);
}
+static bool chardev_init_logfd(Chardev *chr, ChardevBackend *backend,
+ Error **errp)
+{
+ 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 +1026,14 @@ static Chardev *chardev_new(const char *id, const char *typename,
chr->label = g_strdup(id);
chr->gcontext = gcontext;
+ if (!chardev_init_logfd(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 +1044,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/7] chardev/char: split chardev_init_logfd() out of qemu_char_open()
2025-10-13 13:38 ` [PATCH v2 2/7] chardev/char: split chardev_init_logfd() out of qemu_char_open() Vladimir Sementsov-Ogievskiy
@ 2025-10-14 7:30 ` Marc-André Lureau
2025-10-14 9:21 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 21+ messages in thread
From: Marc-André Lureau @ 2025-10-14 7:30 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
On Mon, Oct 13, 2025 at 5:41 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> 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 | 49 +++++++++++++++++++++++++++++++------------------
> 1 file changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index a43b7e5481..d5a2533e8e 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 */
maybe keep that comment?
> - 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,28 @@ void qemu_chr_set_feature(Chardev *chr,
> return set_bit(feature, chr->features);
> }
>
> +static bool chardev_init_logfd(Chardev *chr, ChardevBackend *backend,
> + Error **errp)
> +{
> + 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 +1026,14 @@ static Chardev *chardev_new(const char *id, const char *typename,
> chr->label = g_strdup(id);
> chr->gcontext = gcontext;
>
> + if (!chardev_init_logfd(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 +1044,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
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/7] chardev/char: split chardev_init_logfd() out of qemu_char_open()
2025-10-14 7:30 ` Marc-André Lureau
@ 2025-10-14 9:21 ` Vladimir Sementsov-Ogievskiy
2025-10-14 13:55 ` Marc-André Lureau
0 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-14 9:21 UTC (permalink / raw)
To: Marc-André Lureau
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
On 14.10.25 10:30, Marc-André Lureau wrote:
> On Mon, Oct 13, 2025 at 5:41 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> 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 | 49 +++++++++++++++++++++++++++++++------------------
>> 1 file changed, 31 insertions(+), 18 deletions(-)
>>
>> diff --git a/chardev/char.c b/chardev/char.c
>> index a43b7e5481..d5a2533e8e 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 */
>
> maybe keep that comment?
I a bit don't follow it, honestly.. What it mean? That we should
handle members of common structure here?
Not a problem to put it back to chardev_init_logfd().. But maybe, it
then should be renamed to chardev_init_common()
>
>
>> - 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,28 @@ void qemu_chr_set_feature(Chardev *chr,
>> return set_bit(feature, chr->features);
>> }
>>
>> +static bool chardev_init_logfd(Chardev *chr, ChardevBackend *backend,
>> + Error **errp)
>> +{
>> + 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 +1026,14 @@ static Chardev *chardev_new(const char *id, const char *typename,
>> chr->label = g_strdup(id);
>> chr->gcontext = gcontext;
>>
>> + if (!chardev_init_logfd(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 +1044,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
>>
>>
>
>
> --
> Marc-André Lureau
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/7] chardev/char: split chardev_init_logfd() out of qemu_char_open()
2025-10-14 9:21 ` Vladimir Sementsov-Ogievskiy
@ 2025-10-14 13:55 ` Marc-André Lureau
0 siblings, 0 replies; 21+ messages in thread
From: Marc-André Lureau @ 2025-10-14 13:55 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
Hi
On Tue, Oct 14, 2025 at 1:21 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 14.10.25 10:30, Marc-André Lureau wrote:
> > On Mon, Oct 13, 2025 at 5:41 PM Vladimir Sementsov-Ogievskiy
> > <vsementsov@yandex-team.ru> wrote:
> >>
> >> 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 | 49 +++++++++++++++++++++++++++++++------------------
> >> 1 file changed, 31 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/chardev/char.c b/chardev/char.c
> >> index a43b7e5481..d5a2533e8e 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 */
> >
> > maybe keep that comment?
>
> I a bit don't follow it, honestly.. What it mean? That we should
> handle members of common structure here?
The ChardevBackend type is a union, and all the members inherit from
ChardevCommon. Ideally, we wouldn't cast this way, but that would
require some changes in code generator...
>
> Not a problem to put it back to chardev_init_logfd().. But maybe, it
> then should be renamed to chardev_init_common()
>
> >
> >
> >> - 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,28 @@ void qemu_chr_set_feature(Chardev *chr,
> >> return set_bit(feature, chr->features);
> >> }
> >>
> >> +static bool chardev_init_logfd(Chardev *chr, ChardevBackend *backend,
> >> + Error **errp)
> >> +{
> >> + 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 +1026,14 @@ static Chardev *chardev_new(const char *id, const char *typename,
> >> chr->label = g_strdup(id);
> >> chr->gcontext = gcontext;
> >>
> >> + if (!chardev_init_logfd(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 +1044,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
> >>
> >>
> >
> >
> > --
> > Marc-André Lureau
>
>
> --
> Best regards,
> Vladimir
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/7] chardev/char: qemu_char_open(): add return value
2025-10-13 13:38 [PATCH v2 0/7] chardev: postpone connect Vladimir Sementsov-Ogievskiy
2025-10-13 13:38 ` [PATCH v2 1/7] chardev/char-socket: simplify reconnect-ms handling Vladimir Sementsov-Ogievskiy
2025-10-13 13:38 ` [PATCH v2 2/7] chardev/char: split chardev_init_logfd() out of qemu_char_open() Vladimir Sementsov-Ogievskiy
@ 2025-10-13 13:38 ` Vladimir Sementsov-Ogievskiy
2025-10-14 7:10 ` Marc-André Lureau
2025-10-13 13:38 ` [PATCH v2 4/7] chardev/char: move filename and be_opened handling to qemu_char_open() Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-13 13:38 UTC (permalink / raw)
To: marcandre.lureau
Cc: pbonzini, berrange, eduardo, qemu-devel, vsementsov, raphael,
armbru, yc-core, d-tatianin
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 | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/chardev/char.c b/chardev/char.c
index d5a2533e8e..64ec60c0f2 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)
@@ -1030,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, &local_err)) {
goto fail;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/7] chardev/char: qemu_char_open(): add return value
2025-10-13 13:38 ` [PATCH v2 3/7] chardev/char: qemu_char_open(): add return value Vladimir Sementsov-Ogievskiy
@ 2025-10-14 7:10 ` Marc-André Lureau
2025-10-14 9:13 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 21+ messages in thread
From: Marc-André Lureau @ 2025-10-14 7:10 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
Hi
On Mon, Oct 13, 2025 at 5:40 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> 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 | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index d5a2533e8e..64ec60c0f2 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)
> @@ -1030,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, &local_err)) {
You meant to pass errp instead, since you dropped error_propagate ?
> goto fail;
> }
>
> --
> 2.48.1
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/7] chardev/char: qemu_char_open(): add return value
2025-10-14 7:10 ` Marc-André Lureau
@ 2025-10-14 9:13 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-14 9:13 UTC (permalink / raw)
To: Marc-André Lureau
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
On 14.10.25 10:10, Marc-André Lureau wrote:
> Hi
>
> On Mon, Oct 13, 2025 at 5:40 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> 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 | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/chardev/char.c b/chardev/char.c
>> index d5a2533e8e..64ec60c0f2 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)
>> @@ -1030,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, &local_err)) {
>
> You meant to pass errp instead, since you dropped error_propagate ?
>
Oops, right
>> goto fail;
>> }
>>
>> --
>> 2.48.1
>>
>>
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 4/7] chardev/char: move filename and be_opened handling to qemu_char_open()
2025-10-13 13:38 [PATCH v2 0/7] chardev: postpone connect Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2025-10-13 13:38 ` [PATCH v2 3/7] chardev/char: qemu_char_open(): add return value Vladimir Sementsov-Ogievskiy
@ 2025-10-13 13:38 ` Vladimir Sementsov-Ogievskiy
2025-10-14 7:31 ` Marc-André Lureau
2025-10-13 13:38 ` [PATCH v2 5/7] chardev/char: introduce .init() + .connect() initialization interface Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-13 13:38 UTC (permalink / raw)
To: marcandre.lureau
Cc: pbonzini, berrange, eduardo, qemu-devel, vsementsov, raphael,
armbru, yc-core, d-tatianin
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>
---
chardev/char.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/chardev/char.c b/chardev/char.c
index 64ec60c0f2..6498d53daa 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;
Error *local_err = 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, &local_err)) {
+ if (!qemu_char_open(chr, backend, typename + 8, &local_err)) {
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 4/7] chardev/char: move filename and be_opened handling to qemu_char_open()
2025-10-13 13:38 ` [PATCH v2 4/7] chardev/char: move filename and be_opened handling to qemu_char_open() Vladimir Sementsov-Ogievskiy
@ 2025-10-14 7:31 ` Marc-André Lureau
0 siblings, 0 replies; 21+ messages in thread
From: Marc-André Lureau @ 2025-10-14 7:31 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
On Mon, Oct 13, 2025 at 5:40 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> 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 64ec60c0f2..6498d53daa 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;
> Error *local_err = 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, &local_err)) {
> + if (!qemu_char_open(chr, backend, typename + 8, &local_err)) {
> 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
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 5/7] chardev/char: introduce .init() + .connect() initialization interface
2025-10-13 13:38 [PATCH v2 0/7] chardev: postpone connect Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2025-10-13 13:38 ` [PATCH v2 4/7] chardev/char: move filename and be_opened handling to qemu_char_open() Vladimir Sementsov-Ogievskiy
@ 2025-10-13 13:38 ` Vladimir Sementsov-Ogievskiy
2025-10-14 7:30 ` Marc-André Lureau
2025-10-13 13:38 ` [PATCH v2 6/7] chardev/char-socket: move to .init + .connect api Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-13 13:38 UTC (permalink / raw)
To: marcandre.lureau
Cc: pbonzini, berrange, eduardo, qemu-devel, vsementsov, raphael,
armbru, yc-core, d-tatianin
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>
---
chardev/char-fe.c | 4 ++++
chardev/char.c | 40 +++++++++++++++++++++++++++++++++++++---
include/chardev/char.h | 22 +++++++++++++++++++++-
3 files changed, 62 insertions(+), 4 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 6498d53daa..01ffe37acf 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);
}
@@ -1029,7 +1049,7 @@ static Chardev *chardev_new(const char *id, const char *typename,
{
Object *obj;
Chardev *chr = NULL;
- Error *local_err = NULL;
+ ChardevClass *cc;
assert(g_str_has_prefix(typename, "chardev-"));
assert(id);
@@ -1044,8 +1064,22 @@ static Chardev *chardev_new(const char *id, const char *typename,
goto fail;
}
- if (!qemu_char_open(chr, backend, typename + 8, &local_err)) {
- 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 5/7] chardev/char: introduce .init() + .connect() initialization interface
2025-10-13 13:38 ` [PATCH v2 5/7] chardev/char: introduce .init() + .connect() initialization interface Vladimir Sementsov-Ogievskiy
@ 2025-10-14 7:30 ` Marc-André Lureau
0 siblings, 0 replies; 21+ messages in thread
From: Marc-André Lureau @ 2025-10-14 7:30 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
On Mon, Oct 13, 2025 at 5:40 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> 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 | 40 +++++++++++++++++++++++++++++++++++++---
> include/chardev/char.h | 22 +++++++++++++++++++++-
> 3 files changed, 62 insertions(+), 4 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 6498d53daa..01ffe37acf 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);
> }
> @@ -1029,7 +1049,7 @@ static Chardev *chardev_new(const char *id, const char *typename,
> {
> Object *obj;
> Chardev *chr = NULL;
> - Error *local_err = NULL;
> + ChardevClass *cc;
>
> assert(g_str_has_prefix(typename, "chardev-"));
> assert(id);
> @@ -1044,8 +1064,22 @@ static Chardev *chardev_new(const char *id, const char *typename,
> goto fail;
> }
>
> - if (!qemu_char_open(chr, backend, typename + 8, &local_err)) {
> - 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
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 6/7] chardev/char-socket: move to .init + .connect api
2025-10-13 13:38 [PATCH v2 0/7] chardev: postpone connect Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2025-10-13 13:38 ` [PATCH v2 5/7] chardev/char: introduce .init() + .connect() initialization interface Vladimir Sementsov-Ogievskiy
@ 2025-10-13 13:38 ` Vladimir Sementsov-Ogievskiy
2025-10-14 7:31 ` Marc-André Lureau
2025-10-13 13:38 ` [PATCH v2 7/7] char: vhost-user-blk call connect by hand Vladimir Sementsov-Ogievskiy
2025-10-14 13:40 ` [PATCH v2 0/7] chardev: postpone connect Daniel P. Berrangé
7 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-13 13:38 UTC (permalink / raw)
To: marcandre.lureau
Cc: pbonzini, berrange, eduardo, qemu-devel, vsementsov, raphael,
armbru, yc-core, d-tatianin
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>
---
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 01ffe37acf..4d151f537c 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1221,12 +1221,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 6/7] chardev/char-socket: move to .init + .connect api
2025-10-13 13:38 ` [PATCH v2 6/7] chardev/char-socket: move to .init + .connect api Vladimir Sementsov-Ogievskiy
@ 2025-10-14 7:31 ` Marc-André Lureau
0 siblings, 0 replies; 21+ messages in thread
From: Marc-André Lureau @ 2025-10-14 7:31 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
On Mon, Oct 13, 2025 at 5:40 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> 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 01ffe37acf..4d151f537c 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -1221,12 +1221,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
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 7/7] char: vhost-user-blk call connect by hand
2025-10-13 13:38 [PATCH v2 0/7] chardev: postpone connect Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2025-10-13 13:38 ` [PATCH v2 6/7] chardev/char-socket: move to .init + .connect api Vladimir Sementsov-Ogievskiy
@ 2025-10-13 13:38 ` Vladimir Sementsov-Ogievskiy
2025-10-14 7:30 ` Marc-André Lureau
2025-10-14 13:40 ` [PATCH v2 0/7] chardev: postpone connect Daniel P. Berrangé
7 siblings, 1 reply; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-13 13:38 UTC (permalink / raw)
To: marcandre.lureau
Cc: pbonzini, berrange, eduardo, qemu-devel, vsementsov, raphael,
armbru, yc-core, d-tatianin
Give vhost-user-blk a possibility (and responsibility) to decide when
do connect. It will help us to realize vhost-user-blk backend-transfer
migration feature:
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 don't modify vhost-user-blk.c: it already
do call qemu_chr_fe_wait_connected(), which in turn calls
qemu_chr_connect().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
chardev/char-fe.c | 21 +++++++++++++++++----
hw/core/qdev-properties-system.c | 2 +-
include/chardev/char-fe.h | 2 ++
3 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 973fed5bea..9e2c658cb9 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -189,12 +189,20 @@ 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, ObjectClass *oc,
+ Error **errp)
{
unsigned int tag = 0;
-
- if (!qemu_chr_connect(s, errp)) {
- return false;
+ const char *driver = oc ? object_class_get_name(oc) : NULL;
+
+ /*
+ * vhost-user-blk wants call qemu_chr_connect by hand,
+ * to support backend-transfer migration feature.
+ */
+ if (!driver || !g_str_has_prefix(driver, "vhost-user-blk")) {
+ if (!qemu_chr_connect(s, errp)) {
+ return false;
+ }
}
if (s) {
@@ -218,6 +226,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, NULL, 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..35eed17d4d 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -297,7 +297,7 @@ 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, obj->class, errp)) {
error_prepend(errp, "Property '%s.%s' can't take value '%s': ",
object_get_typename(obj), name, str);
}
diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index 8ef05b3dd0..f1c05cc5ed 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -34,6 +34,8 @@ struct CharBackend {
* 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, ObjectClass *oc,
+ Error **errp);
/**
* qemu_chr_fe_deinit:
--
2.48.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 7/7] char: vhost-user-blk call connect by hand
2025-10-13 13:38 ` [PATCH v2 7/7] char: vhost-user-blk call connect by hand Vladimir Sementsov-Ogievskiy
@ 2025-10-14 7:30 ` Marc-André Lureau
2025-10-14 9:18 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 21+ messages in thread
From: Marc-André Lureau @ 2025-10-14 7:30 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
Hi
On Mon, Oct 13, 2025 at 5:39 PM Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Give vhost-user-blk a possibility (and responsibility) to decide when
> do connect. It will help us to realize vhost-user-blk backend-transfer
> migration feature:
>
> 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 don't modify vhost-user-blk.c: it already
> do call qemu_chr_fe_wait_connected(), which in turn calls
> qemu_chr_connect().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> chardev/char-fe.c | 21 +++++++++++++++++----
> hw/core/qdev-properties-system.c | 2 +-
> include/chardev/char-fe.h | 2 ++
> 3 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index 973fed5bea..9e2c658cb9 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -189,12 +189,20 @@ 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, ObjectClass *oc,
> + Error **errp)
> {
> unsigned int tag = 0;
> -
> - if (!qemu_chr_connect(s, errp)) {
> - return false;
> + const char *driver = oc ? object_class_get_name(oc) : NULL;
> +
> + /*
> + * vhost-user-blk wants call qemu_chr_connect by hand,
> + * to support backend-transfer migration feature.
> + */
> + if (!driver || !g_str_has_prefix(driver, "vhost-user-blk")) {
> + if (!qemu_chr_connect(s, errp)) {
> + return false;
> + }
Why not pass a bool "connect" instead?
DEFINE_PROP_CHR() would use "true".
+ Introduce DEFINE_PROP_CHR_NO_CONNECT() that would use "false".
and wire it with a new PropertyInfo and modified callbacks.
> }
>
> if (s) {
> @@ -218,6 +226,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, NULL, 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..35eed17d4d 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -297,7 +297,7 @@ 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, obj->class, errp)) {
> error_prepend(errp, "Property '%s.%s' can't take value '%s': ",
> object_get_typename(obj), name, str);
> }
> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> index 8ef05b3dd0..f1c05cc5ed 100644
> --- a/include/chardev/char-fe.h
> +++ b/include/chardev/char-fe.h
> @@ -34,6 +34,8 @@ struct CharBackend {
> * 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, ObjectClass *oc,
> + Error **errp);
>
> /**
> * qemu_chr_fe_deinit:
> --
> 2.48.1
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 7/7] char: vhost-user-blk call connect by hand
2025-10-14 7:30 ` Marc-André Lureau
@ 2025-10-14 9:18 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-14 9:18 UTC (permalink / raw)
To: Marc-André Lureau
Cc: pbonzini, berrange, eduardo, qemu-devel, raphael, armbru, yc-core,
d-tatianin
On 14.10.25 10:30, Marc-André Lureau wrote:
> Hi
>
> On Mon, Oct 13, 2025 at 5:39 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> Give vhost-user-blk a possibility (and responsibility) to decide when
>> do connect. It will help us to realize vhost-user-blk backend-transfer
>> migration feature:
>>
>> 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 don't modify vhost-user-blk.c: it already
>> do call qemu_chr_fe_wait_connected(), which in turn calls
>> qemu_chr_connect().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> chardev/char-fe.c | 21 +++++++++++++++++----
>> hw/core/qdev-properties-system.c | 2 +-
>> include/chardev/char-fe.h | 2 ++
>> 3 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
>> index 973fed5bea..9e2c658cb9 100644
>> --- a/chardev/char-fe.c
>> +++ b/chardev/char-fe.c
>> @@ -189,12 +189,20 @@ 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, ObjectClass *oc,
>> + Error **errp)
>> {
>> unsigned int tag = 0;
>> -
>> - if (!qemu_chr_connect(s, errp)) {
>> - return false;
>> + const char *driver = oc ? object_class_get_name(oc) : NULL;
>> +
>> + /*
>> + * vhost-user-blk wants call qemu_chr_connect by hand,
>> + * to support backend-transfer migration feature.
>> + */
>> + if (!driver || !g_str_has_prefix(driver, "vhost-user-blk")) {
>> + if (!qemu_chr_connect(s, errp)) {
>> + return false;
>> + }
>
> Why not pass a bool "connect" instead?
>
> DEFINE_PROP_CHR() would use "true".
>
> + Introduce DEFINE_PROP_CHR_NO_CONNECT() that would use "false".
>
> and wire it with a new PropertyInfo and modified callbacks.
>
Will try, thanks for idea. I don't like this checking by name too, just design it after
.check_peer_type in net/ code.
>> }
>>
>> if (s) {
>> @@ -218,6 +226,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, NULL, 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..35eed17d4d 100644
>> --- a/hw/core/qdev-properties-system.c
>> +++ b/hw/core/qdev-properties-system.c
>> @@ -297,7 +297,7 @@ 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, obj->class, errp)) {
>> error_prepend(errp, "Property '%s.%s' can't take value '%s': ",
>> object_get_typename(obj), name, str);
>> }
>> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
>> index 8ef05b3dd0..f1c05cc5ed 100644
>> --- a/include/chardev/char-fe.h
>> +++ b/include/chardev/char-fe.h
>> @@ -34,6 +34,8 @@ struct CharBackend {
>> * 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, ObjectClass *oc,
>> + Error **errp);
>>
>> /**
>> * qemu_chr_fe_deinit:
>> --
>> 2.48.1
>>
>>
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 0/7] chardev: postpone connect
2025-10-13 13:38 [PATCH v2 0/7] chardev: postpone connect Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2025-10-13 13:38 ` [PATCH v2 7/7] char: vhost-user-blk call connect by hand Vladimir Sementsov-Ogievskiy
@ 2025-10-14 13:40 ` Daniel P. Berrangé
2025-10-14 14:43 ` Vladimir Sementsov-Ogievskiy
7 siblings, 1 reply; 21+ messages in thread
From: Daniel P. Berrangé @ 2025-10-14 13:40 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: marcandre.lureau, pbonzini, eduardo, qemu-devel, raphael, armbru,
yc-core, d-tatianin
On Mon, Oct 13, 2025 at 04:38:29PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> That's a preparation for backend-transfer migration of
> vhost-user-blk. For such migration we are going to transfer
> 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 check:
>
> - if it's vhost-user-blk, do nothing, let vhost-user-blk decide
> when to do connect()
> - otherwise, do connect() at this point
I'm finding it quite unpleasant that we've created a new set of
callbacks just for the socket backend, and not the other chardev
backends.
Conceptually it feels like the problem of transferring in pre-
opened FDs from a previous QEMU should be conceptually relevant
to all the backend types. If it is, then I very much want us to
convert all the backends instead of leaving a pile of technical
debt for someone else in the future.
This series also doesn't illustrate usage of the new model with
pre-opened FDs, so I'm finding it hard to validate whether
this design is effective or not.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 0/7] chardev: postpone connect
2025-10-14 13:40 ` [PATCH v2 0/7] chardev: postpone connect Daniel P. Berrangé
@ 2025-10-14 14:43 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 21+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2025-10-14 14:43 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: marcandre.lureau, pbonzini, eduardo, qemu-devel, raphael, armbru,
yc-core, d-tatianin
On 14.10.25 16:40, Daniel P. Berrangé wrote:
> On Mon, Oct 13, 2025 at 04:38:29PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> That's a preparation for backend-transfer migration of
>> vhost-user-blk. For such migration we are going to transfer
>> 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 check:
>>
>> - if it's vhost-user-blk, do nothing, let vhost-user-blk decide
>> when to do connect()
>> - otherwise, do connect() at this point
>
> I'm finding it quite unpleasant that we've created a new set of
> callbacks just for the socket backend, and not the other chardev
> backends.
>
> Conceptually it feels like the problem of transferring in pre-
> opened FDs from a previous QEMU should be conceptually relevant
> to all the backend types. If it is, then I very much want us to
> convert all the backends instead of leaving a pile of technical
> debt for someone else in the future.
Agree. If the design is approved, I'll try to update all other
chardevs as well.
>
> This series also doesn't illustrate usage of the new model with
> pre-opened FDs, so I'm finding it hard to validate whether
> this design is effective or not.
>
Understand, of course.
[PATCH v2 00/?] vhost-user-blk: live-backend local migration
is coming soon, so the whole picture will be seen.
Still, I'll first resend this one (chardev: postpone connect) and
[PATCH v2 00/23] vhost refactoring and fixes
to base future v2 vhost-user-blk: live-backend local migration
on clear base.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 21+ messages in thread