Re: [libvirt] [PATCH 4/8] Remove all uses of virshReportError except "vshError".
2010/7/8 Chris Lalancette <clalance(a)redhat.com>:
With the current virsh code, error reporting is a bit fragile
because the reporting is decoupled from the place where the
error occurred. There are two places where this can be a problem:
1) In utility functions that don't properly dispatch errors
like the main API functions do. In this case, the error from
the utility function initially gets stored, but then may get
blown away during a cleanup function.
2) If a cleanup function has an additional error. In this case,
what will happen is that we will report the error from the cleanup
function, not the original error, which may make the problem
harder to diagnose.
What this patch does is to report errors exactly at the point
that they occurred, in vshError(). That way the individual
cmd* functions can do whatever cleanup is necessary without
fear of blowing away the appropriate error.
So this patch changes the internal handling but should not affect the
actual error output of virsh? I'm asking because we are losing error
messages this way with this patch applied.
With this in place, it's now the responsibility of the
individual cmd* functions to use vshError when an error
occurs. However, this should make error reporting more consistent.
Signed-off-by: Chris Lalancette <clalance(a)redhat.com>
---
tools/virsh.c | 22 +++++-----------------
1 files changed, 5 insertions(+), 17 deletions(-)
@@ -8727,11 +8722,6 @@ cmdSnapshotCreate(vshControl *ctl, const
vshCmd *cmd)
buffer = strdup("<domainsnapshot/>");
else {
if (virFileReadAll(from, VIRSH_MAX_XML_FILE, &buffer) < 0) {
- /* we have to report the error here because during cleanup
- * we'll run through virDomainFree(), which loses the
- * last error
- */
- virshReportError(ctl);
For example this one. I tested it with and without this patch.
Without this patch we get
virsh # snapshot-create test1 this-file-does-not-exist.xml
error: Failed to open file 'this-file-does-not-exist.xml': No such
file or directory
virsh #
With this patch applied we get
virsh # snapshot-create test1 this-file-does-not-exist.xml
virsh #
Making it look like it succeeded. I assume it's the same pattern in
all places where you removed the call to virshReportError on
virFileReadAll failure.
Ah, okay. Now I see that you fixed this in patch 6/8. Now the error is
more verbose, but that's okay I think.
virsh # snapshot-create test1 this-file-does-not-exist.xml
error: Failed to read contents of 'this-file-does-not-exist.xml'
error: Failed to open file 'this-file-does-not-exist.xml': No such
file or directory
virsh #
Maybe you should merge patch 4 and 6 into one. That way error
reporting for virFileReadAll isn't broken in between those two
commits.
ACK.
Matthias