Changeset 15334 in vbox for trunk/src/VBox/Main

Timestamp:

Dec 11, 2008 7:37:55 PM (16 years ago)

Author:

vboxsync

Message:

Main: #3377: Implicitly detach DVD/Floppy from an unregistered machine (as opposed to hard disks, this does not have side effects).

Location:

trunk/src/VBox/Main

Files:

• DVDDriveImpl.cpp (modified) (10 diffs)
• FloppyDriveImpl.cpp (modified) (10 diffs)
• MachineImpl.cpp (modified) (2 diffs)
• include/DVDDriveImpl.h (modified) (2 diffs)
• include/FloppyDriveImpl.h (modified) (2 diffs)

trunk/src/VBox/Main/DVDDriveImpl.cpp

@@ -275 +275 @@
             if (SUCCEEDED (rc))
             {
-                mData.backup();
-
-                unmount();
-
-                mData->mImage = image;
-                mData->mState = DriveState_ImageMounted;
-
-                /* leave the lock before informing callbacks */
-                alock.unlock();
-
-                mParent->onDVDDriveChange();
+                /* umount() will backup data */
+                rc = unmount();
+                if (SUCCEEDED (rc))
+                {
+                    mData->mImage = image;
+                    mData->mState = DriveState_ImageMounted;
+
+                    /* leave the lock before informing callbacks */
+                    alock.unlock();
+
+                    mParent->onDVDDriveChange();
+                }
             }
         }
@@ -309 +310 @@
         !mData->mHostDrive.equalsTo (aHostDVDDrive))
     {
-        mData.backup();
-
-        unmount();
-
-        mData->mHostDrive = aHostDVDDrive;
-        mData->mState = DriveState_HostDriveCaptured;
-
-        /* leave the lock before informing callbacks */
-        alock.unlock();
-
-        mParent->onDVDDriveChange();
+        /* umount() will backup data */
+        HRESULT rc = unmount();
+        if (SUCCEEDED (rc))
+        {
+            mData->mHostDrive = aHostDVDDrive;
+            mData->mState = DriveState_HostDriveCaptured;
+
+            /* leave the lock before informing callbacks */
+            alock.unlock();
+
+            mParent->onDVDDriveChange();
+        }
     }
 
@@ -338 +340 @@
     if (mData->mState != DriveState_NotMounted)
     {
-        mData.backup();
-
-        unmount();
-
-        mData->mState = DriveState_NotMounted;
-
-        /* leave the lock before informing callbacks */
-        alock.unlock();
-
-        mParent->onDVDDriveChange();
+        /* umount() will backup data */
+        HRESULT rc = unmount();
+        if (SUCCEEDED (rc))
+        {
+            mData->mState = DriveState_NotMounted;
+
+            /* leave the lock before informing callbacks */
+            alock.unlock();
+
+            mParent->onDVDDriveChange();
+        }
     }
 
@@ -385 +388 @@
 
 /**
- *  Loads settings from the given machine node.
- *  May be called once right after this object creation.
- *
- *  @param aMachineNode <Machine> node.
- *
- *  @note Locks this object for writing.
+ * Loads settings from the given machine node. May be called once right after
+ * this object creation.
+ *
+ * @param aMachineNode  <Machine> node.
+ *
+ * @note Locks this object for writing.
  */
 HRESULT DVDDrive::loadSettings (const settings::Key &aMachineNode)
@@ -469 +472 @@
 
 /**
- *  Saves settings to the given machine node.
- *
- *  @param aMachineNode <Machine> node.
- *
- *  @note Locks this object for reading.
+ * Saves settings to the given machine node.
+ *
+ * @param aMachineNode  <Machine> node.
+ *
+ * @note Locks this object for reading.
  */
 HRESULT DVDDrive::saveSettings (settings::Key &aMachineNode)
@@ -530 +533 @@
 
 /**
- *  @note Locks this object for writing.
+ * @note Locks this object for writing.
  */
 bool DVDDrive::rollback()
@@ -567 +570 @@
 
 /**
- *  @note Locks this object for writing, together with the peer object (also
- *  for writing) if there is one.
+ * @note Locks this object for writing, together with the peer object (also for
+ *       writing) if there is one.
  */
 void DVDDrive::commit()
