Re: [Qemu-devel] [PATCH V3 01/11] qemu-img: remove unused parameter in collect_image_info()

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 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. :)
-- 
Best Regards

Wenchao Xia