Changeset 43915 in vbox for trunk/src/VBox/Main/src-server/MachineImpl.cpp

Timestamp:

Nov 19, 2012 7:07:10 PM (12 years ago)

Author:

vboxsync

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

82197

Message:

Main/Machine+Snapshot+Medium: fix cancelling snapshots, used to trigger both backref inconsistency and incorrect lock list/map updates which could cause runtime misbehavior, the config file was already made consistent with the previous changes

File:

• trunk/src/VBox/Main/src-server/MachineImpl.cpp (modified) (24 diffs)

trunk/src/VBox/Main/src-server/MachineImpl.cpp

@@ -1703 +1703 @@
 
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-    if(strFile.isEmpty())
+    if (strFile.isEmpty())
        strFile = "VideoCap.webm";
     mHWData->mVideoCaptureFile = strFile;
@@ -8018 +8018 @@
     }
 
-    /* Deassociate hard disks (only when a real Machine or a SnapshotMachine
+    /* Deassociate media (only when a real Machine or a SnapshotMachine
      * instance is uninitialized; SessionMachine instances refer to real
-     * Machine hard disks). This is necessary for a clean re-initialization of
+     * Machine media). This is necessary for a clean re-initialization of
      * the VM after successfully re-checking the accessibility state. Note
      * that in case of normal Machine or SnapshotMachine uninitialization (as
-     * a result of unregistering or deleting the snapshot), outdated hard
-     * disk attachments will already be uninitialized and deleted, so this
+     * a result of unregistering or deleting the snapshot), outdated media
+     * attachments will already be uninitialized and deleted, so this
      * code will not affect them. */
     if (    !!mMediaData
@@ -8034 +8034 @@
              ++it)
         {
-            ComObjPtr<Medium> hd = (*it)->getMedium();
-            if (hd.isNull())
+            ComObjPtr<Medium> pMedium = (*it)->getMedium();
+            if (pMedium.isNull())
                 continue;
-            HRESULT rc = hd->removeBackReference(mData->mUuid, getSnapshotId());
+            HRESULT rc = pMedium->removeBackReference(mData->mUuid, getSnapshotId());
             AssertComRC(rc);
         }
@@ -10130 +10130 @@
  *       on failure. This is a responsibility of the caller.
  *
- * @note Locks this object for writing.
+ * @note Locks this object and the media tree for writing.
  */
 HRESULT Machine::createImplicitDiffs(IProgress *aProgress,
@@ -10153 +10153 @@
     HRESULT rc = S_OK;
 
+    // use appropriate locked media map (online or offline)
     MediumLockListMap lockedMediaOffline;
     MediumLockListMap *lockedMediaMap;
@@ -10242 +10243 @@
                     else
                         aProgress->SetNextOperation(BstrFmt(tr("Skipping medium '%s'"),
-                                                    pMedium->getBase()->getName().c_str()).raw(),
+                                                            pMedium->getBase()->getName().c_str()).raw(),
                                                     aWeight);        // weight
                 }
@@ -10284 +10285 @@
             {
                 alock.release();
+                /* The currently attached medium will be read-only, change
+                 * the lock type to read. */
                 rc = pMediumLockList->Update(pMedium, false);
                 alock.acquire();
                 AssertComRCThrowRC(rc);
             }
-            rc = lockedMediaMap->Get(pAtt, pMediumLockList);
-            AssertComRCThrowRC(rc);
 
             /* release the locks before the potentially lengthy operation */
@@ -10338 +10339 @@
     catch (HRESULT aRC) { rc = aRC; }
 
-
-    /* unlock all hard disks we locked */
+    /* unlock all hard disks we locked when there is no VM */
     if (!aOnline)
     {
@@ -10347 +10347 @@
         AssertComRC(rc1);
     }
-
-
-    if (FAILED(rc))
-    {
-        MultiResult mrc = rc;
-
-        alock.release();
-        mrc = deleteImplicitDiffs(aOnline);
-    }
-
-    if (aOnline)
-        mData->mSession.mLockedMedia =  *lockedMediaMap;
-    else
-        lockedMediaOffline = *lockedMediaMap;
 
     return rc;
@@ -10372 +10358 @@
  * called from #fixupMedia() when the changes are rolled back.
  *
