[Qemu-devel] [PATCH resend 0/6 RFC] Provide common methods for exchange FD

[Qemu-devel] [PATCH resend 0/6 RFC] Provide common methods for exchange FD

[Lei Li]

This patch series tries to refactor the functions used for 
exchange of FD in current code, provide common methods
for it.

The series is based on the localhost migration with side channel
for ram series as it was already a good shape. But if you want
to merge this first, I'll get rid of the migration part. 

I just tested page flipping migration, and tap/bridge-helper a
bit, but have some environment problem on proxy fs driver. So 
it'd be appreciated if someone could help on verifying whether
it has impact on it. :)

Please let me know if there is anything needs to be improved.

Thanks.


Lei Li (6):
  fd-exchange: provide common methods for exchange of fd
  qemu-bridge-helper: replace send_fd with qemu_send_with_fd
  net/tap: replace recv_fd with qemu_recv_with_fd
  virtfs-proxy-helper: replace send_fd with qemu_send_with_fd
  virtio-9p-proxy: replace v9fs_receivefd with qemu_recv_with_fd
  migration-local: replace send_pipefd with qemu_send_with_fd

 Makefile                    |    2 +-
 fsdev/virtfs-proxy-helper.c |   51 ++++-------------------
 hw/9pfs/virtio-9p-proxy.c   |   60 +-------------------------
 hw/9pfs/virtio-9p-proxy.h   |    5 --
 include/qemu/fd-exchange.h  |   25 +++++++++++
 migration-local.c           |   52 +----------------------
 net/tap.c                   |   40 +----------------
 qemu-bridge-helper.c        |   31 +------------
 util/Makefile.objs          |    1 +
 util/qemu-fd-exchange.c     |   97 +++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 144 insertions(+), 220 deletions(-)
 create mode 100644 include/qemu/fd-exchange.h
 create mode 100644 util/qemu-fd-exchange.c

[Qemu-devel] [PATCH 1/6] qemu-fd-exchange: provide common methods for exchange fd

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 include/qemu/fd-exchange.h |   25 +++++++++++
 util/Makefile.objs         |    1 +
 util/qemu-fd-exchange.c    |   97 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+), 0 deletions(-)
 create mode 100644 include/qemu/fd-exchange.h
 create mode 100644 util/qemu-fd-exchange.c

diff --git a/include/qemu/fd-exchange.h b/include/qemu/fd-exchange.h
new file mode 100644
index 0000000..6929026
--- /dev/null
+++ b/include/qemu/fd-exchange.h
@@ -0,0 +1,25 @@
+/*
+ * Internel common methods for exchange of FD
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef FD_EXCHANGE_H
+#define FD_EXCHANGE_H
+
+#include <sys/socket.h>
+
+union MsgControl {
+    struct cmsghdr cmsg;
+    char control[CMSG_SPACE(sizeof(int))];
+};
+
+ssize_t qemu_send_with_fd(int sockfd, int passed_fd,
+                          const void *buf, size_t len);
+
+ssize_t qemu_recv_with_fd(int sockfd, int *passed_fd,
+                          void *buf, size_t len);
+
+#endif
diff --git a/util/Makefile.objs b/util/Makefile.objs
index af3e5cb..2fb42bf 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -13,3 +13,4 @@ util-obj-y += hexdump.o
 util-obj-y += crc32c.o
 util-obj-y += throttle.o
 util-obj-y += getauxval.o
+util-obj-y += qemu-fd-exchange.o
diff --git a/util/qemu-fd-exchange.c b/util/qemu-fd-exchange.c
new file mode 100644
index 0000000..70a3206
--- /dev/null
+++ b/util/qemu-fd-exchange.c
@@ -0,0 +1,97 @@
+/*
+ * Internel common methods for exchange of FD
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/fd-exchange.h"
+#include "qemu-common.h"
+
+
+ssize_t qemu_send_with_fd(int sockfd, int passed_fd,
+                          const void *buf, size_t len)
+{
+    struct msghdr msg;
+    struct iovec iov;
+    struct cmsghdr *cmsg;
+    union MsgControl msg_control;
+    int retval;
+
+    iov.iov_base = (int *)buf;
+    iov.iov_len = len;
+
+    memset(&msg, 0, sizeof(msg));
+    msg.msg_iov = &iov;
+    msg.msg_iovlen = len;
+    msg.msg_control = &msg_control;
+    msg.msg_controllen = sizeof(msg_control);
+
+    if (passed_fd < 0) {
+        *(int *)buf = passed_fd;
+    } else {
+        msg.msg_control = &msg_control;
+        msg.msg_controllen = sizeof(msg_control);
+
+        cmsg = &msg_control.cmsg;
+        cmsg->cmsg_len = CMSG_LEN(sizeof(passed_fd));
+        cmsg->cmsg_level = SOL_SOCKET;
+        cmsg->cmsg_type = SCM_RIGHTS;
+        memcpy(CMSG_DATA(cmsg), &passed_fd, sizeof(passed_fd));
+
+    }
+
+    do {
+        retval = sendmsg(sockfd, &msg, 0);
+    } while (retval < 0 && errno == EINTR);
+
+    return retval;
+}
+
+ssize_t qemu_recv_with_fd(int sockfd, int *passed_fd,
+                          void *buf, size_t len)
+{
+    struct iovec iov;
+    struct msghdr msg;
+    struct cmsghdr *cmsg;
+    union MsgControl msg_control;
+    int retval;
+    int data = *(int *)buf;
+
+    iov.iov_base = buf;
+    iov.iov_len = len;
+
+    memset(&msg, 0, sizeof(msg));
+    msg.msg_iov = &iov;
+    msg.msg_iovlen = 1;
+    msg.msg_control = &msg_control;
+    msg.msg_controllen = sizeof(msg_control);
+
+    do {
+        retval = recvmsg(sockfd, &msg, 0);
+    } while (retval < 0 && errno == EINTR);
+
+    if (retval <= 0) {
+        return retval;
+    }
+
+    if (data != *(int *)buf) {
+        *passed_fd = data;
+        return 0;
+    }
+
+    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
+        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
+            cmsg->cmsg_level != SOL_SOCKET ||
+            cmsg->cmsg_type != SCM_RIGHTS) {
+            continue;
+        }
+
+        memcpy(passed_fd, CMSG_DATA(cmsg), sizeof(*passed_fd));
+        return 0;
+    }
+
+    *passed_fd = -ENFILE;
+    return retval;
+}
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/6] qemu-bridge-helper: replace send_fd with qemu_send_with_fd

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 Makefile             |    2 +-
 qemu-bridge-helper.c |   31 +++----------------------------
 2 files changed, 4 insertions(+), 29 deletions(-)

diff --git a/Makefile b/Makefile
index bdff4e4..6850f35 100644
--- a/Makefile
+++ b/Makefile
@@ -195,7 +195,7 @@ qemu-img$(EXESUF): qemu-img.o $(block-obj-y) libqemuutil.a libqemustub.a
 qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) libqemuutil.a libqemustub.a
 qemu-io$(EXESUF): qemu-io.o $(block-obj-y) libqemuutil.a libqemustub.a
 
-qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o
+qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o libqemuutil.a
 
 fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o fsdev/virtio-9p-marshal.o libqemuutil.a libqemustub.a
 fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 6a0974e..8303b6b 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -40,6 +40,7 @@
 #endif
 
 #include "qemu/queue.h"
+#include "qemu/fd-exchange.h"
 
 #include "net/tap-linux.h"
 
@@ -174,33 +175,6 @@ static void prep_ifreq(struct ifreq *ifr, const char *ifname)
     snprintf(ifr->ifr_name, IFNAMSIZ, "%s", ifname);
 }
 
-static int send_fd(int c, int fd)
-{
-    char msgbuf[CMSG_SPACE(sizeof(fd))];
-    struct msghdr msg = {
-        .msg_control = msgbuf,
-        .msg_controllen = sizeof(msgbuf),
-    };
-    struct cmsghdr *cmsg;
-    struct iovec iov;
-    char req[1] = { 0x00 };
-
-    cmsg = CMSG_FIRSTHDR(&msg);
-    cmsg->cmsg_level = SOL_SOCKET;
-    cmsg->cmsg_type = SCM_RIGHTS;
-    cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
-    msg.msg_controllen = cmsg->cmsg_len;
-
-    iov.iov_base = req;
-    iov.iov_len = sizeof(req);
-
-    msg.msg_iov = &iov;
-    msg.msg_iovlen = 1;
-    memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
-
-    return sendmsg(c, &msg, 0);
-}
-
 #ifdef CONFIG_LIBCAP
 static int drop_privileges(void)
 {
@@ -239,6 +213,7 @@ int main(int argc, char **argv)
     ACLList acl_list;
     int access_allowed, access_denied;
     int ret = EXIT_SUCCESS;
+    char req[1] = { 0x00 };
 
 #ifdef CONFIG_LIBCAP
     /* if we're run from an suid binary, immediately drop privileges preserving
@@ -424,7 +399,7 @@ int main(int argc, char **argv)
     }
 
     /* write fd to the domain socket */
