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. --- 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, \ --

On Sat, May 03, 2014 at 03:59:40PM -0400, Cole Robinson wrote: > --- > src/libvirt_private.syms | 1 - > src/util/virdbus.c | 19 +++++-------------- > src/util/virdbus.h | 4 ---- > 3 files changed, 5 insertions(+), 19 deletions(-) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index cc3ae2c..8a015eb 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1148,7 +1148,6 @@ virCryptoHashString; > > > # util/virdbus.h > -virDBusCall; > virDBusCallMethod; > virDBusCloseSystemBus; > virDBusCreateMethod; > diff --git a/src/util/virdbus.c b/src/util/virdbus.c > index 4217aea..709d6ee 100644 > --- a/src/util/virdbus.c > +++ b/src/util/virdbus.c > @@ -1404,10 +1404,11 @@ int virDBusCreateReply(DBusMessage **reply, > * > * Returns 0 on success, or -1 upon error > */ > -int virDBusCall(DBusConnection *conn, > - DBusMessage *call, > - DBusMessage **replyout, > - DBusError *error) > +static int > +virDBusCall(DBusConnection *conn, > + DBusMessage *call, > + DBusMessage **replyout, > + DBusError *error) > { > DBusMessage *reply = NULL; > DBusError localerror; > @@ -1694,16 +1695,6 @@ int virDBusCreateReply(DBusMessage **reply ATTRIBUTE_UNUSED, > return -1; > } > > -int virDBusCall(DBusConnection *conn ATTRIBUTE_UNUSED, > - DBusMessage *call ATTRIBUTE_UNUSED, > - DBusMessage **reply ATTRIBUTE_UNUSED, > - DBusError *error ATTRIBUTE_UNUSED) > -{ > - virReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("DBus support not compiled into this binary")); > - return -1; > -} > - > int virDBusCallMethod(DBusConnection *conn ATTRIBUTE_UNUSED, > DBusMessage **reply ATTRIBUTE_UNUSED, > DBusError *error ATTRIBUTE_UNUSED, > diff --git a/src/util/virdbus.h b/src/util/virdbus.h > index a4987b8..d0c7de2 100644 > --- a/src/util/virdbus.h > +++ b/src/util/virdbus.h > @@ -68,10 +68,6 @@ int virDBusCallMethod(DBusConnection *conn, > const char *iface, > const char *member, > const char *types, ...); > -int virDBusCall(DBusConnection *conn, > - DBusMessage *call, > - DBusMessage **reply, > - DBusError *error); > int virDBusMessageRead(DBusMessage *msg, > const char *types, ...); > void virDBusMessageUnref(DBusMessage *msg); > -- Ack. Outside consumers use the higher level virDBusCallMethod. -- Guido

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. --- 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")); + }

On Sat, May 03, 2014 at 03:59:38PM -0400, Cole Robinson wrote: > --- > src/util/virerror.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/util/virerror.h b/src/util/virerror.h > index 2de04f4..fe0e15e 100644 > --- a/src/util/virerror.h > +++ b/src/util/virerror.h > @@ -99,7 +99,7 @@ void virReportSystemErrorFull(int domcode, > #argname, \ > NULL, \ > 0, 0, \ > - _("%s in %s must greater than zero"), \ > + _("%s in %s must be greater than zero"), \ > #argname, __FUNCTION__) > # define virReportInvalidNonZeroArg(argname) \ > virRaiseErrorFull(__FILE__, __FUNCTION__, __LINE__, \ > -- ACK. -- Guido