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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list