@@ -605 +608 @@
 
 /**
- *  @note Locks this object for writing, together with the peer object
- *  represented by @a aThat (locked for reading).
+ * @note Locks this object for writing, together with the peer object
+ *       represented by @a aThat (locked for reading).
  */
 void DVDDrive::copyFrom (DVDDrive *aThat)
@@ -628 +631 @@
 }
 
-// private methods
-////////////////////////////////////////////////////////////////////////////////
-
-/**
- *  Helper to unmount a drive.
- *
- *  @return COM status code
- *
+/**
+ * Helper to unmount a drive.
+ *
+ * @note Must be called from under this object's write lock.
  */
 HRESULT DVDDrive::unmount()
 {
     AssertReturn (isWriteLockOnCurrentThread(), E_FAIL);
+
+    mData.backup();
 
     if (mData->mImage)
@@ -646 +647 @@
         mData->mHostDrive.setNull();
 
-    return S_OK;
-}
+    mData->mState = DriveState_NotMounted;
+
+    return S_OK;
+}
+
+// private methods
+////////////////////////////////////////////////////////////////////////////////
+
 /* vi: set tabstop=4 shiftwidth=4 expandtab: */

trunk/src/VBox/Main/FloppyDriveImpl.cpp

@@ -282 +282 @@
             if (SUCCEEDED (rc))
             {
-                mData.backup();
-
-                unmount();
-
-                mData->mImage = image;
-                mData->mState = DriveState_ImageMounted;
-
-                /* leave the lock before informing callbacks */
-                alock.unlock();
-
-                mParent->onFloppyDriveChange();
+                /* umount() will backup data */
+                rc = unmount();
+                if (SUCCEEDED (rc))
+                {
+                    mData->mImage = image;
+                    mData->mState = DriveState_ImageMounted;
+
+                    /* leave the lock before informing callbacks */
+                    alock.unlock();
+
+                    mParent->onFloppyDriveChange();
+                }
             }
         }
@@ -316 +317 @@
         !mData->mHostDrive.equalsTo (aHostFloppyDrive))
     {
-        mData.backup();
-
-        unmount();
-
-        mData->mHostDrive = aHostFloppyDrive;
-        mData->mState = DriveState_HostDriveCaptured;
-
-        /* leave the lock before informing callbacks */
-        alock.unlock();
-
-        mParent->onFloppyDriveChange();
+        /* umount() will backup data */
+        HRESULT rc = unmount();
+        if (SUCCEEDED (rc))
+        {
+            mData->mHostDrive = aHostFloppyDrive;
+            mData->mState = DriveState_HostDriveCaptured;
+
+            /* leave the lock before informing callbacks */
+            alock.unlock();
+
+            mParent->onFloppyDriveChange();
+        }
     }
 
@@ -345 +347 @@
     if (mData->mState != DriveState_NotMounted)
     {
-        mData.backup();
-
-        unmount();
-
-        mData->mState = DriveState_NotMounted;
-
-        /* leave the lock before informing callbacks */
-        alock.unlock();
-
-        mParent->onFloppyDriveChange();
+        /* umount() will backup data */
+        HRESULT rc = unmount();
+        if (SUCCEEDED (rc))
+        {
+            mData->mState = DriveState_NotMounted;
+
+            /* leave the lock before informing callbacks */
+            alock.unlock();
+
+            mParent->onFloppyDriveChange();
+        }
     }
 
@@ -392 +395 @@
 
 /**
- *  Loads settings from the given machine node.
- *  May be called once right after this object creation.
- *
- *  @param aMachineNode <Machine> node.
- *
- *  @note Locks this object for writing.
+ * Loads settings from the given machine node. May be called once right after
+ * this object creation.
+ *
+ * @param aMachineNode  <Machine> node.
+ *
+ * @note Locks this object for writing.
  */
 HRESULT FloppyDrive::loadSettings (const settings::Key &aMachineNode)
@@ -476 +479 @@
 
 /**
- *  Saves settings to the given machine node.
- *
- *  @param aMachineNode <Machine> node.
- *
- *  @note Locks this object for reading.
+ * Saves settings to the given machine node.
+ *
+ * @param aMachineNode  <Machine> node.
+ *
+ * @note Locks this object for reading.
  */
 HRESULT FloppyDrive::saveSettings (settings::Key &aMachineNode)
