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

2019-12-03 Thread 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, pannengy...@huawei.com wrote:
 From: PanNengyuan 
>>>
>>> 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 
>>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>>>
 ---
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

2019-12-03 Thread 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, pannengy...@huawei.com wrote:
>>> From: PanNengyuan 
>>
>> 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 
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>>
>>> ---
>>>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

2019-12-03 Thread pannengyuan 



On 2019/12/4 3:00, Eric Blake wrote:
> On 11/29/19 1:25 AM, pannengy...@huawei.com wrote:
>> From: PanNengyuan 
>>
>> 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 
>> ---
>>   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: [PATCH V3 1/2] block/nbd: extract the common cleanup code

2019-12-03 Thread 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, pannengy...@huawei.com wrote:
>> From: PanNengyuan 
> 
> 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 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> 
>> ---
>>   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

2019-12-03 Thread Eric Blake 

On 11/29/19 1:25 AM, pannengy...@huawei.com wrote:

From: PanNengyuan 

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 
---
  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: [PATCH V3 1/2] block/nbd: extract the common cleanup code

2019-12-03 Thread 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, pannengy...@huawei.com wrote:
> From: PanNengyuan 

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 

Reviewed-by: Vladimir Sementsov-Ogievskiy 

> ---
>   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

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

2019-11-28 Thread pannengyuan 
From: PanNengyuan 

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 
---
 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