[Qemu-devel] [PATCH v2 0/3] chardev cleanups

[Qemu-devel] [PATCH v2 0/3] chardev cleanups

[marcandre.lureau]

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

Hi

A small series to do some chardev cleanup when removing them and
leaving qemu.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1347077

v1->v2:
- use atexit() for qemu_chr_cleanup()
- add missing braces

Marc-André Lureau (3):
  char: clean up remaining chardevs when leaving
  socket: add listen feature
  socket: unlink unix socket on remove

 include/io/channel.h           |  1 +
 include/qemu/sockets.h         |  1 +
 io/channel-socket.c            | 17 +++++++++++++++++
 qemu-char.c                    | 11 +++++++++++
 tests/test-io-channel-socket.c |  2 +-
 util/qemu-sockets.c            | 18 ++++++++++++++++++
 6 files changed, 49 insertions(+), 1 deletion(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/3] char: clean up remaining chardevs when leaving

[marcandre.lureau]

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

This helps to remove various chardev resources leaks when leaving qemu.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-char.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index c926e9a..98dcd49 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4541,6 +4541,15 @@ void qmp_chardev_remove(const char *id, Error **errp)
     qemu_chr_delete(chr);
 }
 
+static void qemu_chr_cleanup(void)
+{
+    CharDriverState *chr;
+
+    QTAILQ_FOREACH(chr, &chardevs, next) {
+        qemu_chr_delete(chr);
+    }
+}
+
 static void register_types(void)
 {
     register_char_driver("null", CHARDEV_BACKEND_KIND_NULL, NULL,
@@ -4587,6 +4596,8 @@ static void register_types(void)
      * is specified
      */
     qemu_add_machine_init_done_notifier(&muxes_realize_notify);
+
+    atexit(qemu_chr_cleanup);
 }
 
 type_init(register_types);
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/3] socket: add listen feature

[marcandre.lureau]

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

Add a flag to tell whether the channel socket is listening.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/io/channel.h | 1 +
 io/channel-socket.c  | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/io/channel.h b/include/io/channel.h
index d37acd2..e52f059 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -42,6 +42,7 @@ typedef enum QIOChannelFeature QIOChannelFeature;
 enum QIOChannelFeature {
     QIO_CHANNEL_FEATURE_FD_PASS  = (1 << 0),
     QIO_CHANNEL_FEATURE_SHUTDOWN = (1 << 1),
+    QIO_CHANNEL_FEATURE_LISTEN   = (1 << 2),
 };
 
 
diff --git a/io/channel-socket.c b/io/channel-socket.c
index ca8bc20..1cd5848 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -71,6 +71,9 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc,
                           int fd,
                           Error **errp)
 {
+    int val;
+    socklen_t len = sizeof(val);
+
     if (sioc->fd != -1) {
         error_setg(errp, "Socket is already open");
         return -1;
@@ -106,6 +109,10 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc,
         ioc->features |= (1 << QIO_CHANNEL_FEATURE_FD_PASS);
     }
 #endif /* WIN32 */
+    if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &val, &len) == 0 && val) {
+        QIOChannel *ioc = QIO_CHANNEL(sioc);
+        ioc->features |= (1 << QIO_CHANNEL_FEATURE_LISTEN);
+    }
 
     return 0;
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/3] socket: unlink unix socket on remove

[marcandre.lureau]

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

qemu leaves unix socket files behind when removing a listening chardev
or leaving. qemu could clean that up, even if doing so isn't race-free.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1347077

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/sockets.h         |  1 +
 io/channel-socket.c            | 10 ++++++++++
 tests/test-io-channel-socket.c |  2 +-
 util/qemu-sockets.c            | 18 ++++++++++++++++++
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 1bd9218..5dd2648 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -51,6 +51,7 @@ SocketAddress *socket_parse(const char *str, Error **errp);
 int socket_connect(SocketAddress *addr, Error **errp,
                    NonBlockingConnectHandler *callback, void *opaque);
 int socket_listen(SocketAddress *addr, Error **errp);
