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