9:34 p.m.
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(a)redhat.com>
---
src/util/virdbus.c | 31 +++++++++++++++++++------------
src/util/virdbus.h | 4 ++--
src/util/virfirewall.c | 34 ++++++++++------------------------
src/util/virsystemd.c | 15 +++++++--------
4 files changed, 38 insertions(+), 46 deletions(-)
This is a build-breaker fix, but I'm not pushing since the
fix is too complicated to go in without some review.
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..a01b703 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,
@@ -789,10 +777,13 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule,
"sa&s",
ipv,
(int)rule->argsLen,
- rule->args) < 0)
+ rule->args) < 0) {
+ VIR_ERROR("Here fail");
goto cleanup;
+ }
- if (dbus_error_is_set(&error)) {
+ VIR_ERROR("Error %d: %s\n", error.level, error.message);
+ 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 +811,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 +824,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 +850,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 {
--
2.1.0
10:13 p.m.
On Mon, Jan 19, 2015 at 12:34:52PM +0000, 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(a)redhat.com>
---
src/util/virdbus.c | 31 +++++++++++++++++++------------
src/util/virdbus.h | 4 ++--
src/util/virfirewall.c | 34 ++++++++++------------------------
src/util/virsystemd.c | 15 +++++++--------
4 files changed, 38 insertions(+), 46 deletions(-)
This is a build-breaker fix, but I'm not pushing since the
fix is too complicated to go in without some review.
[...]
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
Using virError instead of DBusError is fine, but
+# define dbus_message_unref(m) do {} while (0)
ewww, can't we just cleanly separate DBus code from libvirt code
instead of adding more of these?
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
index b536912..a01b703 100644
--- a/src/util/virfirewall.c
+++ b/src/util/virfirewall.c
[...]
@@ -789,10 +777,13 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr
rule,
"sa&s",
ipv,
(int)rule->argsLen,
- rule->args) < 0)
+ rule->args) < 0) {
+ VIR_ERROR("Here fail");
Leftover from debugging?
goto cleanup;
+ }
- if (dbus_error_is_set(&error)) {
+ VIR_ERROR("Error %d: %s\n", error.level, error.message);
+ 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
[...]
@@ -862,12 +850,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)
Another thing that's pre-existing, but we are adding more and more of
them. I don't like that libvirt is still using systemd stuff even if
configured not to (--without-systemd).
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 {
--
This is pretty hackish IMHO. Since
"org.freedesktop.DBus.Error.UnknownMethod" isn't specific for systemd,
we can make virDBusCall() report whether unknown method was called or
not and then decide what to do with it two layers up in the stack. It
also simplifies virSystemdCreateMachine() a lot.
Martin
10:24 p.m.
On Mon, Jan 19, 2015 at 02:13:12PM +0100, Martin Kletzander wrote:
On Mon, Jan 19, 2015 at 12:34:52PM +0000, 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(a)redhat.com>
>---
>src/util/virdbus.c | 31 +++++++++++++++++++------------
>src/util/virdbus.h | 4 ++--
>src/util/virfirewall.c | 34 ++++++++++------------------------
>src/util/virsystemd.c | 15 +++++++--------
>4 files changed, 38 insertions(+), 46 deletions(-)
>
>This is a build-breaker fix, but I'm not pushing since the
>fix is too complicated to go in without some review.
>
[...]
>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
Using virError instead of DBusError is fine, but
>+# define dbus_message_unref(m) do {} while (0)
ewww, can't we just cleanly separate DBus code from libvirt code
instead of adding more of these?
>diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
>index b536912..a01b703 100644
>--- a/src/util/virfirewall.c
>+++ b/src/util/virfirewall.c
[...]
>@@ -789,10 +777,13 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule,
> "sa&s",
> ipv,
> (int)rule->argsLen,
>- rule->args) < 0)
>+ rule->args) < 0) {
>+ VIR_ERROR("Here fail");
Leftover from debugging?
> goto cleanup;
>+ }
>
>- if (dbus_error_is_set(&error)) {
>+ VIR_ERROR("Error %d: %s\n", error.level, error.message);
>+ 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
[...]
>@@ -862,12 +850,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)
Another thing that's pre-existing, but we are adding more and more of
them. I don't like that libvirt is still using systemd stuff even if
configured not to (--without-systemd).
>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 {
>--
This is pretty hackish IMHO. Since
"org.freedesktop.DBus.Error.UnknownMethod" isn't specific for systemd,
we can make virDBusCall() report whether unknown method was called or
not and then decide what to do with it two layers up in the stack. It
also simplifies virSystemdCreateMachine() a lot.
Not to mention DBus offers introspection of service properties, but I
must admit I have no idea how to make use of it using its C API.
Martin
10:30 p.m.
On Mon, Jan 19, 2015 at 02:24:34PM +0100, Martin Kletzander wrote:
On Mon, Jan 19, 2015 at 02:13:12PM +0100, Martin Kletzander wrote:
>On Mon, Jan 19, 2015 at 12:34:52PM +0000, 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(a)redhat.com>
>>---
>>src/util/virdbus.c | 31 +++++++++++++++++++------------
>>src/util/virdbus.h | 4 ++--
>>src/util/virfirewall.c | 34 ++++++++++------------------------
>>src/util/virsystemd.c | 15 +++++++--------
>>4 files changed, 38 insertions(+), 46 deletions(-)
>>
>>This is a build-breaker fix, but I'm not pushing since the
>>fix is too complicated to go in without some review.
>>
>[...]
>>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
>
>Using virError instead of DBusError is fine, but
>
>>+# define dbus_message_unref(m) do {} while (0)
>
>ewww, can't we just cleanly separate DBus code from libvirt code
>instead of adding more of these?
>
>>diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
>>index b536912..a01b703 100644
>>--- a/src/util/virfirewall.c
>>+++ b/src/util/virfirewall.c
>[...]
>>@@ -789,10 +777,13 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule,
>> "sa&s",
>> ipv,
>> (int)rule->argsLen,
>>- rule->args) < 0)
>>+ rule->args) < 0) {
>>+ VIR_ERROR("Here fail");
>
>Leftover from debugging?
>
>> goto cleanup;
>>+ }
>>
>>- if (dbus_error_is_set(&error)) {
>>+ VIR_ERROR("Error %d: %s\n", error.level, error.message);
>>+ 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
>[...]
>>@@ -862,12 +850,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)
>
>Another thing that's pre-existing, but we are adding more and more of
>them. I don't like that libvirt is still using systemd stuff even if
>configured not to (--without-systemd).
>
>>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 {
>>--
>
>This is pretty hackish IMHO. Since
>"org.freedesktop.DBus.Error.UnknownMethod" isn't specific for systemd,
>we can make virDBusCall() report whether unknown method was called or
>not and then decide what to do with it two layers up in the stack. It
>also simplifies virSystemdCreateMachine() a lot.
>
Not to mention DBus offers introspection of service properties, but I
must admit I have no idea how to make use of it using its C API.
The introspection is primarily intended to non-C language bindings that
need to know what data types to use when encoding/decoding data, since
their languages are typically loosely typed. While you could parse the
introspection to check if a method exists ahead of time, it is really
way overkill for this problem and would require a seriously large amount
of code to deal with it. There's just no win for doing that here.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:28 p.m.
On Mon, Jan 19, 2015 at 02:13:12PM +0100, Martin Kletzander wrote:
On Mon, Jan 19, 2015 at 12:34:52PM +0000, 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(a)redhat.com>
>---
>src/util/virdbus.c | 31 +++++++++++++++++++------------
>src/util/virdbus.h | 4 ++--
>src/util/virfirewall.c | 34 ++++++++++------------------------
>src/util/virsystemd.c | 15 +++++++--------
>4 files changed, 38 insertions(+), 46 deletions(-)
>
>This is a build-breaker fix, but I'm not pushing since the
>fix is too complicated to go in without some review.
>
[...]
>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
Using virError instead of DBusError is fine, but
>+# define dbus_message_unref(m) do {} while (0)
ewww, can't we just cleanly separate DBus code from libvirt code
instead of adding more of these?
This is required because we removed the WITH_DBUS conditional
from virsystemd. To eliminate this would require that we remove
DBusMessage from the virdbus.h header files too. That is doable
but a bigger job and not really needed to fix this current build
problem, so I think this is appropriate fix for this current
patch.
>diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
>index b536912..a01b703 100644
>--- a/src/util/virfirewall.c
>+++ b/src/util/virfirewall.c
[...]
>@@ -789,10 +777,13 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule,
> "sa&s",
> ipv,
> (int)rule->argsLen,
>- rule->args) < 0)
>+ rule->args) < 0) {
>+ VIR_ERROR("Here fail");
Leftover from debugging?
Opps, yes.
> goto cleanup;
>+ }
>
>- if (dbus_error_is_set(&error)) {
>+ VIR_ERROR("Error %d: %s\n", error.level, error.message);
>+ 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
[...]
>@@ -862,12 +850,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)
Another thing that's pre-existing, but we are adding more and more of
them. I don't like that libvirt is still using systemd stuff even if
configured not to (--without-systemd).
The --without-systemd flag is about use of the libsystemd-daemon.so
library, which is used for daemon startup notification.
Everything else related to systemd is just plain DBus API calls, so
it is not appropriate to tie them to the --without-systemd flag. All
DBus calls are dealt with by runtime checks for the service, rather
than compile time checks for a lbirary.
>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 {
>--
This is pretty hackish IMHO. Since
"org.freedesktop.DBus.Error.UnknownMethod" isn't specific for systemd,
we can make virDBusCall() report whether unknown method was called or
not and then decide what to do with it two layers up in the stack. It
also simplifies virSystemdCreateMachine() a lot.
The "org.freedesktop.DBus.Error.UnknownMethod" method is a standard error
code that is to be raised by DBus services, when the method requested does
not exist. This is exactly the scenario we are dealing with here. We tried
to run the CreateMachineWithNetwork method and it doesn't exist, so looking
for the UnknownMethod error name is the correct handling for this scenario
and how DBus error handling is intended to work.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:50 p.m.
On Mon, Jan 19, 2015 at 01:28:15PM +0000, Daniel P. Berrange wrote:
On Mon, Jan 19, 2015 at 02:13:12PM +0100, Martin Kletzander wrote:
> On Mon, Jan 19, 2015 at 12:34:52PM +0000, 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(a)redhat.com>
> >---
> >src/util/virdbus.c | 31 +++++++++++++++++++------------
> >src/util/virdbus.h | 4 ++--
> >src/util/virfirewall.c | 34 ++++++++++------------------------
> >src/util/virsystemd.c | 15 +++++++--------
> >4 files changed, 38 insertions(+), 46 deletions(-)
> >
> >This is a build-breaker fix, but I'm not pushing since the
> >fix is too complicated to go in without some review.
> >
> [...]
> >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
>
> Using virError instead of DBusError is fine, but
>
> >+# define dbus_message_unref(m) do {} while (0)
>
> ewww, can't we just cleanly separate DBus code from libvirt code
> instead of adding more of these?
This is required because we removed the WITH_DBUS conditional
from virsystemd. To eliminate this would require that we remove
DBusMessage from the virdbus.h header files too. That is doable
but a bigger job and not really needed to fix this current build
problem, so I think this is appropriate fix for this current
patch.
>
> >diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> >index b536912..a01b703 100644
> >--- a/src/util/virfirewall.c
> >+++ b/src/util/virfirewall.c
> [...]
> >@@ -789,10 +777,13 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule,
> > "sa&s",
> > ipv,
> > (int)rule->argsLen,
> >- rule->args) < 0)
> >+ rule->args) < 0) {
> >+ VIR_ERROR("Here fail");
>
> Leftover from debugging?
Opps, yes.
> > goto cleanup;
> >+ }
> >
> >- if (dbus_error_is_set(&error)) {
> >+ VIR_ERROR("Error %d: %s\n", error.level, error.message);
> >+ 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
> [...]
> >@@ -862,12 +850,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)
>
> Another thing that's pre-existing, but we are adding more and more of
> them. I don't like that libvirt is still using systemd stuff even if
> configured not to (--without-systemd).
The --without-systemd flag is about use of the libsystemd-daemon.so
library, which is used for daemon startup notification.
Everything else related to systemd is just plain DBus API calls, so
it is not appropriate to tie them to the --without-systemd flag. All
DBus calls are dealt with by runtime checks for the service, rather
than compile time checks for a lbirary.
> >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 {
> >--
>
> This is pretty hackish IMHO. Since
> "org.freedesktop.DBus.Error.UnknownMethod" isn't specific for systemd,
> we can make virDBusCall() report whether unknown method was called or
> not and then decide what to do with it two layers up in the stack. It
> also simplifies virSystemdCreateMachine() a lot.
The "org.freedesktop.DBus.Error.UnknownMethod" method is a standard error
code that is to be raised by DBus services, when the method requested does
not exist. This is exactly the scenario we are dealing with here. We tried
to run the CreateMachineWithNetwork method and it doesn't exist, so looking
for the UnknownMethod error name is the correct handling for this scenario
and how DBus error handling is intended to work.
Yes, I know, my idea was rather something like the following, although
that does not have the DBusError -> virError change done in itself:
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index d9665c1..1f232a6 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -1,7 +1,7 @@
/*
* virdbus.c: helper for using DBus
*
- * Copyright (C) 2012-2014 Red Hat, Inc.
+ * Copyright (C) 2012-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -1522,8 +1522,8 @@ static int
virDBusCall(DBusConnection *conn,
DBusMessage *call,
DBusMessage **replyout,
- DBusError *error)
-
+ DBusError *error,
+ bool missing_ok)
{
DBusMessage *reply = NULL;
DBusError localerror;
@@ -1553,6 +1553,11 @@ virDBusCall(DBusConnection *conn,
error ? error->message : localerror.message);
if (error) {
ret = 0;
+ } else if (missing_ok &&
+ dbus_error_is_set(&error) &&
+ STREQ_NULLABLE("org.freedesktop.DBus.Error.UnknownMethod",
+ error.name)) {
+ ret = -2;
} else {
virReportError(VIR_ERR_DBUS_SERVICE, _("%s: %s"), member,
localerror.message ? localerror.message : _("unknown error"));
@@ -1617,6 +1622,7 @@ virDBusCall(DBusConnection *conn,
int virDBusCallMethod(DBusConnection *conn,
DBusMessage **replyout,
DBusError *error,
+ bool missing_ok,
const char *destination,
const char *path,
const char *iface,
@@ -1634,9 +1640,10 @@ int virDBusCallMethod(DBusConnection *conn,
if (ret < 0)
goto cleanup;
- ret = -1;
+ ret = virDBusCall(conn, call, replyout, error, missing_ok);
- ret = virDBusCall(conn, call, replyout, error);
+ if (ret == -2)
+ VIR_INFO("DBus method '%s' isn't supported", member);
cleanup:
if (call)
--
Martin
11:02 p.m.
On Mon, Jan 19, 2015 at 02:50:19PM +0100, Martin Kletzander wrote:
On Mon, Jan 19, 2015 at 01:28:15PM +0000, Daniel P. Berrange wrote:
>On Mon, Jan 19, 2015 at 02:13:12PM +0100, Martin Kletzander wrote:
>>On Mon, Jan 19, 2015 at 12:34:52PM +0000, 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(a)redhat.com>
>>>---
>>>src/util/virdbus.c | 31 +++++++++++++++++++------------
>>>src/util/virdbus.h | 4 ++--
>>>src/util/virfirewall.c | 34 ++++++++++------------------------
>>>src/util/virsystemd.c | 15 +++++++--------
>>>4 files changed, 38 insertions(+), 46 deletions(-)
>>>
>>>This is a build-breaker fix, but I'm not pushing since the
>>>fix is too complicated to go in without some review.
>>>
>>[...]
>>>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
>>
>>Using virError instead of DBusError is fine, but
>>
>>>+# define dbus_message_unref(m) do {} while (0)
>>
>>ewww, can't we just cleanly separate DBus code from libvirt code
>>instead of adding more of these?
>
>This is required because we removed the WITH_DBUS conditional
>from virsystemd. To eliminate this would require that we remove
>DBusMessage from the virdbus.h header files too. That is doable
>but a bigger job and not really needed to fix this current build
>problem, so I think this is appropriate fix for this current
>patch.
>
>>
>>>diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
>>>index b536912..a01b703 100644
>>>--- a/src/util/virfirewall.c
>>>+++ b/src/util/virfirewall.c
>>[...]
>>>@@ -789,10 +777,13 @@ virFirewallApplyRuleFirewallD(virFirewallRulePtr rule,
>>> "sa&s",
>>> ipv,
>>> (int)rule->argsLen,
>>>- rule->args) < 0)
>>>+ rule->args) < 0) {
>>>+ VIR_ERROR("Here fail");
>>
>>Leftover from debugging?
>
>Opps, yes.
>
>>> goto cleanup;
>>>+ }
>>>
>>>- if (dbus_error_is_set(&error)) {
>>>+ VIR_ERROR("Error %d: %s\n", error.level, error.message);
>>>+ 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
>>[...]
>>>@@ -862,12 +850,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)
>>
>>Another thing that's pre-existing, but we are adding more and more of
>>them. I don't like that libvirt is still using systemd stuff even if
>>configured not to (--without-systemd).
>
>The --without-systemd flag is about use of the libsystemd-daemon.so
>library, which is used for daemon startup notification.
>
>Everything else related to systemd is just plain DBus API calls, so
>it is not appropriate to tie them to the --without-systemd flag. All
>DBus calls are dealt with by runtime checks for the service, rather
>than compile time checks for a lbirary.
>
>>>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 {
>>>--
>>
>>This is pretty hackish IMHO. Since
>>"org.freedesktop.DBus.Error.UnknownMethod" isn't specific for
systemd,
>>we can make virDBusCall() report whether unknown method was called or
>>not and then decide what to do with it two layers up in the stack. It
>>also simplifies virSystemdCreateMachine() a lot.
>
>The "org.freedesktop.DBus.Error.UnknownMethod" method is a standard error
>code that is to be raised by DBus services, when the method requested does
>not exist. This is exactly the scenario we are dealing with here. We tried
>to run the CreateMachineWithNetwork method and it doesn't exist, so looking
>for the UnknownMethod error name is the correct handling for this scenario
>and how DBus error handling is intended to work.
>
Yes, I know, my idea was rather something like the following, although
that does not have the DBusError -> virError change done in itself:
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index d9665c1..1f232a6 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -1,7 +1,7 @@
/*
* virdbus.c: helper for using DBus
*
- * Copyright (C) 2012-2014 Red Hat, Inc.
+ * Copyright (C) 2012-2015 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -1522,8 +1522,8 @@ static int
virDBusCall(DBusConnection *conn,
DBusMessage *call,
DBusMessage **replyout,
- DBusError *error)
-
+ DBusError *error,
+ bool missing_ok)
{
DBusMessage *reply = NULL;
DBusError localerror;
@@ -1553,6 +1553,11 @@ virDBusCall(DBusConnection *conn,
error ? error->message : localerror.message);
if (error) {
ret = 0;
+ } else if (missing_ok &&
+ dbus_error_is_set(&error) &&
+ STREQ_NULLABLE("org.freedesktop.DBus.Error.UnknownMethod",
+ error.name)) {
+ ret = -2;
} else {
virReportError(VIR_ERR_DBUS_SERVICE, _("%s: %s"), member,
localerror.message ? localerror.message : _("unknown error"));
@@ -1617,6 +1622,7 @@ virDBusCall(DBusConnection *conn,
int virDBusCallMethod(DBusConnection *conn,
DBusMessage **replyout,
DBusError *error,
+ bool missing_ok,
const char *destination,
const char *path,
const char *iface,
@@ -1634,9 +1640,10 @@ int virDBusCallMethod(DBusConnection *conn,
if (ret < 0)
goto cleanup;
- ret = -1;
+ ret = virDBusCall(conn, call, replyout, error, missing_ok);
- ret = virDBusCall(conn, call, replyout, error);
+ if (ret == -2)
+ VIR_INFO("DBus method '%s' isn't supported", member);
cleanup:
if (call)
I don't think this scales up particularly well as we have to deal with
other types of DBus errors though, so I'm not convinced special casing
this particular error is useful.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3732
days inactive
3732
days old
6 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Martin Kletzander