[Qemu-devel] [PATCH] chardev-socket: do not blindly reset handlers when switching GMainContext

[Qemu-devel] [PATCH] chardev-socket: do not blindly reset handlers when switching GMainContext

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] chardev-socket: do not blindly reset handlers when switching GMainContext

[Daniel P. Berrangé]

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 :|

Re: [Qemu-devel] [PATCH] chardev-socket: do not blindly reset handlers when switching GMainContext

[Marc-André Lureau]

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

Re: [Qemu-devel] [PATCH] chardev-socket: do not blindly reset handlers when switching GMainContext

[no-reply]

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