[libvirt] [PATCH v2 0/3] Misc error improvements
Collection of misc error improvements, see individual patches for details. Patch 2 and 3 have been ACK'd already. v2: Rebased to master Cole Robinson (3): virerror: Fix incorrect use of RaiseErrorFull virdbus: Remove redundant error macro virdbus: Show method name in error message src/util/virdbus.c | 12 +++--- src/util/virerror.h | 114 ++++++++++++++++------------------------------------ 2 files changed, 41 insertions(+), 85 deletions(-) -- 1.9.0
RaiseErrorFull does not prepend the static error code string (like INVALID_ARG yields "invalid arg: %(msg)s"). We should be using ReportErrorHelper. The generated error objects are slightly different, by not storing the invalid argument name in err->str2. However those fields aren't used anywhere else and aren't documented to contain anything useful, so I don't think it matters. Cc: Daniel P. Berrange <berrange@redhat.com> --- Dan, can you ACK? Eric and Guido requested your explicit review: http://www.redhat.com/archives/libvir-list/2014-May/msg00118.html http://www.redhat.com/archives/libvir-list/2014-May/msg00150.html src/util/virerror.h | 116 +++++++++++++++++----------------------------------- 1 file changed, 38 insertions(+), 78 deletions(-) diff --git a/src/util/virerror.h b/src/util/virerror.h index fe0e15e..872c270 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -69,92 +69,52 @@ void virReportSystemErrorFull(int domcode, (fmt), __VA_ARGS__) # define virReportInvalidNullArg(argname) \ - virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ - VIR_FROM_THIS, \ - VIR_ERR_INVALID_ARG, \ - VIR_ERR_ERROR, \ - __FUNCTION__, \ - #argname, \ - NULL, \ - 0, 0, \ - _("%s in %s must be NULL"), \ - #argname, __FUNCTION__) + virReportErrorHelper(VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + __FILE__, __FUNCTION__, __LINE__, \ + _("%s in %s must be NULL"), \ + #argname, __FUNCTION__) # define virReportInvalidNonNullArg(argname) \ - virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ - VIR_FROM_THIS, \ - VIR_ERR_INVALID_ARG, \ - VIR_ERR_ERROR, \ - __FUNCTION__, \ - #argname, \ - NULL, \ - 0, 0, \ - _("%s in %s must not be NULL"), \ - #argname, __FUNCTION__) + virReportErrorHelper(VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + __FILE__, __FUNCTION__, __LINE__, \ + _("%s in %s must not be NULL"), \ + #argname, __FUNCTION__) # define virReportInvalidPositiveArg(argname) \ - virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ - VIR_FROM_THIS, \ - VIR_ERR_INVALID_ARG, \ - VIR_ERR_ERROR, \ - __FUNCTION__, \ - #argname, \ - NULL, \ - 0, 0, \ - _("%s in %s must be greater than zero"), \ - #argname, __FUNCTION__) + virReportErrorHelper(VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + __FILE__, __FUNCTION__, __LINE__, \ + _("%s in %s must be greater than zero"), \ + #argname, __FUNCTION__) # define virReportInvalidNonZeroArg(argname) \ - virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ - VIR_FROM_THIS, \ - VIR_ERR_INVALID_ARG, \ - VIR_ERR_ERROR, \ - __FUNCTION__, \ - #argname, \ - NULL, \ - 0, 0, \ - _("%s in %s must not be zero"), \ - #argname, __FUNCTION__) + virReportErrorHelper(VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + __FILE__, __FUNCTION__, __LINE__, \ + _("%s in %s must not be zero"), \ + #argname, __FUNCTION__) # define virReportInvalidZeroArg(argname) \ - virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ - VIR_FROM_THIS, \ - VIR_ERR_INVALID_ARG, \ - VIR_ERR_ERROR, \ - __FUNCTION__, \ - #argname, \ - NULL, \ - 0, 0, \ - _("%s in %s must be zero"), \ - #argname, __FUNCTION__) + virReportErrorHelper(VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + __FILE__, __FUNCTION__, __LINE__, \ + _("%s in %s must be zero"), \ + #argname, __FUNCTION__) # define virReportInvalidNonNegativeArg(argname) \ - virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ - VIR_FROM_THIS, \ - VIR_ERR_INVALID_ARG, \ - VIR_ERR_ERROR, \ - __FUNCTION__, \ - #argname, \ - NULL, \ - 0, 0, \ - _("%s in %s must be zero or greater"), \ - #argname, __FUNCTION__) + virReportErrorHelper(VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + __FILE__, __FUNCTION__, __LINE__, \ + _("%s in %s must be zero or greater"), \ + #argname, __FUNCTION__) # define virReportInvalidArg(argname, fmt, ...) \ - virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ - VIR_FROM_THIS, \ - VIR_ERR_INVALID_ARG, \ - VIR_ERR_ERROR, \ - __FUNCTION__, \ - #argname, \ - NULL, \ - 0, 0, \ - (fmt), __VA_ARGS__) + virReportErrorHelper(VIR_FROM_THIS, \ + VIR_ERR_INVALID_ARG, \ + __FILE__, __FUNCTION__, __LINE__, \ + (fmt), __VA_ARGS__) # define virReportDBusServiceError(message, name) \ - virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ - VIR_FROM_THIS, \ - VIR_ERR_DBUS_SERVICE, \ - VIR_ERR_ERROR, \ - __FUNCTION__, \ - name, \ - NULL, \ - 0, 0, \ - "%s", message); + virReportErrorHelper(VIR_FROM_THIS, \ + VIR_ERR_DBUS_SERVICE, \ + __FILE__, __FUNCTION__, __LINE__, \ + "%s", message) # define virReportUnsupportedError() \ virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_NO_SUPPORT, \ -- 1.9.0
On Mon, May 19, 2014 at 06:02:26PM -0400, Cole Robinson wrote:
RaiseErrorFull does not prepend the static error code string (like INVALID_ARG yields "invalid arg: %(msg)s"). We should be using ReportErrorHelper.
The generated error objects are slightly different, by not storing the invalid argument name in err->str2. However those fields aren't used anywhere else and aren't documented to contain anything useful, so I don't think it matters.
This was actually intentional when I created these helpers, since IMHO the string concatenation between the caller supplied error message and the virErrorMsg funtion leads to fugly error messages. So the intent was that these helper macros would provide the complete error message that's relevant to their usage, and ignore virErrorMsg. IOW, I don't think adding a 'invalid argument: ' prefix to these helper macros is particularly useful. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/21/2014 07:07 AM, Daniel P. Berrange wrote:
On Mon, May 19, 2014 at 06:02:26PM -0400, Cole Robinson wrote:
RaiseErrorFull does not prepend the static error code string (like INVALID_ARG yields "invalid arg: %(msg)s"). We should be using ReportErrorHelper.
The generated error objects are slightly different, by not storing the invalid argument name in err->str2. However those fields aren't used anywhere else and aren't documented to contain anything useful, so I don't think it matters.
This was actually intentional when I created these helpers, since IMHO the string concatenation between the caller supplied error message and the virErrorMsg funtion leads to fugly error messages. So the intent was that these helper macros would provide the complete error message that's relevant to their usage, and ignore virErrorMsg.
IOW, I don't think adding a 'invalid argument: ' prefix to these helper macros is particularly useful.
Fair enough. I rebased the two other ACKd patches and pushed. Thanks, Cole
This is the only callsite. We drop use of localerror.name here, because it's not actually useful to us: rather than the parameter name which received an invalid value (which was assumed), it's actually the the dbus errno equivalent. Just use the string. Acked-by: Eric Blake <eblake@redhat.com> --- src/util/virdbus.c | 7 ++++--- src/util/virerror.h | 6 ------ 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index 709d6ee..03ec028 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1423,9 +1423,10 @@ virDBusCall(DBusConnection *conn, error ? error : &localerror))) { if (error) ret = 0; - else - virReportDBusServiceError(localerror.message ? localerror.message : "unknown error", - localerror.name); + else { + virReportError(VIR_ERR_DBUS_SERVICE, "%s", + localerror.message ? localerror.message : _("unknown error")); + } goto cleanup; } diff --git a/src/util/virerror.h b/src/util/virerror.h index 872c270..4b01b77 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -110,12 +110,6 @@ void virReportSystemErrorFull(int domcode, __FILE__, __FUNCTION__, __LINE__, \ (fmt), __VA_ARGS__) -# define virReportDBusServiceError(message, name) \ - virReportErrorHelper(VIR_FROM_THIS, \ - VIR_ERR_DBUS_SERVICE, \ - __FILE__, __FUNCTION__, __LINE__, \ - "%s", message) - # define virReportUnsupportedError() \ virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_NO_SUPPORT, \ __FILE__, __FUNCTION__, __LINE__, __FUNCTION__) -- 1.9.0
If you trigger bug 1033369, we get the error message: error from service: Invalid argument Which is a bit too generic to pinpoint what is actually failing. This changes it to: error from service: CreateMachine: Invalid argument Acked-by: Eric Blake <eblake@redhat.com> --- src/util/virdbus.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index 03ec028..31251fe 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1408,7 +1408,8 @@ static int virDBusCall(DBusConnection *conn, DBusMessage *call, DBusMessage **replyout, - DBusError *error) + DBusError *error, + const char *member) { DBusMessage *reply = NULL; DBusError localerror; @@ -1424,7 +1425,7 @@ virDBusCall(DBusConnection *conn, if (error) ret = 0; else { - virReportError(VIR_ERR_DBUS_SERVICE, "%s", + virReportError(VIR_ERR_DBUS_SERVICE, _("%s: %s"), member, localerror.message ? localerror.message : _("unknown error")); } goto cleanup; @@ -1502,7 +1503,7 @@ int virDBusCallMethod(DBusConnection *conn, ret = -1; - ret = virDBusCall(conn, call, replyout, error); + ret = virDBusCall(conn, call, replyout, error, member); cleanup: if (call) -- 1.9.0
participants (2)
-
Cole Robinson -
Daniel P. Berrange