Changeset 3330 in vbox

Timestamp:

Jun 29, 2007 4:35:37 PM (18 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

22472

Message:

Main: Converted DVDDrive and FloppyDrive to the new locking scheme.

Location:

trunk/src/VBox/Main

Files:

• DVDDriveImpl.cpp (modified) (14 diffs)
• DVDImageImpl.cpp (modified) (1 diff)
• FloppyDriveImpl.cpp (modified) (15 diffs)
• VirtualBoxImpl.cpp (modified) (1 diff)
• include/DVDDriveImpl.h (modified) (5 diffs)
• include/FloppyDriveImpl.h (modified) (5 diffs)

trunk/src/VBox/Main/DVDDriveImpl.cpp

@@ -22 +22 @@
 #include "DVDDriveImpl.h"
 #include "MachineImpl.h"
+#include "VirtualBoxImpl.h"
 #include "Logging.h"
 
 #include <iprt/string.h>
-
-// defines
-////////////////////////////////////////////////////////////////////////////////
+#include <iprt/cpputils.h>
 
 // constructor / destructor
 ////////////////////////////////////////////////////////////////////////////////
 
+DEFINE_EMPTY_CTOR_DTOR (DVDDrive)
+
 HRESULT DVDDrive::FinalConstruct()
 {
@@ -39 +40 @@
 void DVDDrive::FinalRelease()
 {
-    if (isReady())
-        uninit ();
+    uninit();
 }
 
@@ -49 +49 @@
  * Initializes the DVD drive object.
  *
- * @param   parent  handle of our parent object
+ * @param   aParent  Handle of our parent object.
  * @return  COM result indicator
  */
-HRESULT DVDDrive::init (Machine *parent)
-{
-    LogFlowMember (("DVDDrive::init (%p)\n", parent));
-
-    ComAssertRet (parent, E_INVALIDARG);
-
-    AutoLock alock (this);
-    ComAssertRet (!isReady(), E_UNEXPECTED);
-
-    mParent = parent;
-    // mPeer is left null
+HRESULT DVDDrive::init (Machine *aParent)
+{
+    LogFlowThisFunc (("aParent=%p\n", aParent));
+
+    ComAssertRet (aParent, E_INVALIDARG);
+
+    /* Enclose the state transition NotReady->InInit->Ready */
+    AutoInitSpan autoInitSpan (this);
+    AssertReturn (autoInitSpan.isOk(), E_UNEXPECTED);
+
+    unconst (mParent) = aParent;
+    /* mPeer is left null */
 
     mData.allocate();
 
-    setReady (true);
+    /* Confirm a successful initialization */
+    autoInitSpan.setSucceeded();
+
     return S_OK;
 }
@@ -77 +80 @@
  *  @note This object must be destroyed before the original object
  *  it shares data with is destroyed.
- */
-HRESULT DVDDrive::init (Machine *parent, DVDDrive *that)
-{
-    LogFlowMember (("DVDDrive::init (%p, %p)\n", parent, that));
-
-    ComAssertRet (parent && that, E_INVALIDARG);
-
-    AutoLock alock (this);
-    ComAssertRet (!isReady(), E_UNEXPECTED);
-
-    mParent = parent;
-    mPeer = that;
-
-    AutoLock thatlock (that);
-    mData.share (that->mData);
-
-    setReady (true);
+ *
+ *  @note Locks @a aThat object for reading.
+ */
+HRESULT DVDDrive::init (Machine *aParent, DVDDrive *aThat)
+{
+    LogFlowThisFunc (("aParent=%p, aThat=%p\n", aParent, aThat));
+
+    ComAssertRet (aParent && aThat, E_INVALIDARG);
+
+    /* Enclose the state transition NotReady->InInit->Ready */
+    AutoInitSpan autoInitSpan (this);
+    AssertReturn (autoInitSpan.isOk(), E_UNEXPECTED);
+
+    unconst (mParent) = aParent;
+    unconst (mPeer) = aThat;
+
+    AutoCaller thatCaller (aThat);
+    AssertComRCReturnRC (thatCaller.rc());
+
+    AutoReaderLock thatLock (aThat);
+    mData.share (aThat->mData);
+
+    /* Confirm a successful initialization */
+    autoInitSpan.setSucceeded();
+
     return S_OK;
 }
 
 /**
- *  Initializes the guest object given another guest object
+ *  Initializes the DVD drive object given another DVD drive object
  *  (a kind of copy constructor). This object makes a private copy of data
  *  of the original object passed as an argument.
- */
-HRESULT DVDDrive::initCopy (Machine *parent, DVDDrive *that)
-{
-    LogFlowMember (("DVDDrive::initCopy (%p, %p)\n", parent, that));
-
-    ComAssertRet (parent && that, E_INVALIDARG);
-
-    AutoLock alock (this);
-    ComAssertRet (!isReady(), E_UNEXPECTED);
-
-    mParent = parent;
-    // mPeer is left null
-
-    AutoLock thatlock (that);
-    mData.attachCopy (that->mData);
-
-    setReady (true);
+ *
+ *  @note Locks @a aThat object for reading.
+ */
+HRESULT DVDDrive::initCopy (Machine *aParent, DVDDrive *aThat)
+{
+    LogFlowThisFunc (("aParent=%p, aThat=%p\n", aParent, aThat));
+
+    ComAssertRet (aParent && aThat, E_INVALIDARG);
+
+    /* Enclose the state transition NotReady->InInit->Ready */
+    AutoInitSpan autoInitSpan (this);
+    AssertReturn (autoInitSpan.isOk(), E_UNEXPECTED);
+
+    unconst (mParent) = aParent;
+    /* mPeer is left null */
+
+    AutoCaller thatCaller (aThat);
+    AssertComRCReturnRC (thatCaller.rc());
+
+    AutoReaderLock thatLock (aThat);
+    mData.attachCopy (aThat->mData);
+
+    /* Confirm a successful initialization */
+    autoInitSpan.setSucceeded();
+
     return S_OK;
 }
@@ -127 +146 @@
 void DVDDrive::uninit()
 {
-    LogFlowMember (("DVDDrive::uninit()\n"));
+    LogFlowThisFunc (("\n"));
+
+    /* Enclose the state transition Ready->InUninit->NotReady */
+    AutoUninitSpan autoUninitSpan (this);
+    if (autoUninitSpan.uninitDone())
+        return;
+
+    mData.free();
+
+    unconst (mParent).setNull();
+    unconst (mParent).setNull();
+}
+
+// IDVDDrive properties
+////////////////////////////////////////////////////////////////////////////////
+
+STDMETHODIMP DVDDrive::COMGETTER(State) (DriveState_T *aDriveState)
+{
+    if (!aDriveState)
+        return E_POINTER;
+
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    AutoReaderLock alock (this);
+
+    *aDriveState = mData->mDriveState;
+
+    return S_OK;
+}
+
+STDMETHODIMP DVDDrive::COMGETTER(Passthrough) (BOOL *aPassthrough)
+{
+    if (!aPassthrough)
+        return E_POINTER;
+
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    AutoReaderLock alock (this);
+
+    *aPassthrough = mData->mPassthrough;
+
+    return S_OK;
+}
+
+STDMETHODIMP DVDDrive::COMSETTER(Passthrough) (BOOL aPassthrough)
+{
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    /* the machine needs to be mutable */
+    Machine::AutoMutableStateDependency adep (mParent);
+    CheckComRCReturnRC (adep.rc());
 
     AutoLock alock (this);
 
-    AssertReturn (isReady(), (void) 0);
-
-    mData.free();
-
-    mPeer.setNull();
-    mParent.setNull();
-
-    setReady (false);
-}
-
-// IDVDDrive properties
-////////////////////////////////////////////////////////////////////////////////
-
-STDMETHODIMP DVDDrive::COMGETTER(State) (DriveState_T *driveState)
-{
-    if (!driveState)
-        return E_POINTER;
+    if (mData->mPassthrough != aPassthrough)
+    {
+        mData.backup();
+        mData->mPassthrough = aPassthrough;
+    }
+
+    return S_OK;
+}
+
+// IDVDDrive methods
+////////////////////////////////////////////////////////////////////////////////
+
+STDMETHODIMP DVDDrive::MountImage (INPTR GUIDPARAM aImageId)
+{
+    if (Guid::isEmpty (aImageId))
+        return E_INVALIDARG;
+
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    /* the machine needs to be mutable */
+    Machine::AutoMutableStateDependency adep (mParent);
+    CheckComRCReturnRC (adep.rc());
 
     AutoLock alock (this);
-    CHECK_READY();
-
-    *driveState = mData->mDriveState;
-    return S_OK;
-}
-
-STDMETHODIMP DVDDrive::COMGETTER(Passthrough) (BOOL *passthrough)
-{
-    if (!passthrough)
-        return E_POINTER;
-
-    AutoLock alock(this);
-    CHECK_READY();
-
-    *passthrough = mData->mPassthrough;
-    return S_OK;
-}
-
-STDMETHODIMP DVDDrive::COMSETTER(Passthrough) (BOOL passthrough)
-{
-    AutoLock alock(this);
-    CHECK_READY();
-    CHECK_MACHINE_MUTABILITY (mParent);
-
-    if (mData->mPassthrough != passthrough)
-    {
-        mData.backup();
-        mData->mPassthrough = passthrough;
-    }
-    return S_OK;
-}
-
-// IDVDDrive methods
-////////////////////////////////////////////////////////////////////////////////
-
-STDMETHODIMP DVDDrive::MountImage(INPTR GUIDPARAM imageId)
-{
-    if (Guid::isEmpty(imageId))
-        return E_INVALIDARG;
-
-    AutoLock alock (this);
-    CHECK_READY();
-
-    CHECK_MACHINE_MUTABILITY (mParent);
 
     HRESULT rc = E_FAIL;
 
-    // find the image in the collection
-    ComPtr <IVirtualBox> vbox;
-    rc = mParent->COMGETTER(Parent) (vbox.asOutParam());
-    AssertComRCReturn (rc, rc);
+    /* Our lifetime is bound to mParent's lifetime, so we don't add caller.
+     * We also don't lock mParent since its mParent field is const. */
 
     ComPtr <IDVDImage> image;
-    rc = vbox->GetDVDImage (imageId, image.asOutParam());
+    rc = mParent->virtualBox()->GetDVDImage (aImageId, image.asOutParam());
+
     if (SUCCEEDED (rc))
     {
@@ -210 +243 @@
         {
             mData.backup();
+
             unmount();
+
             mData->mDVDImage = image;
             mData->mDriveState = DriveState_ImageMounted;
 
+            /* leave the lock before informing callbacks*/
             alock.unlock();
+
             mParent->onDVDDriveChange();
         }
@@ -222 +259 @@
 }
 
-STDMETHODIMP DVDDrive::CaptureHostDrive(IHostDVDDrive *hostDVDDrive)
-{
-    if (!hostDVDDrive)
+STDMETHODIMP DVDDrive::CaptureHostDrive (IHostDVDDrive *aHostDVDDrive)
+{
+    if (!aHostDVDDrive)
         return E_INVALIDARG;
 
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    /* the machine needs to be mutable */
+    Machine::AutoMutableStateDependency adep (mParent);
+    CheckComRCReturnRC (adep.rc());
+
     AutoLock alock (this);
-    CHECK_READY();
-
-    CHECK_MACHINE_MUTABILITY (mParent);
 
     if (mData->mDriveState != DriveState_HostDriveCaptured ||
-        !mData->mHostDrive.equalsTo (hostDVDDrive))
+        !mData->mHostDrive.equalsTo (aHostDVDDrive))
     {
         mData.backup();
+
         unmount();
-        mData->mHostDrive = hostDVDDrive;
+
+        mData->mHostDrive = aHostDVDDrive;
         mData->mDriveState = DriveState_HostDriveCaptured;
 
+        /* leave the lock before informing callbacks*/
         alock.unlock();
+
         mParent->onDVDDriveChange();
     }
@@ -249 +294 @@
 STDMETHODIMP DVDDrive::Unmount()
 {
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    /* the machine needs to be mutable */
+    Machine::AutoMutableStateDependency adep (mParent);
+    CheckComRCReturnRC (adep.rc());
+
     AutoLock alock (this);
-    CHECK_READY();
-
-    CHECK_MACHINE_MUTABILITY (mParent);
 
     if (mData->mDriveState != DriveState_NotMounted)
     {
         mData.backup();
+
         unmount();
+
         mData->mDriveState = DriveState_NotMounted;
 
+        /* leave the lock before informing callbacks*/
         alock.unlock();
+
         mParent->onDVDDriveChange();
     }
@@ -267 +320 @@
 }
 
-STDMETHODIMP DVDDrive::GetImage(IDVDImage **dvdImage)
-{
-    if (!dvdImage)
+STDMETHODIMP DVDDrive::GetImage (IDVDImage **aDVDImage)
+{
+    if (!aDVDImage)
         return E_POINTER;
 
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    AutoReaderLock alock (this);
+
+    mData->mDVDImage.queryInterfaceTo (aDVDImage);
+
+    return S_OK;
+}
+
+STDMETHODIMP DVDDrive::GetHostDrive(IHostDVDDrive **aHostDrive)
+{
+    if (!aHostDrive)
+        return E_POINTER;
+
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    AutoReaderLock alock (this);
+
+    mData->mHostDrive.queryInterfaceTo (aHostDrive);
+
+    return S_OK;
+}
+
+// public methods only for internal purposes
+////////////////////////////////////////////////////////////////////////////////
+
+/** 
+ *  @note Locks this object for writing.
+ */
+bool DVDDrive::rollback()
+{
+    /* sanity */
+    AutoCaller autoCaller (this);
+    AssertComRCReturn (autoCaller.rc(), false);
+
     AutoLock alock (this);
-    CHECK_READY();
-
-    mData->mDVDImage.queryInterfaceTo (dvdImage);
-    return S_OK;
-}
-
-STDMETHODIMP DVDDrive::GetHostDrive(IHostDVDDrive **hostDrive)
-{
-    if (!hostDrive)
-        return E_POINTER;
-
-    AutoLock alock (this);
-    CHECK_READY();
-
-    mData->mHostDrive.queryInterfaceTo (hostDrive);
-    return S_OK;
-}
-
-// public methods only for internal purposes
-////////////////////////////////////////////////////////////////////////////////
-
-bool DVDDrive::rollback()
-{
-    AutoLock alock (this);
 
     bool changed = false;
@@ -302 +368 @@
     if (mData.isBackedUp())
     {
-        // we need to check all data to see whether anything will be changed
-        // after rollback
+        /* we need to check all data to see whether anything will be changed
+         * after rollback */
         changed = mData.hasActualChanges();
         mData.rollback();
@@ -311 +377 @@
 }
 
+/** 
+ *  @note Locks this object for writing, together with the peer object (also
+ *  for writing) if there is one.
+ */
 void DVDDrive::commit()
 {
-    AutoLock alock (this);
+    /* sanity */
+    AutoCaller autoCaller (this);
+    AssertComRCReturnVoid (autoCaller.rc());
+
+    /* sanity too */
+    AutoCaller thatCaller (mPeer);
+    AssertComRCReturnVoid (thatCaller.rc());
+
+    /* lock both for writing since we modify both */
+    AutoMultiLock <2> alock (this->wlock(), AutoLock::maybeWlock (mPeer));
+
     if (mData.isBackedUp())
     {
@@ -319 +399 @@
         if (mPeer)
         {
-            // attach new data to the peer and reshare it
-            AutoLock peerlock (mPeer);
+            /* attach new data to the peer and reshare it */
             mPeer->mData.attach (mData);
         }
@@ -326 +405 @@
 }
 
+/** 
+ *  @note Locks this object for writing, together with the peer object (locked
+ *  for reading) if there is one.
+ */
 void DVDDrive::copyFrom (DVDDrive *aThat)
 {
-    AutoLock alock (this);
-
-    // this will back up current data
+    /* sanity */
+    AutoCaller autoCaller (this);
+    AssertComRCReturnVoid (autoCaller.rc());
+
+    /* sanity too */
+    AutoCaller thatCaller (mPeer);
+    AssertComRCReturnVoid (thatCaller.rc());
+
+    /* peer is not modified, lock it for reading */
+    AutoMultiLock <2> alock (this->wlock(), AutoLock::maybeRlock (mPeer));
+
+    /* this will back up current data */
     mData.assignCopy (aThat->mData);
 }
@@ -338 +430 @@
 
 /**
- * Helper to unmount a drive
- *
- * @returns COM status code
+ *  Helper to unmount a drive.
+ *
+ *  @return COM status code
  *
  */
 HRESULT DVDDrive::unmount()
 {
+    AssertReturn (isLockedOnCurrentThread(), E_FAIL);
+
     if (mData->mDVDImage)
         mData->mDVDImage.setNull();
     if (mData->mHostDrive)
         mData->mHostDrive.setNull();
-    return S_OK;
-}
+
+    return S_OK;
+}

trunk/src/VBox/Main/DVDImageImpl.cpp

@@ -27 +27 @@
 #include <iprt/path.h>
 #include <iprt/cpputils.h>
+
 #include <VBox/err.h>
 #include <VBox/param.h>

trunk/src/VBox/Main/FloppyDriveImpl.cpp

@@ -22 +22 @@
 #include "FloppyDriveImpl.h"
 #include "MachineImpl.h"
+#include "VirtualBoxImpl.h"
 #include "Logging.h"
 
 #include <iprt/string.h>
-
-// defines
-/////////////////////////////////////////////////////////////////////////////
+#include <iprt/cpputils.h>
 
 // constructor / destructor
 /////////////////////////////////////////////////////////////////////////////
 
+DEFINE_EMPTY_CTOR_DTOR (FloppyDrive)
+
 HRESULT FloppyDrive::FinalConstruct()
 {
@@ -39 +40 @@
 void FloppyDrive::FinalRelease()
 {
-    if (isReady())
-        uninit ();
+    uninit();
 }
 
@@ -49 +49 @@
  * Initializes the Floppy drive object.
  *
- * @returns COM result indicator
- * @param parent handle of our parent object
- */
-HRESULT FloppyDrive::init (Machine *parent)
-{
-    LogFlowMember (("FloppyDrive::init (%p)\n", parent));
-
-    ComAssertRet (parent, E_INVALIDARG);
-
-    AutoLock alock (this);
-    ComAssertRet (!isReady(), E_UNEXPECTED);
-
-    mParent = parent;
-    // mPeer is left null
+ * @param   aParent  Handle of our parent object.
+ * @return  COM result indicator
+ */
+HRESULT FloppyDrive::init (Machine *aParent)
+{
+    LogFlowThisFunc (("aParent=%p\n", aParent));
+
+    ComAssertRet (aParent, E_INVALIDARG);
+
+    /* Enclose the state transition NotReady->InInit->Ready */
+    AutoInitSpan autoInitSpan (this);
+    AssertReturn (autoInitSpan.isOk(), E_UNEXPECTED);
+
+    unconst (mParent) = aParent;
+    /* mPeer is left null */
 
     mData.allocate();
 
-    setReady (true);
+    /* Confirm a successful initialization */
+    autoInitSpan.setSucceeded();
+
     return S_OK;
 }
@@ -77 +80 @@
  *  @note This object must be destroyed before the original object
  *  it shares data with is destroyed.
- */
-HRESULT FloppyDrive::init (Machine *parent, FloppyDrive *that)
-{
-    LogFlowMember (("FloppyDrive::init (%p, %p)\n", parent, that));
-
-    ComAssertRet (parent && that, E_INVALIDARG);
-
-    AutoLock alock (this);
-    ComAssertRet (!isReady(), E_UNEXPECTED);
-
-    mParent = parent;
-    mPeer = that;
-
-    AutoLock thatlock (that);
-    mData.share (that->mData);
-
-    setReady (true);
+ *
+ *  @note Locks @a aThat object for reading.
+ */
+HRESULT FloppyDrive::init (Machine *aParent, FloppyDrive *aThat)
+{
+    LogFlowThisFunc (("aParent=%p, aThat=%p\n", aParent, aThat));
+
+    ComAssertRet (aParent && aThat, E_INVALIDARG);
+
+    /* Enclose the state transition NotReady->InInit->Ready */
+    AutoInitSpan autoInitSpan (this);
+    AssertReturn (autoInitSpan.isOk(), E_UNEXPECTED);
+
+    unconst (mParent) = aParent;
+    unconst (mPeer) = aThat;
+
+    AutoCaller thatCaller (aThat);
+    AssertComRCReturnRC (thatCaller.rc());
+
+    AutoReaderLock thatLock (aThat);
+    mData.share (aThat->mData);
+
+    /* Confirm a successful initialization */
+    autoInitSpan.setSucceeded();
+
     return S_OK;
 }
@@ -101 +112 @@
  *  (a kind of copy constructor). This object makes a private copy of data
  *  of the original object passed as an argument.
- */
-HRESULT FloppyDrive::initCopy (Machine *parent, FloppyDrive *that)
-{
-    LogFlowMember (("FloppyDrive::initCopy (%p, %p)\n", parent, that));
-
-    ComAssertRet (parent && that, E_INVALIDARG);
-
-    AutoLock alock (this);
-    ComAssertRet (!isReady(), E_UNEXPECTED);
-
-    mParent = parent;
-    // mPeer is left null
-
-    AutoLock thatlock (that);
-    mData.attachCopy (that->mData);
-
-    setReady (true);
+ *
+ *  @note Locks @a aThat object for reading.
+ */
+HRESULT FloppyDrive::initCopy (Machine *aParent, FloppyDrive *aThat)
+{
+    LogFlowThisFunc (("aParent=%p, aThat=%p\n", aParent, aThat));
+
+    ComAssertRet (aParent && aThat, E_INVALIDARG);
+
+    /* Enclose the state transition NotReady->InInit->Ready */
+    AutoInitSpan autoInitSpan (this);
+    AssertReturn (autoInitSpan.isOk(), E_UNEXPECTED);
+
+    unconst (mParent) = aParent;
+    /* mPeer is left null */
+
+    AutoCaller thatCaller (aThat);
+    AssertComRCReturnRC (thatCaller.rc());
+
+    AutoReaderLock thatLock (aThat);
+    mData.attachCopy (aThat->mData);
+
+    /* Confirm a successful initialization */
+    autoInitSpan.setSucceeded();
+
     return S_OK;
 }
@@ -127 +146 @@
 void FloppyDrive::uninit()
 {
-    LogFlowMember (("FloppyDrive::uninit()\n"));
+    LogFlowThisFunc (("\n"));
+
+    /* Enclose the state transition Ready->InUninit->NotReady */
+    AutoUninitSpan autoUninitSpan (this);
+    if (autoUninitSpan.uninitDone())
+        return;
+
+    mData.free();
+
+    unconst (mParent).setNull();
+    unconst (mParent).setNull();
+}
+
+// IFloppyDrive properties
+/////////////////////////////////////////////////////////////////////////////
+
+STDMETHODIMP FloppyDrive::COMGETTER(Enabled) (BOOL *aEnabled)
+{
+    if (!aEnabled)
+        return E_POINTER;
+
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    AutoReaderLock alock (this);
+
+    *aEnabled = mData->mEnabled;
+
+    return S_OK;
+}
+
+STDMETHODIMP FloppyDrive::COMSETTER(Enabled) (BOOL aEnabled)
+{
+    LogFlowThisFunc (("aEnabled=%RTbool\n", aEnabled));
+
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    /* the machine needs to be mutable */
+    Machine::AutoMutableStateDependency adep (mParent);
+    CheckComRCReturnRC (adep.rc());
 
     AutoLock alock (this);
-    AssertReturn (isReady(), (void) 0);
-
-    mData.free();
-
-    mPeer.setNull();
-    mParent.setNull();
-
-    setReady (false);
-}
-
-// IFloppyDrive properties
-/////////////////////////////////////////////////////////////////////////////
-
-STDMETHODIMP FloppyDrive::COMGETTER(Enabled) (BOOL *enabled)
-{
-    if (!enabled)
+
+    if (mData->mEnabled != aEnabled)
+    {
+        mData.backup();
+        mData->mEnabled = aEnabled;
+
+        /* leave the lock before informing callbacks*/
+        alock.unlock();
+
+        mParent->onFloppyDriveChange();
+    }
+
+    return S_OK;
+}
+
+STDMETHODIMP FloppyDrive::COMGETTER(State) (DriveState_T *aDriveState)
+{
+    if (!aDriveState)
         return E_POINTER;
 
-    AutoLock alock(this);
-    CHECK_READY();
-
-    *enabled = mData->mEnabled;
-    return S_OK;
-}
-
-STDMETHODIMP FloppyDrive::COMSETTER(Enabled) (BOOL enabled)
-{
-    LogFlowMember(("FloppyDrive::SetEnabled: %s\n", enabled ? "enable" : "disable"));
-
-    AutoLock alock(this);
-    CHECK_READY();
-
-    CHECK_MACHINE_MUTABILITY (mParent);
-
-    if (mData->mEnabled != enabled)
-    {
-        mData.backup();
-        mData->mEnabled = enabled;
-
-        alock.unlock();
-        mParent->onFloppyDriveChange();
-    }
-
-    return S_OK;
-}
-
-STDMETHODIMP FloppyDrive::COMGETTER(State) (DriveState_T *driveState)
-{
-    if (!driveState)
-        return E_POINTER;
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    AutoReaderLock alock (this);
+
+    *aDriveState = mData->mDriveState;
+
+    return S_OK;
+}
+
+// IFloppyDrive methods
+/////////////////////////////////////////////////////////////////////////////
+
+STDMETHODIMP FloppyDrive::MountImage (INPTR GUIDPARAM aImageId)
+{
+    if (Guid::isEmpty (aImageId))
+        return E_INVALIDARG;
+
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    /* the machine needs to be mutable */
+    Machine::AutoMutableStateDependency adep (mParent);
+    CheckComRCReturnRC (adep.rc());
 
     AutoLock alock (this);
-    CHECK_READY();
-
-    *driveState = mData->mDriveState;
-    return S_OK;
-}
-
-// IFloppyDrive methods
-/////////////////////////////////////////////////////////////////////////////
-
-STDMETHODIMP FloppyDrive::MountImage(INPTR GUIDPARAM imageId)
-{
-    if (Guid::isEmpty(imageId))
-        return E_INVALIDARG;
-
-    AutoLock alock (this);
-    CHECK_READY();
-
-    CHECK_MACHINE_MUTABILITY (mParent);
 
     HRESULT rc = E_FAIL;
 
-    // find the image in the collection
-    ComPtr <IVirtualBox> vbox;
-    rc = mParent->COMGETTER(Parent) (vbox.asOutParam());
-    AssertComRCReturn (rc, rc);
+    /* Our lifetime is bound to mParent's lifetime, so we don't add caller.
+     * We also don't lock mParent since its mParent field is const. */
 
     ComPtr <IFloppyImage> image;
-    rc = vbox->GetFloppyImage (imageId, image.asOutParam());
+    rc = mParent->virtualBox()->GetFloppyImage (aImageId, image.asOutParam());
+
     if (SUCCEEDED (rc))
     {
@@ -216 +250 @@
         {
             mData.backup();
+
             unmount();
+
             mData->mFloppyImage = image;
             mData->mDriveState = DriveState_ImageMounted;
 
+            /* leave the lock before informing callbacks*/
             alock.unlock();
+
             mParent->onFloppyDriveChange();
         }
@@ -228 +266 @@
 }
 
-STDMETHODIMP FloppyDrive::CaptureHostDrive(IHostFloppyDrive *hostFloppyDrive)
-{
-    if (!hostFloppyDrive)
+STDMETHODIMP FloppyDrive::CaptureHostDrive (IHostFloppyDrive *aHostFloppyDrive)
+{
+    if (!aHostFloppyDrive)
         return E_INVALIDARG;
 
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    /* the machine needs to be mutable */
+    Machine::AutoMutableStateDependency adep (mParent);
+    CheckComRCReturnRC (adep.rc());
+
     AutoLock alock (this);
-    CHECK_READY();
-
-    CHECK_MACHINE_MUTABILITY (mParent);
 
     if (mData->mDriveState != DriveState_HostDriveCaptured ||
-        !mData->mHostDrive.equalsTo (hostFloppyDrive))
+        !mData->mHostDrive.equalsTo (aHostFloppyDrive))
     {
         mData.backup();
+
         unmount();
-        mData->mHostDrive = hostFloppyDrive;
+
+        mData->mHostDrive = aHostFloppyDrive;
         mData->mDriveState = DriveState_HostDriveCaptured;
 
+        /* leave the lock before informing callbacks*/
         alock.unlock();
+
         mParent->onFloppyDriveChange();
     }
@@ -255 +301 @@
 STDMETHODIMP FloppyDrive::Unmount()
 {
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    /* the machine needs to be mutable */
+    Machine::AutoMutableStateDependency adep (mParent);
+    CheckComRCReturnRC (adep.rc());
+
     AutoLock alock (this);
-    CHECK_READY();
-
-    CHECK_MACHINE_MUTABILITY (mParent);
 
     if (mData->mDriveState != DriveState_NotMounted)
     {
         mData.backup();
+
         unmount();
+
         mData->mDriveState = DriveState_NotMounted;
 
+        /* leave the lock before informing callbacks*/
         alock.unlock();
+
         mParent->onFloppyDriveChange();
     }
@@ -273 +327 @@
 }
 
-STDMETHODIMP FloppyDrive::GetImage(IFloppyImage **floppyImage)
-{
-    if (!floppyImage)
+STDMETHODIMP FloppyDrive::GetImage (IFloppyImage **aFloppyImage)
+{
+    if (!aFloppyImage)
         return E_POINTER;
 
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    AutoReaderLock alock (this);
+
+    mData->mFloppyImage.queryInterfaceTo (aFloppyImage);
+
+    return S_OK;
+}
+
+STDMETHODIMP FloppyDrive::GetHostDrive (IHostFloppyDrive **aHostDrive)
+{
+    if (!aHostDrive)
+        return E_POINTER;
+
+    AutoCaller autoCaller (this);
+    CheckComRCReturnRC (autoCaller.rc());
+
+    AutoReaderLock alock (this);
+
+    mData->mHostDrive.queryInterfaceTo (aHostDrive);
+
+    return S_OK;
+}
+
+// public methods only for internal purposes
+/////////////////////////////////////////////////////////////////////////////
+
+/** 
+ *  @note Locks this object for writing.
+ */
+bool FloppyDrive::rollback()
+{
+    /* sanity */
+    AutoCaller autoCaller (this);
+    AssertComRCReturn (autoCaller.rc(), false);
+
     AutoLock alock (this);
-    CHECK_READY();
-
-    mData->mFloppyImage.queryInterfaceTo (floppyImage);
-    return S_OK;
-}
-
-STDMETHODIMP FloppyDrive::GetHostDrive(IHostFloppyDrive **hostDrive)
-{
-    if (!hostDrive)
-        return E_POINTER;
-
-    AutoLock alock (this);
-    CHECK_READY();
-
-    mData->mHostDrive.queryInterfaceTo (hostDrive);
-    return S_OK;
-}
-
-// public methods only for internal purposes
-/////////////////////////////////////////////////////////////////////////////
-
-bool FloppyDrive::rollback()
-{
-    AutoLock alock (this);
 
     bool changed = false;
@@ -308 +375 @@
     if (mData.isBackedUp())
     {
-        // we need to check all data to see whether anything will be changed
-        // after rollback
+        /* we need to check all data to see whether anything will be changed
+         * after rollback */
         changed = mData.hasActualChanges();
         mData.rollback();
@@ -317 +384 @@
 }
 
+/** 
+ *  @note Locks this object for writing, together with the peer object (also
+ *  for writing) if there is one.
+ */
 void FloppyDrive::commit()
 {
-    AutoLock alock (this);
+    /* sanity */
+    AutoCaller autoCaller (this);
+    AssertComRCReturnVoid (autoCaller.rc());
+
+    /* sanity too */
+    AutoCaller thatCaller (mPeer);
+    AssertComRCReturnVoid (thatCaller.rc());
+
+    /* lock both for writing since we modify both */
+    AutoMultiLock <2> alock (this->wlock(), AutoLock::maybeWlock (mPeer));
+
     if (mData.isBackedUp())
     {
@@ -325 +406 @@
         if (mPeer)
         {
-            // attach new data to the peer and reshare it
-            AutoLock peerlock (mPeer);
+            /* attach new data to the peer and reshare it */
             mPeer->mData.attach (mData);
         }
@@ -332 +412 @@
 }
 
+/** 
+ *  @note Locks this object for writing, together with the peer object (locked
+ *  for reading) if there is one.
+ */
 void FloppyDrive::copyFrom (FloppyDrive *aThat)
 {
-    AutoLock alock (this);
-
-    // this will back up current data
+    /* sanity */
+    AutoCaller autoCaller (this);
+    AssertComRCReturnVoid (autoCaller.rc());
+
+    /* sanity too */
+    AutoCaller thatCaller (mPeer);
+    AssertComRCReturnVoid (thatCaller.rc());
+
+    /* peer is not modified, lock it for reading */
+    AutoMultiLock <2> alock (this->wlock(), AutoLock::maybeRlock (mPeer));
+
+    /* this will back up current data */
     mData.assignCopy (aThat->mData);
 }
@@ -344 +437 @@
 
 /**
- * Helper to unmount a drive
- *
- * @returns COM status code
- *
+ *  Helper to unmount a drive.
+ *
+ *  @return COM status code
  */
 HRESULT FloppyDrive::unmount()
 {
+    AssertReturn (isLockedOnCurrentThread(), E_FAIL);
+
     if (mData->mFloppyImage)
         mData->mFloppyImage.setNull();
     if (mData->mHostDrive)
         mData->mHostDrive.setNull();
-    return S_OK;
-}
+
+    return S_OK;
+}

trunk/src/VBox/Main/VirtualBoxImpl.cpp

@@ -3164 +3164 @@
  *  is returned.
  *
- *  @param id
+ *  @param aId
  *      ID of the DVD image (unused when NULL)
- *  @param filePathFull
+ *  @param aFilePathFull
  *      full path to the image file (unused when NULL)
  *  @param aSetError
  *      if TRUE, the appropriate error info is set in case when the image is not
  *      found and only one search criteria (ID or file name) is specified.
- *  @param dvdImage
+ *  @param aImage
  *      where to store the found DVD image object (can be NULL)
  *

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

@@ -28 +28 @@
 
 class ATL_NO_VTABLE DVDDrive :
+    public VirtualBoxBaseNEXT,
     public VirtualBoxSupportErrorInfoImpl <DVDDrive, IDVDDrive>,
     public VirtualBoxSupportTranslation <DVDDrive>,
-    public VirtualBoxBase,
     public IDVDDrive
 {
@@ -56 +56 @@
     };
 
+    VIRTUALBOXBASE_ADD_ERRORINFO_SUPPORT (DVDDrive)
+
     DECLARE_NOT_AGGREGATABLE(DVDDrive)
 
@@ -67 +69 @@
     NS_DECL_ISUPPORTS
 
+    DECLARE_EMPTY_CTOR_DTOR (DVDDrive)
+
     HRESULT FinalConstruct();
     void FinalRelease();
 
     // public initializer/uninitializer for internal purposes only
-    HRESULT init (Machine *parent);
-    HRESULT init (Machine *parent, DVDDrive *that);
-    HRESULT initCopy (Machine *parent, DVDDrive *that);
+    HRESULT init (Machine *aParent);
+    HRESULT init (Machine *aParent, DVDDrive *aThat);
+    HRESULT initCopy (Machine *aParent, DVDDrive *aThat);
     void uninit();
 
     // IDVDDrive properties
-    STDMETHOD(COMGETTER(State)) (DriveState_T *driveState);
-    STDMETHOD(COMGETTER(Passthrough)) (BOOL *passthrough);
-    STDMETHOD(COMSETTER(Passthrough)) (BOOL passthrough);
+    STDMETHOD(COMGETTER(State)) (DriveState_T *aDriveState);
+    STDMETHOD(COMGETTER(Passthrough)) (BOOL *aPassthrough);
+    STDMETHOD(COMSETTER(Passthrough)) (BOOL aPassthrough);
 
     // IDVDDrive methods
-    STDMETHOD(MountImage)(INPTR GUIDPARAM imageId);
-    STDMETHOD(CaptureHostDrive)(IHostDVDDrive *hostDVDDrive);
+    STDMETHOD(MountImage) (INPTR GUIDPARAM aImageId);
+    STDMETHOD(CaptureHostDrive) (IHostDVDDrive *aHostDVDDrive);
     STDMETHOD(Unmount)();
-    STDMETHOD(GetImage)(IDVDImage **dvdImage);
-    STDMETHOD(GetHostDrive)(IHostDVDDrive **hostDVDDrive);
+    STDMETHOD(GetImage) (IDVDImage **aDVDImage);
+    STDMETHOD(GetHostDrive) (IHostDVDDrive **aHostDVDDrive);
 
     // public methods only for internal purposes
@@ -95 +99 @@
     void commit();
     void copyFrom (DVDDrive *aThat);
+
+    // public methods for internal purposes only
+    // (ensure there is a caller and a read lock before calling them!)
 
     Backupable <Data> &data() { return mData; }
@@ -105 +112 @@
     HRESULT unmount();
 
-    ComObjPtr <Machine, ComWeakRef> mParent;
-    ComObjPtr <DVDDrive> mPeer;
+    const ComObjPtr <Machine, ComWeakRef> mParent;
+    const ComObjPtr <DVDDrive> mPeer;
+
     Backupable <Data> mData;
 };

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

@@ -28 +28 @@
 
 class ATL_NO_VTABLE FloppyDrive :
+    public VirtualBoxBaseNEXT,
     public VirtualBoxSupportErrorInfoImpl <FloppyDrive, IFloppyDrive>,
     public VirtualBoxSupportTranslation <FloppyDrive>,
-    public VirtualBoxBase,
     public IFloppyDrive
 {
@@ -57 +57 @@
     };
 
+    VIRTUALBOXBASE_ADD_ERRORINFO_SUPPORT (FloppyDrive)
+
     DECLARE_NOT_AGGREGATABLE(FloppyDrive)
 
@@ -68 +70 @@
     NS_DECL_ISUPPORTS
 
+    DECLARE_EMPTY_CTOR_DTOR (FloppyDrive)
+
     HRESULT FinalConstruct();
     void FinalRelease();
 
     // public initializer/uninitializer for internal purposes only
-    HRESULT init (Machine *parent);
-    HRESULT init (Machine *parent, FloppyDrive *that);
-    HRESULT initCopy (Machine *parent, FloppyDrive *that);
+    HRESULT init (Machine *aParent);
+    HRESULT init (Machine *aParent, FloppyDrive *aThat);
+    HRESULT initCopy (Machine *parent, FloppyDrive *aThat);
     void uninit();
 
     // IFloppyDrive properties
-    STDMETHOD(COMGETTER(Enabled)) (BOOL *enabled);
-    STDMETHOD(COMSETTER(Enabled)) (BOOL enabled);
-    STDMETHOD(COMGETTER(State)) (DriveState_T *driveState);
+    STDMETHOD(COMGETTER(Enabled)) (BOOL *aEnabled);
+    STDMETHOD(COMSETTER(Enabled)) (BOOL aEnabled);
+    STDMETHOD(COMGETTER(State)) (DriveState_T *aDriveState);
 
     // IFloppyDrive methods
-    STDMETHOD(MountImage)(INPTR GUIDPARAM imageId);
-    STDMETHOD(CaptureHostDrive)(IHostFloppyDrive *hostFloppyDrive);
+    STDMETHOD(MountImage) (INPTR GUIDPARAM aImageId);
+    STDMETHOD(CaptureHostDrive) (IHostFloppyDrive *aHostFloppyDrive);
     STDMETHOD(Unmount)();
-    STDMETHOD(GetImage)(IFloppyImage **floppyImage);
-    STDMETHOD(GetHostDrive)(IHostFloppyDrive **hostFloppyDrive);
+    STDMETHOD(GetImage) (IFloppyImage **aFloppyImage);
+    STDMETHOD(GetHostDrive) (IHostFloppyDrive **aHostFloppyDrive);
 
     // public methods only for internal purposes
@@ -96 +100 @@
     void commit();
     void copyFrom (FloppyDrive *aThat);
+
+    // public methods for internal purposes only
+    // (ensure there is a caller and a read lock before calling them!)
 
     Backupable <Data> &data() { return mData; }
@@ -106 +113 @@
     HRESULT unmount();
 
-    ComObjPtr <Machine, ComWeakRef> mParent;
-    ComObjPtr <FloppyDrive> mPeer;
+    const ComObjPtr <Machine, ComWeakRef> mParent;
+    const ComObjPtr <FloppyDrive> mPeer;
+
     Backupable <Data> mData;
 };