[PATCH] chardev: introduce 'reconnect-ms' and deprecate 'reconnect'

Daniil Tatianin posted 1 patch 2 months, 3 weeks ago
There is a newer version of this series
chardev/char-socket.c         | 33 ++++++++++++++++++++++++---------
chardev/char.c                |  3 +++
include/chardev/char-socket.h |  2 +-
qapi/char.json                | 17 +++++++++++++++--
4 files changed, 43 insertions(+), 12 deletions(-)
[PATCH] chardev: introduce 'reconnect-ms' and deprecate 'reconnect'
Posted by Daniil Tatianin 2 months, 3 weeks ago
The 'reconnect' option only allows to specify the time in seconds,
which is way too long for certain workflows.

We have a lightweight disk backend server, which takes about 20ms to
live update, but due to this limitation in QEMU, previously the guest
disk controller would hang for one second because it would take this
long for QEMU to reinitialize the socket connection.

Introduce a new option called 'reconnect-ms', which is the same as
'reconnect', except the value is treated as milliseconds. These are
mutually exclusive and specifying both results in an error.

'reconnect' is also deprecated by this commit to make it possible to
remove it in the future as to not keep two options that control the
same thing.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
 chardev/char-socket.c         | 33 ++++++++++++++++++++++++---------
 chardev/char.c                |  3 +++
 include/chardev/char-socket.h |  2 +-
 qapi/char.json                | 17 +++++++++++++++--
 4 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1ca9441b1b..c24331ac23 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -74,7 +74,7 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
     assert(!s->reconnect_timer);
     name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
     s->reconnect_timer = qemu_chr_timeout_add_ms(chr,
-                                                 s->reconnect_time * 1000,
+                                                 s->reconnect_time_ms,
                                                  socket_reconnect_timeout,
                                                  chr);
     g_source_set_name(s->reconnect_timer, name);
@@ -481,7 +481,7 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
     if (emit_close) {
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
     }
-    if (s->reconnect_time && !s->reconnect_timer) {
+    if (s->reconnect_time_ms && !s->reconnect_timer) {
         qemu_chr_socket_restart_timer(chr);
     }
 }
