* [PATCH v3 1/5] vfio-user: simplify vfio_user_process()
2025-12-01 9:56 [PATCH v3 0/5] vfio-user coverity fixes John Levon
@ 2025-12-01 9:56 ` John Levon
2025-12-02 10:42 ` Mark Cave-Ayland
2025-12-01 9:56 ` [PATCH v3 2/5] vfio-user: clarify partial message handling John Levon
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: John Levon @ 2025-12-01 9:56 UTC (permalink / raw)
To: qemu-devel
Cc: John Levon, Thanos Makatos, Peter Maydell, Cédric Le Goater
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 1/5] vfio-user: simplify vfio_user_process()
2025-12-01 9:56 ` [PATCH v3 1/5] vfio-user: simplify vfio_user_process() John Levon
@ 2025-12-02 10:42 ` Mark Cave-Ayland
0 siblings, 0 replies; 12+ messages in thread
From: Mark Cave-Ayland @ 2025-12-02 10:42 UTC (permalink / raw)
To: John Levon, qemu-devel
Cc: Thanos Makatos, Peter Maydell, Cédric Le Goater
On 01/12/2025 09: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: Mark Cave-Ayland <mark.caveayland@nutanix.com>
ATB,
Mark.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 2/5] vfio-user: clarify partial message handling
2025-12-01 9:56 [PATCH v3 0/5] vfio-user coverity fixes John Levon
2025-12-01 9:56 ` [PATCH v3 1/5] vfio-user: simplify vfio_user_process() John Levon
@ 2025-12-01 9:56 ` John Levon
2025-12-02 10:43 ` Mark Cave-Ayland
2025-12-01 9:56 ` [PATCH v3 3/5] vfio-user: refactor out header handling John Levon
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: John Levon @ 2025-12-01 9:56 UTC (permalink / raw)
To: qemu-devel
Cc: John Levon, Thanos Makatos, Peter Maydell, Cédric Le Goater
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 2/5] vfio-user: clarify partial message handling
2025-12-01 9:56 ` [PATCH v3 2/5] vfio-user: clarify partial message handling John Levon
@ 2025-12-02 10:43 ` Mark Cave-Ayland
0 siblings, 0 replies; 12+ messages in thread
From: Mark Cave-Ayland @ 2025-12-02 10:43 UTC (permalink / raw)
To: John Levon, qemu-devel
Cc: Thanos Makatos, Peter Maydell, Cédric Le Goater
On 01/12/2025 09: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: Mark Cave-Ayland <mark.caveayland@nutanix.com>
ATB,
Mark.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 3/5] vfio-user: refactor out header handling
2025-12-01 9:56 [PATCH v3 0/5] vfio-user coverity fixes John Levon
2025-12-01 9:56 ` [PATCH v3 1/5] vfio-user: simplify vfio_user_process() John Levon
2025-12-01 9:56 ` [PATCH v3 2/5] vfio-user: clarify partial message handling John Levon
@ 2025-12-01 9:56 ` John Levon
2025-12-02 10:53 ` Mark Cave-Ayland
2025-12-01 9:56 ` [PATCH v3 4/5] vfio-user: simplify vfio_user_recv_one() John Levon
2025-12-01 9:56 ` [PATCH v3 5/5] vfio-user: recycle msg on failure John Levon
4 siblings, 1 reply; 12+ messages in thread
From: John Levon @ 2025-12-01 9:56 UTC (permalink / raw)
To: qemu-devel
Cc: John Levon, Thanos Makatos, Peter Maydell, Cédric Le Goater
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 3/5] vfio-user: refactor out header handling
2025-12-01 9:56 ` [PATCH v3 3/5] vfio-user: refactor out header handling John Levon
@ 2025-12-02 10:53 ` Mark Cave-Ayland
2025-12-03 7:40 ` Cédric Le Goater
0 siblings, 1 reply; 12+ messages in thread
From: Mark Cave-Ayland @ 2025-12-02 10:53 UTC (permalink / raw)
To: John Levon, qemu-devel
Cc: Thanos Makatos, Peter Maydell, Cédric Le Goater
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.
> + 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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3 3/5] vfio-user: refactor out header handling
2025-12-02 10:53 ` Mark Cave-Ayland
@ 2025-12-03 7:40 ` Cédric Le Goater
0 siblings, 0 replies; 12+ messages in thread
From: Cédric Le Goater @ 2025-12-03 7:40 UTC (permalink / raw)
To: Mark Cave-Ayland, John Levon, qemu-devel; +Cc: Thanos Makatos, Peter Maydell
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.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 4/5] vfio-user: simplify vfio_user_recv_one()
2025-12-01 9:56 [PATCH v3 0/5] vfio-user coverity fixes John Levon
` (2 preceding siblings ...)
2025-12-01 9:56 ` [PATCH v3 3/5] vfio-user: refactor out header handling John Levon
@ 2025-12-01 9:56 ` John Levon
2025-12-02 11:11 ` Mark Cave-Ayland
2025-12-01 9:56 ` [PATCH v3 5/5] vfio-user: recycle msg on failure John Levon
4 siblings, 1 reply; 12+ messages in thread
From: John Levon @ 2025-12-01 9:56 UTC (permalink / raw)
To: qemu-devel
Cc: John Levon, Thanos Makatos, Peter Maydell, Cédric Le Goater
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);
--
2.43.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 4/5] vfio-user: simplify vfio_user_recv_one()
2025-12-01 9:56 ` [PATCH v3 4/5] vfio-user: simplify vfio_user_recv_one() John Levon
@ 2025-12-02 11:11 ` Mark Cave-Ayland
0 siblings, 0 replies; 12+ messages in thread
From: Mark Cave-Ayland @ 2025-12-02 11:11 UTC (permalink / raw)
To: John Levon, qemu-devel
Cc: Thanos Makatos, Peter Maydell, Cédric Le Goater
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 5/5] vfio-user: recycle msg on failure
2025-12-01 9:56 [PATCH v3 0/5] vfio-user coverity fixes John Levon
` (3 preceding siblings ...)
2025-12-01 9:56 ` [PATCH v3 4/5] vfio-user: simplify vfio_user_recv_one() John Levon
@ 2025-12-01 9:56 ` John Levon
2025-12-02 11:14 ` Mark Cave-Ayland
4 siblings, 1 reply; 12+ messages in thread
From: John Levon @ 2025-12-01 9:56 UTC (permalink / raw)
To: qemu-devel
Cc: John Levon, Thanos Makatos, Peter Maydell, Cédric Le Goater
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 d1d63816b3..d06978a74f 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 5/5] vfio-user: recycle msg on failure
2025-12-01 9:56 ` [PATCH v3 5/5] vfio-user: recycle msg on failure John Levon
@ 2025-12-02 11:14 ` Mark Cave-Ayland
0 siblings, 0 replies; 12+ messages in thread
From: Mark Cave-Ayland @ 2025-12-02 11:14 UTC (permalink / raw)
To: John Levon, qemu-devel
Cc: Thanos Makatos, Peter Maydell, Cédric Le Goater
On 01/12/2025 09:56, 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>
> ---
> 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 d1d63816b3..d06978a74f 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;
> }
Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
ATB,
Mark.
^ permalink raw reply [flat|nested] 12+ messages in thread