Changeset 23441 in vbox for trunk/src

Timestamp:

Sep 30, 2009 12:52:22 PM (15 years ago)

Author:

vboxsync

Message:

Main/MachineImpl: fix subtly broken snapshot creation (attachments without medium were omitted, resulting in a broken VM config after saving state, and triggering consistency check assertions when reverting to current snapshot), and also fix ISO image locking when starting a VM

File:

• trunk/src/VBox/Main/MachineImpl.cpp (modified) (17 diffs)

trunk/src/VBox/Main/MachineImpl.cpp

@@ -2079 +2079 @@
     {
         return setError(VBOX_E_OBJECT_IN_USE,
-                        tr("Hard disk '%ls' is already attached to this virtual machine"),
+                        tr("Medium '%ls' is already attached to this virtual machine"),
                         medium->locationFull().raw());
     }
@@ -2308 +2308 @@
     if (associate && !medium.isNull())
     {
-        /* as the last step, associate the hard disk to the VM */
+        /* as the last step, associate the medium to the VM */
         rc = medium->attachTo(mData->mUuid);
         /* here we can fail because of Deleting, or being in process of
@@ -5476 +5476 @@
         }
 
-        /* backup mHDData to let registeredInit() properly rollback on failure
+        /* backup mMediaData to let registeredInit() properly rollback on failure
          * (= limited accessibility) */
 
@@ -6329 +6329 @@
  * Used when taking a snapshot or when discarding the current state.
  *
- * This method assumes that mHDData contains the original hard disk attachments
+ * This method assumes that mMediaData contains the original hard disk attachments
  * it needs to create diffs for. On success, these attachments will be replaced
  * with the created diffs. On failure, #deleteImplicitDiffs() is implicitly
- * called to delete created diffs which will also rollback mHDData and restore
+ * called to delete created diffs which will also rollback mMediaData and restore
  * whatever was backed up before calling this method.
  *
@@ -6369 +6369 @@
 
     /* must be in a protective state because we leave the lock below */
-    AssertReturn(mData->mMachineState == MachineState_Saving ||
-                  mData->mMachineState == MachineState_Discarding, E_FAIL);
+    AssertReturn(   mData->mMachineState == MachineState_Saving
+                 || mData->mMachineState == MachineState_Discarding, E_FAIL);
 
     HRESULT rc = S_OK;
@@ -6400 +6400 @@
 
         /* remember the current list (note that we don't use backup() since
-         * mHDData may be already backed up) */
+         * mMediaData may be already backed up) */
         MediaData::AttachmentList atts = mMediaData->mAttachments;
 
