Skip to contentSkip to conversation

Delete AF_UNIX socket after close

PATCH Superseded Pavel Balaev 2 participants 2 messages May 21, 2018

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Pavel Balaev

author
#0 lore raw

PATCH 1/1 Delete AF_UNIX socket after close

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 <[email protected]>
---
 include/qemu/sockets.h |  1 -
 io/channel-socket.c    | 40 +++++++++++++++-------------------------
 util/qemu-sockets.c    | 21 ---------------------
 3 files changed, 15 insertions(+), 47 deletions(-)
include/qemu/sockets.h +0 -1
index 8140fea685..a786bd76d7 100644
@@ -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. */
io/channel-socket.c +15 -25
index 57cfb4d3a6..3c88ca4130 100644
@@ -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, &err);
-            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,
 };
util/qemu-sockets.c +0 -21
index 8bd8bb64eb..aedcb5b9c0 100644
@@ -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;
Signature · 1 line
2.16.1
#1 lore raw
Quoted 7 lines · 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().

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

Quoted 39 lines
Signed-off-by Pavel Balaev <[email protected]>
---
 include/qemu/sockets.h |  1 -
 io/channel-socket.c    | 40 +++++++++++++++-------------------------
 util/qemu-sockets.c    | 21 ---------------------
 3 files changed, 15 insertions(+), 47 deletions(-)
include/qemu/sockets.h +0 -1
index 8140fea685..a786bd76d7 100644
@@ -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. */
io/channel-socket.c +0 -12
index 57cfb4d3a6..3c88ca4130 100644
@@ -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, &err);
-            if (err) {
-                error_report_err(err);

io/channel-socket.c static void qio_channel_socket_init(Object *obj) · line 407

Quoted 10 lines
io/channel-socket.c +0 -10
-                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.

Quoted 21 lines
io/channel-socket.c +5 -2
-
-
 #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) {

io/channel-socket.c qio_channel_socket_close(QIOChannel *ioc, · line 690

Quoted 10 lines
io/channel-socket.c +10 -0
+            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.

Quoted 47 lines
io/channel-socket.c +0 -1
     }
     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,
 };
util/qemu-sockets.c +0 -21
index 8bd8bb64eb..aedcb5b9c0 100644
@@ -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;
Signature · 1 line
2.16.1

Regards,
Daniel

Signature · 3 lines
|: 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 :|