Re: [libvirt] [PATCH] Improve error reporting in virsh
On Thu, Feb 05, 2009 at 05:05:53PM +0000, John Levon wrote:
On Thu, Feb 05, 2009 at 12:34:32PM +0000, Daniel P. Berrange wrote:
> > +static void
> > +virshReportError(vshControl *ctl)
> > +{
> > + if (last_error == NULL)
> > + return;
> > +
> > + if (last_error->code == VIR_ERR_OK) {
> > + vshError(ctl, TRUE, "%s", _("unknown error"));
> > + return;
> > + }
> > +
> > + vshError(ctl, TRUE, "%s", last_error->message);
> > }
>
>
> Since you only ever print out the 'last_error->message'
> field here, I think its better to just do strdup() of
> that field, instead of adding a new virCloneError API.
Very much disagree. That happens to be true in virsh's case, but it's
not in general. Currently there's no way to save off a virErrorPtr, and
that's a significant hole in the API
Ok, how about a slightly different virSaveLastError() call, avoiding
the need to do the pair of call virGetLastEror() + virCloneError()
> 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
> > +#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)'
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 :|