@@ -1080,9 +1080,9 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
         } else {
             Error *err = NULL;
             if (tcp_chr_connect_client_sync(chr, &err) < 0) {
-                if (s->reconnect_time) {
+                if (s->reconnect_time_ms) {
                     error_free(err);
-                    g_usleep(s->reconnect_time * 1000ULL * 1000ULL);
+                    g_usleep(s->reconnect_time_ms * 1000ULL);
                 } else {
                     error_propagate(errp, err);
                     return -1;
@@ -1267,13 +1267,13 @@ skip_listen:
 
 
 static int qmp_chardev_open_socket_client(Chardev *chr,
-                                          int64_t reconnect,
+                                          int64_t reconnect_ms,
                                           Error **errp)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
-    if (reconnect > 0) {
-        s->reconnect_time = reconnect;
+    if (reconnect_ms > 0) {
+        s->reconnect_time_ms = reconnect_ms;
         tcp_chr_connect_client_async(chr);
         return 0;
     } else {
@@ -1371,7 +1371,7 @@ 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   = sock->has_reconnect ? sock->reconnect : 0;
+    int64_t reconnect_ms = 0;
     SocketAddress *addr;
 
     s->is_listen = is_listen;
@@ -1443,7 +1443,13 @@ static void qmp_chardev_open_socket(Chardev *chr,
             return;
         }
     } else {
-        if (qmp_chardev_open_socket_client(chr, reconnect, errp) < 0) {
+        if (sock->has_reconnect) {
+            reconnect_ms = sock->reconnect * 1000ULL;
+        } else if (sock->has_reconnect_ms) {
+            reconnect_ms = sock->reconnect_ms;
+        }
+
+        if (qmp_chardev_open_socket_client(chr, reconnect_ms, errp) < 0) {
             return;
         }
     }
@@ -1509,6 +1515,15 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     sock->wait = qemu_opt_get_bool(opts, "wait", true);
     sock->has_reconnect = qemu_opt_find(opts, "reconnect");
     sock->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
+    sock->has_reconnect_ms = qemu_opt_find(opts, "reconnect-ms");
+    sock->reconnect_ms = qemu_opt_get_number(opts, "reconnect-ms", 0);
+
+    if (sock->has_reconnect_ms && sock->has_reconnect) {
+        error_setg(errp,
+            "'reconnect' and 'reconnect-ms' are mutually exclusive");
+        return;
+    }
+
     sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
     sock->tls_authz = g_strdup(qemu_opt_get(opts, "tls-authz"));
 
diff --git a/chardev/char.c b/chardev/char.c
index ba847b6e9e..35623c78a3 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -888,6 +888,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "reconnect",
             .type = QEMU_OPT_NUMBER,
+        },{
+            .name = "reconnect-ms",
+            .type = QEMU_OPT_NUMBER,
         },{
             .name = "telnet",
             .type = QEMU_OPT_BOOL,
diff --git a/include/chardev/char-socket.h b/include/chardev/char-socket.h
index 0708ca6fa9..d6d13ad37f 100644
--- a/include/chardev/char-socket.h
+++ b/include/chardev/char-socket.h
@@ -74,7 +74,7 @@ struct SocketChardev {
     bool is_websock;
 
     GSource *reconnect_timer;
-    int64_t reconnect_time;
+    int64_t reconnect_time_ms;
     bool connect_err_reported;
 
     QIOTask *connect_task;
diff --git a/qapi/char.json b/qapi/char.json
index ef58445cee..7f117438c6 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -273,7 +273,19 @@
 #
 # @reconnect: For a client socket, if a socket is disconnected, then
 #     attempt a reconnect after the given number of seconds.  Setting
-#     this to zero disables this function.  (default: 0) (Since: 2.2)
+#     this to zero disables this function.  The use of this member is
+#     deprecated, use @reconnect-ms instead. (default: 0) (Since: 2.2)
+#
+# @reconnect-ms: For a client socket, if a socket is disconnected,
+#     then attempt a reconnect after the given number of milliseconds.
+#     Setting this to zero disables this function.  This member is
+#     mutually exclusive with @reconnect.
+#     (default: 0) (Since: 9.2)
+#
+# Features:
+#
+# @deprecated: Member @reconnect is deprecated.  Use @reconnect-ms
+#     instead.
 #
 # Since: 1.4
 ##
@@ -287,7 +299,8 @@
             '*telnet': 'bool',
             '*tn3270': 'bool',
             '*websocket': 'bool',
-            '*reconnect': 'int' },
+            '*reconnect': { 'type': 'int', 'features': [ 'deprecated' ] },
+            '*reconnect-ms': 'int' },
   'base': 'ChardevCommon' }
 
 ##
-- 
2.34.1
Re: [PATCH] chardev: introduce 'reconnect-ms' and deprecate 'reconnect'
Posted by Paolo Bonzini 2 months, 2 weeks ago
On 9/4/24 07:19, Daniil Tatianin wrote:
> The 'reconnect' option only allows to specify the time in seconds,
> which is way too long for certain workflows.
> 
> We have a lightweight disk backend server, which takes about 20ms to
> live update, but due to this limitation in QEMU, previously the guest
> disk controller would hang for one second because it would take this
> long for QEMU to reinitialize the socket connection.
> 
> Introduce a new option called 'reconnect-ms', which is the same as
> 'reconnect', except the value is treated as milliseconds. These are
> mutually exclusive and specifying both results in an error.
> 
> 'reconnect' is also deprecated by this commit to make it possible to
> remove it in the future as to not keep two options that control the
> same thing.

Please add it also to docs/about/deprecated.rst

Otherwise looks good, thanks!

Paolo

> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ACKed-by: Peter Krempa <pkrempa@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   chardev/char-socket.c         | 33 ++++++++++++++++++++++++---------
>   chardev/char.c                |  3 +++
>   include/chardev/char-socket.h |  2 +-
>   qapi/char.json                | 17 +++++++++++++++--
>   4 files changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 1ca9441b1b..c24331ac23 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -74,7 +74,7 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
>       assert(!s->reconnect_timer);
>       name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
>       s->reconnect_timer = qemu_chr_timeout_add_ms(chr,
> -                                                 s->reconnect_time * 1000,
> +                                                 s->reconnect_time_ms,
>                                                    socket_reconnect_timeout,
>                                                    chr);
>       g_source_set_name(s->reconnect_timer, name);
> @@ -481,7 +481,7 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
>       if (emit_close) {
>           qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>       }
> -    if (s->reconnect_time && !s->reconnect_timer) {
> +    if (s->reconnect_time_ms && !s->reconnect_timer) {
>           qemu_chr_socket_restart_timer(chr);
>       }
>   }
> @@ -1080,9 +1080,9 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
>           } else {
>               Error *err = NULL;
>               if (tcp_chr_connect_client_sync(chr, &err) < 0) {
> -                if (s->reconnect_time) {
> +                if (s->reconnect_time_ms) {
>                       error_free(err);
> -                    g_usleep(s->reconnect_time * 1000ULL * 1000ULL);
> +                    g_usleep(s->reconnect_time_ms * 1000ULL);
>                   } else {
>                       error_propagate(errp, err);
>                       return -1;
> @@ -1267,13 +1267,13 @@ skip_listen:
>   
>   
>   static int qmp_chardev_open_socket_client(Chardev *chr,
> -                                          int64_t reconnect,
> +                                          int64_t reconnect_ms,
>                                             Error **errp)
>   {
>       SocketChardev *s = SOCKET_CHARDEV(chr);
>   
> -    if (reconnect > 0) {
> -        s->reconnect_time = reconnect;
> +    if (reconnect_ms > 0) {
> +        s->reconnect_time_ms = reconnect_ms;
>           tcp_chr_connect_client_async(chr);
>           return 0;
>       } else {
> @@ -1371,7 +1371,7 @@ 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   = sock->has_reconnect ? sock->reconnect : 0;
> +    int64_t reconnect_ms = 0;
>       SocketAddress *addr;
>   
>       s->is_listen = is_listen;
> @@ -1443,7 +1443,13 @@ static void qmp_chardev_open_socket(Chardev *chr,
>               return;
>           }
>       } else {
> -        if (qmp_chardev_open_socket_client(chr, reconnect, errp) < 0) {
> +        if (sock->has_reconnect) {
> +            reconnect_ms = sock->reconnect * 1000ULL;
> +        } else if (sock->has_reconnect_ms) {
> +            reconnect_ms = sock->reconnect_ms;
> +        }
> +
> +        if (qmp_chardev_open_socket_client(chr, reconnect_ms, errp) < 0) {
>               return;
>           }
>       }
> @@ -1509,6 +1515,15 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>       sock->wait = qemu_opt_get_bool(opts, "wait", true);
>       sock->has_reconnect = qemu_opt_find(opts, "reconnect");
>       sock->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
> +    sock->has_reconnect_ms = qemu_opt_find(opts, "reconnect-ms");
> +    sock->reconnect_ms = qemu_opt_get_number(opts, "reconnect-ms", 0);
> +
> +    if (sock->has_reconnect_ms && sock->has_reconnect) {
> +        error_setg(errp,
> +            "'reconnect' and 'reconnect-ms' are mutually exclusive");
> +        return;
> +    }
> +
>       sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
>       sock->tls_authz = g_strdup(qemu_opt_get(opts, "tls-authz"));
>   
> diff --git a/chardev/char.c b/chardev/char.c
> index ba847b6e9e..35623c78a3 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -888,6 +888,9 @@ QemuOptsList qemu_chardev_opts = {
>           },{
>               .name = "reconnect",
>               .type = QEMU_OPT_NUMBER,
> +        },{
> +            .name = "reconnect-ms",
> +            .type = QEMU_OPT_NUMBER,
>           },{
>               .name = "telnet",
>               .type = QEMU_OPT_BOOL,
> diff --git a/include/chardev/char-socket.h b/include/chardev/char-socket.h
> index 0708ca6fa9..d6d13ad37f 100644
> --- a/include/chardev/char-socket.h
> +++ b/include/chardev/char-socket.h
> @@ -74,7 +74,7 @@ struct SocketChardev {
>       bool is_websock;
>   
>       GSource *reconnect_timer;
> -    int64_t reconnect_time;
> +    int64_t reconnect_time_ms;
>       bool connect_err_reported;
>   
>       QIOTask *connect_task;
> diff --git a/qapi/char.json b/qapi/char.json
> index ef58445cee..7f117438c6 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -273,7 +273,19 @@
>   #
>   # @reconnect: For a client socket, if a socket is disconnected, then
>   #     attempt a reconnect after the given number of seconds.  Setting
> -#     this to zero disables this function.  (default: 0) (Since: 2.2)
> +#     this to zero disables this function.  The use of this member is
> +#     deprecated, use @reconnect-ms instead. (default: 0) (Since: 2.2)
> +#
> +# @reconnect-ms: For a client socket, if a socket is disconnected,
> +#     then attempt a reconnect after the given number of milliseconds.
> +#     Setting this to zero disables this function.  This member is
> +#     mutually exclusive with @reconnect.
> +#     (default: 0) (Since: 9.2)
> +#
> +# Features:
> +#
> +# @deprecated: Member @reconnect is deprecated.  Use @reconnect-ms
> +#     instead.
>   #
>   # Since: 1.4
>   ##
> @@ -287,7 +299,8 @@
>               '*telnet': 'bool',
>               '*tn3270': 'bool',
>               '*websocket': 'bool',
> -            '*reconnect': 'int' },
> +            '*reconnect': { 'type': 'int', 'features': [ 'deprecated' ] },
> +            '*reconnect-ms': 'int' },
>     'base': 'ChardevCommon' }
>   
>   ##
Re: [PATCH] chardev: introduce 'reconnect-ms' and deprecate 'reconnect'
Posted by Peter Krempa 2 months, 2 weeks ago
On Wed, Sep 04, 2024 at 08:19:13 +0300, Daniil Tatianin wrote:
> The 'reconnect' option only allows to specify the time in seconds,
> which is way too long for certain workflows.
> 
> We have a lightweight disk backend server, which takes about 20ms to
> live update, but due to this limitation in QEMU, previously the guest
> disk controller would hang for one second because it would take this
> long for QEMU to reinitialize the socket connection.
> 
> Introduce a new option called 'reconnect-ms', which is the same as
> 'reconnect', except the value is treated as milliseconds. These are
> mutually exclusive and specifying both results in an error.
> 
> 'reconnect' is also deprecated by this commit to make it possible to
> remove it in the future as to not keep two options that control the
> same thing.
> 
> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
> ---

[...]

> diff --git a/qapi/char.json b/qapi/char.json
> index ef58445cee..7f117438c6 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -273,7 +273,19 @@
>  #
>  # @reconnect: For a client socket, if a socket is disconnected, then
>  #     attempt a reconnect after the given number of seconds.  Setting
> -#     this to zero disables this function.  (default: 0) (Since: 2.2)
> +#     this to zero disables this function.  The use of this member is
> +#     deprecated, use @reconnect-ms instead. (default: 0) (Since: 2.2)
> +#
> +# @reconnect-ms: For a client socket, if a socket is disconnected,
> +#     then attempt a reconnect after the given number of milliseconds.
> +#     Setting this to zero disables this function.  This member is
> +#     mutually exclusive with @reconnect.
> +#     (default: 0) (Since: 9.2)
> +#
> +# Features:
> +#
> +# @deprecated: Member @reconnect is deprecated.  Use @reconnect-ms
> +#     instead.
>  #
>  # Since: 1.4
>  ##
> @@ -287,7 +299,8 @@
>              '*telnet': 'bool',
>              '*tn3270': 'bool',
>              '*websocket': 'bool',
> -            '*reconnect': 'int' },
> +            '*reconnect': { 'type': 'int', 'features': [ 'deprecated' ] },
> +            '*reconnect-ms': 'int' },
>    'base': 'ChardevCommon' }

This is a little off-topic:

So I wanted to make libvirt use the new parameter to stay ahead
deprecation. I've applied this patch to qemu, dumped capabilities and
pretty much expected a bunch of test cases in libvirt fail as they'd be
using a deprecated field as libvirt is supposed to validate everything.

And the test suite passed unexpectedly. I've dug further and noticed
that for some reason libvirt doesn't still use JSON parameters for
-chardev (which is the pre-requisite for validation).

I've also noticed that at some point I attempted to convert it over
witnessed by having an (unused) capability named QEMU_CAPS_CHARDEV_JSON
that I've introduced.

My questions are:
1) Does '-chardev' accept JSON identical to 'chardev-add' QMP command?

If yes:
2) Since when can that be used? (What can I use as a witness)
3) Are there any gotchas?

I wonder this as I'd love to finish that out, but I really don't fancy
digging into qemu to find a gotcha 3/4 of the way there.

Anyways, as I've already stated, this patch is okay for libvirt, but I
didn't review the implementation, thus, on behalf of libvirt:

ACKed-by: Peter Krempa <pkrempa@redhat.com>
-chardev with a JSON argument (was: [PATCH] chardev: introduce 'reconnect-ms' and deprecate 'reconnect')
Posted by Markus Armbruster 2 months, 1 week ago
Peter Krempa <pkrempa@redhat.com> writes:

> This is a little off-topic:
>
> So I wanted to make libvirt use the new parameter to stay ahead
> deprecation. I've applied this patch to qemu, dumped capabilities and
> pretty much expected a bunch of test cases in libvirt fail as they'd be
> using a deprecated field as libvirt is supposed to validate everything.
>
> And the test suite passed unexpectedly. I've dug further and noticed
> that for some reason libvirt doesn't still use JSON parameters for
> -chardev (which is the pre-requisite for validation).
>
> I've also noticed that at some point I attempted to convert it over
> witnessed by having an (unused) capability named QEMU_CAPS_CHARDEV_JSON
> that I've introduced.
>
> My questions are:
> 1) Does '-chardev' accept JSON identical to 'chardev-add' QMP command?

Sadly, no.

How badly do you want it?

> If yes:

If we implemented it:

> 2) Since when can that be used? (What can I use as a witness)

I figure we'd provide a witness the same way we did when we added JSON
support to -device: add a feature @json-cli to chardev-add.

> 3) Are there any gotchas?