@@ -6413 +6413 @@
         {
             MediumAttachment* pAtt = *it;
-            if (pAtt->type() != DeviceType_HardDisk)
-                continue;
-
-            Medium* pHD = pAtt->medium();
-            Assert(pHD);
-            /* type cannot be changed while attached => no need to lock */
-            if (pHD->type() != MediumType_Normal)
+
+            DeviceType_T devType = pAtt->type();
+            Medium* medium = pAtt->medium();
+
+            if (   devType != DeviceType_HardDisk
+                || medium == NULL
+                || medium->type() != MediumType_Normal)
             {
                 /* copy the attachment as is */
-                Assert(pHD->type() == MediumType_Writethrough);
-                aProgress->SetNextOperation(BstrFmt(tr("Skipping medium '%s'"),
-                                                    pHD->base()->name().raw()),
-                                            aWeight);        // weight
+
+                /** @todo the progress object created in Console::TakeSnaphot
+                 * only expects operations for hard disks. Later other
+                 * device types need to show up in the progress as well. */
+                if (devType == DeviceType_HardDisk)
+                {
+                    if (medium == NULL)
+                        aProgress->SetNextOperation(Bstr(tr("Skipping attachment without medium")),
+                                                    aWeight);        // weight
+                    else
+                        aProgress->SetNextOperation(BstrFmt(tr("Skipping medium '%s'"),
+                                                            medium->base()->name().raw()),
+                                                    aWeight);        // weight
+                }
 
                 mMediaData->mAttachments.push_back(pAtt);
@@ -6433 +6443 @@
             /* need a diff */
             aProgress->SetNextOperation(BstrFmt(tr("Creating differencing hard disk for '%s'"),
-                                                pHD->base()->name().raw()),
+                                                medium->base()->name().raw()),
                                         aWeight);        // weight
 
@@ -6439 +6449 @@
             diff.createObject();
             rc = diff->init(mParent,
-                            pHD->preferredDiffFormat().raw(),
+                            medium->preferredDiffFormat().raw(),
                             BstrFmt("%ls"RTPATH_SLASH_STR,
                                     mUserData->mSnapshotFolderFull.raw()).raw());
@@ -6447 +6457 @@
             alock.leave();
 
-            rc = pHD->createDiffStorageAndWait(diff,
-                                               MediumVariant_Standard,
-                                               NULL);
+            rc = medium->createDiffStorageAndWait(diff,
+                                                  MediumVariant_Standard,
+                                                  NULL);
 
             // at this point, the old image is still locked for writing, but instead
@@ -6458 +6468 @@
                 diff->LockWrite(NULL);
                 mData->mSession.mLockedMedia.push_back(Data::Session::LockedMedia::value_type(ComPtr<IMedium>(diff), true));
-                pHD->UnlockWrite(NULL);
-                pHD->LockRead(NULL);
-                mData->mSession.mLockedMedia.push_back(Data::Session::LockedMedia::value_type(ComPtr<IMedium>(pHD), false));
+                medium->UnlockWrite(NULL);
+                medium->LockRead(NULL);
+                mData->mSession.mLockedMedia.push_back(Data::Session::LockedMedia::value_type(ComPtr<IMedium>(medium), false));
             }
 
@@ -6513 +6523 @@
 /**
  * Deletes implicit differencing hard disks created either by
- * #createImplicitDiffs() or by #AttachMedium() and rolls back mHDData.
+ * #createImplicitDiffs() or by #AttachMedium() and rolls back mMediaData.
  *
  * Note that to delete hard disks created by #AttachMedium() this method is
@@ -6688 +6698 @@
  * created diffs on failure.
  *
- * Does nothing if the hard disk attachment data (mHDData) is not changed (not
+ * Does nothing if the hard disk attachment data (mMediaData) is not changed (not
  * backed up).
  *
- * When the data is backed up, this method will commit mHDData if @a aCommit is
+ * When the data is backed up, this method will commit mMediaData if @a aCommit is
  * @c true and rollback it otherwise before returning.
  *
@@ -7257 +7267 @@
         mPeer->mUserData.attach (mUserData);
         mPeer->mHWData.attach (mHWData);
-        /* mHDData is reshared by fixupMedia */
-        // mPeer->mHDData.attach (mHDData);
+        /* mMediaData is reshared by fixupMedia */
+        // mPeer->mMediaData.attach(mMediaData);
         Assert(mPeer->mMediaData.data() == mMediaData.data());
     }
@@ -9340 +9350 @@
     {
         /* delete all differencing hard disks created (this will also attach
-         * their parents back by rolling back mHDData) */
+         * their parents back by rolling back mMediaData) */
         fixupMedia(false /* aCommit */);
 
@@ -10037 +10047 @@
         HRESULT rc = S_OK;
 
-        /* lock hard disks */
+        /* lock all medium objects attached to the VM */
         for (MediaData::AttachmentList::const_iterator it = mMediaData->mAttachments.begin();
              it != mMediaData->mAttachments.end();
              ++it)
         {
+            DeviceType_T devType = (*it)->type();
             ComObjPtr<Medium> hd = (*it)->medium();
 
@@ -10052 +10063 @@
                 if (first)
                 {
-                    rc = hd->LockWrite (&mediaState);
-                    CheckComRCThrowRC(rc);
+                    if (devType != DeviceType_DVD)
+                    {
+                        /* HardDisk and Floppy medium must be locked for writing */
+                        rc = hd->LockWrite(&mediaState);
+                        CheckComRCThrowRC(rc);
+                    }
+                    else
+                    {
+                        /* DVD medium must be locked for reading */
+                        rc = hd->LockRead(&mediaState);
+                        CheckComRCThrowRC(rc);
+                    }
 
                     mData->mSession.mLockedMedia.push_back (