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

[Paolo Bonzini <pbonzini@redhat.com>]

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' }
>   
>   ##