[libvirt] [PATCH 4/8] Remove all uses of virshReportError except "vshError".

[Matthias Bolte]

2010/7/8 Chris Lalancette <clalance at 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 at 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