[Qemu-devel] [PATCH 00/12] chardev: refactoring & many bugfixes related tcp_chr_wait_connected

[Qemu-devel] [PATCH 00/12] chardev: refactoring & many bugfixes related tcp_chr_wait_connected

[Daniel P. Berrangé]

This series comes out of a discussion between myself & Yongji Xie in:

  https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg01881.html

I eventually understood that the problem faced was that
tcp_chr_wait_connected was racing with the background connection attempt
previously started, causing two connections to be established. This
broke because some vhost user servers only allow a single connection.

After messing around with the code alot the final solution was in fact
very easy. We simply have to delay the first background connection
attempt until the main loop is running. It will then automatically
turn into a no-op if tcp_chr_wait_connected has been run. This is
dealt with in the last patch in this series

I believe this should solve the problem Yongji Xie faced, and thus not
require us to add support for "nowait" option with client sockets at
all. The reconnect=1 option effectively already implements nowait
semantics, and now plays nicely with tcp_chr_wait_connected.

In investigating this I found various other bugs that needed fixing and
identified some useful refactoring to simplify / clarify the code, hence
this very long series.

This series passes make check-unit & check-qtest (for x86_64 target).

I did a test with tests/vhost-user-bridge too, which was in fact already
broken for the same reason Yongji Xie found - it only accepts a single
connection. This series fixes use of reconnect=1 with vhost-user-bridge.

Despite me not adding new functionality in this series, I think we could
probably do with further unit test cover for the socket chardev backend.
I'll investigate this while waiting for reviews on this series so far.

Daniel P. Berrangé (12):
  chardev: fix validation of options for QMP created chardevs
  chardev: forbid 'reconnect' option with server sockets
  chardev: forbid 'wait' option with client sockets
  chardev: remove many local variables in qemu_chr_parse_socket
  chardev: ensure qemu_chr_parse_compat reports missing driver error
  chardev: remove unused 'sioc' variable & cleanup paths
  chardev: split tcp_chr_wait_connected into two methods
  chardev: split up qmp_chardev_open_socket connection code
  chardev: use a state machine for socket connection state
  chardev: honour the reconnect setting in tcp_chr_wait_connected
  chardev: disallow TLS/telnet/websocket with tcp_chr_wait_connected
  chardev: fix race with client connections in tcp_chr_wait_connected

 chardev/char-socket.c          | 428 +++++++++++++++++++++++----------
 chardev/char.c                 |   2 +
 tests/ivshmem-test.c           |   2 +-
 tests/libqtest.c               |   4 +-
 tests/test-filter-redirector.c |   4 +-
 5 files changed, 303 insertions(+), 137 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 01/12] chardev: fix validation of options for QMP created chardevs

[Daniel P. Berrangé]

The TLS creds option is not valid with certain address types. The user
config was only checked for errors when parsing legacy QemuOpts, thus
the user could pass unsupported values via QMP.

Pull all code for validating options out into a new method
qmp_chardev_validate_socket, that is called from the main
qmp_chardev_open_socket method. This adds a missing check for rejecting
TLS creds with the vsock address type.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------
 1 file changed, 66 insertions(+), 26 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index eaa8e8b68f..6669acb35f 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
     return false;
 }
 
