systemd: fix build without dbus

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@redhat.com>

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 26c098f18c120a8a575dafaee8c9fbc762e10ac3..3064037fcc88fbb2188fafb26bb8bb7ca451d3b9 100644 (file)
--- 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 a2eec83253d8b5dfb120031a49d699951877fd3a..528e93c20ea94b64cb0eb35445a1f79f8815f3df 100644 (file)
--- 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 d9665c1f7ff74ff1ed43ff011df6505539771afc..3522ae02e2bc256616ff8aaa68ad5a38b125f628 100644 (file)
--- 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 d0c7de29dcba20f61c8b3067c206c0f37a67c76a..e2b8d2b1bc1f98f038558552051d44fa885b27f9 100644 (file)
--- 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 f5d7f54fd84f3d13500a581aacca115d39b46fe8..9635c8212fdf60cd24556a8f87890c73e8913202 100644 (file)
--- 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 5c8578f7912ae70f68a37cecc10c3671f9947bc3..ad3a9464aedee453990565e57931ec702c221048 100644 (file)
--- 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 b5369127670ce79790bc6376b12b64d72b36e730..cd7afa53bcbc9ff31746e48d893cf2d95b83f567 100644 (file)
--- 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 3eea5c27004e1fac3057990d17abd55fab599e51..6de265be5918552b5534d603fbed38140a957db8 100644 (file)
--- 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 {