9:06 a.m.
From: "Jason J. Herne" <jjherne(a)us.ibm.com>
qemuDomainObjStart is checking the return code from qemuDomainObjRestore for
errors even after determining that the return code is 0. This causes the
following error message to appear even when the restore was successful.
Unable to restore from managed state [path]. Maybe the file is corrupted?
A simple conditional to handle the error case takes care of the problem.
Signed-off-by: Jason J. Herne <jjherne(a)us.ibm.com>
---
src/qemu/qemu_driver.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2b852eb..cec2b6c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6081,14 +6081,15 @@ qemuDomainObjStart(virConnectPtr conn,
else
vm->hasManagedSave = false;
}
-
- if (ret > 0) {
- VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
- } else {
- VIR_WARN("Unable to restore from managed state %s. "
- "Maybe the file is corrupted?", managed_save);
- goto cleanup;
+ else {
+ if (ret > 0) {
+ VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
+ } else {
+ VIR_WARN("Unable to restore from managed state %s. "
+ "Maybe the file is corrupted?", managed_save);
+ }
}
+ goto cleanup;
}
}
--
1.8.3.2
6:24 p.m.
On 05/27/2014 08:06 AM, Jason J. Herne wrote:
From: "Jason J. Herne" <jjherne(a)us.ibm.com>
qemuDomainObjStart is checking the return code from qemuDomainObjRestore for
errors even after determining that the return code is 0. This causes the
following error message to appear even when the restore was successful.
Unable to restore from managed state [path]. Maybe the file is corrupted?
A simple conditional to handle the error case takes care of the problem.
Signed-off-by: Jason J. Herne <jjherne(a)us.ibm.com>
---
src/qemu/qemu_driver.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2b852eb..cec2b6c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6081,14 +6081,15 @@ qemuDomainObjStart(virConnectPtr conn,
else
vm->hasManagedSave = false;
}
-
- if (ret > 0) {
- VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
- } else {
- VIR_WARN("Unable to restore from managed state %s. "
- "Maybe the file is corrupted?", managed_save);
- goto cleanup;
+ else {
Wrong coding style; the else should appear next to the } of the if.
+ if (ret > 0) {
+ VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
+ } else {
+ VIR_WARN("Unable to restore from managed state %s. "
+ "Maybe the file is corrupted?", managed_save);
+ }
}
+ goto cleanup;
This goto is placed wrong; it causes us to skip starting the domain even
when loading managed state was successful.
But you are right that the logic is screwy here. Does this diff look
better?
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index f008763..bae70ff 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -6080,9 +6080,7 @@ qemuDomainObjStart(virConnectPtr conn,
VIR_WARN("Failed to remove the managed state %s",
managed_save);
else
vm->hasManagedSave = false;
- }
-
- if (ret > 0) {
+ } else if (ret > 0) {
VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
} else {
VIR_WARN("Unable to restore from managed state %s. "
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6:28 p.m.
On 05/27/2014 05:24 PM, Eric Blake wrote:
On 05/27/2014 08:06 AM, Jason J. Herne wrote:
> From: "Jason J. Herne" <jjherne(a)us.ibm.com>
>
> qemuDomainObjStart is checking the return code from qemuDomainObjRestore for
> errors even after determining that the return code is 0. This causes the
> following error message to appear even when the restore was successful.
>
> + if (ret > 0) {
> + VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
> + } else {
> + VIR_WARN("Unable to restore from managed state %s. "
> + "Maybe the file is corrupted?",
managed_save);
> + }
> }
> + goto cleanup;
This goto is placed wrong; it causes us to skip starting the domain even
when loading managed state was successful.
Actually, it looks like we WANT to goto cleanup for ret == 0, and that
the real problem is that commit cfc28c66 botched up for being noisy on
success. Maybe the fix we want is:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index f008763..86190ff 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -6085,8 +6085,9 @@ qemuDomainObjStart(virConnectPtr conn,
if (ret > 0) {
VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
} else {
- VIR_WARN("Unable to restore from managed state %s. "
- "Maybe the file is corrupted?", managed_save);
+ if (ret < 0)
+ VIR_WARN("Unable to restore from managed state %s. "
+ "Maybe the file is corrupted?", managed_save);
goto cleanup;
}
}
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:45 a.m.
On 05/28/14 01:28, Eric Blake wrote:
On 05/27/2014 05:24 PM, Eric Blake wrote:
> On 05/27/2014 08:06 AM, Jason J. Herne wrote:
>> From: "Jason J. Herne" <jjherne(a)us.ibm.com>
>>
>> qemuDomainObjStart is checking the return code from qemuDomainObjRestore for
>> errors even after determining that the return code is 0. This causes the
>> following error message to appear even when the restore was successful.
>>
>
>> + if (ret > 0) {
>> + VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
>> + } else {
>> + VIR_WARN("Unable to restore from managed state %s.
"
>> + "Maybe the file is corrupted?",
managed_save);
>> + }
>> }
>> + goto cleanup;
>
> This goto is placed wrong; it causes us to skip starting the domain even
> when loading managed state was successful.
Actually, it looks like we WANT to goto cleanup for ret == 0, and that
Indeed.
the real problem is that commit cfc28c66 botched up for being noisy
on
success. Maybe the fix we want is:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index f008763..86190ff 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -6085,8 +6085,9 @@ qemuDomainObjStart(virConnectPtr conn,
if (ret > 0) {
VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
} else {
- VIR_WARN("Unable to restore from managed state %s. "
- "Maybe the file is corrupted?", managed_save);
+ if (ret < 0)
+ VIR_WARN("Unable to restore from managed state %s. "
+ "Maybe the file is corrupted?", managed_save);
goto cleanup;
}
}
But this patch isn't ideal and makes the logic in the code even more entangled.
qemuDomainObjRestore returns 1 on corrupted image that was removed, 0 on sucess
and -1 on other errors. The condition right above that hunk tests success case.
We should connect this failure case condition to the else section of that
condition so that we don't make it even weirder.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c6f0b46..03b5a5e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6080,14 +6080,14 @@ qemuDomainObjStart(virConnectPtr conn,
VIR_WARN("Failed to remove the managed state %s",
managed_save);
else
vm->hasManagedSave = false;
- }
- if (ret > 0) {
- VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
- } else {
+ goto cleanup;
+ } else if (ret < 0) {
VIR_WARN("Unable to restore from managed state %s. "
"Maybe the file is corrupted?", managed_save);
goto cleanup;
+ } else {
+ VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
}
}
}
Peter
9:43 a.m.
On 05/28/2014 02:45 AM, Peter Krempa wrote:
> }
But this patch isn't ideal and makes the logic in the code even more entangled.
qemuDomainObjRestore returns 1 on corrupted image that was removed, 0 on sucess
and -1 on other errors. The condition right above that hunk tests success case.
We should connect this failure case condition to the else section of that
condition so that we don't make it even weirder.
Your approach is even more legible; consider this a pre-approved ACK if
you turn it into a formal commit and apply it in time for 1.2.5.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c6f0b46..03b5a5e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6080,14 +6080,14 @@ qemuDomainObjStart(virConnectPtr conn,
VIR_WARN("Failed to remove the managed state %s",
managed_save);
else
vm->hasManagedSave = false;
- }
- if (ret > 0) {
- VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
- } else {
+ goto cleanup;
+ } else if (ret < 0) {
VIR_WARN("Unable to restore from managed state %s. "
"Maybe the file is corrupted?", managed_save);
goto cleanup;
+ } else {
+ VIR_WARN("Ignoring incomplete managed state %s",
managed_save);
}
}
}
Peter
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:32 p.m.
On 05/28/14 16:43, Eric Blake wrote:
On 05/28/2014 02:45 AM, Peter Krempa wrote:
>> }
>
> But this patch isn't ideal and makes the logic in the code even more entangled.
> qemuDomainObjRestore returns 1 on corrupted image that was removed, 0 on sucess
> and -1 on other errors. The condition right above that hunk tests success case.
> We should connect this failure case condition to the else section of that
> condition so that we don't make it even weirder.
Your approach is even more legible; consider this a pre-approved ACK if
you turn it into a formal commit and apply it in time for 1.2.5.
The fix is pushed now.
Peter
3822
days inactive
3823
days old
5 comments
3 participants
participants (3)
-
Eric Blake -
Jason J. Herne -
Peter Krempa