[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]