Not aware of any.  Can't be 100% sure until we try.

> I wonder this as I'd love to finish that out, but I really don't fancy
> digging into qemu to find a gotcha 3/4 of the way there.

Understandable :)

> Anyways, as I've already stated, this patch is okay for libvirt, but I
> didn't review the implementation, thus, on behalf of libvirt:
>
> ACKed-by: Peter Krempa <pkrempa@redhat.com>

Thanks!
Re: -chardev with a JSON argument (was: [PATCH] chardev: introduce 'reconnect-ms' and deprecate 'reconnect')
Posted by Kevin Wolf 2 months, 1 week ago
Am 14.09.2024 um 10:42 hat Markus Armbruster geschrieben:
> Peter Krempa <pkrempa@redhat.com> writes:
> 
> > This is a little off-topic:
> >
> > So I wanted to make libvirt use the new parameter to stay ahead
> > deprecation. I've applied this patch to qemu, dumped capabilities and
> > pretty much expected a bunch of test cases in libvirt fail as they'd be
> > using a deprecated field as libvirt is supposed to validate everything.
> >
> > And the test suite passed unexpectedly. I've dug further and noticed
> > that for some reason libvirt doesn't still use JSON parameters for
> > -chardev (which is the pre-requisite for validation).
> >
> > I've also noticed that at some point I attempted to convert it over
> > witnessed by having an (unused) capability named QEMU_CAPS_CHARDEV_JSON
> > that I've introduced.
> >
> > My questions are:
> > 1) Does '-chardev' accept JSON identical to 'chardev-add' QMP command?
> 
> Sadly, no.
> 
> How badly do you want it?

I suppose even my old patches to fully QAPIfy -chardev could be revived,
which would not only add JSON support, but also make sure that
everything that works in JSON also works with keyval syntax, both ways
stay in sync in the future and that you can access non-scalar options
without JSON:

https://repo.or.cz/qemu/kevin.git/shortlog/refs/tags/qapi-alias-chardev-v4

Without QAPI aliases, they will have to contain some ugly code to do the
compatibility conversion, but whatever can be generated can also be
written manually...

Kevin
Re: -chardev with a JSON argument (was: [PATCH] chardev: introduce 'reconnect-ms' and deprecate 'reconnect')
Posted by Peter Krempa 2 months, 1 week ago
On Sat, Sep 14, 2024 at 10:42:36 +0200, Markus Armbruster wrote:
> Peter Krempa <pkrempa@redhat.com> writes:
> 
> > This is a little off-topic:
> >
> > So I wanted to make libvirt use the new parameter to stay ahead
> > deprecation. I've applied this patch to qemu, dumped capabilities and
> > pretty much expected a bunch of test cases in libvirt fail as they'd be
> > using a deprecated field as libvirt is supposed to validate everything.
> >
> > And the test suite passed unexpectedly. I've dug further and noticed
> > that for some reason libvirt doesn't still use JSON parameters for
> > -chardev (which is the pre-requisite for validation).
> >
> > I've also noticed that at some point I attempted to convert it over
> > witnessed by having an (unused) capability named QEMU_CAPS_CHARDEV_JSON
> > that I've introduced.
> >
> > My questions are:
> > 1) Does '-chardev' accept JSON identical to 'chardev-add' QMP command?
> 
> Sadly, no.