+void socket_listen_cleanup(int fd, Error **errp);
 int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
 
 /* Old, ipv4 only bits.  Don't use for new code. */
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 1cd5848..6ec87f8 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -400,7 +400,17 @@ static void qio_channel_socket_init(Object *obj)
 static void qio_channel_socket_finalize(Object *obj)
 {
     QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
+
     if (ioc->fd != -1) {
+        if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) {
+            Error *err = NULL;
+
+            socket_listen_cleanup(ioc->fd, &err);
+            if (err) {
+                error_report_err(err);
+                err = NULL;
+            }
+        }
 #ifdef WIN32
         WSAEventSelect(ioc->fd, NULL, 0);
 #endif
diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index 855306b..f73e063 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -383,7 +383,7 @@ static void test_io_channel_unix(bool async)
 
     qapi_free_SocketAddress(listen_addr);
     qapi_free_SocketAddress(connect_addr);
-    unlink(TEST_SOCKET);
+    g_assert(g_file_test(TEST_SOCKET, G_FILE_TEST_EXISTS) == FALSE);
 }
 
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 0d6cd1f..5d03695 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -997,6 +997,24 @@ int socket_listen(SocketAddress *addr, Error **errp)
     return fd;
 }
 
+void socket_listen_cleanup(int fd, Error **errp)
+{
+    SocketAddress *addr;
+
+    addr = socket_local_address(fd, errp);
+
+    if (addr->type == SOCKET_ADDRESS_KIND_UNIX
+        && addr->u.q_unix.data->path) {
+        if (unlink(addr->u.q_unix.data->path) < 0 && errno != ENOENT) {
+            error_setg_errno(errp, errno,
+                             "Failed to unlink socket %s",
+                             addr->u.q_unix.data->path);
+        }
+    }
+
+    g_free(addr);
+}
+
 int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
 {
     int fd;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 0/3] chardev cleanups

[Paolo Bonzini]

On 16/06/2016 21:28, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi
> 
> A small series to do some chardev cleanup when removing them and
> leaving qemu.
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1347077
> 
> v1->v2:
> - use atexit() for qemu_chr_cleanup()
> - add missing braces
> 
> Marc-André Lureau (3):
>   char: clean up remaining chardevs when leaving
>   socket: add listen feature
>   socket: unlink unix socket on remove
> 
>  include/io/channel.h           |  1 +
>  include/qemu/sockets.h         |  1 +
>  io/channel-socket.c            | 17 +++++++++++++++++
>  qemu-char.c                    | 11 +++++++++++
>  tests/test-io-channel-socket.c |  2 +-
>  util/qemu-sockets.c            | 18 ++++++++++++++++++
>  6 files changed, 49 insertions(+), 1 deletion(-)
> 

Looks good, I'll queue them if Dan acks patch 2.

Paolo

Re: [Qemu-devel] [PATCH v2 3/3] socket: unlink unix socket on remove

[Michael S. Tsirkin]

On Thu, Jun 16, 2016 at 09:28:52PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> qemu leaves unix socket files behind when removing a listening chardev
> or leaving. qemu could clean that up, even if doing so isn't race-free.
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1347077
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

We normally only unlink what we created.

unlinking files we didn't create
looks like a silent change that might easily break
existing users.


