src/util/virdbus.c | 18 ++++++------------ src/util/virdbus.h | 6 ++---- tests/virfirewalltest.c | 9 +++------ tests/virpolkittest.c | 3 +-- 4 files changed, 12 insertions(+), 24 deletions(-)
This reverts commit 39dded7bb61444bb608fadd3f82f6fe93d08fd0e.
This commit broke virpolkittest on Ubuntu 18 which has an old
dbus (v1.12.2). Any other distro with the recent one works
(v1.12.16) which hints its a bug in dbus somewhere. Revert the
commit to stop tickling it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/util/virdbus.c | 18 ++++++------------
src/util/virdbus.h | 6 ++----
tests/virfirewalltest.c | 9 +++------
tests/virpolkittest.c | 3 +--
4 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index 64513eef14..b0ac8d7055 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -1456,7 +1456,6 @@ int virDBusCreateMethod(DBusMessage **call,
/**
* virDBusCreateReplyV:
- * @msg: the message to reply to
* @reply: pointer to be filled with a method reply message
* @types: type signature for following method arguments
* @args: method arguments
@@ -1469,14 +1468,13 @@ int virDBusCreateMethod(DBusMessage **call,
* as variadic args. See virDBusCreateMethodV for a
* description of this parameter.
*/
-int virDBusCreateReplyV(DBusMessage *msg,
- DBusMessage **reply,
+int virDBusCreateReplyV(DBusMessage **reply,
const char *types,
va_list args)
{
int ret = -1;
- if (!(*reply = dbus_message_new_method_return(msg))) {
+ if (!(*reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN))) {
virReportOOMError();
goto cleanup;
}
@@ -1495,7 +1493,6 @@ int virDBusCreateReplyV(DBusMessage *msg,
/**
* virDBusCreateReply:
- * @msg: the message to reply to
* @reply: pointer to be filled with a method reply message
* @types: type signature for following method arguments
* @...: method arguments
@@ -1503,15 +1500,14 @@ int virDBusCreateReplyV(DBusMessage *msg,
* See virDBusCreateReplyV for a description of the
* behaviour of this method.
*/
-int virDBusCreateReply(DBusMessage *msg,
- DBusMessage **reply,
+int virDBusCreateReply(DBusMessage **reply,
const char *types, ...)
{
va_list args;
int ret;
va_start(args, types);
- ret = virDBusCreateReplyV(msg, reply, types, args);
+ ret = virDBusCreateReplyV(reply, types, args);
va_end(args);
return ret;
@@ -1815,8 +1811,7 @@ int virDBusCreateMethodV(DBusMessage **call ATTRIBUTE_UNUSED,
return -1;
}
-int virDBusCreateReplyV(DBusMessage *msg ATTRIBUTE_UNUSED,
- DBusMessage **reply ATTRIBUTE_UNUSED,
+int virDBusCreateReplyV(DBusMessage **reply ATTRIBUTE_UNUSED,
const char *types ATTRIBUTE_UNUSED,
va_list args ATTRIBUTE_UNUSED)
{
@@ -1825,8 +1820,7 @@ int virDBusCreateReplyV(DBusMessage *msg ATTRIBUTE_UNUSED,
return -1;
}
-int virDBusCreateReply(DBusMessage *msg ATTRIBUTE_UNUSED,
- DBusMessage **reply ATTRIBUTE_UNUSED,
+int virDBusCreateReply(DBusMessage **reply ATTRIBUTE_UNUSED,
const char *types ATTRIBUTE_UNUSED, ...)
{
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/util/virdbus.h b/src/util/virdbus.h
index 0303e91045..083c074d59 100644
--- a/src/util/virdbus.h
+++ b/src/util/virdbus.h
@@ -52,11 +52,9 @@ int virDBusCreateMethodV(DBusMessage **call,
const char *member,
const char *types,
va_list args);
-int virDBusCreateReply(DBusMessage *msg,
- DBusMessage **reply,
+int virDBusCreateReply(DBusMessage **reply,
const char *types, ...);
-int virDBusCreateReplyV(DBusMessage *msg,
- DBusMessage **reply,
+int virDBusCreateReplyV(DBusMessage **reply,
const char *types,
va_list args);
diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c
index e5eeb52175..78685a3bf4 100644
--- a/tests/virfirewalltest.c
+++ b/tests/virfirewalltest.c
@@ -150,8 +150,7 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
if (nargs == 1 &&
STREQ(type, "ipv4") &&
STREQ(args[0], "-L")) {
- if (virDBusCreateReply(message,
- &reply,
+ if (virDBusCreateReply(&reply,
"s", TEST_FILTER_TABLE_LIST) < 0)
goto error;
} else if (nargs == 3 &&
@@ -159,13 +158,11 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
STREQ(args[0], "-t") &&
STREQ(args[1], "nat") &&
STREQ(args[2], "-L")) {
- if (virDBusCreateReply(message,
- &reply,
+ if (virDBusCreateReply(&reply,
"s", TEST_NAT_TABLE_LIST) < 0)
goto error;
} else {
- if (virDBusCreateReply(message,
- &reply,
+ if (virDBusCreateReply(&reply,
"s", "success") < 0)
goto error;
}
diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c
index 845ceb1736..ce1ff92bf2 100644
--- a/tests/virpolkittest.c
+++ b/tests/virpolkittest.c
@@ -123,8 +123,7 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block,
VIR_FREE(cancellationId);
virStringListFreeCount(details, detailslen);
- if (virDBusCreateReply(message,
- &reply,
+ if (virDBusCreateReply(&reply,
"(bba&{ss})",
is_authorized,
is_challenge,
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hi On Fri, Sep 6, 2019 at 7:37 PM Michal Privoznik <mprivozn@redhat.com> wrote: > > This reverts commit 39dded7bb61444bb608fadd3f82f6fe93d08fd0e. > > This commit broke virpolkittest on Ubuntu 18 which has an old > dbus (v1.12.2). Any other distro with the recent one works > (v1.12.16) which hints its a bug in dbus somewhere. Revert the > commit to stop tickling it. > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> This patch is no longer necessary for dbus-vmstate in this series. I found this "bug" when implementing an alternative solution. So reverting it is fine by me. However, it may be revealing a transient bug in virDBusMessageIterDecode() rather than libdbus. It would be worth to have VIR_DEBUG() output from the failing test. For now: Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > src/util/virdbus.c | 18 ++++++------------ > src/util/virdbus.h | 6 ++---- > tests/virfirewalltest.c | 9 +++------ > tests/virpolkittest.c | 3 +-- > 4 files changed, 12 insertions(+), 24 deletions(-) > > diff --git a/src/util/virdbus.c b/src/util/virdbus.c > index 64513eef14..b0ac8d7055 100644 > --- a/src/util/virdbus.c > +++ b/src/util/virdbus.c > @@ -1456,7 +1456,6 @@ int virDBusCreateMethod(DBusMessage **call, > > /** > * virDBusCreateReplyV: > - * @msg: the message to reply to > * @reply: pointer to be filled with a method reply message > * @types: type signature for following method arguments > * @args: method arguments > @@ -1469,14 +1468,13 @@ int virDBusCreateMethod(DBusMessage **call, > * as variadic args. See virDBusCreateMethodV for a > * description of this parameter. > */ > -int virDBusCreateReplyV(DBusMessage *msg, > - DBusMessage **reply, > +int virDBusCreateReplyV(DBusMessage **reply, > const char *types, > va_list args) > { > int ret = -1; > > - if (!(*reply = dbus_message_new_method_return(msg))) { > + if (!(*reply = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN))) { > virReportOOMError(); > goto cleanup; > } > @@ -1495,7 +1493,6 @@ int virDBusCreateReplyV(DBusMessage *msg, > > /** > * virDBusCreateReply: > - * @msg: the message to reply to > * @reply: pointer to be filled with a method reply message > * @types: type signature for following method arguments > * @...: method arguments > @@ -1503,15 +1500,14 @@ int virDBusCreateReplyV(DBusMessage *msg, > * See virDBusCreateReplyV for a description of the > * behaviour of this method. > */ > -int virDBusCreateReply(DBusMessage *msg, > - DBusMessage **reply, > +int virDBusCreateReply(DBusMessage **reply, > const char *types, ...) > { > va_list args; > int ret; > > va_start(args, types); > - ret = virDBusCreateReplyV(msg, reply, types, args); > + ret = virDBusCreateReplyV(reply, types, args); > va_end(args); > > return ret; > @@ -1815,8 +1811,7 @@ int virDBusCreateMethodV(DBusMessage **call ATTRIBUTE_UNUSED, > return -1; > } > > -int virDBusCreateReplyV(DBusMessage *msg ATTRIBUTE_UNUSED, > - DBusMessage **reply ATTRIBUTE_UNUSED, > +int virDBusCreateReplyV(DBusMessage **reply ATTRIBUTE_UNUSED, > const char *types ATTRIBUTE_UNUSED, > va_list args ATTRIBUTE_UNUSED) > { > @@ -1825,8 +1820,7 @@ int virDBusCreateReplyV(DBusMessage *msg ATTRIBUTE_UNUSED, > return -1; > } > > -int virDBusCreateReply(DBusMessage *msg ATTRIBUTE_UNUSED, > - DBusMessage **reply ATTRIBUTE_UNUSED, > +int virDBusCreateReply(DBusMessage **reply ATTRIBUTE_UNUSED, > const char *types ATTRIBUTE_UNUSED, ...) > { > virReportError(VIR_ERR_INTERNAL_ERROR, > diff --git a/src/util/virdbus.h b/src/util/virdbus.h > index 0303e91045..083c074d59 100644 > --- a/src/util/virdbus.h > +++ b/src/util/virdbus.h > @@ -52,11 +52,9 @@ int virDBusCreateMethodV(DBusMessage **call, > const char *member, > const char *types, > va_list args); > -int virDBusCreateReply(DBusMessage *msg, > - DBusMessage **reply, > +int virDBusCreateReply(DBusMessage **reply, > const char *types, ...); > -int virDBusCreateReplyV(DBusMessage *msg, > - DBusMessage **reply, > +int virDBusCreateReplyV(DBusMessage **reply, > const char *types, > va_list args); > > diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c > index e5eeb52175..78685a3bf4 100644 > --- a/tests/virfirewalltest.c > +++ b/tests/virfirewalltest.c > @@ -150,8 +150,7 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block, > if (nargs == 1 && > STREQ(type, "ipv4") && > STREQ(args[0], "-L")) { > - if (virDBusCreateReply(message, > - &reply, > + if (virDBusCreateReply(&reply, > "s", TEST_FILTER_TABLE_LIST) < 0) > goto error; > } else if (nargs == 3 && > @@ -159,13 +158,11 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block, > STREQ(args[0], "-t") && > STREQ(args[1], "nat") && > STREQ(args[2], "-L")) { > - if (virDBusCreateReply(message, > - &reply, > + if (virDBusCreateReply(&reply, > "s", TEST_NAT_TABLE_LIST) < 0) > goto error; > } else { > - if (virDBusCreateReply(message, > - &reply, > + if (virDBusCreateReply(&reply, > "s", "success") < 0) > goto error; > } > diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c > index 845ceb1736..ce1ff92bf2 100644 > --- a/tests/virpolkittest.c > +++ b/tests/virpolkittest.c > @@ -123,8 +123,7 @@ VIR_MOCK_WRAP_RET_ARGS(dbus_connection_send_with_reply_and_block, > VIR_FREE(cancellationId); > virStringListFreeCount(details, detailslen); > > - if (virDBusCreateReply(message, > - &reply, > + if (virDBusCreateReply(&reply, > "(bba&{ss})", > is_authorized, > is_challenge, > -- > 2.21.0 > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 09, 2019 at 03:47:21PM +0400, Marc-André Lureau wrote: > Hi > > On Fri, Sep 6, 2019 at 7:37 PM Michal Privoznik <mprivozn@redhat.com> wrote: > > > > This reverts commit 39dded7bb61444bb608fadd3f82f6fe93d08fd0e. > > > > This commit broke virpolkittest on Ubuntu 18 which has an old > > dbus (v1.12.2). Any other distro with the recent one works > > (v1.12.16) which hints its a bug in dbus somewhere. Revert the > > commit to stop tickling it. > > > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > > This patch is no longer necessary for dbus-vmstate in this series. I > found this "bug" when implementing an alternative solution. So > reverting it is fine by me. > > However, it may be revealing a transient bug in > virDBusMessageIterDecode() rather than libdbus. It would be worth to > have VIR_DEBUG() output from the failing test. It is not really a bug - rather its due to the short cut taken by the test suite. virpolkittest is mocking the dbus_connection_send_with_reply_and_block method. This method accepts a DBusMessage input (the MethodCall) and has to create DBusMessage as output (the MethodReply) The DBusMessage input has no serial number set, since this is the job of the real dbus_connection_send_with_reply_and_block() method impl. The dbus_message_new_method_return() impl expects the original message to have a valid serial number though. Thus when we create the DBusMessage output, we hit an assertion failure when calling dbus_message_new_method_return(). This is in fact why the code intentionally uses dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN) in the first place. What's strange though is why it ever succeeds on any distro. The assertion we're seeing should *always* hit us on every distro. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 09, 2019 at 02:11:36PM +0100, Daniel P. Berrangé wrote: > On Mon, Sep 09, 2019 at 03:47:21PM +0400, Marc-André Lureau wrote: > > Hi > > > > On Fri, Sep 6, 2019 at 7:37 PM Michal Privoznik <mprivozn@redhat.com> wrote: > > > > > > This reverts commit 39dded7bb61444bb608fadd3f82f6fe93d08fd0e. > > > > > > This commit broke virpolkittest on Ubuntu 18 which has an old > > > dbus (v1.12.2). Any other distro with the recent one works > > > (v1.12.16) which hints its a bug in dbus somewhere. Revert the > > > commit to stop tickling it. > > > > > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > > > > This patch is no longer necessary for dbus-vmstate in this series. I > > found this "bug" when implementing an alternative solution. So > > reverting it is fine by me. > > > > However, it may be revealing a transient bug in > > virDBusMessageIterDecode() rather than libdbus. It would be worth to > > have VIR_DEBUG() output from the failing test. > > It is not really a bug - rather its due to the short cut taken by > the test suite. > > virpolkittest is mocking the dbus_connection_send_with_reply_and_block > method. > > This method accepts a DBusMessage input (the MethodCall) and has to > create DBusMessage as output (the MethodReply) > > The DBusMessage input has no serial number set, since this is the job > of the real dbus_connection_send_with_reply_and_block() method impl. > > The dbus_message_new_method_return() impl expects the original message > to have a valid serial number though. > > Thus when we create the DBusMessage output, we hit an assertion failure > when calling dbus_message_new_method_return(). > > This is in fact why the code intentionally uses > > dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN) > > in the first place. > > What's strange though is why it ever succeeds on any distro. The > assertion we're seeing should *always* hit us on every distro. Oh, I missed that we now have a mock dbus_message_set_reply_serial method that does nothing. I wonder if on certain builds the compiler decided to inline that method when called from dbus_message_new_method_return, so our mock didn't take effect. Oddly I can't reproduce it even on Ubuntu 18 using 'make ci-test@ubuntu-18' Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.