[libvirt] [PATCH v2] systemd: fix build without dbus

Pavel Hrdina phrdina at redhat.com
Wed Jan 21 13:58:27 UTC 2015


On 01/19/2015 02:36 PM, Daniel P. Berrange wrote:
> 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 virSetError.
>
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>   src/util/virdbus.c     | 31 +++++++++++++++++++------------
>   src/util/virdbus.h     |  4 ++--
>   src/util/virfirewall.c | 29 ++++++-----------------------
>   src/util/virsystemd.c  | 15 +++++++--------
>   4 files changed, 34 insertions(+), 45 deletions(-)
>
> In v2:
>
>   - Removed left over debug statements
>
> 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/virfirewall.c b/src/util/virfirewall.c
> index b536912..2fa7f65 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);
> +            virSetError(&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..0b71b26 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"));
> +            virSetError(&error);
> +            virResetError(&error);
>               goto cleanup;
>           }
>       } else {
>

ACK with this squashed in:

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

Pavel




More information about the libvir-list mailing list