> ---
>  include/qemu/sockets.h         |  1 +
>  io/channel-socket.c            | 10 ++++++++++
>  tests/test-io-channel-socket.c |  2 +-
>  util/qemu-sockets.c            | 18 ++++++++++++++++++
>  4 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 1bd9218..5dd2648 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -51,6 +51,7 @@ SocketAddress *socket_parse(const char *str, Error **errp);
>  int socket_connect(SocketAddress *addr, Error **errp,
>                     NonBlockingConnectHandler *callback, void *opaque);
>  int socket_listen(SocketAddress *addr, Error **errp);
> +void socket_listen_cleanup(int fd, Error **errp);
>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>  
>  /* Old, ipv4 only bits.  Don't use for new code. */
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 1cd5848..6ec87f8 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -400,7 +400,17 @@ static void qio_channel_socket_init(Object *obj)
>  static void qio_channel_socket_finalize(Object *obj)
>  {
>      QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
> +
>      if (ioc->fd != -1) {
> +        if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) {
> +            Error *err = NULL;
> +
> +            socket_listen_cleanup(ioc->fd, &err);
> +            if (err) {
> +                error_report_err(err);
> +                err = NULL;
> +            }
> +        }
>  #ifdef WIN32
>          WSAEventSelect(ioc->fd, NULL, 0);
>  #endif
> diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
> index 855306b..f73e063 100644
> --- a/tests/test-io-channel-socket.c
> +++ b/tests/test-io-channel-socket.c
> @@ -383,7 +383,7 @@ static void test_io_channel_unix(bool async)
>  
>      qapi_free_SocketAddress(listen_addr);
>      qapi_free_SocketAddress(connect_addr);
> -    unlink(TEST_SOCKET);
> +    g_assert(g_file_test(TEST_SOCKET, G_FILE_TEST_EXISTS) == FALSE);
>  }
>  
>  
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 0d6cd1f..5d03695 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -997,6 +997,24 @@ int socket_listen(SocketAddress *addr, Error **errp)
>      return fd;
>  }
>  
> +void socket_listen_cleanup(int fd, Error **errp)
> +{
> +    SocketAddress *addr;
> +
> +    addr = socket_local_address(fd, errp);
> +
> +    if (addr->type == SOCKET_ADDRESS_KIND_UNIX
> +        && addr->u.q_unix.data->path) {
> +        if (unlink(addr->u.q_unix.data->path) < 0 && errno != ENOENT) {
> +            error_setg_errno(errp, errno,
> +                             "Failed to unlink socket %s",
> +                             addr->u.q_unix.data->path);
> +        }
> +    }
> +
> +    g_free(addr);
> +}
> +
>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
>  {
>      int fd;
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH v2 3/3] socket: unlink unix socket on remove

[Michael S. Tsirkin]

On Thu, Jun 23, 2016 at 07:41:55AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 16, 2016 at 09:28:52PM +0200, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > qemu leaves unix socket files behind when removing a listening chardev
> > or leaving. qemu could clean that up, even if doing so isn't race-free.
> > 
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1347077
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> We normally only unlink what we created.
> 
> unlinking files we didn't create
> looks like a silent change that might easily break
> existing users.

Maybe what you want is a need_unlink feature.
Set it for unix sockets only, that would make some sense.


