| Message ID | 20240905085703.106156-1-d-tatianin@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] chardev: introduce 'reconnect-ms' and deprecate 'reconnect' | expand |
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@yandex-team.ru> > Acked-by: Peter Krempa <pkrempa@redhat.com> > Signed-off-by: Daniil Tatianin <d-tatianin@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' } > > ##
Daniil Tatianin <d-tatianin@yandex-team.ru> writes: > 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. Good: $ upstream-qemu -nodefaults -chardev socket,id=chr0,path=test-hmp,server=off,reconnect=1,reconnect-ms=2 upstream-qemu: -chardev socket,id=chr0,path=test-hmp,server=off,reconnect=1,reconnect-ms=2: 'reconnect' and 'reconnect-ms' are mutually exclusive Bad: $ upstream-qemu -nodefaults -S -display none -qmp stdio {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 9}, "package": "v9.1.0-211-ga0866249bd"}, "capabilities": ["oob"]}} {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}} {"return": {}} {"execute":"chardev-add", "arguments": {"id":"chr0", "backend": {"type": "socket", "data": {"server": false, "addr": {"type": "unix", "data": {"path": "xyz"}}, "reconnect": 1, "reconnect-ms": 2}}}} {"return": {}} upstream-qemu: Unable to connect character device chr0: Failed to connect to 'xyz': No such file or directory We're not rejecting simultaneous use of @reconnect and @reconnect-ms here. Moreover, you somehow regressed the handling of the "unable to connect" error. Before the patch, behavior is correct: $ upstream-qemu -nodefaults -S -display none -qmp stdio {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 9}, "package": "v9.1.0-210-g4b7ea33074"}, "capabilities": ["oob"]}} {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}} {"return": {}} {"execute":"chardev-add", "arguments": {"id":"chr0", "backend": {"type": "socket", "data": {"server": false, "addr": {"type": "unix", "data": {"path": "xyz"}}}}}} {"error": {"class": "GenericError", "desc": "Failed to add chardev 'chr0': Failed to connect to 'xyz': No such file or directory"}} > '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@yandex-team.ru> > Acked-by: Peter Krempa <pkrempa@redhat.com> > Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
On 9/13/24 11:57 AM, Markus Armbruster wrote: > Daniil Tatianin <d-tatianin@yandex-team.ru> writes: > >> 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. > Good: > > $ upstream-qemu -nodefaults -chardev socket,id=chr0,path=test-hmp,server=off,reconnect=1,reconnect-ms=2 > upstream-qemu: -chardev socket,id=chr0,path=test-hmp,server=off,reconnect=1,reconnect-ms=2: 'reconnect' and 'reconnect-ms' are mutually exclusive > > Bad: > > $ upstream-qemu -nodefaults -S -display none -qmp stdio > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 9}, "package": "v9.1.0-211-ga0866249bd"}, "capabilities": ["oob"]}} > {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}} > {"return": {}} > {"execute":"chardev-add", "arguments": {"id":"chr0", "backend": {"type": "socket", "data": {"server": false, "addr": {"type": "unix", "data": {"path": "xyz"}}, "reconnect": 1, "reconnect-ms": 2}}}} > {"return": {}} > upstream-qemu: Unable to connect character device chr0: Failed to connect to 'xyz': No such file or directory > > We're not rejecting simultaneous use of @reconnect and @reconnect-ms > here. > > Moreover, you somehow regressed the handling of the "unable to connect" > error. Before the patch, behavior is correct: > > $ upstream-qemu -nodefaults -S -display none -qmp stdio > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 1, "major": 9}, "package": "v9.1.0-210-g4b7ea33074"}, "capabilities": ["oob"]}} > {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}} > {"return": {}} > {"execute":"chardev-add", "arguments": {"id":"chr0", "backend": {"type": "socket", "data": {"server": false, "addr": {"type": "unix", "data": {"path": "xyz"}}}}}} > {"error": {"class": "GenericError", "desc": "Failed to add chardev 'chr0': Failed to connect to 'xyz': No such file or directory"}} Oh wow, thanks for spotting! I'll be sure to take a look at these. >> '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@yandex-team.ru> >> Acked-by: Peter Krempa <pkrempa@redhat.com> >> Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
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' } ##