-    if (send_fd(unixfd, fd) == -1) {
+    if (qemu_send_with_fd(unixfd, fd, &req, sizeof(req)) == -1) {
         fprintf(stderr, "failed to write fd to unix socket: %s\n",
                 strerror(errno));
         ret = EXIT_FAILURE;
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/6] net/tap: replace recv_fd with qemu_recv_with_fd

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 net/tap.c |   40 +++-------------------------------------
 1 files changed, 3 insertions(+), 37 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 39c1cda..97ee2e8 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -39,6 +39,7 @@
 #include "sysemu/sysemu.h"
 #include "qemu-common.h"
 #include "qemu/error-report.h"
+#include "qemu/fd-exchange.h"
 
 #include "net/tap.h"
 
@@ -385,40 +386,6 @@ static int launch_script(const char *setup_script, const char *ifname, int fd)
     return -1;
 }
 
-static int recv_fd(int c)
-{
-    int fd;
-    uint8_t msgbuf[CMSG_SPACE(sizeof(fd))];
-    struct msghdr msg = {
-        .msg_control = msgbuf,
-        .msg_controllen = sizeof(msgbuf),
-    };
-    struct cmsghdr *cmsg;
-    struct iovec iov;
-    uint8_t req[1];
-    ssize_t len;
-
-    cmsg = CMSG_FIRSTHDR(&msg);
-    cmsg->cmsg_level = SOL_SOCKET;
-    cmsg->cmsg_type = SCM_RIGHTS;
-    cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
-    msg.msg_controllen = cmsg->cmsg_len;
-
-    iov.iov_base = req;
-    iov.iov_len = sizeof(req);
-
-    msg.msg_iov = &iov;
-    msg.msg_iovlen = 1;
-
-    len = recvmsg(c, &msg, 0);
-    if (len > 0) {
-        memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd));
-        return fd;
-    }
-
-    return len;
-}
-
 static int net_bridge_run_helper(const char *helper, const char *bridge)
 {
     sigset_t oldmask, mask;
@@ -489,12 +456,11 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
 
     } else if (pid > 0) {
         int fd;
+        char req[1] = { 0x00 };
 
         close(sv[1]);
 
-        do {
-            fd = recv_fd(sv[0]);
-        } while (fd == -1 && errno == EINTR);
+        qemu_recv_with_fd(sv[0], &fd, &req, sizeof(req));
 
         close(sv[0]);
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 4/6] virtfs-proxy-helper: replace send_fd with qemu_send_with_fd

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 fsdev/virtfs-proxy-helper.c |   51 ++++++------------------------------------
 hw/9pfs/virtio-9p-proxy.h   |    5 ----
 2 files changed, 8 insertions(+), 48 deletions(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 713a7b2..44c6e61 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -23,6 +23,7 @@
 #include "qemu-common.h"
 #include "qemu/sockets.h"
 #include "qemu/xattr.h"
+#include "qemu/fd-exchange.h"
 #include "virtio-9p-marshal.h"
 #include "hw/9pfs/virtio-9p-proxy.h"
 #include "fsdev/virtio-9p-marshal.h"
@@ -203,48 +204,6 @@ static int read_request(int sockfd, struct iovec *iovec, ProxyHeader *header)
     return 0;
 }
 
