[Qemu-devel] [PATCH v2] block/nbd: Fix the regression to free leaked visitor

[Qemu-devel] [PATCH v2] block/nbd: Fix the regression to free leaked visitor

[Ashijeet Acharya]

This patch frees the leaked visitor in nbd_refresh_filename() and uses
visit_free() to fix it. The leak was introduced by the commit 491d6c7.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
Changes in v2:
- Include the regression commit id in the commit message
---
 block/nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd.c b/block/nbd.c
index 8ef1438..ff9d01a 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -545,6 +545,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
         qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid));
     }
 
+    visit_free(ov);
     qdict_flatten(opts);
     bs->full_open_options = opts;
 }
-- 
2.6.2

Re: [Qemu-devel] [PATCH v2] block/nbd: Fix the regression to free leaked visitor

[Kevin Wolf]

Am 02.11.2016 um 10:48 hat Ashijeet Acharya geschrieben:
> This patch frees the leaked visitor in nbd_refresh_filename() and uses
> visit_free() to fix it. The leak was introduced by the commit 491d6c7.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

I don't think this would generally be called a regression, so I'd change
the subject to just "nbd: Fix leaker visitor".

> Changes in v2:
> - Include the regression commit id in the commit message
> ---
>  block/nbd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 8ef1438..ff9d01a 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -545,6 +545,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
>          qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid));
>      }
>  
> +    visit_free(ov);
>      qdict_flatten(opts);
>      bs->full_open_options = opts;
>  }

I would prefer freeing the visitor immediately after visit_complete() so
that everything visitor related is in a single place.

Both of these points don't make your patch wrong, of course, but would
you mind changing them?

Kevin

Re: [Qemu-devel] [PATCH v2] block/nbd: Fix the regression to free leaked visitor

[Ashijeet Acharya]

On Wed, Nov 2, 2016 at 4:00 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 02.11.2016 um 10:48 hat Ashijeet Acharya geschrieben:
>> This patch frees the leaked visitor in nbd_refresh_filename() and uses
>> visit_free() to fix it. The leak was introduced by the commit 491d6c7.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> I don't think this would generally be called a regression, so I'd change
> the subject to just "nbd: Fix leaker visitor".
>
>> Changes in v2:
>> - Include the regression commit id in the commit message
>> ---
>>  block/nbd.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 8ef1438..ff9d01a 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -545,6 +545,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
>>          qdict_put(opts, "tls-creds", qstring_from_str(s->tlscredsid));
>>      }
>>
>> +    visit_free(ov);
>>      qdict_flatten(opts);
>>      bs->full_open_options = opts;
>>  }
>
> I would prefer freeing the visitor immediately after visit_complete() so
> that everything visitor related is in a single place.
>
> Both of these points don't make your patch wrong, of course, but would
> you mind changing them?

Sure, I will send v3 straight away.

Ashijeet

> Kevin