The virDBusMethodCall method has a DBusError as one of its parameters. If the caller wants to pass a non-NULL value for this, it immediately makes the calling code require DBus at build time. This has led to breakage of non-DBus builds several times. It is desirable that only the virdbus.c file should need WITH_DBUS conditionals, so we must ideally remove the DBusError parameter from the method. We can't simply raise a libvirt error, since the whole point of this parameter is to give the callers a way to check if the error is one they want to ignore, without having the logs polluted with an error message. So, we add a virErrorPtr parameter which the caller can then either ignore or raise using the new virReportErrorObject method. This new method is distinct from virSetError in that it ensures the logging hooks are run. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- po/POTFILES.in | 1 - src/libvirt_private.syms | 1 + src/util/virdbus.c | 31 ++++++++------ src/util/virdbus.h | 4 +- src/util/virerror.c | 104 +++++++++++++++++++++++++++++++++++++---------- src/util/virerror.h | 8 ++++ src/util/virfirewall.c | 29 +++---------- src/util/virsystemd.c | 15 ++++--- 8 files changed, 125 insertions(+), 68 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 26c098f..3064037 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -217,7 +217,6 @@ src/util/virstorageencryption.c src/util/virstoragefile.c src/util/virstring.c src/util/virsysinfo.c -src/util/virsystemd.c src/util/virerror.c src/util/virerror.h src/util/virtime.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a2eec83..528e93c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1284,6 +1284,7 @@ virErrorInitialize; virErrorSetErrnoFromLastError; virLastErrorIsSystemErrno; virRaiseErrorFull; +virRaiseErrorObject; virReportErrorHelper; virReportOOMErrorFull; virReportSystemErrorFull; diff --git a/src/util/virdbus.c b/src/util/virdbus.c index d9665c1..3522ae0 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -1522,7 +1522,7 @@ static int virDBusCall(DBusConnection *conn, DBusMessage *call, DBusMessage **replyout, - DBusError *error) + virErrorPtr error) { DBusMessage *reply = NULL; @@ -1530,8 +1530,9 @@ virDBusCall(DBusConnection *conn, int ret = -1; const char *iface, *member, *path, *dest; - if (!error) - dbus_error_init(&localerror); + dbus_error_init(&localerror); + if (error) + memset(error, 0, sizeof(*error)); iface = dbus_message_get_interface(call); member = dbus_message_get_member(call); @@ -1545,13 +1546,20 @@ virDBusCall(DBusConnection *conn, if (!(reply = dbus_connection_send_with_reply_and_block(conn, call, VIR_DBUS_METHOD_CALL_TIMEOUT_MILLIS, - error ? error : &localerror))) { + &localerror))) { PROBE(DBUS_METHOD_ERROR, "'%s.%s' on '%s' at '%s' error %s: %s", iface, member, path, dest, - error ? error->name : localerror.name, - error ? error->message : localerror.message); + localerror.name, + localerror.message); if (error) { + error->level = VIR_ERR_ERROR; + error->code = VIR_ERR_DBUS_SERVICE; + error->domain = VIR_FROM_DBUS; + if (VIR_STRDUP(error->message, localerror.message) < 0) + goto cleanup; + if (VIR_STRDUP(error->str1, localerror.name) < 0) + goto cleanup; ret = 0; } else { virReportError(VIR_ERR_DBUS_SERVICE, _("%s: %s"), member, @@ -1567,8 +1575,9 @@ virDBusCall(DBusConnection *conn, ret = 0; cleanup: - if (!error) - dbus_error_free(&localerror); + if (ret < 0 && error) + virResetError(error); + dbus_error_free(&localerror); if (reply) { if (ret == 0 && replyout) *replyout = reply; @@ -1616,7 +1625,7 @@ virDBusCall(DBusConnection *conn, */ int virDBusCallMethod(DBusConnection *conn, DBusMessage **replyout, - DBusError *error, + virErrorPtr error, const char *destination, const char *path, const char *iface, @@ -1634,8 +1643,6 @@ int virDBusCallMethod(DBusConnection *conn, if (ret < 0) goto cleanup; - ret = -1; - ret = virDBusCall(conn, call, replyout, error); cleanup: @@ -1832,7 +1839,7 @@ int virDBusCreateReply(DBusMessage **reply ATTRIBUTE_UNUSED, int virDBusCallMethod(DBusConnection *conn ATTRIBUTE_UNUSED, DBusMessage **reply ATTRIBUTE_UNUSED, - DBusError *error ATTRIBUTE_UNUSED, + virErrorPtr error ATTRIBUTE_UNUSED, const char *destination ATTRIBUTE_UNUSED, const char *path ATTRIBUTE_UNUSED, const char *iface ATTRIBUTE_UNUSED, diff --git a/src/util/virdbus.h b/src/util/virdbus.h index d0c7de2..e2b8d2b 100644 --- a/src/util/virdbus.h +++ b/src/util/virdbus.h @@ -28,7 +28,7 @@ # else # define DBusConnection void # define DBusMessage void -# define DBusError void +# define dbus_message_unref(m) do {} while (0) # endif # include "internal.h" @@ -62,7 +62,7 @@ int virDBusCreateReplyV(DBusMessage **reply, int virDBusCallMethod(DBusConnection *conn, DBusMessage **reply, - DBusError *error, + virErrorPtr error, const char *destination, const char *path, const char *iface, diff --git a/src/util/virerror.c b/src/util/virerror.c index f5d7f54..9635c82 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -618,6 +618,39 @@ virDispatchError(virConnectPtr conn) } +/* + * Reports an error through the logging subsystem + */ +static +void virRaiseErrorLog(const char *filename, + const char *funcname, + size_t linenr, + virErrorPtr err, + virLogMetadata *meta) +{ + int priority; + + /* + * Hook up the error or warning to the logging facility + */ + priority = virErrorLevelPriority(err->level); + if (virErrorLogPriorityFilter) + priority = virErrorLogPriorityFilter(err, priority); + + /* We don't want to pollute stderr if no logging outputs + * are explicitly requested by the user, since the default + * error function already pollutes stderr and most apps + * hate & thus disable that too. If the daemon has set + * a priority filter though, we should always forward + * all errors to the logging code. + */ + if (virLogGetNbOutputs() > 0 || + virErrorLogPriorityFilter) + virLogMessage(&virLogSelf, + priority, + filename, linenr, funcname, + meta, "%s", err->message); +} /** * virRaiseErrorFull: @@ -639,7 +672,7 @@ virDispatchError(virConnectPtr conn) * immediately if a callback is found and store it for later handling. */ void -virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED, +virRaiseErrorFull(const char *filename, const char *funcname, size_t linenr, int domain, @@ -655,7 +688,6 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED, int save_errno = errno; virErrorPtr to; char *str; - int priority; virLogMetadata meta[] = { { .key = "LIBVIRT_DOMAIN", .s = NULL, .iv = domain }, { .key = "LIBVIRT_CODE", .s = NULL, .iv = code }, @@ -709,30 +741,58 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED, to->int1 = int1; to->int2 = int2; - /* - * Hook up the error or warning to the logging facility - */ - priority = virErrorLevelPriority(level); - if (virErrorLogPriorityFilter) - priority = virErrorLogPriorityFilter(to, priority); - - /* We don't want to pollute stderr if no logging outputs - * are explicitly requested by the user, since the default - * error function already pollutes stderr and most apps - * hate & thus disable that too. If the daemon has set - * a priority filter though, we should always forward - * all errors to the logging code. - */ - if (virLogGetNbOutputs() > 0 || - virErrorLogPriorityFilter) - virLogMessage(&virLogSelf, - priority, - filename, linenr, funcname, - meta, "%s", str); + virRaiseErrorLog(filename, funcname, linenr, + to, meta); errno = save_errno; } + +/** + * virRaiseErrorObject: + * @filename: filename where error was raised + * @funcname: function name where error was raised + * @linenr: line number where error was raised + * @newerr: the error object to report + * + * Sets the thread local error object to be a copy of + * @newerr and logs the error + * + * This is like virRaiseErrorFull, except that it accepts the + * error information via a pre-filled virErrorPtr object + * + * This is like virSetError, except that it will trigger the + * logging callbacks. + * + * The caller must clear the @newerr instance afterwards, since + * it will be copied into the thread local error. + */ +void virRaiseErrorObject(const char *filename, + const char *funcname, + size_t linenr, + virErrorPtr newerr) +{ + int saved_errno = errno; + virErrorPtr err; + virLogMetadata meta[] = { + { .key = "LIBVIRT_DOMAIN", .s = NULL, .iv = newerr->domain }, + { .key = "LIBVIRT_CODE", .s = NULL, .iv = newerr->code }, + { .key = NULL }, + }; + + err = virLastErrorObject(); + if (!err) + goto cleanup; + + virResetError(err); + virCopyError(newerr, err); + virRaiseErrorLog(filename, funcname, linenr, + err, meta); + cleanup: + errno = saved_errno; +} + + /** * virErrorMsg: * @error: the virErrorNumber diff --git a/src/util/virerror.h b/src/util/virerror.h index 5c8578f..ad3a946 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -47,6 +47,11 @@ void virRaiseErrorFull(const char *filename, const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(12, 13); +void virRaiseErrorObject(const char *filename, + const char *funcname, + size_t linenr, + virErrorPtr err); + void virReportErrorHelper(int domcode, int errcode, const char *filename, const char *funcname, @@ -165,6 +170,9 @@ void virReportOOMErrorFull(int domcode, virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) +# define virReportErrorObject(obj) \ + virRaiseErrorObject(__FILE__, __FUNCTION__, __LINE__, obj) + int virSetError(virErrorPtr newerr); void virDispatchError(virConnectPtr conn); const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen); diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index b536912..cd7afa5 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -156,7 +156,6 @@ static int virFirewallValidateBackend(virFirewallBackend backend) { VIR_DEBUG("Validating backend %d", backend); -#if WITH_DBUS if (backend == VIR_FIREWALL_BACKEND_AUTOMATIC || backend == VIR_FIREWALL_BACKEND_FIREWALLD) { int rv = virDBusIsServiceRegistered(VIR_FIREWALL_FIREWALLD_SERVICE); @@ -180,16 +179,6 @@ virFirewallValidateBackend(virFirewallBackend backend) backend = VIR_FIREWALL_BACKEND_FIREWALLD; } } -#else - if (backend == VIR_FIREWALL_BACKEND_AUTOMATIC) { - VIR_DEBUG("DBus support disabled, trying direct backend"); - backend = VIR_FIREWALL_BACKEND_DIRECT; - } else if (backend == VIR_FIREWALL_BACKEND_FIREWALLD) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("firewalld firewall backend requested, but DBus support disabled")); - return -1; - } -#endif if (backend == VIR_FIREWALL_BACKEND_DIRECT) { const char *commands[] = { @@ -755,7 +744,6 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule, } -#ifdef WITH_DBUS static int virFirewallApplyRuleFirewallD(virFirewallRulePtr rule, bool ignoreErrors, @@ -764,13 +752,13 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule, const char *ipv = virFirewallLayerFirewallDTypeToString(rule->layer); DBusConnection *sysbus = virDBusGetSystemBus(); DBusMessage *reply = NULL; - DBusError error; + virError error; int ret = -1; if (!sysbus) return -1; - dbus_error_init(&error); + memset(&error, 0, sizeof(error)); if (!ipv) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -792,7 +780,7 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule, rule->args) < 0) goto cleanup; - if (dbus_error_is_set(&error)) { + if (error.level == VIR_ERR_ERROR) { /* * As of firewalld-0.3.9.3-1.fc20.noarch the name and * message fields in the error look like @@ -820,11 +808,9 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule, */ if (ignoreErrors) { VIR_DEBUG("Ignoring error '%s': '%s'", - error.name, error.message); + error.str1, error.message); } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to apply rule '%s'"), - error.message); + virReportErrorObject(&error); goto cleanup; } } else { @@ -835,12 +821,11 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule, ret = 0; cleanup: - dbus_error_free(&error); + virResetError(&error); if (reply) dbus_message_unref(reply); return ret; } -#endif static int virFirewallApplyRule(virFirewallPtr firewall, @@ -862,12 +847,10 @@ virFirewallApplyRule(virFirewallPtr firewall, if (virFirewallApplyRuleDirect(rule, ignoreErrors, &output) < 0) return -1; break; -#if WITH_DBUS case VIR_FIREWALL_BACKEND_FIREWALLD: if (virFirewallApplyRuleFirewallD(rule, ignoreErrors, &output) < 0) return -1; break; -#endif default: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unexpected firewall engine backend")); diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 3eea5c2..6de265b 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -252,8 +252,8 @@ int virSystemdCreateMachine(const char *name, VIR_DEBUG("Attempting to create machine via systemd"); if (virAtomicIntGet(&hasCreateWithNetwork)) { - DBusError error; - dbus_error_init(&error); + virError error; + memset(&error, 0, sizeof(error)); if (virDBusCallMethod(conn, NULL, @@ -280,21 +280,20 @@ int virSystemdCreateMachine(const char *name, "Before", "as", 1, "libvirt-guests.service") < 0) goto cleanup; - if (dbus_error_is_set(&error)) { + if (error.level == VIR_ERR_ERROR) { if (STREQ_NULLABLE("org.freedesktop.DBus.Error.UnknownMethod", - error.name)) { + error.str1)) { VIR_INFO("CreateMachineWithNetwork isn't supported, switching " "to legacy CreateMachine method for systemd-machined"); - dbus_error_free(&error); + virResetError(&error); virAtomicIntSet(&hasCreateWithNetwork, 0); /* Could re-structure without Using goto, but this * avoids another atomic read which would trigger * another memory barrier */ goto fallback; } - virReportError(VIR_ERR_DBUS_SERVICE, - _("CreateMachineWithNetwork: %s"), - error.message ? error.message : _("unknown error")); + virReportErrorObject(&error); + virResetError(&error); goto cleanup; } } else { -- 2.1.0