diff mbox

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

Message ID 1358147387-8221-2-git-send-email-xiawenc@linux.vnet.ibm.com
State New
Headers show

Commit Message

Wayne Xia Jan. 14, 2013, 7:09 a.m. UTC 
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(-)

Comments

Luiz Capitulino Jan. 14, 2013, 5:08 p.m. UTC | #1 
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. UTC | #2 
于 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. UTC | #3 
于 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. UTC | #4 
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. UTC | #5 
于 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. UTC | #6 
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.
diff mbox

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