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

[Kevin Wolf]

Am 05.09.2013 um 15:55 hat Max Reitz geschrieben:
> Add an Error ** parameter to bdrv_create and its associated functions to
> allow more specific error messages.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block.c               | 72 
> +++++++++++++++++++++++++++++++++------------------
>  block/cow.c           |  2 +-
>  block/qcow.c          |  2 +-
>  block/qcow2.c         |  2 +-
>  block/qed.c           |  2 +-
>  block/raw_bsd.c       |  2 +-
>  block/vvfat.c         |  2 +-
>  include/block/block.h |  5 ++--
>  qemu-img.c            | 16 ++++--------
>  9 files changed, 61 insertions(+), 44 deletions(-)

> @@ -1053,11 +1078,9 @@ int bdrv_open(BlockDriverState *bs, const char 
> *filename, QDict *options,
>                  drv->format_name);
>          }
>  
> -        ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options);
> +        ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, 
> &local_err);
>          free_option_parameters(create_options);
>          if (ret < 0) {
> -            error_setg_errno(errp, -ret, "Could not create temporary overlay 
> "
> -                             "'%s'", tmp_filename);
>              goto fail;
>          }

I think this message adds value, because a message that makes sense for
a caller of bdrv_create(), doesn't necessarily make sense for a
bdrv_open() caller without further context (like that it's about the
temporary overlay).

I guess you should do something along the lines of:

    error_setg_errno(errp, -ret, "Could not create temporary overlay "
                     "'%s': %s", tmp_filename, error_get_pretty(local_err));
    error_free(local_err);
    local_err = NULL;

> @@ -4553,6 +4576,7 @@ void bdrv_img_create(const char *filename, const char 
> *fmt,
>      BlockDriverState *bs = NULL;
>      BlockDriver *drv, *proto_drv;
>      BlockDriver *backing_drv = NULL;
> +    Error *local_err = NULL;
>      int ret = 0;
>  
>      /* Find driver and parse its options */
> @@ -4639,10 +4663,8 @@ void bdrv_img_create(const char *filename, const char 
> *fmt,
>              bs = bdrv_new("");
>  
>              ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
> -                            backing_drv, NULL);
> +                            backing_drv, &local_err);
>              if (ret < 0) {
> -                error_setg_errno(errp, -ret, "Could not open '%s'",
> -                                 backing_file->value.s);

Same thing about context of the error message here.

>                  goto out;
>              }
>              bdrv_get_geometry(bs, &size);
> @@ -4661,22 +4683,19 @@ void bdrv_img_create(const char *filename, const char 
> *fmt,
>          print_option_parameters(param);
>          puts("");
>      }
> -    ret = bdrv_create(drv, filename, param);
> -    if (ret < 0) {
> -        if (ret == -ENOTSUP) {
> -            error_setg(errp,"Formatting or formatting option not supported 
> for "
> -                            "file format '%s'", fmt);
> -        } else if (ret == -EFBIG) {
> -            const char *cluster_size_hint = "";
> -            if (get_option_parameter(create_options, 
> BLOCK_OPT_CLUSTER_SIZE)) {
> -                cluster_size_hint = " (try using a larger cluster size)";
> -            }
> -            error_setg(errp, "The image size is too large for file format 
> '%s'%s",
> -                       fmt, cluster_size_hint);
> -        } else {
> -            error_setg(errp, "%s: error while creating %s: %s", filename, 
> fmt,
> -                       strerror(-ret));
> +    ret = bdrv_create(drv, filename, param, &local_err);
> +    if (ret == -EFBIG) {
> +        /* This is generally a better message than whatever the driver would
> +         * deliver (especially because of the cluster_size_hint), since that
> +         * is most probably not much different from "image too large". */
> +        const char *cluster_size_hint = "";
> +        if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) {
> +            cluster_size_hint = " (try using a larger cluster size)";
>          }
> +        error_setg(errp, "The image size is too large for file format '%s'"
> +                   "%s", fmt, cluster_size_hint);
> +        error_free(local_err);
> +        local_err = NULL;
>      }

This should eventually be moved into the block drivers as well, but okay
with me for now.

Kevin