Re: [libvirt] [PATCH] Improve error reporting in virsh
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