Re: [Qemu-devel] [RFC v2 3/6] block: Error parameter for open functions

[Kevin Wolf]

Am 05.09.2013 um 15:55 hat Max Reitz geschrieben:
> Add an Error ** parameter to bdrv_open, bdrv_file_open and associated
> functions to allow more specific error messages.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block.c               | 98 
> ++++++++++++++++++++++++++++++++-------------------
>  block/blkdebug.c      |  2 +-
>  block/blkverify.c     |  4 +--
>  block/cow.c           |  2 +-
>  block/mirror.c        |  5 +--
>  block/qcow.c          |  2 +-
>  block/qcow2.c         |  4 +--
>  block/qed.c           |  3 +-
>  block/sheepdog.c      |  4 +--
>  block/vmdk.c          |  5 +--
>  block/vvfat.c         |  2 +-
>  blockdev.c            | 30 +++++++---------
>  include/block/block.h |  6 ++--
>  qemu-img.c            | 23 +++++++-----
>  qemu-io.c             | 14 +++++---
>  qemu-nbd.c            |  6 ++--
>  16 files changed, 125 insertions(+), 85 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f485906..8da32b2 100644
> --- a/block.c
> +++ b/block.c
> @@ -525,7 +525,7 @@ BlockDriver *bdrv_find_protocol(const char *filename,
>  }
>  
>  static int find_image_format(BlockDriverState *bs, const char *filename,
> -                             BlockDriver **pdrv)
> +                             BlockDriver **pdrv, Error **errp)
>  {
>      int score, score_max;
>      BlockDriver *drv1, *drv;
> @@ -536,6 +536,7 @@ static int find_image_format(BlockDriverState *bs, const 
> char *filename,
>      if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
>          drv = bdrv_find_format("raw");
>          if (!drv) {
> +            error_setg(errp, "Could not find raw image format");
>              ret = -ENOENT;
>          }
>          *pdrv = drv;
> @@ -544,6 +545,8 @@ static int find_image_format(BlockDriverState *bs, const 
> char *filename,
>  
>      ret = bdrv_pread(bs, 0, buf, sizeof(buf));
>      if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Could not read image for determining 
> its "
> +                         "format");
>          *pdrv = NULL;
>          return ret;
>      }
> @@ -560,6 +563,8 @@ static int find_image_format(BlockDriverState *bs, const 
> char *filename,
>          }
>      }
>      if (!drv) {
> +        error_setg(errp, "Could not determine image format: No compatible "
> +                   "driver found");
>          ret = -ENOENT;
>      }
>      *pdrv = drv;
> @@ -679,10 +684,11 @@ static int bdrv_open_flags(BlockDriverState *bs, int 
> flags)
>   * Removes all processed options from *options.
>   */
>  static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
> -    QDict *options, int flags, BlockDriver *drv)
> +    QDict *options, int flags, BlockDriver *drv, Error **errp)
>  {
>      int ret, open_flags;
>      const char *filename;
> +    Error *local_err = NULL;
>  
>      assert(drv != NULL);
>      assert(bs->file == NULL);
> @@ -711,6 +717,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BlockDriverState *file,
>      bs->read_only = !(open_flags & BDRV_O_RDWR);
>  
>      if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
> +        error_setg(errp, "Driver '%s' is not whitelisted", drv->format_name);
>          return -ENOTSUP;
>      }
>  
> @@ -734,25 +741,30 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BlockDriverState *file,
>      if (drv->bdrv_file_open) {
>          assert(file == NULL);
>          assert(drv->bdrv_parse_filename || filename != NULL);
> -        ret = drv->bdrv_file_open(bs, options, open_flags, NULL);
> +        ret = drv->bdrv_file_open(bs, options, open_flags, &local_err);
>      } else {
>          if (file == NULL) {
> -            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't use '%s' as a "
> -                          "block driver for the protocol level",
> -                          drv->format_name);
> +            error_setg(errp, "Can't use '%s' as a block driver for the "
> +                       "protocol level", drv->format_name);
>              ret = -EINVAL;
>              goto free_and_fail;
>          }
>          bs->file = file;
> -        ret = drv->bdrv_open(bs, options, open_flags, NULL);
> +        ret = drv->bdrv_open(bs, options, open_flags, &local_err);
>      }
>  
>      if (ret < 0) {
> +        if (error_is_set(&local_err)) {
> +            error_propagate(errp, local_err);
> +        } else {
> +            error_setg_errno(errp, -ret, "Could not open '%s'", filename);

filename can be NULL. This happens in cases like:

-drive file.driver=nbd,file.host=localhost

> +        }
>          goto free_and_fail;
>      }
>  
>      ret = refresh_total_sectors(bs, bs->total_sectors);
>      if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Could not refresh total sector count");
>          goto free_and_fail;
>      }
>  

> diff --git a/qemu-img.c b/qemu-img.c
> index b9a848d..607981e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -264,6 +264,7 @@ static BlockDriverState *bdrv_new_open(const char 
> *filename,
>      BlockDriverState *bs;
>      BlockDriver *drv;
>      char password[256];
> +    Error *local_err = NULL;
>      int ret;
>  
>      bs = bdrv_new("image");
> @@ -278,9 +279,11 @@ static BlockDriverState *bdrv_new_open(const char 
> *filename,
>          drv = NULL;
>      }
>  
> -    ret = bdrv_open(bs, filename, NULL, flags, drv);
> +    ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err);
>      if (ret < 0) {
> -        error_report("Could not open '%s': %s", filename, strerror(-ret));
> +        error_report("Could not open '%s': %s", filename,
> +                     error_get_pretty(local_err));
> +        error_free(local_err);
>          goto fail;
>      }
>  
> @@ -407,7 +410,7 @@ static int img_create(int argc, char **argv)
>      bdrv_img_create(filename, fmt, base_filename, base_fmt,
>                      options, img_size, BDRV_O_FLAGS, &local_err, quiet);
>      if (error_is_set(&local_err)) {
> -        error_report("%s", error_get_pretty(local_err));
> +        error_report("%s: %s", filename, error_get_pretty(local_err));

This is an independent change. :-)

It's probably also better handled with the loc_*() functions that define
an area where some given information (like the filename) is prepended to
all error messages printed by error_report().

>          error_free(local_err);
>          return 1;
>      }

Kevin