[2/2] qemu-img: better error message when opening a backing file fails
diff mbox series

Message ID 20190721181508.28608-3-mlevitsk@redhat.com
State New
Headers show
Series
  • [1/2] LUKS: better error message when creating too large files
Related show

Commit Message

Maxim Levitsky July 21, 2019, 6:15 p.m. UTC
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(-)

Comments

Daniel P. Berrangé July 22, 2019, 9:15 a.m. UTC | #1
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
Kevin Wolf July 22, 2019, 9:41 a.m. UTC | #2
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
>
Maxim Levitsky July 22, 2019, 9:46 a.m. UTC | #3
On Mon, 2019-07-22 at 11:41 +0200, Kevin Wolf wrote:
> 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

You raise a very good point, I didn't thought about this.
Thanks,

Best regards,
	Maxim Levitsky
Maxim Levitsky July 22, 2019, 10:01 a.m. UTC | #4
On Mon, 2019-07-22 at 10:15 +0100, Daniel P. Berrangé wrote:
> 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.
I agree, but I guess I need to drop this patch because of possible nesting of the backing files,
as Kevin Wolf pointed out.

Best regards,
	Maxim Levitsky

Patch
diff mbox series

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;
     }