Re: [Qemu-devel] [PATCH] Delete AF_UNIX socket after close

[Daniel P . Berrangé]

On Mon, May 21, 2018 at 02:10:18PM +0300, Pavel Balaev wrote:
> Hello,
> 
> Since version 2.12.0 AF_UNIX socket created for QMP exchange is not
> deleted on instance shutdown.
> 
> This is due to the fact that function qio_channel_socket_finalize() is
> called after qio_channel_socket_close().

Hmm, finalize() has always been called as the last thing on the
object. I wonder if the problem was that previously we simply
never called close() at all, in some case and relied on finalize
closing the socket ?

Either way the current code was clearly broken

> Signed-off-by: Pavel Balaev 
> ---
>  include/qemu/sockets.h |  1 -
>  io/channel-socket.c| 40 +++-
>  util/qemu-sockets.c| 21 -
>  3 files changed, 15 insertions(+), 47 deletions(-)
> 
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 8140fea685..a786bd76d7 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -42,7 +42,6 @@ int unix_connect(const char *path, Error **errp);
>  SocketAddress *socket_parse(const char *str, Error **errp);
>  int socket_connect(SocketAddress *addr, Error **errp);
>  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 57cfb4d3a6..3c88ca4130 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -383,30 +383,6 @@ static void qio_channel_socket_init(Object *obj)
>  ioc->fd = -1;
>  }
>  
> -static void qio_channel_socket_finalize(Object *obj)
> -{
> -QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
> -
> -if (ioc->fd != -1) {
> -QIOChannel *ioc_local = QIO_CHANNEL(ioc);
> -if (qio_channel_has_feature(ioc_local, QIO_CHANNEL_FEATURE_LISTEN)) {
> -Error *err = NULL;
> -
> -socket_listen_cleanup(ioc->fd, );
> -if (err) {
> -error_report_err(err);
> -err = NULL;
> -}
> -}
> -#ifdef WIN32
> -WSAEventSelect(ioc->fd, NULL, 0);
> -#endif
> -closesocket(ioc->fd);
> -ioc->fd = -1;
> -}
> -}

This is not right - we can't assume that the owner has definitely
called close().  Some codepaths might call it, others might not.
So we must have a finalize method that cleans up.

> -
> -
>  #ifndef WIN32
>  static void qio_channel_socket_copy_fds(struct msghdr *msg,
>  int **fds, size_t *nfds)
> @@ -687,6 +663,8 @@ qio_channel_socket_close(QIOChannel *ioc,
>  QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
>  
>  if (sioc->fd != -1) {
> +SocketAddress *addr = socket_local_address(sioc->fd, errp);
> +
>  #ifdef WIN32
>  WSAEventSelect(sioc->fd, NULL, 0);
>  #endif
> @@ -697,6 +675,19 @@ qio_channel_socket_close(QIOChannel *ioc,
>  return -1;
>  }
>  sioc->fd = -1;
> +
> +if (addr && addr->type == SOCKET_ADDRESS_TYPE_UNIX
> +&& addr->u.q_unix.path) {
> +if (unlink(addr->u.q_unix.path) < 0 && errno != ENOENT) {
> +error_setg_errno(errp, errno,
> + "Failed to unlink socket %s",
> + addr->u.q_unix.path);
> +}
> +}
> +
> +if (addr) {
> +qapi_free_SocketAddress(addr);
> +}

This bit is good though.

>  }
>  return 0;
>  }
> @@ -770,7 +761,6 @@ static const TypeInfo qio_channel_socket_info = {
>  .name = TYPE_QIO_CHANNEL_SOCKET,
>  .instance_size = sizeof(QIOChannelSocket),
>  .instance_init = qio_channel_socket_init,
> -.instance_finalize = qio_channel_socket_finalize,
>  .class_init = qio_channel_socket_class_init,
>  };
>  
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 8bd8bb64eb..aedcb5b9c0 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1120,27 +1120,6 @@ 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) {
> -return;
> -}
> -
> -if (addr->type == SOCKET_ADDRESS_TYPE_UNIX
> -&& addr->u.q_unix.path) {
> -if (unlink(addr->u.q_unix.path) < 0 && errno != ENOENT) {
> -error_setg_errno(errp, errno,
> - "Failed to unlink socket %s",
> - addr->u.q_unix.path);
> -}
> -}
> -
> -qapi_free_SocketAddress(addr);
> -}
> -
>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
>  {
>  int fd;
> -- 
> 2.16.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange 

[Qemu-devel] [PATCH] Delete AF_UNIX socket after close

[Pavel Balaev]

Hello,

Since version 2.12.0 AF_UNIX socket created for QMP exchange is not
deleted on instance shutdown.

This is due to the fact that function qio_channel_socket_finalize() is
called after qio_channel_socket_close().


Signed-off-by: Pavel Balaev 
---
 include/qemu/sockets.h |  1 -
 io/channel-socket.c| 40 +++-
 util/qemu-sockets.c| 21 -
 3 files changed, 15 insertions(+), 47 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 8140fea685..a786bd76d7 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -42,7 +42,6 @@ int unix_connect(const char *path, Error **errp);
 SocketAddress *socket_parse(const char *str, Error **errp);
 int socket_connect(SocketAddress *addr, Error **errp);
 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 57cfb4d3a6..3c88ca4130 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -383,30 +383,6 @@ static void qio_channel_socket_init(Object *obj)
 ioc->fd = -1;
 }
 
