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