[PATCH v2 0/5] vfio-user coverity fixes

[PATCH v2 0/5] vfio-user coverity fixes

[John Levon]

Fix some coverity-reported issues with error handling in vfio-user message
processing.

John Levon (5):
  vfio-user: simplify vfio_user_process()
  vfio-user: clarify partial message handling
  vfio-user: refactor out header handling
  vfio-user: simplify vfio_user_recv_one()
  vfio-user: recycle msg on failure

 hw/vfio-user/proxy.c | 204 ++++++++++++++++++++++++-------------------
 1 file changed, 113 insertions(+), 91 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] vfio-user: simplify vfio_user_process()

[John Levon]

It can figure out if it's a reply by itself, rather than passing that
information in.

Signed-off-by: John Levon <john.levon@nutanix.com>
---
 hw/vfio-user/proxy.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
index bbd7ec243d..75845d7c89 100644
--- a/hw/vfio-user/proxy.c
+++ b/hw/vfio-user/proxy.c
@@ -147,8 +147,7 @@ VFIOUserFDs *vfio_user_getfds(int numfds)
 /*
  * Process a received message.
  */
-static void vfio_user_process(VFIOUserProxy *proxy, VFIOUserMsg *msg,
-                              bool isreply)
+static void vfio_user_process(VFIOUserProxy *proxy, VFIOUserMsg *msg)
 {
 
     /*
@@ -157,7 +156,7 @@ static void vfio_user_process(VFIOUserProxy *proxy, VFIOUserMsg *msg,
      *
      * Requests get queued for the BH.
      */
-    if (isreply) {
+    if ((msg->hdr->flags & VFIO_USER_TYPE) == VFIO_USER_REPLY) {
         msg->complete = true;
         if (msg->type == VFIO_MSG_WAIT) {
             qemu_cond_signal(&msg->cv);
@@ -187,7 +186,6 @@ static int vfio_user_complete(VFIOUserProxy *proxy, Error **errp)
 {
     VFIOUserMsg *msg = proxy->part_recv;
     size_t msgleft = proxy->recv_left;
-    bool isreply;
     char *data;
     int ret;
 
@@ -214,8 +212,7 @@ static int vfio_user_complete(VFIOUserProxy *proxy, Error **errp)
      */
     proxy->part_recv = NULL;
     proxy->recv_left = 0;
-    isreply = (msg->hdr->flags & VFIO_USER_TYPE) == VFIO_USER_REPLY;
-    vfio_user_process(proxy, msg, isreply);
+    vfio_user_process(proxy, msg);
 
     /* return positive value */
     return 1;
@@ -381,7 +378,7 @@ static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
         data += ret;
     }
 
-    vfio_user_process(proxy, msg, isreply);
+    vfio_user_process(proxy, msg);
     return 0;
 
     /*
-- 
2.43.0

[PATCH v2 2/5] vfio-user: clarify partial message handling

[John Levon]

Improve a comment for this.

Signed-off-by: John Levon <john.levon@nutanix.com>
---
 hw/vfio-user/proxy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
index 75845d7c89..82c76c6665 100644
--- a/hw/vfio-user/proxy.c
+++ b/hw/vfio-user/proxy.c
@@ -362,7 +362,10 @@ static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
     while (msgleft > 0) {
         ret = qio_channel_read(proxy->ioc, data, msgleft, errp);
 
-        /* prepare to complete read on next iternation */
+        /*
+         * We'll complete this read on the next go around; keep track of the
+         * partial message until then.
+         */
         if (ret == QIO_CHANNEL_ERR_BLOCK) {
             proxy->part_recv = msg;
             proxy->recv_left = msgleft;
-- 
2.43.0

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

[John Levon]

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");
+        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.
-- 
2.43.0

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

[John Levon]

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..aa5b971fb6 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");
+                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);
-- 
2.43.0

[PATCH v2 5/5] vfio-user: recycle msg on failure

[John Levon]

If we fail to read an incoming request, recycle the message.

Resolves: Coverity CID 1611807
Resolves: Coverity CID 1611808
Signed-off-by: John Levon <john.levon@nutanix.com>
---
 hw/vfio-user/proxy.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
index aa5b971fb6..28542a5e83 100644
--- a/hw/vfio-user/proxy.c
+++ b/hw/vfio-user/proxy.c
@@ -412,11 +412,22 @@ err:
     for (i = 0; i < numfds; i++) {
         close(fdp[i]);
     }
-    if (isreply && msg != NULL) {
-        /* force an error to keep sending thread from hanging */
-        vfio_user_set_error(msg->hdr, EINVAL);
-        msg->complete = true;
-        qemu_cond_signal(&msg->cv);
+    if (msg != NULL) {
+        if (msg->type == VFIO_MSG_REQ) {
+            /*
+             * Clean up the request message on failure. Change type back to
+             * NOWAIT to free.
+             */
+            msg->type = VFIO_MSG_NOWAIT;
+            vfio_user_recycle(proxy, msg);
+        } else {
+            /*
+             * Report an error back to the sender. Sender will recycle msg.
+             */
+            vfio_user_set_error(msg->hdr, EINVAL);
+            msg->complete = true;
+            qemu_cond_signal(&msg->cv);
+        }
     }
     return -1;
 }
-- 
2.43.0

Re: [PATCH v2 5/5] vfio-user: recycle msg on failure

[John Levon]

On Mon, Nov 17, 2025 at 09:26:56PM +0530, John Levon wrote:

> If we fail to read an incoming request, recycle the message.
> 
> Resolves: Coverity CID 1611807
> Resolves: Coverity CID 1611808
> Signed-off-by: John Levon <john.levon@nutanix.com>

Peter, I did not hear back about a coverity account so was unable to directly
test this; could you please help out?

thanks
john

Re: [PATCH v2 1/5] vfio-user: simplify vfio_user_process()

[Cédric Le Goater]

On 11/17/25 16:56, John Levon wrote:
> It can figure out if it's a reply by itself, rather than passing that
> information in.
> 
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
>   hw/vfio-user/proxy.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
> index bbd7ec243d..75845d7c89 100644
> --- a/hw/vfio-user/proxy.c
> +++ b/hw/vfio-user/proxy.c
> @@ -147,8 +147,7 @@ VFIOUserFDs *vfio_user_getfds(int numfds)
>   /*
>    * Process a received message.
>    */
> -static void vfio_user_process(VFIOUserProxy *proxy, VFIOUserMsg *msg,
> -                              bool isreply)
> +static void vfio_user_process(VFIOUserProxy *proxy, VFIOUserMsg *msg)
>   {
>   
>       /*
> @@ -157,7 +156,7 @@ static void vfio_user_process(VFIOUserProxy *proxy, VFIOUserMsg *msg,
>        *
>        * Requests get queued for the BH.
>        */
> -    if (isreply) {
> +    if ((msg->hdr->flags & VFIO_USER_TYPE) == VFIO_USER_REPLY) {
>           msg->complete = true;
>           if (msg->type == VFIO_MSG_WAIT) {
>               qemu_cond_signal(&msg->cv);
> @@ -187,7 +186,6 @@ static int vfio_user_complete(VFIOUserProxy *proxy, Error **errp)
>   {
>       VFIOUserMsg *msg = proxy->part_recv;
>       size_t msgleft = proxy->recv_left;
> -    bool isreply;
>       char *data;
>       int ret;
>   
> @@ -214,8 +212,7 @@ static int vfio_user_complete(VFIOUserProxy *proxy, Error **errp)
>        */
>       proxy->part_recv = NULL;
>       proxy->recv_left = 0;
> -    isreply = (msg->hdr->flags & VFIO_USER_TYPE) == VFIO_USER_REPLY;
> -    vfio_user_process(proxy, msg, isreply);
> +    vfio_user_process(proxy, msg);
>   
>       /* return positive value */
>       return 1;
> @@ -381,7 +378,7 @@ static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
>           data += ret;
>       }
>   
> -    vfio_user_process(proxy, msg, isreply);
> +    vfio_user_process(proxy, msg);
>       return 0;
>   
>       /*


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

Thanks,

C.

Re: [PATCH v2 2/5] vfio-user: clarify partial message handling

[Cédric Le Goater]

On 11/17/25 16:56, John Levon wrote:
> Improve a comment for this.
> 
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
>   hw/vfio-user/proxy.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
> index 75845d7c89..82c76c6665 100644
> --- a/hw/vfio-user/proxy.c
> +++ b/hw/vfio-user/proxy.c
> @@ -362,7 +362,10 @@ static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
>       while (msgleft > 0) {
>           ret = qio_channel_read(proxy->ioc, data, msgleft, errp);
>   
> -        /* prepare to complete read on next iternation */
> +        /*
> +         * We'll complete this read on the next go around; keep track of the
> +         * partial message until then.
> +         */
>           if (ret == QIO_CHANNEL_ERR_BLOCK) {
>               proxy->part_recv = msg;
>               proxy->recv_left = msgleft;


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

Thanks,

C.

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

[Cédric Le Goater]

On 11/17/25 16: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(-)
> 


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

Thanks,

C.

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

[Cédric Le Goater]

On 11/17/25 16: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.

It's still difficult to read :) Could we have feedback from Thanos or Mark ?

Thanks,

C.




> 
> 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..aa5b971fb6 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");
> +                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);

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

[John Levon]

On Tue, Nov 18, 2025 at 03:57:56PM +0100, Cédric Le Goater wrote:

> On 11/17/25 16: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.
> 
> It's still difficult to read :) Could we have feedback from Thanos or Mark ?

It is surely better though?

regards
john

Re: [PATCH v2 5/5] vfio-user: recycle msg on failure

[Philippe Mathieu-Daudé]

Hi John,

On 17/11/25 16:58, John Levon wrote:
> On Mon, Nov 17, 2025 at 09:26:56PM +0530, John Levon wrote:
> 
>> If we fail to read an incoming request, recycle the message.
>>
>> Resolves: Coverity CID 1611807
>> Resolves: Coverity CID 1611808
>> Signed-off-by: John Levon <john.levon@nutanix.com>
> 
> Peter, I did not hear back about a coverity account so was unable to directly
> test this; could you please help out?

IIRC you just need to https://scan.coverity.com/users/sign_up then
add the https://scan.coverity.com/projects/qemu project.

Regards,

Phil.

Re: [PATCH v2 5/5] vfio-user: recycle msg on failure

[John Levon]

On Tue, Nov 18, 2025 at 04:18:00PM +0100, Philippe Mathieu-Daudé wrote:

> Hi John,
> 
> On 17/11/25 16:58, John Levon wrote:
> > On Mon, Nov 17, 2025 at 09:26:56PM +0530, John Levon wrote:
> > 
> > > If we fail to read an incoming request, recycle the message.
> > > 
> > > Resolves: Coverity CID 1611807
> > > Resolves: Coverity CID 1611808
> > > Signed-off-by: John Levon <john.levon@nutanix.com>
> > 
> > Peter, I did not hear back about a coverity account so was unable to directly
> > test this; could you please help out?
> 
> IIRC you just need to

Thanks - sorry, but not clear how to submit my patch for a scan now

regards
john

Re: [PATCH v2 5/5] vfio-user: recycle msg on failure

[John Levon]

On Tue, Nov 18, 2025 at 04:18:00PM +0100, Philippe Mathieu-Daudé wrote:

> > Peter, I did not hear back about a coverity account so was unable to directly
> > test this; could you please help out?

I think I do not have permissions to submit a build.

I placed my coverity build output here:
http://movementarian.org/~movement/cov-int.tar.xz

regards
john

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

[Cédric Le Goater]

On 11/18/25 16:03, John Levon wrote:
> On Tue, Nov 18, 2025 at 03:57:56PM +0100, Cédric Le Goater wrote:
> 
>> On 11/17/25 16: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.
>>
>> It's still difficult to read :) Could we have feedback from Thanos or Mark ?
> 
> It is surely better though?
> 
It seems so but I lack the expertise (and time) to dig in. I am waiting
for further feedback before adding them to vfio-next. We still roughly
have 10days for QEMU 10.2. That would be nice.


Thanks,

C.

RE: [PATCH v2 4/5] vfio-user: simplify vfio_user_recv_one()

[Thanos Makatos]

> -----Original Message-----
> From: John Levon <john.levon@nutanix.com>
> Sent: 17 November 2025 15:57
> To: qemu-devel@nongnu.org
> Cc: John Levon <john.levon@nutanix.com>; Thanos Makatos
> <thanos.makatos@nutanix.com>; Cédric Le Goater <clg@redhat.com>; Peter
> Maydell <peter.maydell@linaro.org>
> Subject: [PATCH v2 4/5] vfio-user: simplify vfio_user_recv_one()
> 
> 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..aa5b971fb6 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");

nit, "unexpected FDs in response"?

> +                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);
> --
> 2.43.0

Reviewed-by: Thanos Makatos <thanos.makatos@nutanix.com>

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

[Peter Maydell]

On Tue, 18 Nov 2025 at 14:58, Cédric Le Goater <clg@redhat.com> wrote:
>
> On 11/17/25 16: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.
>
> It's still difficult to read :) Could we have feedback from Thanos or Mark ?

FWIW, when I suggested a restructure of this (because it popped
up in a false positive Coverity warning because Coverity couldn't
figure out the logic in it) the shape of code I had in mind
was something like

static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
{
    /* common prep, header read, etc code here */

    switch (hdr.flags & VFIO_USER_TYPE) {
    case VFIO_USER_REQUEST:
        return vfio_user_handle_request(...stuff...);
        break;
    case VFIO_USER_REPLY:
        return vfio_user_handle_request(...stuff...);
        break;
    default:
        error_setg(errp, "unknown message type");
        goto fatal;
    }
}

and then use a shared helper function for the
"read rest of message" logic which is the only other
part the request and reply codepaths seem to share
once they diverge.

In particular this means that the error-exit logic
doesn't have to have "if (isreply)" checks in it,
because the error-exit path in _handle_reply only
deals with tidying up the reply path, and similarly
for the path in _handle_request. This is the kind of
"codepaths diverge and then rejoin and then diverge
again" flow that Coverity (and I think human readers)
tends to have trouble with.

There are some fiddly loose ends here so it's not quite
as neat as the above pseudocode sketch (e.g. maybe the
handle_* functions need to return a value and
vfio_user_recv_one does some cleanup on failure too).
But anyway, that was the thing I had in my head.

thanks
-- PMM

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

[John Levon]

On Tue, Nov 25, 2025 at 05:13:14PM +0000, Peter Maydell wrote:

> static int vfio_user_recv_one(VFIOUserProxy *proxy, Error **errp)
> {
>     /* common prep, header read, etc code here */
> 
>     switch (hdr.flags & VFIO_USER_TYPE) {
>     case VFIO_USER_REQUEST:
>         return vfio_user_handle_request(...stuff...);
>         break;
>     case VFIO_USER_REPLY:
>         return vfio_user_handle_request(...stuff...);
>         break;
>     default:
>         error_setg(errp, "unknown message type");
>         goto fatal;
>     }
> }

I did look at this variant, but it really wasn't that much nicer, and still had
a non-trivial amount of duplication.

If you feel super strongly I can present that version.

regards
john