[PATCH v1 0/2] introduce qemu_socketpiar()

[PATCH v1 0/2] introduce qemu_socketpiar()

[tugy]

From: Guoyi Tu <tugy@chinatelecom.cn>

Introduce qemu_socketpair() to create socket pair fd, and
set the close-on-exec flag at default as with the other type
of socket does.

besides, the live update feature is developing, so it's necessary
to do that.

Guoyi Tu (2):
  oslib-posix: Introduce qemu_socketpair()
  vhost-user: Call qemu_socketpair() instead of socketpair()

 hw/display/vhost-user-gpu.c |  3 ++-
 hw/virtio/vhost-user.c      |  2 +-
 include/qemu/sockets.h      | 18 ++++++++++++++++++
 util/oslib-posix.c          | 19 +++++++++++++++++++
 4 files changed, 40 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v1 1/2] oslib-posix: Introduce qemu_socketpair()

[tugy]

From: Guoyi Tu <tugy@chinatelecom.cn>

qemu_socketpair() will create a pair of connected sockets
with FD_CLOEXEC set

Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
---
 include/qemu/sockets.h | 18 ++++++++++++++++++
 util/oslib-posix.c     | 19 +++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 038faa157f..036745e586 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -14,6 +14,24 @@ int inet_aton(const char *cp, struct in_addr *ia);
 /* misc helpers */
 bool fd_is_socket(int fd);
 int qemu_socket(int domain, int type, int protocol);
+
+#ifndef WIN32
+/**
+ * qemu_socketpair:
+ * @domain: specifies a communication domain, such as PF_UNIX
+ * @type: specifies the socket type.
+ * @protocol: specifies a particular protocol to be used with the  socket
+ * @sv: an array to store the pair of socket created
+ *
+ * Creates an unnamed pair of connected sockets in the specified domain,
+ * of the specified type, and using the optionally specified protocol.
+ * And automatically set the close-on-exec flags on the returned sockets
+ *
+ * Return 0 on success.
+ */
+int qemu_socketpair(int domain, int type, int protocol, int sv[2]);
+#endif
+
 int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
 int socket_set_cork(int fd, int v);
 int socket_set_nodelay(int fd);
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 7a34c1657c..e5f5ebe469 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -261,6 +261,25 @@ void qemu_set_cloexec(int fd)
     assert(f != -1);
 }
 
+int qemu_socketpair(int domain, int type, int protocol, int sv[2])
+{
+    int ret;
+
+#ifdef SOCK_CLOEXEC
+    ret = socketpair(domain, type | SOCK_CLOEXEC, protocol, sv);
+    if (ret != -1 || errno != EINVAL) {
+        return ret;
+    }
+#endif
+    ret = socketpair(domain, type, protocol, sv);;
+    if (ret == 0) {
+        qemu_set_cloexec(sv[0]);
+        qemu_set_cloexec(sv[1]);
+    }
+
+    return ret;
+}
+
 char *
 qemu_get_local_state_dir(void)
 {
-- 
2.25.1

[PATCH v1 2/2] vhost-user: Call qemu_socketpair() instead of socketpair()

[tugy]

From: Guoyi Tu <tugy@chinatelecom.cn>

As the close-on-exec flags is not set on the file descriptors returned
by socketpair() at default, the fds will survive across exec' function.

In the case that exec' function get invoked, such as the live-update feature
which is been developing, it will cause fd leaks.

To address this problem, we should call qemu_socketpair() to create an pair of
connected sockets with the close-on-exec flag set.

Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
---
 hw/display/vhost-user-gpu.c | 3 ++-
 hw/virtio/vhost-user.c      | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 3340ef9e5f..19c0e20103 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/sockets.h"
 #include "hw/qdev-properties.h"
 #include "hw/virtio/virtio-gpu.h"
 #include "chardev/char-fe.h"
@@ -375,7 +376,7 @@ vhost_user_gpu_do_set_socket(VhostUserGPU *g, Error **errp)
     Chardev *chr;
     int sv[2];
 
-    if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
+    if (qemu_socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
         error_setg_errno(errp, errno, "socketpair() failed");
         return false;
     }
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 75b8df21a4..4d2b227028 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1726,7 +1726,7 @@ static int vhost_setup_slave_channel(struct vhost_dev *dev)
         return 0;
     }
 
