[libvirt] [PATCH] Improve error reporting in virsh

[John Levon]

On Thu, Feb 05, 2009 at 05:34:20PM +0000, Daniel P. Berrange wrote:

> Ok, how about a slightly different virSaveLastError() call, avoiding
> the need to do the pair of call virGetLastEror() + virCloneError()

Sounds reasonable to me.

> > > Also, it is neccessary to free the error message/object
> > > after reporting it to avoid a memory leak.
> > 
> > We're passing TRUE to vshError() so immediately exiting - there's no
> > place at which we can free it.
> 
> Doesn't this mean that virsh will exit whenever any libvirt command
> raises an error ? Not any difference it using it in single-command
> mode, but if using it interactively you don't want it to exit like
> this, just because you, or example, typoed with a domain name that
> doesn't exist

D'oh, quite right.

> > > > +#define virXendError(conn, codeval, fmt...)                                  \
> > > > +    do {                                                                     \
> > > > +        if (virGetLastError() == NULL) {                                     \
> > > > +            virReportErrorHelper(conn, VIR_FROM_XEND, codeval, __FILE__,     \
> > > > +                                 __FUNCTION__, __LINE__, fmt);               \
> > > > +        }                                                                    \
> > > > +    } while (0)
> > > > + 
> > > >  #define virXendErrorInt(conn, code, ival)                                    \
> > > > -        virXendError(conn, code, "%d", ival)
> > > > +    virXendError(conn, code, "%d", ival)
> > > 
> > > I don't like this change - we are trying to remove all driver specific
> > > error reporting macros.
> > 
> > Can you explain further? How do you expect to report driver-specific
> > issues without doing it at the point of the error? Why does my change
> > affect this?
> 
> 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.

Anyway, I'm not having much luck in arguing my case with you, but I
thought I'd explain this at least, because the code is broken and I
don't believe it's possible to fix it your way.

regards
john:w