Re: [Qemu-devel] [PATCH 06/17] migration-local: add send_pipefd()
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 li...@linux.vnet.ibm.com --- 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? Daniel -- Lei
Re: [Qemu-devel] [PATCH 06/17] migration-local: add send_pipefd()
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 li...@linux.vnet.ibm.com --- 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 :|
Re: [Qemu-devel] [PATCH 06/17] migration-local: add send_pipefd()
Il 03/12/2013 12:19, Lei Li ha scritto: 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 li...@linux.vnet.ibm.com --- 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. You would have to implement it in such a way that the buffer is specified in the function, for example: 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); The functions can go in util/ (I think not in qemu-socket.c, a new file is preferrable). I don't think it's particularly important, but it's definitely welcome. Paolo
Re: [Qemu-devel] [PATCH 06/17] migration-local: add send_pipefd()
On 12/03/2013 07:35 PM, Daniel P. Berrange wrote: 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 li...@linux.vnet.ibm.com --- 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. Just this is already a good shape and the refactoring may need a little more time since some details might needs to be considered and better to discuss in a separate thread. I am happy to take any chance to contribute to community, as I can learn a lot from you guys and it's really good experience that my work could be useful to lots of people. And I believe this is not my last patch for it. :) Regards, Daniel -- Lei
Re: [Qemu-devel] [PATCH 06/17] migration-local: add send_pipefd()
On 12/03/2013 07:52 PM, Paolo Bonzini wrote: Il 03/12/2013 12:19, Lei Li ha scritto: 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 li...@linux.vnet.ibm.com --- 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. You would have to implement it in such a way that the buffer is specified in the function, for example: 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); The functions can go in util/ (I think not in qemu-socket.c, a new file is preferrable). I don't think it's particularly important, but it's definitely welcome. Hi Paolo, Thanks for your specified suggestion! As it needs to test the related code (tap/bridge Proxy FS flipping migration), I will work on it after back from my vacation next week. :-) Paolo -- Lei
Re: [Qemu-devel] [PATCH 06/17] migration-local: add send_pipefd()
On 11/29/2013 07:14 PM, Daniel P. Berrange wrote: On Fri, Nov 29, 2013 at 06:06:13PM +0800, Lei Li wrote: This patch adds send_pipefd() to pass the pipe file descriptor to destination process. Signed-off-by: Lei Li li...@linux.vnet.ibm.com --- 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; +} There are already two copies of this function in QEMU, not to mention several copies of code for receving FDs. Rather than adding yet more copies of this functionality it would be much better to add 2 methods to util/qemu-sockets.{c,h} for sending and receiving file descriptors and update all existing code to use them. Hi Daniel, Make sense, sounds like a good plan to me. Just take a quick look, seems there are some differences between them, I will have a try in a separate thread after back from my vacation next week. Thanks for your suggestion. Daniel -- Lei
Re: [Qemu-devel] [PATCH 06/17] migration-local: add send_pipefd()
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 li...@linux.vnet.ibm.com --- 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. 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 06/17] migration-local: add send_pipefd()
On Fri, Nov 29, 2013 at 06:06:13PM +0800, Lei Li wrote: This patch adds send_pipefd() to pass the pipe file descriptor to destination process. Signed-off-by: Lei Li li...@linux.vnet.ibm.com --- 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; +} There are already two copies of this function in QEMU, not to mention several copies of code for receving FDs. Rather than adding yet more copies of this functionality it would be much better to add 2 methods to util/qemu-sockets.{c,h} for sending and receiving file descriptors and update all existing code to use them. 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 06/17] migration-local: add send_pipefd()
Il 21/11/2013 10:11, Lei Li ha scritto: +struct cmsghdr *cmptr; +char req[1] = { 0x01 }; About this, see my reply to patch 8. +if (pipefd 0) { +msg.msg_control = NULL; +msg.msg_controllen = 0; +/* Negative status means error */ +req[0] = pipefd; No need for this. qemu_fopen_socket_local has failed already, and you will never get here. Paolo +} else { +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; +}