Re: [Qemu-devel] [PATCH 06/17] migration-local: add send_pipefd()

[Daniel P. Berrange]

On Tue, Dec 03, 2013 at 07:19:40PM +0800, Lei Li wrote:
> On 12/02/2013 05:33 PM, Daniel P. Berrange wrote:
> >On Mon, Dec 02, 2013 at 05:19:06PM +0800, Lei Li wrote:
> >>This patch adds send_pipefd() to pass the pipe file descriptor
> >>to destination process.
> >>
> >>Signed-off-by: Lei Li <address@hidden>
> >>---
> >>  migration-local.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 files changed, 46 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/migration-local.c b/migration-local.c
> >>index 929ed60..f479530 100644
> >>--- a/migration-local.c
> >>+++ b/migration-local.c
> >>@@ -167,3 +167,49 @@ 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;
> >>+}
> >Just a reminder about my comments from previous posting. This is
> >introducing a 3rd private function for sending FDs. The existing
> >code should be refactored into qemu-socket.{c,h} and shared.
> 
> Hi Daniel,
> 
> Yes, I remembered your suggestion. As my reply in the previous version,
> I'll make this refactoring in a separate thread. There are some differences
> between these private functions (like data type and length of bytes
> transmitted), may need a little time to get the common method settle down,
> and would be better to do some test to make sure there is no impact on them.
> And now this is a complete series as an experimental version, do you mind if
> the refactoring would be posted after this series?

IMHO the refactoring should be a pre-requisite of this series. I've seen
too many times where future refactoring was promised but never arrived
because the motivation to fix it is gone once the main series is committed.
It is up to QEMU maintainers though - this is just my personal opinion.

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