From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49810) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VFMx5-0002vN-H2 for qemu-devel@nongnu.org; Fri, 30 Aug 2013 07:33:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VFMx0-0003Zi-H5 for qemu-devel@nongnu.org; Fri, 30 Aug 2013 07:33:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46976) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VFMx0-0003ZQ-9H for qemu-devel@nongnu.org; Fri, 30 Aug 2013 07:33:14 -0400 Date: Fri, 30 Aug 2013 07:33:10 -0400 From: Luiz Capitulino Message-ID: <20130830073310.4a4f20c6@redhat.com> In-Reply-To: <52200693.6070602@linux.vnet.ibm.com> References: <1377571931-9144-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1377571931-9144-2-git-send-email-xiawenc@linux.vnet.ibm.com> <20130829105007.37454b3d@redhat.com> <52200693.6070602@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH V2 1/3] qemu-iotests: add unix socket help program List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, anthony@codemonkey.ws, armbru@redhat.com On Fri, 30 Aug 2013 10:42:27 +0800 Wenchao Xia wrote: > =E4=BA=8E 2013-8-29 22:50, Luiz Capitulino =E5=86=99=E9=81=93: > > On Tue, 27 Aug 2013 10:52:09 +0800 > > Wenchao Xia wrote: > > > >> This program can do a sendmsg call to transfer fd with unix > >> socket, which is not supported in python2. > >> > >> The built binary will not be deleted in clean, but it is a > >> existing issue in ./tests, which should be solved in another > >> patch. > > > > Review comments in addition to Eric's. > > > >> > >> Signed-off-by: Wenchao Xia > >> --- > >> configure | 2 +- > >> tests/Makefile | 4 +- > >> tests/qemu-iotests/socket_scm_helper.c | 119 ++++++++++++++++++++++= ++++++++++ > >> 3 files changed, 123 insertions(+), 2 deletions(-) > >> create mode 100644 tests/qemu-iotests/socket_scm_helper.c > >> > >> diff --git a/configure b/configure > >> index 0a55c20..5080c38 100755 > >> --- a/configure > >> +++ b/configure > >> @@ -4544,7 +4544,7 @@ if [ "$dtc_internal" =3D "yes" ]; then > >> fi > >> > >> # build tree in object directory in case the source is not in the cu= rrent directory > >> -DIRS=3D"tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos te= sts/qapi-schema tests/tcg/xtensa" > >> +DIRS=3D"tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos te= sts/qapi-schema tests/tcg/xtensa tests/qemu-iotests" > >> DIRS=3D"$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw" > >> DIRS=3D"$DIRS roms/seabios roms/vgabios" > >> DIRS=3D"$DIRS qapi-generated" > >> diff --git a/tests/Makefile b/tests/Makefile > >> index baba9e9..d2f3fcb 100644 > >> --- a/tests/Makefile > >> +++ b/tests/Makefile > >> @@ -172,6 +172,8 @@ tests/boot-order-test$(EXESUF): tests/boot-order-t= est.o $(libqos-obj-y) > >> tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y) > >> tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y) > >> tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y) > >> +tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/soc= ket_scm_helper.o > >> + $(call LINK, $^) > >> > >> # QTest rules > >> > >> @@ -250,7 +252,7 @@ check-report.html: check-report.xml > >> # Other tests > >> > >> .PHONY: check-tests/qemu-iotests-quick.sh > >> -check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh qemu-i= mg$(EXESUF) qemu-io$(EXESUF) > >> +check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh qemu-i= mg$(EXESUF) qemu-io$(EXESUF) tests/qemu-iotests/socket_scm_helper$(EXESUF) > >> $< > >> > >> .PHONY: check-tests/test-qapi.py > >> diff --git a/tests/qemu-iotests/socket_scm_helper.c b/tests/qemu-iotes= ts/socket_scm_helper.c > >> new file mode 100644 > >> index 0000000..1955a3e > >> --- /dev/null > >> +++ b/tests/qemu-iotests/socket_scm_helper.c > >> @@ -0,0 +1,119 @@ > >> +/* > >> + * SCM_RIGHT with unix socket help program for test > >> + * > >> + * Copyright IBM, Inc. 2013 > >> + * > >> + * Authors: > >> + * Wenchao Xia > >> + * > >> + * This work is licensed under the terms of the GNU LGPL, version 2 o= r later. > >> + * See the COPYING.LIB file in the top-level directory. > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +/* #define SOCKET_SCM_DEBUG */ > >> + > >> +static int send_fd(int fd, int fd_to_send, const char *data, size_t l= en) > >> +{ > >> + struct msghdr msg; > >> + struct iovec iov[1]; > >> + int ret; > >> + char control[CMSG_SPACE(sizeof(int))]; > >> + struct cmsghdr *cmsg; > >> + > >> + if (fd < 0) { > >> + fprintf(stderr, "Socket fd is invalid.\n"); > >> + return -1; > >> + } > >> + if (fd_to_send < 0) { > >> + fprintf(stderr, "Fd to send is invalid.\n"); > >> + return -1; > >> + } > >> + if (data =3D=3D NULL || len <=3D 0) { > >> + fprintf(stderr, "Data buffer must be valid.\n"); > >> + return -1; > >> + } > > > > I haven't looked at the test itself yet, but my idea for the helper was > > to have something like "pass-fd < file > < qmp-socket >". So, what the > > helper does is to open 'file' and pass its fd to 'qmp-socket'. Seems > > simpler to me. > > > OK, will use that style. Do you think the data parameter should be > kept? I added it to let test framework choose one char, which can avoid > break the qmp protocol. If you're passing a QMP command there then it can stay. >=20 > >> + > >> + memset(&msg, 0, sizeof(msg)); > >> + memset(control, 0, sizeof(control)); > >> + > >> + iov[0].iov_base =3D (void *)data; > >> + iov[0].iov_len =3D len; > >> + > >> + msg.msg_iov =3D iov; > >> + msg.msg_iovlen =3D 1; > >> + > >> + msg.msg_control =3D control; > >> + msg.msg_controllen =3D sizeof(control); > >> + > >> + cmsg =3D CMSG_FIRSTHDR(&msg); > >> + > >> + cmsg->cmsg_len =3D CMSG_LEN(sizeof(int)); > >> + cmsg->cmsg_level =3D SOL_SOCKET; > >> + cmsg->cmsg_type =3D SCM_RIGHTS; > >> + memcpy(CMSG_DATA(cmsg), &fd, sizeof(int)); > >> + > >> + do { > >> + ret =3D sendmsg(fd, &msg, 0); > >> + } while (ret < 0 && errno =3D=3D EINTR); > >> + > >> + if (ret < 0) { > >> + fprintf(stderr, "Failed to send msg, reason: %s.\n", strerror= (errno)); > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +/* > >> + * To make things simple, the caller need to specify: > >> + * 1. socket fd. > >> + * 2. fd to send. > >> + * 3. msg to send. > >> + */ > >> +int main(int argc, char **argv, char **envp) > >> +{ > >> + int sock, fd, ret, buflen; > >> + const char *buf; > >> +#ifdef SOCKET_SCM_DEBUG > >> + int i; > >> + for (i =3D 0; i < argc; i++) { > >> + fprintf(stderr, "Parameter %d: %s.\n", i, argv[i]); > >> + } > >> +#endif > >> + > >> + if (argc < 4) { > >> + fprintf(stderr, > >> + "Invalid parameter, use it as:\n" > >> + "%s SOCKET_FD FD_TO_SEND MSG.\n", > >> + argv[0]); > >> + return 1; > >> + } > > > > Some like "usage: %s < socket-fd > < fd-to-send > < msg >\n" is more > > readable, IMO. > > > It is better, will use that. >=20 > >> + > >> + errno =3D 0; > >> + sock =3D strtol(argv[1], NULL, 10); > >> + if (errno) { > >> + fprintf(stderr, "Failed in strtol for socket fd, reason: %s.\= n", > >> + strerror(errno)); > >> + return 1; > >> + } > > > > Apart from what Eric suggested, you could move this to a function. > > > OK. >=20 > >> + fd =3D strtol(argv[2], NULL, 10); > >> + if (errno) { > >> + fprintf(stderr, "Failed in strtol for fd to send, reason: %s.= \n", > >> + strerror(errno)); > >> + return 1; > >> + } > >> + > >> + buf =3D argv[3]; > >> + buflen =3D strlen(buf); > >> + > >> + ret =3D send_fd(sock, fd, buf, buflen); > >> + if (ret < 0) { > >> + return 1; > >> + } > >> + return 0; > >> +} > > >=20 >=20