* [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open()
2019-11-29 7:25 [PATCH V3 1/2] block/nbd: extract the common cleanup code pannengyuan
@ 2019-11-29 7:25 ` pannengyuan
2019-12-03 17:52 ` for 4.2 ??? " Vladimir Sementsov-Ogievskiy
2019-12-03 17:38 ` [PATCH V3 1/2] block/nbd: extract the common cleanup code Vladimir Sementsov-Ogievskiy
2019-12-03 19:00 ` Eric Blake
2 siblings, 1 reply; 12+ messages in thread
From: pannengyuan @ 2019-11-29 7:25 UTC (permalink / raw)
To: eblake, kwolf, mreitz, sgarzare
Cc: liyiting, zhang.zhanghailiang, qemu-block, PanNengyuan,
qemu-devel, kuhn.chenqun
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* for 4.2 ??? Re: [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open()
2019-11-29 7:25 ` [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open() pannengyuan
@ 2019-12-03 17:52 ` Vladimir Sementsov-Ogievskiy
2019-12-03 18:54 ` Eric Blake
0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-12-03 17:52 UTC (permalink / raw)
To: pannengyuan, eblake, kwolf, mreitz, sgarzare
Cc: liyiting, kuhn.chenqun, zhang.zhanghailiang, qemu-block, qemu-devel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: for 4.2 ??? Re: [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open()
2019-12-03 17:52 ` for 4.2 ??? " Vladimir Sementsov-Ogievskiy
@ 2019-12-03 18:54 ` Eric Blake
2019-12-03 21:59 ` Eric Blake
2019-12-04 3:30 ` pannengyuan
0 siblings, 2 replies; 12+ messages in thread
From: Eric Blake @ 2019-12-03 18:54 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, pannengyuan, kwolf, mreitz, sgarzare
Cc: liyiting, zhang.zhanghailiang, qemu-block, qemu-stable,
qemu-devel, kuhn.chenqun
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: for 4.2 ??? Re: [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open()
2019-12-03 18:54 ` Eric Blake
@ 2019-12-03 21:59 ` Eric Blake
2019-12-04 3:30 ` pannengyuan
1 sibling, 0 replies; 12+ messages in thread
From: Eric Blake @ 2019-12-03 21:59 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, pannengyuan, kwolf, mreitz, sgarzare
Cc: liyiting, zhang.zhanghailiang, qemu-block, qemu-devel,
qemu-stable, kuhn.chenqun
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: for 4.2 ??? Re: [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open()
2019-12-03 18:54 ` Eric Blake
2019-12-03 21:59 ` Eric Blake
@ 2019-12-04 3:30 ` pannengyuan
1 sibling, 0 replies; 12+ messages in thread
From: pannengyuan @ 2019-12-04 3:30 UTC (permalink / raw)
To: Eric Blake, Vladimir Sementsov-Ogievskiy, kwolf, mreitz, sgarzare
Cc: liyiting, zhang.zhanghailiang, qemu-block, qemu-stable,
qemu-devel, kuhn.chenqun
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code
2019-11-29 7:25 [PATCH V3 1/2] block/nbd: extract the common cleanup code pannengyuan
2019-11-29 7:25 ` [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open() pannengyuan
@ 2019-12-03 17:38 ` Vladimir Sementsov-Ogievskiy
2019-12-04 3:12 ` pannengyuan
2019-12-03 19:00 ` Eric Blake
2 siblings, 1 reply; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-12-03 17:38 UTC (permalink / raw)
To: pannengyuan, eblake, kwolf, mreitz, sgarzare
Cc: liyiting, kuhn.chenqun, zhang.zhanghailiang, qemu-block, qemu-devel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code
2019-12-03 17:38 ` [PATCH V3 1/2] block/nbd: extract the common cleanup code Vladimir Sementsov-Ogievskiy
@ 2019-12-04 3:12 ` pannengyuan
2019-12-04 7:19 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 12+ messages in thread
From: pannengyuan @ 2019-12-04 3:12 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, eblake, kwolf, mreitz, sgarzare
Cc: liyiting, kuhn.chenqun, zhang.zhanghailiang, qemu-block, qemu-devel
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)
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code
2019-12-04 3:12 ` pannengyuan
@ 2019-12-04 7:19 ` Vladimir Sementsov-Ogievskiy
2019-12-04 7:24 ` Pan Nengyuan
0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-12-04 7:19 UTC (permalink / raw)
To: pannengyuan, eblake, kwolf, mreitz, sgarzare
Cc: liyiting, kuhn.chenqun, zhang.zhanghailiang, qemu-block, qemu-devel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code
2019-12-04 7:19 ` Vladimir Sementsov-Ogievskiy
@ 2019-12-04 7:24 ` Pan Nengyuan
0 siblings, 0 replies; 12+ messages in thread
From: Pan Nengyuan @ 2019-12-04 7:24 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, eblake, kwolf, mreitz, sgarzare
Cc: liyiting, kuhn.chenqun, zhang.zhanghailiang, qemu-block, qemu-devel
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)
>>>>
>>>
>>>
>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code
2019-11-29 7:25 [PATCH V3 1/2] block/nbd: extract the common cleanup code pannengyuan
2019-11-29 7:25 ` [PATCH V3 2/2] block/nbd: fix memory leak in nbd_open() pannengyuan
2019-12-03 17:38 ` [PATCH V3 1/2] block/nbd: extract the common cleanup code Vladimir Sementsov-Ogievskiy
@ 2019-12-03 19:00 ` Eric Blake
2019-12-04 3:20 ` pannengyuan
2 siblings, 1 reply; 12+ messages in thread
From: Eric Blake @ 2019-12-03 19:00 UTC (permalink / raw)
To: pannengyuan, kwolf, mreitz, sgarzare
Cc: liyiting, kuhn.chenqun, qemu-devel, qemu-block, zhang.zhanghailiang
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH V3 1/2] block/nbd: extract the common cleanup code
2019-12-03 19:00 ` Eric Blake
@ 2019-12-04 3:20 ` pannengyuan
0 siblings, 0 replies; 12+ messages in thread
From: pannengyuan @ 2019-12-04 3:20 UTC (permalink / raw)
To: Eric Blake, kwolf, mreitz, sgarzare
Cc: liyiting, kuhn.chenqun, qemu-devel, qemu-block, zhang.zhanghailiang
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)
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread