[Qemu-trivial] [PATCH 0/2] RFC: Trivial error message fixes for luks format

[Qemu-trivial] [PATCH 0/2] RFC: Trivial error message fixes for luks format

[Maxim Levitsky]

These are attempts to improve a bit error message
based on bunch of luks related bugzillas assigned to me.
Feel free to reject these if you think that it doesn't
make the messages better.

Best regards,
	Maxim Levitsky

Maxim Levitsky (2):
  LUKS: better error message when creating too large files
  qemu-img: better error message when opening a backing file fails

 block.c        |  1 +
 block/crypto.c | 25 +++++++++++++++++++++----
 qemu-img.c     |  2 +-
 3 files changed, 23 insertions(+), 5 deletions(-)

-- 
2.17.2

[Qemu-trivial] [PATCH 1/2] LUKS: better error message when creating too large files

[Maxim Levitsky]

Currently if you attampt to create too large file with luks you
get the following error message:

Formatting 'test.luks', fmt=luks size=17592186044416 key-secret=sec0
qemu-img: test.luks: Could not resize file: File too large

While for raw format the error message is
qemu-img: test.img: The image size is too large for file format 'raw'


The reason for this is that qemu-img checks for errono of the failure,
and presents the later error when it is -EFBIG

However crypto generic code 'swallows' the errno and replaces it
with -EIO.

As an attempt to make it better, we can make luks driver,
detect -EFBIG and in this case present a better error message,
which is what this patch does

The new error message is:

qemu-img: error creating test.luks: The requested file size is too large

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534898
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/crypto.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index 8237424ae6..73b1013fa1 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -102,18 +102,35 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
                                       Error **errp)
 {
     struct BlockCryptoCreateData *data = opaque;
+    Error *local_error = NULL;
+    int ret;
 
     if (data->size > INT64_MAX || headerlen > INT64_MAX - data->size) {
-        error_setg(errp, "The requested file size is too large");
-        return -EFBIG;
+        ret = -EFBIG;
+        goto error;
     }
 
     /* User provided size should reflect amount of space made
      * available to the guest, so we must take account of that
      * which will be used by the crypto header
      */
-    return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF,
-                        errp);
+    ret = blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF,
+                       &local_error);
+
+    if (ret >= 0) {
+        return ret;
+    }
+
+error:
+    if (ret == -EFBIG) {
+        /* Replace the error message with a better one */
+        error_free(local_error);
+        error_setg(errp, "The requested file size is too large");
+    } else {
+        error_propagate(errp, local_error);
+    }
+
+    return ret;
 }
 
 
-- 
2.17.2

Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/2] LUKS: better error message when creating too large files

[Daniel P. Berrangé]

On Sun, Jul 21, 2019 at 09:15:07PM +0300, Maxim Levitsky wrote:
> Currently if you attampt to create too large file with luks you
> get the following error message:
> 
> Formatting 'test.luks', fmt=luks size=17592186044416 key-secret=sec0
> qemu-img: test.luks: Could not resize file: File too large
> 
> While for raw format the error message is
> qemu-img: test.img: The image size is too large for file format 'raw'
> 
> 
> The reason for this is that qemu-img checks for errono of the failure,
> and presents the later error when it is -EFBIG
> 
> However crypto generic code 'swallows' the errno and replaces it
> with -EIO.
> 
> As an attempt to make it better, we can make luks driver,
> detect -EFBIG and in this case present a better error message,
> which is what this patch does
> 
> The new error message is:
> 
> qemu-img: error creating test.luks: The requested file size is too large
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534898
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/crypto.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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 :|

Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/2] LUKS: better error message when creating too large files

[Maxim Levitsky]

