| Message ID | 20161007153617.28704-2-fullmanet@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07.10.2016 17:36, Reda Sallahi wrote: > Previously img_open() always printed file opening errors even if the intended > action is just to check the file existence as is sometimes the case in > qemu-img dd. > > This adds an argument (openerror) to img_open() that specifies whether to print > an opening error or not. > > Signed-off-by: Reda Sallahi <fullmanet@gmail.com> > --- > qemu-img.c | 54 ++++++++++++++++++++++++++++++-------------------- > tests/qemu-iotests/160 | 1 + > 2 files changed, 33 insertions(+), 22 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 219fd86..6c088bd 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -269,7 +269,7 @@ static int img_open_password(BlockBackend *blk, const char *filename, > > static BlockBackend *img_open_opts(const char *optstr, > QemuOpts *opts, int flags, bool writethrough, > - bool quiet) > + bool quiet, bool openerror) I think it would be better to introduce an own function which checks whether the image file exists or not. There are three reasons for this: First, if we ever have the infrastructure to actually check this, using it there is more straightforward. Not too good of a reason, though, since that's a big "if". Second, img_open_opts() invokes img_open_password(). We probably don't want to do that if we just want to check the existence of an image file. That's a better reason, and however you decide, you should make sure that img_open_password() is not called when we just want to confirm the existence of an image file. And third, having an own function would make this patch simpler because you don't have to change all the previous places that call img_open_opts(). So the new function (static bool img_check_existence(const char *optstr, QemuOpts *opts, int flags) or something) would just invoke qemu_opts_to_qdict(), blk_new_open() (with NULL as errp) and then return false if blk_new_open() failed or call blk_unref() on the returned BB if it succeeded and then return true. Max > { > QDict *options; > Error *local_err = NULL; > @@ -277,7 +277,9 @@ static BlockBackend *img_open_opts(const char *optstr, > options = qemu_opts_to_qdict(opts, NULL); > blk = blk_new_open(NULL, NULL, options, flags, &local_err); > if (!blk) { > - error_reportf_err(local_err, "Could not open '%s': ", optstr); > + if (openerror) { > + error_reportf_err(local_err, "Could not open '%s': ", optstr); > + } > return NULL; > } > blk_set_enable_write_cache(blk, !writethrough); > @@ -291,7 +293,8 @@ static BlockBackend *img_open_opts(const char *optstr, > > static BlockBackend *img_open_file(const char *filename, > const char *fmt, int flags, > - bool writethrough, bool quiet) > + bool writethrough, bool quiet, > + bool openerror) > { > BlockBackend *blk; > Error *local_err = NULL; > @@ -304,7 +307,9 @@ static BlockBackend *img_open_file(const char *filename, > > blk = blk_new_open(filename, NULL, options, flags, &local_err); > if (!blk) { > - error_reportf_err(local_err, "Could not open '%s': ", filename); > + if (openerror) { > + error_reportf_err(local_err, "Could not open '%s': ", filename); > + } > return NULL; > } > blk_set_enable_write_cache(blk, !writethrough); > @@ -320,7 +325,7 @@ static BlockBackend *img_open_file(const char *filename, > static BlockBackend *img_open(bool image_opts, > const char *filename, > const char *fmt, int flags, bool writethrough, > - bool quiet) > + bool quiet, bool openerror) > { > BlockBackend *blk; > if (image_opts) { > @@ -334,9 +339,11 @@ static BlockBackend *img_open(bool image_opts, > if (!opts) { > return NULL; > } > - blk = img_open_opts(filename, opts, flags, writethrough, quiet); > + blk = img_open_opts(filename, opts, flags, writethrough, quiet, > + openerror); > } else { > - blk = img_open_file(filename, fmt, flags, writethrough, quiet); > + blk = img_open_file(filename, fmt, flags, writethrough, quiet, > + openerror); > } > return blk; > } > @@ -706,7 +713,7 @@ static int img_check(int argc, char **argv) > return 1; > } > > - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet); > + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet, true); > if (!blk) { > return 1; > } > @@ -898,7 +905,7 @@ static int img_commit(int argc, char **argv) > return 1; > } > > - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet); > + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet, true); > if (!blk) { > return 1; > } > @@ -1232,13 +1239,15 @@ static int img_compare(int argc, char **argv) > goto out3; > } > > - blk1 = img_open(image_opts, filename1, fmt1, flags, writethrough, quiet); > + blk1 = img_open(image_opts, filename1, fmt1, flags, writethrough, quiet, > + true); > if (!blk1) { > ret = 2; > goto out3; > } > > - blk2 = img_open(image_opts, filename2, fmt2, flags, writethrough, quiet); > + blk2 = img_open(image_opts, filename2, fmt2, flags, writethrough, quiet, > + true); > if (!blk2) { > ret = 2; > goto out2; > @@ -1943,7 +1952,7 @@ static int img_convert(int argc, char **argv) > total_sectors = 0; > for (bs_i = 0; bs_i < bs_n; bs_i++) { > blk[bs_i] = img_open(image_opts, argv[optind + bs_i], > - fmt, src_flags, src_writethrough, quiet); > + fmt, src_flags, src_writethrough, quiet, true); > if (!blk[bs_i]) { > ret = -1; > goto out; > @@ -2088,7 +2097,8 @@ static int img_convert(int argc, char **argv) > * the bdrv_create() call which takes different params. > * Not critical right now, so fix can wait... > */ > - out_blk = img_open_file(out_filename, out_fmt, flags, writethrough, quiet); > + out_blk = img_open_file(out_filename, out_fmt, flags, writethrough, quiet, > + true); > if (!out_blk) { > ret = -1; > goto out; > @@ -2280,7 +2290,7 @@ static ImageInfoList *collect_image_info_list(bool image_opts, > g_hash_table_insert(filenames, (gpointer)filename, NULL); > > blk = img_open(image_opts, filename, fmt, > - BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false); > + BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false, true); > if (!blk) { > goto err; > } > @@ -2606,7 +2616,7 @@ static int img_map(int argc, char **argv) > return 1; > } > > - blk = img_open(image_opts, filename, fmt, 0, false, false); > + blk = img_open(image_opts, filename, fmt, 0, false, false, true); > if (!blk) { > return 1; > } > @@ -2750,7 +2760,7 @@ static int img_snapshot(int argc, char **argv) > } > > /* Open the image */ > - blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet); > + blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet, true); > if (!blk) { > return 1; > } > @@ -2925,7 +2935,7 @@ static int img_rebase(int argc, char **argv) > * Ignore the old backing file for unsafe rebase in case we want to correct > * the reference to a renamed or moved backing file. > */ > - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet); > + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet, true); > if (!blk) { > ret = -1; > goto out; > @@ -3265,7 +3275,7 @@ static int img_resize(int argc, char **argv) > qemu_opts_del(param); > > blk = img_open(image_opts, filename, fmt, > - BDRV_O_RDWR, false, quiet); > + BDRV_O_RDWR, false, quiet, true); > if (!blk) { > ret = -1; > goto out; > @@ -3423,7 +3433,7 @@ static int img_amend(int argc, char **argv) > goto out; > } > > - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet); > + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet, true); > if (!blk) { > ret = -1; > goto out; > @@ -3741,7 +3751,7 @@ static int img_bench(int argc, char **argv) > goto out; > } > > - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet); > + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet, true); > if (!blk) { > ret = -1; > goto out; > @@ -4025,7 +4035,7 @@ static int img_dd(int argc, char **argv) > ret = -1; > goto out; > } > - blk1 = img_open(image_opts, in.filename, fmt, 0, false, false); > + blk1 = img_open(image_opts, in.filename, fmt, 0, false, false, true); > > if (!blk1) { > ret = -1; > @@ -4093,7 +4103,7 @@ static int img_dd(int argc, char **argv) > } > > blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR, > - false, false); > + false, false, true); > > if (!blk2) { > ret = -1; > diff --git a/tests/qemu-iotests/160 b/tests/qemu-iotests/160 > index 53b3c30..f44834f 100755 > --- a/tests/qemu-iotests/160 > +++ b/tests/qemu-iotests/160 > @@ -65,6 +65,7 @@ for skip in $TEST_SKIP_BLOCKS; do > echo "== Compare the images with qemu-img compare ==" > > $QEMU_IMG compare "$TEST_IMG.out.dd" "$TEST_IMG.out" > + rm -f "$TEST_IMG.out.dd" > done > > echo >
diff --git a/qemu-img.c b/qemu-img.c index 219fd86..6c088bd 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -269,7 +269,7 @@ static int img_open_password(BlockBackend *blk, const char *filename, static BlockBackend *img_open_opts(const char *optstr, QemuOpts *opts, int flags, bool writethrough, - bool quiet) + bool quiet, bool openerror) { QDict *options; Error *local_err = NULL; @@ -277,7 +277,9 @@ static BlockBackend *img_open_opts(const char *optstr, options = qemu_opts_to_qdict(opts, NULL); blk = blk_new_open(NULL, NULL, options, flags, &local_err); if (!blk) { - error_reportf_err(local_err, "Could not open '%s': ", optstr); + if (openerror) { + error_reportf_err(local_err, "Could not open '%s': ", optstr); + } return NULL; } blk_set_enable_write_cache(blk, !writethrough); @@ -291,7 +293,8 @@ static BlockBackend *img_open_opts(const char *optstr, static BlockBackend *img_open_file(const char *filename, const char *fmt, int flags, - bool writethrough, bool quiet) + bool writethrough, bool quiet, + bool openerror) { BlockBackend *blk; Error *local_err = NULL; @@ -304,7 +307,9 @@ static BlockBackend *img_open_file(const char *filename, blk = blk_new_open(filename, NULL, options, flags, &local_err); if (!blk) { - error_reportf_err(local_err, "Could not open '%s': ", filename); + if (openerror) { + error_reportf_err(local_err, "Could not open '%s': ", filename); + } return NULL; } blk_set_enable_write_cache(blk, !writethrough); @@ -320,7 +325,7 @@ static BlockBackend *img_open_file(const char *filename, static BlockBackend *img_open(bool image_opts, const char *filename, const char *fmt, int flags, bool writethrough, - bool quiet) + bool quiet, bool openerror) { BlockBackend *blk; if (image_opts) { @@ -334,9 +339,11 @@ static BlockBackend *img_open(bool image_opts, if (!opts) { return NULL; } - blk = img_open_opts(filename, opts, flags, writethrough, quiet); + blk = img_open_opts(filename, opts, flags, writethrough, quiet, + openerror); } else { - blk = img_open_file(filename, fmt, flags, writethrough, quiet); + blk = img_open_file(filename, fmt, flags, writethrough, quiet, + openerror); } return blk; } @@ -706,7 +713,7 @@ static int img_check(int argc, char **argv) return 1; } - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet); + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet, true); if (!blk) { return 1; } @@ -898,7 +905,7 @@ static int img_commit(int argc, char **argv) return 1; } - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet); + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet, true); if (!blk) { return 1; } @@ -1232,13 +1239,15 @@ static int img_compare(int argc, char **argv) goto out3; } - blk1 = img_open(image_opts, filename1, fmt1, flags, writethrough, quiet); + blk1 = img_open(image_opts, filename1, fmt1, flags, writethrough, quiet, + true); if (!blk1) { ret = 2; goto out3; } - blk2 = img_open(image_opts, filename2, fmt2, flags, writethrough, quiet); + blk2 = img_open(image_opts, filename2, fmt2, flags, writethrough, quiet, + true); if (!blk2) { ret = 2; goto out2; @@ -1943,7 +1952,7 @@ static int img_convert(int argc, char **argv) total_sectors = 0; for (bs_i = 0; bs_i < bs_n; bs_i++) { blk[bs_i] = img_open(image_opts, argv[optind + bs_i], - fmt, src_flags, src_writethrough, quiet); + fmt, src_flags, src_writethrough, quiet, true); if (!blk[bs_i]) { ret = -1; goto out; @@ -2088,7 +2097,8 @@ static int img_convert(int argc, char **argv) * the bdrv_create() call which takes different params. * Not critical right now, so fix can wait... */ - out_blk = img_open_file(out_filename, out_fmt, flags, writethrough, quiet); + out_blk = img_open_file(out_filename, out_fmt, flags, writethrough, quiet, + true); if (!out_blk) { ret = -1; goto out; @@ -2280,7 +2290,7 @@ static ImageInfoList *collect_image_info_list(bool image_opts, g_hash_table_insert(filenames, (gpointer)filename, NULL); blk = img_open(image_opts, filename, fmt, - BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false); + BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false, true); if (!blk) { goto err; } @@ -2606,7 +2616,7 @@ static int img_map(int argc, char **argv) return 1; } - blk = img_open(image_opts, filename, fmt, 0, false, false); + blk = img_open(image_opts, filename, fmt, 0, false, false, true); if (!blk) { return 1; } @@ -2750,7 +2760,7 @@ static int img_snapshot(int argc, char **argv) } /* Open the image */ - blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet); + blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet, true); if (!blk) { return 1; } @@ -2925,7 +2935,7 @@ static int img_rebase(int argc, char **argv) * Ignore the old backing file for unsafe rebase in case we want to correct * the reference to a renamed or moved backing file. */ - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet); + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet, true); if (!blk) { ret = -1; goto out; @@ -3265,7 +3275,7 @@ static int img_resize(int argc, char **argv) qemu_opts_del(param); blk = img_open(image_opts, filename, fmt, - BDRV_O_RDWR, false, quiet); + BDRV_O_RDWR, false, quiet, true); if (!blk) { ret = -1; goto out; @@ -3423,7 +3433,7 @@ static int img_amend(int argc, char **argv) goto out; } - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet); + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet, true); if (!blk) { ret = -1; goto out; @@ -3741,7 +3751,7 @@ static int img_bench(int argc, char **argv) goto out; } - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet); + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet, true); if (!blk) { ret = -1; goto out; @@ -4025,7 +4035,7 @@ static int img_dd(int argc, char **argv) ret = -1; goto out; } - blk1 = img_open(image_opts, in.filename, fmt, 0, false, false); + blk1 = img_open(image_opts, in.filename, fmt, 0, false, false, true); if (!blk1) { ret = -1; @@ -4093,7 +4103,7 @@ static int img_dd(int argc, char **argv) } blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR, - false, false); + false, false, true); if (!blk2) { ret = -1; diff --git a/tests/qemu-iotests/160 b/tests/qemu-iotests/160 index 53b3c30..f44834f 100755 --- a/tests/qemu-iotests/160 +++ b/tests/qemu-iotests/160 @@ -65,6 +65,7 @@ for skip in $TEST_SKIP_BLOCKS; do echo "== Compare the images with qemu-img compare ==" $QEMU_IMG compare "$TEST_IMG.out.dd" "$TEST_IMG.out" + rm -f "$TEST_IMG.out.dd" done echo
Previously img_open() always printed file opening errors even if the intended action is just to check the file existence as is sometimes the case in qemu-img dd. This adds an argument (openerror) to img_open() that specifies whether to print an opening error or not. Signed-off-by: Reda Sallahi <fullmanet@gmail.com> --- qemu-img.c | 54 ++++++++++++++++++++++++++++++-------------------- tests/qemu-iotests/160 | 1 + 2 files changed, 33 insertions(+), 22 deletions(-)