Re: [PATCH v3 4/5] vfio-user: simplify vfio_user_recv_one()

[Mark Cave-Ayland <mark.caveayland@nutanix.com>]

On 01/12/2025 09:56, John Levon wrote:

> This function was unnecessarily difficult to understand due to the
> separate handling of request and reply messages. Use common code for
> both where we can.
> 
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
>   hw/vfio-user/proxy.c | 68 +++++++++++++++++++-------------------------
>   1 file changed, 30 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
> index 87e50501af..d1d63816b3 100644
> --- a/hw/vfio-user/proxy.c
> +++ b/hw/vfio-user/proxy.c
> @@ -281,15 +281,14 @@ static int vfio_user_recv_hdr(VFIOUserProxy *proxy, Error **errp,
>    */
>   static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
>   {
> -    VFIOUserMsg *msg = NULL;
>       g_autofree int *fdp = NULL;
> -    VFIOUserFDs *reqfds;
> -    VFIOUserHdr hdr;
> +    VFIOUserMsg *msg = NULL;
>       bool isreply = false;
> -    int i, ret;
> -    size_t msgleft, numfds = 0;
> +    size_t msgleft = 0;
> +    size_t numfds = 0;
>       char *data = NULL;
> -    char *buf = NULL;
> +    VFIOUserHdr hdr;
> +    int i, ret;
>   
>       /*
>        * Complete any partial reads
> @@ -317,8 +316,8 @@ static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
>       }
>   
>       /*
> -     * For replies, find the matching pending request.
> -     * For requests, reap incoming FDs.
> +     * Find the matching request if this is a reply, or initialize a new
> +     * server->client request.
>        */
>       if (isreply) {
>           QTAILQ_FOREACH(msg, &proxy->pending, next) {
> @@ -332,51 +331,44 @@ static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
>           }
>           QTAILQ_REMOVE(&proxy->pending, msg, next);
>   
> -        /*
> -         * Process any received FDs
> -         */
> -        if (numfds != 0) {
> -            if (msg->fds == NULL || msg->fds->recv_fds < numfds) {
> -                error_setg(errp, "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(errp, "reply larger than recv buffer");
>               goto err;
>           }
> -        *msg->hdr = hdr;
> -        data = (char *)msg->hdr + sizeof(hdr);
>       } else {
> +        void *buf;
> +
>           if (hdr.size > proxy->max_xfer_size + sizeof(VFIOUserDMARW)) {
>               error_setg(errp, "vfio_user_recv request larger than max");
>               goto err;
>           }
> +
>           buf = g_malloc0(hdr.size);
> -        memcpy(buf, &hdr, sizeof(hdr));
> -        data = buf + sizeof(hdr);
> -        msg = vfio_user_getmsg(proxy, (VFIOUserHdr *)buf, reqfds);
> +        msg = vfio_user_getmsg(proxy, buf, NULL);
>           msg->type = VFIO_MSG_REQ;
>       }
>   
> +    *msg->hdr = hdr;
> +    data = (char *)msg->hdr + sizeof(hdr);
> +
> +    if (numfds != 0) {
> +        if (msg->type == VFIO_MSG_REQ) {
> +            msg->fds = vfio_user_getfds(numfds);
> +        } else {
> +            if (msg->fds == NULL || msg->fds->recv_fds < numfds) {
> +                error_setg(errp, "unexpected FDs in reply");
> +                goto err;
> +            }
> +            msg->fds->recv_fds = numfds;
> +        }
> +
> +        memcpy(msg->fds->fds, fdp, numfds * sizeof(int));
> +    }
> +
>       /*
> -     * Read rest of message.
> +     * Read rest of message into the data buffer.
>        */
> +
>       msgleft = hdr.size - sizeof(hdr);
>       while (msgleft > 0) {
>           ret = qio_channel_read(proxy->ioc, data, msgleft, errp);

Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>


ATB,

Mark.