Re: [Qemu-devel] [PATCH] qemu-img: check bs_n when use old style option

[858585 jemmy]

On Thu, Apr 20, 2017 at 4:11 PM, Fam Zheng  wrote:
> On Thu, 04/20 15:59, 858585 jemmy wrote:
>> On Thu, Apr 20, 2017 at 3:51 PM, Fam Zheng  wrote:
>> > On Thu, 04/20 12:04, jemmy858...@gmail.com wrote:
>> >> From: Lidong Chen 
>> >>
>> >> When use old style option like -o backing_file, img_convert
>> >> continue run when bs_n > 1, this patch fix this bug.
>> >>
>> >> Signed-off-by: Lidong Chen 
>> >> ---
>> >>  qemu-img.c | 7 +++
>> >>  1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/qemu-img.c b/qemu-img.c
>> >> index b220cf7..c673aef 100644
>> >> --- a/qemu-img.c
>> >> +++ b/qemu-img.c
>> >> @@ -2225,6 +2225,13 @@ static int img_convert(int argc, char **argv)
>> >>  out_baseimg = out_baseimg_param;
>> >>  }
>> >>
>> >> +if (bs_n > 1 && out_baseimg) {
>> >> +error_report("-B makes no sense when concatenating multiple 
>> >> input "
>> >> + "images");
>> >> +ret = -1;
>> >> +goto out;
>> >> +}
>> >> +
>> >>  /* Check if compression is supported */
>> >>  if (compress) {
>> >>  bool encryption =
>> >> --
>> >> 1.8.3.1
>> >>
>> >>
>> >
>> > Is this essentially the same as the check a few lines above:
>> >
>> > ...
>> > if (bs_n < 1) {
>> > error_exit("Must specify image file name");
>> > }
>> >
>> >
>> > if (bs_n > 1 && out_baseimg) {
>> > error_report("-B makes no sense when concatenating multiple input "
>> >  "images");
>> > ret = -1;
>> > goto out;
>> > }
>> >
>> > src_flags = 0;
>> > ret = bdrv_parse_cache_mode(src_cache, _flags, _writethrough);
>> > if (ret < 0) {
>> > error_report("Invalid source cache option: %s", src_cache);
>> > goto out;
>> > }
>> > ...
>> >
>> > How about moving that down?
>> moving that down is ok.
>> but will exit later if use -B option.
>> which way do you think better?
>
> Exiting later is not a problem, I assume? And it's better to avoid duplicating
> code if possible.
>
> BTW if you do that way, it's better to "s/-B/Specifying backing image/" in the
> error message (to be compatible with -o backing_file syntax).
Thanks. i will submit this patch again.


>
> Fam

Re: [Qemu-devel] [PATCH] qemu-img: check bs_n when use old style option

[Fam Zheng]

On Thu, 04/20 15:59, 858585 jemmy wrote:
> On Thu, Apr 20, 2017 at 3:51 PM, Fam Zheng  wrote:
> > On Thu, 04/20 12:04, jemmy858...@gmail.com wrote:
> >> From: Lidong Chen 
> >>
> >> When use old style option like -o backing_file, img_convert
> >> continue run when bs_n > 1, this patch fix this bug.
> >>
> >> Signed-off-by: Lidong Chen 
> >> ---
> >>  qemu-img.c | 7 +++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/qemu-img.c b/qemu-img.c
> >> index b220cf7..c673aef 100644
> >> --- a/qemu-img.c
> >> +++ b/qemu-img.c
> >> @@ -2225,6 +2225,13 @@ static int img_convert(int argc, char **argv)
> >>  out_baseimg = out_baseimg_param;
> >>  }
> >>
> >> +if (bs_n > 1 && out_baseimg) {
> >> +error_report("-B makes no sense when concatenating multiple input 
> >> "
> >> + "images");
> >> +ret = -1;
> >> +goto out;
> >> +}
> >> +
> >>  /* Check if compression is supported */
> >>  if (compress) {
> >>  bool encryption =
> >> --
> >> 1.8.3.1
> >>
> >>
> >
> > Is this essentially the same as the check a few lines above:
> >
> > ...
> > if (bs_n < 1) {
> > error_exit("Must specify image file name");
> > }
> >
> >
> > if (bs_n > 1 && out_baseimg) {
> > error_report("-B makes no sense when concatenating multiple input "
> >  "images");
> > ret = -1;
> > goto out;
> > }
> >
> > src_flags = 0;
> > ret = bdrv_parse_cache_mode(src_cache, _flags, _writethrough);
> > if (ret < 0) {
> > error_report("Invalid source cache option: %s", src_cache);
> > goto out;
> > }
> > ...
> >
> > How about moving that down?
> moving that down is ok.
> but will exit later if use -B option.
> which way do you think better?

Exiting later is not a problem, I assume? And it's better to avoid duplicating
code if possible.

BTW if you do that way, it's better to "s/-B/Specifying backing image/" in the
error message (to be compatible with -o backing_file syntax).

Fam

Re: [Qemu-devel] [PATCH] qemu-img: check bs_n when use old style option

[858585 jemmy]

On Thu, Apr 20, 2017 at 3:51 PM, Fam Zheng  wrote:
> On Thu, 04/20 12:04, jemmy858...@gmail.com wrote:
>> From: Lidong Chen 
>>
>> When use old style option like -o backing_file, img_convert
>> continue run when bs_n > 1, this patch fix this bug.
>>
>> Signed-off-by: Lidong Chen 
>> ---
>>  qemu-img.c | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index b220cf7..c673aef 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -2225,6 +2225,13 @@ static int img_convert(int argc, char **argv)
>>  out_baseimg = out_baseimg_param;
>>  }
>>
>> +if (bs_n > 1 && out_baseimg) {
>> +error_report("-B makes no sense when concatenating multiple input "
>> + "images");
>> +ret = -1;
>> +goto out;
>> +}
>> +
>>  /* Check if compression is supported */
>>  if (compress) {
>>  bool encryption =
>> --
>> 1.8.3.1
>>
>>
>
> Is this essentially the same as the check a few lines above:
>
> ...
> if (bs_n < 1) {
> error_exit("Must specify image file name");
> }
>
>
> if (bs_n > 1 && out_baseimg) {
> error_report("-B makes no sense when concatenating multiple input "
>  "images");
> ret = -1;
> goto out;
> }
>
> src_flags = 0;
> ret = bdrv_parse_cache_mode(src_cache, _flags, _writethrough);
> if (ret < 0) {
> error_report("Invalid source cache option: %s", src_cache);
> goto out;
> }
> ...
>
> How about moving that down?
moving that down is ok.
but will exit later if use -B option.
which way do you think better?


>
> Fam

Re: [Qemu-devel] [PATCH] qemu-img: check bs_n when use old style option

[Fam Zheng]

On Thu, 04/20 12:04, jemmy858...@gmail.com wrote:
> From: Lidong Chen 
> 
> When use old style option like -o backing_file, img_convert
> continue run when bs_n > 1, this patch fix this bug.
> 
> Signed-off-by: Lidong Chen 
> ---
>  qemu-img.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index b220cf7..c673aef 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2225,6 +2225,13 @@ static int img_convert(int argc, char **argv)
>  out_baseimg = out_baseimg_param;
>  }
>  
> +if (bs_n > 1 && out_baseimg) {
> +error_report("-B makes no sense when concatenating multiple input "
> + "images");
> +ret = -1;
> +goto out;
> +}
> +
>  /* Check if compression is supported */
>  if (compress) {
>  bool encryption =
> -- 
> 1.8.3.1
> 
> 

Is this essentially the same as the check a few lines above:

...
if (bs_n < 1) {
error_exit("Must specify image file name");
}


if (bs_n > 1 && out_baseimg) {
error_report("-B makes no sense when concatenating multiple input "
 "images");
ret = -1;
goto out;
}

src_flags = 0;
ret = bdrv_parse_cache_mode(src_cache, _flags, _writethrough);
if (ret < 0) {
error_report("Invalid source cache option: %s", src_cache);
goto out;
}
...

How about moving that down?

Fam