-static int send_fd(int sockfd, int fd)
-{
-    struct msghdr msg;
-    struct iovec iov;
-    int retval, data;
-    struct cmsghdr *cmsg;
-    union MsgControl msg_control;
-
-    iov.iov_base = &data;
-    iov.iov_len = sizeof(data);
-
-    memset(&msg, 0, sizeof(msg));
-    msg.msg_iov = &iov;
-    msg.msg_iovlen = 1;
-    /* No ancillary data on error */
-    if (fd < 0) {
-        /* fd is really negative errno if the request failed  */
-        data = fd;
-    } else {
-        data = V9FS_FD_VALID;
-        msg.msg_control = &msg_control;
-        msg.msg_controllen = sizeof(msg_control);
-
-        cmsg = &msg_control.cmsg;
-        cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
-        cmsg->cmsg_level = SOL_SOCKET;
-        cmsg->cmsg_type = SCM_RIGHTS;
-        memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
-    }
-
-    do {
-        retval = sendmsg(sockfd, &msg, 0);
-    } while (retval < 0 && errno == EINTR);
-    if (fd >= 0) {
-        close(fd);
-    }
-    if (retval < 0) {
-        return retval;
-    }
-    return 0;
-}
-
 static int send_status(int sockfd, struct iovec *iovec, int status)
 {
     ProxyHeader header;
@@ -784,11 +743,17 @@ static void usage(char *prog)
 static int process_reply(int sock, int type,
                          struct iovec *out_iovec, int retval)
 {
+    int data = V9FS_FD_VALID;
+
     switch (type) {
     case T_OPEN:
     case T_CREATE:
-        if (send_fd(sock, retval) < 0) {
+        if (qemu_send_with_fd(sock, retval, &data, sizeof(data)) < 0) {
             return -1;
+        } else {
+            if (retval >= 0) {
+                close(retval);
+            }
         }
         break;
     case T_MKNOD:
diff --git a/hw/9pfs/virtio-9p-proxy.h b/hw/9pfs/virtio-9p-proxy.h
index 005c1ad..e359ac5 100644
--- a/hw/9pfs/virtio-9p-proxy.h
+++ b/hw/9pfs/virtio-9p-proxy.h
@@ -24,11 +24,6 @@
 #define proxy_marshal(out_sg, offset, fmt, args...) \
     v9fs_marshal(out_sg, 1, offset, 0, fmt, ##args)
 
-union MsgControl {
-    struct cmsghdr cmsg;
-    char control[CMSG_SPACE(sizeof(int))];
-};
-
 typedef struct {
     uint32_t type;
     uint32_t size;
-- 
1.7.7.6

[Qemu-devel] [PATCH 5/6] virtio-9p-proxy: replace v9fs_receivefd with qemu_recv_with_fd

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 hw/9pfs/virtio-9p-proxy.c |   60 ++------------------------------------------
 1 files changed, 3 insertions(+), 57 deletions(-)

diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
index 5f44bb7..f34b845 100644
--- a/hw/9pfs/virtio-9p-proxy.c
+++ b/hw/9pfs/virtio-9p-proxy.c
@@ -14,6 +14,7 @@
 #include "hw/virtio/virtio.h"
 #include "virtio-9p.h"
 #include "qemu/error-report.h"
+#include "qemu/fd-exchange.h"
 #include "fsdev/qemu-fsdev.h"
 #include "virtio-9p-proxy.h"
 
@@ -24,62 +25,6 @@ typedef struct V9fsProxy {
     struct iovec out_iovec;
 } V9fsProxy;
 
-/*
- * Return received file descriptor on success in *status.
- * errno is also returned on *status (which will be < 0)
- * return < 0 on transport error.
- */
-static int v9fs_receivefd(int sockfd, int *status)
-{
-    struct iovec iov;
-    struct msghdr msg;
-    struct cmsghdr *cmsg;
-    int retval, data, fd;
-    union MsgControl msg_control;
-
-    iov.iov_base = &data;
-    iov.iov_len = sizeof(data);
-
-    memset(&msg, 0, sizeof(msg));
-    msg.msg_iov = &iov;
-    msg.msg_iovlen = 1;
-    msg.msg_control = &msg_control;
-    msg.msg_controllen = sizeof(msg_control);
-
-    do {
-        retval = recvmsg(sockfd, &msg, 0);
-    } while (retval < 0 && errno == EINTR);
-    if (retval <= 0) {
-        return retval;
-    }
-    /*
-     * data is set to V9FS_FD_VALID, if ancillary data is sent.  If this
-     * request doesn't need ancillary data (fd) or an error occurred,
-     * data is set to negative errno value.
-     */
-    if (data != V9FS_FD_VALID) {
-        *status = data;
-        return 0;
-    }
-    /*
-     * File descriptor (fd) is sent in the ancillary data. Check if we
-     * indeed received it. One of the reasons to fail to receive it is if
-     * we exceeded the maximum number of file descriptors!
-     */
-    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
-        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
-            cmsg->cmsg_level != SOL_SOCKET ||
-            cmsg->cmsg_type != SCM_RIGHTS) {
-            continue;
-        }
-        fd = *((int *)CMSG_DATA(cmsg));
-        *status = fd;
-        return 0;
-    }
-    *status = -ENFILE;  /* Ancillary data sent but not received */
-    return 0;
-}
-
 static ssize_t socket_read(int sockfd, void *buff, size_t size)
 {
     ssize_t retval, total = 0;
@@ -307,6 +252,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
     V9fsString *name, *value;
     V9fsString *path, *oldpath;
     struct iovec *iovec = NULL, *reply = NULL;
+    int data = V9FS_FD_VALID;
 
     qemu_mutex_lock(&proxy->mutex);
 
@@ -548,7 +494,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
          * A file descriptor is returned as response for
          * T_OPEN,T_CREATE on success
          */
-        if (v9fs_receivefd(proxy->sockfd, &retval) < 0) {
+        if (qemu_recv_with_fd(proxy->sockfd, &retval, &data, sizeof(data)) < 0) {
             goto close_error;
         }
         break;
-- 
1.7.7.6

[Qemu-devel] [PATCH 6/6] migration-local: replace send_pipefd with qemu_send_with_fd

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 migration-local.c |   52 +++-------------------------------------------------
 1 files changed, 3 insertions(+), 49 deletions(-)

diff --git a/migration-local.c b/migration-local.c
index ce4c070..c01ba06 100644
--- a/migration-local.c
+++ b/migration-local.c
@@ -26,6 +26,7 @@
 #include "sysemu/sysemu.h"
 #include "block/block.h"
 #include "qemu/sockets.h"
+#include "qemu/fd-exchange.h"
 #include "migration/block.h"
 #include "qemu/thread.h"
 #include "qmp-commands.h"
@@ -169,8 +170,6 @@ static int qemu_local_close(void *opaque)
     return 0;
 }
 
-static int send_pipefd(int sockfd, int pipefd);
-
 static size_t qemu_local_save_ram(QEMUFile *f, void *opaque,
                                   MemoryRegion *mr, ram_addr_t offset,
                                   size_t size, int *bytes_sent)
@@ -179,13 +178,14 @@ static size_t qemu_local_save_ram(QEMUFile *f, void *opaque,
     ram_addr_t current_addr = mr->ram_addr + offset;
     void *ram_addr;
     ssize_t ret;
+    char req[1] = { 0x01 };
 
     if (s->unix_page_flipping) {
         qemu_put_be64(s->file, current_addr | RAM_SAVE_FLAG_HOOK);
         qemu_fflush(s->file);
 
         if (!s->pipefd_passed) {
-            ret = send_pipefd(s->sockfd, s->pipefd[0]);
+            ret = qemu_send_with_fd(s->sockfd, s->pipefd[0], &req, sizeof(req));
             if (ret < 0) {
                 fprintf(stderr, "failed to pass PIPE\n");
                 return ret;
@@ -342,49 +342,3 @@ fail:
     g_free(s);
     return NULL;
 }
-
-
-/*
- * Pass a pipe file descriptor to another process.
- *
- * Return negative value If pipefd < 0. Return 0 on
- * success.
- *
- */
-static int send_pipefd(int sockfd, int pipefd)
-{
-    struct msghdr msg;
-    struct iovec iov[1];
-    ssize_t ret;
-    char req[1] = { 0x01 };
-
-    union {
-      struct cmsghdr cm;
-      char control[CMSG_SPACE(sizeof(int))];
-    } control_un;
-    struct cmsghdr *cmptr;
-
-    msg.msg_control = control_un.control;
-    msg.msg_controllen = sizeof(control_un.control);
-
-    cmptr = CMSG_FIRSTHDR(&msg);
-    cmptr->cmsg_len = CMSG_LEN(sizeof(int));
-    cmptr->cmsg_level = SOL_SOCKET;
-    cmptr->cmsg_type = SCM_RIGHTS;
-    *((int *) CMSG_DATA(cmptr)) = pipefd;
-
-    msg.msg_name = NULL;
-    msg.msg_namelen = 0;
-
-    iov[0].iov_base = req;
-    iov[0].iov_len = sizeof(req);
-    msg.msg_iov = iov;
-    msg.msg_iovlen = 1;
-
-    ret = sendmsg(sockfd, &msg, 0);
-    if (ret <= 0) {
-        DPRINTF("sendmsg error: %s\n", strerror(errno));
-    }
-
-    return ret;
-}
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH resend 0/6 RFC] Provide common methods for exchange FD

[Lei Li]

Any comments?

On 01/08/2014 05:12 PM, Lei Li wrote:
> This patch series tries to refactor the functions used for
> exchange of FD in current code, provide common methods
> for it.
>
> The series is based on the localhost migration with side channel
> for ram series as it was already a good shape. But if you want
> to merge this first, I'll get rid of the migration part.
>
> I just tested page flipping migration, and tap/bridge-helper a
> bit, but have some environment problem on proxy fs driver. So
> it'd be appreciated if someone could help on verifying whether
> it has impact on it. :)
>
> Please let me know if there is anything needs to be improved.
>
> Thanks.
>
>
> Lei Li (6):
>    fd-exchange: provide common methods for exchange of fd
>    qemu-bridge-helper: replace send_fd with qemu_send_with_fd
>    net/tap: replace recv_fd with qemu_recv_with_fd
>    virtfs-proxy-helper: replace send_fd with qemu_send_with_fd
>    virtio-9p-proxy: replace v9fs_receivefd with qemu_recv_with_fd
>    migration-local: replace send_pipefd with qemu_send_with_fd
>
>   Makefile                    |    2 +-
>   fsdev/virtfs-proxy-helper.c |   51 ++++-------------------
>   hw/9pfs/virtio-9p-proxy.c   |   60 +-------------------------
>   hw/9pfs/virtio-9p-proxy.h   |    5 --
>   include/qemu/fd-exchange.h  |   25 +++++++++++
>   migration-local.c           |   52 +----------------------
>   net/tap.c                   |   40 +----------------
>   qemu-bridge-helper.c        |   31 +------------
>   util/Makefile.objs          |    1 +
>   util/qemu-fd-exchange.c     |   97 +++++++++++++++++++++++++++++++++++++++++++
>   10 files changed, 144 insertions(+), 220 deletions(-)
>   create mode 100644 include/qemu/fd-exchange.h
>   create mode 100644 util/qemu-fd-exchange.c
>
>


-- 
Lei

Re: [Qemu-devel] [PATCH 4/6] virtfs-proxy-helper: replace send_fd with qemu_send_with_fd

[Daniel P. Berrange]

On Wed, Jan 08, 2014 at 05:12:54PM +0800, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  fsdev/virtfs-proxy-helper.c |   51 ++++++------------------------------------
>  hw/9pfs/virtio-9p-proxy.h   |    5 ----
>  2 files changed, 8 insertions(+), 48 deletions(-)
> 
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index 713a7b2..44c6e61 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c

> -static int send_fd(int sockfd, int fd)
> -{
...
> -    /* No ancillary data on error */
> -    if (fd < 0) {
> -        /* fd is really negative errno if the request failed  */
> -        data = fd;
> -    } else {
> -        data = V9FS_FD_VALID;

The way data is initialized here...

> @@ -784,11 +743,17 @@ static void usage(char *prog)
>  static int process_reply(int sock, int type,
>                           struct iovec *out_iovec, int retval)
>  {
> +    int data = V9FS_FD_VALID;


Doesn't match what you do here.

> +
>      switch (type) {
>      case T_OPEN:
>      case T_CREATE:
> -        if (send_fd(sock, retval) < 0) {
> +        if (qemu_send_with_fd(sock, retval, &data, sizeof(data)) < 0) {
>              return -1;
> +        } else {
> +            if (retval >= 0) {
> +                close(retval);
> +            }
>          }
>          break;
>      case T_MKNOD:

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 5/6] virtio-9p-proxy: replace v9fs_receivefd with qemu_recv_with_fd

[Daniel P. Berrange]

On Wed, Jan 08, 2014 at 05:12:55PM +0800, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  hw/9pfs/virtio-9p-proxy.c |   60 ++------------------------------------------
>  1 files changed, 3 insertions(+), 57 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
> index 5f44bb7..f34b845 100644
> --- a/hw/9pfs/virtio-9p-proxy.c
> +++ b/hw/9pfs/virtio-9p-proxy.c

> -    do {
> -        retval = recvmsg(sockfd, &msg, 0);
> -    } while (retval < 0 && errno == EINTR);
> -    if (retval <= 0) {
> -        return retval;
> -    }
> -    /*
> -     * data is set to V9FS_FD_VALID, if ancillary data is sent.  If this
> -     * request doesn't need ancillary data (fd) or an error occurred,
> -     * data is set to negative errno value.
> -     */
> -    if (data != V9FS_FD_VALID) {
> -        *status = data;
> -        return 0;
> -    }

This code is handling the 'data' value...


> @@ -307,6 +252,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
>      V9fsString *name, *value;
>      V9fsString *path, *oldpath;
>      struct iovec *iovec = NULL, *reply = NULL;
> +    int data = V9FS_FD_VALID;
>  
>      qemu_mutex_lock(&proxy->mutex);
>  
> @@ -548,7 +494,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
>           * A file descriptor is returned as response for
>           * T_OPEN,T_CREATE on success
>           */
> -        if (v9fs_receivefd(proxy->sockfd, &retval) < 0) {
> +        if (qemu_recv_with_fd(proxy->sockfd, &retval, &data, sizeof(data)) < 0) {
>              goto close_error;
>          }

...but this code is ignoring the return value in 'data'.


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 resend 0/6 RFC] Provide common methods for exchange FD

[Daniel P. Berrange]

On Wed, Jan 08, 2014 at 05:12:50PM +0800, Lei Li wrote:
> This patch series tries to refactor the functions used for 
> exchange of FD in current code, provide common methods
> for it.
> 
> The series is based on the localhost migration with side channel
> for ram series as it was already a good shape. But if you want
> to merge this first, I'll get rid of the migration part. 
> 
> I just tested page flipping migration, and tap/bridge-helper a
> bit, but have some environment problem on proxy fs driver. So 
> it'd be appreciated if someone could help on verifying whether
> it has impact on it. :)
> 
> Please let me know if there is anything needs to be improved.

This looks like a good improvement to me, aside from a couple
of small bugs.

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 1/6] qemu-fd-exchange: provide common methods for exchange fd

[Eric Blake]

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

On 01/08/2014 02:12 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  include/qemu/fd-exchange.h |   25 +++++++++++
>  util/Makefile.objs         |    1 +
>  util/qemu-fd-exchange.c    |   97 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 123 insertions(+), 0 deletions(-)
>  create mode 100644 include/qemu/fd-exchange.h
>  create mode 100644 util/qemu-fd-exchange.c
> 
> diff --git a/include/qemu/fd-exchange.h b/include/qemu/fd-exchange.h
> new file mode 100644
> index 0000000..6929026
> --- /dev/null
> +++ b/include/qemu/fd-exchange.h
> @@ -0,0 +1,25 @@
> +/*
> + * Internel common methods for exchange of FD
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.

Any reason you can't use GPLv2+?  Limiting to exactly version 2 means
your file cannot be copied into programs that want a wider array of
licensing optoins.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/6] qemu-fd-exchange: provide common methods for exchange fd

[Eric Blake]

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

On 01/08/2014 02:12 AM, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  include/qemu/fd-exchange.h |   25 +++++++++++
>  util/Makefile.objs         |    1 +
>  util/qemu-fd-exchange.c    |   97 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 123 insertions(+), 0 deletions(-)
>  create mode 100644 include/qemu/fd-exchange.h
>  create mode 100644 util/qemu-fd-exchange.c
> 
> diff --git a/include/qemu/fd-exchange.h b/include/qemu/fd-exchange.h
> new file mode 100644
> index 0000000..6929026
> --- /dev/null
> +++ b/include/qemu/fd-exchange.h
> @@ -0,0 +1,25 @@
> +/*
> + * Internel common methods for exchange of FD

s/Internel/Internal/


> +++ b/util/qemu-fd-exchange.c
> @@ -0,0 +1,97 @@
> +/*
> + * Internel common methods for exchange of FD

and again.


> +ssize_t qemu_send_with_fd(int sockfd, int passed_fd,
> +                          const void *buf, size_t len)
> +{
> +    struct msghdr msg;
> +    struct iovec iov;
> +    struct cmsghdr *cmsg;
> +    union MsgControl msg_control;
> +    int retval;
> +
> +    iov.iov_base = (int *)buf;
> +    iov.iov_len = len;
> +
> +    memset(&msg, 0, sizeof(msg));
> +    msg.msg_iov = &iov;
> +    msg.msg_iovlen = len;
> +    msg.msg_control = &msg_control;
> +    msg.msg_controllen = sizeof(msg_control);
> +
> +    if (passed_fd < 0) {
> +        *(int *)buf = passed_fd;

Is it safe to assume that buf is aligned well enough to be casting it to
int* then dereferencing it?  Why not just type the parameter correctly
to begin with?  And why are you even writing into the caller's buffer
when they pass a negative fd, but leaving it alone when they pass a
non-negative fd?

> +ssize_t qemu_recv_with_fd(int sockfd, int *passed_fd,
> +                          void *buf, size_t len)
> +{
> +    struct iovec iov;
> +    struct msghdr msg;
> +    struct cmsghdr *cmsg;
> +    union MsgControl msg_control;
> +    int retval;
> +    int data = *(int *)buf;

Again, why not type buf correctly, since otherwise you risk a user
passing in a buffer that is unsuitably aligned for dereferencing as an
int pointer.

> +
> +    iov.iov_base = buf;
> +    iov.iov_len = len;
> +
> +    memset(&msg, 0, sizeof(msg));
> +    msg.msg_iov = &iov;
> +    msg.msg_iovlen = 1;
> +    msg.msg_control = &msg_control;
> +    msg.msg_controllen = sizeof(msg_control);
> +

Should you take advantage of Linux' ability to use MSG_CMSG_CLOEXEC to
guarantee the received fd is atomically marked cloexec when possible?

> +    do {
> +        retval = recvmsg(sockfd, &msg, 0);
> +    } while (retval < 0 && errno == EINTR);
> +
> +    if (retval <= 0) {
> +        return retval;
> +    }
> +
> +    if (data != *(int *)buf) {
> +        *passed_fd = data;
> +        return 0;
> +    }
> +
> +    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> +        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
> +            cmsg->cmsg_level != SOL_SOCKET ||
> +            cmsg->cmsg_type != SCM_RIGHTS) {
> +            continue;
> +        }
> +
> +        memcpy(passed_fd, CMSG_DATA(cmsg), sizeof(*passed_fd));
> +        return 0;
> +    }

And even when MSG_CMSG_CLOEXEC is not available, shouldn't you ensure
that cloexec is set after the fact?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/6] virtfs-proxy-helper: replace send_fd with qemu_send_with_fd

[Lei Li]

On 01/16/2014 06:15 PM, Daniel P. Berrange wrote:
> On Wed, Jan 08, 2014 at 05:12:54PM +0800, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   fsdev/virtfs-proxy-helper.c |   51 ++++++------------------------------------
>>   hw/9pfs/virtio-9p-proxy.h   |    5 ----
>>   2 files changed, 8 insertions(+), 48 deletions(-)
>>
>> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
>> index 713a7b2..44c6e61 100644
>> --- a/fsdev/virtfs-proxy-helper.c
>> +++ b/fsdev/virtfs-proxy-helper.c
>> -static int send_fd(int sockfd, int fd)
>> -{
> ...
>> -    /* No ancillary data on error */
>> -    if (fd < 0) {
>> -        /* fd is really negative errno if the request failed  */
>> -        data = fd;
>> -    } else {
>> -        data = V9FS_FD_VALID;
> The way data is initialized here...
>
>> @@ -784,11 +743,17 @@ static void usage(char *prog)
>>   static int process_reply(int sock, int type,
>>                            struct iovec *out_iovec, int retval)
>>   {
>> +    int data = V9FS_FD_VALID;
>
> Doesn't match what you do here.

Well, it looks like it does not match the original order,
because the 'data' has to be passed to the common methods by
the parameter *buf first, as there would be different data
value set for the check by those callers. But the logical
is the same:

if the passed_fd is negative, 'data' will be set to the
negative fd; otherwise it'll be the check value.

>
>> +
>>       switch (type) {
>>       case T_OPEN:
>>       case T_CREATE:
>> -        if (send_fd(sock, retval) < 0) {
>> +        if (qemu_send_with_fd(sock, retval, &data, sizeof(data)) < 0) {
>>               return -1;
>> +        } else {
>> +            if (retval >= 0) {
>> +                close(retval);
>> +            }
>>           }
>>           break;
>>       case T_MKNOD:
> Regards,
> Daniel


-- 
Lei

Re: [Qemu-devel] [PATCH 5/6] virtio-9p-proxy: replace v9fs_receivefd with qemu_recv_with_fd

[Lei Li]

On 01/16/2014 06:16 PM, Daniel P. Berrange wrote:
> On Wed, Jan 08, 2014 at 05:12:55PM +0800, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   hw/9pfs/virtio-9p-proxy.c |   60 ++------------------------------------------
>>   1 files changed, 3 insertions(+), 57 deletions(-)
>>
>> diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c
>> index 5f44bb7..f34b845 100644
>> --- a/hw/9pfs/virtio-9p-proxy.c
>> +++ b/hw/9pfs/virtio-9p-proxy.c
>> -    do {
>> -        retval = recvmsg(sockfd, &msg, 0);
>> -    } while (retval < 0 && errno == EINTR);
>> -    if (retval <= 0) {
>> -        return retval;
>> -    }
>> -    /*
>> -     * data is set to V9FS_FD_VALID, if ancillary data is sent.  If this
>> -     * request doesn't need ancillary data (fd) or an error occurred,
>> -     * data is set to negative errno value.
>> -     */
>> -    if (data != V9FS_FD_VALID) {
>> -        *status = data;
>> -        return 0;
>> -    }
> This code is handling the 'data' value...
>
>
>> @@ -307,6 +252,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
>>       V9fsString *name, *value;
>>       V9fsString *path, *oldpath;
>>       struct iovec *iovec = NULL, *reply = NULL;
>> +    int data = V9FS_FD_VALID;
>>   
>>       qemu_mutex_lock(&proxy->mutex);
>>   
>> @@ -548,7 +494,7 @@ static int v9fs_request(V9fsProxy *proxy, int type,
>>            * A file descriptor is returned as response for
>>            * T_OPEN,T_CREATE on success
>>            */
>> -        if (v9fs_receivefd(proxy->sockfd, &retval) < 0) {
>> +        if (qemu_recv_with_fd(proxy->sockfd, &retval, &data, sizeof(data)) < 0) {
>>               goto close_error;
>>           }
> ...but this code is ignoring the return value in 'data'.

It is not be ignored. The above logical is put into the common
method, like:

     if (data != *(int *)buf) {
         *passed_fd = data;
         return 0;
     }

>
>
> Daniel


-- 
Lei

Re: [Qemu-devel] [PATCH 1/6] qemu-fd-exchange: provide common methods for exchange fd

[Lei Li]

On 01/16/2014 11:16 PM, Eric Blake wrote:
> On 01/08/2014 02:12 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   include/qemu/fd-exchange.h |   25 +++++++++++
>>   util/Makefile.objs         |    1 +
>>   util/qemu-fd-exchange.c    |   97 ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 123 insertions(+), 0 deletions(-)
>>   create mode 100644 include/qemu/fd-exchange.h
>>   create mode 100644 util/qemu-fd-exchange.c
>>
>> diff --git a/include/qemu/fd-exchange.h b/include/qemu/fd-exchange.h
>> new file mode 100644
>> index 0000000..6929026
>> --- /dev/null
>> +++ b/include/qemu/fd-exchange.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * Internel common methods for exchange of FD
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
> Any reason you can't use GPLv2+?  Limiting to exactly version 2 means
> your file cannot be copied into programs that want a wider array of
> licensing optoins.

Er... it's my miss copy, apologize to this. :(

>


-- 
Lei

Re: [Qemu-devel] [PATCH 1/6] qemu-fd-exchange: provide common methods for exchange fd

[Lei Li]

On 01/16/2014 11:26 PM, Eric Blake wrote:
> On 01/08/2014 02:12 AM, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   include/qemu/fd-exchange.h |   25 +++++++++++
>>   util/Makefile.objs         |    1 +
>>   util/qemu-fd-exchange.c    |   97 ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 123 insertions(+), 0 deletions(-)
>>   create mode 100644 include/qemu/fd-exchange.h
>>   create mode 100644 util/qemu-fd-exchange.c
>>
>> diff --git a/include/qemu/fd-exchange.h b/include/qemu/fd-exchange.h
>> new file mode 100644
>> index 0000000..6929026
>> --- /dev/null
>> +++ b/include/qemu/fd-exchange.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * Internel common methods for exchange of FD
> s/Internel/Internal/
>
>
>> +++ b/util/qemu-fd-exchange.c
>> @@ -0,0 +1,97 @@
>> +/*
>> + * Internel common methods for exchange of FD
> and again.

Good catch! Thanks.

>
>> +ssize_t qemu_send_with_fd(int sockfd, int passed_fd,
>> +                          const void *buf, size_t len)
>> +{
>> +    struct msghdr msg;
>> +    struct iovec iov;
>> +    struct cmsghdr *cmsg;
>> +    union MsgControl msg_control;
>> +    int retval;
>> +
>> +    iov.iov_base = (int *)buf;
>> +    iov.iov_len = len;
>> +
>> +    memset(&msg, 0, sizeof(msg));
>> +    msg.msg_iov = &iov;
>> +    msg.msg_iovlen = len;
>> +    msg.msg_control = &msg_control;
>> +    msg.msg_controllen = sizeof(msg_control);
>> +
>> +    if (passed_fd < 0) {
>> +        *(int *)buf = passed_fd;
> Is it safe to assume that buf is aligned well enough to be casting it to
> int* then dereferencing it?  Why not just type the parameter correctly

That's because there would be different type for this parameter.

> to begin with?  And why are you even writing into the caller's buffer
> when they pass a negative fd, but leaving it alone when they pass a
> non-negative fd?

That's just the original logical of exchange fd for proxy fs driver,

     if (fd < 0) {
         data = fd;
     } else {
         data = V9FS_FD_VALID;
         ....
     }

This common method don't leave it alone when a non-negative fd passed,
it'll be the same as the check value passed from the caller.

>> +ssize_t qemu_recv_with_fd(int sockfd, int *passed_fd,
>> +                          void *buf, size_t len)
>> +{
>> +    struct iovec iov;
>> +    struct msghdr msg;
>> +    struct cmsghdr *cmsg;
>> +    union MsgControl msg_control;
>> +    int retval;
>> +    int data = *(int *)buf;
> Again, why not type buf correctly, since otherwise you risk a user
> passing in a buffer that is unsuitably aligned for dereferencing as an
> int pointer.
>
>> +
>> +    iov.iov_base = buf;
>> +    iov.iov_len = len;
>> +
>> +    memset(&msg, 0, sizeof(msg));
>> +    msg.msg_iov = &iov;
>> +    msg.msg_iovlen = 1;
>> +    msg.msg_control = &msg_control;
>> +    msg.msg_controllen = sizeof(msg_control);
>> +
> Should you take advantage of Linux' ability to use MSG_CMSG_CLOEXEC to
> guarantee the received fd is atomically marked cloexec when possible?

Whether close the fd in the common method depends on the process
of these current users (they are not the same). It'd be better to
let the users handling the close of fd to fit it.



>
>> +    do {
>> +        retval = recvmsg(sockfd, &msg, 0);
>> +    } while (retval < 0 && errno == EINTR);
>> +
>> +    if (retval <= 0) {
>> +        return retval;
>> +    }
>> +
>> +    if (data != *(int *)buf) {
>> +        *passed_fd = data;
>> +        return 0;
>> +    }
>> +
>> +    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
>> +        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
>> +            cmsg->cmsg_level != SOL_SOCKET ||
>> +            cmsg->cmsg_type != SCM_RIGHTS) {
>> +            continue;
>> +        }
>> +
>> +        memcpy(passed_fd, CMSG_DATA(cmsg), sizeof(*passed_fd));
>> +        return 0;
>> +    }
> And even when MSG_CMSG_CLOEXEC is not available, shouldn't you ensure
> that cloexec is set after the fact?

That's a good suggestion, thanks.



-- 
Lei

Re: [Qemu-devel] [PATCH 1/6] qemu-fd-exchange: provide common methods for exchange fd

[Daniel P. Berrange]

On Wed, Jan 08, 2014 at 05:12:51PM +0800, Lei Li wrote:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  include/qemu/fd-exchange.h |   25 +++++++++++
>  util/Makefile.objs         |    1 +
>  util/qemu-fd-exchange.c    |   97 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 123 insertions(+), 0 deletions(-)
>  create mode 100644 include/qemu/fd-exchange.h
>  create mode 100644 util/qemu-fd-exchange.c
> 
> diff --git a/include/qemu/fd-exchange.h b/include/qemu/fd-exchange.h
> new file mode 100644
> index 0000000..6929026
> --- /dev/null
> +++ b/include/qemu/fd-exchange.h
> @@ -0,0 +1,25 @@
> +/*
> + * Internel common methods for exchange of FD
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef FD_EXCHANGE_H
> +#define FD_EXCHANGE_H
> +
> +#include <sys/socket.h>
> +
> +union MsgControl {
> +    struct cmsghdr cmsg;
> +    char control[CMSG_SPACE(sizeof(int))];
> +};
> +
> +ssize_t qemu_send_with_fd(int sockfd, int passed_fd,
> +                          const void *buf, size_t len);
> +
> +ssize_t qemu_recv_with_fd(int sockfd, int *passed_fd,
> +                          void *buf, size_t len);
> +
> +#endif
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index af3e5cb..2fb42bf 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -13,3 +13,4 @@ util-obj-y += hexdump.o
>  util-obj-y += crc32c.o
>  util-obj-y += throttle.o
>  util-obj-y += getauxval.o
> +util-obj-y += qemu-fd-exchange.o
> diff --git a/util/qemu-fd-exchange.c b/util/qemu-fd-exchange.c
> new file mode 100644
> index 0000000..70a3206
> --- /dev/null
> +++ b/util/qemu-fd-exchange.c
> @@ -0,0 +1,97 @@
> +/*
> + * Internel common methods for exchange of FD
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/fd-exchange.h"
> +#include "qemu-common.h"
> +
> +
> +ssize_t qemu_send_with_fd(int sockfd, int passed_fd,
> +                          const void *buf, size_t len)
> +{
> +    struct msghdr msg;
> +    struct iovec iov;
> +    struct cmsghdr *cmsg;
> +    union MsgControl msg_control;
> +    int retval;
> +
> +    iov.iov_base = (int *)buf;
> +    iov.iov_len = len;
> +
> +    memset(&msg, 0, sizeof(msg));
> +    msg.msg_iov = &iov;
> +    msg.msg_iovlen = len;
> +    msg.msg_control = &msg_control;
> +    msg.msg_controllen = sizeof(msg_control);
> +
> +    if (passed_fd < 0) {
> +        *(int *)buf = passed_fd;

You are casting 'char *buf' to an 'int *' but many of the
callers only pass in a pointer to a 'char buf[1]'. So you
are overflowing the array and also likely causing alignment
violations on ARM platforms.

> +    } else {
> +        msg.msg_control = &msg_control;
> +        msg.msg_controllen = sizeof(msg_control);
> +
> +        cmsg = &msg_control.cmsg;
> +        cmsg->cmsg_len = CMSG_LEN(sizeof(passed_fd));
> +        cmsg->cmsg_level = SOL_SOCKET;
> +        cmsg->cmsg_type = SCM_RIGHTS;
> +        memcpy(CMSG_DATA(cmsg), &passed_fd, sizeof(passed_fd));
> +
> +    }
> +
> +    do {
> +        retval = sendmsg(sockfd, &msg, 0);
> +    } while (retval < 0 && errno == EINTR);
> +
> +    return retval;
> +}
> +
> +ssize_t qemu_recv_with_fd(int sockfd, int *passed_fd,
> +                          void *buf, size_t len)
> +{
> +    struct iovec iov;
> +    struct msghdr msg;
> +    struct cmsghdr *cmsg;
> +    union MsgControl msg_control;
> +    int retval;
> +    int data = *(int *)buf;
> +
> +    iov.iov_base = buf;
> +    iov.iov_len = len;
> +
> +    memset(&msg, 0, sizeof(msg));
> +    msg.msg_iov = &iov;
> +    msg.msg_iovlen = 1;
> +    msg.msg_control = &msg_control;
> +    msg.msg_controllen = sizeof(msg_control);
> +
> +    do {
> +        retval = recvmsg(sockfd, &msg, 0);
> +    } while (retval < 0 && errno == EINTR);
> +
> +    if (retval <= 0) {
> +        return retval;
> +    }
> +
> +    if (data != *(int *)buf) {
> +        *passed_fd = data;
> +        return 0;
> +    }

Again cast issues

> +
> +    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> +        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
> +            cmsg->cmsg_level != SOL_SOCKET ||
> +            cmsg->cmsg_type != SCM_RIGHTS) {
> +            continue;
> +        }
> +
> +        memcpy(passed_fd, CMSG_DATA(cmsg), sizeof(*passed_fd));
> +        return 0;
> +    }
> +
> +    *passed_fd = -ENFILE;
> +    return retval;
> +}
> -- 

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 1/6] qemu-fd-exchange: provide common methods for exchange fd

[Lei Li]

On 01/17/2014 06:02 PM, Daniel P. Berrange wrote:
> On Wed, Jan 08, 2014 at 05:12:51PM +0800, Lei Li wrote:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   include/qemu/fd-exchange.h |   25 +++++++++++
>>   util/Makefile.objs         |    1 +
>>   util/qemu-fd-exchange.c    |   97 ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 123 insertions(+), 0 deletions(-)
>>   create mode 100644 include/qemu/fd-exchange.h
>>   create mode 100644 util/qemu-fd-exchange.c
>>
>> diff --git a/include/qemu/fd-exchange.h b/include/qemu/fd-exchange.h
>> new file mode 100644
>> index 0000000..6929026
>> --- /dev/null
>> +++ b/include/qemu/fd-exchange.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * Internel common methods for exchange of FD
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef FD_EXCHANGE_H
>> +#define FD_EXCHANGE_H
>> +
>> +#include <sys/socket.h>
>> +
>> +union MsgControl {
>> +    struct cmsghdr cmsg;
>> +    char control[CMSG_SPACE(sizeof(int))];
>> +};
>> +
>> +ssize_t qemu_send_with_fd(int sockfd, int passed_fd,
>> +                          const void *buf, size_t len);
>> +
>> +ssize_t qemu_recv_with_fd(int sockfd, int *passed_fd,
>> +                          void *buf, size_t len);
>> +
>> +#endif
>> diff --git a/util/Makefile.objs b/util/Makefile.objs
>> index af3e5cb..2fb42bf 100644
>> --- a/util/Makefile.objs
>> +++ b/util/Makefile.objs
>> @@ -13,3 +13,4 @@ util-obj-y += hexdump.o
>>   util-obj-y += crc32c.o
>>   util-obj-y += throttle.o
>>   util-obj-y += getauxval.o
>> +util-obj-y += qemu-fd-exchange.o
>> diff --git a/util/qemu-fd-exchange.c b/util/qemu-fd-exchange.c
>> new file mode 100644
>> index 0000000..70a3206
>> --- /dev/null
>> +++ b/util/qemu-fd-exchange.c
>> @@ -0,0 +1,97 @@
>> +/*
>> + * Internel common methods for exchange of FD
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu/fd-exchange.h"
>> +#include "qemu-common.h"
>> +
>> +
>> +ssize_t qemu_send_with_fd(int sockfd, int passed_fd,
>> +                          const void *buf, size_t len)
>> +{
>> +    struct msghdr msg;
>> +    struct iovec iov;
>> +    struct cmsghdr *cmsg;
>> +    union MsgControl msg_control;
>> +    int retval;
>> +
>> +    iov.iov_base = (int *)buf;
>> +    iov.iov_len = len;
>> +
>> +    memset(&msg, 0, sizeof(msg));
>> +    msg.msg_iov = &iov;
>> +    msg.msg_iovlen = len;
>> +    msg.msg_control = &msg_control;
>> +    msg.msg_controllen = sizeof(msg_control);
>> +
>> +    if (passed_fd < 0) {
>> +        *(int *)buf = passed_fd;
> You are casting 'char *buf' to an 'int *' but many of the
> callers only pass in a pointer to a 'char buf[1]'. So you
> are overflowing the array and also likely causing alignment
> violations on ARM platforms.

You are right, will fix it.

Thanks.

>
>> +    } else {
>> +        msg.msg_control = &msg_control;
>> +        msg.msg_controllen = sizeof(msg_control);
>> +
>> +        cmsg = &msg_control.cmsg;
>> +        cmsg->cmsg_len = CMSG_LEN(sizeof(passed_fd));
>> +        cmsg->cmsg_level = SOL_SOCKET;
>> +        cmsg->cmsg_type = SCM_RIGHTS;
>> +        memcpy(CMSG_DATA(cmsg), &passed_fd, sizeof(passed_fd));
>> +
>> +    }
>> +
>> +    do {
>> +        retval = sendmsg(sockfd, &msg, 0);
>> +    } while (retval < 0 && errno == EINTR);
>> +
>> +    return retval;
>> +}
>> +
>> +ssize_t qemu_recv_with_fd(int sockfd, int *passed_fd,
>> +                          void *buf, size_t len)
>> +{
>> +    struct iovec iov;
>> +    struct msghdr msg;
>> +    struct cmsghdr *cmsg;
>> +    union MsgControl msg_control;
>> +    int retval;
>> +    int data = *(int *)buf;
>> +
>> +    iov.iov_base = buf;
>> +    iov.iov_len = len;
>> +
>> +    memset(&msg, 0, sizeof(msg));
>> +    msg.msg_iov = &iov;
>> +    msg.msg_iovlen = 1;
>> +    msg.msg_control = &msg_control;
>> +    msg.msg_controllen = sizeof(msg_control);
>> +
>> +    do {
>> +        retval = recvmsg(sockfd, &msg, 0);
>> +    } while (retval < 0 && errno == EINTR);
>> +
>> +    if (retval <= 0) {
>> +        return retval;
>> +    }
>> +
>> +    if (data != *(int *)buf) {
>> +        *passed_fd = data;
>> +        return 0;
>> +    }
> Again cast issues
>
>> +
>> +    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
>> +        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
>> +            cmsg->cmsg_level != SOL_SOCKET ||
>> +            cmsg->cmsg_type != SCM_RIGHTS) {
>> +            continue;
>> +        }
>> +
>> +        memcpy(passed_fd, CMSG_DATA(cmsg), sizeof(*passed_fd));
>> +        return 0;
>> +    }
>> +
>> +    *passed_fd = -ENFILE;
>> +    return retval;
>> +}
>> -- 
> Regards,
> Daniel


-- 
Lei