Patchwork [V3,01/11] qemu-img: remove unused parameter in collect_image_info()

login
register
mail settings
Submitter Wayne Xia
Date Jan. 14, 2013, 7:09 a.m.
Message ID <1358147387-8221-2-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/211715/
State New
Headers show

Comments

Wayne Xia - Jan. 14, 2013, 7:09 a.m.
Parameter *fmt was not used, so remove it.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qemu-img.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
Luiz Capitulino - Jan. 14, 2013, 5:08 p.m.
On Mon, 14 Jan 2013 15:09:37 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>   Parameter *fmt was not used, so remove it.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  qemu-img.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 85d3740..9dab48f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
>  
>  static void collect_image_info(BlockDriverState *bs,
>                     ImageInfo *info,
> -                   const char *filename,
> -                   const char *fmt)
> +                   const char *filename)

collect_image_info_list() doc reads:

 @fmt: topmost image format (may be NULL to autodetect)

However, right now only fmt=NULL is supported, as collect_image_info()
ignores fmt altogether.

So, if this patch is correct we better update the comment. Otherwise,
we should improve collect_image_info() to actually obey fmt != NULL.

>  {
>      uint64_t total_sectors;
>      char backing_filename[1024];
> @@ -1361,7 +1360,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>          }
>  
>          info = g_new0(ImageInfo, 1);
> -        collect_image_info(bs, info, filename, fmt);
> +        collect_image_info(bs, info, filename);
>          collect_snapshots(bs, info);
>  
>          elem = g_new0(ImageInfoList, 1);
Wayne Xia - Jan. 15, 2013, 7:27 a.m.
于 2013-1-15 1:08, Luiz Capitulino 写道:
> On Mon, 14 Jan 2013 15:09:37 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>>    Parameter *fmt was not used, so remove it.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   qemu-img.c |    5 ++---
>>   1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 85d3740..9dab48f 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
>>
>>   static void collect_image_info(BlockDriverState *bs,
>>                      ImageInfo *info,
>> -                   const char *filename,
>> -                   const char *fmt)
>> +                   const char *filename)
>
> collect_image_info_list() doc reads:
>
>   @fmt: topmost image format (may be NULL to autodetect)
>
> However, right now only fmt=NULL is supported, as collect_image_info()
> ignores fmt altogether.
>
> So, if this patch is correct we better update the comment. Otherwise,
> we should improve collect_image_info() to actually obey fmt != NULL.
>
   @fmt was ignored in the function and I can't see a reason to have
it while *bs contains the info, will change the comments.

>>   {
>>       uint64_t total_sectors;
>>       char backing_filename[1024];
>> @@ -1361,7 +1360,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>>           }
>>
>>           info = g_new0(ImageInfo, 1);
>> -        collect_image_info(bs, info, filename, fmt);
>> +        collect_image_info(bs, info, filename);
>>           collect_snapshots(bs, info);
>>
>>           elem = g_new0(ImageInfoList, 1);
>
Wayne Xia - Jan. 15, 2013, 7:58 a.m.
于 2013-1-15 15:27, Wenchao Xia 写道:
> 于 2013-1-15 1:08, Luiz Capitulino 写道:
>> On Mon, 14 Jan 2013 15:09:37 +0800
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>
>>>    Parameter *fmt was not used, so remove it.
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   qemu-img.c |    5 ++---
>>>   1 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index 85d3740..9dab48f 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
>>>
>>>   static void collect_image_info(BlockDriverState *bs,
>>>                      ImageInfo *info,
>>> -                   const char *filename,
>>> -                   const char *fmt)
>>> +                   const char *filename)
>>
>> collect_image_info_list() doc reads:
>>
>>   @fmt: topmost image format (may be NULL to autodetect)
>>
>> However, right now only fmt=NULL is supported, as collect_image_info()
>> ignores fmt altogether.
>>
>> So, if this patch is correct we better update the comment. Otherwise,
>> we should improve collect_image_info() to actually obey fmt != NULL.
>>
>    @fmt was ignored in the function and I can't see a reason to have
> it while *bs contains the info, will change the comments.
>
   Hi, *fmt was used only in collect_image_info_list() when it tries to
