[libvirt] [PATCH] Better error reporting in virsh.
When hitting failures in virsh, a common idiom is to jump to a cleanup label, free some resources, and then return a FALSE error code to vshCommandRun. In theory, vshCommandRun is then supposed to print out the last error. The problem is that many of the cleanup paths have library calls to free resources, and all of those library calls clear out the last error. This is leading to situations where no error is being reported at all. This patch remedies the situation somewhat by printing out the errors inside the command methods themselves when we know it will go through a cleanup path that will lose the error. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- tools/virsh.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d241fa0..aaae7ca 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2623,9 +2623,8 @@ cmdDomXMLFromNative(vshControl *ctl, const vshCmd *cmd) format = vshCommandOptString(cmd, "format", NULL); configFile = vshCommandOptString(cmd, "config", NULL); - if (virFileReadAll(configFile, 1024*1024, &configData) < 0) { + if (virFileReadAll(configFile, 1024*1024, &configData) < 0) return FALSE; - } xmlData = virConnectDomainXMLFromNative(ctl->conn, format, configData, flags); if (xmlData != NULL) { @@ -2669,9 +2668,8 @@ cmdDomXMLToNative(vshControl *ctl, const vshCmd *cmd) format = vshCommandOptString(cmd, "format", NULL); xmlFile = vshCommandOptString(cmd, "xml", NULL); - if (virFileReadAll(xmlFile, 1024*1024, &xmlData) < 0) { + if (virFileReadAll(xmlFile, 1024*1024, &xmlData) < 0) return FALSE; - } configData = virConnectDomainXMLToNative(ctl->conn, format, xmlData, flags); if (configData != NULL) { @@ -4330,9 +4328,8 @@ cmdNodeDeviceCreate(vshControl *ctl, const vshCmd *cmd) return FALSE; } - if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { + if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) return FALSE; - } dev = virNodeDeviceCreateXML(ctl->conn, buffer, 0); VIR_FREE(buffer); @@ -5427,6 +5424,7 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd) } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { + virshReportError(ctl); virStoragePoolFree(pool); return FALSE; } @@ -5488,6 +5486,7 @@ cmdVolCreateFrom(vshControl *ctl, const vshCmd *cmd) goto cleanup; if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { + virshReportError(ctl); goto cleanup; } @@ -6928,6 +6927,7 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd) } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { + virshReportError(ctl); virDomainFree(dom); return FALSE; } @@ -6995,6 +6995,7 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { + virshReportError(ctl); virDomainFree(dom); return FALSE; } @@ -7062,6 +7063,7 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) } if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { + virshReportError(ctl); virDomainFree(dom); return FALSE; } -- 1.6.6.1
On 04/05/2010 11:37 AM, Chris Lalancette wrote:
This patch remedies the situation somewhat by printing out the errors inside the command methods themselves when we know it will go through a cleanup path that will lose the error.
Sounds reasonable for purposes of better reporting, but it also sounds a bit fragile - is it always easy to tell what might be clearing the error later? Is this something where we might inadvertently break things by rewriting a cleanup: label, and end up with duplicate messages down the road?
@@ -2623,9 +2623,8 @@ cmdDomXMLFromNative(vshControl *ctl, const vshCmd *cmd) format = vshCommandOptString(cmd, "format", NULL); configFile = vshCommandOptString(cmd, "config", NULL);
- if (virFileReadAll(configFile, 1024*1024, &configData) < 0) { + if (virFileReadAll(configFile, 1024*1024, &configData) < 0) return FALSE; - }
Why the unrelated no-op change here, and in the next couple of hunks?
@@ -5427,6 +5424,7 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd) }
if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { + virshReportError(ctl); virStoragePoolFree(pool); return FALSE;
Should we add comments that remind us that we know that virStoragePoolFree will clear any errors? Likewise for the other changes in this file? At any rate, ACK that it improves the virsh experience. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/05/2010 05:20 PM, Eric Blake wrote:
On 04/05/2010 11:37 AM, Chris Lalancette wrote:
This patch remedies the situation somewhat by printing out the errors inside the command methods themselves when we know it will go through a cleanup path that will lose the error.
Sounds reasonable for purposes of better reporting, but it also sounds a bit fragile - is it always easy to tell what might be clearing the error later? Is this something where we might inadvertently break things by rewriting a cleanup: label, and end up with duplicate messages down the road?
Yeah, I agree it is fragile. We need to come up with a better way to do this. Maybe in the error path we can save any "previous" error before we call the cleanup functions, and then restore it afterwards? I'm not sure. In any case I think it's a separate patch.
@@ -2623,9 +2623,8 @@ cmdDomXMLFromNative(vshControl *ctl, const vshCmd *cmd) format = vshCommandOptString(cmd, "format", NULL); configFile = vshCommandOptString(cmd, "config", NULL);
- if (virFileReadAll(configFile, 1024*1024, &configData) < 0) { + if (virFileReadAll(configFile, 1024*1024, &configData) < 0) return FALSE; - }
Why the unrelated no-op change here, and in the next couple of hunks?
I was just auditing all of the virFileReadAll uses, and I saw that these didn't conform to the "usual" style in libvirt. I just fixed them at the same time.
@@ -5427,6 +5424,7 @@ cmdVolCreate(vshControl *ctl, const vshCmd *cmd) }
if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) { + virshReportError(ctl); virStoragePoolFree(pool); return FALSE;
Should we add comments that remind us that we know that virStoragePoolFree will clear any errors? Likewise for the other changes in this file?
Yeah, again, this whole thing is pretty fragile. We'll come up with a better solution.
At any rate, ACK that it improves the virsh experience.
Thanks. I've pushed this patch as-is, and we'll do further improvements in additional patches. -- Chris Lalancette
participants (2)
-
Chris Lalancette -
Eric Blake