[Qemu-devel] [PATCH 00/13] A bunch of little socket + chardev fixes

[Qemu-devel] [PATCH 00/13] A bunch of little socket + chardev fixes

[Gerd Hoffmann]

  Hi,

Here is a colletion of little bugfixes and minor improvements for the
chardev and socket code.

cheers,
  Gerd

Gerd Hoffmann (13):
  qemu-socket: zero-initialize SocketAddress
  qemu-socket: drop pointless allocation
  qemu-socket: catch monitor_get_fd failures
  qemu-char: check optional fields using has_*
  qemu-char: use more specific error_setg_* variants
  qemu-char: print notification to stderr
  qemu-char: fix documentation for telnet+wait socket flags
  qemu-char: don't leak opts on error
  qemu-char: use ChardevBackendKind in in CharDriver
  qemu-char: minor mux chardev fixes
  qemu-char: add -chardev mux support
  qemu-char: report udb backend errors
  qemu-socket: don't leak opts on error

 qapi-schema.json    |    7 ++++---
 qemu-char.c         |   43 +++++++++++++++++++++++++++++++------------
 util/qemu-sockets.c |    8 +++++---
 3 files changed, 40 insertions(+), 18 deletions(-)

-- 
1.7.9.7

[Qemu-devel] [PATCH 01/13] qemu-socket: zero-initialize SocketAddress

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/qemu-sockets.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index fdd8dc4..364bd8c 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -855,7 +855,7 @@ SocketAddress *socket_parse(const char *str, Error **errp)
 {
     SocketAddress *addr = NULL;
 
-    addr = g_new(SocketAddress, 1);
+    addr = g_new0(SocketAddress, 1);
     if (strstart(str, "unix:", NULL)) {
         if (str[5] == '\0') {
             error_setg(errp, "invalid Unix socket address");
-- 
1.7.9.7

Re: [Qemu-devel] [Qemu-trivial] [PATCH 01/13] qemu-socket: zero-initialize SocketAddress

[Michael Tokarev]

21.06.2013 14:38, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  util/qemu-sockets.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index fdd8dc4..364bd8c 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -855,7 +855,7 @@ SocketAddress *socket_parse(const char *str, Error **errp)
>  {
>      SocketAddress *addr = NULL;
>  
> -    addr = g_new(SocketAddress, 1);
> +    addr = g_new0(SocketAddress, 1);

While at it we can remove the =NULL assignment too, guess, someting like this:

--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -848,9 +848,7 @@ int unix_nonblocking_connect(const char *path,

 SocketAddress *socket_parse(const char *str, Error **errp)
 {
-    SocketAddress *addr = NULL;
-
-    addr = g_new(SocketAddress, 1);
+    SocketAddress *addr = addr = g_new0(SocketAddress, 1);
     if (strstart(str, "unix:", NULL)) {
         if (str[5] == '\0') {
             error_setg(errp, "invalid Unix socket address");

Is that okay with you? :)

Not that it matter much actually (I guess gcc may optimize it
out entirely by its own already).

And not that the original issue is a big issue really, because
in each case each relevant field is initialized.  It's still
nice to see stuff clean in debugger and other places, but the
code which actually uses this struct should work fine without
the change.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 01/13] qemu-socket: zero-initialize SocketAddress

[Gerd Hoffmann]

On 06/21/13 20:15, Michael Tokarev wrote:
> 21.06.2013 14:38, Gerd Hoffmann wrote:
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  util/qemu-sockets.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index fdd8dc4..364bd8c 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -855,7 +855,7 @@ SocketAddress *socket_parse(const char *str, Error **errp)
>>  {
>>      SocketAddress *addr = NULL;
>>  
>> -    addr = g_new(SocketAddress, 1);
>> +    addr = g_new0(SocketAddress, 1);
> 
> While at it we can remove the =NULL assignment too, guess, someting like this:
> 
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -848,9 +848,7 @@ int unix_nonblocking_connect(const char *path,
> 
>  SocketAddress *socket_parse(const char *str, Error **errp)
>  {
> -    SocketAddress *addr = NULL;
> -
> -    addr = g_new(SocketAddress, 1);
> +    SocketAddress *addr = addr = g_new0(SocketAddress, 1);
>      if (strstart(str, "unix:", NULL)) {
>          if (str[5] == '\0') {
>              error_setg(errp, "invalid Unix socket address");
> 
> Is that okay with you? :)

Yes.

> And not that the original issue is a big issue really, because
> in each case each relevant field is initialized.

Except in the error cases.

cheers,
  Gerd

[Qemu-devel] [PATCH 02/13] qemu-socket: drop pointless allocation

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/qemu-sockets.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 364bd8c..d73c112 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -876,7 +876,6 @@ SocketAddress *socket_parse(const char *str, Error **errp)
         }
     } else {
         addr->kind = SOCKET_ADDRESS_KIND_INET;
-        addr->inet = g_new(InetSocketAddress, 1);
         addr->inet = inet_parse(str, errp);
         if (addr->inet == NULL) {
             goto fail;
-- 
1.7.9.7

[Qemu-devel] [PATCH 03/13] qemu-socket: catch monitor_get_fd failures

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/qemu-sockets.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d73c112..7776016 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -908,6 +908,9 @@ int socket_connect(SocketAddress *addr, Error **errp,
 
     case SOCKET_ADDRESS_KIND_FD:
         fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
+        if (fd < 0) {
+            break;
+        }
         if (callback) {
             qemu_set_nonblock(fd);
             callback(fd, opaque);
-- 
1.7.9.7

Re: [Qemu-devel] [Qemu-trivial] [PATCH 03/13] qemu-socket: catch monitor_get_fd failures

[Michael Tokarev]

21.06.2013 14:38, Gerd Hoffmann wrote:
>      case SOCKET_ADDRESS_KIND_FD:
>          fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
> +        if (fd < 0) {
> +            break;
> +        }
>          if (callback) {
>              qemu_set_nonblock(fd);
>              callback(fd, opaque);

     case SOCKET_ADDRESS_KIND_FD:
         fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
-        if (callback) {
+        if (fd >= 0 && callback) {
             qemu_set_nonblock(fd);
             callback(fd, opaque);
         }

?

Thanks,

/mjt

[Qemu-devel] [PATCH 04/13] qemu-char: check optional fields using has_*

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 2c3cfe6..ee7c70e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3500,7 +3500,7 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
 {
     HANDLE out;
 
-    if (file->in) {
+    if (file->has_in) {
         error_setg(errp, "input file not supported");
         return NULL;
     }
@@ -3551,7 +3551,7 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
         return NULL;
     }
 
-    if (file->in) {
+    if (file->has_in) {
         flags = O_RDONLY;
         in = qmp_chardev_open_file_source(file->in, flags, errp);
         if (error_is_set(errp)) {
-- 
1.7.9.7

[Qemu-devel] [PATCH 05/13] qemu-char: use more specific error_setg_* variants

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index ee7c70e..cfc68cd 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2604,7 +2604,7 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
 
     memset(&ss, 0, ss_len);
     if (getsockname(fd, (struct sockaddr *) &ss, &ss_len) != 0) {
-        error_setg(errp, "getsockname: %s", strerror(errno));
+        error_setg_errno(errp, errno, "getsockname");
         return NULL;
     }
 
@@ -3536,7 +3536,7 @@ static int qmp_chardev_open_file_source(char *src, int flags,
 
     TFR(fd = qemu_open(src, flags, 0666));
     if (fd == -1) {
-        error_setg(errp, "open %s: %s", src, strerror(errno));
+        error_setg_file_open(errp, errno, src);
     }
     return fd;
 }
-- 
1.7.9.7

[Qemu-devel] [PATCH 06/13] qemu-char: print notification to stderr

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index cfc68cd..3e0044b 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2666,8 +2666,8 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
     }
 
     if (is_listen && is_waitconnect) {
-        printf("QEMU waiting for connection on: %s\n",
-               chr->filename);
+        fprintf(stderr, "QEMU waiting for connection on: %s\n",
+                chr->filename);
         tcp_chr_accept(s->listen_chan, G_IO_IN, chr);
         qemu_set_nonblock(s->listen_fd);
     }
-- 
1.7.9.7

[Qemu-devel] [PATCH 07/13] qemu-char: fix documentation for telnet+wait socket flags

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qapi-schema.json |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index a80ee40..a29ce57 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3185,10 +3185,11 @@
 # @addr: socket address to listen on (server=true)
 #        or connect to (server=false)
 # @server: #optional create server socket (default: true)
-# @wait: #optional wait for connect (not used for server
-#        sockets, default: false)
+# @wait: #optional wait for incoming connection on server
+#        sockets (default: false).
 # @nodelay: #optional set TCP_NODELAY socket option (default: false)
-# @telnet: #optional enable telnet protocol (default: false)
+# @telnet: #optional enable telnet protocol on server
+#          sockets (default: false)
 #
 # Since: 1.4
 ##
-- 
1.7.9.7

[Qemu-devel] [PATCH 08/13] qemu-char: don't leak opts on error

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 3e0044b..5751391 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3185,7 +3185,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
     if (i == NULL) {
         error_setg(errp, "chardev: backend \"%s\" not found",
                    qemu_opt_get(opts, "backend"));
-        return NULL;
+        goto err;
     }
 
     if (!cd->open) {
-- 
1.7.9.7

[Qemu-devel] [PATCH 09/13] qemu-char: use ChardevBackendKind in in CharDriver

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 5751391..1c0903f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3127,7 +3127,7 @@ typedef struct CharDriver {
     /* old, pre qapi */
     CharDriverState *(*open)(QemuOpts *opts);
     /* new, qapi-based */
-    int kind;
+    ChardevBackendKind kind;
     void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
 } CharDriver;
 
-- 
1.7.9.7

Re: [Qemu-devel] [Qemu-trivial] [PATCH 09/13] qemu-char: use ChardevBackendKind in in CharDriver

[Michael Tokarev]

21.06.2013 14:38, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qemu-char.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 5751391..1c0903f 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3127,7 +3127,7 @@ typedef struct CharDriver {
>      /* old, pre qapi */
>      CharDriverState *(*open)(QemuOpts *opts);
>      /* new, qapi-based */
> -    int kind;
> +    ChardevBackendKind kind;
>      void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
>  } CharDriver;

How about switching argument of register_char_driver_qapi() to the same
type as well?

Thanks,

/mjt

[Qemu-devel] [PATCH 10/13] qemu-char: minor mux chardev fixes

[Gerd Hoffmann]

mux failure path has a memory leak.  creating a mux chardev can't
fail though, so just assert() that instead of fixing an error path
which never ever runs anyway ...

Also fix bid being leaked while being at it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 1c0903f..65c0c48 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3193,7 +3193,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
         ChardevBackend *backend = g_new0(ChardevBackend, 1);
         ChardevReturn *ret = NULL;
         const char *id = qemu_opts_id(opts);
-        const char *bid = NULL;
+        char *bid = NULL;
 
         if (qemu_opt_get_bool(opts, "mux", 0)) {
             bid = g_strdup_printf("%s-base", id);
@@ -3220,9 +3220,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
             backend->kind = CHARDEV_BACKEND_KIND_MUX;
             backend->mux->chardev = g_strdup(bid);
             ret = qmp_chardev_add(id, backend, errp);
-            if (error_is_set(errp)) {
-                goto qapi_out;
-            }
+            assert(!error_is_set(errp));
         }
 
         chr = qemu_chr_find(id);
@@ -3230,6 +3228,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
     qapi_out:
         qapi_free_ChardevBackend(backend);
         qapi_free_ChardevReturn(ret);
+        g_free(bid);
         return chr;
     }
 
-- 
1.7.9.7

[Qemu-devel] [PATCH 11/13] qemu-char: add -chardev mux support

[Gerd Hoffmann]

Allow to explicitly create mux chardevs on the command line,
like you can using QMP.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index 65c0c48..c83ff21 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3122,6 +3122,19 @@ static void qemu_chr_parse_memory(QemuOpts *opts, ChardevBackend *backend,
     }
 }
 
+static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
+                               Error **errp)
+{
+    const char *chardev = qemu_opt_get(opts, "chardev");
+
+    if (chardev == NULL) {
+        error_setg(errp, "chardev: mux: no chardev given");
+        return;
+    }
+    backend->mux = g_new0(ChardevMux, 1);
+    backend->mux->chardev = g_strdup(chardev);
+}
+
 typedef struct CharDriver {
     const char *name;
     /* old, pre qapi */
@@ -3488,6 +3501,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "size",
             .type = QEMU_OPT_SIZE,
+        },{
+            .name = "chardev",
+            .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },
@@ -3778,6 +3794,8 @@ static void register_types(void)
     register_char_driver_qapi("console", CHARDEV_BACKEND_KIND_CONSOLE, NULL);
     register_char_driver_qapi("pipe", CHARDEV_BACKEND_KIND_PIPE,
                               qemu_chr_parse_pipe);
+    register_char_driver_qapi("mux", CHARDEV_BACKEND_KIND_MUX,
+                              qemu_chr_parse_mux);
 }
 
 type_init(register_types);
-- 
1.7.9.7

[Qemu-devel] [PATCH 12/13] qemu-char: report udb backend errors

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index c83ff21..6b4daf3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2255,6 +2255,8 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
 
     fd = inet_dgram_opts(opts, &local_err);
     if (fd < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return NULL;
     }
     return qemu_chr_open_udp_fd(fd);
-- 
1.7.9.7

[Qemu-devel] [PATCH 13/13] qemu-socket: don't leak opts on error

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/qemu-sockets.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 7776016..29a3564 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -971,7 +971,7 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
 
     default:
         error_setg(errp, "socket type unsupported for datagram");
-        return -1;
+        fd = -1;
     }
     qemu_opts_del(opts);
     return fd;
-- 
1.7.9.7

Re: [Qemu-devel] [Qemu-trivial] [PATCH 00/13] A bunch of little socket + chardev fixes

[Michael Tokarev]

21.06.2013 14:38, Gerd Hoffmann wrote:
>   Hi,
> 
> Here is a colletion of little bugfixes and minor improvements for the
> chardev and socket code.

Thank you Gerd!  You had me busy for some time verifying it all ;)

The series looks fine, except:

> Gerd Hoffmann (13):
>   qemu-socket: zero-initialize SocketAddress

Maybe we should drop the =NULL initializer as well, suggested
patch included.

>   qemu-socket: drop pointless allocation
>   qemu-socket: catch monitor_get_fd failures

A nitpick, suggested patch included.

>   qemu-char: check optional fields using has_*
>   qemu-char: use more specific error_setg_* variants
>   qemu-char: print notification to stderr
>   qemu-char: fix documentation for telnet+wait socket flags
>   qemu-char: don't leak opts on error
>   qemu-char: use ChardevBackendKind in in CharDriver

In other place int is used instead of this type.
(sysemu/sysemu.h where the prototype is declared
#includes qapi-types.h)

>   qemu-char: minor mux chardev fixes
>   qemu-char: add -chardev mux support
>   qemu-char: report udb backend errors

syntax error in the description, I can fix that when applying

>   qemu-socket: don't leak opts on error

Looks good.  I can fix all the above if you want :)

Thanks!

/mjt