2:25 a.m.
Ping :) I think this one should be good to go now!
On 9/5/24 11:57 AM, 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.
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)yandex-team.ru>
> Acked-by: Peter Krempa <pkrempa(a)redhat.com>
> Signed-off-by: Daniil Tatianin <d-tatianin(a)yandex-team.ru>
> ---
>
> Changes since v0:
> - Mention the deprecation in docs (Paolo)
>
> ---
> chardev/char-socket.c | 33 ++++++++++++++++++++++++---------
> chardev/char.c | 3 +++
> docs/about/deprecated.rst | 6 ++++++
> include/chardev/char-socket.h | 2 +-
> qapi/char.json | 17 +++++++++++++++--
> 5 files changed, 49 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/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 88f0f03786..e5db9bc6e9 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -430,6 +430,12 @@ Backend ``memory`` (since 9.0)
>
> ``memory`` is a deprecated synonym for ``ringbuf``.
>
> +``reconnect`` (since 9.2)
> +^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The ``reconnect`` option only allows specifiying second granularity timeouts,
> +which is not enough for all types of use cases, use ``reconnect-ms`` instead.
> +
> CPU device properties
>
'''''''''''''''''''''
>
> 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' }
>
> ##