Yeah, in the meanwhile I had a look and also remembered that we spoke
about why this was the case. (All the 'wrapper' objects making the
schema of 'chardev-add' extremely unpleasant)

> How badly do you want it?

So the main motivation is to have just one instance of the code
generating the config for qemu. As JSON has type information inside
libvirt always generates JSON internally first. This is the case for
-object, -device, -netdev, -blockdev, ...

For '-chardev' we currently (in tree) have two separate formatters for
'-chardev' and QMP 'chardev-add'. I do have a reasonalby looking rework
in the works which unifies them (with a few quirky  "if (commandline)"
blocks).

In cases when we need to support the old syntax for any reason we do
have a converter that takes JSON and outputs the qemuopts syntax. This
is possible for now as we have mostly flat structures and the only
difference for now is how the arrays are processed in -device vs -netdev
(by callbacks to the converter).

For -chardev this will require a bit more logic as we need to avoid the
extra wrappers for commandline output and few fields have different
names in QMP than on the commandline.

My main goal here is to achieve validation against the QMP schema, and
it's much easier for us to add test cases via XML than hardcoding them
in C.

So honestly I don't think we want it too badly. Definitely not so bad
that having it with bad design. I reckon that doing the refactor will
also simplify things for the future if the QMP design will be changed to
e.g. drop the wrappers.

