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

From: Kevin Wolf <kwolf@redhat.com>
To: Maxim Levitsky <mlevitsk@redhat.com>
Cc: qemu-block@nongnu.org, qemu-trivial@nongnu.org,
	Michael Tokarev <mjt@tls.msk.ru>,
	Laurent Vivier <laurent@vivier.eu>,
	qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 2/2] qemu-img: better error message when opening a backing file fails
Date: Mon, 22 Jul 2019 11:41:03 +0200	[thread overview]
Message-ID: <20190722094103.GA6583@localhost.localdomain> (raw)
In-Reply-To: <20190721181508.28608-3-mlevitsk@redhat.com>

Am 21.07.2019 um 20:15 hat Maxim Levitsky geschrieben:
> 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.
> "

The old error message was just unspecific. Your new error message can be
actively misleading because you just unconditionally print the filename
of the direct backing file, even though the error could have occurred
while opening the backing file of the backing file (or even further down
the backing chain).

It's a common problem we have with backing files and error messages: We
either don't print the filename where the error actually happened (like
in this case), or we print all of the backing files in the chain (such
as "Could not open top.qcow2: Could not open mid.qcow2: Could not open
base.qcow2: Invalid something").

Ideally, we'd find a way to print only the backing filename in such
cases ("Could not open base.qcow2: Invalid something"). I'd gladly
accept a patch that fixes error messages in this way for both open and
create, but I'm afraid that your approach in this patch is too
simplistic and not an improvement.

Kevin

> 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");
> 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
> 