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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]