Re: [PATCH 1/6] vhost: fix vduse device creation error handling

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

On 10/23/24 18:06, David Marchand wrote:
> On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch fixes missing reconnection log unmapping
>> miss in the error path at VDUSE creation time.
>>
>> Coverity issue: 445525
>> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> VDUSE* in the title, for consistency please.

Fixed in v2.

> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> One comment below.
> 
>> ---
>>   lib/vhost/vduse.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
>> index f9ac317438..89bc29a33f 100644
>> --- a/lib/vhost/vduse.c
>> +++ b/lib/vhost/vduse.c
>> @@ -488,7 +488,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>>          struct vduse_dev_config *dev_config = NULL;
> 
> dev_config could be allocated on the stack, rather than the heap.
> 
> But at least, move dev_config declaration in the block where it is
> used, this will limit the chance a future change introduces a leak.
> 

I moved dev_config declaration in the block, having it declared in the
heap makes it less readable IMHO.

> 
>>          const char *name = path + strlen("/dev/vduse/");
>>          char reconnect_file[PATH_MAX];
>> -       struct vhost_reconnect_data *reconnect_log = NULL;
>> +       struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
>>          bool reconnect = false;
>>
>>          if (vduse.fdset == NULL) {
> 
> 

Thanks,
Maxime