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