-    if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
+    if (qemu_socketpair(PF_UNIX, SOCK_STREAM, 0, sv) == -1) {
         int saved_errno = errno;
         error_report("socketpair() failed");
         return -saved_errno;
-- 
2.25.1

Re: [PATCH v1 0/2] introduce qemu_socketpiar()

[Guoyi Tu]

Ping...

Any advises are welcome

On 8/23/22 15:50, tugy@chinatelecom.cn wrote:
> From: Guoyi Tu <tugy@chinatelecom.cn>
> 
> Introduce qemu_socketpair() to create socket pair fd, and
> set the close-on-exec flag at default as with the other type
> of socket does.
> 
> besides, the live update feature is developing, so it's necessary
> to do that.
> 
> Guoyi Tu (2):
>    oslib-posix: Introduce qemu_socketpair()
>    vhost-user: Call qemu_socketpair() instead of socketpair()
> 
>   hw/display/vhost-user-gpu.c |  3 ++-
>   hw/virtio/vhost-user.c      |  2 +-
>   include/qemu/sockets.h      | 18 ++++++++++++++++++
>   util/oslib-posix.c          | 19 +++++++++++++++++++
>   4 files changed, 40 insertions(+), 2 deletions(-)
> 

Re: [PATCH v1 0/2] introduce qemu_socketpiar()

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

Hi

On Tue, Aug 23, 2022 at 12:00 PM <tugy@chinatelecom.cn> wrote:

> From: Guoyi Tu <tugy@chinatelecom.cn>
>
> Introduce qemu_socketpair() to create socket pair fd, and
> set the close-on-exec flag at default as with the other type
> of socket does.
>
> besides, the live update feature is developing, so it's necessary
> to do that.
>
> Guoyi Tu (2):
>   oslib-posix: Introduce qemu_socketpair()
>   vhost-user: Call qemu_socketpair() instead of socketpair()
>

Looks like a good idea to me. We will eventually extend the support for
win32 (as discussed in "[PATCH 19/51] tests/qtest: Build
test-filter-{mirror, redirector} cases for posix only").

There are other places where you can replace existing socketpair() calls in
the code base. Why not do it?

Current patches lgtm
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 1630 bytes --]

Re: [PATCH v1 0/2] introduce qemu_socketpiar()

[Christian Schoenebeck]

On Dienstag, 23. August 2022 09:50:38 CEST tugy@chinatelecom.cn wrote:
> From: Guoyi Tu <tugy@chinatelecom.cn>
> 
> Introduce qemu_socketpair() to create socket pair fd, and
> set the close-on-exec flag at default as with the other type
> of socket does.
> 
> besides, the live update feature is developing, so it's necessary
> to do that.
> 
> Guoyi Tu (2):
>   oslib-posix: Introduce qemu_socketpair()
>   vhost-user: Call qemu_socketpair() instead of socketpair()
> 
>  hw/display/vhost-user-gpu.c |  3 ++-
>  hw/virtio/vhost-user.c      |  2 +-
>  include/qemu/sockets.h      | 18 ++++++++++++++++++
>  util/oslib-posix.c          | 19 +++++++++++++++++++
>  4 files changed, 40 insertions(+), 2 deletions(-)

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Re: [PATCH v1 0/2] introduce qemu_socketpiar()

[Guoyi Tu]

On 9/5/22 19:19, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 23, 2022 at 12:00 PM <tugy@chinatelecom.cn 
> <mailto:tugy@chinatelecom.cn>> wrote:
> 
>     From: Guoyi Tu <tugy@chinatelecom.cn <mailto:tugy@chinatelecom.cn>>
> 
>     Introduce qemu_socketpair() to create socket pair fd, and
>     set the close-on-exec flag at default as with the other type
>     of socket does.
> 
>     besides, the live update feature is developing, so it's necessary
>     to do that.
> 
>     Guoyi Tu (2):
>        oslib-posix: Introduce qemu_socketpair()
>        vhost-user: Call qemu_socketpair() instead of socketpair()
> 
> 
> Looks like a good idea to me. We will eventually extend the support for 
> win32 (as discussed in "[PATCH 19/51] tests/qtest: Build 
> test-filter-{mirror, redirector} cases for posix only").
> 
> There are other places where you can replace existing socketpair() calls 
> in the code base. Why not do it?
> 
yeah, Thanks for reminding me, Maybe i can replace all the existing 
socketpair() calls in a separate patch if this patch can be merged to 
upstream.

--
Guoyi

> Current patches lgtm
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com 
> <mailto:marcandre.lureau@redhat.com>>
> 
> -- 
> Marc-André Lureau

Re: [PATCH v1 0/2] introduce qemu_socketpiar()

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

Hi

On Mon, Sep 5, 2022 at 4:28 PM Guoyi Tu <tugy@chinatelecom.cn> wrote:

>
>
> On 9/5/22 19:19, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, Aug 23, 2022 at 12:00 PM <tugy@chinatelecom.cn
> > <mailto:tugy@chinatelecom.cn>> wrote:
> >
> >     From: Guoyi Tu <tugy@chinatelecom.cn <mailto:tugy@chinatelecom.cn>>
> >
> >     Introduce qemu_socketpair() to create socket pair fd, and
> >     set the close-on-exec flag at default as with the other type
> >     of socket does.
> >
> >     besides, the live update feature is developing, so it's necessary
> >     to do that.
> >
> >     Guoyi Tu (2):
> >        oslib-posix: Introduce qemu_socketpair()
> >        vhost-user: Call qemu_socketpair() instead of socketpair()
> >
> >
> > Looks like a good idea to me. We will eventually extend the support for
> > win32 (as discussed in "[PATCH 19/51] tests/qtest: Build
> > test-filter-{mirror, redirector} cases for posix only").
> >
> > There are other places where you can replace existing socketpair() calls
> > in the code base. Why not do it?
> >
> yeah, Thanks for reminding me, Maybe i can replace all the existing
> socketpair() calls in a separate patch if this patch can be merged to
> upstream.
>

It needs to be reviewed though, all may not want to set CLOEXEC, but most
should. Else, we should probably consider switching to GSpawn which does
dup or unset CLOEXEC for explicitely passed fds.


> --
> Guoyi
>
> > Current patches lgtm
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com
> > <mailto:marcandre.lureau@redhat.com>>
> >
> > --
> > Marc-André Lureau
>


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2908 bytes --]

Re: [PATCH v1 0/2] introduce qemu_socketpiar()

[Guoyi Tu]

On 9/5/22 20:33, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Sep 5, 2022 at 4:28 PM Guoyi Tu <tugy@chinatelecom.cn 
> <mailto:tugy@chinatelecom.cn>> wrote:
> 
> 
> 
>     On 9/5/22 19:19, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Tue, Aug 23, 2022 at 12:00 PM <tugy@chinatelecom.cn
>     <mailto:tugy@chinatelecom.cn>
>      > <mailto:tugy@chinatelecom.cn <mailto:tugy@chinatelecom.cn>>> wrote:
>      >
>      >     From: Guoyi Tu <tugy@chinatelecom.cn
>     <mailto:tugy@chinatelecom.cn> <mailto:tugy@chinatelecom.cn
>     <mailto:tugy@chinatelecom.cn>>>
>      >
>      >     Introduce qemu_socketpair() to create socket pair fd, and
>      >     set the close-on-exec flag at default as with the other type
>      >     of socket does.
>      >
>      >     besides, the live update feature is developing, so it's necessary
>      >     to do that.
>      >
>      >     Guoyi Tu (2):
>      >        oslib-posix: Introduce qemu_socketpair()
>      >        vhost-user: Call qemu_socketpair() instead of socketpair()
>      >
>      >
>      > Looks like a good idea to me. We will eventually extend the
>     support for
>      > win32 (as discussed in "[PATCH 19/51] tests/qtest: Build
>      > test-filter-{mirror, redirector} cases for posix only").
>      >
>      > There are other places where you can replace existing
>     socketpair() calls
>      > in the code base. Why not do it?
>      >
>     yeah, Thanks for reminding me, Maybe i can replace all the existing
>     socketpair() calls in a separate patch if this patch can be merged to
>     upstream.
> 
> 
> It needs to be reviewed though, all may not want to set CLOEXEC, but 
> most should. Else, we should probably consider switching to GSpawn which 
> does dup or unset CLOEXEC for explicitely passed fds.

Got it. Special care will be taken when replace those functions.

Thanks again.

--
Guoyi
>     --
>     Guoyi
> 
>      > Current patches lgtm
>      > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com>
>      > <mailto:marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com>>>
>      >
>      > --
>      > Marc-André Lureau
> 
> 
> 
> -- 
> Marc-André Lureau

Re: [PATCH v1 0/2] introduce qemu_socketpiar()

[Guoyi Tu]

Hi Peter,

what do you think about this patches? If this could be merged in 
upstream, i will start to update the test codes

On 8/23/22 15:50, tugy@chinatelecom.cn wrote:
> From: Guoyi Tu <tugy@chinatelecom.cn>
> 
> Introduce qemu_socketpair() to create socket pair fd, and
> set the close-on-exec flag at default as with the other type
> of socket does.
> 
> besides, the live update feature is developing, so it's necessary
> to do that.
> 
> Guoyi Tu (2):
>    oslib-posix: Introduce qemu_socketpair()
>    vhost-user: Call qemu_socketpair() instead of socketpair()
> 
>   hw/display/vhost-user-gpu.c |  3 ++-
>   hw/virtio/vhost-user.c      |  2 +-
>   include/qemu/sockets.h      | 18 ++++++++++++++++++
>   util/oslib-posix.c          | 19 +++++++++++++++++++
>   4 files changed, 40 insertions(+), 2 deletions(-)
>