| Message ID | 1492661065-18510-1-git-send-email-lidongchen@tencent.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 04/20 12:04, jemmy858585@gmail.com wrote: > From: Lidong Chen <lidongchen@tencent.com> > > 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 <lidongchen@tencent.com> > --- > 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, &src_flags, &src_writethrough); if (ret < 0) { error_report("Invalid source cache option: %s", src_cache); goto out; } ... How about moving that down? Fam
On Thu, Apr 20, 2017 at 3:51 PM, Fam Zheng <famz@redhat.com> wrote: > On Thu, 04/20 12:04, jemmy858585@gmail.com wrote: >> From: Lidong Chen <lidongchen@tencent.com> >> >> 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 <lidongchen@tencent.com> >> --- >> 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, &src_flags, &src_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
On Thu, 04/20 15:59, 858585 jemmy wrote: > On Thu, Apr 20, 2017 at 3:51 PM, Fam Zheng <famz@redhat.com> wrote: > > On Thu, 04/20 12:04, jemmy858585@gmail.com wrote: > >> From: Lidong Chen <lidongchen@tencent.com> > >> > >> 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 <lidongchen@tencent.com> > >> --- > >> 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, &src_flags, &src_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
On Thu, Apr 20, 2017 at 4:11 PM, Fam Zheng <famz@redhat.com> wrote: > On Thu, 04/20 15:59, 858585 jemmy wrote: >> On Thu, Apr 20, 2017 at 3:51 PM, Fam Zheng <famz@redhat.com> wrote: >> > On Thu, 04/20 12:04, jemmy858585@gmail.com wrote: >> >> From: Lidong Chen <lidongchen@tencent.com> >> >> >> >> 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 <lidongchen@tencent.com> >> >> --- >> >> 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, &src_flags, &src_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
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 =