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>

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

src/libvirt_private.syms

@@ -1284,6 +1284,7 @@ virErrorInitialize;
 virErrorSetErrnoFromLastError;
 virLastErrorIsSystemErrno;
 virRaiseErrorFull;
+virRaiseErrorObject;
 virReportErrorHelper;
 virReportOOMErrorFull;
 virReportSystemErrorFull;

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,
+              localerror.name,
-              error ? error->message : localerror.message);
+              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)
+    if (ret < 0 && error)
-        dbus_error_free(&localerror);
+        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,

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,

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;
 
-    /*
+    virRaiseErrorLog(filename, funcname, linenr,
-     * Hook up the error or warning to the logging facility
+                     to, meta);
-     */
-    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);
 
     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

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);

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,
+            virReportErrorObject(&error);
-                           _("Unable to apply rule '%s'"),
-                           error.message);
             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"));

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;
+        virError error;
-        dbus_error_init(&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,
+            virReportErrorObject(&error);
-                           _("CreateMachineWithNetwork: %s"),
+            virResetError(&error);
-                           error.message ? error.message : _("unknown error"));
             goto cleanup;
         }
     } else {