chardev/char-socket.c | 2 +- tests/test-char.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-)
If the socket is connecting or connected, tcp_chr_update_read_handler will
be called but it should not set the NetListener's callbacks again.
Otherwise, tcp_chr_accept is invoked while the socket is in connected
state and you get an assertion failure.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
chardev/char-socket.c | 2 +-
tests/test-char.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 96 insertions(+), 1 deletion(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 4fcdd8a..6d287ba 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -632,7 +632,7 @@ static void tcp_chr_update_read_handler(Chardev *chr)
{
SocketChardev *s = SOCKET_CHARDEV(chr);
- if (s->listener) {
+ if (s->listener && s->state == TCP_CHARDEV_STATE_DISCONNECTED) {
/*
* It's possible that chardev context is changed in
* qemu_chr_be_update_read_handlers(). Reset it for QIO net
diff --git a/tests/test-char.c b/tests/test-char.c
index 63b4d32..f1dec89 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -1003,6 +1003,97 @@ static void char_socket_client_test(gconstpointer opaque)
g_free(optstr);
}
+static void
+count_closed_event(void *opaque, int event)
+{
+ int *count = opaque;
+ if (event == CHR_EVENT_CLOSED) {
+ (*count)++;
+ }
+}
+
+
+static void char_socket_server_two_clients_test(gconstpointer opaque)
+{
+ SocketAddress *incoming_addr = (gpointer) opaque;
+ Chardev *chr;
+ CharBackend be = {0};
+ QObject *qaddr;
+ SocketAddress *addr;
+ Visitor *v;
+ char *optstr;
+ QemuOpts *opts;
+ QIOChannelSocket *ioc1, *ioc2;
+ int closed = 0;
+
+ g_setenv("QTEST_SILENT_ERRORS", "1", 1);
+ /*
+ * We rely on addr containing "nowait", otherwise
+ * qemu_chr_new() will block until a client connects. We
+ * can't spawn our client thread though, because until
+ * qemu_chr_new() returns we don't know what TCP port was
+ * allocated by the OS
+ */
+ optstr = char_socket_addr_to_opt_str(incoming_addr,
+ false,
+ NULL,
+ true);
+ opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"),
+ optstr, true);
+ g_assert_nonnull(opts);
+ chr = qemu_chr_new_from_opts(opts, NULL, &error_abort);
+ qemu_opts_del(opts);
+ g_assert_nonnull(chr);
+ g_assert(!object_property_get_bool(OBJECT(chr), "connected", &error_abort));
+
+ qaddr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
+ g_assert_nonnull(qaddr);
+
+ v = qobject_input_visitor_new(qaddr);
+ visit_type_SocketAddress(v, "addr", &addr, &error_abort);
+ visit_free(v);
+ qobject_unref(qaddr);
+
+ qemu_chr_fe_init(&be, chr, &error_abort);
+
+ qemu_chr_fe_set_handlers(&be, NULL, NULL, count_closed_event, NULL,
+ &closed, NULL, true);
+
+ ioc1 = qio_channel_socket_new();
+ qio_channel_socket_connect_sync(ioc1, addr, &error_abort);
+ qemu_chr_wait_connected(chr, &error_abort);
+
+ /* switch the chardev to another context */
+ GMainContext *ctx = g_main_context_new();
+ qemu_chr_fe_set_handlers(&be, NULL, NULL, count_closed_event, NULL,
+ &closed, ctx, true);
+
+ /* Start a second connection while the first is still connected.
+ * It will be placed in the listen() backlog, and connect() will
+ * succeed immediately.
+ */
+ ioc2 = qio_channel_socket_new();
+ qio_channel_socket_connect_sync(ioc2, addr, &error_abort);
+
+ object_unparent(OBJECT(ioc1));
+ /* The two connections should now be processed serially. */
+ while (g_main_context_iteration(ctx, TRUE)) {
+ if (closed == 1 && ioc2) {
+ object_unparent(OBJECT(ioc2));
+ ioc2 = NULL;
+ }
+ if (closed == 2) {
+ break;
+ }
+ }
+
+ qapi_free_SocketAddress(addr);
+ object_unparent(OBJECT(chr));
+ g_main_context_unref(ctx);
+ g_free(optstr);
+ g_unsetenv("QTEST_SILENT_ERRORS");
+}
+
#ifdef HAVE_CHARDEV_SERIAL
static void char_serial_test(void)
@@ -1346,6 +1437,10 @@ int main(int argc, char **argv)
SOCKET_SERVER_TEST(unix, &unixaddr);
SOCKET_CLIENT_TEST(unix, &unixaddr);
#endif
+ g_test_add_data_func("/char/socket/server/two-clients/tcp", &tcpaddr,
+ char_socket_server_two_clients_test);
+ g_test_add_data_func("/char/socket/server/two-clients/unix", &unixaddr,
+ char_socket_server_two_clients_test);
g_test_add_func("/char/udp", char_udp_test);
--
1.8.3.1
On Wed, Feb 20, 2019 at 04:40:16PM +0100, Paolo Bonzini wrote:
> If the socket is connecting or connected, tcp_chr_update_read_handler will
> be called but it should not set the NetListener's callbacks again.
> Otherwise, tcp_chr_accept is invoked while the socket is in connected
> state and you get an assertion failure.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> chardev/char-socket.c | 2 +-
> tests/test-char.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 96 insertions(+), 1 deletion(-)
> diff --git a/tests/test-char.c b/tests/test-char.c
> index 63b4d32..f1dec89 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -1346,6 +1437,10 @@ int main(int argc, char **argv)
> SOCKET_SERVER_TEST(unix, &unixaddr);
> SOCKET_CLIENT_TEST(unix, &unixaddr);
> #endif
> + g_test_add_data_func("/char/socket/server/two-clients/tcp", &tcpaddr,
> + char_socket_server_two_clients_test);
> + g_test_add_data_func("/char/socket/server/two-clients/unix", &unixaddr,
> + char_socket_server_two_clients_test);
The use of "unixaddr" needs to be inside a "#ifndef WIN32" conditional.
If that is fixed, then consider it
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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 :|
Hi
On Wed, Feb 20, 2019 at 5:08 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> If the socket is connecting or connected, tcp_chr_update_read_handler will
> be called but it should not set the NetListener's callbacks again.
> Otherwise, tcp_chr_accept is invoked while the socket is in connected
> state and you get an assertion failure.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Change looks good,
> ---
> chardev/char-socket.c | 2 +-
> tests/test-char.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 4fcdd8a..6d287ba 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -632,7 +632,7 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> {
> SocketChardev *s = SOCKET_CHARDEV(chr);
>
> - if (s->listener) {
> + if (s->listener && s->state == TCP_CHARDEV_STATE_DISCONNECTED) {
> /*
> * It's possible that chardev context is changed in
> * qemu_chr_be_update_read_handlers(). Reset it for QIO net
> diff --git a/tests/test-char.c b/tests/test-char.c
> index 63b4d32..f1dec89 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -1003,6 +1003,97 @@ static void char_socket_client_test(gconstpointer opaque)
> g_free(optstr);
> }
>
> +static void
> +count_closed_event(void *opaque, int event)
> +{
> + int *count = opaque;
> + if (event == CHR_EVENT_CLOSED) {
> + (*count)++;
> + }
> +}
> +
> +
> +static void char_socket_server_two_clients_test(gconstpointer opaque)
> +{
> + SocketAddress *incoming_addr = (gpointer) opaque;
> + Chardev *chr;
> + CharBackend be = {0};
> + QObject *qaddr;
> + SocketAddress *addr;
> + Visitor *v;
> + char *optstr;
> + QemuOpts *opts;
> + QIOChannelSocket *ioc1, *ioc2;
> + int closed = 0;
> +
> + g_setenv("QTEST_SILENT_ERRORS", "1", 1);
> + /*
> + * We rely on addr containing "nowait", otherwise
> + * qemu_chr_new() will block until a client connects. We
> + * can't spawn our client thread though, because until
> + * qemu_chr_new() returns we don't know what TCP port was
> + * allocated by the OS
> + */
> + optstr = char_socket_addr_to_opt_str(incoming_addr,
> + false,
> + NULL,
> + true);
> + opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"),
> + optstr, true);
> + g_assert_nonnull(opts);
> + chr = qemu_chr_new_from_opts(opts, NULL, &error_abort);
> + qemu_opts_del(opts);
> + g_assert_nonnull(chr);
> + g_assert(!object_property_get_bool(OBJECT(chr), "connected", &error_abort));
> +
> + qaddr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
> + g_assert_nonnull(qaddr);
> +
> + v = qobject_input_visitor_new(qaddr);
> + visit_type_SocketAddress(v, "addr", &addr, &error_abort);
> + visit_free(v);
> + qobject_unref(qaddr);
> +
> + qemu_chr_fe_init(&be, chr, &error_abort);
> +
> + qemu_chr_fe_set_handlers(&be, NULL, NULL, count_closed_event, NULL,
> + &closed, NULL, true);
> +
> + ioc1 = qio_channel_socket_new();
> + qio_channel_socket_connect_sync(ioc1, addr, &error_abort);
> + qemu_chr_wait_connected(chr, &error_abort);
> +
> + /* switch the chardev to another context */
> + GMainContext *ctx = g_main_context_new();
> + qemu_chr_fe_set_handlers(&be, NULL, NULL, count_closed_event, NULL,
> + &closed, ctx, true);
> +
> + /* Start a second connection while the first is still connected.
> + * It will be placed in the listen() backlog, and connect() will
> + * succeed immediately.
> + */
> + ioc2 = qio_channel_socket_new();
> + qio_channel_socket_connect_sync(ioc2, addr, &error_abort);
> +
> + object_unparent(OBJECT(ioc1));
But object_unparent() is incorrect here, QIOChannel isn't parented.
unref() instead.
> + /* The two connections should now be processed serially. */
> + while (g_main_context_iteration(ctx, TRUE)) {
> + if (closed == 1 && ioc2) {
> + object_unparent(OBJECT(ioc2));
> + ioc2 = NULL;
> + }
> + if (closed == 2) {
> + break;
> + }
> + }
> +
> + qapi_free_SocketAddress(addr);
> + object_unparent(OBJECT(chr));
> + g_main_context_unref(ctx);
> + g_free(optstr);
> + g_unsetenv("QTEST_SILENT_ERRORS");
> +}
> +
>
> #ifdef HAVE_CHARDEV_SERIAL
> static void char_serial_test(void)
> @@ -1346,6 +1437,10 @@ int main(int argc, char **argv)
> SOCKET_SERVER_TEST(unix, &unixaddr);
> SOCKET_CLIENT_TEST(unix, &unixaddr);
> #endif
> + g_test_add_data_func("/char/socket/server/two-clients/tcp", &tcpaddr,
> + char_socket_server_two_clients_test);
> + g_test_add_data_func("/char/socket/server/two-clients/unix", &unixaddr,
> + char_socket_server_two_clients_test);
tabs
with that,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
> g_test_add_func("/char/udp", char_udp_test);
> --
> 1.8.3.1
>
>
--
Marc-André Lureau
Patchew URL: https://patchew.org/QEMU/1550677216-5077-1-git-send-email-pbonzini@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 1550677216-5077-1-git-send-email-pbonzini@redhat.com Subject: [Qemu-devel] [PATCH] chardev-socket: do not blindly reset handlers when switching GMainContext Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 3b90d7e1d7 chardev-socket: do not blindly reset handlers when switching GMainContext === OUTPUT BEGIN === WARNING: Block comments use a leading /* on a separate line #102: FILE: tests/test-char.c:1071: + /* Start a second connection while the first is still connected. ERROR: code indent should never use tabs #136: FILE: tests/test-char.c:1441: +^I^I char_socket_server_two_clients_test);$ ERROR: code indent should never use tabs #138: FILE: tests/test-char.c:1443: +^I^I char_socket_server_two_clients_test);$ total: 2 errors, 1 warnings, 115 lines checked Commit 3b90d7e1d778 (chardev-socket: do not blindly reset handlers when switching GMainContext) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/1550677216-5077-1-git-send-email-pbonzini@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
© 2016 - 2025 Red Hat, Inc.