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

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

> > +#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?

> If some part of the Xen driver is overwriting 
> an existing error message during the failure path, either that is 
> explicit, or a mistake that needs to be fixed.

I just fixed this to be like the other places where we do this? They're
all bugs too?

regards
john


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