| Message ID | 20190721181508.28608-3-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RFC: Trivial error message fixes for luks format | expand |
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
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 >
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
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
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; }
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(-)