On Mon, 2019-07-22 at 10:05 +0100, Daniel P. Berrangé wrote:
> On Sun, Jul 21, 2019 at 09:15:07PM +0300, Maxim Levitsky wrote:
> > Currently if you attampt to create too large file with luks you
> > get the following error message:
> > 
> > Formatting 'test.luks', fmt=luks size=17592186044416 key-secret=sec0
> > qemu-img: test.luks: Could not resize file: File too large
> > 
> > While for raw format the error message is
> > qemu-img: test.img: The image size is too large for file format 'raw'
> > 
> > 
> > The reason for this is that qemu-img checks for errono of the failure,
> > and presents the later error when it is -EFBIG
> > 
> > However crypto generic code 'swallows' the errno and replaces it
> > with -EIO.
> > 
> > As an attempt to make it better, we can make luks driver,
> > detect -EFBIG and in this case present a better error message,
> > which is what this patch does
> > 
> > The new error message is:
> > 
> > qemu-img: error creating test.luks: The requested file size is too large
> > 
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534898
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/crypto.c | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Regards,
> Daniel
Hi!

Do you think that we should merge this or, shall I close the bugreport
as WONTFIX?

Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH 1/2] LUKS: better error message when creating too large files

[Maxim Levitsky]

On Mon, 2019-07-22 at 10:05 +0100, Daniel P. Berrangé wrote:
> On Sun, Jul 21, 2019 at 09:15:07PM +0300, Maxim Levitsky wrote:
> > Currently if you attampt to create too large file with luks you
> > get the following error message:
> > 
> > Formatting 'test.luks', fmt=luks size=17592186044416 key-secret=sec0
> > qemu-img: test.luks: Could not resize file: File too large
> > 
> > While for raw format the error message is
> > qemu-img: test.img: The image size is too large for file format 'raw'
> > 
> > 
> > The reason for this is that qemu-img checks for errono of the failure,
> > and presents the later error when it is -EFBIG
> > 
> > However crypto generic code 'swallows' the errno and replaces it
> > with -EIO.
> > 
> > As an attempt to make it better, we can make luks driver,
> > detect -EFBIG and in this case present a better error message,
> > which is what this patch does
> > 
> > The new error message is:
> > 
> > qemu-img: error creating test.luks: The requested file size is too large
> > 
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534898
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/crypto.c | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Could someone pick to a maintainer tree, now that freeze is over?


Best regards,
	Maxim Levitsky
> 
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH 1/2] LUKS: better error message when creating too large files

[Maxim Levitsky]

On Thu, 2020-04-30 at 15:18 +0300, Maxim Levitsky wrote:
> On Mon, 2019-07-22 at 10:05 +0100, Daniel P. Berrangé wrote:
> > On Sun, Jul 21, 2019 at 09:15:07PM +0300, Maxim Levitsky wrote:
> > > Currently if you attampt to create too large file with luks you
> > > get the following error message:
> > > 
> > > Formatting 'test.luks', fmt=luks size=17592186044416 key-secret=sec0
> > > qemu-img: test.luks: Could not resize file: File too large
> > > 
> > > While for raw format the error message is
> > > qemu-img: test.img: The image size is too large for file format 'raw'
> > > 
> > > 
> > > The reason for this is that qemu-img checks for errono of the failure,
> > > and presents the later error when it is -EFBIG
> > > 
> > > However crypto generic code 'swallows' the errno and replaces it
> > > with -EIO.
> > > 
> > > As an attempt to make it better, we can make luks driver,
> > > detect -EFBIG and in this case present a better error message,
> > > which is what this patch does
> > > 
> > > The new error message is:
> > > 
> > > qemu-img: error creating test.luks: The requested file size is too large
> > > 
> > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534898
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > >  block/crypto.c | 25 +++++++++++++++++++++----
> > >  1 file changed, 21 insertions(+), 4 deletions(-)
> > 
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> 
> Could someone pick to a maintainer tree, now that freeze is over?
> 
> 
> Best regards,
> 	Maxim Levitsky
> > 
> > Regards,
> > Daniel
> 
> 
I replied to an old version, please disregard,
Best regards,
	Maxim Levitsky

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

[Maxim Levitsky]

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

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

[Daniel P. Berrangé]

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 :|

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

[Maxim Levitsky]

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

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

[Kevin Wolf]

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
> 

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

[Maxim Levitsky]

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