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

[Luiz Capitulino]

On Wed, 16 Jan 2013 11:10:14 +0800
Wenchao Xia <address@hidden> wrote:

> 于 2013-1-15 19:11, Luiz Capitulino 写道:
> > On Tue, 15 Jan 2013 15:58:34 +0800
> > Wenchao Xia <address@hidden> 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 <address@hidden> wrote:
> >>>>
> >>>>>     Parameter *fmt was not used, so remove it.
> >>>>>
> >>>>> Reviewed-by: Eric Blake <address@hidden>
> >>>>> Signed-off-by: Wenchao Xia <address@hidden>
> >>>>> ---
> >>>>>    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.