https://bugzilla.redhat.com/show_bug.cgi?id=1487322

In ace45e67abbd I've tried to fix a problem that we get the reply
to a D-Bus call while we were sleeping. In that case the callback
was never set. So I've changed the code that the callback is
called directly in this case. However, I hadn't realized that
since callback is called out of order it lock the virNetDaemon.
Exactly the very same virNetDaemon that we are dealing with right
now and have it locked already (in
virNetDaemonAddShutdownInhibition())

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/rpc/virnetdaemon.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index 00247cfc3..e3b9390af 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -439,14 +439,12 @@ virNetDaemonAutoShutdown(virNetDaemonPtr dmn,
 
 #if defined(WITH_DBUS) && defined(DBUS_TYPE_UNIX_FD)
 static void
-virNetDaemonGotInhibitReply(DBusPendingCall *pending,
-                            void *opaque)
+virNetDaemonGotInhibitReplyLocked(DBusPendingCall *pending,
+                                  virNetDaemonPtr dmn)
 {
-    virNetDaemonPtr dmn = opaque;
     DBusMessage *reply;
     int fd;
 
-    virObjectLock(dmn);
     dmn->autoShutdownCallingInhibit = false;
 
     VIR_DEBUG("dmn=%p", dmn);
@@ -470,11 +468,22 @@ virNetDaemonGotInhibitReply(DBusPendingCall *pending,
     virDBusMessageUnref(reply);
 
  cleanup:
-    virObjectUnlock(dmn);
     dbus_pending_call_unref(pending);
 }
 
 
+static void
+virNetDaemonGotInhibitReply(DBusPendingCall *pending,
+                            void *opaque)
+{
+    virNetDaemonPtr dmn = opaque;
+
+    virObjectLock(dmn);
+    virNetDaemonGotInhibitReplyLocked(pending, dmn);
+    virObjectUnlock(dmn);
+}
+
+
 /* As per: http://www.freedesktop.org/wiki/Software/systemd/inhibit */
 static void
 virNetDaemonCallInhibit(virNetDaemonPtr dmn,
@@ -516,7 +525,7 @@ virNetDaemonCallInhibit(virNetDaemonPtr dmn,
                                         25 * 1000) &&
         pendingReply) {
         if (dbus_pending_call_get_completed(pendingReply)) {
-            virNetDaemonGotInhibitReply(pendingReply, dmn);
+            virNetDaemonGotInhibitReplyLocked(pendingReply, dmn);
         } else {
             dbus_pending_call_set_notify(pendingReply,
                                          virNetDaemonGotInhibitReply,
-- 
2.13.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Sep 1, 2017 at 10:39 AM, Michal Privoznik <mprivozn@redhat.com>
wrote:

> https://bugzilla.redhat.com/show_bug.cgi?id=1487322
>
> In ace45e67abbd I've tried to fix a problem that we get the reply
> to a D-Bus call while we were sleeping. In that case the callback
> was never set. So I've changed the code that the callback is
> called directly in this case. However, I hadn't realized that
> since callback is called out of order it lock the virNetDaemon.
> Exactly the very same virNetDaemon that we are dealing with right
> now and have it locked already (in
> virNetDaemonAddShutdownInhibition())
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>
>
Change builds fine (on top of 3.6) and seems to fix the issue.
Survived 20 minutes in my stress loop, which it never did before.

Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Sep 01, 2017 at 10:39:10AM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1487322
>
> In ace45e67abbd I've tried to fix a problem that we get the reply

s/I've/I --> the point in the past is known ;)

> to a D-Bus call while we were sleeping. In that case the callback
> was never set. So I've changed the code that the callback is

s/I've/I --> again, we know exactly when :)

> called directly in this case. However, I hadn't realized that
> since callback is called out of order it lock the virNetDaemon.

s/since callback/since the callback
s/lock/locks
s/virNetDaemon/virNetDaemon object

also I'm not sure about 'hadn't realized' being used in this specific case, but
I'd rather not object to that.

> Exactly the very same virNetDaemon that we are dealing with right
> now and have it locked already (in

s/have/having or 'that we have already locked' (preferred)

> virNetDaemonAddShutdownInhibition())
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

ACK

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list