Changeset 15318 in vbox for trunk/src

Timestamp:

Dec 11, 2008 5:27:02 PM (16 years ago)

Author:

vboxsync

Message:

Main: Fixed a deadlock in VBoxSVC when attaching a hard disk indirectly, through creating a new diff for it.

Location:

trunk/src/VBox/Main

Files:

• VirtualBoxImpl.cpp (modified) (3 diffs)
• include/HardDisk2Impl.h (modified) (1 diff)
• include/VirtualBoxImpl.h (modified) (3 diffs)

trunk/src/VBox/Main/VirtualBoxImpl.cpp

@@ -932 +932 @@
     CheckComRCReturnRC (rc);
 
-    /*
-     *  we can safely cast child to Machine * here because only Machine
-     *  implementations of IMachine can be among our children
-     */
+    /* We need the children map lock here to keep the getDependentChild() result
+     * valid until we finish */
+    AutoReadLock chLock (childrenLock());
+
+    /* We can safely cast child to Machine * here because only Machine
+     * implementations of IMachine can be among our children. */
     Machine *machine = static_cast <Machine *> (getDependentChild (aMachine));
-    if (!machine)
-    {
-        /*
-         *  this machine was not created by CreateMachine()
-         *  or opened by OpenMachine() or loaded during startup
-         */
+    if (machine == NULL)
+    {
+        /* this machine was not created by CreateMachine() or opened by
+         * OpenMachine() or loaded during startup */
         return setError (VBOX_E_INVALID_OBJECT_STATE,
             tr ("The machine named '%ls' is not created within this "
@@ -3589 +3589 @@
  * @param aTo       Where to store a reference to the underlying object.
  *
- * @note Locks this object for reading.
+ * @note Locks #childrenLock() for reading.
  */
 HRESULT VirtualBox::cast (IHardDisk2 *aFrom, ComObjPtr <HardDisk2> &aTo)
@@ -3598 +3598 @@
     AssertComRCReturn (autoCaller.rc(), autoCaller.rc());
 
-    AutoReadLock alock (this);
+    /* We need the children map lock here to keep the getDependentChild() result
+     * valid until we finish */
+    AutoReadLock chLock (childrenLock());
 
     VirtualBoxBase *child = getDependentChild (aFrom);

trunk/src/VBox/Main/include/HardDisk2Impl.h

@@ -224 +224 @@
      *    object lock.
      */
-    RWLockHandle *treeLock() { return mVirtualBox->hardDiskTreeHandle(); }
+    RWLockHandle *treeLock() { return mVirtualBox->hardDiskTreeLockHandle(); }
 
     /** Reimplements VirtualBoxWithTypedChildren::childrenLock() to return

trunk/src/VBox/Main/include/VirtualBoxImpl.h

@@ -362 +362 @@
      *    object lock.
      */
-    RWLockHandle *hardDiskTreeHandle() { return &mHardDiskTreeHandle; }
+    RWLockHandle *hardDiskTreeLockHandle() { return &mHardDiskTreeLockHandle; }
 
     /* for VirtualBoxSupportErrorInfoImpl */
@@ -380 +380 @@
 
     typedef std::map <Guid, ComObjPtr <HardDisk2> > HardDisk2Map;
+
+    /**
+     * Reimplements VirtualBoxWithTypedChildren::childrenLock() to return a
+     * dedicated lock instead of the main object lock. The dedicated lock for
+     * child map operations frees callers of init() methods of these children
+     * from acquiring a write parent (VirtualBox) lock (which would be mandatory
+     * otherwise). Since VirtualBox has a lot of heterogenous children which
+     * init() methods are called here and there, it definitely makes sense.
+     */
+    RWLockHandle *childrenLock() { return &mChildrenMapLockHandle; }
 
     HRESULT checkMediaForConflicts2 (const Guid &aId, const Bstr &aLocation,
@@ -490 +500 @@
     RWLockHandle mSafeLock;
 
-    RWLockHandle mHardDiskTreeHandle;
+    RWLockHandle mHardDiskTreeLockHandle;
+    RWLockHandle mChildrenMapLockHandle;
 
     static Bstr sVersion;