Re: [PATCH] vhost: fix VDUSE devices registration

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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