Re: [Qemu-devel] [PATCH V2 1/3] qemu-iotests: add unix socket help program

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-8-29 22:50, Luiz Capitulino 写道:
> On Tue, 27 Aug 2013 10:52:09 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> 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 <xiawenc@linux.vnet.ibm.com>
>> ---
>>   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" = "yes" ]; then
>>   fi
>>
>>   # build tree in object directory in case the source is not in the current directory
>> -DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos tests/qapi-schema tests/tcg/xtensa"
>> +DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos tests/qapi-schema tests/tcg/xtensa tests/qemu-iotests"
>>   DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw"
>>   DIRS="$DIRS roms/seabios roms/vgabios"
>>   DIRS="$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-test.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/socket_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-img$(EXESUF) qemu-io$(EXESUF)
>> +check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh qemu-img$(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-iotests/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    <xiawenc@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
>> + * See the COPYING.LIB file in the top-level directory.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <errno.h>
>> +#include <sys/socket.h>
>> +#include <sys/un.h>
>> +#include <stdlib.h>
>> +
>> +/* #define SOCKET_SCM_DEBUG */
>> +
>> +static int send_fd(int fd, int fd_to_send, const char *data, size_t len)
>> +{
>> +    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 == NULL || len <= 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.

>> +
>> +    memset(&msg, 0, sizeof(msg));
>> +    memset(control, 0, sizeof(control));
>> +
>> +    iov[0].iov_base = (void *)data;
>> +    iov[0].iov_len = len;
>> +
>> +    msg.msg_iov = iov;
>> +    msg.msg_iovlen = 1;
>> +
>> +    msg.msg_control = control;
>> +    msg.msg_controllen = sizeof(control);
>> +
>> +    cmsg = CMSG_FIRSTHDR(&msg);
>> +
>> +    cmsg->cmsg_len = CMSG_LEN(sizeof(int));
>> +    cmsg->cmsg_level = SOL_SOCKET;
>> +    cmsg->cmsg_type = SCM_RIGHTS;
>> +    memcpy(CMSG_DATA(cmsg), &fd, sizeof(int));
>> +
>> +    do {
>> +        ret = sendmsg(fd, &msg, 0);
>> +    } while (ret < 0 && errno == 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 = 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.

>> +
>> +    errno = 0;
>> +    sock = 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.

>> +    fd = strtol(argv[2], NULL, 10);
>> +    if (errno) {
>> +        fprintf(stderr, "Failed in strtol for fd to send, reason: %s.\n",
>> +                strerror(errno));
>> +        return 1;
>> +    }
>> +
>> +    buf = argv[3];
>> +    buflen = strlen(buf);
>> +
>> +    ret = send_fd(sock, fd, buf, buflen);
>> +    if (ret < 0) {
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>


-- 
Best Regards

Wenchao Xia