open the image, and it is not useful any more in collect_image_info,
so nothing need change in comments.

>>>   {
>>>       uint64_t total_sectors;
>>>       char backing_filename[1024];
>>> @@ -1361,7 +1360,7 @@ static ImageInfoList
>>> *collect_image_info_list(const char *filename,
>>>           }
>>>
>>>           info = g_new0(ImageInfo, 1);
>>> -        collect_image_info(bs, info, filename, fmt);
>>> +        collect_image_info(bs, info, filename);
>>>           collect_snapshots(bs, info);
>>>
>>>           elem = g_new0(ImageInfoList, 1);
>>
>
>
Luiz Capitulino - Jan. 15, 2013, 11:11 a.m.
On Tue, 15 Jan 2013 15:58:34 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013-1-15 15:27, Wenchao Xia 写道:
> > 于 2013-1-15 1:08, Luiz Capitulino 写道:
> >> On Mon, 14 Jan 2013 15:09:37 +0800
> >> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>
> >>>    Parameter *fmt was not used, so remove it.
> >>>
> >>> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>> ---
> >>>   qemu-img.c |    5 ++---
> >>>   1 files changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/qemu-img.c b/qemu-img.c
> >>> index 85d3740..9dab48f 100644
> >>> --- a/qemu-img.c
> >>> +++ b/qemu-img.c
> >>> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
> >>>
> >>>   static void collect_image_info(BlockDriverState *bs,
> >>>                      ImageInfo *info,
> >>> -                   const char *filename,
> >>> -                   const char *fmt)
> >>> +                   const char *filename)
> >>
> >> collect_image_info_list() doc reads:
> >>
> >>   @fmt: topmost image format (may be NULL to autodetect)
> >>
> >> However, right now only fmt=NULL is supported, as collect_image_info()
> >> ignores fmt altogether.
> >>
> >> So, if this patch is correct we better update the comment. Otherwise,
> >> we should improve collect_image_info() to actually obey fmt != NULL.
> >>
> >    @fmt was ignored in the function and I can't see a reason to have
> > it while *bs contains the info, will change the comments.
> >
>    Hi, *fmt was used only in collect_image_info_list() when it tries to
> open the image, and it is not useful any more in collect_image_info,
> so nothing need change in comments.

This really doesn't answer my comment above. The code comment implies that
fmt may be NULL or non-NULL and they have different behavior.

If you choose to touch fmt (as this patch does) then please, do the
right thing. Otherwise it's better to let it untouched.
Wayne Xia - Jan. 16, 2013, 3:10 a.m.
于 2013-1-15 19:11, Luiz Capitulino 写道:
> On Tue, 15 Jan 2013 15:58:34 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-1-15 15:27, Wenchao Xia 写道:
>>> 于 2013-1-15 1:08, Luiz Capitulino 写道:
>>>> On Mon, 14 Jan 2013 15:09:37 +0800
>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>
>>>>>     Parameter *fmt was not used, so remove it.
>>>>>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>> ---
>>>>>    qemu-img.c |    5 ++---
>>>>>    1 files changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/qemu-img.c b/qemu-img.c
>>>>> index 85d3740..9dab48f 100644
>>>>> --- a/qemu-img.c
>>>>> +++ b/qemu-img.c
>>>>> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
>>>>>
>>>>>    static void collect_image_info(BlockDriverState *bs,
>>>>>                       ImageInfo *info,
>>>>> -                   const char *filename,
>>>>> -                   const char *fmt)
>>>>> +                   const char *filename)
>>>>
>>>> collect_image_info_list() doc reads:
>>>>
>>>>    @fmt: topmost image format (may be NULL to autodetect)
>>>>
>>>> However, right now only fmt=NULL is supported, as collect_image_info()
>>>> ignores fmt altogether.
>>>>
>>>> So, if this patch is correct we better update the comment. Otherwise,
>>>> we should improve collect_image_info() to actually obey fmt != NULL.
>>>>
>>>     @fmt was ignored in the function and I can't see a reason to have
>>> it while *bs contains the info, will change the comments.
>>>
>>     Hi, *fmt was used only in collect_image_info_list() when it tries to
>> open the image, and it is not useful any more in collect_image_info,
>> so nothing need change in comments.
>
> This really doesn't answer my comment above. The code comment implies that
> fmt may be NULL or non-NULL and they have different behavior.
>
> If you choose to touch fmt (as this patch does) then please, do the
> right thing. Otherwise it's better to let it untouched.
>
   I think the "fmt may be NULL or non-NULL" indeed have different