- * @note Locks this object for writing.
+ * @note Locks this object and the media tree for writing.
  */
-// Tod - deal withh offline atts .....
-HRESULT Machine::deleteImplicitDiffs(bool      aOnline)
+HRESULT Machine::deleteImplicitDiffs(bool aOnline)
 {
     LogFlowThisFunc(("aOnline=%d\n", aOnline));
@@ -10382 +10367 @@
     AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
-    AutoWriteLock alock(this  COMMA_LOCKVAL_SRC_POS);
-    AutoMultiWriteLock2 alock2(this->lockHandle(),
+    AutoMultiWriteLock2 alock(this->lockHandle(),
                               &mParent->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
-    /* Don't want backed up state. */
+    /* We absolutely must have backed up state. */
     AssertReturn(mMediaData.isBackedUp(), E_FAIL);
 
@@ -10392 +10376 @@
     MachineState_T oldState = mData->mMachineState;
 
-    MediaData::AttachmentList implicitAtts;
-    const MediaData::AttachmentList &oldAtts = mMediaData.backedUpData()->mAttachments;
-
-    // Locked Media Map.
+    /* will release the lock before the potentially lengthy operation,
+     * so protect with the special state (unless already protected) */
+    if (   oldState != MachineState_Saving
+        && oldState != MachineState_LiveSnapshotting
+        && oldState != MachineState_RestoringSnapshot
+        && oldState != MachineState_DeletingSnapshot
+        && oldState != MachineState_DeletingSnapshotOnline
+        && oldState != MachineState_DeletingSnapshotPaused
+       )
+        setMachineState(MachineState_SettingUp);
+
+    // use appropriate locked media map (online or offline)
     MediumLockListMap lockedMediaOffline;
     MediumLockListMap *lockedMediaMap;
@@ -10405 +10397 @@
     try
     {
-        // create Medium lock lists for offline atts as per createImplicitAtts.
         if (!aOnline)
         {
@@ -10441 +10432 @@
 
             if (FAILED(rc))
-                throw(rc);
-
+                throw rc;
         } // end of offline
 
-
-        /* Start from scratch */
-
-        /* Go through remembered attachments and create diffs for normal hard
-         * disks and attach them */
+        /* Lock lists are now up to date and include implicitly created media */
+
+        /* Go through remembered attachments and delete all implicitly created
+         * diffs and fix up the attachment information */
+        const MediaData::AttachmentList &oldAtts = mMediaData.backedUpData()->mAttachments;
+        MediaData::AttachmentList implicitAtts;
         for (MediaData::AttachmentList::const_iterator it = mMediaData->mAttachments.begin();
              it != mMediaData->mAttachments.end();
              ++it)
         {
-            MediumAttachment* pAtt = *it;
-
-            //  Ensure we get a hard disk.
-            ComObjPtr<Medium> hd = pAtt->getMedium();
-
-            if (hd.isNull())
+            ComObjPtr<MediumAttachment> pAtt = *it;
+            ComObjPtr<Medium> pMedium = pAtt->getMedium();
+            if (pMedium.isNull())
                 continue;
 
-            // DeviceType_T devType = pAtt->getType();
-
-            // Implicit atts go on the list of implicit atts for deletion and back reference gets removed.
+            // Implicit attachments go on the list for deletion and back references are removed.
             if (pAtt->isImplicit())
             {
                 /* Deassociate and mark for deletion */
                 LogFlowThisFunc(("Detaching '%s', pending deletion\n", pAtt->getLogName()));
-                rc = hd->removeBackReference(mData->mUuid);
+                rc = pMedium->removeBackReference(mData->mUuid);
                 if (FAILED(rc))
-                   throw(rc);
+                   throw rc;
                 implicitAtts.push_back(pAtt);
                 continue;
             }
 
-            /* Was this hard disk attached before? */
-            if (!findAttachment(oldAtts, hd))
+            /* Was this medium attached before? */
+            if (!findAttachment(oldAtts, pMedium))
             {
                 /* no: de-associate */
                 LogFlowThisFunc(("Detaching '%s', no deletion\n", pAtt->getLogName()));
-                rc = hd->removeBackReference(mData->mUuid);
+                rc = pMedium->removeBackReference(mData->mUuid);
                 if (FAILED(rc))
-                    throw setError(rc);
+                    throw rc;
                 continue;
             }
@@ -10489 +10475 @@
         }
 
+        /* If there are implicit attachments to delete, throw away the lock
+         * map contents (which will unlock all media) since the medium
+         * attachments will be rolled back. Below we need to completely
+         * recreate the lock map anyway since it is infinitely complex to
+         * do this incrementally (would need reconstructing each attachment
+         * change, which would be extremely hairy). */
+        if (implicitAtts.size() != 0)
+        {
+            ErrorInfoKeeper eik;
+
+            HRESULT rc1 = lockedMediaMap->Clear();
+            AssertComRC(rc1);
+        }
+
         /* rollback hard disk changes */
         mMediaData.rollback();
@@ -10495 +10495 @@
 
         // Delete unused implicit diffs.
-        if(implicitAtts.size() != 0)
-        {
-            if (    oldState != MachineState_Saving
-                    && oldState != MachineState_LiveSnapshotting
-                    && oldState != MachineState_RestoringSnapshot
-                    && oldState != MachineState_DeletingSnapshot
-                    && oldState != MachineState_DeletingSnapshotOnline
-                    && oldState != MachineState_DeletingSnapshotPaused
-               )
-                   setMachineState(MachineState_SettingUp);
-
+        if (implicitAtts.size() != 0)
+        {
             alock.release();
 
-            // Loop round implicitatts
-            // a) Remove medum lock list.
-            // b) Delete HD storage from media list.
-            // c) Remove medium lock list.
-            MediaData::AttachmentList::const_iterator ittodelete;
             for (MediaData::AttachmentList::const_iterator it = implicitAtts.begin();
                  it != implicitAtts.end();
                  ++it)
             {
-                // Remove attachment.
-                MediumAttachment* pAtt = *it;
+                // Remove medium associated with this attachment.
+                ComObjPtr<MediumAttachment> pAtt = *it;
                 Assert(pAtt);
-                LogFlowThisFunc(("Deleting '%s'\n", (pAtt)->getLogName()));
-                ComObjPtr<Medium> hd = pAtt->getMedium();
-                Assert(hd);
-                mMediaData->mAttachments.remove(pAtt);
-                rc = lockedMediaMap->Unlock();
-
-                if (FAILED(rc))
-                    throw(rc);
-
-                // Remove from locked media map.
-                MediumLockList *pMediumLockList;
-                rc = lockedMediaMap->Get(pAtt, pMediumLockList);
-
-                if (FAILED(rc))
-                    throw(rc);
-                rc = lockedMediaMap->Remove(pAtt);
-                if (FAILED(rc))
-                    throw(rc);
-                rc = lockedMediaMap->Lock();
-                alock2.release();
-                rc = hd->deleteStorage(NULL /*aProgress*/, true /*aWait*/);
-
-                if (FAILED(rc))
-                    throw(rc);
-                AssertMsg(SUCCEEDED(rc), ("rc=%Rhrc it=%s hd=%s\n", rc, pAtt->getLogName(), hd->getLocationFull().c_str() ));
-
-                // Only way to delete lock list entry is
-                // by iterator so find the iterator with this lock list entry.
-                // Remove from Media Lock List.
-                MediumLockList::Base::iterator lockListBegin = pMediumLockList->GetBegin();
-                MediumLockList::Base::iterator lockListEnd   = pMediumLockList->GetEnd();
-                for (MediumLockList::Base::iterator it2 = lockListBegin; it2 != lockListEnd; ++it2 )
+                LogFlowThisFunc(("Deleting '%s'\n", pAtt->getLogName()));
+                ComObjPtr<Medium> pMedium = pAtt->getMedium();
+                Assert(pMedium);
+
+                rc = pMedium->deleteStorage(NULL /*aProgress*/, true /*aWait*/);
+                // continue on delete failure, just collect error messages
+                AssertMsg(SUCCEEDED(rc), ("rc=%Rhrc it=%s hd=%s\n", rc, pAtt->getLogName(), pMedium->getLocationFull().c_str() ));
+                mrc = rc;
+            }
+
+            alock.acquire();
+
+            /* if there is a VM recreate media lock map as mentioned above,
+             * otherwise it is a waste of time and we leave things unlocked */
+            if (aOnline)
+            {
+                const ComObjPtr<SessionMachine> pMachine = mData->mSession.mMachine;
+                /* must never be NULL, but better safe than sorry */
+                if (!pMachine.isNull())
                 {
-                    MediumLock &mediumLock = *it2;
-                    const ComObjPtr<Medium> pMedium = mediumLock.GetMedium();
-
-                    if (pMedium == hd)
-                    {
-                        rc = pMediumLockList->RemoveByIterator(it2);
-                        if (FAILED(rc))
-                            throw(rc);
-                        break;
-                    }
+                    alock.release();
+                    rc = mData->mSession.mMachine->lockMedia();
+                    alock.acquire();
+                    if (FAILED(rc))
+                        throw rc;
                 }
             }
         }
-        if (mData->mMachineState == MachineState_SettingUp)
-            setMachineState(oldState);
-    }
-
+    }
     catch (HRESULT aRC) {rc = aRC;}
 
-    // Unlock all hard disks that we locked if offline).
+    if (mData->mMachineState == MachineState_SettingUp)
+        setMachineState(oldState);
+
+    /* unlock all hard disks we locked when there is no VM */
     if (!aOnline)
     {
         ErrorInfoKeeper eik;
-        rc = lockedMediaMap->Clear();
-        if (FAILED(rc))
-            throw (rc);
-    }
-
-
-    if (aOnline)
-        mData->mSession.mLockedMedia = *lockedMediaMap;
-    else
-        lockedMediaOffline = *lockedMediaMap;
+
+        HRESULT rc1 = lockedMediaMap->Clear();
+        AssertComRC(rc1);
+    }
 
     return rc;
@@ -11018 +10980 @@
 {
     AutoCaller autoCaller(this);
-    AssertComRCReturnVoid (autoCaller.rc());
+    AssertComRCReturnVoid(autoCaller.rc());
 
     // AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-    LogFlowThisFunc(("Entering rollbackmedia\n"));
+    LogFlowThisFunc(("Entering rollbackMedia\n"));
 
     HRESULT rc = S_OK;
@@ -11063 +11025 @@
     /** @todo convert all this Machine-based voodoo to MediumAttachment
      * based rollback logic. */
-    deleteImplicitDiffs(true);
+    deleteImplicitDiffs(Global::IsOnline(mData->mMachineState));
 
     return;
@@ -12888 +12850 @@
 }
 
+STDMETHODIMP SessionMachine::LockMedia()
+{
+    AutoCaller autoCaller(this);
+    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
+
+    AutoMultiWriteLock2 alock(this->lockHandle(),
+                              &mParent->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+
+    AssertReturn(   mData->mMachineState == MachineState_Starting
+                 || mData->mMachineState == MachineState_Restoring
+                 || mData->mMachineState == MachineState_TeleportingIn, E_FAIL);
+
+    clearError();
+    alock.release();
+    return lockMedia();
+}
+
+STDMETHODIMP SessionMachine::UnlockMedia()
+{
+    unlockMedia();
+    return S_OK;
+}
+
 STDMETHODIMP SessionMachine::EjectMedium(IMediumAttachment *aAttachment,
                                          IMediumAttachment **aNewAttachment)
@@ -13207 +13192 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn (autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
     ComPtr<IInternalSessionControl> directControl;
@@ -13227 +13212 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn (autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
     ComPtr<IInternalSessionControl> directControl;
@@ -13365 +13350 @@
 
     AutoCaller autoCaller(this);
-    AssertComRCReturn (autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
 
     ComPtr<IInternalSessionControl> directControl;
@@ -13611 +13596 @@
  * succeeded individual locks will be undone).
  *
- * This method is intended to be called when the machine is in Starting or
- * Restoring state and asserts otherwise.
+ * The caller is responsible for doing the necessary state sanity checks.
  *
  * The locks made by this method must be undone by calling #unlockMedia() when
@@ -13625 +13609 @@
                               &mParent->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
-    AssertReturn(   mData->mMachineState == MachineState_Starting
-                 || mData->mMachineState == MachineState_Restoring
-                 || mData->mMachineState == MachineState_TeleportingIn, E_FAIL);
     /* bail out if trying to lock things with already set up locking */
     AssertReturn(mData->mSession.mLockedMedia.IsEmpty(), E_FAIL);
 
-    clearError();
     MultiResult mrc(S_OK);