> > If yes:
> 
> If we implemented it:
> 
> > 2) Since when can that be used? (What can I use as a witness)
> 
> I figure we'd provide a witness the same way we did when we added JSON
> support to -device: add a feature @json-cli to chardev-add.

Yup, that'll always work :)


> > 3) Are there any gotchas?
> 
> Not aware of any.  Can't be 100% sure until we try.
> 
> > I wonder this as I'd love to finish that out, but I really don't fancy
> > digging into qemu to find a gotcha 3/4 of the way there.
> 
> Understandable :)
> 
> > Anyways, as I've already stated, this patch is okay for libvirt, but I
> > didn't review the implementation, thus, on behalf of libvirt:
> >
> > ACKed-by: Peter Krempa <pkrempa@redhat.com>
> 
> Thanks!
>
Re: [PATCH] chardev: introduce 'reconnect-ms' and deprecate 'reconnect'
Posted by Vladimir Sementsov-Ogievskiy 2 months, 2 weeks ago
On 04.09.24 08:19, Daniil Tatianin wrote:
> The 'reconnect' option only allows to specify the time in seconds,
> which is way too long for certain workflows.
> 
> We have a lightweight disk backend server, which takes about 20ms to
> live update, but due to this limitation in QEMU, previously the guest
> disk controller would hang for one second because it would take this
> long for QEMU to reinitialize the socket connection.
> 
> Introduce a new option called 'reconnect-ms', which is the same as
> 'reconnect', except the value is treated as milliseconds. These are
> mutually exclusive and specifying both results in an error.
> 
> 'reconnect' is also deprecated by this commit to make it possible to
> remove it in the future as to not keep two options that control the
> same thing.
> 
> Signed-off-by: Daniil Tatianin<d-tatianin@yandex-team.ru>

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

-- 
Best regards,
Vladimir