Message ID | 1358147387-8221-2-git-send-email-xiawenc@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
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);
于 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); >
于 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); >> > >
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.
于 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. :)
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 --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);