> 
> > ---
> >  include/qemu/sockets.h         |  1 +
> >  io/channel-socket.c            | 10 ++++++++++
> >  tests/test-io-channel-socket.c |  2 +-
> >  util/qemu-sockets.c            | 18 ++++++++++++++++++
> >  4 files changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> > index 1bd9218..5dd2648 100644
> > --- a/include/qemu/sockets.h
> > +++ b/include/qemu/sockets.h
> > @@ -51,6 +51,7 @@ SocketAddress *socket_parse(const char *str, Error **errp);
> >  int socket_connect(SocketAddress *addr, Error **errp,
> >                     NonBlockingConnectHandler *callback, void *opaque);
> >  int socket_listen(SocketAddress *addr, Error **errp);
> > +void socket_listen_cleanup(int fd, Error **errp);
> >  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
> >  
> >  /* Old, ipv4 only bits.  Don't use for new code. */
> > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > index 1cd5848..6ec87f8 100644
> > --- a/io/channel-socket.c
> > +++ b/io/channel-socket.c
> > @@ -400,7 +400,17 @@ static void qio_channel_socket_init(Object *obj)
> >  static void qio_channel_socket_finalize(Object *obj)
> >  {
> >      QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
> > +
> >      if (ioc->fd != -1) {
> > +        if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) {
> > +            Error *err = NULL;
> > +
> > +            socket_listen_cleanup(ioc->fd, &err);
> > +            if (err) {
> > +                error_report_err(err);
> > +                err = NULL;
> > +            }
> > +        }
> >  #ifdef WIN32
> >          WSAEventSelect(ioc->fd, NULL, 0);
> >  #endif
> > diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
> > index 855306b..f73e063 100644
> > --- a/tests/test-io-channel-socket.c
> > +++ b/tests/test-io-channel-socket.c
> > @@ -383,7 +383,7 @@ static void test_io_channel_unix(bool async)
> >  
> >      qapi_free_SocketAddress(listen_addr);
> >      qapi_free_SocketAddress(connect_addr);
> > -    unlink(TEST_SOCKET);
> > +    g_assert(g_file_test(TEST_SOCKET, G_FILE_TEST_EXISTS) == FALSE);
> >  }
> >  
> >  
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 0d6cd1f..5d03695 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -997,6 +997,24 @@ int socket_listen(SocketAddress *addr, Error **errp)
> >      return fd;
> >  }
> >  
> > +void socket_listen_cleanup(int fd, Error **errp)
> > +{
> > +    SocketAddress *addr;
> > +
> > +    addr = socket_local_address(fd, errp);
> > +
> > +    if (addr->type == SOCKET_ADDRESS_KIND_UNIX
> > +        && addr->u.q_unix.data->path) {
> > +        if (unlink(addr->u.q_unix.data->path) < 0 && errno != ENOENT) {
> > +            error_setg_errno(errp, errno,
> > +                             "Failed to unlink socket %s",
> > +                             addr->u.q_unix.data->path);
> > +        }
> > +    }
> > +
> > +    g_free(addr);
> > +}
> > +
> >  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
> >  {
> >      int fd;
> > -- 
> > 2.7.4
> > 

Re: [Qemu-devel] [PATCH v2 3/3] socket: unlink unix socket on remove

[Marc-André Lureau]

Hi

----- Original Message -----
> On Thu, Jun 23, 2016 at 07:41:55AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jun 16, 2016 at 09:28:52PM +0200, marcandre.lureau@redhat.com
> > wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > > qemu leaves unix socket files behind when removing a listening chardev
> > > or leaving. qemu could clean that up, even if doing so isn't race-free.
> > > 
> > > Fixes:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1347077
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > We normally only unlink what we created.

What do you mean by "we", qemu currently unlink before creating the unix socket. No further cleanup.
 
> > 
> > unlinking files we didn't create
> > looks like a silent change that might easily break
> > existing users.

That shouldn't remove files we didn't create. But a bad user could recreate the unix socket after qemu did, and that would remove it. I don't think that's a valid use case though, so it should be fine.

> Maybe what you want is a need_unlink feature.
> Set it for unix sockets only, that would make some sense.

Oh perhaps what you mean is that if the fd was passed, we should cleanup the unix socket? Yes, I think we should do that then. I'll update the series.

thanks
 
> 
> > 
> > > ---
> > >  include/qemu/sockets.h         |  1 +
> > >  io/channel-socket.c            | 10 ++++++++++
> > >  tests/test-io-channel-socket.c |  2 +-
> > >  util/qemu-sockets.c            | 18 ++++++++++++++++++
> > >  4 files changed, 30 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> > > index 1bd9218..5dd2648 100644
> > > --- a/include/qemu/sockets.h
> > > +++ b/include/qemu/sockets.h
> > > @@ -51,6 +51,7 @@ SocketAddress *socket_parse(const char *str, Error
> > > **errp);
> > >  int socket_connect(SocketAddress *addr, Error **errp,
> > >                     NonBlockingConnectHandler *callback, void *opaque);
> > >  int socket_listen(SocketAddress *addr, Error **errp);
> > > +void socket_listen_cleanup(int fd, Error **errp);
> > >  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error
> > >  **errp);
> > >  
> > >  /* Old, ipv4 only bits.  Don't use for new code. */
> > > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > > index 1cd5848..6ec87f8 100644
> > > --- a/io/channel-socket.c
> > > +++ b/io/channel-socket.c
> > > @@ -400,7 +400,17 @@ static void qio_channel_socket_init(Object *obj)
> > >  static void qio_channel_socket_finalize(Object *obj)
> > >  {
> > >      QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
> > > +
> > >      if (ioc->fd != -1) {
> > > +        if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) {
> > > +            Error *err = NULL;
> > > +
> > > +            socket_listen_cleanup(ioc->fd, &err);
> > > +            if (err) {
> > > +                error_report_err(err);
> > > +                err = NULL;
> > > +            }
> > > +        }
> > >  #ifdef WIN32
> > >          WSAEventSelect(ioc->fd, NULL, 0);
> > >  #endif
> > > diff --git a/tests/test-io-channel-socket.c
> > > b/tests/test-io-channel-socket.c
> > > index 855306b..f73e063 100644
> > > --- a/tests/test-io-channel-socket.c
> > > +++ b/tests/test-io-channel-socket.c
> > > @@ -383,7 +383,7 @@ static void test_io_channel_unix(bool async)
> > >  
> > >      qapi_free_SocketAddress(listen_addr);
> > >      qapi_free_SocketAddress(connect_addr);
> > > -    unlink(TEST_SOCKET);
> > > +    g_assert(g_file_test(TEST_SOCKET, G_FILE_TEST_EXISTS) == FALSE);
> > >  }
> > >  
> > >  
> > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > > index 0d6cd1f..5d03695 100644
> > > --- a/util/qemu-sockets.c
> > > +++ b/util/qemu-sockets.c
> > > @@ -997,6 +997,24 @@ int socket_listen(SocketAddress *addr, Error **errp)
> > >      return fd;
> > >  }
> > >  
> > > +void socket_listen_cleanup(int fd, Error **errp)
> > > +{
> > > +    SocketAddress *addr;
> > > +
> > > +    addr = socket_local_address(fd, errp);
> > > +
> > > +    if (addr->type == SOCKET_ADDRESS_KIND_UNIX
> > > +        && addr->u.q_unix.data->path) {
> > > +        if (unlink(addr->u.q_unix.data->path) < 0 && errno != ENOENT) {
> > > +            error_setg_errno(errp, errno,
> > > +                             "Failed to unlink socket %s",
> > > +                             addr->u.q_unix.data->path);
> > > +        }
> > > +    }
> > > +
> > > +    g_free(addr);
> > > +}
> > > +
> > >  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error
> > >  **errp)
> > >  {
> > >      int fd;
> > > --
> > > 2.7.4
> > > 
> 

Re: [Qemu-devel] [PATCH v2 1/3] char: clean up remaining chardevs when leaving

[Daniel P. Berrange]

On Thu, Jun 16, 2016 at 09:28:50PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This helps to remove various chardev resources leaks when leaving qemu.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qemu-char.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 2/3] socket: add listen feature