behavior for that later following is called:
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
                            false);
but it is not related to collect_image_info(), it is more like a
slip in coding having add *fmt in above funtion. :)
Luiz Capitulino - Jan. 16, 2013, 5:56 p.m.
On Wed, 16 Jan 2013 11:10:14 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013-1-15 19:11, Luiz Capitulino 写道:
> > On Tue, 15 Jan 2013 15:58:34 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >> 于 2013-1-15 15:27, Wenchao Xia 写道:
> >>> 于 2013-1-15 1:08, Luiz Capitulino 写道:
> >>>> On Mon, 14 Jan 2013 15:09:37 +0800
> >>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>
> >>>>>     Parameter *fmt was not used, so remove it.
> >>>>>
> >>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>>> ---
> >>>>>    qemu-img.c |    5 ++---
> >>>>>    1 files changed, 2 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/qemu-img.c b/qemu-img.c
> >>>>> index 85d3740..9dab48f 100644
> >>>>> --- a/qemu-img.c
> >>>>> +++ b/qemu-img.c
> >>>>> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
> >>>>>
> >>>>>    static void collect_image_info(BlockDriverState *bs,
> >>>>>                       ImageInfo *info,
> >>>>> -                   const char *filename,
> >>>>> -                   const char *fmt)
> >>>>> +                   const char *filename)
> >>>>
> >>>> collect_image_info_list() doc reads:
> >>>>
> >>>>    @fmt: topmost image format (may be NULL to autodetect)
> >>>>
> >>>> However, right now only fmt=NULL is supported, as collect_image_info()
> >>>> ignores fmt altogether.
> >>>>
> >>>> So, if this patch is correct we better update the comment. Otherwise,
> >>>> we should improve collect_image_info() to actually obey fmt != NULL.
> >>>>
> >>>     @fmt was ignored in the function and I can't see a reason to have
> >>> it while *bs contains the info, will change the comments.
> >>>
> >>     Hi, *fmt was used only in collect_image_info_list() when it tries to
> >> open the image, and it is not useful any more in collect_image_info,
> >> so nothing need change in comments.
> >
> > This really doesn't answer my comment above. The code comment implies that
> > fmt may be NULL or non-NULL and they have different behavior.
> >
> > If you choose to touch fmt (as this patch does) then please, do the
> > right thing. Otherwise it's better to let it untouched.
> >
>    I think the "fmt may be NULL or non-NULL" indeed have different
> behavior for that later following is called:
> bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
>                             false);
> but it is not related to collect_image_info(), it is more like a
> slip in coding having add *fmt in above funtion. :)

Oh, you seem to be right. Sorry for the noise.

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 85d3740..9dab48f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1186,8 +1186,7 @@  static void dump_json_image_info(ImageInfo *info)
 
 static void collect_image_info(BlockDriverState *bs,
                    ImageInfo *info,
-                   const char *filename,
-                   const char *fmt)
+                   const char *filename)
 {
     uint64_t total_sectors;
     char backing_filename[1024];
@@ -1361,7 +1360,7 @@  static ImageInfoList *collect_image_info_list(const char *filename,
         }
 
         info = g_new0(ImageInfo, 1);
-        collect_image_info(bs, info, filename, fmt);
+        collect_image_info(bs, info, filename);
         collect_snapshots(bs, info);
 
         elem = g_new0(ImageInfoList, 1);