RE: [RFC v4 08/21] vfio-user: define socket receive functions

From: Thanos Makatos <thanos.makatos@nutanix.com>
To: Thanos Makatos <thanos.makatos@nutanix.com>,
	John Johnson <john.g.johnson@oracle.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: RE: [RFC v4 08/21] vfio-user: define socket receive functions
Date: Tue, 15 Feb 2022 14:50:24 +0000	[thread overview]
Message-ID: <DM8PR02MB80052AF463504BB4BBA397F48B349@DM8PR02MB8005.namprd02.prod.outlook.com> (raw)
In-Reply-To: <DM8PR02MB8005963C1D2E796FB7420FBC8B349@DM8PR02MB8005.namprd02.prod.outlook.com>

> > +/*
> > + * Receive and process one incoming message.
> > + *
> > + * For replies, find matching outgoing request and wake any waiters.
> > + * For requests, queue in incoming list and run request BH.
> > + */
> > +static int vfio_user_recv_one(VFIOProxy *proxy)
> > +{
> > +    VFIOUserMsg *msg = NULL;
> > +    g_autofree int *fdp = NULL;
> > +    VFIOUserFDs *reqfds;
> > +    VFIOUserHdr hdr;
> > +    struct iovec iov = {
> > +        .iov_base = &hdr,
> > +        .iov_len = sizeof(hdr),
> > +    };
> > +    bool isreply = false;
> > +    int i, ret;
> > +    size_t msgleft, numfds = 0;
> > +    char *data = NULL;
> > +    char *buf = NULL;
> > +    Error *local_err = NULL;
> > +
> > +    /*
> > +     * Read header
> > +     */
> > +    ret = qio_channel_readv_full(proxy->ioc, &iov, 1, &fdp, &numfds,
> > +                                 &local_err);
> > +    if (ret == QIO_CHANNEL_ERR_BLOCK) {
> > +        return ret;
> > +    }
> > +    if (ret <= 0) {
> > +        /* read error or other side closed connection */
> > +        if (ret == 0) {
> > +            error_setg(&local_err, "vfio_user_recv server closed socket");
> > +        } else {
> > +            error_prepend(&local_err, "vfio_user_recv");
> > +        }
> > +        goto fatal;
> > +    }
> > +    if (ret < sizeof(msg)) {
> > +        error_setg(&local_err, "vfio_user_recv short read of header");
> > +        goto fatal;
> > +    }
> 
> Print received size for debug purposes?
> 
> > +
> > +    /*
> > +     * Validate header
> > +     */
> > +    if (hdr.size < sizeof(VFIOUserHdr)) {
> > +        error_setg(&local_err, "vfio_user_recv bad header size");
> > +        goto fatal;
> > +    }
> 
> Print header size?
> 
> > +    switch (hdr.flags & VFIO_USER_TYPE) {
> > +    case VFIO_USER_REQUEST:
> > +        isreply = false;
> > +        break;
> > +    case VFIO_USER_REPLY:
> > +        isreply = true;
> > +        break;
> > +    default:
> > +        error_setg(&local_err, "vfio_user_recv unknown message type");
> > +        goto fatal;
> > +    }
> 
> Print message type?
> 
> > +
> > +    /*
> > +     * For replies, find the matching pending request.
> > +     * For requests, reap incoming FDs.
> > +     */
> > +    if (isreply) {
> > +        QTAILQ_FOREACH(msg, &proxy->pending, next) {
> > +            if (hdr.id == msg->id) {
> > +                break;
> > +            }
> > +        }
> > +        if (msg == NULL) {
> > +            error_setg(&local_err, "vfio_user_recv unexpected reply");
> > +            goto err;
> > +        }
> > +        QTAILQ_REMOVE(&proxy->pending, msg, next);
> > +
> > +        /*
> > +         * Process any received FDs
> > +         */
> > +        if (numfds != 0) {
> > +            if (msg->fds == NULL || msg->fds->recv_fds < numfds) {
> > +                error_setg(&local_err, "vfio_user_recv unexpected FDs");
> > +                goto err;
> > +            }
> > +            msg->fds->recv_fds = numfds;
> > +            memcpy(msg->fds->fds, fdp, numfds * sizeof(int));
> > +        }
> > +    } else {
> > +        if (numfds != 0) {
> > +            reqfds = vfio_user_getfds(numfds);
> > +            memcpy(reqfds->fds, fdp, numfds * sizeof(int));
> > +        } else {
> > +            reqfds = NULL;
> > +        }
> > +    }
> > +
> > +    /*
> > +     * Put the whole message into a single buffer.
> > +     */
> > +    if (isreply) {
> > +        if (hdr.size > msg->rsize) {
> > +            error_setg(&local_err,
> > +                       "vfio_user_recv reply larger than recv buffer");
> > +            goto err;
> > +        }
> 
> Print hdr.size and msg->rsize?
> 
> > +        *msg->hdr = hdr;
> > +        data = (char *)msg->hdr + sizeof(hdr);
> > +    } else {
> > +        if (hdr.size > max_xfer_size) {
> > +            error_setg(&local_err, "vfio_user_recv request larger than max");
> > +            goto err;
> > +        }
> 
> Print hdr.size?

On second thought, should we dump the entire header in case of such errors? If not by default then at least in debug builds?