[libvirt] [PATCH] Revert "dbus: correctly build reply message"

Michal Privoznik posted 1 patch 4 years, 6 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/0dab0f8dbdcfa85deb8db0b984509bce683204ac.1567784192.git.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(-)
[libvirt] [PATCH] Revert "dbus: correctly build reply message"
Posted by Michal Privoznik 4 years, 6 months ago
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
Re: [libvirt] [PATCH] Revert "dbus: correctly build reply message"
Posted by Marc-André Lureau 4 years, 6 months ago
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
Re: [libvirt] [PATCH] Revert "dbus: correctly build reply message"
Posted by Daniel P. Berrangé 4 years, 6 months ago
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
Re: [libvirt] [PATCH] Revert "dbus: correctly build reply message"
Posted by Daniel P. Berrangé 4 years, 6 months ago
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