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

[Maxim Levitsky <mlevitsk@redhat.com>]

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