Re: [PATCH v8 06/20] multi-process: define MPQemuMsg format and transmission functions

[Stefan Hajnoczi]

On Fri, Jul 31, 2020 at 02:20:13PM -0400, Jagannathan Raman wrote:
> +static int mpqemu_readv(QIOChannel *ioc, struct iovec *iov, int **fds,
> +                        size_t *nfds, Error **errp)

readv(2) and similar functions take an int iovcnt argument while
mpqemu_readv() takes just a single struct iovec. The name mpqemu_read()
seems more appopriate and iov argument could be replaced by void *buf,
size_t len. That is cleaner because this function modifies the iov
argument (callers need to be be careful!).

> +{
> +    struct iovec *l_iov = iov;
> +    size_t *l_nfds = nfds;
> +    unsigned int niov = 1;
> +    int **l_fds = fds;
> +    size_t size, len;
> +    Error *local_err = NULL;
> +
> +    size = iov->iov_len;
> +
> +    while (size > 0) {
> +        len = qio_channel_readv_full(ioc, l_iov, niov, l_fds, l_nfds,
> +                                     &local_err);
> +
> +        if (len == QIO_CHANNEL_ERR_BLOCK) {
> +            if (qemu_in_coroutine()) {
> +                qio_channel_yield(ioc, G_IO_IN);
> +            } else {
> +                qio_channel_wait(ioc, G_IO_IN);
> +            }
> +            continue;
> +        }
> +
> +        if (len <= 0) {

size_t is unsigned so this does not detect negative values. Please use
qio_channel_readv_full()'s return type (ssize_t) instead.

> +            error_propagate(errp, local_err);
> +            return -EIO;
> +        }
> +
> +        l_fds = NULL;
> +        l_nfds = NULL;
> +
> +        size -= len;
> +
> +        (void)iov_discard_front(&l_iov, &niov, len);
> +    }
> +
> +    return iov->iov_len;

read()-style functions return the number of bytes read. mpqemu_readv()
returns the number of unread bytes remaining. Maintaining consistency
with read() function conventions will reduce the chance of bugs, so I
suggest returning the number of bytes read instead.

> +}
> +
> +void mpqemu_msg_recv(MPQemuMsg *msg, QIOChannel *ioc, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    int *fds = NULL;
> +    struct iovec hdr, data;
> +    size_t nfds = 0;
> +
> +    hdr.iov_base = msg;
> +    hdr.iov_len = MPQEMU_MSG_HDR_SIZE;
> +
> +    if (mpqemu_readv(ioc, &hdr, &fds, &nfds, &local_err) < 0) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }

Can we read less than MPQEMU_MSG_HDR_SIZE if the peer closes the socket?
That case probably needs to be handled.

> +void mpqemu_msg_cleanup(MPQemuMsg *msg)
> +{
> +    if (msg->data2) {
> +        free(msg->data2);

The buffer was allocated with g_malloc0() so g_free() should be used to
free it.

signature.asc
Description: PGP signature