[Daniel P. Berrange]

On Thu, Jun 16, 2016 at 09:28:51PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add a flag to tell whether the channel socket is listening.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/io/channel.h | 1 +
>  io/channel-socket.c  | 7 +++++++
>  2 files changed, 8 insertions(+)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 3/3] socket: unlink unix socket on remove

[Daniel P. Berrange]

On Thu, Jun 16, 2016 at 09:28:52PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> qemu leaves unix socket files behind when removing a listening chardev
> or leaving. qemu could clean that up, even if doing so isn't race-free.
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1347077
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/sockets.h         |  1 +
>  io/channel-socket.c            | 10 ++++++++++
>  tests/test-io-channel-socket.c |  2 +-
>  util/qemu-sockets.c            | 18 ++++++++++++++++++
>  4 files changed, 30 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 0/3] chardev cleanups

[Daniel P. Berrange]

On Fri, Jun 17, 2016 at 02:34:41PM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 21:28, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Hi
> > 
> > A small series to do some chardev cleanup when removing them and
> > leaving qemu.
> > 
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1347077
> > 
> > v1->v2:
> > - use atexit() for qemu_chr_cleanup()
> > - add missing braces
> > 
> > Marc-André Lureau (3):
> >   char: clean up remaining chardevs when leaving
> >   socket: add listen feature
> >   socket: unlink unix socket on remove
> > 
> >  include/io/channel.h           |  1 +
> >  include/qemu/sockets.h         |  1 +
> >  io/channel-socket.c            | 17 +++++++++++++++++
> >  qemu-char.c                    | 11 +++++++++++
> >  tests/test-io-channel-socket.c |  2 +-
> >  util/qemu-sockets.c            | 18 ++++++++++++++++++
> >  6 files changed, 49 insertions(+), 1 deletion(-)
> > 
> 
> Looks good, I'll queue them if Dan acks patch 2.

Go ahead, its all fine with me.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 3/3] socket: unlink unix socket on remove

[Michael S. Tsirkin]

