[PATCH V3 1/2] block/nbd: extract the common cleanup code

[PATCH V3 1/2] block/nbd: extract the common cleanup code

[pannengyuan]

From: PanNengyuan <pannengyuan@huawei.com>

The BDRVNBDState cleanup code is common in two places, add
nbd_free_bdrvstate_prop() function to do these cleanups (suggested by
Stefano Garzarella).

Signed-off-by: PanNengyuan <pannengyuan@huawei.com>
---
 block/nbd.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 1239761..5805979 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -94,6 +94,8 @@ typedef struct BDRVNBDState {
 
 static int nbd_client_connect(BlockDriverState *bs, Error **errp);
 
+static void nbd_free_bdrvstate_prop(BDRVNBDState *s);
+
 static void nbd_channel_error(BDRVNBDState *s, int ret)
 {
     if (ret == -EIO) {
@@ -1486,6 +1488,15 @@ static int nbd_client_connect(BlockDriverState *bs, Error **errp)
     }
 }
 
+static void nbd_free_bdrvstate_prop(BDRVNBDState *s)
+{
+    object_unref(OBJECT(s->tlscreds));
+    qapi_free_SocketAddress(s->saddr);
+    g_free(s->export);
+    g_free(s->tlscredsid);
+    g_free(s->x_dirty_bitmap);
+}
+
 /*
  * Parse nbd_open options
  */
@@ -1855,10 +1866,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
 
  error:
     if (ret < 0) {
-        object_unref(OBJECT(s->tlscreds));
-        qapi_free_SocketAddress(s->saddr);
-        g_free(s->export);
-        g_free(s->tlscredsid);
+        nbd_free_bdrvstate_prop(s);
     }
     qemu_opts_del(opts);
     return ret;
@@ -1937,12 +1945,7 @@ static void nbd_close(BlockDriverState *bs)
     BDRVNBDState *s = bs->opaque;
 
     nbd_client_close(bs);
-
-    object_unref(OBJECT(s->tlscreds));
-    qapi_free_SocketAddress(s->saddr);
-    g_free(s->export);
-    g_free(s->tlscredsid);
-    g_free(s->x_dirty_bitmap);
+    nbd_free_bdrvstate_prop(s);
 }
 
 static int64_t nbd_getlength(BlockDriverState *bs)
-- 
2.7.2.windows.1

[PATCH V3 2/2] block/nbd: fix memory leak in nbd_open()

[pannengyuan]

From: PanNengyuan <pannengyuan@huawei.com>

In currently implementation there will be a memory leak when
nbd_client_connect() returns error status. Here is an easy way to
reproduce:

1. run qemu-iotests as follow and check the result with asan:
    ./check -raw 143

Following is the asan output backtrack:
Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0x7f629688a560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
    #1 0x7f6295e7e015 in g_malloc0  (/usr/lib64/libglib-2.0.so.0+0x50015)
    #2 0x56281dab4642 in qobject_input_start_struct  /mnt/sdb/qemu-4.2.0-rc0/qapi/qobject-input-visitor.c:295
    #3 0x56281dab1a04 in visit_start_struct  /mnt/sdb/qemu-4.2.0-rc0/qapi/qapi-visit-core.c:49
    #4 0x56281dad1827 in visit_type_SocketAddress  qapi/qapi-visit-sockets.c:386
    #5 0x56281da8062f in nbd_config   /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1716
    #6 0x56281da8062f in nbd_process_options /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1829
    #7 0x56281da8062f in nbd_open /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1873

Direct leak of 15 byte(s) in 1 object(s) allocated from:
    #0 0x7f629688a3a0 in malloc (/usr/lib64/libasan.so.3+0xc73a0)
    #1 0x7f6295e7dfbd in g_malloc (/usr/lib64/libglib-2.0.so.0+0x4ffbd)
    #2 0x7f6295e96ace in g_strdup (/usr/lib64/libglib-2.0.so.0+0x68ace)
    #3 0x56281da804ac in nbd_process_options /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1834
    #4 0x56281da804ac in nbd_open /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1873

