[libvirt] [PATCH] Improve error reporting in virsh

[Daniel P. Berrange]

On Thu, Feb 05, 2009 at 06:09:10PM +0000, John Levon wrote:
> On Thu, Feb 05, 2009 at 05:34:20PM +0000, Daniel P. Berrange wrote:

> > Just have the virXendError macro call virReportErrorHelper  directly,
> > as it already does - there is no need to wrap it in the conditional
> > check 'if(virGetLastError() == NULL)' 
> 
> I'm trying to connect this comment to your other ones, but can't, so
> I'll have to attempt to guess what you're saying :)
> 
> I think you're just NAKing the entire change, with your reasoning being
> that the code must be fixed to only set an error once.  Unfortunately
> fixing code your way seems close to impossible. To give just one
> example, xenDaemonFormatSxprNet():
> 
> 5335         virXendError(conn, VIR_ERR_INTERNAL_ERROR,
> 5336                      _("unsupported network type %d"), def->type);
> 
> This part of it is "lowest level", so should always report an error. but
> it also calls virNetworkGetBridgeName() which *isn't*. This CAN report
> an error, but we immediately over-write it:
> 
> 5374         bridge = virNetworkGetBridgeName(network);
> 5375         virNetworkFree(network);
> 5376         if (!bridge) {
> 5377             virXendError(conn, VIR_ERR_NO_SOURCE, "%s",
> 5378                          def->data.network.name);
> 
> Also, there's at least two call stacks that reach this point:
> 
> xenDaemonDomainDefineXML()
>  xenDaemonFormatSxpr()    
>   xenDaemonFormatSxprNet()
> 
> xenDaemonAttachDevice()
>  xenDaemonFormatSxprNet()
> 
> In the second case, we are OK. In the first, we're already over-writing
> the error from xenDaemonFormatSxprNet().
> 
> Or for another example, it took just a couple of seconds looking at the
> xenDaemonDomainDefineXML() code paths to find one where
> virDomainDefParseString() would not set an error.
> 
> I do not think it's really possible to get things right the way you're
> suggesting. The fact that the code is broken everywhere backs me up
> pretty strongly here. How do you propose to reasonably test such
> changes?
> 
> In contrast, "only set an error if one isn't set already" is simple,
> immediately understandable, and covers almost all the common cases.

I'm not totally against this idea, but with a few caveats.

First, we'd need to make some changes to the libvirtd  because it calls
into a number of APIs, both public entry points & some of the internal
helpers directly, but does not reset the error in betweeen these calls.
Of course it also doesn't actually do anything with errors that these
calls may raise - they just happen to be printed on stderr. If we only
set when not already set, then most won't even appear on stderr in
the libvirtd context. Not sure of the best / least error prone way to 
fix this really. 

I'd still like to try and find a way to make the error raising process
more robust. The idea of having calls to virRaiseError, which may or
may not raise an error, depending on whether the thing we just called
remembered to raise an error is not a great situation to be in. 

As you point out it is pretty hard to get this right and/or identify
places where it is wrong. As a starting point, though adding code to
virRaiseError which prints a stack trace to stderr() whenever it is
called with an error already set, would help our awareness. Obviously
this would only want to be turned on in dev-builds.

Given my experiance with the OCAML/CIL stuff on threading, I think it
might also be practical to perform static analysis of the callpaths
and identify a fairly large proportion of flaws this way. I think it
would be possible to just track exit paths, and keep note of whether
there are any virRaiseError calls in each method, and match against
call locations. Ok, not easy, but I'll have a stab at this next time
I have a spare moment.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|