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

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

Hello John,

On 12/2/25 11:53, Mark Cave-Ayland wrote:
> On 01/12/2025 09:56, 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..87e50501af 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;
>> +    }
>> +
>> +    /* read error or other side closed connection */
>> +    if (ret <= 0) {
>> +        error_setg(errp, "failed to read header");
> 
> Can we use error_setg_errno() here to record the real underlying return code in errp? Otherwise with this change it gets lost due to vfio_user_recv_hdr() always returning -1 if there is an error.

Can you please send a v4 with this change requested by Mark ?

I will then prepare a last QEMU 10.2 PR with this series and
the vfio-user doc update I left behind.

Thanks,

C.




> 
>> +        return -1;
>> +    }
>> +
>> +    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)
>>           /* 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;
>> +        }
>>           goto fatal;
>>       }
>> -    trace_vfio_user_recv_hdr(proxy->sockname, hdr.id, hdr.command, hdr.size,
>> -                             hdr.flags);
>>       /*
>>        * For replies, find the matching pending request.
> 
> 
> ATB,
> 
> Mark.
>