[Qemu-devel] [PULL 2/2] Delete AF_UNIX socket after close

Daniel P. Berrangé posted 2 patches 7 years, 7 months ago
There is a newer version of this series
[Qemu-devel] [PULL 2/2] Delete AF_UNIX socket after close
Posted by Daniel P. Berrangé 7 years, 7 months ago
From: Pavel Balaev <mail@void.so>

This is a second attempt at sending this patch:

http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg04697.html

Signed-off-by: Pavel Balaev <mail@void.so>
---
 io/channel-socket.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 57cfb4d3a6..b50e63a053 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -685,8 +685,10 @@ qio_channel_socket_close(QIOChannel *ioc,
                          Error **errp)
 {
     QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
+    int rc = 0;
 
     if (sioc->fd != -1) {
+        SocketAddress *addr = socket_local_address(sioc->fd, errp);
 #ifdef WIN32
         WSAEventSelect(sioc->fd, NULL, 0);
 #endif
@@ -697,8 +699,22 @@ 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);
+                rc = -1;
+            }
+        }
+
+        if (addr) {
+            qapi_free_SocketAddress(addr);
+        }
     }
-    return 0;
+    return rc;
 }
 
 static int
-- 
2.17.1


Re: [Qemu-devel] [PULL 2/2] Delete AF_UNIX socket after close
Posted by Eric Blake 7 years, 7 months ago
On 06/28/2018 06:02 AM, Daniel P. Berrangé wrote:
> From: Pavel Balaev <mail@void.so>
> 
> This is a second attempt at sending this patch:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg04697.html

I'm not stopping the pull request, but this particular commit message is 
not very useful. A year from now, looking through 'git log' will tell us 
nothing about the "why" for this patch (the subject line only covers the 
"what").  And at that time, no one will care how many failed attempts 
went through the list, but only what actually got committed.  Better 
would have been just directly using the message from that mail:

>> 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().

As a hint for future patches, mentioning that a post is a second version 
and replaces an earlier post to the list is best done after the --- line 
(where it is still readable on list as an aid to reviewers, but dropped 
by the maintainer using 'git am' as unnecessary fluff for the git log). 
More patch submission tips at: https://wiki.qemu.org/Contribute/SubmitAPatch

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

Re: [Qemu-devel] [PULL 2/2] Delete AF_UNIX socket after close
Posted by Daniel P. Berrangé 7 years, 7 months ago
On Thu, Jun 28, 2018 at 07:02:38AM -0500, Eric Blake wrote:
> On 06/28/2018 06:02 AM, Daniel P. Berrangé wrote:
> > From: Pavel Balaev <mail@void.so>
> > 
> > This is a second attempt at sending this patch:
> > 
> > http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg04697.html
> 
> I'm not stopping the pull request, but this particular commit message is not
> very useful. A year from now, looking through 'git log' will tell us nothing
> about the "why" for this patch (the subject line only covers the "what").
> And at that time, no one will care how many failed attempts went through the
> list, but only what actually got committed.  Better would have been just
> directly using the message from that mail:
> 
> > > 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().
> 
> As a hint for future patches, mentioning that a post is a second version and
> replaces an earlier post to the list is best done after the --- line (where
> it is still readable on list as an aid to reviewers, but dropped by the
> maintainer using 'git am' as unnecessary fluff for the git log). More patch
> submission tips at: https://wiki.qemu.org/Contribute/SubmitAPatch

Yeah, sorry my bad - i had intended to fix up the commit message but
completely forgot.

Peter, please ignore this PR, I will resend.

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