@@ -537 +540 @@
 
 /**
- *  @note Locks this object for writing.
+ * @note Locks this object for writing.
  */
 bool FloppyDrive::rollback()
@@ -574 +577 @@
 
 /**
- *  @note Locks this object for writing, together with the peer object (also
- *  for writing) if there is one.
+ * @note Locks this object for writing, together with the peer object (also for
+ *       writing) if there is one.
  */
 void FloppyDrive::commit()
@@ -612 +615 @@
 
 /**
- *  @note Locks this object for writing, together with the peer object (locked
- *  for reading) if there is one.
+ * @note Locks this object for writing, together with the peer object (locked
+ *       for reading) if there is one.
  */
 void FloppyDrive::copyFrom (FloppyDrive *aThat)
@@ -633 +636 @@
 }
 
-// private methods
-/////////////////////////////////////////////////////////////////////////////
-
-/**
- *  Helper to unmount a drive.
- *
- *  @return COM status code
+/**
+ * Helper to unmount a drive.
+ *
+ * @note Must be called from under this object's write lock.
  */
 HRESULT FloppyDrive::unmount()
 {
     AssertReturn (isWriteLockOnCurrentThread(), E_FAIL);
+
+    mData.backup();
 
     if (mData->mImage)
@@ -650 +652 @@
         mData->mHostDrive.setNull();
 
-    return S_OK;
-}
+    mData->mState = DriveState_NotMounted;
+
+    return S_OK;
+}
+
+// private methods
+/////////////////////////////////////////////////////////////////////////////
+
 /* vi: set tabstop=4 shiftwidth=4 expandtab: */

trunk/src/VBox/Main/MachineImpl.cpp

@@ -4102 +4102 @@
     AssertReturn (autoCaller.state() == Ready, E_FAIL);
 
+    /* we will probably modify these and want to prevent concurrent
+     * modifications until we finish */
+    AutoWriteLock dvdLock (mDVDDrive);
+    AutoWriteLock floppyLock (mFloppyDrive);
+
     if (aRegistered)
     {
@@ -4138 +4143 @@
                     "has %d hard disks attached"),
                 mUserData->mName.raw(), mHDData->mAttachments.size());
-    }
+
+        /* Note that we do not prevent unregistration of a DVD or Floppy image
+         * is attached: as opposed to hard disks detaching such an image
+         * implicitly in this method (which we will do below) won't have any
+         * side effects (like detached orphan base and diff hard disks etc).*/
+    }
+
+    HRESULT rc = S_OK;
 
     /* Ensure the settings are saved. If we are going to be registered and
      * isConfigLocked() is FALSE then it means that no config file exists yet,
-     * so create it. */
+     * so create it by calling saveSettings() too. */
     if (isModified() || (aRegistered && !isConfigLocked()))
     {
-        HRESULT rc = saveSettings();
+        rc = saveSettings();
         CheckComRCReturnRC (rc);
     }
 
-    mData->mRegistered = aRegistered;
-
-    return S_OK;
+    /* Implicitly detach DVD/Floppy */
+    rc = mDVDDrive->unmount();
+    if (SUCCEEDED (rc))
+        rc = mFloppyDrive->unmount();
+
+    if (SUCCEEDED (rc))
+    {
+        /* we may have had implicit modifications we want to fix on success */
+        commit();
+
+        mData->mRegistered = aRegistered;
+    }
+    else
+    {
+        /* we may have had implicit modifications we want to cancel on failure*/
+        rollback (false /* aNotify */);
+    }
+
+    return rc;
 }
 

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

@@ -108 +108 @@
     void copyFrom (DVDDrive *aThat);
 
+    HRESULT unmount();
+
     // public methods for internal purposes only
     // (ensure there is a caller and a read lock before calling them!)
@@ -118 +120 @@
 private:
 
-    HRESULT unmount();
-
     const ComObjPtr <Machine, ComWeakRef> mParent;
     const ComObjPtr <DVDDrive> mPeer;

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

@@ -108 +108 @@
     void copyFrom (FloppyDrive *aThat);
 
+    HRESULT unmount();
+
     // public methods for internal purposes only
     // (ensure there is a caller and a read lock before calling them!)
@@ -118 +120 @@
 private:
 
-    HRESULT unmount();
-
     const ComObjPtr <Machine, ComWeakRef> mParent;
     const ComObjPtr <FloppyDrive> mPeer;