On Thu, Jun 23, 2016 at 05:08:03AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Thu, Jun 23, 2016 at 07:41:55AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jun 16, 2016 at 09:28:52PM +0200, marcandre.lureau@redhat.com
> > > wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > 
> > > > qemu leaves unix socket files behind when removing a listening chardev
> > > > or leaving. qemu could clean that up, even if doing so isn't race-free.
> > > > 
> > > > Fixes:
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1347077
> > > > 
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > > We normally only unlink what we created.
> 
> What do you mean by "we", qemu currently unlink before creating the unix socket. No further cleanup.
>  
> > > 
> > > unlinking files we didn't create
> > > looks like a silent change that might easily break
> > > existing users.
> 
> That shouldn't remove files we didn't create. But a bad user could recreate the unix socket after qemu did, and that would remove it. I don't think that's a valid use case though, so it should be fine.
> 
> > Maybe what you want is a need_unlink feature.
> > Set it for unix sockets only, that would make some sense.
> 
> Oh perhaps what you mean is that if the fd was passed, we should cleanup the unix socket? Yes, I think we should do that then. I'll update the series.
> 
> thanks

I'd like it better contained - that's all. So let's set a flag that says
"must unlink" as opposed to "it's listening".

> > 
> > > 
> > > > ---
> > > >  include/qemu/sockets.h         |  1 +
> > > >  io/channel-socket.c            | 10 ++++++++++
> > > >  tests/test-io-channel-socket.c |  2 +-
> > > >  util/qemu-sockets.c            | 18 ++++++++++++++++++
> > > >  4 files changed, 30 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> > > > index 1bd9218..5dd2648 100644
> > > > --- a/include/qemu/sockets.h
> > > > +++ b/include/qemu/sockets.h
> > > > @@ -51,6 +51,7 @@ SocketAddress *socket_parse(const char *str, Error
> > > > **errp);
> > > >  int socket_connect(SocketAddress *addr, Error **errp,
> > > >                     NonBlockingConnectHandler *callback, void *opaque);
> > > >  int socket_listen(SocketAddress *addr, Error **errp);
> > > > +void socket_listen_cleanup(int fd, Error **errp);
> > > >  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error
> > > >  **errp);
> > > >  
> > > >  /* Old, ipv4 only bits.  Don't use for new code. */
> > > > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > > > index 1cd5848..6ec87f8 100644
> > > > --- a/io/channel-socket.c
> > > > +++ b/io/channel-socket.c
> > > > @@ -400,7 +400,17 @@ static void qio_channel_socket_init(Object *obj)
> > > >  static void qio_channel_socket_finalize(Object *obj)
> > > >  {
> > > >      QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
> > > > +
> > > >      if (ioc->fd != -1) {
> > > > +        if (QIO_CHANNEL(ioc)->features & QIO_CHANNEL_FEATURE_LISTEN) {
> > > > +            Error *err = NULL;
> > > > +
> > > > +            socket_listen_cleanup(ioc->fd, &err);
> > > > +            if (err) {
> > > > +                error_report_err(err);
> > > > +                err = NULL;
> > > > +            }
> > > > +        }
> > > >  #ifdef WIN32
> > > >          WSAEventSelect(ioc->fd, NULL, 0);
> > > >  #endif
> > > > diff --git a/tests/test-io-channel-socket.c
> > > > b/tests/test-io-channel-socket.c
> > > > index 855306b..f73e063 100644
> > > > --- a/tests/test-io-channel-socket.c
> > > > +++ b/tests/test-io-channel-socket.c
> > > > @@ -383,7 +383,7 @@ static void test_io_channel_unix(bool async)
> > > >  
> > > >      qapi_free_SocketAddress(listen_addr);
> > > >      qapi_free_SocketAddress(connect_addr);
> > > > -    unlink(TEST_SOCKET);
> > > > +    g_assert(g_file_test(TEST_SOCKET, G_FILE_TEST_EXISTS) == FALSE);
> > > >  }
> > > >  
> > > >  
> > > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > > > index 0d6cd1f..5d03695 100644
> > > > --- a/util/qemu-sockets.c
> > > > +++ b/util/qemu-sockets.c
> > > > @@ -997,6 +997,24 @@ int socket_listen(SocketAddress *addr, Error **errp)
> > > >      return fd;
> > > >  }
> > > >  
> > > > +void socket_listen_cleanup(int fd, Error **errp)
> > > > +{
> > > > +    SocketAddress *addr;
> > > > +
> > > > +    addr = socket_local_address(fd, errp);
> > > > +
> > > > +    if (addr->type == SOCKET_ADDRESS_KIND_UNIX
> > > > +        && addr->u.q_unix.data->path) {
> > > > +        if (unlink(addr->u.q_unix.data->path) < 0 && errno != ENOENT) {
> > > > +            error_setg_errno(errp, errno,
> > > > +                             "Failed to unlink socket %s",
> > > > +                             addr->u.q_unix.data->path);
> > > > +        }
> > > > +    }
> > > > +
> > > > +    g_free(addr);
> > > > +}
> > > > +
> > > >  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error
> > > >  **errp)
> > > >  {
> > > >      int fd;
> > > > --
> > > > 2.7.4
> > > > 
> > 

Re: [Qemu-devel] [PATCH v2 3/3] socket: unlink unix socket on remove

[Marc-André Lureau]

On Thu, Jun 23, 2016 at 7:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > Maybe what you want is a need_unlink feature.
>> > Set it for unix sockets only, that would make some sense.
>>
>> Oh perhaps what you mean is that if the fd was passed, we should cleanup the unix socket? Yes, I think we should do that then. I'll update the series.

Actually it's not possible to pass a listening fd to a socket chardev
today (the path argument doesn't understand /dev/fdset), so only path
created by qemu will be cleaned up.

>
> I'd like it better contained - that's all. So let's set a flag that says
> "must unlink" as opposed to "it's listening".

You suggest to rename QIO_CHANNEL_FEATURE_LISTEN to
QIO_CHANNEL_FEATURE_LISTEN_MUST_UNLINK ? Or to add another feature
flag? I don't think that brings anything useful here.

What would you think Daniel?

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 3/3] socket: unlink unix socket on remove

[Daniel P. Berrange]

On Fri, Jun 24, 2016 at 02:08:52PM +0200, Marc-André Lureau wrote:
> On Thu, Jun 23, 2016 at 7:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > Maybe what you want is a need_unlink feature.
> >> > Set it for unix sockets only, that would make some sense.
> >>
> >> Oh perhaps what you mean is that if the fd was passed, we should cleanup the unix socket? Yes, I think we should do that then. I'll update the series.
> 
> Actually it's not possible to pass a listening fd to a socket chardev
> today (the path argument doesn't understand /dev/fdset), so only path
> created by qemu will be cleaned up.
> 
> >
> > I'd like it better contained - that's all. So let's set a flag that says
> > "must unlink" as opposed to "it's listening".
> 
> You suggest to rename QIO_CHANNEL_FEATURE_LISTEN to
> QIO_CHANNEL_FEATURE_LISTEN_MUST_UNLINK ? Or to add another feature
> flag? I don't think that brings anything useful here.

IMHO the existing QIO_CHANNEL_FEATURE_LISTEN makes more sense than
a QIO_CHANNEL_FEATURE_MUST_UNLINK, so I'd like to see your current
patches go in as is.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 3/3] socket: unlink unix socket on remove

[Michael S. Tsirkin]

On Fri, Jun 24, 2016 at 02:08:52PM +0200, Marc-André Lureau wrote:
> On Thu, Jun 23, 2016 at 7:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > Maybe what you want is a need_unlink feature.
> >> > Set it for unix sockets only, that would make some sense.
> >>
> >> Oh perhaps what you mean is that if the fd was passed, we should cleanup the unix socket? Yes, I think we should do that then. I'll update the series.
> 
> Actually it's not possible to pass a listening fd to a socket chardev
> today (the path argument doesn't understand /dev/fdset), so only path
> created by qemu will be cleaned up.
> 
> >
> > I'd like it better contained - that's all. So let's set a flag that says
> > "must unlink" as opposed to "it's listening".
> 
> You suggest to rename QIO_CHANNEL_FEATURE_LISTEN to
> QIO_CHANNEL_FEATURE_LISTEN_MUST_UNLINK ?

QIO_CHANNEL_FEATURE_UNLINK_ON_CLOSE

or something like this.

Or maybe QIO_CHANNEL_FEATURE_BOUND

> Or to add another feature
> flag? I don't think that brings anything useful here.

The point is that in the future we might be listening on sockets where
we did not bind it.  I would think that in that case, we do not want to
unlink it. So name should reflect this somehow.

> What would you think Daniel?
> 
> -- 
> Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 3/3] socket: unlink unix socket on remove

[Daniel P. Berrange]

On Sat, Jun 25, 2016 at 01:07:10AM +0300, Michael S. Tsirkin wrote:
> On Fri, Jun 24, 2016 at 02:08:52PM +0200, Marc-André Lureau wrote:
> > On Thu, Jun 23, 2016 at 7:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >> > Maybe what you want is a need_unlink feature.
> > >> > Set it for unix sockets only, that would make some sense.
> > >>
> > >> Oh perhaps what you mean is that if the fd was passed, we should cleanup the unix socket? Yes, I think we should do that then. I'll update the series.
> > 
> > Actually it's not possible to pass a listening fd to a socket chardev
> > today (the path argument doesn't understand /dev/fdset), so only path
> > created by qemu will be cleaned up.
> > 
> > >
> > > I'd like it better contained - that's all. So let's set a flag that says
> > > "must unlink" as opposed to "it's listening".
> > 
> > You suggest to rename QIO_CHANNEL_FEATURE_LISTEN to
> > QIO_CHANNEL_FEATURE_LISTEN_MUST_UNLINK ?
> 
> QIO_CHANNEL_FEATURE_UNLINK_ON_CLOSE
> 
> or something like this.
> 
> Or maybe QIO_CHANNEL_FEATURE_BOUND
> 
> > Or to add another feature
> > flag? I don't think that brings anything useful here.
> 
> The point is that in the future we might be listening on sockets where
> we did not bind it.  I would think that in that case, we do not want to
> unlink it. So name should reflect this somehow.

There is no possibility of listening on sockets without binding to them
with the current design of the code. If & when we want such support we
can adapt as needed, but I don't see any real point in trying to support
something that is impossible right now.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 1/3] char: clean up remaining chardevs when leaving

[Paolo Bonzini]

On 16/06/2016 21:28, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This helps to remove various chardev resources leaks when leaving qemu.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qemu-char.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index c926e9a..98dcd49 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -4541,6 +4541,15 @@ void qmp_chardev_remove(const char *id, Error **errp)
>      qemu_chr_delete(chr);
>  }
>  
> +static void qemu_chr_cleanup(void)
> +{
> +    CharDriverState *chr;
> +
> +    QTAILQ_FOREACH(chr, &chardevs, next) {
> +        qemu_chr_delete(chr);
> +    }
> +}

