Re: [PATCH] vhost: fix VDUSE devices registration

Re: [PATCH] vhost: fix VDUSE devices registration

[Ariel Otilibili-Anieli]

Hello Maxime,

On Friday, January 31, 2025 14:09 CET, Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> This patch fixes a regression in vhost_driver_register()
> causing VDUSE devices registration to fail systematically
> because the return value was initialized to -1 and not
> changed later on for this type of devices.
> 
> Fixes: 4d2aa150769b ("vhost: remove check around mutex init")

Thanks for the heads up. I indeed committed 4d2aa150769b ("vhost: remove check around mutex init"); and it contained a hunk for vhost_driver_register().

I applied this patch against the tip of the main; from what I saw, there is no overlap with 4d2aa150769b ("vhost: remove check around mutex init").

It looks 4d2aa150769b ("vhost: remove check around mutex init") came up first, because I was the last person who edited the file.

The tags should be rather these ones:

Fixes: 0adb8eccc6a6 ("vhost: add VDUSE device creation and destruction")
Fixes: 78b2e3bae1af ("vhost: fix initialization")
Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
Fixes: 8f972312b8f4 ("vhost: support vhost-user")

Does my reasoning make sense? Let me know.

:) For my understanding; now that 4d2aa150769b ("vhost: remove check around mutex init") needs a fix, is there a way by which I could have detect the regression?

Your help will be much appreciated,
Ariel

Re: [PATCH] vhost: fix VDUSE devices registration

[Maxime Coquelin]

Hi Ariel,

On 1/31/25 6:34 PM, Ariel Otilibili-Anieli wrote:
> Hello Maxime,
> 
> On Friday, January 31, 2025 14:09 CET, Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> 
>> This patch fixes a regression in vhost_driver_register()
>> causing VDUSE devices registration to fail systematically
>> because the return value was initialized to -1 and not
>> changed later on for this type of devices.
>>
>> Fixes: 4d2aa150769b ("vhost: remove check around mutex init")
> 
> Thanks for the heads up. I indeed committed 4d2aa150769b ("vhost: remove check around mutex init"); and it contained a hunk for vhost_driver_register().
> 
> I applied this patch against the tip of the main; from what I saw, there is no overlap with 4d2aa150769b ("vhost: remove check around mutex init").
> 
> It looks 4d2aa150769b ("vhost: remove check around mutex init") came up first, because I was the last person who edited the file.
> 
> The tags should be rather these ones:
> 
> Fixes: 0adb8eccc6a6 ("vhost: add VDUSE device creation and destruction")
> Fixes: 78b2e3bae1af ("vhost: fix initialization")
> Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode")
> Fixes: 8f972312b8f4 ("vhost: support vhost-user")
> 
> Does my reasoning make sense? Let me know.

Not really :)
Without your patch, ret was assigned by the pthread_mutex_init() return,
so always 0. With your patch, this assignment is removed so ret will
always be -1 for VDUSE devices.

So before your patch, VDUSE devices registration was functional, with
your patch it breaks systematically.

We don't want to backport my patch to LTS that aren't imapcted, so
tagging your patch as the one introducing the regression is the right
thing to do.

> :) For my understanding; now that 4d2aa150769b ("vhost: remove check around mutex init") needs a fix, is there a way by which I could have detect the regression?

It could have been detected by testing VDUSE, that's how I noticed it.
But VDUSE is still fairly recent, and it is not yet tested by the CI.

Now that it is supported in at least Fedora without any kernel change,
we should work on adding CI testing for it.

> Your help will be much appreciated,
> Ariel
> 

Thanks for your contribution,
Maxime

[PATCH] vhost: fix VDUSE devices registration

[Maxime Coquelin]

This patch fixes a regression in vhost_driver_register()
causing VDUSE devices registration to fail systematically
because the return value was initialized to -1 and not
changed later on for this type of devices.

Fixes: 4d2aa150769b ("vhost: remove check around mutex init")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/socket.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
index 433a42bf80..894a0f0dcb 100644
--- a/lib/vhost/socket.c
+++ b/lib/vhost/socket.c
@@ -893,7 +893,6 @@ vhost_user_socket_mem_free(struct vhost_user_socket *vsocket)
 int
 rte_vhost_driver_register(const char *path, uint64_t flags)
 {
-	int ret = -1;
 	struct vhost_user_socket *vsocket;
 
 	if (!path)
@@ -997,7 +996,6 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
 	} else {
 #ifndef RTE_LIBRTE_VHOST_POSTCOPY
 		VHOST_CONFIG_LOG(path, ERR, "Postcopy requested but not compiled");
-		ret = -1;
 		goto out_mutex;
 #endif
 	}
@@ -1012,15 +1010,14 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
 		} else {
 			vsocket->is_server = true;
 		}
-		ret = create_unix_socket(vsocket);
-		if (ret < 0)
+		if (create_unix_socket(vsocket) < 0)
 			goto out_mutex;
 	}
 
 	vhost_user.vsockets[vhost_user.vsocket_cnt++] = vsocket;
 
 	pthread_mutex_unlock(&vhost_user.mutex);
-	return ret;
+	return 0;
 
 out_mutex:
 	if (pthread_mutex_destroy(&vsocket->conn_mutex)) {
@@ -1028,8 +1025,7 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
 	}
 out:
 	pthread_mutex_unlock(&vhost_user.mutex);
-
-	return ret;
+	return -1;
 }
 
 static bool
-- 
2.48.1