Indirect leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x7f629688a3a0 in malloc (/usr/lib64/libasan.so.3+0xc73a0)
    #1 0x7f6295e7dfbd in g_malloc (/usr/lib64/libglib-2.0.so.0+0x4ffbd)
    #2 0x7f6295e96ace in g_strdup (/usr/lib64/libglib-2.0.so.0+0x68ace)
    #3 0x56281dab41a3 in qobject_input_type_str_keyval /mnt/sdb/qemu-4.2.0-rc0/qapi/qobject-input-visitor.c:536
    #4 0x56281dab2ee9 in visit_type_str /mnt/sdb/qemu-4.2.0-rc0/qapi/qapi-visit-core.c:297
    #5 0x56281dad0fa1 in visit_type_UnixSocketAddress_members qapi/qapi-visit-sockets.c:141
    #6 0x56281dad17b6 in visit_type_SocketAddress_members qapi/qapi-visit-sockets.c:366
    #7 0x56281dad186a in visit_type_SocketAddress qapi/qapi-visit-sockets.c:393
    #8 0x56281da8062f in nbd_config /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1716
    #9 0x56281da8062f in nbd_process_options /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1829
    #10 0x56281da8062f in nbd_open /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1873

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: PanNengyuan <pannengyuan@huawei.com>
---
Changes v2 to v1:
- add a new function to do the common cleanups (suggested by Stefano
  Garzarella).
---
Changes v3 to v2:
- split in two patches(suggested by Stefano Garzarella)
---
 block/nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd.c b/block/nbd.c
index 5805979..09d6925 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1889,6 +1889,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
 
     ret = nbd_client_connect(bs, errp);
     if (ret < 0) {
+        nbd_free_bdrvstate_prop(s);
         return ret;
     }
     /* successfully connected */
-- 
2.7.2.windows.1

Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code

[Vladimir Sementsov-Ogievskiy]

Hi!

First, please, when sending more than one patch, create a cover-letter. Also,
summarize (in cover letter) what was changed since previous version.

29.11.2019 10:25, pannengyuan@huawei.com wrote:
> From: PanNengyuan <pannengyuan@huawei.com>

Strange line. Check you git preferences. Such line appears (and make sense)
when you are sending patches authored by someone else.. But here is your name,
the same as in email's From:.

> 
> The BDRVNBDState cleanup code is common in two places, add
> nbd_free_bdrvstate_prop() function to do these cleanups (suggested by
> Stefano Garzarella).
> 
> Signed-off-by: PanNengyuan <pannengyuan@huawei.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   block/nbd.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 1239761..5805979 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -94,6 +94,8 @@ typedef struct BDRVNBDState {
>   
>   static int nbd_client_connect(BlockDriverState *bs, Error **errp);
>   
> +static void nbd_free_bdrvstate_prop(BDRVNBDState *s);
> +
>   static void nbd_channel_error(BDRVNBDState *s, int ret)
>   {
>       if (ret == -EIO) {
> @@ -1486,6 +1488,15 @@ static int nbd_client_connect(BlockDriverState *bs, Error **errp)
>       }
>   }
>   
> +static void nbd_free_bdrvstate_prop(BDRVNBDState *s)
> +{
> +    object_unref(OBJECT(s->tlscreds));
> +    qapi_free_SocketAddress(s->saddr);
> +    g_free(s->export);
> +    g_free(s->tlscredsid);
> +    g_free(s->x_dirty_bitmap);
> +}
> +
>   /*
>    * Parse nbd_open options
>    */
> @@ -1855,10 +1866,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>   
>    error:
>       if (ret < 0) {
> -        object_unref(OBJECT(s->tlscreds));
> -        qapi_free_SocketAddress(s->saddr);
> -        g_free(s->export);
> -        g_free(s->tlscredsid);
> +        nbd_free_bdrvstate_prop(s);
>       }
>       qemu_opts_del(opts);
>       return ret;
> @@ -1937,12 +1945,7 @@ static void nbd_close(BlockDriverState *bs)
>       BDRVNBDState *s = bs->opaque;
>   
>       nbd_client_close(bs);
> -
> -    object_unref(OBJECT(s->tlscreds));
> -    qapi_free_SocketAddress(s->saddr);
> -    g_free(s->export);
> -    g_free(s->tlscredsid);
> -    g_free(s->x_dirty_bitmap);
> +    nbd_free_bdrvstate_prop(s);
>   }
>   
>   static int64_t nbd_getlength(BlockDriverState *bs)
> 


-- 
Best regards,
Vladimir

