Re: [PATCH v4 3/5] vfio-user: refactor out header handling

["Cédric Le Goater" <clg@redhat.com>]

On 12/3/25 11:03, John Levon wrote:
> Simplify vfio_user_recv_one() by moving the header handling out to a
> helper function.
> 
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
>   hw/vfio-user/proxy.c | 101 +++++++++++++++++++++++++------------------
>   1 file changed, 60 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
> index 82c76c6665..e0f9202535 100644
> --- a/hw/vfio-user/proxy.c
> +++ b/hw/vfio-user/proxy.c
> @@ -218,6 +218,61 @@ static int vfio_user_complete(VFIOUserProxy *proxy, Error **errp)
>       return 1;
>   }
>   
> +static int vfio_user_recv_hdr(VFIOUserProxy *proxy, Error **errp,
> +                              VFIOUserHdr *hdr, int **fdp, size_t *numfdp,
> +                              bool *isreply)
> +{
> +    struct iovec iov = {
> +        .iov_base = hdr,
> +        .iov_len = sizeof(*hdr),
> +    };
> +    int ret;
> +
> +    /*
> +     * Read header
> +     */
> +    ret = qio_channel_readv_full(proxy->ioc, &iov, 1, fdp, numfdp, 0,
> +                                 errp);
> +    if (ret == QIO_CHANNEL_ERR_BLOCK) {
> +        return ret;
> +    }
> +
> +    if (ret < 0) {
> +        error_setg_errno(errp, errno, "failed to read header");
> +        return -1;
> +    } else if (ret == 0) {
> +        error_setg(errp, "failed to read header: EOF");
> +        return -1;
> +    } else if (ret < sizeof(*hdr)) {
> +        error_setg(errp, "short read of header");
> +        return -1;
> +    }
> +
> +    /*
> +     * Validate header
> +     */
> +    if (hdr->size < sizeof(*hdr)) {
> +        error_setg(errp, "bad header size");
> +        return -1;
> +    }
> +
> +    switch (hdr->flags & VFIO_USER_TYPE) {
> +    case VFIO_USER_REQUEST:
> +        *isreply = false;
> +        break;
> +    case VFIO_USER_REPLY:
> +        *isreply = true;
> +        break;
> +    default:
> +        error_setg(errp, "unknown message type");
> +        return -1;
> +    }
> +
> +    trace_vfio_user_recv_hdr(proxy->sockname, hdr->id, hdr->command, hdr->size,
> +                             hdr->flags);
> +    return 0;
> +}
> +
>   /*
>    * Receive and process one incoming message.
>    *
> @@ -230,10 +285,6 @@ static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
>       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;
> @@ -257,45 +308,13 @@ static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)

I wouldn't say vfio_user_recv_one() is nice to read ...

>           /* else fall into reading another msg */
>       }
>   
> -    /*
> -     * Read header
> -     */
> -    ret = qio_channel_readv_full(proxy->ioc, &iov, 1, &fdp, &numfds, 0,
> -                                 errp);
> -    if (ret == QIO_CHANNEL_ERR_BLOCK) {
> -        return ret;
> -    }
> -
> -    /* read error or other side closed connection */
> -    if (ret <= 0) {
> -        goto fatal;
> -    }
> -
> -    if (ret < sizeof(hdr)) {
> -        error_setg(errp, "short read of header");
> -        goto fatal;
> -    }
> -
> -    /*
> -     * Validate header
> -     */
> -    if (hdr.size < sizeof(VFIOUserHdr)) {
> -        error_setg(errp, "bad header size");
> -        goto fatal;
> -    }
> -    switch (hdr.flags & VFIO_USER_TYPE) {
> -    case VFIO_USER_REQUEST:
> -        isreply = false;
> -        break;
> -    case VFIO_USER_REPLY:
> -        isreply = true;
> -        break;
> -    default:
> -        error_setg(errp, "unknown message type");
> +    ret = vfio_user_recv_hdr(proxy, errp, &hdr, &fdp, &numfds, &isreply);
> +    if (ret < 0) {
> +        if (ret == QIO_CHANNEL_ERR_BLOCK) {
> +            return ret;

OK. That's how the goto fatal is avoided.

> +        }
>           goto fatal;
>       }
> -    trace_vfio_user_recv_hdr(proxy->sockname, hdr.id, hdr.command, hdr.size,
> -                             hdr.flags);
>   
>       /*
>        * For replies, find the matching pending request.
Looks ok to me.

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.