Re: [Qemu-block] [PATCH 1/2] qemu-img: make img_open() able to surpress file opening errors

[Max Reitz]

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 <address@hidden>
> ---
>  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
>

signature.asc
Description: OpenPGP digital signature