for 4.2 ??? Re: [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open()

[Vladimir Sementsov-Ogievskiy]

It's just a memory leak, but it's a regression in 4.2.

Should we take it into 4.2?


29.11.2019 10:25, pannengyuan@huawei.com wrote:
> From: PanNengyuan <pannengyuan@huawei.com>
> 
> In currently implementation there will be a memory leak when
> nbd_client_connect() returns error status. Here is an easy way to
> reproduce:
> 
> 1. run qemu-iotests as follow and check the result with asan:
>      ./check -raw 143
> 
> Following is the asan output backtrack:
> Direct leak of 40 byte(s) in 1 object(s) allocated from:
>      #0 0x7f629688a560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
>      #1 0x7f6295e7e015 in g_malloc0  (/usr/lib64/libglib-2.0.so.0+0x50015)
>      #2 0x56281dab4642 in qobject_input_start_struct  /mnt/sdb/qemu-4.2.0-rc0/qapi/qobject-input-visitor.c:295
>      #3 0x56281dab1a04 in visit_start_struct  /mnt/sdb/qemu-4.2.0-rc0/qapi/qapi-visit-core.c:49
>      #4 0x56281dad1827 in visit_type_SocketAddress  qapi/qapi-visit-sockets.c:386
>      #5 0x56281da8062f in nbd_config   /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1716
>      #6 0x56281da8062f in nbd_process_options /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1829
>      #7 0x56281da8062f in nbd_open /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1873
> 
> Direct leak of 15 byte(s) in 1 object(s) allocated from:
>      #0 0x7f629688a3a0 in malloc (/usr/lib64/libasan.so.3+0xc73a0)
>      #1 0x7f6295e7dfbd in g_malloc (/usr/lib64/libglib-2.0.so.0+0x4ffbd)
>      #2 0x7f6295e96ace in g_strdup (/usr/lib64/libglib-2.0.so.0+0x68ace)
>      #3 0x56281da804ac in nbd_process_options /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1834
>      #4 0x56281da804ac in nbd_open /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1873
> 
> Indirect leak of 24 byte(s) in 1 object(s) allocated from:
>      #0 0x7f629688a3a0 in malloc (/usr/lib64/libasan.so.3+0xc73a0)
>      #1 0x7f6295e7dfbd in g_malloc (/usr/lib64/libglib-2.0.so.0+0x4ffbd)
>      #2 0x7f6295e96ace in g_strdup (/usr/lib64/libglib-2.0.so.0+0x68ace)
>      #3 0x56281dab41a3 in qobject_input_type_str_keyval /mnt/sdb/qemu-4.2.0-rc0/qapi/qobject-input-visitor.c:536
>      #4 0x56281dab2ee9 in visit_type_str /mnt/sdb/qemu-4.2.0-rc0/qapi/qapi-visit-core.c:297
>      #5 0x56281dad0fa1 in visit_type_UnixSocketAddress_members qapi/qapi-visit-sockets.c:141
>      #6 0x56281dad17b6 in visit_type_SocketAddress_members qapi/qapi-visit-sockets.c:366
>      #7 0x56281dad186a in visit_type_SocketAddress qapi/qapi-visit-sockets.c:393
>      #8 0x56281da8062f in nbd_config /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1716
>      #9 0x56281da8062f in nbd_process_options /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1829
>      #10 0x56281da8062f in nbd_open /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1873
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: PanNengyuan <pannengyuan@huawei.com>

May add:

Fixes: 8f071c9db506e03ab

(I found it just by git log -p, could you please check it by your reproduce?)

Ohh, that's my commit, I'm sorry. If you discovered this commit by yourself,
and added Fixes: tag and myself to CC, I'd have reviewed it a lot earlier..

> ---
> Changes v2 to v1:
> - add a new function to do the common cleanups (suggested by Stefano
>    Garzarella).
> ---
> Changes v3 to v2:
> - split in two patches(suggested by Stefano Garzarella)
> ---
>   block/nbd.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 5805979..09d6925 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1889,6 +1889,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
>   
>       ret = nbd_client_connect(bs, errp);
>       if (ret < 0) {
> +        nbd_free_bdrvstate_prop(s);
>           return ret;
>       }
>       /* successfully connected */
> 

Thanks for fixing!

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: for 4.2 ??? Re: [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open()

[Eric Blake]

On 12/3/19 11:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> It's just a memory leak, but it's a regression in 4.2.
> 
> Should we take it into 4.2?

Sorry, I was on holiday and then jury service, so I missed any chance at 
getting this into -rc3.  The memory leak only happens on failure, and 
you'd have to be pretty desperate to purposefully attempt to open a lot 
of NBD devices where you know you'll get a failure just to trigger 
enough of a leak to cause the OOM-killer to target qemu.  So I'm fine if 
this is deferred to 5.0, and just cc's qemu-stable (now done).

I'll queue this through my NBD tree for 5.0.

> 
> 
> 29.11.2019 10:25, pannengyuan@huawei.com wrote:
>> From: PanNengyuan <pannengyuan@huawei.com>

>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: PanNengyuan <pannengyuan@huawei.com>

I'm not one to tell you that your name is written incorrectly, but it 
does look odd to have a single word rather than a space between two 
capitalized portions.  If that's really how you want your S-o-b and 
authorship to appear, I'm happy to preserve it; but you may want to 
consider updating your git settings, and posting a v4 with an updated 
spelling if you would prefer something different.  (It is also 
acceptable to use UTF-8 characters; some people like listing an S-o-b in 
both native characters and a Westernized variant).

> 
> May add:
> 
> Fixes: 8f071c9db506e03ab

Yes, information like that helps in deciding how long the problem has 
been present (in this case, it is indeed a regression added in 4.2, even 
if minor in nature).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code

[Eric Blake]

On 11/29/19 1:25 AM, pannengyuan@huawei.com wrote:
> From: PanNengyuan <pannengyuan@huawei.com>
> 
> The BDRVNBDState cleanup code is common in two places, add
> nbd_free_bdrvstate_prop() function to do these cleanups (suggested by
> Stefano Garzarella).
> 
> Signed-off-by: PanNengyuan <pannengyuan@huawei.com>
> ---
>   block/nbd.c | 23 +++++++++++++----------
>   1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 1239761..5805979 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -94,6 +94,8 @@ typedef struct BDRVNBDState {
>   
>   static int nbd_client_connect(BlockDriverState *bs, Error **errp);
>   
> +static void nbd_free_bdrvstate_prop(BDRVNBDState *s);
> +

Why do you need a static function prototype?  Just implement the 
function prior to its first use, then you won't need a forward declaration.

>   static void nbd_channel_error(BDRVNBDState *s, int ret)
>   {
>       if (ret == -EIO) {
> @@ -1486,6 +1488,15 @@ static int nbd_client_connect(BlockDriverState *bs, Error **errp)
>       }
>   }
>   
> +static void nbd_free_bdrvstate_prop(BDRVNBDState *s)
> +{
> +    object_unref(OBJECT(s->tlscreds));
> +    qapi_free_SocketAddress(s->saddr);
> +    g_free(s->export);
> +    g_free(s->tlscredsid);
> +    g_free(s->x_dirty_bitmap);
> +}

In fact, it appears that you did just that, as the first use...

Bike-shedding: the name 'nbd_free_bdrvstate_prop' doesn't seem right to 
me - when I see a function with 'free' in the name taking a single 
pointer, I assume that the given pointer (here, BDRVNBDState *s) is 
freed - but your function does NOT free then incoming pointer.  Rather, 
you are clearing out the contents within a pre-allocated object which 
remains allocated.  What's more, since the object remains allocated, I'm 
surprised that you are not setting fields to NULL to prevent 
use-after-free bugs.

Either this function should also free s (in which case naming it merely 
'nbd_free_bdrvstate' might be better), or you should consider naming it 
'nbd_clear_bdrvstate' and assigning cleared fields to NULL.

> +
>   /*
>    * Parse nbd_open options
>    */
> @@ -1855,10 +1866,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>   
>    error:
>       if (ret < 0) {
> -        object_unref(OBJECT(s->tlscreds));
> -        qapi_free_SocketAddress(s->saddr);
> -        g_free(s->export);
> -        g_free(s->tlscredsid);
> +        nbd_free_bdrvstate_prop(s);

...is here.

>       }
>       qemu_opts_del(opts);
>       return ret;
> @@ -1937,12 +1945,7 @@ static void nbd_close(BlockDriverState *bs)
>       BDRVNBDState *s = bs->opaque;
>   
>       nbd_client_close(bs);
> -
> -    object_unref(OBJECT(s->tlscreds));
> -    qapi_free_SocketAddress(s->saddr);
> -    g_free(s->export);
> -    g_free(s->tlscredsid);
> -    g_free(s->x_dirty_bitmap);
> +    nbd_free_bdrvstate_prop(s);
>   }
>   
>   static int64_t nbd_getlength(BlockDriverState *bs)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: for 4.2 ??? Re: [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open()

[Eric Blake]

On 12/3/19 12:54 PM, Eric Blake wrote:
> On 12/3/19 11:52 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It's just a memory leak, but it's a regression in 4.2.
>>
>> Should we take it into 4.2?
> 
> Sorry, I was on holiday and then jury service, so I missed any chance at 
> getting this into -rc3.  The memory leak only happens on failure, and 
> you'd have to be pretty desperate to purposefully attempt to open a lot 
> of NBD devices where you know you'll get a failure just to trigger 
> enough of a leak to cause the OOM-killer to target qemu.  So I'm fine if 
> this is deferred to 5.0, and just cc's qemu-stable (now done).
> 
> I'll queue this through my NBD tree for 5.0.

Actually, given the review comments on 1/2, we'll probably be better off 
with a v4 for the series.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code

[pannengyuan]

On 2019/12/4 1:38, Vladimir Sementsov-Ogievskiy wrote:
> Hi!
> 
> First, please, when sending more than one patch, create a cover-letter. Also,
> summarize (in cover letter) what was changed since previous version.
In previous version, I only send one patch(2/2 in this version), so I
only add a change summarize in 2/2 patch in this version. should I add a
summarize in 1/2 patch too if 1/2 patch is a new one?

> 
> 29.11.2019 10:25, pannengyuan@huawei.com wrote:
>> From: PanNengyuan <pannengyuan@huawei.com>
> 
> Strange line. Check you git preferences. Such line appears (and make sense)
> when you are sending patches authored by someone else.. But here is your name,
> the same as in email's From:.

Thanks for your reminding. I will correct it in next version.

> 
>>
>> The BDRVNBDState cleanup code is common in two places, add
>> nbd_free_bdrvstate_prop() function to do these cleanups (suggested by
>> Stefano Garzarella).
>>
>> Signed-off-by: PanNengyuan <pannengyuan@huawei.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
>> ---
>>   block/nbd.c | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 1239761..5805979 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -94,6 +94,8 @@ typedef struct BDRVNBDState {
>>   
>>   static int nbd_client_connect(BlockDriverState *bs, Error **errp);
>>   
>> +static void nbd_free_bdrvstate_prop(BDRVNBDState *s);
>> +
>>   static void nbd_channel_error(BDRVNBDState *s, int ret)
>>   {
>>       if (ret == -EIO) {
>> @@ -1486,6 +1488,15 @@ static int nbd_client_connect(BlockDriverState *bs, Error **errp)
>>       }
>>   }
>>   
>> +static void nbd_free_bdrvstate_prop(BDRVNBDState *s)
>> +{
>> +    object_unref(OBJECT(s->tlscreds));
>> +    qapi_free_SocketAddress(s->saddr);
>> +    g_free(s->export);
>> +    g_free(s->tlscredsid);
>> +    g_free(s->x_dirty_bitmap);
>> +}
>> +
>>   /*
>>    * Parse nbd_open options
>>    */
>> @@ -1855,10 +1866,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>>   
>>    error:
>>       if (ret < 0) {
>> -        object_unref(OBJECT(s->tlscreds));
>> -        qapi_free_SocketAddress(s->saddr);
>> -        g_free(s->export);
>> -        g_free(s->tlscredsid);
>> +        nbd_free_bdrvstate_prop(s);
>>       }
>>       qemu_opts_del(opts);
>>       return ret;
>> @@ -1937,12 +1945,7 @@ static void nbd_close(BlockDriverState *bs)
>>       BDRVNBDState *s = bs->opaque;
>>   
>>       nbd_client_close(bs);
>> -
>> -    object_unref(OBJECT(s->tlscreds));
>> -    qapi_free_SocketAddress(s->saddr);
>> -    g_free(s->export);
>> -    g_free(s->tlscredsid);
>> -    g_free(s->x_dirty_bitmap);
>> +    nbd_free_bdrvstate_prop(s);
>>   }
>>   
>>   static int64_t nbd_getlength(BlockDriverState *bs)
>>
> 
> 

Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code

[pannengyuan]

On 2019/12/4 3:00, Eric Blake wrote:
> On 11/29/19 1:25 AM, pannengyuan@huawei.com wrote:
>> From: PanNengyuan <pannengyuan@huawei.com>
>>
>> The BDRVNBDState cleanup code is common in two places, add
>> nbd_free_bdrvstate_prop() function to do these cleanups (suggested by
>> Stefano Garzarella).
>>
>> Signed-off-by: PanNengyuan <pannengyuan@huawei.com>
>> ---
>>   block/nbd.c | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 1239761..5805979 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -94,6 +94,8 @@ typedef struct BDRVNBDState {
>>     static int nbd_client_connect(BlockDriverState *bs, Error **errp);
>>   +static void nbd_free_bdrvstate_prop(BDRVNBDState *s);
>> +
> 
> Why do you need a static function prototype?  Just implement the
> function prior to its first use, then you won't need a forward declaration.

Yes, It's not necessary. I will change it.

> 
>>   static void nbd_channel_error(BDRVNBDState *s, int ret)
>>   {
>>       if (ret == -EIO) {
>> @@ -1486,6 +1488,15 @@ static int nbd_client_connect(BlockDriverState
>> *bs, Error **errp)
>>       }
>>   }
>>   +static void nbd_free_bdrvstate_prop(BDRVNBDState *s)
>> +{
>> +    object_unref(OBJECT(s->tlscreds));
>> +    qapi_free_SocketAddress(s->saddr);
>> +    g_free(s->export);
>> +    g_free(s->tlscredsid);
>> +    g_free(s->x_dirty_bitmap);
>> +}
> 
> In fact, it appears that you did just that, as the first use...
> 
> Bike-shedding: the name 'nbd_free_bdrvstate_prop' doesn't seem right to
> me - when I see a function with 'free' in the name taking a single
> pointer, I assume that the given pointer (here, BDRVNBDState *s) is
> freed - but your function does NOT free then incoming pointer.  Rather,
> you are clearing out the contents within a pre-allocated object which
> remains allocated.  What's more, since the object remains allocated, I'm
> surprised that you are not setting fields to NULL to prevent
> use-after-free bugs.
> 
> Either this function should also free s (in which case naming it merely
> 'nbd_free_bdrvstate' might be better), or you should consider naming it
> 'nbd_clear_bdrvstate' and assigning cleared fields to NULL.
> 

thanks, 'nbd_clear_bdrvstate' seems nice. I will replace the name and
set fields to NULL in next version.

>> +
>>   /*
>>    * Parse nbd_open options
>>    */
>> @@ -1855,10 +1866,7 @@ static int nbd_process_options(BlockDriverState
>> *bs, QDict *options,
>>      error:
>>       if (ret < 0) {
>> -        object_unref(OBJECT(s->tlscreds));
>> -        qapi_free_SocketAddress(s->saddr);
>> -        g_free(s->export);
>> -        g_free(s->tlscredsid);
>> +        nbd_free_bdrvstate_prop(s);
> 
> ...is here.
> 
>>       }
>>       qemu_opts_del(opts);
>>       return ret;
>> @@ -1937,12 +1945,7 @@ static void nbd_close(BlockDriverState *bs)
>>       BDRVNBDState *s = bs->opaque;
>>         nbd_client_close(bs);
>> -
>> -    object_unref(OBJECT(s->tlscreds));
>> -    qapi_free_SocketAddress(s->saddr);
>> -    g_free(s->export);
>> -    g_free(s->tlscredsid);
>> -    g_free(s->x_dirty_bitmap);
>> +    nbd_free_bdrvstate_prop(s);
>>   }
>>     static int64_t nbd_getlength(BlockDriverState *bs)
>>
> 

Re: for 4.2 ??? Re: [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open()

[pannengyuan]

On 2019/12/4 2:54, Eric Blake wrote:
> On 12/3/19 11:52 AM, Vladimir Sementsov-Ogievskiy wrote:
>> It's just a memory leak, but it's a regression in 4.2.
>>
>> Should we take it into 4.2?
> 
> Sorry, I was on holiday and then jury service, so I missed any chance at
> getting this into -rc3.  The memory leak only happens on failure, and
> you'd have to be pretty desperate to purposefully attempt to open a lot
> of NBD devices where you know you'll get a failure just to trigger
> enough of a leak to cause the OOM-killer to target qemu.  So I'm fine if
> this is deferred to 5.0, and just cc's qemu-stable (now done).
> 
> I'll queue this through my NBD tree for 5.0.
> 
>>
>>
>> 29.11.2019 10:25, pannengyuan@huawei.com wrote:
>>> From: PanNengyuan <pannengyuan@huawei.com>
> 
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: PanNengyuan <pannengyuan@huawei.com>
> 
> I'm not one to tell you that your name is written incorrectly, but it
> does look odd to have a single word rather than a space between two
> capitalized portions.  If that's really how you want your S-o-b and
> authorship to appear, I'm happy to preserve it; but you may want to
> consider updating your git settings, and posting a v4 with an updated
> spelling if you would prefer something different.  (It is also
> acceptable to use UTF-8 characters; some people like listing an S-o-b in
> both native characters and a Westernized variant).
> 

Thanks for your advice, I will update my git settings.

>>
>> May add:
>>
>> Fixes: 8f071c9db506e03ab
> 
> Yes, information like that helps in deciding how long the problem has
> been present (in this case, it is indeed a regression added in 4.2, even
> if minor in nature).
> 

ok, I will add it next time.

Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code

[Vladimir Sementsov-Ogievskiy]

04.12.2019 6:12, pannengyuan wrote:
> 
> 
> On 2019/12/4 1:38, Vladimir Sementsov-Ogievskiy wrote:
>> Hi!
>>
>> First, please, when sending more than one patch, create a cover-letter. Also,
>> summarize (in cover letter) what was changed since previous version.
> In previous version, I only send one patch(2/2 in this version), so I
> only add a change summarize in 2/2 patch in this version. should I add a
> summarize in 1/2 patch too if 1/2 patch is a new one?

Yes, something simple like:

01: new patch
02: rebased on 01

(also, If you didn't read https://wiki.qemu.org/Contribute/SubmitAPatch, do it)

> 
>>
>> 29.11.2019 10:25, pannengyuan@huawei.com wrote:
>>> From: PanNengyuan <pannengyuan@huawei.com>
>>
>> Strange line. Check you git preferences. Such line appears (and make sense)
>> when you are sending patches authored by someone else.. But here is your name,
>> the same as in email's From:.
> 
> Thanks for your reminding. I will correct it in next version.
> 
>>
>>>
>>> The BDRVNBDState cleanup code is common in two places, add
>>> nbd_free_bdrvstate_prop() function to do these cleanups (suggested by
>>> Stefano Garzarella).
>>>
>>> Signed-off-by: PanNengyuan <pannengyuan@huawei.com>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>>> ---
>>>    block/nbd.c | 23 +++++++++++++----------
>>>    1 file changed, 13 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/block/nbd.c b/block/nbd.c
>>> index 1239761..5805979 100644
>>> --- a/block/nbd.c
>>> +++ b/block/nbd.c
>>> @@ -94,6 +94,8 @@ typedef struct BDRVNBDState {
>>>    
>>>    static int nbd_client_connect(BlockDriverState *bs, Error **errp);
>>>    
>>> +static void nbd_free_bdrvstate_prop(BDRVNBDState *s);
>>> +
>>>    static void nbd_channel_error(BDRVNBDState *s, int ret)
>>>    {
>>>        if (ret == -EIO) {
>>> @@ -1486,6 +1488,15 @@ static int nbd_client_connect(BlockDriverState *bs, Error **errp)
>>>        }
>>>    }
>>>    
>>> +static void nbd_free_bdrvstate_prop(BDRVNBDState *s)
>>> +{
>>> +    object_unref(OBJECT(s->tlscreds));
>>> +    qapi_free_SocketAddress(s->saddr);
>>> +    g_free(s->export);
>>> +    g_free(s->tlscredsid);
>>> +    g_free(s->x_dirty_bitmap);
>>> +}
>>> +
>>>    /*
>>>     * Parse nbd_open options
>>>     */
>>> @@ -1855,10 +1866,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>>>    
>>>     error:
>>>        if (ret < 0) {
>>> -        object_unref(OBJECT(s->tlscreds));
>>> -        qapi_free_SocketAddress(s->saddr);
>>> -        g_free(s->export);
>>> -        g_free(s->tlscredsid);
>>> +        nbd_free_bdrvstate_prop(s);
>>>        }
>>>        qemu_opts_del(opts);
>>>        return ret;
>>> @@ -1937,12 +1945,7 @@ static void nbd_close(BlockDriverState *bs)
>>>        BDRVNBDState *s = bs->opaque;
>>>    
>>>        nbd_client_close(bs);
>>> -
>>> -    object_unref(OBJECT(s->tlscreds));
>>> -    qapi_free_SocketAddress(s->saddr);
>>> -    g_free(s->export);
>>> -    g_free(s->tlscredsid);
>>> -    g_free(s->x_dirty_bitmap);
>>> +    nbd_free_bdrvstate_prop(s);
>>>    }
>>>    
>>>    static int64_t nbd_getlength(BlockDriverState *bs)
>>>
>>
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code

[Pan Nengyuan]

On 2019/12/4 15:19, Vladimir Sementsov-Ogievskiy wrote:
> 04.12.2019 6:12, pannengyuan wrote:
>>
>>
>> On 2019/12/4 1:38, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi!
>>>
>>> First, please, when sending more than one patch, create a cover-letter. Also,
>>> summarize (in cover letter) what was changed since previous version.
>> In previous version, I only send one patch(2/2 in this version), so I
>> only add a change summarize in 2/2 patch in this version. should I add a
>> summarize in 1/2 patch too if 1/2 patch is a new one?
> 
> Yes, something simple like:
> 
> 01: new patch
> 02: rebased on 01
> 
> (also, If you didn't read https://wiki.qemu.org/Contribute/SubmitAPatch, do it)

Ok, thanks.

> 
>>
>>>
>>> 29.11.2019 10:25, pannengyuan@huawei.com wrote:
>>>> From: PanNengyuan <pannengyuan@huawei.com>
>>>
>>> Strange line. Check you git preferences. Such line appears (and make sense)
>>> when you are sending patches authored by someone else.. But here is your name,
>>> the same as in email's From:.
>>
>> Thanks for your reminding. I will correct it in next version.
>>
>>>
>>>>
>>>> The BDRVNBDState cleanup code is common in two places, add
>>>> nbd_free_bdrvstate_prop() function to do these cleanups (suggested by
>>>> Stefano Garzarella).
>>>>
>>>> Signed-off-by: PanNengyuan <pannengyuan@huawei.com>
>>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>>> ---
>>>>    block/nbd.c | 23 +++++++++++++----------
>>>>    1 file changed, 13 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/block/nbd.c b/block/nbd.c
>>>> index 1239761..5805979 100644
>>>> --- a/block/nbd.c
>>>> +++ b/block/nbd.c
>>>> @@ -94,6 +94,8 @@ typedef struct BDRVNBDState {
>>>>    
>>>>    static int nbd_client_connect(BlockDriverState *bs, Error **errp);
>>>>    
>>>> +static void nbd_free_bdrvstate_prop(BDRVNBDState *s);
>>>> +
>>>>    static void nbd_channel_error(BDRVNBDState *s, int ret)
>>>>    {
>>>>        if (ret == -EIO) {
>>>> @@ -1486,6 +1488,15 @@ static int nbd_client_connect(BlockDriverState *bs, Error **errp)
>>>>        }
>>>>    }
>>>>    
>>>> +static void nbd_free_bdrvstate_prop(BDRVNBDState *s)
>>>> +{
>>>> +    object_unref(OBJECT(s->tlscreds));
>>>> +    qapi_free_SocketAddress(s->saddr);
>>>> +    g_free(s->export);
>>>> +    g_free(s->tlscredsid);
>>>> +    g_free(s->x_dirty_bitmap);
>>>> +}
>>>> +
>>>>    /*
>>>>     * Parse nbd_open options
>>>>     */
>>>> @@ -1855,10 +1866,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options,
>>>>    
>>>>     error:
>>>>        if (ret < 0) {
>>>> -        object_unref(OBJECT(s->tlscreds));
>>>> -        qapi_free_SocketAddress(s->saddr);
>>>> -        g_free(s->export);
>>>> -        g_free(s->tlscredsid);
>>>> +        nbd_free_bdrvstate_prop(s);
>>>>        }
>>>>        qemu_opts_del(opts);
>>>>        return ret;
>>>> @@ -1937,12 +1945,7 @@ static void nbd_close(BlockDriverState *bs)
>>>>        BDRVNBDState *s = bs->opaque;
>>>>    
>>>>        nbd_client_close(bs);
>>>> -
>>>> -    object_unref(OBJECT(s->tlscreds));
>>>> -    qapi_free_SocketAddress(s->saddr);
>>>> -    g_free(s->export);
>>>> -    g_free(s->tlscredsid);
>>>> -    g_free(s->x_dirty_bitmap);
>>>> +    nbd_free_bdrvstate_prop(s);
>>>>    }
>>>>    
>>>>    static int64_t nbd_getlength(BlockDriverState *bs)
>>>>
>>>
>>>
>>
> 
>