FYI, this patch is necessary on top:

diff --git a/qemu-char.c b/qemu-char.c
index 016badb..bc04ced 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4551,9 +4551,9 @@

 static void qemu_chr_cleanup(void)
 {
-    CharDriverState *chr;
+    CharDriverState *chr, *tmp;

-    QTAILQ_FOREACH(chr, &chardevs, next) {
+    QTAILQ_FOREACH_SAFE(chr, &chardevs, next, tmp) {
         qemu_chr_delete(chr);
     }
 }


(Reproducer: start QEMU with MALLOC_PERTURB_=42 and type "quit" on the
monitor).

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v2 1/3] char: clean up remaining chardevs when leaving

[Marc-André Lureau]

Hi

On Wed, Jun 29, 2016 at 12:53 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 16/06/2016 21:28, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> This helps to remove various chardev resources leaks when leaving qemu.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  qemu-char.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index c926e9a..98dcd49 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -4541,6 +4541,15 @@ void qmp_chardev_remove(const char *id, Error **errp)
>>      qemu_chr_delete(chr);
>>  }
>>
>> +static void qemu_chr_cleanup(void)
>> +{
>> +    CharDriverState *chr;
>> +
>> +    QTAILQ_FOREACH(chr, &chardevs, next) {
>> +        qemu_chr_delete(chr);
>> +    }
>> +}
>
> FYI, this patch is necessary on top:
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 016badb..bc04ced 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -4551,9 +4551,9 @@
>
>  static void qemu_chr_cleanup(void)
>  {
> -    CharDriverState *chr;
> +    CharDriverState *chr, *tmp;
>
> -    QTAILQ_FOREACH(chr, &chardevs, next) {
> +    QTAILQ_FOREACH_SAFE(chr, &chardevs, next, tmp) {
>          qemu_chr_delete(chr);
>      }
>  }
>

ack, I guess you'll have it in your pull request? squash or seperate, anyhow.

thanks



-- 
Marc-André Lureau