[libvirt] [PATCH] qemuDomainObjStart: Warn on corrupted image
If the managedsave image is corrupted, e.g. the XML part is, we fail to parse it and throw an error, e.g.: error: Failed to start domain jms8 error: XML error: missing security model when using multiple labels This is okay, as we can't really start the machine and avoid undefined qemu behaviour. On the other hand, the error message doesn't give a clue to users what should they do. The consensus here would be to thrown a warning to logs saying "Hey, you've got a corrupted file". Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c3daad..08be24b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6041,8 +6041,11 @@ qemuDomainObjStart(virConnectPtr conn, if (ret > 0) VIR_WARN("Ignoring incomplete managed state %s", managed_save); - else + else { + VIR_WARN("Unable to restore from managed state %s. " + "Maybe the file is corrupted?", managed_save); goto cleanup; + } } } -- 1.8.3.2
On 11/11/2013 08:48 AM, Michal Privoznik wrote:
If the managedsave image is corrupted, e.g. the XML part is, we fail to parse it and throw an error, e.g.:
error: Failed to start domain jms8 error: XML error: missing security model when using multiple labels
This is okay, as we can't really start the machine and avoid undefined qemu behaviour. On the other hand, the error message doesn't give a clue to users what should they do. The consensus here would be to thrown
s/thrown/throw/
a warning to logs saying "Hey, you've got a corrupted file".
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/11/13 16:48, Michal Privoznik wrote:
If the managedsave image is corrupted, e.g. the XML part is, we fail to parse it and throw an error, e.g.:
error: Failed to start domain jms8 error: XML error: missing security model when using multiple labels
This is okay, as we can't really start the machine and avoid undefined qemu behaviour. On the other hand, the error message doesn't give a clue to users what should they do. The consensus here would be to thrown
Well a normal user won't check the logs as a first operation. This is a very good example place where libvirt would really benefit from having "stacked" error messages as the low level message doesn't give the user a clue about what happened.
a warning to logs saying "Hey, you've got a corrupted file".
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
I'm NOT ACKing this right away as I personally don't like the change of behavior to report an error in case of a corrupt managed save. We will now require the users to manually delete the corrupted file instead. Peter
On 11.11.2013 16:58, Peter Krempa wrote:
On 11/11/13 16:48, Michal Privoznik wrote:
If the managedsave image is corrupted, e.g. the XML part is, we fail to parse it and throw an error, e.g.:
error: Failed to start domain jms8 error: XML error: missing security model when using multiple labels
This is okay, as we can't really start the machine and avoid undefined qemu behaviour. On the other hand, the error message doesn't give a clue to users what should they do. The consensus here would be to thrown
Well a normal user won't check the logs as a first operation. This is a very good example place where libvirt would really benefit from having "stacked" error messages as the low level message doesn't give the user a clue about what happened.
Couldn't agree more. But that's slightly more of work to be done.
a warning to logs saying "Hey, you've got a corrupted file".
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
I'm NOT ACKing this right away as I personally don't like the change of behavior to report an error in case of a corrupt managed save. We will now require the users to manually delete the corrupted file instead.
Peter
I don't think we are changing the behavior. We are still throwing the original error message. Just instead of not telling how to solve this, we do now. Michal
On 11/11/2013 08:58 AM, Peter Krempa wrote:
On 11/11/13 16:48, Michal Privoznik wrote:
If the managedsave image is corrupted, e.g. the XML part is, we fail to parse it and throw an error, e.g.:
error: Failed to start domain jms8 error: XML error: missing security model when using multiple labels
This is okay, as we can't really start the machine and avoid undefined qemu behaviour. On the other hand, the error message doesn't give a clue to users what should they do. The consensus here would be to thrown
Well a normal user won't check the logs as a first operation. This is a very good example place where libvirt would really benefit from having "stacked" error messages as the low level message doesn't give the user a clue about what happened.
a warning to logs saying "Hey, you've got a corrupted file".
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
I'm NOT ACKing this right away as I personally don't like the change of behavior to report an error in case of a corrupt managed save. We will now require the users to manually delete the corrupted file instead.
This patch had no change in behavior other than an added log message. The problem is that we have pre-existing different behavior for two different cases of broken save images: behaving differently for broken XML (requiring the user to edit the corrupted file, or manually delete it) than we are for working XML but broken image (where we log a message, but otherwise pretend the save file never existed).
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Nov 11, 2013 at 16:48:07 +0100, Michal Privoznik wrote:
If the managedsave image is corrupted, e.g. the XML part is, we fail to parse it and throw an error, e.g.:
error: Failed to start domain jms8 error: XML error: missing security model when using multiple labels
This is okay, as we can't really start the machine and avoid undefined qemu behaviour. On the other hand, the error message doesn't give a clue to users what should they do. The consensus here would be to thrown a warning to logs saying "Hey, you've got a corrupted file".
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c3daad..08be24b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6041,8 +6041,11 @@ qemuDomainObjStart(virConnectPtr conn,
if (ret > 0)
Should be "if (ret > 0) {"
VIR_WARN("Ignoring incomplete managed state %s", managed_save); - else + else {
and "} else {" to follow are coding style rules.
+ VIR_WARN("Unable to restore from managed state %s. " + "Maybe the file is corrupted?", managed_save); goto cleanup; + } } }
Jirka
On 11.11.2013 17:07, Jiri Denemark wrote:
On Mon, Nov 11, 2013 at 16:48:07 +0100, Michal Privoznik wrote:
If the managedsave image is corrupted, e.g. the XML part is, we fail to parse it and throw an error, e.g.:
error: Failed to start domain jms8 error: XML error: missing security model when using multiple labels
This is okay, as we can't really start the machine and avoid undefined qemu behaviour. On the other hand, the error message doesn't give a clue to users what should they do. The consensus here would be to thrown a warning to logs saying "Hey, you've got a corrupted file".
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c3daad..08be24b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6041,8 +6041,11 @@ qemuDomainObjStart(virConnectPtr conn,
if (ret > 0)
Should be "if (ret > 0) {"
VIR_WARN("Ignoring incomplete managed state %s", managed_save); - else + else {
and "} else {" to follow are coding style rules.
Oh, right. Fixed and pushed. Thank you guys. Michal
participants (4)
-
Eric Blake -
Jiri Denemark -
Michal Privoznik -
Peter Krempa