+
+static bool qmp_chardev_validate_socket(ChardevSocket *sock,
+                                        SocketAddress *addr,
+                                        Error **errp)
+{
+    /* Validate any options which have a dependancy on address type */
+    switch (addr->type) {
+    case SOCKET_ADDRESS_TYPE_FD:
+        if (sock->has_reconnect) {
+            error_setg(errp,
+                       "'reconnect' option is incompatible with "
+                       "'fd' address type");
+            return false;
+        }
+        if (sock->has_tls_creds &&
+            !(sock->has_server && sock->server)) {
+            error_setg(errp,
+                       "'tls_creds' option is incompatible with "
+                       "'fd' address type as client");
+            return false;
+        }
+        break;
+
+    case SOCKET_ADDRESS_TYPE_UNIX:
+        if (sock->has_tls_creds) {
+            error_setg(errp,
+                       "'tls_creds' option is incompatible with "
+                       "'unix' address type");
+            return false;
+        }
+        break;
+
+    case SOCKET_ADDRESS_TYPE_INET:
+        break;
+
+    case SOCKET_ADDRESS_TYPE_VSOCK:
+        if (sock->has_tls_creds) {
+            error_setg(errp,
+                       "'tls_creds' option is incompatible with "
+                       "'vsock' address type");
+            return false;
+        }
+
+    default:
+        break;
+    }
+
+    /* Validate any options which have a dependancy on client vs server */
+    if (!(sock->has_server && sock->server)) {
+        if (sock->has_websocket && sock->websocket) {
+            error_setg(errp, "%s", "Websocket client is not implemented");
+            return false;
+        }
+    }
+
+    return true;
+}
+
+
 static void qmp_chardev_open_socket(Chardev *chr,
                                     ChardevBackend *backend,
                                     bool *be_opened,
@@ -1004,11 +1063,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
     QIOChannelSocket *sioc = NULL;
     SocketAddress *addr;
 
-    if (!is_listen && is_websock) {
-        error_setg(errp, "%s", "Websocket client is not implemented");
-        goto error;
-    }
-
     s->is_listen = is_listen;
     s->is_telnet = is_telnet;
     s->is_tn3270 = is_tn3270;
@@ -1049,10 +1103,10 @@ static void qmp_chardev_open_socket(Chardev *chr,
 
     s->addr = addr = socket_address_flatten(sock->addr);
 
-    if (sock->has_reconnect && addr->type == SOCKET_ADDRESS_TYPE_FD) {
-        error_setg(errp, "'reconnect' option is incompatible with 'fd'");
+    if (!qmp_chardev_validate_socket(sock, addr, errp)) {
         goto error;
     }
+
     qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
     /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */
     if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) {
@@ -1140,27 +1194,12 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
         return;
     }
 
-    backend->type = CHARDEV_BACKEND_KIND_SOCKET;
-    if (path) {
-        if (tls_creds) {
-            error_setg(errp, "TLS can only be used over TCP socket");
-            return;
-        }
-    } else if (host) {
-        if (!port) {
-            error_setg(errp, "chardev: socket: no port given");
-            return;
-        }
-    } else if (fd) {
-        /* We don't know what host to validate against when in client mode */
-        if (tls_creds && !is_listen) {
-            error_setg(errp, "TLS can not be used with pre-opened client FD");
-            return;
-        }
-    } else {
-        g_assert_not_reached();
+    if (host && !port) {
+        error_setg(errp, "chardev: socket: no port given");
+        return;
     }
 
+    backend->type = CHARDEV_BACKEND_KIND_SOCKET;
     sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
     qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
 
@@ -1178,6 +1217,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     sock->wait = is_waitconnect;
     sock->has_reconnect = qemu_opt_find(opts, "reconnect");
     sock->reconnect = reconnect;
+    sock->has_tls_creds = tls_creds;
     sock->tls_creds = g_strdup(tls_creds);
 
     addr = g_new0(SocketAddressLegacy, 1);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 01/12] chardev: fix validation of options for QMP created chardevs

[Marc-André Lureau]

Hi

On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The TLS creds option is not valid with certain address types. The user
> config was only checked for errors when parsing legacy QemuOpts, thus
> the user could pass unsupported values via QMP.
>
> Pull all code for validating options out into a new method
> qmp_chardev_validate_socket, that is called from the main
> qmp_chardev_open_socket method. This adds a missing check for rejecting
> TLS creds with the vsock address type.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------
>  1 file changed, 66 insertions(+), 26 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index eaa8e8b68f..6669acb35f 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
>      return false;
>  }
>
> +
> +static bool qmp_chardev_validate_socket(ChardevSocket *sock,
> +                                        SocketAddress *addr,
> +                                        Error **errp)
> +{
> +    /* Validate any options which have a dependancy on address type */
> +    switch (addr->type) {
> +    case SOCKET_ADDRESS_TYPE_FD:
> +        if (sock->has_reconnect) {
> +            error_setg(errp,
> +                       "'reconnect' option is incompatible with "
> +                       "'fd' address type");
> +            return false;
> +        }
> +        if (sock->has_tls_creds &&
> +            !(sock->has_server && sock->server)) {
> +            error_setg(errp,
> +                       "'tls_creds' option is incompatible with "
> +                       "'fd' address type as client");
> +            return false;
> +        }
> +        break;
> +
> +    case SOCKET_ADDRESS_TYPE_UNIX:
> +        if (sock->has_tls_creds) {
> +            error_setg(errp,
> +                       "'tls_creds' option is incompatible with "
> +                       "'unix' address type");
> +            return false;
> +        }
> +        break;
> +
> +    case SOCKET_ADDRESS_TYPE_INET:
> +        break;
> +
> +    case SOCKET_ADDRESS_TYPE_VSOCK:
> +        if (sock->has_tls_creds) {
> +            error_setg(errp,
> +                       "'tls_creds' option is incompatible with "
> +                       "'vsock' address type");
> +            return false;
> +        }
> +
> +    default:
> +        break;
> +    }
> +
> +    /* Validate any options which have a dependancy on client vs server */
> +    if (!(sock->has_server && sock->server)) {
> +        if (sock->has_websocket && sock->websocket) {
> +            error_setg(errp, "%s", "Websocket client is not implemented");
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
> +
>  static void qmp_chardev_open_socket(Chardev *chr,
>                                      ChardevBackend *backend,
>                                      bool *be_opened,
> @@ -1004,11 +1063,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
>      QIOChannelSocket *sioc = NULL;
>      SocketAddress *addr;
>
> -    if (!is_listen && is_websock) {
> -        error_setg(errp, "%s", "Websocket client is not implemented");
> -        goto error;
> -    }
> -
>      s->is_listen = is_listen;
>      s->is_telnet = is_telnet;
>      s->is_tn3270 = is_tn3270;
> @@ -1049,10 +1103,10 @@ static void qmp_chardev_open_socket(Chardev *chr,
>
>      s->addr = addr = socket_address_flatten(sock->addr);
>
> -    if (sock->has_reconnect && addr->type == SOCKET_ADDRESS_TYPE_FD) {
> -        error_setg(errp, "'reconnect' option is incompatible with 'fd'");
> +    if (!qmp_chardev_validate_socket(sock, addr, errp)) {
>          goto error;
>      }
> +
>      qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
>      /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */
>      if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) {
> @@ -1140,27 +1194,12 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>          return;
>      }
>
> -    backend->type = CHARDEV_BACKEND_KIND_SOCKET;
> -    if (path) {
> -        if (tls_creds) {
> -            error_setg(errp, "TLS can only be used over TCP socket");
> -            return;
> -        }
> -    } else if (host) {
> -        if (!port) {
> -            error_setg(errp, "chardev: socket: no port given");
> -            return;
> -        }
> -    } else if (fd) {
> -        /* We don't know what host to validate against when in client mode */
> -        if (tls_creds && !is_listen) {
> -            error_setg(errp, "TLS can not be used with pre-opened client FD");
> -            return;
> -        }
> -    } else {
> -        g_assert_not_reached();
> +    if (host && !port) {
> +        error_setg(errp, "chardev: socket: no port given");
> +        return;
>      }
>
> +    backend->type = CHARDEV_BACKEND_KIND_SOCKET;
>      sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
>      qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
>
> @@ -1178,6 +1217,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      sock->wait = is_waitconnect;
>      sock->has_reconnect = qemu_opt_find(opts, "reconnect");
>      sock->reconnect = reconnect;
> +    sock->has_tls_creds = tls_creds;
>      sock->tls_creds = g_strdup(tls_creds);
>
>      addr = g_new0(SocketAddressLegacy, 1);
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH 01/12] chardev: fix validation of options for QMP created chardevs

[Thomas Huth]

On 2019-01-15 15:52, Daniel P. Berrangé wrote:
> The TLS creds option is not valid with certain address types. The user
> config was only checked for errors when parsing legacy QemuOpts, thus
> the user could pass unsupported values via QMP.
> 
> Pull all code for validating options out into a new method
> qmp_chardev_validate_socket, that is called from the main
> qmp_chardev_open_socket method. This adds a missing check for rejecting
> TLS creds with the vsock address type.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------
>  1 file changed, 66 insertions(+), 26 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index eaa8e8b68f..6669acb35f 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
>      return false;
>  }
>  
> +

Please remove the additional empty line.

> +static bool qmp_chardev_validate_socket(ChardevSocket *sock,
> +                                        SocketAddress *addr,
> +                                        Error **errp)
> +{
> +    /* Validate any options which have a dependancy on address type */

I'd maybe rather write "dependency" which is AFAIK the more common
spelling - but I'm not a native speaker, so feel free to ignore me here.

> +    switch (addr->type) {
> +    case SOCKET_ADDRESS_TYPE_FD:
> +        if (sock->has_reconnect) {
> +            error_setg(errp,
> +                       "'reconnect' option is incompatible with "
> +                       "'fd' address type");
> +            return false;
> +        }
> +        if (sock->has_tls_creds &&
> +            !(sock->has_server && sock->server)) {
> +            error_setg(errp,
> +                       "'tls_creds' option is incompatible with "
> +                       "'fd' address type as client");
> +            return false;
> +        }
> +        break;
> +
> +    case SOCKET_ADDRESS_TYPE_UNIX:
> +        if (sock->has_tls_creds) {
> +            error_setg(errp,
> +                       "'tls_creds' option is incompatible with "
> +                       "'unix' address type");
> +            return false;
> +        }
> +        break;
> +
> +    case SOCKET_ADDRESS_TYPE_INET:
> +        break;

You could drop the empty case.

> +    case SOCKET_ADDRESS_TYPE_VSOCK:
> +        if (sock->has_tls_creds) {
> +            error_setg(errp,
> +                       "'tls_creds' option is incompatible with "
> +                       "'vsock' address type");
> +            return false;
> +        }
> +
> +    default:
> +        break;

You could drop the empty default case.

> +    }
> +
> +    /* Validate any options which have a dependancy on client vs server */
> +    if (!(sock->has_server && sock->server)) {
> +        if (sock->has_websocket && sock->websocket) {
> +            error_setg(errp, "%s", "Websocket client is not implemented");
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
> +

No duplicated empty lines, please.

>  static void qmp_chardev_open_socket(Chardev *chr,
>                                      ChardevBackend *backend,
>                                      bool *be_opened,
> @@ -1004,11 +1063,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
>      QIOChannelSocket *sioc = NULL;
>      SocketAddress *addr;
>  
> -    if (!is_listen && is_websock) {
> -        error_setg(errp, "%s", "Websocket client is not implemented");
> -        goto error;
> -    }
> -
>      s->is_listen = is_listen;
>      s->is_telnet = is_telnet;
>      s->is_tn3270 = is_tn3270;
> @@ -1049,10 +1103,10 @@ static void qmp_chardev_open_socket(Chardev *chr,
>  
>      s->addr = addr = socket_address_flatten(sock->addr);
>  
> -    if (sock->has_reconnect && addr->type == SOCKET_ADDRESS_TYPE_FD) {
> -        error_setg(errp, "'reconnect' option is incompatible with 'fd'");
> +    if (!qmp_chardev_validate_socket(sock, addr, errp)) {
>          goto error;
>      }
> +
>      qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
>      /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */
>      if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) {
> @@ -1140,27 +1194,12 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>          return;
>      }
>  
> -    backend->type = CHARDEV_BACKEND_KIND_SOCKET;
> -    if (path) {
> -        if (tls_creds) {
> -            error_setg(errp, "TLS can only be used over TCP socket");
> -            return;
> -        }
> -    } else if (host) {
> -        if (!port) {
> -            error_setg(errp, "chardev: socket: no port given");
> -            return;
> -        }
> -    } else if (fd) {
> -        /* We don't know what host to validate against when in client mode */
> -        if (tls_creds && !is_listen) {
> -            error_setg(errp, "TLS can not be used with pre-opened client FD");
> -            return;
> -        }
> -    } else {
> -        g_assert_not_reached();
> +    if (host && !port) {
> +        error_setg(errp, "chardev: socket: no port given");
> +        return;
>      }
>  
> +    backend->type = CHARDEV_BACKEND_KIND_SOCKET;
>      sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
>      qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
>  
> @@ -1178,6 +1217,7 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      sock->wait = is_waitconnect;
>      sock->has_reconnect = qemu_opt_find(opts, "reconnect");
>      sock->reconnect = reconnect;
> +    sock->has_tls_creds = tls_creds;
>      sock->tls_creds = g_strdup(tls_creds);
>  
>      addr = g_new0(SocketAddressLegacy, 1);
> 

With at least the redundant empty lines removed:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 01/12] chardev: fix validation of options for QMP created chardevs

[Daniel P. Berrangé]

On Wed, Jan 16, 2019 at 06:07:41AM +0100, Thomas Huth wrote:
> On 2019-01-15 15:52, Daniel P. Berrangé wrote:
> > The TLS creds option is not valid with certain address types. The user
> > config was only checked for errors when parsing legacy QemuOpts, thus
> > the user could pass unsupported values via QMP.
> > 
> > Pull all code for validating options out into a new method
> > qmp_chardev_validate_socket, that is called from the main
> > qmp_chardev_open_socket method. This adds a missing check for rejecting
> > TLS creds with the vsock address type.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------
> >  1 file changed, 66 insertions(+), 26 deletions(-)
> > 
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index eaa8e8b68f..6669acb35f 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
> >      return false;
> >  }
> >  
> > +
> 
> Please remove the additional empty line.

Having two blanks lines between functions is intentional to
give visual separation.

> > +static bool qmp_chardev_validate_socket(ChardevSocket *sock,
> > +                                        SocketAddress *addr,
> > +                                        Error **errp)
> > +{
> > +    /* Validate any options which have a dependancy on address type */
> 
> I'd maybe rather write "dependency" which is AFAIK the more common
> spelling - but I'm not a native speaker, so feel free to ignore me here.
> 
> > +    switch (addr->type) {
> > +    case SOCKET_ADDRESS_TYPE_FD:
> > +        if (sock->has_reconnect) {
> > +            error_setg(errp,
> > +                       "'reconnect' option is incompatible with "
> > +                       "'fd' address type");
> > +            return false;
> > +        }
> > +        if (sock->has_tls_creds &&
> > +            !(sock->has_server && sock->server)) {
> > +            error_setg(errp,
> > +                       "'tls_creds' option is incompatible with "
> > +                       "'fd' address type as client");
> > +            return false;
> > +        }
> > +        break;
> > +
> > +    case SOCKET_ADDRESS_TYPE_UNIX:
> > +        if (sock->has_tls_creds) {
> > +            error_setg(errp,
> > +                       "'tls_creds' option is incompatible with "
> > +                       "'unix' address type");
> > +            return false;
> > +        }
> > +        break;
> > +
> > +    case SOCKET_ADDRESS_TYPE_INET:
> > +        break;
> 
> You could drop the empty case.

I preferred to explicitly list all cases, so it is clear what
needs to be handled here when further checks are added later.

> 
> > +    case SOCKET_ADDRESS_TYPE_VSOCK:
> > +        if (sock->has_tls_creds) {
> > +            error_setg(errp,
> > +                       "'tls_creds' option is incompatible with "
> > +                       "'vsock' address type");
> > +            return false;
> > +        }
> > +

Opps, missing default.

> > +    default:
> > +        break;
> 
> You could drop the empty default case.

If that is not there, then the compiler forces the
listing of SOCKET_ADDRESS_TYPE__MAX instead due
to -Wswitch


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 01/12] chardev: fix validation of options for QMP created chardevs

[Markus Armbruster]

Eric, there's a QAPI code generation idea at the end.

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Jan 16, 2019 at 06:07:41AM +0100, Thomas Huth wrote:
>> On 2019-01-15 15:52, Daniel P. Berrangé wrote:
>> > The TLS creds option is not valid with certain address types. The user
>> > config was only checked for errors when parsing legacy QemuOpts, thus
>> > the user could pass unsupported values via QMP.
>> > 
>> > Pull all code for validating options out into a new method
>> > qmp_chardev_validate_socket, that is called from the main
>> > qmp_chardev_open_socket method. This adds a missing check for rejecting
>> > TLS creds with the vsock address type.
>> > 
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> >  chardev/char-socket.c | 92 +++++++++++++++++++++++++++++++------------
>> >  1 file changed, 66 insertions(+), 26 deletions(-)
>> > 
>> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> > index eaa8e8b68f..6669acb35f 100644
>> > --- a/chardev/char-socket.c
>> > +++ b/chardev/char-socket.c
>> > @@ -987,6 +987,65 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
>> >      return false;
>> >  }
>> >  
>> > +
>> 
>> Please remove the additional empty line.
>
> Having two blanks lines between functions is intentional to
> give visual separation.
>
>> > +static bool qmp_chardev_validate_socket(ChardevSocket *sock,
>> > +                                        SocketAddress *addr,
>> > +                                        Error **errp)
>> > +{
>> > +    /* Validate any options which have a dependancy on address type */
>> 
>> I'd maybe rather write "dependency" which is AFAIK the more common
>> spelling - but I'm not a native speaker, so feel free to ignore me here.

For what it's worth, my dictionary wants dependency.

>> > +    switch (addr->type) {
>> > +    case SOCKET_ADDRESS_TYPE_FD:
>> > +        if (sock->has_reconnect) {
>> > +            error_setg(errp,
>> > +                       "'reconnect' option is incompatible with "
>> > +                       "'fd' address type");
>> > +            return false;
>> > +        }
>> > +        if (sock->has_tls_creds &&
>> > +            !(sock->has_server && sock->server)) {
>> > +            error_setg(errp,
>> > +                       "'tls_creds' option is incompatible with "
>> > +                       "'fd' address type as client");
>> > +            return false;
>> > +        }
>> > +        break;
>> > +
>> > +    case SOCKET_ADDRESS_TYPE_UNIX:
>> > +        if (sock->has_tls_creds) {
>> > +            error_setg(errp,
>> > +                       "'tls_creds' option is incompatible with "
>> > +                       "'unix' address type");
>> > +            return false;
>> > +        }
>> > +        break;
>> > +
>> > +    case SOCKET_ADDRESS_TYPE_INET:
>> > +        break;
>> 
>> You could drop the empty case.
>
> I preferred to explicitly list all cases, so it is clear what
> needs to be handled here when further checks are added later.

Matter of taste, your choice unless maintainer overrules.

>> 
>> > +    case SOCKET_ADDRESS_TYPE_VSOCK:
>> > +        if (sock->has_tls_creds) {
>> > +            error_setg(errp,
>> > +                       "'tls_creds' option is incompatible with "
>> > +                       "'vsock' address type");
>> > +            return false;
>> > +        }
>> > +
>
> Opps, missing default.

I guess you mean break.

>> > +    default:
>> > +        break;
>> 
>> You could drop the empty default case.
>
> If that is not there, then the compiler forces the
> listing of SOCKET_ADDRESS_TYPE__MAX instead due
> to -Wswitch

I wonder whether generating something like

    typedef enum SocketAddressType {
        SOCKET_ADDRESS_TYPE_INET,
        SOCKET_ADDRESS_TYPE_UNIX,
        SOCKET_ADDRESS_TYPE_VSOCK,
        SOCKET_ADDRESS_TYPE_FD,
    } SocketAddressType;

    #define SOCKET_ADDRESS_TYPE__MAX (SOCKET_ADDRESS_TYPE_FD + 1)

would be better.

Re: [Qemu-devel] [PATCH 01/12] chardev: fix validation of options for QMP created chardevs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

On 1/17/19 3:21 AM, Markus Armbruster wrote:
> Eric, there's a QAPI code generation idea at the end.
> 

>> If that is not there, then the compiler forces the
>> listing of SOCKET_ADDRESS_TYPE__MAX instead due
>> to -Wswitch
> 
> I wonder whether generating something like
> 
>     typedef enum SocketAddressType {
>         SOCKET_ADDRESS_TYPE_INET,
>         SOCKET_ADDRESS_TYPE_UNIX,
>         SOCKET_ADDRESS_TYPE_VSOCK,
>         SOCKET_ADDRESS_TYPE_FD,
>     } SocketAddressType;
> 
>     #define SOCKET_ADDRESS_TYPE__MAX (SOCKET_ADDRESS_TYPE_FD + 1)
> 
> would be better.

Or even an anon enum instead of a define:

typedef enum SocketAddressType {
    SOCKET_ADDRESS_TYPE_INET,
    SOCKET_ADDRESS_TYPE_UNIX,
    SOCKET_ADDRESS_TYPE_VSOCK,
    SOCKET_ADDRESS_TYPE_FD,
} SocketAddressType;
enum {
    SOCKET_ADDRESS_TYPE__MAX = SOCKET_ADDRESS_TYPE_FD + 1
};

The idea is interesting; since it is generated code, the change is
fairly easy to make.  I don't know how much fall-out we'd get in the
rest of the code base - only one way to find out.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH 02/12] chardev: forbid 'reconnect' option with server sockets

[Daniel P. Berrangé]

The 'reconnect' option is used to give the sleep time, in seconds,
before a client socket attempts to re-establish a connection to the
server. It does not make sense to set this for server sockets, as they
will always accept a new client connection immediately after the
previous one went away.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 6669acb35f..4570755adf 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1035,7 +1035,14 @@ static bool qmp_chardev_validate_socket(ChardevSocket *sock,
     }
 
     /* Validate any options which have a dependancy on client vs server */
-    if (!(sock->has_server && sock->server)) {
+    if (!sock->has_server || sock->server) {
+        if (sock->has_reconnect) {
+            error_setg(errp,
+                       "'reconnect' option is incompatible with "
+                       "socket in server listen mode");
+            return false;
+        }
+    } else {
         if (sock->has_websocket && sock->websocket) {
             error_setg(errp, "%s", "Websocket client is not implemented");
             return false;
-- 
2.20.1

Re: [Qemu-devel] [PATCH 02/12] chardev: forbid 'reconnect' option with server sockets

[Marc-André Lureau]

On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The 'reconnect' option is used to give the sleep time, in seconds,
> before a client socket attempts to re-establish a connection to the
> server. It does not make sense to set this for server sockets, as they
> will always accept a new client connection immediately after the
> previous one went away.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 6669acb35f..4570755adf 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1035,7 +1035,14 @@ static bool qmp_chardev_validate_socket(ChardevSocket *sock,
>      }
>
>      /* Validate any options which have a dependancy on client vs server */
> -    if (!(sock->has_server && sock->server)) {
> +    if (!sock->has_server || sock->server) {
> +        if (sock->has_reconnect) {
> +            error_setg(errp,
> +                       "'reconnect' option is incompatible with "
> +                       "socket in server listen mode");
> +            return false;
> +        }
> +    } else {
>          if (sock->has_websocket && sock->websocket) {
>              error_setg(errp, "%s", "Websocket client is not implemented");
>              return false;
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH 02/12] chardev: forbid 'reconnect' option with server sockets

[Thomas Huth]

On 2019-01-15 15:52, Daniel P. Berrangé wrote:
> The 'reconnect' option is used to give the sleep time, in seconds,
> before a client socket attempts to re-establish a connection to the
> server. It does not make sense to set this for server sockets, as they
> will always accept a new client connection immediately after the
> previous one went away.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 6669acb35f..4570755adf 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1035,7 +1035,14 @@ static bool qmp_chardev_validate_socket(ChardevSocket *sock,
>      }
>  
>      /* Validate any options which have a dependancy on client vs server */
> -    if (!(sock->has_server && sock->server)) {
> +    if (!sock->has_server || sock->server) {
> +        if (sock->has_reconnect) {
> +            error_setg(errp,
> +                       "'reconnect' option is incompatible with "
> +                       "socket in server listen mode");
> +            return false;
> +        }
> +    } else {
>          if (sock->has_websocket && sock->websocket) {
>              error_setg(errp, "%s", "Websocket client is not implemented");
>              return false;
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH 03/12] chardev: forbid 'wait' option with client sockets

[Daniel P. Berrangé]

The 'wait'/'nowait' parameter is used to tell server sockets whether to
block until a client is accepted during initialization. Client chardevs
have always silently ignored this option. Various tests were mistakenly
passing this option for their client chardevs.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c          | 12 +++++++++++-
 tests/ivshmem-test.c           |  2 +-
 tests/libqtest.c               |  4 ++--
 tests/test-filter-redirector.c |  4 ++--
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 4570755adf..233f16f72d 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1047,6 +1047,12 @@ static bool qmp_chardev_validate_socket(ChardevSocket *sock,
             error_setg(errp, "%s", "Websocket client is not implemented");
             return false;
         }
+        if (sock->has_wait) {
+            error_setg(errp, "%s",
+                       "'wait' option is incompatible with "
+                       "socket in client connect mode");
+            return false;
+        }
     }
 
     return true;
@@ -1220,7 +1226,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     sock->tn3270 = is_tn3270;
     sock->has_websocket = true;
     sock->websocket = is_websock;
-    sock->has_wait = true;
+    /*
+     * We have different default to QMP for 'wait' when 'server'
+     * is set, hence we can't just check for existance of 'wait'
+     */
+    sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
     sock->wait = is_waitconnect;
     sock->has_reconnect = qemu_opt_find(opts, "reconnect");
     sock->reconnect = reconnect;
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index fe5eb304b1..faffc1c624 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -293,7 +293,7 @@ static void *server_thread(void *data)
 
 static void setup_vm_with_server(IVState *s, int nvectors, bool msi)
 {
-    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
+    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s "
                                 "-device ivshmem%s,chardev=chr0,vectors=%d",
                                 tmpserver,
                                 msi ? "-doorbell" : ",size=1M,msi=off",
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 55750dd68d..79bcb24b1c 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -232,9 +232,9 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     qtest_add_abrt_handler(kill_qemu_hook_func, s);
 
     command = g_strdup_printf("exec %s "
-                              "-qtest unix:%s,nowait "
+                              "-qtest unix:%s "
                               "-qtest-log %s "
-                              "-chardev socket,path=%s,nowait,id=char0 "
+                              "-chardev socket,path=%s,id=char0 "
                               "-mon chardev=char0,mode=control "
                               "-machine accel=qtest "
                               "-display none "
diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
index 9ca9feabf8..6dc21dd4fb 100644
--- a/tests/test-filter-redirector.c
+++ b/tests/test-filter-redirector.c
@@ -96,7 +96,7 @@ static void test_redirector_tx(void)
         "-device %s,netdev=qtest-bn0,id=qtest-e0 "
         "-chardev socket,id=redirector0,path=%s,server,nowait "
         "-chardev socket,id=redirector1,path=%s,server,nowait "
-        "-chardev socket,id=redirector2,path=%s,nowait "
+        "-chardev socket,id=redirector2,path=%s "
         "-object filter-redirector,id=qtest-f0,netdev=qtest-bn0,"
         "queue=tx,outdev=redirector0 "
         "-object filter-redirector,id=qtest-f1,netdev=qtest-bn0,"
@@ -166,7 +166,7 @@ static void test_redirector_rx(void)
         "-device %s,netdev=qtest-bn0,id=qtest-e0 "
         "-chardev socket,id=redirector0,path=%s,server,nowait "
         "-chardev socket,id=redirector1,path=%s,server,nowait "
-        "-chardev socket,id=redirector2,path=%s,nowait "
+        "-chardev socket,id=redirector2,path=%s "
         "-object filter-redirector,id=qtest-f0,netdev=qtest-bn0,"
         "queue=rx,indev=redirector0 "
         "-object filter-redirector,id=qtest-f1,netdev=qtest-bn0,"
-- 
2.20.1

Re: [Qemu-devel] [PATCH 03/12] chardev: forbid 'wait' option with client sockets

[Marc-André Lureau]

On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The 'wait'/'nowait' parameter is used to tell server sockets whether to
> block until a client is accepted during initialization. Client chardevs
> have always silently ignored this option. Various tests were mistakenly
> passing this option for their client chardevs.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c          | 12 +++++++++++-
>  tests/ivshmem-test.c           |  2 +-
>  tests/libqtest.c               |  4 ++--
>  tests/test-filter-redirector.c |  4 ++--
>  4 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 4570755adf..233f16f72d 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1047,6 +1047,12 @@ static bool qmp_chardev_validate_socket(ChardevSocket *sock,
>              error_setg(errp, "%s", "Websocket client is not implemented");
>              return false;
>          }
> +        if (sock->has_wait) {
> +            error_setg(errp, "%s",
> +                       "'wait' option is incompatible with "
> +                       "socket in client connect mode");
> +            return false;
> +        }
>      }
>
>      return true;
> @@ -1220,7 +1226,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      sock->tn3270 = is_tn3270;
>      sock->has_websocket = true;
>      sock->websocket = is_websock;
> -    sock->has_wait = true;
> +    /*
> +     * We have different default to QMP for 'wait' when 'server'
> +     * is set, hence we can't just check for existance of 'wait'
> +     */
> +    sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
>      sock->wait = is_waitconnect;
>      sock->has_reconnect = qemu_opt_find(opts, "reconnect");
>      sock->reconnect = reconnect;
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index fe5eb304b1..faffc1c624 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -293,7 +293,7 @@ static void *server_thread(void *data)
>
>  static void setup_vm_with_server(IVState *s, int nvectors, bool msi)
>  {
> -    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
> +    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s "
>                                  "-device ivshmem%s,chardev=chr0,vectors=%d",
>                                  tmpserver,
>                                  msi ? "-doorbell" : ",size=1M,msi=off",
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 55750dd68d..79bcb24b1c 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -232,9 +232,9 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
>      qtest_add_abrt_handler(kill_qemu_hook_func, s);
>
>      command = g_strdup_printf("exec %s "
> -                              "-qtest unix:%s,nowait "
> +                              "-qtest unix:%s "
>                                "-qtest-log %s "
> -                              "-chardev socket,path=%s,nowait,id=char0 "
> +                              "-chardev socket,path=%s,id=char0 "
>                                "-mon chardev=char0,mode=control "
>                                "-machine accel=qtest "
>                                "-display none "
> diff --git a/tests/test-filter-redirector.c b/tests/test-filter-redirector.c
> index 9ca9feabf8..6dc21dd4fb 100644
> --- a/tests/test-filter-redirector.c
> +++ b/tests/test-filter-redirector.c
> @@ -96,7 +96,7 @@ static void test_redirector_tx(void)
>          "-device %s,netdev=qtest-bn0,id=qtest-e0 "
>          "-chardev socket,id=redirector0,path=%s,server,nowait "
>          "-chardev socket,id=redirector1,path=%s,server,nowait "
> -        "-chardev socket,id=redirector2,path=%s,nowait "
> +        "-chardev socket,id=redirector2,path=%s "
>          "-object filter-redirector,id=qtest-f0,netdev=qtest-bn0,"
>          "queue=tx,outdev=redirector0 "
>          "-object filter-redirector,id=qtest-f1,netdev=qtest-bn0,"
> @@ -166,7 +166,7 @@ static void test_redirector_rx(void)
>          "-device %s,netdev=qtest-bn0,id=qtest-e0 "
>          "-chardev socket,id=redirector0,path=%s,server,nowait "
>          "-chardev socket,id=redirector1,path=%s,server,nowait "
> -        "-chardev socket,id=redirector2,path=%s,nowait "
> +        "-chardev socket,id=redirector2,path=%s "
>          "-object filter-redirector,id=qtest-f0,netdev=qtest-bn0,"
>          "queue=rx,indev=redirector0 "
>          "-object filter-redirector,id=qtest-f1,netdev=qtest-bn0,"
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH 03/12] chardev: forbid 'wait' option with client sockets

[Thomas Huth]

On 2019-01-15 15:52, Daniel P. Berrangé wrote:
> The 'wait'/'nowait' parameter is used to tell server sockets whether to
> block until a client is accepted during initialization. Client chardevs
> have always silently ignored this option. Various tests were mistakenly
> passing this option for their client chardevs.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-socket.c          | 12 +++++++++++-
>  tests/ivshmem-test.c           |  2 +-
>  tests/libqtest.c               |  4 ++--
>  tests/test-filter-redirector.c |  4 ++--
>  4 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 4570755adf..233f16f72d 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1047,6 +1047,12 @@ static bool qmp_chardev_validate_socket(ChardevSocket *sock,
>              error_setg(errp, "%s", "Websocket client is not implemented");
>              return false;
>          }
> +        if (sock->has_wait) {
> +            error_setg(errp, "%s",
> +                       "'wait' option is incompatible with "
> +                       "socket in client connect mode");
> +            return false;
> +        }
>      }
>  
>      return true;
> @@ -1220,7 +1226,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      sock->tn3270 = is_tn3270;
>      sock->has_websocket = true;
>      sock->websocket = is_websock;
> -    sock->has_wait = true;
> +    /*
> +     * We have different default to QMP for 'wait' when 'server'
> +     * is set, hence we can't just check for existance of 'wait'
> +     */
> +    sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
>      sock->wait = is_waitconnect;
>      sock->has_reconnect = qemu_opt_find(opts, "reconnect");
>      sock->reconnect = reconnect;
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index fe5eb304b1..faffc1c624 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -293,7 +293,7 @@ static void *server_thread(void *data)
>  
>  static void setup_vm_with_server(IVState *s, int nvectors, bool msi)
>  {
> -    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
> +    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s "
>                                  "-device ivshmem%s,chardev=chr0,vectors=%d",
>                                  tmpserver,
>                                  msi ? "-doorbell" : ",size=1M,msi=off",

I think there will be a conflict with the patch 'Remove deprecated
"ivshmem" legacy device' that is currently in Michael's PULL request ...
could you please rebase this patch in a day or two once that PULL has
been merged?

Apart from that, patch looks fine to me.

Acked-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH 04/12] chardev: remove many local variables in qemu_chr_parse_socket

[Daniel P. Berrangé]

Now that all validation is separated off into a separate method,
we can directly populate the ChardevSocket struct from the
QemuOpts values, avoiding many local variables.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 233f16f72d..ba86284ea9 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1186,18 +1186,10 @@ error:
 static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
                                   Error **errp)
 {
-    bool is_listen      = qemu_opt_get_bool(opts, "server", false);
-    bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
-    bool is_telnet      = qemu_opt_get_bool(opts, "telnet", false);
-    bool is_tn3270      = qemu_opt_get_bool(opts, "tn3270", false);
-    bool is_websock     = qemu_opt_get_bool(opts, "websocket", false);
-    bool do_nodelay     = !qemu_opt_get_bool(opts, "delay", true);
-    int64_t reconnect   = qemu_opt_get_number(opts, "reconnect", 0);
     const char *path = qemu_opt_get(opts, "path");
     const char *host = qemu_opt_get(opts, "host");
     const char *port = qemu_opt_get(opts, "port");
     const char *fd = qemu_opt_get(opts, "fd");
-    const char *tls_creds = qemu_opt_get(opts, "tls-creds");
     SocketAddressLegacy *addr;
     ChardevSocket *sock;
 
@@ -1216,26 +1208,30 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
     sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
     qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
 
-    sock->has_nodelay = true;
-    sock->nodelay = do_nodelay;
+    sock->has_nodelay = qemu_opt_get(opts, "delay");
+    sock->nodelay = !qemu_opt_get_bool(opts, "delay", true);
+    /*
+     * We have different default to QMP for 'server', hence
+     * we can't just check for existance of 'server'
+     */
     sock->has_server = true;
-    sock->server = is_listen;
-    sock->has_telnet = true;
-    sock->telnet = is_telnet;
-    sock->has_tn3270 = true;
-    sock->tn3270 = is_tn3270;
-    sock->has_websocket = true;
-    sock->websocket = is_websock;
+    sock->server = qemu_opt_get_bool(opts, "server", false);
+    sock->has_telnet = qemu_opt_get(opts, "telnet");
+    sock->telnet = qemu_opt_get_bool(opts, "telnet", false);
+    sock->has_tn3270 = qemu_opt_get(opts, "tn3270");
+    sock->tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
+    sock->has_websocket = qemu_opt_get(opts, "websocket");
+    sock->websocket = qemu_opt_get_bool(opts, "websocket", false);
     /*
      * We have different default to QMP for 'wait' when 'server'
      * is set, hence we can't just check for existance of 'wait'
      */
-    sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
-    sock->wait = is_waitconnect;
+    sock->has_wait = qemu_opt_find(opts, "wait") || sock->server;
+    sock->wait = qemu_opt_get_bool(opts, "wait", true);
     sock->has_reconnect = qemu_opt_find(opts, "reconnect");
-    sock->reconnect = reconnect;
-    sock->has_tls_creds = tls_creds;
-    sock->tls_creds = g_strdup(tls_creds);
+    sock->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
+    sock->has_tls_creds = qemu_opt_get(opts, "tls-creds");
+    sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
 
     addr = g_new0(SocketAddressLegacy, 1);
     if (path) {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 04/12] chardev: remove many local variables in qemu_chr_parse_socket

[Marc-André Lureau]

Hi

On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> Now that all validation is separated off into a separate method,
> we can directly populate the ChardevSocket struct from the
> QemuOpts values, avoiding many local variables.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 233f16f72d..ba86284ea9 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1186,18 +1186,10 @@ error:
>  static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>                                    Error **errp)
>  {
> -    bool is_listen      = qemu_opt_get_bool(opts, "server", false);
> -    bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
> -    bool is_telnet      = qemu_opt_get_bool(opts, "telnet", false);
> -    bool is_tn3270      = qemu_opt_get_bool(opts, "tn3270", false);
> -    bool is_websock     = qemu_opt_get_bool(opts, "websocket", false);
> -    bool do_nodelay     = !qemu_opt_get_bool(opts, "delay", true);
> -    int64_t reconnect   = qemu_opt_get_number(opts, "reconnect", 0);
>      const char *path = qemu_opt_get(opts, "path");
>      const char *host = qemu_opt_get(opts, "host");
>      const char *port = qemu_opt_get(opts, "port");
>      const char *fd = qemu_opt_get(opts, "fd");
> -    const char *tls_creds = qemu_opt_get(opts, "tls-creds");
>      SocketAddressLegacy *addr;
>      ChardevSocket *sock;
>
> @@ -1216,26 +1208,30 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
>      qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
>
> -    sock->has_nodelay = true;
> -    sock->nodelay = do_nodelay;
> +    sock->has_nodelay = qemu_opt_get(opts, "delay");
> +    sock->nodelay = !qemu_opt_get_bool(opts, "delay", true);
> +    /*
> +     * We have different default to QMP for 'server', hence
> +     * we can't just check for existance of 'server'
> +     */
>      sock->has_server = true;
> -    sock->server = is_listen;
> -    sock->has_telnet = true;
> -    sock->telnet = is_telnet;
> -    sock->has_tn3270 = true;
> -    sock->tn3270 = is_tn3270;
> -    sock->has_websocket = true;
> -    sock->websocket = is_websock;
> +    sock->server = qemu_opt_get_bool(opts, "server", false);
> +    sock->has_telnet = qemu_opt_get(opts, "telnet");
> +    sock->telnet = qemu_opt_get_bool(opts, "telnet", false);
> +    sock->has_tn3270 = qemu_opt_get(opts, "tn3270");
> +    sock->tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
> +    sock->has_websocket = qemu_opt_get(opts, "websocket");
> +    sock->websocket = qemu_opt_get_bool(opts, "websocket", false);
>      /*
>       * We have different default to QMP for 'wait' when 'server'
>       * is set, hence we can't just check for existance of 'wait'
>       */
> -    sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
> -    sock->wait = is_waitconnect;
> +    sock->has_wait = qemu_opt_find(opts, "wait") || sock->server;
> +    sock->wait = qemu_opt_get_bool(opts, "wait", true);

That changes slightly the behaviour, since wait was not parsed before
when !is_listen.

Since we are "correcting" (breaking?) things anyway in previous patches too:

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>      sock->has_reconnect = qemu_opt_find(opts, "reconnect");
> -    sock->reconnect = reconnect;
> -    sock->has_tls_creds = tls_creds;
> -    sock->tls_creds = g_strdup(tls_creds);
> +    sock->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
> +    sock->has_tls_creds = qemu_opt_get(opts, "tls-creds");
> +    sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
>
>      addr = g_new0(SocketAddressLegacy, 1);
>      if (path) {
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH 04/12] chardev: remove many local variables in qemu_chr_parse_socket

[Daniel P. Berrangé]

On Tue, Jan 15, 2019 at 11:18:21PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > Now that all validation is separated off into a separate method,
> > we can directly populate the ChardevSocket struct from the
> > QemuOpts values, avoiding many local variables.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  chardev/char-socket.c | 40 ++++++++++++++++++----------------------
> >  1 file changed, 18 insertions(+), 22 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 233f16f72d..ba86284ea9 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -1186,18 +1186,10 @@ error:
> >  static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> >                                    Error **errp)
> >  {
> > -    bool is_listen      = qemu_opt_get_bool(opts, "server", false);
> > -    bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
> > -    bool is_telnet      = qemu_opt_get_bool(opts, "telnet", false);
> > -    bool is_tn3270      = qemu_opt_get_bool(opts, "tn3270", false);
> > -    bool is_websock     = qemu_opt_get_bool(opts, "websocket", false);
> > -    bool do_nodelay     = !qemu_opt_get_bool(opts, "delay", true);
> > -    int64_t reconnect   = qemu_opt_get_number(opts, "reconnect", 0);
> >      const char *path = qemu_opt_get(opts, "path");
> >      const char *host = qemu_opt_get(opts, "host");
> >      const char *port = qemu_opt_get(opts, "port");
> >      const char *fd = qemu_opt_get(opts, "fd");
> > -    const char *tls_creds = qemu_opt_get(opts, "tls-creds");
> >      SocketAddressLegacy *addr;
> >      ChardevSocket *sock;
> >
> > @@ -1216,26 +1208,30 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> >      sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
> >      qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
> >
> > -    sock->has_nodelay = true;
> > -    sock->nodelay = do_nodelay;
> > +    sock->has_nodelay = qemu_opt_get(opts, "delay");
> > +    sock->nodelay = !qemu_opt_get_bool(opts, "delay", true);
> > +    /*
> > +     * We have different default to QMP for 'server', hence
> > +     * we can't just check for existance of 'server'
> > +     */
> >      sock->has_server = true;
> > -    sock->server = is_listen;
> > -    sock->has_telnet = true;
> > -    sock->telnet = is_telnet;
> > -    sock->has_tn3270 = true;
> > -    sock->tn3270 = is_tn3270;
> > -    sock->has_websocket = true;
> > -    sock->websocket = is_websock;
> > +    sock->server = qemu_opt_get_bool(opts, "server", false);
> > +    sock->has_telnet = qemu_opt_get(opts, "telnet");
> > +    sock->telnet = qemu_opt_get_bool(opts, "telnet", false);
> > +    sock->has_tn3270 = qemu_opt_get(opts, "tn3270");
> > +    sock->tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
> > +    sock->has_websocket = qemu_opt_get(opts, "websocket");
> > +    sock->websocket = qemu_opt_get_bool(opts, "websocket", false);
> >      /*
> >       * We have different default to QMP for 'wait' when 'server'
> >       * is set, hence we can't just check for existance of 'wait'
> >       */
> > -    sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
> > -    sock->wait = is_waitconnect;
> > +    sock->has_wait = qemu_opt_find(opts, "wait") || sock->server;
> > +    sock->wait = qemu_opt_get_bool(opts, "wait", true);
> 
> That changes slightly the behaviour, since wait was not parsed before
> when !is_listen.
>
> Since we are "correcting" (breaking?) things anyway in previous patches too:

Yes, in the !is_listen case, the previous patch will see has_wait == true
and raise an error.  So this patch is not a change in behaviour wrt the
previous patch, but the series as a whole is a change in behaviour due to
reporting errors for bad "wait" usage in clients.


> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> >      sock->has_reconnect = qemu_opt_find(opts, "reconnect");
> > -    sock->reconnect = reconnect;
> > -    sock->has_tls_creds = tls_creds;
> > -    sock->tls_creds = g_strdup(tls_creds);
> > +    sock->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
> > +    sock->has_tls_creds = qemu_opt_get(opts, "tls-creds");
> > +    sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
> >
> >      addr = g_new0(SocketAddressLegacy, 1);
> >      if (path) {

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 04/12] chardev: remove many local variables in qemu_chr_parse_socket

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3111 bytes --]

[adding Markus in relation to a QemuOpts observation]

On 1/15/19 8:52 AM, Daniel P. Berrangé wrote:
> Now that all validation is separated off into a separate method,
> we can directly populate the ChardevSocket struct from the
> QemuOpts values, avoiding many local variables.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
> 

> @@ -1216,26 +1208,30 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
>      qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
>  
> -    sock->has_nodelay = true;
> -    sock->nodelay = do_nodelay;
> +    sock->has_nodelay = qemu_opt_get(opts, "delay");
> +    sock->nodelay = !qemu_opt_get_bool(opts, "delay", true);

Unrelated to this patch, but my recent proposal to make QemuOpts be a
bit more typesafe:

https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg02042.html

does not like users calling qemu_opt_get() of an option that has an
integer default (as opposed to a string default), even if the only
reason the caller was doing it was to learn if the option is present.  I
wonder if we should introduce

bool qemu_opt_has(QemuOpts *opts, const char *name)

and then replace existing callers which merely call qemu_opt_get() to
learn whether an optional option was present/defaulted but without
caring about its string value.  I also wonder if Coccinelle can be
employed to determine which callers fit that bill (that is, detect when
a const char * result is ignored or solely used in a bool context, vs.
when it is actually used as a string parameter to another function
and/or saved in a const char * variable).

Now, on to actual review,

> +    /*
> +     * We have different default to QMP for 'server', hence
> +     * we can't just check for existance of 'server'

s/existance/existence/

> +     */
>      sock->has_server = true;
> -    sock->server = is_listen;
> -    sock->has_telnet = true;
> -    sock->telnet = is_telnet;
> -    sock->has_tn3270 = true;
> -    sock->tn3270 = is_tn3270;
> -    sock->has_websocket = true;
> -    sock->websocket = is_websock;
> +    sock->server = qemu_opt_get_bool(opts, "server", false);
> +    sock->has_telnet = qemu_opt_get(opts, "telnet");
> +    sock->telnet = qemu_opt_get_bool(opts, "telnet", false);
> +    sock->has_tn3270 = qemu_opt_get(opts, "tn3270");
> +    sock->tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
> +    sock->has_websocket = qemu_opt_get(opts, "websocket");
> +    sock->websocket = qemu_opt_get_bool(opts, "websocket", false);
>      /*
>       * We have different default to QMP for 'wait' when 'server'
>       * is set, hence we can't just check for existance of 'wait'

but then again, you were copy-pasting an existing typo.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 04/12] chardev: remove many local variables in qemu_chr_parse_socket

[Daniel P. Berrangé]

On Tue, Jan 15, 2019 at 01:33:25PM -0600, Eric Blake wrote:
> [adding Markus in relation to a QemuOpts observation]
> 
> On 1/15/19 8:52 AM, Daniel P. Berrangé wrote:
> > Now that all validation is separated off into a separate method,
> > we can directly populate the ChardevSocket struct from the
> > QemuOpts values, avoiding many local variables.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  chardev/char-socket.c | 40 ++++++++++++++++++----------------------
> >  1 file changed, 18 insertions(+), 22 deletions(-)
> > 
> 
> > @@ -1216,26 +1208,30 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> >      sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
> >      qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
> >  
> > -    sock->has_nodelay = true;
> > -    sock->nodelay = do_nodelay;
> > +    sock->has_nodelay = qemu_opt_get(opts, "delay");
> > +    sock->nodelay = !qemu_opt_get_bool(opts, "delay", true);
> 
> Unrelated to this patch, but my recent proposal to make QemuOpts be a
> bit more typesafe:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg02042.html
> 
> does not like users calling qemu_opt_get() of an option that has an
> integer default (as opposed to a string default), even if the only
> reason the caller was doing it was to learn if the option is present.  I
> wonder if we should introduce
> 
> bool qemu_opt_has(QemuOpts *opts, const char *name)
> 
> and then replace existing callers which merely call qemu_opt_get() to
> learn whether an optional option was present/defaulted but without
> caring about its string value.  I also wonder if Coccinelle can be
> employed to determine which callers fit that bill (that is, detect when
> a const char * result is ignored or solely used in a bool context, vs.
> when it is actually used as a string parameter to another function
> and/or saved in a const char * variable).

Yes, if you're going to enforce type safety on qemu_opt_get, then
then we definitely need to have  qemu_opt_has()  function to check
for existance, as this is needed in other places too.


> 
> Now, on to actual review,
> 
> > +    /*
> > +     * We have different default to QMP for 'server', hence
> > +     * we can't just check for existance of 'server'
> 
> s/existance/existence/
> 
> > +     */
> >      sock->has_server = true;
> > -    sock->server = is_listen;
> > -    sock->has_telnet = true;
> > -    sock->telnet = is_telnet;
> > -    sock->has_tn3270 = true;
> > -    sock->tn3270 = is_tn3270;
> > -    sock->has_websocket = true;
> > -    sock->websocket = is_websock;
> > +    sock->server = qemu_opt_get_bool(opts, "server", false);
> > +    sock->has_telnet = qemu_opt_get(opts, "telnet");
> > +    sock->telnet = qemu_opt_get_bool(opts, "telnet", false);
> > +    sock->has_tn3270 = qemu_opt_get(opts, "tn3270");
> > +    sock->tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
> > +    sock->has_websocket = qemu_opt_get(opts, "websocket");
> > +    sock->websocket = qemu_opt_get_bool(opts, "websocket", false);
> >      /*
> >       * We have different default to QMP for 'wait' when 'server'
> >       * is set, hence we can't just check for existance of 'wait'
> 
> but then again, you were copy-pasting an existing typo.

Both typos were from this series


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

[Qemu-devel] [PATCH 05/12] chardev: ensure qemu_chr_parse_compat reports missing driver error

[Daniel P. Berrangé]

If no valid char driver was identified the qemu_chr_parse_compat method
was silent, leaving callers no clue what failed.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/chardev/char.c b/chardev/char.c
index ccba36bafb..b99f3692f7 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -490,6 +490,8 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
         return opts;
     }
 
+    error_report("'%s' is not a valid char driver", filename);
+
 fail:
     qemu_opts_del(opts);
     return NULL;
-- 
2.20.1

Re: [Qemu-devel] [PATCH 05/12] chardev: ensure qemu_chr_parse_compat reports missing driver error

[Marc-André Lureau]

Hi

On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> If no valid char driver was identified the qemu_chr_parse_compat method
> was silent, leaving callers no clue what failed.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index ccba36bafb..b99f3692f7 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -490,6 +490,8 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename,
>          return opts;
>      }
>
> +    error_report("'%s' is not a valid char driver", filename);
> +
>  fail:
>      qemu_opts_del(opts);
>      return NULL;
> --
> 2.20.1
>

[Qemu-devel] [PATCH 06/12] chardev: remove unused 'sioc' variable & cleanup paths

[Daniel P. Berrangé]

The 'sioc' variable in qmp_chardev_open_socket was unused since

  commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24
  Author: Peter Xu <peterx@redhat.com>
  Date:   Tue Mar 6 13:33:17 2018 +0800

    chardev: use chardev's gcontext for async connect

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index ba86284ea9..3b6ff6619b 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1073,7 +1073,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
     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;
-    QIOChannelSocket *sioc = NULL;
     SocketAddress *addr;
 
     s->is_listen = is_listen;
@@ -1088,7 +1087,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
         if (!creds) {
             error_setg(errp, "No TLS credentials with id '%s'",
                        sock->tls_creds);
-            goto error;
+            return;
         }
         s->tls_creds = (QCryptoTLSCreds *)
             object_dynamic_cast(creds,
@@ -1096,20 +1095,20 @@ static void qmp_chardev_open_socket(Chardev *chr,
         if (!s->tls_creds) {
             error_setg(errp, "Object with id '%s' is not TLS credentials",
                        sock->tls_creds);
-            goto error;
+            return;
         }
         object_ref(OBJECT(s->tls_creds));
         if (is_listen) {
             if (s->tls_creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
                 error_setg(errp, "%s",
                            "Expected TLS credentials for server endpoint");
-                goto error;
+                return;
             }
         } else {
             if (s->tls_creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) {
                 error_setg(errp, "%s",
                            "Expected TLS credentials for client endpoint");
-                goto error;
+                return;
             }
         }
     }
@@ -1117,7 +1116,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
     s->addr = addr = socket_address_flatten(sock->addr);
 
     if (!qmp_chardev_validate_socket(sock, addr, errp)) {
-        goto error;
+        return;
     }
 
     qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
@@ -1153,7 +1152,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
             if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
                 object_unref(OBJECT(s->listener));
                 s->listener = NULL;
-                goto error;
+                return;
             }
 
             qapi_free_SocketAddress(s->addr);
@@ -1171,16 +1170,9 @@ static void qmp_chardev_open_socket(Chardev *chr,
                                                       chr->gcontext);
             }
         } else if (qemu_chr_wait_connected(chr, errp) < 0) {
-            goto error;
+            return;
         }
     }
-
-    return;
-
-error:
-    if (sioc) {
-        object_unref(OBJECT(sioc));
-    }
 }
 
 static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
-- 
2.20.1

Re: [Qemu-devel] [PATCH 06/12] chardev: remove unused 'sioc' variable & cleanup paths

[Marc-André Lureau]

On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The 'sioc' variable in qmp_chardev_open_socket was unused since
>
>   commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24
>   Author: Peter Xu <peterx@redhat.com>
>   Date:   Tue Mar 6 13:33:17 2018 +0800
>
>     chardev: use chardev's gcontext for async connect
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index ba86284ea9..3b6ff6619b 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1073,7 +1073,6 @@ static void qmp_chardev_open_socket(Chardev *chr,
>      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;
> -    QIOChannelSocket *sioc = NULL;
>      SocketAddress *addr;
>
>      s->is_listen = is_listen;
> @@ -1088,7 +1087,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
>          if (!creds) {
>              error_setg(errp, "No TLS credentials with id '%s'",
>                         sock->tls_creds);
> -            goto error;
> +            return;
>          }
>          s->tls_creds = (QCryptoTLSCreds *)
>              object_dynamic_cast(creds,
> @@ -1096,20 +1095,20 @@ static void qmp_chardev_open_socket(Chardev *chr,
>          if (!s->tls_creds) {
>              error_setg(errp, "Object with id '%s' is not TLS credentials",
>                         sock->tls_creds);
> -            goto error;
> +            return;
>          }
>          object_ref(OBJECT(s->tls_creds));
>          if (is_listen) {
>              if (s->tls_creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
>                  error_setg(errp, "%s",
>                             "Expected TLS credentials for server endpoint");
> -                goto error;
> +                return;
>              }
>          } else {
>              if (s->tls_creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) {
>                  error_setg(errp, "%s",
>                             "Expected TLS credentials for client endpoint");
> -                goto error;
> +                return;
>              }
>          }
>      }
> @@ -1117,7 +1116,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
>      s->addr = addr = socket_address_flatten(sock->addr);
>
>      if (!qmp_chardev_validate_socket(sock, addr, errp)) {
> -        goto error;
> +        return;
>      }
>
>      qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
> @@ -1153,7 +1152,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
>              if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
>                  object_unref(OBJECT(s->listener));
>                  s->listener = NULL;
> -                goto error;
> +                return;
>              }
>
>              qapi_free_SocketAddress(s->addr);
> @@ -1171,16 +1170,9 @@ static void qmp_chardev_open_socket(Chardev *chr,
>                                                        chr->gcontext);
>              }
>          } else if (qemu_chr_wait_connected(chr, errp) < 0) {
> -            goto error;
> +            return;
>          }
>      }
> -
> -    return;
> -
> -error:
> -    if (sioc) {
> -        object_unref(OBJECT(sioc));
> -    }
>  }
>
>  static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH 06/12] chardev: remove unused 'sioc' variable & cleanup paths

[Thomas Huth]

On 2019-01-15 15:52, Daniel P. Berrangé wrote:
> The 'sioc' variable in qmp_chardev_open_socket was unused since
> 
>   commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24
>   Author: Peter Xu <peterx@redhat.com>
>   Date:   Tue Mar 6 13:33:17 2018 +0800
> 
>     chardev: use chardev's gcontext for async connect
[...]
> -error:
> -    if (sioc) {
> -        object_unref(OBJECT(sioc));
> -    }

So who is doing the object_unref() now in case of errors? That commit
did not take care of it ... so it sounds like we could be leaving
references behind in case of errors here?

 Thomas

Re: [Qemu-devel] [PATCH 06/12] chardev: remove unused 'sioc' variable & cleanup paths

[Peter Xu]

On Wed, Jan 16, 2019 at 06:24:49AM +0100, Thomas Huth wrote:
> On 2019-01-15 15:52, Daniel P. Berrangé wrote:
> > The 'sioc' variable in qmp_chardev_open_socket was unused since
> > 
> >   commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24
> >   Author: Peter Xu <peterx@redhat.com>
> >   Date:   Tue Mar 6 13:33:17 2018 +0800
> > 
> >     chardev: use chardev's gcontext for async connect
> [...]
> > -error:
> > -    if (sioc) {
> > -        object_unref(OBJECT(sioc));
> > -    }
> 
> So who is doing the object_unref() now in case of errors? That commit
> did not take care of it ... so it sounds like we could be leaving
> references behind in case of errors here?

IMHO it'll be done finally in qemu_chr_socket_connected() no matter
whether it's succeeded or not:

    cleanup:
        object_unref(OBJECT(sioc));

In other words, I think the old error path is not valid even before
commit 3e7d4d20d3 because IIUC when reaching the error label the sioc
should never be set (and if it tries to do an object_unref here it
would be a real bug).

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 06/12] chardev: remove unused 'sioc' variable & cleanup paths

[Thomas Huth]

On 2019-01-16 06:47, Peter Xu wrote:
> On Wed, Jan 16, 2019 at 06:24:49AM +0100, Thomas Huth wrote:
>> On 2019-01-15 15:52, Daniel P. Berrangé wrote:
>>> The 'sioc' variable in qmp_chardev_open_socket was unused since
>>>
>>>   commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24
>>>   Author: Peter Xu <peterx@redhat.com>
>>>   Date:   Tue Mar 6 13:33:17 2018 +0800
>>>
>>>     chardev: use chardev's gcontext for async connect
>> [...]
>>> -error:
>>> -    if (sioc) {
>>> -        object_unref(OBJECT(sioc));
>>> -    }
>>
>> So who is doing the object_unref() now in case of errors? That commit
>> did not take care of it ... so it sounds like we could be leaving
>> references behind in case of errors here?
> 
> IMHO it'll be done finally in qemu_chr_socket_connected() no matter
> whether it's succeeded or not:
> 
>     cleanup:
>         object_unref(OBJECT(sioc));
> 
> In other words, I think the old error path is not valid even before
> commit 3e7d4d20d3 because IIUC when reaching the error label the sioc
> should never be set (and if it tries to do an object_unref here it
> would be a real bug).

Right, looking at the qmp_chardev_open_socket() function again (before
the rework in 3e7d4d20d3a5), I see now that all locations that use "goto
error" should have sioc = NULL. So this patch here is fine:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 06/12] chardev: remove unused 'sioc' variable & cleanup paths

[Daniel P. Berrangé]

On Wed, Jan 16, 2019 at 06:24:49AM +0100, Thomas Huth wrote:
> On 2019-01-15 15:52, Daniel P. Berrangé wrote:
> > The 'sioc' variable in qmp_chardev_open_socket was unused since
> > 
> >   commit 3e7d4d20d3a528b1ed10b1dc3d83119bfb0c5f24
> >   Author: Peter Xu <peterx@redhat.com>
> >   Date:   Tue Mar 6 13:33:17 2018 +0800
> > 
> >     chardev: use chardev's gcontext for async connect
> [...]
> > -error:
> > -    if (sioc) {
> > -        object_unref(OBJECT(sioc));
> > -    }
> 
> So who is doing the object_unref() now in case of errors? That commit
> did not take care of it ... so it sounds like we could be leaving
> references behind in case of errors here?

As shown in the patch diff chunks, no code is ever setting "sioc" in
this method, so it can't ever be non-NULL

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

[Qemu-devel] [PATCH 07/12] chardev: split tcp_chr_wait_connected into two methods

[Daniel P. Berrangé]

The tcp_chr_wait_connected method can deal with either server or client
chardevs, but some callers only care about one of these possibilities.
The tcp_chr_wait_connected method will also need some refactoring to
reliably deal with its primary goal of allowing a device frontend to
wait for an established connection, which will interfere with other
callers.

Split it into two methods, one responsible for server initiated
connections, the other responsible for client initiated connections.
In doing this split the tcp_char_connect_async() method is renamed
to become consistent with naming of the new methods.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 59 +++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 3b6ff6619b..3bd1be7631 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -886,30 +886,47 @@ static void tcp_chr_accept(QIONetListener *listener,
     tcp_chr_new_client(chr, cioc);
 }
 
-static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
+
+static int tcp_chr_connect_client_sync(Chardev *chr, Error **errp)
+{
+    SocketChardev *s = SOCKET_CHARDEV(chr);
+    QIOChannelSocket *sioc = qio_channel_socket_new();
+    tcp_chr_set_client_ioc_name(chr, sioc);
+    if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
+        object_unref(OBJECT(sioc));
+        return -1;
+    }
+    tcp_chr_new_client(chr, sioc);
+    object_unref(OBJECT(sioc));
+    return 0;
+}
+
+
+static void tcp_chr_accept_server_sync(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
     QIOChannelSocket *sioc;
+    info_report("QEMU waiting for connection on: %s",
+                chr->filename);
+    sioc = qio_net_listener_wait_client(s->listener);
+    tcp_chr_set_client_ioc_name(chr, sioc);
+    tcp_chr_new_client(chr, sioc);
+    object_unref(OBJECT(sioc));
+}
+
 
+static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
+{
+    SocketChardev *s = SOCKET_CHARDEV(chr);
     /* It can't wait on s->connected, since it is set asynchronously
      * in TLS and telnet cases, only wait for an accepted socket */
     while (!s->ioc) {
         if (s->is_listen) {
-            info_report("QEMU waiting for connection on: %s",
-                        chr->filename);
-            sioc = qio_net_listener_wait_client(s->listener);
-            tcp_chr_set_client_ioc_name(chr, sioc);
-            tcp_chr_new_client(chr, sioc);
-            object_unref(OBJECT(sioc));
+            tcp_chr_accept_server_sync(chr);
         } else {
-            sioc = qio_channel_socket_new();
-            tcp_chr_set_client_ioc_name(chr, sioc);
-            if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
-                object_unref(OBJECT(sioc));
+            if (tcp_chr_connect_client_sync(chr, errp) < 0) {
                 return -1;
             }
-            tcp_chr_new_client(chr, sioc);
-            object_unref(OBJECT(sioc));
         }
     }
 
@@ -958,7 +975,7 @@ cleanup:
     object_unref(OBJECT(sioc));
 }
 
-static void tcp_chr_connect_async(Chardev *chr)
+static void tcp_chr_connect_client_async(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
     QIOChannelSocket *sioc;
@@ -982,7 +999,7 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
         return false;
     }
 
-    tcp_chr_connect_async(chr);
+    tcp_chr_connect_client_async(chr);
 
     return false;
 }
@@ -1139,7 +1156,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
     }
 
     if (s->reconnect_time) {
-        tcp_chr_connect_async(chr);
+        tcp_chr_connect_client_async(chr);
     } else {
         if (s->is_listen) {
             char *name;
@@ -1159,17 +1176,15 @@ static void qmp_chardev_open_socket(Chardev *chr,
             s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
             update_disconnected_filename(s);
 
-            if (is_waitconnect &&
-                qemu_chr_wait_connected(chr, errp) < 0) {
-                return;
-            }
-            if (!s->ioc) {
+            if (is_waitconnect) {
+                tcp_chr_accept_server_sync(chr);
+            } else {
                 qio_net_listener_set_client_func_full(s->listener,
                                                       tcp_chr_accept,
                                                       chr, NULL,
                                                       chr->gcontext);
             }
-        } else if (qemu_chr_wait_connected(chr, errp) < 0) {
+        } else if (tcp_chr_connect_client_sync(chr, errp) < 0) {
             return;
         }
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 07/12] chardev: split tcp_chr_wait_connected into two methods

[Marc-André Lureau]

Hi

On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The tcp_chr_wait_connected method can deal with either server or client
> chardevs, but some callers only care about one of these possibilities.
> The tcp_chr_wait_connected method will also need some refactoring to
> reliably deal with its primary goal of allowing a device frontend to
> wait for an established connection, which will interfere with other
> callers.
>
> Split it into two methods, one responsible for server initiated
> connections, the other responsible for client initiated connections.
> In doing this split the tcp_char_connect_async() method is renamed
> to become consistent with naming of the new methods.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 59 +++++++++++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 3b6ff6619b..3bd1be7631 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -886,30 +886,47 @@ static void tcp_chr_accept(QIONetListener *listener,
>      tcp_chr_new_client(chr, cioc);
>  }
>
> -static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
> +
> +static int tcp_chr_connect_client_sync(Chardev *chr, Error **errp)
> +{
> +    SocketChardev *s = SOCKET_CHARDEV(chr);
> +    QIOChannelSocket *sioc = qio_channel_socket_new();
> +    tcp_chr_set_client_ioc_name(chr, sioc);
> +    if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
> +        object_unref(OBJECT(sioc));
> +        return -1;
> +    }
> +    tcp_chr_new_client(chr, sioc);
> +    object_unref(OBJECT(sioc));
> +    return 0;
> +}
> +
> +
> +static void tcp_chr_accept_server_sync(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>      QIOChannelSocket *sioc;
> +    info_report("QEMU waiting for connection on: %s",
> +                chr->filename);
> +    sioc = qio_net_listener_wait_client(s->listener);
> +    tcp_chr_set_client_ioc_name(chr, sioc);
> +    tcp_chr_new_client(chr, sioc);
> +    object_unref(OBJECT(sioc));
> +}
> +
>
> +static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
> +{
> +    SocketChardev *s = SOCKET_CHARDEV(chr);
>      /* It can't wait on s->connected, since it is set asynchronously
>       * in TLS and telnet cases, only wait for an accepted socket */
>      while (!s->ioc) {
>          if (s->is_listen) {
> -            info_report("QEMU waiting for connection on: %s",
> -                        chr->filename);
> -            sioc = qio_net_listener_wait_client(s->listener);
> -            tcp_chr_set_client_ioc_name(chr, sioc);
> -            tcp_chr_new_client(chr, sioc);
> -            object_unref(OBJECT(sioc));
> +            tcp_chr_accept_server_sync(chr);
>          } else {
> -            sioc = qio_channel_socket_new();
> -            tcp_chr_set_client_ioc_name(chr, sioc);
> -            if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
> -                object_unref(OBJECT(sioc));
> +            if (tcp_chr_connect_client_sync(chr, errp) < 0) {
>                  return -1;
>              }
> -            tcp_chr_new_client(chr, sioc);
> -            object_unref(OBJECT(sioc));
>          }
>      }
>
> @@ -958,7 +975,7 @@ cleanup:
>      object_unref(OBJECT(sioc));
>  }
>
> -static void tcp_chr_connect_async(Chardev *chr)
> +static void tcp_chr_connect_client_async(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>      QIOChannelSocket *sioc;
> @@ -982,7 +999,7 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
>          return false;
>      }
>
> -    tcp_chr_connect_async(chr);
> +    tcp_chr_connect_client_async(chr);
>
>      return false;
>  }
> @@ -1139,7 +1156,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
>      }
>
>      if (s->reconnect_time) {
> -        tcp_chr_connect_async(chr);
> +        tcp_chr_connect_client_async(chr);
>      } else {
>          if (s->is_listen) {
>              char *name;
> @@ -1159,17 +1176,15 @@ static void qmp_chardev_open_socket(Chardev *chr,
>              s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
>              update_disconnected_filename(s);
>
> -            if (is_waitconnect &&
> -                qemu_chr_wait_connected(chr, errp) < 0) {
> -                return;
> -            }
> -            if (!s->ioc) {
> +            if (is_waitconnect) {
> +                tcp_chr_accept_server_sync(chr);

Is the wait_connected() loop really unnecessary there? looks like it
is. And there is no error path either. ok

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> +            } else {
>                  qio_net_listener_set_client_func_full(s->listener,
>                                                        tcp_chr_accept,
>                                                        chr, NULL,
>                                                        chr->gcontext);
>              }
> -        } else if (qemu_chr_wait_connected(chr, errp) < 0) {
> +        } else if (tcp_chr_connect_client_sync(chr, errp) < 0) {
>              return;
>          }
>      }
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH 07/12] chardev: split tcp_chr_wait_connected into two methods

[Daniel P. Berrangé]

On Tue, Jan 15, 2019 at 11:44:37PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > The tcp_chr_wait_connected method can deal with either server or client
> > chardevs, but some callers only care about one of these possibilities.
> > The tcp_chr_wait_connected method will also need some refactoring to
> > reliably deal with its primary goal of allowing a device frontend to
> > wait for an established connection, which will interfere with other
> > callers.
> >
> > Split it into two methods, one responsible for server initiated
> > connections, the other responsible for client initiated connections.
> > In doing this split the tcp_char_connect_async() method is renamed
> > to become consistent with naming of the new methods.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  chardev/char-socket.c | 59 +++++++++++++++++++++++++++----------------
> >  1 file changed, 37 insertions(+), 22 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 3b6ff6619b..3bd1be7631 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -886,30 +886,47 @@ static void tcp_chr_accept(QIONetListener *listener,
> >      tcp_chr_new_client(chr, cioc);
> >  }
> >
> > -static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
> > +
> > +static int tcp_chr_connect_client_sync(Chardev *chr, Error **errp)
> > +{
> > +    SocketChardev *s = SOCKET_CHARDEV(chr);
> > +    QIOChannelSocket *sioc = qio_channel_socket_new();
> > +    tcp_chr_set_client_ioc_name(chr, sioc);
> > +    if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
> > +        object_unref(OBJECT(sioc));
> > +        return -1;
> > +    }
> > +    tcp_chr_new_client(chr, sioc);
> > +    object_unref(OBJECT(sioc));
> > +    return 0;
> > +}
> > +
> > +
> > +static void tcp_chr_accept_server_sync(Chardev *chr)
> >  {
> >      SocketChardev *s = SOCKET_CHARDEV(chr);
> >      QIOChannelSocket *sioc;
> > +    info_report("QEMU waiting for connection on: %s",
> > +                chr->filename);
> > +    sioc = qio_net_listener_wait_client(s->listener);
> > +    tcp_chr_set_client_ioc_name(chr, sioc);
> > +    tcp_chr_new_client(chr, sioc);
> > +    object_unref(OBJECT(sioc));
> > +}
> > +
> >
> > +static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
> > +{
> > +    SocketChardev *s = SOCKET_CHARDEV(chr);
> >      /* It can't wait on s->connected, since it is set asynchronously
> >       * in TLS and telnet cases, only wait for an accepted socket */
> >      while (!s->ioc) {
> >          if (s->is_listen) {
> > -            info_report("QEMU waiting for connection on: %s",
> > -                        chr->filename);
> > -            sioc = qio_net_listener_wait_client(s->listener);
> > -            tcp_chr_set_client_ioc_name(chr, sioc);
> > -            tcp_chr_new_client(chr, sioc);
> > -            object_unref(OBJECT(sioc));
> > +            tcp_chr_accept_server_sync(chr);
> >          } else {
> > -            sioc = qio_channel_socket_new();
> > -            tcp_chr_set_client_ioc_name(chr, sioc);
> > -            if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
> > -                object_unref(OBJECT(sioc));
> > +            if (tcp_chr_connect_client_sync(chr, errp) < 0) {
> >                  return -1;
> >              }
> > -            tcp_chr_new_client(chr, sioc);
> > -            object_unref(OBJECT(sioc));
> >          }
> >      }
> >
> > @@ -958,7 +975,7 @@ cleanup:
> >      object_unref(OBJECT(sioc));
> >  }
> >
> > -static void tcp_chr_connect_async(Chardev *chr)
> > +static void tcp_chr_connect_client_async(Chardev *chr)
> >  {
> >      SocketChardev *s = SOCKET_CHARDEV(chr);
> >      QIOChannelSocket *sioc;
> > @@ -982,7 +999,7 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
> >          return false;
> >      }
> >
> > -    tcp_chr_connect_async(chr);
> > +    tcp_chr_connect_client_async(chr);
> >
> >      return false;
> >  }
> > @@ -1139,7 +1156,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
> >      }
> >
> >      if (s->reconnect_time) {
> > -        tcp_chr_connect_async(chr);
> > +        tcp_chr_connect_client_async(chr);
> >      } else {
> >          if (s->is_listen) {
> >              char *name;
> > @@ -1159,17 +1176,15 @@ static void qmp_chardev_open_socket(Chardev *chr,
> >              s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
> >              update_disconnected_filename(s);
> >
> > -            if (is_waitconnect &&
> > -                qemu_chr_wait_connected(chr, errp) < 0) {
> > -                return;
> > -            }
> > -            if (!s->ioc) {
> > +            if (is_waitconnect) {
> > +                tcp_chr_accept_server_sync(chr);
> 
> Is the wait_connected() loop really unnecessary there? looks like it
> is. And there is no error path either. ok

Yes, we only need to accept a single client during normal startup.

Even the loop in tcp_chr_wait_connected is actually bogus in the
current code, but I've not removed that since later patches will
need it again.

> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> > +            } else {
> >                  qio_net_listener_set_client_func_full(s->listener,
> >                                                        tcp_chr_accept,
> >                                                        chr, NULL,
> >                                                        chr->gcontext);
> >              }
> > -        } else if (qemu_chr_wait_connected(chr, errp) < 0) {
> > +        } else if (tcp_chr_connect_client_sync(chr, errp) < 0) {
> >              return;
> >          }
> >      }
> > --
> > 2.20.1
> >

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

[Qemu-devel] [PATCH 08/12] chardev: split up qmp_chardev_open_socket connection code

[Daniel P. Berrangé]

In qmp_chardev_open_socket the code for connecting client chardevs is
split across two conditionals far apart with some server chardev code in
the middle. Split up the method so that code for client connection setup
is separate from code for server connection setup.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 96 +++++++++++++++++++++++++++----------------
 1 file changed, 60 insertions(+), 36 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 3bd1be7631..385504b021 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1005,6 +1005,61 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
 }
 
 
+static int qmp_chardev_open_socket_server(Chardev *chr,
+                                          bool is_telnet,
+                                          bool is_waitconnect,
+                                          Error **errp)
+{
+    SocketChardev *s = SOCKET_CHARDEV(chr);
+    char *name;
+    if (is_telnet) {
+        s->do_telnetopt = 1;
+    }
+    s->listener = qio_net_listener_new();
+
+    name = g_strdup_printf("chardev-tcp-listener-%s", chr->label);
+    qio_net_listener_set_name(s->listener, name);
+    g_free(name);
+
+    if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
+        object_unref(OBJECT(s->listener));
+        s->listener = NULL;
+        return -1;
+    }
+
+    qapi_free_SocketAddress(s->addr);
+    s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
+    update_disconnected_filename(s);
+
+    if (is_waitconnect) {
+        tcp_chr_accept_server_sync(chr);
+    } else {
+        qio_net_listener_set_client_func_full(s->listener,
+                                              tcp_chr_accept,
+                                              chr, NULL,
+                                              chr->gcontext);
+    }
+
+    return 0;
+}
+
+
+static int qmp_chardev_open_socket_client(Chardev *chr,
+                                          int64_t reconnect,
+                                          Error **errp)
+{
+    SocketChardev *s = SOCKET_CHARDEV(chr);
+
+    if (reconnect > 0) {
+        s->reconnect_time = reconnect;
+        tcp_chr_connect_client_async(chr);
+        return 0;
+    } else {
+        return tcp_chr_connect_client_sync(chr, errp);
+    }
+}
+
+
 static bool qmp_chardev_validate_socket(ChardevSocket *sock,
                                         SocketAddress *addr,
                                         Error **errp)
@@ -1147,44 +1202,13 @@ static void qmp_chardev_open_socket(Chardev *chr,
 
     update_disconnected_filename(s);
 
-    if (is_listen) {
-        if (is_telnet || is_tn3270) {
-            s->do_telnetopt = 1;
+    if (s->is_listen) {
+        if (qmp_chardev_open_socket_server(chr, is_telnet || is_tn3270,
+                                           is_waitconnect, errp) < 0) {
+            return;
         }
-    } else if (reconnect > 0) {
-        s->reconnect_time = reconnect;
-    }
-
-    if (s->reconnect_time) {
-        tcp_chr_connect_client_async(chr);
     } else {
-        if (s->is_listen) {
-            char *name;
-            s->listener = qio_net_listener_new();
-
-            name = g_strdup_printf("chardev-tcp-listener-%s", chr->label);
-            qio_net_listener_set_name(s->listener, name);
-            g_free(name);
-
-            if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
-                object_unref(OBJECT(s->listener));
-                s->listener = NULL;
-                return;
-            }
-
-            qapi_free_SocketAddress(s->addr);
-            s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
-            update_disconnected_filename(s);
-
-            if (is_waitconnect) {
-                tcp_chr_accept_server_sync(chr);
-            } else {
-                qio_net_listener_set_client_func_full(s->listener,
-                                                      tcp_chr_accept,
-                                                      chr, NULL,
-                                                      chr->gcontext);
-            }
-        } else if (tcp_chr_connect_client_sync(chr, errp) < 0) {
+        if (qmp_chardev_open_socket_client(chr, reconnect, errp) < 0) {
             return;
         }
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 08/12] chardev: split up qmp_chardev_open_socket connection code

[Marc-André Lureau]

On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> In qmp_chardev_open_socket the code for connecting client chardevs is
> split across two conditionals far apart with some server chardev code in
> the middle. Split up the method so that code for client connection setup
> is separate from code for server connection setup.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c | 96 +++++++++++++++++++++++++++----------------
>  1 file changed, 60 insertions(+), 36 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 3bd1be7631..385504b021 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1005,6 +1005,61 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
>  }
>
>
> +static int qmp_chardev_open_socket_server(Chardev *chr,
> +                                          bool is_telnet,
> +                                          bool is_waitconnect,
> +                                          Error **errp)
> +{
> +    SocketChardev *s = SOCKET_CHARDEV(chr);
> +    char *name;
> +    if (is_telnet) {
> +        s->do_telnetopt = 1;
> +    }
> +    s->listener = qio_net_listener_new();
> +
> +    name = g_strdup_printf("chardev-tcp-listener-%s", chr->label);
> +    qio_net_listener_set_name(s->listener, name);
> +    g_free(name);
> +
> +    if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
> +        object_unref(OBJECT(s->listener));
> +        s->listener = NULL;
> +        return -1;
> +    }
> +
> +    qapi_free_SocketAddress(s->addr);
> +    s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
> +    update_disconnected_filename(s);
> +
> +    if (is_waitconnect) {
> +        tcp_chr_accept_server_sync(chr);
> +    } else {
> +        qio_net_listener_set_client_func_full(s->listener,
> +                                              tcp_chr_accept,
> +                                              chr, NULL,
> +                                              chr->gcontext);
> +    }
> +
> +    return 0;
> +}
> +
> +
> +static int qmp_chardev_open_socket_client(Chardev *chr,
> +                                          int64_t reconnect,
> +                                          Error **errp)
> +{
> +    SocketChardev *s = SOCKET_CHARDEV(chr);
> +
> +    if (reconnect > 0) {
> +        s->reconnect_time = reconnect;
> +        tcp_chr_connect_client_async(chr);
> +        return 0;
> +    } else {
> +        return tcp_chr_connect_client_sync(chr, errp);
> +    }
> +}
> +
> +
>  static bool qmp_chardev_validate_socket(ChardevSocket *sock,
>                                          SocketAddress *addr,
>                                          Error **errp)
> @@ -1147,44 +1202,13 @@ static void qmp_chardev_open_socket(Chardev *chr,
>
>      update_disconnected_filename(s);
>
> -    if (is_listen) {
> -        if (is_telnet || is_tn3270) {
> -            s->do_telnetopt = 1;
> +    if (s->is_listen) {
> +        if (qmp_chardev_open_socket_server(chr, is_telnet || is_tn3270,
> +                                           is_waitconnect, errp) < 0) {
> +            return;
>          }
> -    } else if (reconnect > 0) {
> -        s->reconnect_time = reconnect;
> -    }
> -
> -    if (s->reconnect_time) {
> -        tcp_chr_connect_client_async(chr);
>      } else {
> -        if (s->is_listen) {
> -            char *name;
> -            s->listener = qio_net_listener_new();
> -
> -            name = g_strdup_printf("chardev-tcp-listener-%s", chr->label);
> -            qio_net_listener_set_name(s->listener, name);
> -            g_free(name);
> -
> -            if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
> -                object_unref(OBJECT(s->listener));
> -                s->listener = NULL;
> -                return;
> -            }
> -
> -            qapi_free_SocketAddress(s->addr);
> -            s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
> -            update_disconnected_filename(s);
> -
> -            if (is_waitconnect) {
> -                tcp_chr_accept_server_sync(chr);
> -            } else {
> -                qio_net_listener_set_client_func_full(s->listener,
> -                                                      tcp_chr_accept,
> -                                                      chr, NULL,
> -                                                      chr->gcontext);
> -            }
> -        } else if (tcp_chr_connect_client_sync(chr, errp) < 0) {
> +        if (qmp_chardev_open_socket_client(chr, reconnect, errp) < 0) {
>              return;
>          }
>      }
> --
> 2.20.1
>

[Qemu-devel] [PATCH 09/12] chardev: use a state machine for socket connection state

[Daniel P. Berrangé]

The socket connection state is indicated via the 'bool connected' field
in the SocketChardev struct. This variable is somewhat misleading
though, as it is only set to true once the connection has completed all
required handshakes (eg for TLS, telnet or websockets). IOW there is a
period of time in which the socket is connected, but the "connected"
flag is still false.

The socket chardev really has three states that it can be in,
disconnected, connecting and connected and those should be tracked
explicitly.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 63 +++++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 14 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 385504b021..96a60eb105 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -46,6 +46,12 @@ typedef struct {
     size_t buflen;
 } TCPChardevTelnetInit;
 
+typedef enum {
+    TCP_CHARDEV_STATE_DISCONNECTED,
+    TCP_CHARDEV_STATE_CONNECTING,
+    TCP_CHARDEV_STATE_CONNECTED,
+} TCPChardevState;
+
 typedef struct {
     Chardev parent;
     QIOChannel *ioc; /* Client I/O channel */
@@ -53,7 +59,7 @@ typedef struct {
     QIONetListener *listener;
     GSource *hup_source;
     QCryptoTLSCreds *tls_creds;
-    int connected;
+    TCPChardevState state;
     int max_size;
     int do_telnetopt;
     int do_nodelay;
@@ -82,6 +88,21 @@ typedef struct {
 static gboolean socket_reconnect_timeout(gpointer opaque);
 static void tcp_chr_telnet_init(Chardev *chr);
 
+static void tcp_chr_change_state(SocketChardev *s, TCPChardevState state)
+{
+    switch (state) {
+    case TCP_CHARDEV_STATE_DISCONNECTED:
+        break;
+    case TCP_CHARDEV_STATE_CONNECTING:
+        assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED);
+        break;
+    case TCP_CHARDEV_STATE_CONNECTED:
+        assert(s->state == TCP_CHARDEV_STATE_CONNECTING);
+        break;
+    }
+    s->state = state;
+}
+
 static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
 {
     if (s->reconnect_timer) {
@@ -96,7 +117,7 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
     SocketChardev *s = SOCKET_CHARDEV(chr);
     char *name;
 
-    assert(s->connected == 0);
+    assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED);
     name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
     s->reconnect_timer = qemu_chr_timeout_add_ms(chr,
                                                  s->reconnect_time * 1000,
@@ -131,7 +152,7 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
-    if (s->connected) {
+    if (s->state == TCP_CHARDEV_STATE_CONNECTED) {
         int ret =  io_channel_send_full(s->ioc, buf, len,
                                         s->write_msgfds,
                                         s->write_msgfds_num);
@@ -164,7 +185,7 @@ static int tcp_chr_read_poll(void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
     SocketChardev *s = SOCKET_CHARDEV(opaque);
-    if (!s->connected) {
+    if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
         return 0;
     }
     s->max_size = qemu_chr_be_can_write(chr);
@@ -277,7 +298,7 @@ static int tcp_set_msgfds(Chardev *chr, int *fds, int num)
     s->write_msgfds = NULL;
     s->write_msgfds_num = 0;
 
-    if (!s->connected ||
+    if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||
         !qio_channel_has_feature(s->ioc,
                                  QIO_CHANNEL_FEATURE_FD_PASS)) {
         return -1;
@@ -389,7 +410,7 @@ static void tcp_chr_free_connection(Chardev *chr)
     s->ioc = NULL;
     g_free(chr->filename);
     chr->filename = NULL;
-    s->connected = 0;
+    tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
 }
 
 static const char *qemu_chr_socket_protocol(SocketChardev *s)
@@ -442,12 +463,12 @@ static void update_disconnected_filename(SocketChardev *s)
 
 /* NB may be called even if tcp_chr_connect has not been
  * reached, due to TLS or telnet initialization failure,
- * so can *not* assume s->connected == true
+ * so can *not* assume s->state == TCP_CHARDEV_STATE_CONNECTED
  */
 static void tcp_chr_disconnect(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
-    bool emit_close = s->connected;
+    bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
 
     tcp_chr_free_connection(chr);
 
@@ -471,7 +492,8 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     uint8_t buf[CHR_READ_BUF_LEN];
     int len, size;
 
-    if (!s->connected || s->max_size <= 0) {
+    if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||
+        s->max_size <= 0) {
         return TRUE;
     }
     len = sizeof(buf);
@@ -508,7 +530,7 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
     SocketChardev *s = SOCKET_CHARDEV(chr);
     int size;
 
-    if (!s->connected) {
+    if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
         return 0;
     }
 
@@ -564,7 +586,7 @@ static void update_ioc_handlers(SocketChardev *s)
 {
     Chardev *chr = CHARDEV(s);
 
-    if (!s->connected) {
+    if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
         return;
     }
 
@@ -589,7 +611,7 @@ static void tcp_chr_connect(void *opaque)
     g_free(chr->filename);
     chr->filename = qemu_chr_compute_filename(s);
 
-    s->connected = 1;
+    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTED);
     update_ioc_handlers(s);
     qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }
@@ -828,7 +850,7 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
-    if (s->ioc != NULL) {
+    if (s->state != TCP_CHARDEV_STATE_CONNECTING) {
         return -1;
     }
 
@@ -865,11 +887,17 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
 {
     int ret;
     QIOChannelSocket *sioc;
+    SocketChardev *s = SOCKET_CHARDEV(chr);
+
+    if (s->state != TCP_CHARDEV_STATE_DISCONNECTED) {
+        return -1;
+    }
 
     sioc = qio_channel_socket_new_fd(fd, NULL);
     if (!sioc) {
         return -1;
     }
+    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
     tcp_chr_set_client_ioc_name(chr, sioc);
     ret = tcp_chr_new_client(chr, sioc);
     object_unref(OBJECT(sioc));
@@ -881,7 +909,9 @@ static void tcp_chr_accept(QIONetListener *listener,
                            void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
+    SocketChardev *s = SOCKET_CHARDEV(chr);
 
+    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
     tcp_chr_set_client_ioc_name(chr, cioc);
     tcp_chr_new_client(chr, cioc);
 }
@@ -891,8 +921,10 @@ static int tcp_chr_connect_client_sync(Chardev *chr, Error **errp)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
     QIOChannelSocket *sioc = qio_channel_socket_new();
+    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
     tcp_chr_set_client_ioc_name(chr, sioc);
     if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
+        tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
         object_unref(OBJECT(sioc));
         return -1;
     }
@@ -908,6 +940,7 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
     QIOChannelSocket *sioc;
     info_report("QEMU waiting for connection on: %s",
                 chr->filename);
+    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
     sioc = qio_net_listener_wait_client(s->listener);
     tcp_chr_set_client_ioc_name(chr, sioc);
     tcp_chr_new_client(chr, sioc);
@@ -963,6 +996,7 @@ static void qemu_chr_socket_connected(QIOTask *task, void *opaque)
     Error *err = NULL;
 
     if (qio_task_propagate_error(task, &err)) {
+        tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
         check_report_connect_error(chr, err);
         error_free(err);
         goto cleanup;
@@ -980,6 +1014,7 @@ static void tcp_chr_connect_client_async(Chardev *chr)
     SocketChardev *s = SOCKET_CHARDEV(chr);
     QIOChannelSocket *sioc;
 
+    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
     sioc = qio_channel_socket_new();
     tcp_chr_set_client_ioc_name(chr, sioc);
     qio_channel_socket_connect_async(sioc, s->addr,
@@ -1307,7 +1342,7 @@ char_socket_get_connected(Object *obj, Error **errp)
 {
     SocketChardev *s = SOCKET_CHARDEV(obj);
 
-    return s->connected;
+    return s->state == TCP_CHARDEV_STATE_CONNECTED;
 }
 
 static void char_socket_class_init(ObjectClass *oc, void *data)
-- 
2.20.1

Re: [Qemu-devel] [PATCH 09/12] chardev: use a state machine for socket connection state

[Marc-André Lureau]

Hi

On Tue, Jan 15, 2019 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The socket connection state is indicated via the 'bool connected' field
> in the SocketChardev struct. This variable is somewhat misleading
> though, as it is only set to true once the connection has completed all
> required handshakes (eg for TLS, telnet or websockets). IOW there is a
> period of time in which the socket is connected, but the "connected"
> flag is still false.
>
> The socket chardev really has three states that it can be in,
> disconnected, connecting and connected and those should be tracked
> explicitly.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

You could split that patch, to introduce CONNECTING in a separate step.

But lgtm so,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c | 63 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 385504b021..96a60eb105 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -46,6 +46,12 @@ typedef struct {
>      size_t buflen;
>  } TCPChardevTelnetInit;
>
> +typedef enum {
> +    TCP_CHARDEV_STATE_DISCONNECTED,
> +    TCP_CHARDEV_STATE_CONNECTING,
> +    TCP_CHARDEV_STATE_CONNECTED,
> +} TCPChardevState;
> +
>  typedef struct {
>      Chardev parent;
>      QIOChannel *ioc; /* Client I/O channel */
> @@ -53,7 +59,7 @@ typedef struct {
>      QIONetListener *listener;
>      GSource *hup_source;
>      QCryptoTLSCreds *tls_creds;
> -    int connected;
> +    TCPChardevState state;
>      int max_size;
>      int do_telnetopt;
>      int do_nodelay;
> @@ -82,6 +88,21 @@ typedef struct {
>  static gboolean socket_reconnect_timeout(gpointer opaque);
>  static void tcp_chr_telnet_init(Chardev *chr);
>
> +static void tcp_chr_change_state(SocketChardev *s, TCPChardevState state)
> +{
> +    switch (state) {
> +    case TCP_CHARDEV_STATE_DISCONNECTED:
> +        break;
> +    case TCP_CHARDEV_STATE_CONNECTING:
> +        assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED);
> +        break;
> +    case TCP_CHARDEV_STATE_CONNECTED:
> +        assert(s->state == TCP_CHARDEV_STATE_CONNECTING);
> +        break;
> +    }
> +    s->state = state;
> +}
> +
>  static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
>  {
>      if (s->reconnect_timer) {
> @@ -96,7 +117,7 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>      char *name;
>
> -    assert(s->connected == 0);
> +    assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED);
>      name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
>      s->reconnect_timer = qemu_chr_timeout_add_ms(chr,
>                                                   s->reconnect_time * 1000,
> @@ -131,7 +152,7 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>
> -    if (s->connected) {
> +    if (s->state == TCP_CHARDEV_STATE_CONNECTED) {
>          int ret =  io_channel_send_full(s->ioc, buf, len,
>                                          s->write_msgfds,
>                                          s->write_msgfds_num);
> @@ -164,7 +185,7 @@ static int tcp_chr_read_poll(void *opaque)
>  {
>      Chardev *chr = CHARDEV(opaque);
>      SocketChardev *s = SOCKET_CHARDEV(opaque);
> -    if (!s->connected) {
> +    if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
>          return 0;
>      }
>      s->max_size = qemu_chr_be_can_write(chr);
> @@ -277,7 +298,7 @@ static int tcp_set_msgfds(Chardev *chr, int *fds, int num)
>      s->write_msgfds = NULL;
>      s->write_msgfds_num = 0;
>
> -    if (!s->connected ||
> +    if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||
>          !qio_channel_has_feature(s->ioc,
>                                   QIO_CHANNEL_FEATURE_FD_PASS)) {
>          return -1;
> @@ -389,7 +410,7 @@ static void tcp_chr_free_connection(Chardev *chr)
>      s->ioc = NULL;
>      g_free(chr->filename);
>      chr->filename = NULL;
> -    s->connected = 0;
> +    tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
>  }
>
>  static const char *qemu_chr_socket_protocol(SocketChardev *s)
> @@ -442,12 +463,12 @@ static void update_disconnected_filename(SocketChardev *s)
>
>  /* NB may be called even if tcp_chr_connect has not been
>   * reached, due to TLS or telnet initialization failure,
> - * so can *not* assume s->connected == true
> + * so can *not* assume s->state == TCP_CHARDEV_STATE_CONNECTED
>   */
>  static void tcp_chr_disconnect(Chardev *chr)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> -    bool emit_close = s->connected;
> +    bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
>
>      tcp_chr_free_connection(chr);
>
> @@ -471,7 +492,8 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
>      uint8_t buf[CHR_READ_BUF_LEN];
>      int len, size;
>
> -    if (!s->connected || s->max_size <= 0) {
> +    if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||
> +        s->max_size <= 0) {
>          return TRUE;
>      }
>      len = sizeof(buf);
> @@ -508,7 +530,7 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>      int size;
>
> -    if (!s->connected) {
> +    if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
>          return 0;
>      }
>
> @@ -564,7 +586,7 @@ static void update_ioc_handlers(SocketChardev *s)
>  {
>      Chardev *chr = CHARDEV(s);
>
> -    if (!s->connected) {
> +    if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
>          return;
>      }
>
> @@ -589,7 +611,7 @@ static void tcp_chr_connect(void *opaque)
>      g_free(chr->filename);
>      chr->filename = qemu_chr_compute_filename(s);
>
> -    s->connected = 1;
> +    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTED);
>      update_ioc_handlers(s);
>      qemu_chr_be_event(chr, CHR_EVENT_OPENED);
>  }
> @@ -828,7 +850,7 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>
> -    if (s->ioc != NULL) {
> +    if (s->state != TCP_CHARDEV_STATE_CONNECTING) {
>          return -1;
>      }
>
> @@ -865,11 +887,17 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
>  {
>      int ret;
>      QIOChannelSocket *sioc;
> +    SocketChardev *s = SOCKET_CHARDEV(chr);
> +
> +    if (s->state != TCP_CHARDEV_STATE_DISCONNECTED) {
> +        return -1;
> +    }
>
>      sioc = qio_channel_socket_new_fd(fd, NULL);
>      if (!sioc) {
>          return -1;
>      }
> +    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
>      tcp_chr_set_client_ioc_name(chr, sioc);
>      ret = tcp_chr_new_client(chr, sioc);
>      object_unref(OBJECT(sioc));
> @@ -881,7 +909,9 @@ static void tcp_chr_accept(QIONetListener *listener,
>                             void *opaque)
>  {
>      Chardev *chr = CHARDEV(opaque);
> +    SocketChardev *s = SOCKET_CHARDEV(chr);
>
> +    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
>      tcp_chr_set_client_ioc_name(chr, cioc);
>      tcp_chr_new_client(chr, cioc);
>  }
> @@ -891,8 +921,10 @@ static int tcp_chr_connect_client_sync(Chardev *chr, Error **errp)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>      QIOChannelSocket *sioc = qio_channel_socket_new();
> +    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
>      tcp_chr_set_client_ioc_name(chr, sioc);
>      if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
> +        tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
>          object_unref(OBJECT(sioc));
>          return -1;
>      }
> @@ -908,6 +940,7 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
>      QIOChannelSocket *sioc;
>      info_report("QEMU waiting for connection on: %s",
>                  chr->filename);
> +    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
>      sioc = qio_net_listener_wait_client(s->listener);
>      tcp_chr_set_client_ioc_name(chr, sioc);
>      tcp_chr_new_client(chr, sioc);
> @@ -963,6 +996,7 @@ static void qemu_chr_socket_connected(QIOTask *task, void *opaque)
>      Error *err = NULL;
>
>      if (qio_task_propagate_error(task, &err)) {
> +        tcp_chr_change_state(s, TCP_CHARDEV_STATE_DISCONNECTED);
>          check_report_connect_error(chr, err);
>          error_free(err);
>          goto cleanup;
> @@ -980,6 +1014,7 @@ static void tcp_chr_connect_client_async(Chardev *chr)
>      SocketChardev *s = SOCKET_CHARDEV(chr);
>      QIOChannelSocket *sioc;
>
> +    tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
>      sioc = qio_channel_socket_new();
>      tcp_chr_set_client_ioc_name(chr, sioc);
>      qio_channel_socket_connect_async(sioc, s->addr,
> @@ -1307,7 +1342,7 @@ char_socket_get_connected(Object *obj, Error **errp)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(obj);
>
> -    return s->connected;
> +    return s->state == TCP_CHARDEV_STATE_CONNECTED;
>  }
>
>  static void char_socket_class_init(ObjectClass *oc, void *data)
> --
> 2.20.1
>

[Qemu-devel] [PATCH 10/12] chardev: honour the reconnect setting in tcp_chr_wait_connected

[Daniel P. Berrangé]

If establishing a client connection fails, the tcp_chr_wait_connected
method should sleep for the reconnect timeout and then retry the
attempt. This ensures the callers don't immediately abort with an
error when the initial connection fails.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 96a60eb105..91d775e9c5 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -957,8 +957,15 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
         if (s->is_listen) {
             tcp_chr_accept_server_sync(chr);
         } else {
-            if (tcp_chr_connect_client_sync(chr, errp) < 0) {
-                return -1;
+            Error *err = NULL;
+            if (tcp_chr_connect_client_sync(chr, &err) < 0) {
+                if (s->reconnect_time) {
+                    error_free(err);
+                    g_usleep(s->reconnect_time);
+                } else {
+                    error_propagate(errp, err);
+                    return -1;
+                }
             }
         }
     }
-- 
2.20.1

Re: [Qemu-devel] [PATCH 10/12] chardev: honour the reconnect setting in tcp_chr_wait_connected

[Marc-André Lureau]

Hi

On Tue, Jan 15, 2019 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> If establishing a client connection fails, the tcp_chr_wait_connected
> method should sleep for the reconnect timeout and then retry the
> attempt. This ensures the callers don't immediately abort with an
> error when the initial connection fails.

Obviously, that function is already a bit problematic, since it may
block the thread with no easy way to cancel (well, the _sync functions
in general..).

You change doesn't make it much worse, but it will now loop also in
case of errors. That's what reconnect_time is supposed to do, so

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 96a60eb105..91d775e9c5 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -957,8 +957,15 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
>          if (s->is_listen) {
>              tcp_chr_accept_server_sync(chr);
>          } else {
> -            if (tcp_chr_connect_client_sync(chr, errp) < 0) {
> -                return -1;
> +            Error *err = NULL;
> +            if (tcp_chr_connect_client_sync(chr, &err) < 0) {
> +                if (s->reconnect_time) {
> +                    error_free(err);
> +                    g_usleep(s->reconnect_time);
> +                } else {
> +                    error_propagate(errp, err);
> +                    return -1;
> +                }
>              }
>          }
>      }
> --
> 2.20.1
>

[Qemu-devel] [PATCH 11/12] chardev: disallow TLS/telnet/websocket with tcp_chr_wait_connected

[Daniel P. Berrangé]

In the previous commit

    commit 1dc8a6695c731abb7461c637b2512c3670d82be4
    Author: Marc-André Lureau <marcandre.lureau@redhat.com>
    Date:   Tue Aug 16 12:33:32 2016 +0400

      char: fix waiting for TLS and telnet connection

the tcp_chr_wait_connected() method was changed to check for a non-NULL
's->ioc' as a sign that there is already a connection present, as
opposed to checking the "connected" flag to supposedly fix handling of
TLS/telnet connections.

The original code would repeatedly call tcp_chr_wait_connected creating
many connections as 'connected' would never become true. The changed
code would still repeatedly call tcp_chr_wait_connected busy waiting
because s->ioc is set but the chardev will never see CHR_EVENT_OPENED.
IOW, the code is still broken with TLS/telnet, but in a different way.

Checking for a non-NULL 's->ioc' does not mean that a CHR_EVENT_OPENED
will be ready for a TLS/telnet connection. These protocols (and the
websocket protocol) all require the main loop to be running in order
to complete the protocol handshake before emitting CHR_EVENT_OPENED.
The tcp_chr_wait_connected() method is only used during early startup
before a main loop is running, so TLS/telnet/websock connections can
never complete initialization.

Making this work would require changing tcp_chr_wait_connected to run
a main loop. This is quite complex since we must not allow GSource's
that other parts of QEMU have registered to run yet. The current callers
of tcp_chr_wait_connected do not require use of the TLS/telnet/websocket
protocols, so the simplest option is to just forbid this combination
completely for now.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 91d775e9c5..7e98a95bbd 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -951,8 +951,20 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
 static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
-    /* It can't wait on s->connected, since it is set asynchronously
-     * in TLS and telnet cases, only wait for an accepted socket */
+    const char *opts[] = { "telnet", "tn3270", "websock", "tls-creds" };
+    bool optset[] = { s->is_telnet, s->is_tn3270, s->is_websock, s->tls_creds };
+    size_t i;
+
+    QEMU_BUILD_BUG_ON(G_N_ELEMENTS(opts) != G_N_ELEMENTS(optset));
+    for (i = 0; i < G_N_ELEMENTS(opts); i++) {
+        if (optset[i]) {
+            error_setg(errp,
+                       "'%s' option is incompatible with waiting for "
+                       "connection during early startup", opts[i]);
+            return -1;
+        }
+    }
+
     while (!s->ioc) {
         if (s->is_listen) {
             tcp_chr_accept_server_sync(chr);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 11/12] chardev: disallow TLS/telnet/websocket with tcp_chr_wait_connected

[Marc-André Lureau]

On Tue, Jan 15, 2019 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> In the previous commit
>
>     commit 1dc8a6695c731abb7461c637b2512c3670d82be4
>     Author: Marc-André Lureau <marcandre.lureau@redhat.com>
>     Date:   Tue Aug 16 12:33:32 2016 +0400
>
>       char: fix waiting for TLS and telnet connection
>
> the tcp_chr_wait_connected() method was changed to check for a non-NULL
> 's->ioc' as a sign that there is already a connection present, as
> opposed to checking the "connected" flag to supposedly fix handling of
> TLS/telnet connections.
>
> The original code would repeatedly call tcp_chr_wait_connected creating
> many connections as 'connected' would never become true. The changed
> code would still repeatedly call tcp_chr_wait_connected busy waiting
> because s->ioc is set but the chardev will never see CHR_EVENT_OPENED.
> IOW, the code is still broken with TLS/telnet, but in a different way.
>
> Checking for a non-NULL 's->ioc' does not mean that a CHR_EVENT_OPENED
> will be ready for a TLS/telnet connection. These protocols (and the
> websocket protocol) all require the main loop to be running in order
> to complete the protocol handshake before emitting CHR_EVENT_OPENED.
> The tcp_chr_wait_connected() method is only used during early startup
> before a main loop is running, so TLS/telnet/websock connections can
> never complete initialization.
>
> Making this work would require changing tcp_chr_wait_connected to run
> a main loop. This is quite complex since we must not allow GSource's
> that other parts of QEMU have registered to run yet. The current callers
> of tcp_chr_wait_connected do not require use of the TLS/telnet/websocket
> protocols, so the simplest option is to just forbid this combination
> completely for now.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 91d775e9c5..7e98a95bbd 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -951,8 +951,20 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
>  static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> -    /* It can't wait on s->connected, since it is set asynchronously
> -     * in TLS and telnet cases, only wait for an accepted socket */
> +    const char *opts[] = { "telnet", "tn3270", "websock", "tls-creds" };
> +    bool optset[] = { s->is_telnet, s->is_tn3270, s->is_websock, s->tls_creds };
> +    size_t i;
> +
> +    QEMU_BUILD_BUG_ON(G_N_ELEMENTS(opts) != G_N_ELEMENTS(optset));
> +    for (i = 0; i < G_N_ELEMENTS(opts); i++) {
> +        if (optset[i]) {
> +            error_setg(errp,
> +                       "'%s' option is incompatible with waiting for "
> +                       "connection during early startup", opts[i]);

"during early startup" ? I think you could also reach this by using
chardev-add & netdev_add.

> +            return -1;
> +        }
> +    }
> +
>      while (!s->ioc) {
>          if (s->is_listen) {
>              tcp_chr_accept_server_sync(chr);
> --
> 2.20.1
>

Re: [Qemu-devel] [PATCH 11/12] chardev: disallow TLS/telnet/websocket with tcp_chr_wait_connected

[Daniel P. Berrangé]

On Wed, Jan 16, 2019 at 01:54:54AM +0400, Marc-André Lureau wrote:
> On Tue, Jan 15, 2019 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > In the previous commit
> >
> >     commit 1dc8a6695c731abb7461c637b2512c3670d82be4
> >     Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> >     Date:   Tue Aug 16 12:33:32 2016 +0400
> >
> >       char: fix waiting for TLS and telnet connection
> >
> > the tcp_chr_wait_connected() method was changed to check for a non-NULL
> > 's->ioc' as a sign that there is already a connection present, as
> > opposed to checking the "connected" flag to supposedly fix handling of
> > TLS/telnet connections.
> >
> > The original code would repeatedly call tcp_chr_wait_connected creating
> > many connections as 'connected' would never become true. The changed
> > code would still repeatedly call tcp_chr_wait_connected busy waiting
> > because s->ioc is set but the chardev will never see CHR_EVENT_OPENED.
> > IOW, the code is still broken with TLS/telnet, but in a different way.
> >
> > Checking for a non-NULL 's->ioc' does not mean that a CHR_EVENT_OPENED
> > will be ready for a TLS/telnet connection. These protocols (and the
> > websocket protocol) all require the main loop to be running in order
> > to complete the protocol handshake before emitting CHR_EVENT_OPENED.
> > The tcp_chr_wait_connected() method is only used during early startup
> > before a main loop is running, so TLS/telnet/websock connections can
> > never complete initialization.
> >
> > Making this work would require changing tcp_chr_wait_connected to run
> > a main loop. This is quite complex since we must not allow GSource's
> > that other parts of QEMU have registered to run yet. The current callers
> > of tcp_chr_wait_connected do not require use of the TLS/telnet/websocket
> > protocols, so the simplest option is to just forbid this combination
> > completely for now.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> > ---
> >  chardev/char-socket.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 91d775e9c5..7e98a95bbd 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -951,8 +951,20 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
> >  static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
> >  {
> >      SocketChardev *s = SOCKET_CHARDEV(chr);
> > -    /* It can't wait on s->connected, since it is set asynchronously
> > -     * in TLS and telnet cases, only wait for an accepted socket */
> > +    const char *opts[] = { "telnet", "tn3270", "websock", "tls-creds" };
> > +    bool optset[] = { s->is_telnet, s->is_tn3270, s->is_websock, s->tls_creds };
> > +    size_t i;
> > +
> > +    QEMU_BUILD_BUG_ON(G_N_ELEMENTS(opts) != G_N_ELEMENTS(optset));
> > +    for (i = 0; i < G_N_ELEMENTS(opts); i++) {
> > +        if (optset[i]) {
> > +            error_setg(errp,
> > +                       "'%s' option is incompatible with waiting for "
> > +                       "connection during early startup", opts[i]);
> 
> "during early startup" ? I think you could also reach this by using
> chardev-add & netdev_add.

Yes, I didn't realize that at first. It also means that patch 12 is in
fact still broken in the hotplug case.


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

[Qemu-devel] [PATCH 12/12] chardev: fix race with client connections in tcp_chr_wait_connected

[Daniel P. Berrangé]

When the 'reconnect' option is given for a client connection, the
qmp_chardev_open_socket_client method will run an asynchronous
connection attempt. The QIOChannel socket executes this is a single use
background thread, so the connection will succeed immediately (assuming
the server is listening). The chardev, however, won't get the result
from this background thread until the main loop starts running and
processes idle callbacks.

Thus when tcp_chr_wait_connected is run s->ioc will be NULL, and the
state will still be TCP_CHARDEV_STATE_DISCONNECTED, but there will
already be an established connection that will be associated with the
chardev by the pending idle callback.  tcp_chr_wait_connected doesn't
see this and so attempts to establish another connection synchronously.

If the server allows multiple connections this is unhelpful but not a
fatal problem as the duplicate connection will get ignored by the
tcp_chr_new_client method when it sees the state is already connected.

If the server only supports a single connection, however, the
tcp_chr_wait_connected method will hang forever because the server will
not accept its synchronous connection attempt until the first connection
is closed.

To deal with this we must ensure that qmp_chardev_open_socket_client
does not actually start the asynchronous connection attempt. Instead it
should schedule a timer with 0ms expiry time, which will only be
processed once the main loop starts running. The tcp_chr_wait_connected
method can now safely do a synchronous connection attempt without
creating a race condition. When the timer expires it will see that a
connection has already been established and take no further action.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 7e98a95bbd..07942d7a1b 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -965,7 +965,25 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
         }
     }
 
-    while (!s->ioc) {
+    /*
+     * We expect state to be as follows:
+     *
+     *  - server
+     *    - wait   -> CONNECTED
+     *    - nowait -> DISCONNECTED
+     *  - client
+     *    - reconnect == 0 -> CONNECTED
+     *    - reconnect != 0 -> DISCONNECTED
+     *
+     */
+    if (s->state == TCP_CHARDEV_STATE_CONNECTING) {
+        error_setg(errp,
+                   "Unexpected 'connecting' state when waiting for "
+                   "connection during early startup");
+        return -1;
+    }
+
+    while (s->state != TCP_CHARDEV_STATE_CONNECTED) {
         if (s->is_listen) {
             tcp_chr_accept_server_sync(chr);
         } else {
@@ -1106,7 +1124,15 @@ static int qmp_chardev_open_socket_client(Chardev *chr,
 
     if (reconnect > 0) {
         s->reconnect_time = reconnect;
-        tcp_chr_connect_client_async(chr);
+        /*
+         * We must not start the socket connect attempt until the main
+         * loop is running, otherwise qemu_chr_wait_connect will not be
+         * able to take over connection establishment during startup
+         */
+        s->reconnect_timer = qemu_chr_timeout_add_ms(chr,
+                                                     0,
+                                                     socket_reconnect_timeout,
+                                                     chr);
         return 0;
     } else {
         return tcp_chr_connect_client_sync(chr, errp);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 00/12] chardev: refactoring & many bugfixes related tcp_chr_wait_connected

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190115145256.9593-1-berrange@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===

  CC      hw/char/debugcon.o
In function 'acpi_table_install',
    inlined from 'acpi_table_add' at /tmp/qemu-test/src/hw/acpi/core.c:296:5:
/tmp/qemu-test/src/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
         strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/acpi/core.c:203:9: error: 'strncpy' specified bound 6 equals destination size [-Werror=stringop-truncation]
         strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/acpi/core.c:207:9: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation]
         strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                 sizeof ext_hdr->oem_table_id);
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/acpi/core.c:216:9: error: 'strncpy' specified bound 4 equals destination size [-Werror=stringop-truncation]
         strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                 sizeof ext_hdr->asl_compiler_id);
---
make: *** [/tmp/qemu-test/src/rules.mak:69: hw/acpi/core.o] Error 1
make: *** Waiting for unfinished jobs....
/tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
/tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
     strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/sheepdog.o] Error 1
/tmp/qemu-test/src/hw/acpi/aml-build.c: In function 'build_header':
/tmp/qemu-test/src/hw/acpi/aml-build.c:1535:9: error: 'strncpy' specified bound 6 equals destination size [-Werror=stringop-truncation]
         strncpy((char *)h->oem_id, oem_id, sizeof h->oem_id);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/qemu-test/src/hw/acpi/aml-build.c:1541:9: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation]
         strncpy((char *)h->oem_table_id, oem_table_id, sizeof(h->oem_table_id));
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


The full log is available at
http://patchew.org/logs/20190115145256.9593-1-berrange@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com