-static void qio_channel_socket_finalize(Object *obj)
-{
-QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
-
-if (ioc->fd != -1) {
-QIOChannel *ioc_local = QIO_CHANNEL(ioc);
-if (qio_channel_has_feature(ioc_local, QIO_CHANNEL_FEATURE_LISTEN)) {
-Error *err = NULL;
-
-socket_listen_cleanup(ioc->fd, );
-if (err) {
-error_report_err(err);
-err = NULL;
-}
-}
-#ifdef WIN32
-WSAEventSelect(ioc->fd, NULL, 0);
-#endif
-closesocket(ioc->fd);
-ioc->fd = -1;
-}
-}
-
-
 #ifndef WIN32
 static void qio_channel_socket_copy_fds(struct msghdr *msg,
 int **fds, size_t *nfds)
@@ -687,6 +663,8 @@ qio_channel_socket_close(QIOChannel *ioc,
 QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
 
 if (sioc->fd != -1) {
+SocketAddress *addr = socket_local_address(sioc->fd, errp);
+
 #ifdef WIN32
 WSAEventSelect(sioc->fd, NULL, 0);
 #endif
@@ -697,6 +675,19 @@ qio_channel_socket_close(QIOChannel *ioc,
 return -1;
 }
 sioc->fd = -1;
+
+if (addr && addr->type == SOCKET_ADDRESS_TYPE_UNIX
+&& addr->u.q_unix.path) {
+if (unlink(addr->u.q_unix.path) < 0 && errno != ENOENT) {
+error_setg_errno(errp, errno,
+ "Failed to unlink socket %s",
+ addr->u.q_unix.path);
+}
+}
+
+if (addr) {
+qapi_free_SocketAddress(addr);
+}
 }
 return 0;
 }
@@ -770,7 +761,6 @@ static const TypeInfo qio_channel_socket_info = {
 .name = TYPE_QIO_CHANNEL_SOCKET,
 .instance_size = sizeof(QIOChannelSocket),
 .instance_init = qio_channel_socket_init,
-.instance_finalize = qio_channel_socket_finalize,
 .class_init = qio_channel_socket_class_init,
 };
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8bd8bb64eb..aedcb5b9c0 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1120,27 +1120,6 @@ 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) {
-return;
-}
-
-if (addr->type == SOCKET_ADDRESS_TYPE_UNIX
-&& addr->u.q_unix.path) {
-if (unlink(addr->u.q_unix.path) < 0 && errno != ENOENT) {
-error_setg_errno(errp, errno,
- "Failed to unlink socket %s",
- addr->u.q_unix.path);
-}
-}
-
-qapi_free_SocketAddress(addr);
-}
-
 int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
 {
 int fd;
-- 
2.16.1