Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/2] qemu-img: better error message when opening a backing file fails

["Daniel P. Berrangé" <berrange@redhat.com>]

On Sun, Jul 21, 2019 at 09:15:08PM +0300, Maxim Levitsky wrote:
> Currently we print message like that:
> 
> "
> new_file.qcow2 : error message
> "
> 
> However the error could have come from opening the backing file (e.g when it missing encryption keys),
> thus try to clarify this by using this format:
> 
> "
> qemu-img: error creating new_file.qcow2: base_file.qcow2: error message
> Could not open backing image to determine size.
> "
> 
> 
> Test used:
> 
> qemu-img create -f qcow2 \
>         --object secret,id=sec0,data=hunter9 \
>         --object secret,id=sec1,data=my_new_secret_password \
>         -b 'json:{ "encrypt.key-secret": "sec1", "driver": "qcow2", "file": { "driver": "file", "filename": "base.qcow2" }}' \
>         -o encrypt.format=luks,encrypt.key-secret=sec1 \
>         sn.qcow2
> 
> 
> Error message before:
> 
> qemu-img: sn.qcow2: Invalid password, cannot unlock any keyslot
> Could not open backing image to determine size.
> 
> 
> Error message after:
> 
> qemu-img: error creating sn.qcow2: \
> 	json:{ "encrypt.key-secret": "sec1", "driver": "qcow2", "file": { "driver": "file", "filename": "base.qcow2" }}: \
> 	Invalid password, cannot unlock any keyslot
> Could not open backing image to determine size.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block.c    | 1 +
>  qemu-img.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 29e931e217..5eb47b2199 100644
> --- a/block.c
> +++ b/block.c
> @@ -5790,6 +5790,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                              "This may become an error in future versions.\n");
>              local_err = NULL;
>          } else if (!bs) {
> +            error_prepend(&local_err, "%s: ", backing_file);
>              /* Couldn't open bs, do not have size */
>              error_append_hint(&local_err,
>                                "Could not open backing image to determine size.\n");

I think it'd be better todo

              error_append_hint(&local_err,
                                "Could not open backing image '%s' to determine size.\n",
                                 backing_file);

At least when backing_file isn't a horrible blob of JSON, the error
message is easier to read this way IMHO.

> diff --git a/qemu-img.c b/qemu-img.c
> index 79983772de..134bf2fbe0 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -545,7 +545,7 @@ static int img_create(int argc, char **argv)
>      bdrv_img_create(filename, fmt, base_filename, base_fmt,
>                      options, img_size, flags, quiet, &local_err);
>      if (local_err) {
> -        error_reportf_err(local_err, "%s: ", filename);
> +        error_reportf_err(local_err, "error creating %s: ", filename);
>          goto fail;
>      }
>  
> -- 
> 2.17.2
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|