Changeset 55214 in vbox for trunk/src/VBox/Main/src-server

Timestamp:

Apr 13, 2015 3:53:01 PM (10 years ago)

Author:

vboxsync

Message:

Main/Console+Machine+Session+Snapshot: move the save state and snapshot related methods from IConsole to IMachine, with lots of unavoidable code restructuring and cleanup. Also define two new machine states (so that the "Saving" one is specifically for saving state now) which requires more changes everywhere
Frontends: necessary adjustments
doc/SDK: document the changes

Location:

trunk/src/VBox/Main/src-server

Files:

• HostPower.cpp (modified) (3 diffs)
• MachineImpl.cpp (modified) (56 diffs)
• Performance.cpp (modified) (2 diffs)
• SnapshotImpl.cpp (modified) (54 diffs)

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

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2006-2014 Oracle Corporation
+ * Copyright (C) 2006-2015 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -139 +139 @@
 
             /* save running VMs */
-            SessionMachinesList::const_iterator it2 = machines.begin();
-            for (VirtualBox::InternalControlList::const_iterator it = controls.begin();
-                 it != controls.end() && it2 != machines.end();
-                 ++it, ++it2)
+            for (SessionMachinesList::const_iterator it = machines.begin();
+                 it != machines.end();
+                 ++it)
             {
-                ComPtr<SessionMachine> pMachine = *it2;
+                ComPtr<SessionMachine> pMachine = *it;
                 rc = pMachine->GetExtraData(Bstr("VBoxInternal2/SavestateOnBatteryLow").raw(),
                                             value.asOutParam());
@@ -160 +159 @@
                 if (fGlobal + fPerVM >= 0)
                 {
-                    ComPtr<IInternalSessionControl> pControl = *it;
                     ComPtr<IProgress> progress;
 
-                    /* note that SaveStateWithReason() will simply return a failure
-                     * if the VM is in an inappropriate state */
-                    rc = pControl->SaveStateWithReason(Reason_HostBatteryLow, progress.asOutParam());
+                    /* SessionMachine::i_saveStateWithReason() will return
+                     * a failure if the VM is in an inappropriate state */
+                    rc = pMachine->i_saveStateWithReason(Reason_HostBatteryLow, progress);
                     if (FAILED(rc))
                     {

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

@@ -131 +131 @@
 
     mSession.mPID              = NIL_RTPROCESS;
+    mSession.mLockType         = LockType_Null;
     mSession.mState            = SessionState_Unlocked;
 }
@@ -2694 +2695 @@
 
     *aState = mData->mMachineState;
+    Assert(mData->mMachineState != MachineState_Null);
 
     return S_OK;
@@ -3248 +3250 @@
 
         // copy pointers to W (the write-locking session) before leaving lock (these must not be NULL)
+        ComAssertRet(mData->mSession.mLockType == LockType_Write || mData->mSession.mLockType == LockType_VM, E_FAIL);
         ComPtr<IInternalSessionControl> pSessionW = mData->mSession.mDirectControl;
         ComAssertRet(!pSessionW.isNull(), E_FAIL);
@@ -3263 +3266 @@
         // get the console of the session holding the write lock (this is a remote call)
         ComPtr<IConsole> pConsoleW;
-        LogFlowThisFunc(("Calling GetRemoteConsole()...\n"));
-        rc = pSessionW->GetRemoteConsole(pConsoleW.asOutParam());
-        LogFlowThisFunc(("GetRemoteConsole() returned %08X\n", rc));
-        if (FAILED(rc))
-            // the failure may occur w/o any error info (from RPC), so provide one
-            return setError(VBOX_E_VM_ERROR,
-                            tr("Failed to get a console object from the direct session (%Rhrc)"), rc);
-
-        ComAssertRet(!pConsoleW.isNull(), E_FAIL);
+        if (mData->mSession.mLockType == LockType_VM)
+        {
+            LogFlowThisFunc(("Calling GetRemoteConsole()...\n"));
+            rc = pSessionW->COMGETTER(RemoteConsole)(pConsoleW.asOutParam());
+            LogFlowThisFunc(("GetRemoteConsole() returned %08X\n", rc));
+            if (FAILED(rc))
+                // the failure may occur w/o any error info (from RPC), so provide one
+                return setError(VBOX_E_VM_ERROR,
+                                tr("Failed to get a console object from the direct session (%Rhrc)"), rc);
+            ComAssertRet(!pConsoleW.isNull(), E_FAIL);
+        }
 
         // share the session machine and W's console with the caller's session
@@ -3315 +3320 @@
         RTPROCESS pid = NIL_RTPROCESS;
         AssertCompile(sizeof(ULONG) == sizeof(RTPROCESS));
-        pSessionControl->GetPID((ULONG*)&pid);
+        pSessionControl->COMGETTER(PID)((ULONG*)&pid);
         Assert(pid != NIL_RTPROCESS);
 
@@ -3432 +3437 @@
                 /* get the console from the direct session */
                 ComPtr<IConsole> console;
-                rc = pSessionControl->GetRemoteConsole(console.asOutParam());
+                rc = pSessionControl->COMGETTER(RemoteConsole)(console.asOutParam());
                 ComAssertComRC(rc);
 
@@ -3505 +3510 @@
         if (SUCCEEDED(rc))
         {
+            mData->mSession.mLockType = aLockType;
             /* memorize the direct session control and cache IUnknown for it */
             mData->mSession.mDirectControl = pSessionControl;
@@ -3525 +3531 @@
         if (FAILED(rc))
             sessionMachine->uninit();
-
     }
 
@@ -5172 +5177 @@
 }
 
-struct Machine::DeleteTask
-{
-    ComObjPtr<Machine>          pMachine;
-    RTCList<ComPtr<IMedium> >   llMediums;
-    StringsList                 llFilesToDelete;
-    ComObjPtr<Progress>         pProgress;
+/**
+ * Task record for deleting a machine config.
+ */
+struct Machine::DeleteConfigTask
+    : public Machine::Task
+{
+    DeleteConfigTask(Machine *m,
+                     Progress *p,
+                     const Utf8Str &t,
+                     const RTCList<ComPtr<IMedium> > &llMediums,
+                     const StringsList &llFilesToDelete)
+        : Task(m, p, t),
+          m_llMediums(llMediums),
+          m_llFilesToDelete(llFilesToDelete)
+    {}
+
+    void handler()
+    {
+        m_pMachine->i_deleteConfigHandler(*this);
+    }
+
+    RTCList<ComPtr<IMedium> >   m_llMediums;
+    StringsList                 m_llFilesToDelete;
 };
 
-HRESULT Machine::deleteConfig(const std::vector<ComPtr<IMedium> > &aMedia, ComPtr<IProgress> &aProgress)
-{
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    HRESULT rc = i_checkStateDependency(MutableStateDep);
-    if (FAILED(rc)) return rc;
-
-    if (mData->mRegistered)
-        return setError(VBOX_E_INVALID_VM_STATE,
-                        tr("Cannot delete settings of a registered machine"));
-
-    DeleteTask *pTask = new DeleteTask;
-    pTask->pMachine = this;
-
-    // collect files to delete
-    pTask->llFilesToDelete = mData->llFilesToDelete;            // saved states pushed here by Unregister()
-
-    for (size_t i = 0; i < aMedia.size(); ++i)
-    {
-        IMedium *pIMedium(aMedia[i]);
-        ComObjPtr<Medium> pMedium = static_cast<Medium*>(pIMedium);
-        if (pMedium.isNull())
-            return setError(E_INVALIDARG, "The given medium pointer with index %d is invalid", i);
-        SafeArray<BSTR> ids;
-        rc = pMedium->COMGETTER(MachineIds)(ComSafeArrayAsOutParam(ids));
-        if (FAILED(rc)) return rc;
-        /* At this point the medium should not have any back references
-         * anymore. If it has it is attached to another VM and *must* not
-         * deleted. */
-        if (ids.size() < 1)
-            pTask->llMediums.append(pMedium);
-    }
-    if (mData->pMachineConfigFile->fileExists())
-        pTask->llFilesToDelete.push_back(mData->m_strConfigFileFull);
-
-    pTask->pProgress.createObject();
-    pTask->pProgress->init(i_getVirtualBox(),
-                           static_cast<IMachine*>(this) /* aInitiator */,
-                           Bstr(tr("Deleting files")).raw(),
-                           true /* fCancellable */,
-                           (ULONG)(pTask->llFilesToDelete.size() + pTask->llMediums.size() + 1),   // cOperations
-                           BstrFmt(tr("Deleting '%s'"), pTask->llFilesToDelete.front().c_str()).raw());
-
-    int vrc = RTThreadCreate(NULL,
-                             Machine::deleteThread,
-                             (void*)pTask,
-                             0,
-                             RTTHREADTYPE_MAIN_WORKER,
-                             0,
-                             "MachineDelete");
-
-    pTask->pProgress.queryInterfaceTo(aProgress.asOutParam());
-
-    if (RT_FAILURE(vrc))
-    {
-        delete pTask;
-        return setError(E_FAIL, "Could not create MachineDelete thread (%Rrc)", vrc);
-    }
-
-    LogFlowFuncLeave();
-
-    return S_OK;
-}
-
 /**
- * Static task wrapper passed to RTThreadCreate() in Machine::Delete() which then
- * calls Machine::deleteTaskWorker() on the actual machine object.
- * @param Thread
- * @param pvUser
- * @return
- */
-/*static*/
-DECLCALLBACK(int) Machine::deleteThread(RTTHREAD Thread, void *pvUser)
-{
-    LogFlowFuncEnter();
-
-    DeleteTask *pTask = (DeleteTask*)pvUser;
-    Assert(pTask);
-    Assert(pTask->pMachine);
-    Assert(pTask->pProgress);
-
-    HRESULT rc = pTask->pMachine->i_deleteTaskWorker(*pTask);
-    pTask->pProgress->i_notifyComplete(rc);
-
-    delete pTask;
-
-    LogFlowFuncLeave();
-
-    NOREF(Thread);
-
-    return VINF_SUCCESS;
-}
-
-/**
- * Task thread implementation for Machine::Delete(), called from Machine::deleteThread().
+ * Task thread implementation for SessionMachine::DeleteConfig(), called from
+ * SessionMachine::taskHandler().
+ *
+ * @note Locks this object for writing.
+ *
  * @param task
  * @return
  */
-HRESULT Machine::i_deleteTaskWorker(DeleteTask &task)
-{
+void Machine::i_deleteConfigHandler(DeleteConfigTask &task)
+{
+    LogFlowThisFuncEnter();
+
     AutoCaller autoCaller(this);
-    if (FAILED(autoCaller.rc())) return autoCaller.rc();
+    LogFlowThisFunc(("state=%d\n", getObjectState().getState()));
+    if (FAILED(autoCaller.rc()))
+    {
+        /* we might have been uninitialized because the session was accidentally
+         * closed by the client, so don't assert */
+        HRESULT rc = setError(E_FAIL,
+                              tr("The session has been accidentally closed"));
+        task.m_pProgress->i_notifyComplete(rc);
+        LogFlowThisFuncLeave();
+        return;
+    }
 
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
@@ -5303 +5248 @@
         i_setMachineState(MachineState_SettingUp);
         alock.release();
-        for (size_t i = 0; i < task.llMediums.size(); ++i)
-        {
-            ComObjPtr<Medium> pMedium = (Medium*)(IMedium*)task.llMediums.at(i);
+        for (size_t i = 0; i < task.m_llMediums.size(); ++i)
+        {
+            ComObjPtr<Medium> pMedium = (Medium*)(IMedium*)(task.m_llMediums.at(i));
             {
                 AutoCaller mac(pMedium);
                 if (FAILED(mac.rc())) throw mac.rc();
                 Utf8Str strLocation = pMedium->i_getLocationFull();
-                rc = task.pProgress->SetNextOperation(BstrFmt(tr("Deleting '%s'"), strLocation.c_str()).raw(), 1);
+                rc = task.m_pProgress->SetNextOperation(BstrFmt(tr("Deleting '%s'"), strLocation.c_str()).raw(), 1);
                 if (FAILED(rc)) throw rc;
                 LogFunc(("Deleting file %s\n", strLocation.c_str()));
@@ -5319 +5264 @@
                 rc = pMedium->DeleteStorage(pProgress2.asOutParam());
                 if (FAILED(rc)) throw rc;
-                rc = task.pProgress->WaitForAsyncProgressCompletion(pProgress2);
+                rc = task.m_pProgress->WaitForAsyncProgressCompletion(pProgress2);
                 if (FAILED(rc)) throw rc;
                 /* Check the result of the asynchronous process. */
@@ -5347 +5292 @@
         // medium storage files from the IMedium list passed in, and the
         // machine XML file)
-        StringsList::const_iterator it = task.llFilesToDelete.begin();
-        while (it != task.llFilesToDelete.end())
+        StringsList::const_iterator it = task.m_llFilesToDelete.begin();
+        while (it != task.m_llFilesToDelete.end())
         {
             const Utf8Str &strFile = *it;
@@ -5358 +5303 @@
 
             ++it;
-            if (it == task.llFilesToDelete.end())
+            if (it == task.m_llFilesToDelete.end())
             {
-                rc = task.pProgress->SetNextOperation(Bstr(tr("Cleaning up machine directory")).raw(), 1);
+                rc = task.m_pProgress->SetNextOperation(Bstr(tr("Cleaning up machine directory")).raw(), 1);
                 if (FAILED(rc)) throw rc;
                 break;
             }
 
-            rc = task.pProgress->SetNextOperation(BstrFmt(tr("Deleting '%s'"), it->c_str()).raw(), 1);
+            rc = task.m_pProgress->SetNextOperation(BstrFmt(tr("Deleting '%s'"), it->c_str()).raw(), 1);
             if (FAILED(rc)) throw rc;
         }
@@ -5438 +5383 @@
     catch (HRESULT aRC) { rc = aRC; }
 
-    return rc;
+    task.m_pProgress->i_notifyComplete(rc);
+
+    LogFlowThisFuncLeave();
+}
+
+HRESULT Machine::deleteConfig(const std::vector<ComPtr<IMedium> > &aMedia, ComPtr<IProgress> &aProgress)
+{
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    HRESULT rc = i_checkStateDependency(MutableStateDep);
+    if (FAILED(rc)) return rc;
+
+    if (mData->mRegistered)
+        return setError(VBOX_E_INVALID_VM_STATE,
+                        tr("Cannot delete settings of a registered machine"));
+
+    // collect files to delete
+    StringsList llFilesToDelete(mData->llFilesToDelete);    // saved states pushed here by Unregister()
+    if (mData->pMachineConfigFile->fileExists())
+        llFilesToDelete.push_back(mData->m_strConfigFileFull);
+
+    RTCList<ComPtr<IMedium> > llMediums;
+    for (size_t i = 0; i < aMedia.size(); ++i)
+    {
+        IMedium *pIMedium(aMedia[i]);
+        ComObjPtr<Medium> pMedium = static_cast<Medium*>(pIMedium);
+        if (pMedium.isNull())
+            return setError(E_INVALIDARG, "The given medium pointer with index %d is invalid", i);
+        SafeArray<BSTR> ids;
+        rc = pMedium->COMGETTER(MachineIds)(ComSafeArrayAsOutParam(ids));
+        if (FAILED(rc)) return rc;
+        /* At this point the medium should not have any back references
+         * anymore. If it has it is attached to another VM and *must* not
+         * deleted. */
+        if (ids.size() < 1)
+            llMediums.append(pMedium);
+    }
+
+    ComObjPtr<Progress> pProgress;
+    pProgress.createObject();
+    rc = pProgress->init(i_getVirtualBox(),
+                         static_cast<IMachine*>(this) /* aInitiator */,
+                         Bstr(tr("Deleting files")).raw(),
+                         true /* fCancellable */,
+                         (ULONG)(llFilesToDelete.size() + llMediums.size() + 1),   // cOperations
+                         BstrFmt(tr("Deleting '%s'"), llFilesToDelete.front().c_str()).raw());
+    if (FAILED(rc))
+        return rc;
+
+    /* create and start the task on a separate thread (note that it will not
+     * start working until we release alock) */
+    DeleteConfigTask *pTask = new DeleteConfigTask(this, pProgress, "DeleteVM", llMediums, llFilesToDelete);
+    rc = pTask->createThread();
+    if (FAILED(rc))
+        return rc;
+
+    pProgress.queryInterfaceTo(aProgress.asOutParam());
+
+    LogFlowFuncLeave();
+
+    return S_OK;
 }
 
@@ -5534 +5539 @@
                             Global::stringifySessionState(mData->mSession.mState));
 
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -5556 +5562 @@
                             Global::stringifySessionState(mData->mSession.mState));
 
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -5609 +5616 @@
 
     ComPtr<IInternalSessionControl> directControl;
-    directControl = mData->mSession.mDirectControl;
-
-    /* fail if we were called after #OnSessionEnd() is called.  This is a
-     * silly race condition. */
-
-    /** @todo This code is bothering API clients (like python script clients) with
-     *        the AccessGuestProperty call, creating unncessary IPC.  Need to
-     *        have a way of figuring out which kind of direct session it is... */
+    {
+        AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
+    }
+
+    /* ignore calls made after #OnSessionEnd() is called */
     if (!directControl)
         rc = E_ACCESSDENIED;
@@ -5769 +5775 @@
     try
     {
-        ComPtr<IInternalSessionControl> directControl = mData->mSession.mDirectControl;
+        ComPtr<IInternalSessionControl> directControl;
+        {
+            AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+            if (mData->mSession.mLockType == LockType_VM)
+                directControl = mData->mSession.mDirectControl;
+        }
 
         BSTR dummy = NULL; /* will not be changed (setter) */
@@ -5896 +5907 @@
     HRESULT rc;
     ComPtr<IInternalSessionControl> directControl;
-    directControl = mData->mSession.mDirectControl;
-
+    {
+        AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
+    }
 
     com::SafeArray<BSTR> bNames;
@@ -7086 +7100 @@
     return rc;
 
+}
+
+HRESULT Machine::saveState(ComPtr<IProgress> &aProgress)
+{
+    NOREF(aProgress);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    // This check should always fail.
+    HRESULT rc = i_checkStateDependency(MutableStateDep);
+    if (FAILED(rc)) return rc;
+
+    AssertFailedReturn(E_NOTIMPL);
+}
+
+HRESULT Machine::adoptSavedState(const com::Utf8Str &aSavedStateFile)
+{
+    NOREF(aSavedStateFile);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    // This check should always fail.
+    HRESULT rc = i_checkStateDependency(MutableStateDep);
+    if (FAILED(rc)) return rc;
+
+    AssertFailedReturn(E_NOTIMPL);
+}
+
+HRESULT Machine::discardSavedState(BOOL aFRemoveFile)
+{
+    NOREF(aFRemoveFile);
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    // This check should always fail.
+    HRESULT rc = i_checkStateDependency(MutableStateDep);
+    if (FAILED(rc)) return rc;
+
+    AssertFailedReturn(E_NOTIMPL);
 }
 
@@ -7718 +7768 @@
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    if (    mData->mSession.mState == SessionState_Locked
+    if (    (   mData->mSession.mState == SessionState_Locked
+             && mData->mSession.mLockType == LockType_VM)
          || (aAllowClosing && mData->mSession.mState == SessionState_Unlocking)
        )
@@ -8419 +8470 @@
     LogFlowThisFuncEnter();
     LogFlowThisFunc(("aMachineState=%s\n", Global::stringifyMachineState(aMachineState) ));
+    Assert(aMachineState != MachineState_Null);
 
     AutoCaller autoCaller(this);
@@ -10581 +10633 @@
 
     /* must be in a protective state because we release the lock below */
-    AssertReturn(   mData->mMachineState == MachineState_Saving
+    AssertReturn(   mData->mMachineState == MachineState_Snapshotting
+                 || mData->mMachineState == MachineState_OnlineSnapshotting
                  || mData->mMachineState == MachineState_LiveSnapshotting
                  || mData->mMachineState == MachineState_RestoringSnapshot
@@ -10670 +10723 @@
                 /* copy the attachment as is */
 
-                /** @todo the progress object created in Console::TakeSnaphot
+                /** @todo the progress object created in SessionMachine::TakeSnaphot
                  * only expects operations for hard disks. Later other
                  * device types need to show up in the progress as well. */
@@ -10832 +10885 @@
     /* will release the lock before the potentially lengthy operation,
      * so protect with the special state (unless already protected) */
-    if (   oldState != MachineState_Saving
+    if (   oldState != MachineState_Snapshotting
+        && oldState != MachineState_OnlineSnapshotting
         && oldState != MachineState_LiveSnapshotting
         && oldState != MachineState_RestoringSnapshot
@@ -10969 +11023 @@
                 mrc = rc;
             }
+            // Clear the list of deleted implicit attachments now, while not
+            // holding the lock, as it will ultimately trigger Medium::uninit()
+            // calls which assume that the media tree lock isn't held.
+            implicitAtts.clear();
 
             alock.acquire();
@@ -12490 +12548 @@
     }
 
-    Assert(    mConsoleTaskData.strStateFilePath.isEmpty()
-            || !mConsoleTaskData.mSnapshot);
-    if (!mConsoleTaskData.strStateFilePath.isEmpty())
-    {
-        LogWarningThisFunc(("canceling failed save state request!\n"));
-        endSavingState(E_FAIL, tr("Machine terminated with pending save state!"));
-    }
-    else if (!mConsoleTaskData.mSnapshot.isNull())
-    {
-        LogWarningThisFunc(("canceling untaken snapshot!\n"));
-
-        /* delete all differencing hard disks created (this will also attach
-         * their parents back by rolling back mMediaData) */
-        i_rollbackMedia();
-
-        // delete the saved state file (it might have been already created)
-        // AFTER killing the snapshot so that releaseSavedStateFile() won't
-        // think it's still in use
-        Utf8Str strStateFile = mConsoleTaskData.mSnapshot->i_getStateFilePath();
-        mConsoleTaskData.mSnapshot->uninit();
-        i_releaseSavedStateFile(strStateFile, NULL /* pSnapshotToIgnore */ );
-    }
-
     mData->mSession.mPID = NIL_RTPROCESS;
 
@@ -12613 +12648 @@
 
     /* reset the rest of session data */
+    mData->mSession.mLockType = LockType_Null;
     mData->mSession.mMachine.setNull();
     mData->mSession.mState = SessionState_Unlocked;
@@ -12698 +12734 @@
 }
 
+////////////////////////////////////////////////////////////////////////////////
+//
+// SessionMachine task records
+//
+////////////////////////////////////////////////////////////////////////////////
+
+/**
+ * Task record for saving the machine state.
+ */
+struct SessionMachine::SaveStateTask
+    : public Machine::Task
+{
+    SaveStateTask(SessionMachine *m,
+                  Progress *p,
+                  const Utf8Str &t,
+                  Reason_T enmReason,
+                  const Utf8Str &strStateFilePath)
+        : Task(m, p, t),
+          m_enmReason(enmReason),
+          m_strStateFilePath(strStateFilePath)
+    {}
+
+    void handler()
+    {
+        ((SessionMachine *)(Machine *)m_pMachine)->i_saveStateHandler(*this);
+    }
+
+    Reason_T m_enmReason;
+    Utf8Str m_strStateFilePath;
+};
+
+/**
+ * Task thread implementation for SessionMachine::SaveState(), called from
+ * SessionMachine::taskHandler().
+ *
+ * @note Locks this object for writing.
+ *
+ * @param task
+ * @return
+ */
+void SessionMachine::i_saveStateHandler(SaveStateTask &task)
+{
+    LogFlowThisFuncEnter();
+
+    AutoCaller autoCaller(this);
+    LogFlowThisFunc(("state=%d\n", getObjectState().getState()));
+    if (FAILED(autoCaller.rc()))
+    {
+        /* we might have been uninitialized because the session was accidentally
+         * closed by the client, so don't assert */
+        HRESULT rc = setError(E_FAIL,
+                              tr("The session has been accidentally closed"));
+        task.m_pProgress->i_notifyComplete(rc);
+        LogFlowThisFuncLeave();
+        return;
+    }
+
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    HRESULT rc = S_OK;
+
+    try
+    {
+        ComPtr<IInternalSessionControl> directControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
+        if (directControl.isNull())
+            throw setError(VBOX_E_INVALID_VM_STATE,
+                           tr("Trying to save state without a running VM"));
+        alock.release();
+        BOOL fSuspendedBySave;
+        rc = directControl->SaveStateWithReason(task.m_enmReason, task.m_pProgress, Bstr(task.m_strStateFilePath).raw(), task.m_machineStateBackup != MachineState_Paused, &fSuspendedBySave);
+        Assert(!fSuspendedBySave);
+        alock.acquire();
+
+        AssertStmt(   (SUCCEEDED(rc) && mData->mMachineState == MachineState_Saved)
+                   || (FAILED(rc) && mData->mMachineState == MachineState_Saving),
+                   throw E_FAIL);
+
+        if (SUCCEEDED(rc))
+        {
+            mSSData->strStateFilePath = task.m_strStateFilePath;
+
+            /* save all VM settings */
+            rc = i_saveSettings(NULL);
+                    // no need to check whether VirtualBox.xml needs saving also since
+                    // we can't have a name change pending at this point
+        }
+        else
+        {
+            // On failure, set the state to the state we had at the beginning.
+            i_setMachineState(task.m_machineStateBackup);
+            i_updateMachineStateOnClient();
+
+            // Delete the saved state file (might have been already created).
+            // No need to check whether this is shared with a snapshot here
+            // because we certainly created a fresh saved state file here.
+            RTFileDelete(task.m_strStateFilePath.c_str());
+        }
+    }
+    catch (HRESULT aRC) { rc = aRC; }
+
+    task.m_pProgress->i_notifyComplete(rc);
+
+    LogFlowThisFuncLeave();
+}
+
 /**
  *  @note Locks this object for writing.
  */
-HRESULT SessionMachine::setRemoveSavedStateFile(BOOL aRemove)
+HRESULT SessionMachine::saveState(ComPtr<IProgress> &aProgress)
+{
+    return i_saveStateWithReason(Reason_Unspecified, aProgress);
+}
+
+HRESULT SessionMachine::i_saveStateWithReason(Reason_T aReason, ComPtr<IProgress> &aProgress)
 {
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    mRemoveSavedState = RT_BOOL(aRemove);
-
-    return S_OK;
-}
+    HRESULT rc = i_checkStateDependency(MutableOrRunningStateDep);
+    if (FAILED(rc)) return rc;
+
+    if (   mData->mMachineState != MachineState_Running
+        && mData->mMachineState != MachineState_Paused
+       )
+        return setError(VBOX_E_INVALID_VM_STATE,
+            tr("Cannot save the execution state as the machine is not running or paused (machine state: %s)"),
+            Global::stringifyMachineState(mData->mMachineState));
+
+    ComObjPtr<Progress> pProgress;
+    pProgress.createObject();
+    rc = pProgress->init(i_getVirtualBox(),
+                         static_cast<IMachine *>(this) /* aInitiator */,
+                         Bstr(tr("Saving the execution state of the virtual machine")).raw(),
+                         FALSE /* aCancelable */);
+    if (FAILED(rc))
+        return rc;
+
+    Utf8Str strStateFilePath;
+    i_composeSavedStateFilename(strStateFilePath);
+
+    /* create and start the task on a separate thread (note that it will not
+     * start working until we release alock) */
+    SaveStateTask *pTask = new SaveStateTask(this, pProgress, "SaveState", aReason, strStateFilePath);
+    rc = pTask->createThread();
+    if (FAILED(rc))
+        return rc;
+
+    /* set the state to Saving (expected by Session::SaveStateWithReason()) */
+    i_setMachineState(MachineState_Saving);
+    i_updateMachineStateOnClient();
+
+    pProgress.queryInterfaceTo(aProgress.asOutParam());
+
+    return S_OK;
+}
+
+/**
+ *  @note Locks this object for writing.
+ */
+HRESULT SessionMachine::adoptSavedState(const com::Utf8Str &aSavedStateFile)
+{
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    HRESULT rc = i_checkStateDependency(MutableStateDep);
+    if (FAILED(rc)) return rc;
+
+    if (   mData->mMachineState != MachineState_PoweredOff
+        && mData->mMachineState != MachineState_Teleported
+        && mData->mMachineState != MachineState_Aborted
+       )
+        return setError(VBOX_E_INVALID_VM_STATE,
+            tr("Cannot adopt the saved machine state as the machine is not in Powered Off, Teleported or Aborted state (machine state: %s)"),
+            Global::stringifyMachineState(mData->mMachineState));
+
+    com::Utf8Str stateFilePathFull;
+    int vrc = i_calculateFullPath(aSavedStateFile, stateFilePathFull);
+    if (RT_FAILURE(vrc))
+        return setError(VBOX_E_FILE_ERROR,
+                        tr("Invalid saved state file path '%s' (%Rrc)"),
+                        aSavedStateFile.c_str(),
+                        vrc);
+
+    mSSData->strStateFilePath = stateFilePathFull;
+
+    /* The below i_setMachineState() will detect the state transition and will
+     * update the settings file */
+
+    return i_setMachineState(MachineState_Saved);
+}
+
+/**
+ *  @note Locks this object for writing.
+ */
+HRESULT SessionMachine::discardSavedState(BOOL aFRemoveFile)
+{
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    HRESULT rc = i_checkStateDependency(MutableStateDep);
+    if (FAILED(rc)) return rc;
+
+    if (mData->mMachineState != MachineState_Saved)
+        return setError(VBOX_E_INVALID_VM_STATE,
+            tr("Cannot delete the machine state as the machine is not in the saved state (machine state: %s)"),
+            Global::stringifyMachineState(mData->mMachineState));
+
+    mRemoveSavedState = RT_BOOL(aFRemoveFile);
+
+    /*
+     * Saved -> PoweredOff transition will be detected in the SessionMachine
+     * and properly handled.
+     */
+    rc = i_setMachineState(MachineState_PoweredOff);
+    return rc;
+}
+
 
 /**
@@ -13087 +13328 @@
 }
 
-/**
- *  @note Locks this object for writing.
- */
-HRESULT SessionMachine::beginSavingState(ComPtr<IProgress> &aProgress,
-                                         com::Utf8Str &aStateFilePath)
-{
-    LogFlowThisFuncEnter();
-
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    AssertReturn(    mData->mMachineState == MachineState_Paused
-                  && mConsoleTaskData.mLastState == MachineState_Null
-                  && mConsoleTaskData.strStateFilePath.isEmpty(),
-                 E_FAIL);
-
-    /* create a progress object to track operation completion */
-    ComObjPtr<Progress> pProgress;
-    pProgress.createObject();
-    pProgress->init(i_getVirtualBox(),
-                    static_cast<IMachine *>(this) /* aInitiator */,
-                    Bstr(tr("Saving the execution state of the virtual machine")).raw(),
-                    FALSE /* aCancelable */);
-
-    /* stateFilePath is null when the machine is not running */
-    if (mData->mMachineState == MachineState_Paused)
-        i_composeSavedStateFilename(aStateFilePath);
-
-    /* fill in the console task data */
-    mConsoleTaskData.mLastState = mData->mMachineState;
-    mConsoleTaskData.strStateFilePath = aStateFilePath;
-    mConsoleTaskData.mProgress = pProgress;
-
-    /* set the state to Saving (this is expected by Console::SaveState()) */
-    i_setMachineState(MachineState_Saving);
-
-    pProgress.queryInterfaceTo(aProgress.asOutParam());
-
-    return S_OK;
-}
-
-/**
- *  @note Locks mParent + this object for writing.
- */
-HRESULT SessionMachine::endSavingState(LONG aResult,
-                                       const com::Utf8Str &aErrMsg)
-{
-    LogFlowThisFunc(("\n"));
-
-    /* endSavingState() need mParent lock */
-    AutoMultiWriteLock2 alock(mParent, this COMMA_LOCKVAL_SRC_POS);
-
-    AssertReturn(    (   (SUCCEEDED(aResult) && mData->mMachineState == MachineState_Saved)
-                      || (FAILED(aResult) && mData->mMachineState == MachineState_Saving))
-                  && mConsoleTaskData.mLastState != MachineState_Null
-                  && !mConsoleTaskData.strStateFilePath.isEmpty(),
-                 E_FAIL);
-
-    /*
-     * On failure, set the state to the state we had when BeginSavingState()
-     * was called (this is expected by Console::SaveState() and the associated
-     * task). On success the VM process already changed the state to
-     * MachineState_Saved, so no need to do anything.
-     */
-    if (FAILED(aResult))
-        i_setMachineState(mConsoleTaskData.mLastState);
-
-    return i_endSavingState(aResult, aErrMsg);
-}
-
-/**
- *  @note Locks this object for writing.
- */
-HRESULT SessionMachine::adoptSavedState(const com::Utf8Str &aSavedStateFile)
-{
-    LogFlowThisFunc(("\n"));
-
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    AssertReturn(   mData->mMachineState == MachineState_PoweredOff
-                 || mData->mMachineState == MachineState_Teleported
-                 || mData->mMachineState == MachineState_Aborted
-                 , E_FAIL); /** @todo setError. */
-
-    com::Utf8Str stateFilePathFull;
-    int vrc = i_calculateFullPath(aSavedStateFile, stateFilePathFull);
-    if (RT_FAILURE(vrc))
-        return setError(VBOX_E_FILE_ERROR,
-                        tr("Invalid saved state file path '%s' (%Rrc)"),
-                        aSavedStateFile.c_str(),
-                        vrc);
-
-    mSSData->strStateFilePath = stateFilePathFull;
-
-    /* The below i_setMachineState() will detect the state transition and will
-     * update the settings file */
-
-    return i_setMachineState(MachineState_Saved);
-}
-
 HRESULT SessionMachine::pullGuestProperties(std::vector<com::Utf8Str> &aNames,
                                             std::vector<com::Utf8Str> &aValues,
@@ -13263 +13405 @@
             case MachineState_Teleporting:
             case MachineState_TeleportingPausedVM:
+            case MachineState_OnlineSnapshotting:
             case MachineState_LiveSnapshotting:
             case MachineState_DeletingSnapshotOnline:
@@ -13536 +13679 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13561 +13705 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13589 +13734 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13612 +13758 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13635 +13782 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13658 +13806 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13681 +13830 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13701 +13851 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13724 +13875 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13747 +13899 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13770 +13923 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13793 +13947 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13816 +13971 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13839 +13995 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13862 +14019 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13885 +14043 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13951 +14110 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -13983 +14143 @@
     {
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
     }
 
@@ -14000 +14161 @@
 // protected methods
 /////////////////////////////////////////////////////////////////////////////
-
-/**
- *  Helper method to finalize saving the state.
- *
- *  @note Must be called from under this object's lock.
- *
- *  @param aRc      S_OK if the snapshot has been taken successfully
- *  @param aErrMsg  human readable error message for failure
- *
- *  @note Locks mParent + this objects for writing.
- */
-HRESULT SessionMachine::i_endSavingState(HRESULT aRc, const Utf8Str &aErrMsg)
-{
-    LogFlowThisFuncEnter();
-
-    AutoCaller autoCaller(this);
-    AssertComRCReturn(autoCaller.rc(), autoCaller.rc());
-
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    HRESULT rc = S_OK;
-
-    if (SUCCEEDED(aRc))
-    {
-        mSSData->strStateFilePath = mConsoleTaskData.strStateFilePath;
-
-        /* save all VM settings */
-        rc = i_saveSettings(NULL);
-                // no need to check whether VirtualBox.xml needs saving also since
-                // we can't have a name change pending at this point
-    }
-    else
-    {
-        // delete the saved state file (it might have been already created);
-        // we need not check whether this is shared with a snapshot here because
-        // we certainly created this saved state file here anew
-        RTFileDelete(mConsoleTaskData.strStateFilePath.c_str());
-    }
-
-    /* notify the progress object about operation completion */
-    Assert(mConsoleTaskData.mProgress);
-    if (SUCCEEDED(aRc))
-        mConsoleTaskData.mProgress->i_notifyComplete(S_OK);
-    else
-    {
-        if (aErrMsg.length())
-            mConsoleTaskData.mProgress->i_notifyComplete(aRc,
-                                                         COM_IIDOF(ISession),
-                                                         getComponentName(),
-                                                         aErrMsg.c_str());
-        else
-            mConsoleTaskData.mProgress->i_notifyComplete(aRc);
-    }
-
-    /* clear out the temporary saved state data */
-    mConsoleTaskData.mLastState = MachineState_Null;
-    mConsoleTaskData.strStateFilePath.setNull();
-    mConsoleTaskData.mProgress.setNull();
-
-    LogFlowThisFuncLeave();
-    return rc;
-}
 
 /**
@@ -14259 +14358 @@
                  || oldMachineState == MachineState_Paused
                  || oldMachineState == MachineState_Teleporting
+                 || oldMachineState == MachineState_OnlineSnapshotting
                  || oldMachineState == MachineState_LiveSnapshotting
                  || oldMachineState == MachineState_Stuck
@@ -14273 +14373 @@
                  || aMachineState == MachineState_Aborted
                 )
-             /* ignore PoweredOff->Saving->PoweredOff transition when taking a
-              * snapshot */
-             && (   mConsoleTaskData.mSnapshot.isNull()
-                 || mConsoleTaskData.mLastState >= MachineState_Running /** @todo Live Migration: clean up (lazy bird) */
-                )
             )
     {
@@ -14310 +14405 @@
     {
         /*
-         *  delete the saved state after Console::ForgetSavedState() is called
+         *  delete the saved state after SessionMachine::ForgetSavedState() is called
          *  or if the VM process (owning a direct VM session) crashed while the
          *  VM was Saved
@@ -14456 +14551 @@
         AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
         AssertReturn(!!mData, E_FAIL);
-        directControl = mData->mSession.mDirectControl;
+        if (mData->mSession.mLockType == LockType_VM)
+            directControl = mData->mSession.mDirectControl;
 
         /* directControl may be already set to NULL here in #OnSessionEnd()
@@ -14469 +14565 @@
         if (mData->mSession.mState == SessionState_Unlocking)
             return S_OK;
-
-        AssertReturn(!directControl.isNull(), E_FAIL);
-    }
+    }
+
+    /* ignore notifications sent after #OnSessionEnd() is called */
+    if (!directControl)
+        return S_OK;
 
     return directControl->UpdateMachineState(mData->mMachineState);
 }
 
-HRESULT Machine::setRemoveSavedStateFile(BOOL aRemove)
-{
-    NOREF(aRemove);
-    ReturnComNotImplemented();
-}
+
+/**
+ * Static Machine method that can get passed to RTThreadCreate to
+ * have a thread started for a Task. See Machine::Task.
+ */
+/* static */ DECLCALLBACK(int) Machine::taskHandler(RTTHREAD /* thread */, void *pvUser)
+{
+    AssertReturn(pvUser, VERR_INVALID_POINTER);
+
+    Task *pTask = static_cast<Task *>(pvUser);
+    pTask->handler();
+    /** @todo r=klaus it would be safer to update the progress object here,
+     * as it avoids possible races due to scoping issues/tricks in the handler */
+    // it's our responsibility to delete the task
+    delete pTask;
+
+    return 0;
+}
+
+/*static*/
+HRESULT Machine::i_setErrorStatic(HRESULT aResultCode, const char *pcszMsg, ...)
+{
+    va_list args;
+    va_start(args, pcszMsg);
+    HRESULT rc = setErrorInternal(aResultCode,
+                                  getStaticClassIID(),
+                                  getStaticComponentName(),
+                                  Utf8Str(pcszMsg, args),
+                                  false /* aWarning */,
+                                  true /* aLogIt */);
+    va_end(args);
+    return rc;
+}
+
 
 HRESULT Machine::updateState(MachineState_T aState)
@@ -14558 +14685 @@
 }
 
-HRESULT Machine::beginSavingState(ComPtr<IProgress> &aProgress,
-                                  com::Utf8Str &aStateFilePath)
-{
-    NOREF(aProgress);
-    NOREF(aStateFilePath);
-    ReturnComNotImplemented();
-}
-
-HRESULT Machine::endSavingState(LONG aResult,
-                                const com::Utf8Str &aErrMsg)
-{
-    NOREF(aResult);
-    NOREF(aErrMsg);
-    ReturnComNotImplemented();
-}
-
-HRESULT Machine::adoptSavedState(const com::Utf8Str &aSavedStateFile)
-{
-    NOREF(aSavedStateFile);
-    ReturnComNotImplemented();
-}
-
-HRESULT Machine::beginTakingSnapshot(const ComPtr<IConsole> &aInitiator,
-                                     const com::Utf8Str &aName,
-                                     const com::Utf8Str &aDescription,
-                                     const ComPtr<IProgress> &aConsoleProgress,
-                                     BOOL aFTakingSnapshotOnline,
-                                     com::Utf8Str &aStateFilePath)
-{
-    NOREF(aInitiator);
-    NOREF(aName);
-    NOREF(aDescription);
-    NOREF(aConsoleProgress);
-    NOREF(aFTakingSnapshotOnline);
-    NOREF(aStateFilePath);
-    ReturnComNotImplemented();
-}
-
-HRESULT Machine::endTakingSnapshot(BOOL aSuccess)
-{
-    NOREF(aSuccess);
-    ReturnComNotImplemented();
-}
-
-HRESULT Machine::deleteSnapshot(const ComPtr<IConsole> &aInitiator,
-                                const com::Guid &aStartId,
-                                const com::Guid &aEndId,
-                                BOOL aDeleteAllChildren,
-                                MachineState_T *aMachineState,
-                                ComPtr<IProgress> &aProgress)
-{
-    NOREF(aInitiator);
-    NOREF(aStartId);
-    NOREF(aEndId);
-    NOREF(aDeleteAllChildren);
-    NOREF(aMachineState);
-    NOREF(aProgress);
-    ReturnComNotImplemented();
-}
-
 HRESULT Machine::finishOnlineMergeMedium()
 {
-    ReturnComNotImplemented();
-}
-
-HRESULT Machine::restoreSnapshot(const ComPtr<IConsole> &aInitiator,
-                                 const ComPtr<ISnapshot> &aSnapshot,
-                                 MachineState_T *aMachineState,
-                                 ComPtr<IProgress> &aProgress)
-{
-    NOREF(aInitiator);
-    NOREF(aSnapshot);
-    NOREF(aMachineState);
-    NOREF(aProgress);
     ReturnComNotImplemented();
 }

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

@@ -5 +5 @@
 
 /*
- * Copyright (C) 2008-2014 Oracle Corporation
+ * Copyright (C) 2008-2015 Oracle Corporation
  *
  * This file is part of VirtualBox Open Source Edition (OSE), as
@@ -311 +311 @@
 
         /* get the associated console; this is a remote call (!) */
-        ret = directControl->GetRemoteConsole(mConsole.asOutParam());
+        ret = directControl->COMGETTER(RemoteConsole)(mConsole.asOutParam());
         if (ret != S_OK)
             return ret;

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

@@ -412 +412 @@
 }
 
-HRESULT Snapshot::getChildrenCount(ULONG* count)
+HRESULT Snapshot::getChildrenCount(ULONG *count)
 {
     *count = i_getChildrenCount();
@@ -1305 +1305 @@
 
 /**
- * Abstract base class for SessionMachine::RestoreSnapshotTask and
- * SessionMachine::DeleteSnapshotTask. This is necessary since
- * RTThreadCreate cannot call a method as its thread function, so
- * instead we have it call the static SessionMachine::taskHandler,
- * which can then call the handler() method in here (implemented
- * by the children).
+ * Still abstract base class for SessionMachine::TakeSnapshotTask,
+ * SessionMachine::RestoreSnapshotTask and SessionMachine::DeleteSnapshotTask.
  */
 struct SessionMachine::SnapshotTask
+    : public SessionMachine::Task
 {
     SnapshotTask(SessionMachine *m,
                  Progress *p,
+                 const Utf8Str &t,
                  Snapshot *s)
-        : pMachine(m),
-          pProgress(p),
-          machineStateBackup(m->mData->mMachineState), // save the current machine state
-          pSnapshot(s)
+        : Task(m, p, t),
+          m_pSnapshot(s)
     {}
 
-    void modifyBackedUpState(MachineState_T s)
-    {
-        *const_cast<MachineState_T*>(&machineStateBackup) = s;
-    }
-
-    virtual void handler() = 0;
-
-    ComObjPtr<SessionMachine>       pMachine;
-    ComObjPtr<Progress>             pProgress;
-    const MachineState_T            machineStateBackup;
-    ComObjPtr<Snapshot>             pSnapshot;
+    ComObjPtr<Snapshot> m_pSnapshot;
 };
 
-/** Restore snapshot state task */
+/** Take snapshot task */
+struct SessionMachine::TakeSnapshotTask
+    : public SessionMachine::SnapshotTask
+{
+    TakeSnapshotTask(SessionMachine *m,
+                     Progress *p,
+                     const Utf8Str &t,
+                     Snapshot *s,
+                     const Utf8Str &strName,
+                     const Utf8Str &strDescription,
+                     bool fPause,
+                     uint32_t uMemSize,
+                     bool fTakingSnapshotOnline)
+        : SnapshotTask(m, p, t, s),
+          m_strName(strName),
+          m_strDescription(strDescription),
+          m_fPause(fPause),
+          m_uMemSize(uMemSize),
+          m_fTakingSnapshotOnline(fTakingSnapshotOnline)
+    {
+        if (fTakingSnapshotOnline)
+            m_pDirectControl = m->mData->mSession.mDirectControl;
+    }
+
+    void handler()
+    {
+        ((SessionMachine *)(Machine *)m_pMachine)->i_takeSnapshotHandler(*this);
+    }
+
+    Utf8Str m_strName;
+    Utf8Str m_strDescription;
+    Utf8Str m_strStateFilePath;
+    ComPtr<IInternalSessionControl> m_pDirectControl;
+    bool m_fPause;
+    uint32_t m_uMemSize;
+    bool m_fTakingSnapshotOnline;
+};
+
+/** Restore snapshot task */
 struct SessionMachine::RestoreSnapshotTask
     : public SessionMachine::SnapshotTask
@@ -1342 +1366 @@
     RestoreSnapshotTask(SessionMachine *m,
                         Progress *p,
+                        const Utf8Str &t,
                         Snapshot *s)
-        : SnapshotTask(m, p, s)
+        : SnapshotTask(m, p, t, s)
     {}
 
     void handler()
     {
-        pMachine->i_restoreSnapshotHandler(*this);
+        ((SessionMachine *)(Machine *)m_pMachine)->i_restoreSnapshotHandler(*this);
     }
 };
@@ -1358 +1383 @@
     DeleteSnapshotTask(SessionMachine *m,
                        Progress *p,
+                       const Utf8Str &t,
                        bool fDeleteOnline,
                        Snapshot *s)
-        : SnapshotTask(m, p, s),
+        : SnapshotTask(m, p, t, s),
           m_fDeleteOnline(fDeleteOnline)
     {}
@@ -1366 +1392 @@
     void handler()
     {
-        pMachine->i_deleteSnapshotHandler(*this);
+        ((SessionMachine *)(Machine *)m_pMachine)->i_deleteSnapshotHandler(*this);
     }
 
@@ -1372 +1398 @@
 };
 
-/**
- * Static SessionMachine method that can get passed to RTThreadCreate to
- * have a thread started for a SnapshotTask. See SnapshotTask above.
- *
- * This calls either RestoreSnapshotTask::handler() or DeleteSnapshotTask::handler().
- */
-
-/* static */ DECLCALLBACK(int) SessionMachine::taskHandler(RTTHREAD /* thread */, void *pvUser)
-{
-    AssertReturn(pvUser, VERR_INVALID_POINTER);
-
-    SnapshotTask *task = static_cast<SnapshotTask*>(pvUser);
-    task->handler();
-
-    // it's our responsibility to delete the task
-    delete task;
-
-    return 0;
-}
 
 ////////////////////////////////////////////////////////////////////////////////
 //
-// TakeSnapshot methods (SessionMachine and related tasks)
+// TakeSnapshot methods (Machine and related tasks)
 //
 ////////////////////////////////////////////////////////////////////////////////
 
-/**
- * Implementation for IInternalMachineControl::beginTakingSnapshot().
- *
- * Gets called indirectly from Console::TakeSnapshot, which creates a
- * progress object in the client and then starts a thread
- * (Console::fntTakeSnapshotWorker) which then calls this.
- *
- * In other words, the asynchronous work for taking snapshots takes place
- * on the _client_ (in the Console). This is different from restoring
- * or deleting snapshots, which start threads on the server.
- *
- * This does the server-side work of taking a snapshot: it creates differencing
- * images for all hard disks attached to the machine and then creates a
- * Snapshot object with a corresponding SnapshotMachine to save the VM settings.
- *
- * The client's fntTakeSnapshotWorker() blocks while this takes place.
- * After this returns successfully, fntTakeSnapshotWorker() will begin
- * saving the machine state to the snapshot object and reconfigure the
- * hard disks.
- *
- * When the console is done, it calls SessionMachine::EndTakingSnapshot().
- *
- * @note Locks mParent + this object for writing.
- *
- * @param aInitiator in: The console on which Console::TakeSnapshot was called.
- * @param aName  in: The name for the new snapshot.
- * @param aDescription  in: A description for the new snapshot.
- * @param aConsoleProgress  in: The console's (client's) progress object.
- * @param fTakingSnapshotOnline  in: True if an online snapshot is being taken (i.e. machine is running).
- * @param aStateFilePath out: name of file in snapshots folder to which the console should write the VM state.
+HRESULT Machine::takeSnapshot(const com::Utf8Str &aName,
+                              const com::Utf8Str &aDescription,
+                              BOOL fPause,
+                              ComPtr<IProgress> &aProgress)
+{
+    NOREF(aName);
+    NOREF(aDescription);
+    NOREF(fPause);
+    NOREF(aProgress);
+    ReturnComNotImplemented();
+}
+
+HRESULT SessionMachine::takeSnapshot(const com::Utf8Str &aName,
+                                     const com::Utf8Str &aDescription,
+                                     BOOL fPause,
+                                     ComPtr<IProgress> &aProgress)
+{
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+    LogFlowThisFunc(("aName='%s' mMachineState=%d\n", aName.c_str(), mData->mMachineState));
+
+    if (Global::IsTransient(mData->mMachineState))
+        return setError(VBOX_E_INVALID_VM_STATE,
+                        tr("Cannot take a snapshot of the machine while it is changing the state (machine state: %s)"),
+                        Global::stringifyMachineState(mData->mMachineState));
+
+    HRESULT rc = S_OK;
+
+    // prepare the progress object:
+    // a) count the no. of hard disk attachments to get a matching no. of progress sub-operations
+    ULONG cOperations = 2;              // always at least setting up + finishing up
+    ULONG ulTotalOperationsWeight = 2;  // one each for setting up + finishing up
+
+    for (MediaData::AttachmentList::iterator it = mMediaData->mAttachments.begin();
+         it != mMediaData->mAttachments.end();
+         ++it)
+    {
+        const ComObjPtr<MediumAttachment> pAtt(*it);
+        AutoReadLock attlock(pAtt COMMA_LOCKVAL_SRC_POS);
+        AutoCaller attCaller(pAtt);
+        if (pAtt->i_getType() == DeviceType_HardDisk)
+        {
+            ++cOperations;
+
+            // assume that creating a diff image takes as long as saving a 1MB state
+            ulTotalOperationsWeight += 1;
+        }
+    }
+
+    // b) one extra sub-operations for online snapshots OR offline snapshots that have a saved state (needs to be copied)
+    const bool fTakingSnapshotOnline = Global::IsOnline(mData->mMachineState);
+    LogFlowThisFunc(("fTakingSnapshotOnline = %d\n", fTakingSnapshotOnline));
+    if (fTakingSnapshotOnline)
+    {
+        ++cOperations;
+        ulTotalOperationsWeight += mHWData->mMemorySize;
+    }
+
+    // finally, create the progress object
+    ComObjPtr<Progress> pProgress;
+    pProgress.createObject();
+    rc = pProgress->init(mParent,
+                         static_cast<IMachine *>(this),
+                         Bstr(tr("Taking a snapshot of the virtual machine")).raw(),
+                         fTakingSnapshotOnline /* aCancelable */,
+                         cOperations,
+                         ulTotalOperationsWeight,
+                         Bstr(tr("Setting up snapshot operation")).raw(),      // first sub-op description
+                         1);        // ulFirstOperationWeight
+    if (FAILED(rc))
+        return rc;
+
+    /* create and start the task on a separate thread (note that it will not
+     * start working until we release alock) */
+    TakeSnapshotTask *pTask = new TakeSnapshotTask(this,
+                                                   pProgress,
+                                                   "TakeSnap",
+                                                   NULL /* pSnapshot */,
+                                                   aName,
+                                                   aDescription,
+                                                   !!fPause,
+                                                   mHWData->mMemorySize,
+                                                   fTakingSnapshotOnline);
+    rc = pTask->createThread();
+    if (FAILED(rc))
+        return rc;
+
+    /* set the proper machine state (note: after creating a Task instance) */
+    if (fTakingSnapshotOnline)
+    {
+        if (pTask->m_machineStateBackup != MachineState_Paused && !fPause)
+            i_setMachineState(MachineState_LiveSnapshotting);
+        else
+            i_setMachineState(MachineState_OnlineSnapshotting);
+        i_updateMachineStateOnClient();
+    }
+    else
+        i_setMachineState(MachineState_Snapshotting);
+
+    pTask->m_pProgress.queryInterfaceTo(aProgress.asOutParam());
+
+    return rc;
+}
+
+/**
+ * Task thread implementation for SessionMachine::TakeSnapshot(), called from
+ * SessionMachine::taskHandler().
+ *
+ * @note Locks this object for writing.
+ *
+ * @param task
  * @return
  */
-HRESULT SessionMachine::beginTakingSnapshot(const ComPtr<IConsole> &aInitiator,
-                                            const com::Utf8Str &aName,
-                                            const com::Utf8Str &aDescription,
-                                            const ComPtr<IProgress> &aConsoleProgress,
-                                            BOOL  aFTakingSnapshotOnline,
-                                            com::Utf8Str &aStateFilePath)
-{
-    NOREF(aInitiator);
+void SessionMachine::i_takeSnapshotHandler(TakeSnapshotTask &task)
+{
     LogFlowThisFuncEnter();
 
-    LogFlowThisFunc(("aName='%s' aFTakingSnapshotOnline=%RTbool\n", aName.c_str(), aFTakingSnapshotOnline));
+    // Taking a snapshot consists of the following:
+    // 1) creating a Snapshot object with the current state of the machine
+    //    (hardware + storage)
+    // 2) creating a diff image for each virtual hard disk, into which write
+    //    operations go after the snapshot has been created
+    // 3) if the machine is online: saving the state of the virtual machine
+    //    (in the VM process)
+    // 4) reattach the hard disks
+    // 5) update the various snapshot/machine objects, save settings
+
+    HRESULT rc = S_OK;
+    AutoCaller autoCaller(this);
+    LogFlowThisFunc(("state=%d\n", getObjectState().getState()));
+    if (FAILED(autoCaller.rc()))
+    {
+        /* we might have been uninitialized because the session was accidentally
+         * closed by the client, so don't assert */
+        rc = setError(E_FAIL,
+                      tr("The session has been accidentally closed"));
+        task.m_pProgress->i_notifyComplete(rc);
+        LogFlowThisFuncLeave();
+        return;
+    }
 
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    AssertReturn(    !Global::IsOnlineOrTransient(mData->mMachineState)
-                  || mData->mMachineState == MachineState_Running
-                  || mData->mMachineState == MachineState_Paused, E_FAIL);
-    AssertReturn(mConsoleTaskData.mLastState == MachineState_Null, E_FAIL);
-    AssertReturn(mConsoleTaskData.mSnapshot.isNull(), E_FAIL);
-
-    if (   mData->mCurrentSnapshot
-        && mData->mCurrentSnapshot->i_getDepth() >= SETTINGS_SNAPSHOT_DEPTH_MAX)
-    {
-
-        return setError(VBOX_E_INVALID_OBJECT_STATE,
-                        tr("Cannot take another snapshot for machine '%s', because it exceeds the maximum snapshot depth limit. Please delete some earlier snapshot which you no longer need"),
-                        mUserData->s.strName.c_str());
-    }
-
-    if (    !aFTakingSnapshotOnline
-         && mData->mMachineState != MachineState_Saved
-       )
-    {
-        /* save all current settings to ensure current changes are committed and
-         * hard disks are fixed up */
-        HRESULT rc = i_saveSettings(NULL);
+    bool fBeganTakingSnapshot = false;
+    BOOL fSuspendedBySave     = FALSE;
+
+    try
+    {
+        // @todo: at this point we have to be in the right state!!!!
+        AssertStmt(   !Global::IsOnlineOrTransient(mData->mMachineState)
+                   || mData->mMachineState == MachineState_Snapshotting
+                   || mData->mMachineState == MachineState_OnlineSnapshotting
+                   || mData->mMachineState == MachineState_LiveSnapshotting, throw E_FAIL);
+        AssertStmt(task.m_machineStateBackup != mData->mMachineState, throw E_FAIL);
+        AssertStmt(task.m_pSnapshot.isNull(), throw E_FAIL);
+
+        if (   mData->mCurrentSnapshot
+            && mData->mCurrentSnapshot->i_getDepth() >= SETTINGS_SNAPSHOT_DEPTH_MAX)
+        {
+            throw setError(VBOX_E_INVALID_OBJECT_STATE,
+                           tr("Cannot take another snapshot for machine '%s', because it exceeds the maximum snapshot depth limit. Please delete some earlier snapshot which you no longer need"),
+                           mUserData->s.strName.c_str());
+        }
+
+        if (    !task.m_fTakingSnapshotOnline
+             && mData->mMachineState != MachineState_Saved)
+        {
+            /* save settings to ensure current changes are committed and
+             * hard disks are fixed up */
+            rc = i_saveSettings(NULL);
                 // no need to check for whether VirtualBox.xml needs changing since
                 // we can't have a machine XML rename pending at this point
-        if (FAILED(rc)) return rc;
-    }
-
-    /* create an ID for the snapshot */
-    Guid snapshotId;
-    snapshotId.create();
-
-    /* stateFilePath is null when the machine is not online nor saved */
-    if (aFTakingSnapshotOnline)
-    {
-        Bstr value;
-        HRESULT rc = GetExtraData(Bstr("VBoxInternal2/ForceTakeSnapshotWithoutState").raw(),
-                                  value.asOutParam());
-        if (FAILED(rc) || value != "1")
-            // creating a new online snapshot: we need a fresh saved state file
-            i_composeSavedStateFilename(aStateFilePath);
-    }
-    else if (mData->mMachineState == MachineState_Saved)
-        // taking an online snapshot from machine in "saved" state: then use existing state file
-        aStateFilePath = mSSData->strStateFilePath;
-
-    if (aStateFilePath.isNotEmpty())
-    {
-        // ensure the directory for the saved state file exists
-        HRESULT rc = VirtualBox::i_ensureFilePathExists(aStateFilePath, true /* fCreate */);
-        if (FAILED(rc)) return rc;
-    }
-
-    /* create a snapshot machine object */
-    ComObjPtr<SnapshotMachine> snapshotMachine;
-    snapshotMachine.createObject();
-    HRESULT rc = snapshotMachine->init(this, snapshotId.ref(), aStateFilePath);
-    AssertComRCReturn(rc, rc);
-
-    /* create a snapshot object */
-    RTTIMESPEC time;
-    ComObjPtr<Snapshot> pSnapshot;
-    pSnapshot.createObject();
-    rc = pSnapshot->init(mParent,
-                         snapshotId,
-                         aName,
-                         aDescription,
-                         *RTTimeNow(&time),
-                         snapshotMachine,
-                         mData->mCurrentSnapshot);
-    AssertComRCReturnRC(rc);
-
-    /* fill in the snapshot data */
-    mConsoleTaskData.mLastState = mData->mMachineState;
-    mConsoleTaskData.mSnapshot = pSnapshot;
-
-    /// @todo in the long run the progress object should be moved to
-    // VBoxSVC to avoid trouble with monitoring the progress object state
-    // when the process where it lives is terminating shortly after the
-    // operation completed.
-
-    try
-    {
+            if (FAILED(rc))
+                throw rc;
+        }
+
+        /* task.m_strStateFilePath is "" when the machine is offline or saved */
+        if (task.m_fTakingSnapshotOnline)
+        {
+            Bstr value;
+            HRESULT rc = GetExtraData(Bstr("VBoxInternal2/ForceTakeSnapshotWithoutState").raw(),
+                                      value.asOutParam());
+            if (FAILED(rc) || value != "1")
+                // creating a new online snapshot: we need a fresh saved state file
+                i_composeSavedStateFilename(task.m_strStateFilePath);
+        }
+        else if (mData->mMachineState == MachineState_Saved)
+            // taking an offline snapshot from machine in "saved" state: use existing state file
+            task.m_strStateFilePath = mSSData->strStateFilePath;
+
+        if (task.m_strStateFilePath.isNotEmpty())
+        {
+            // ensure the directory for the saved state file exists
+            rc = VirtualBox::i_ensureFilePathExists(task.m_strStateFilePath, true /* fCreate */);
+            if (FAILED(rc))
+                throw rc;
+        }
+
+        /* STEP 1: create the snapshot object */
+
+        /* create an ID for the snapshot */
+        Guid snapshotId;
+        snapshotId.create();
+
+        /* create a snapshot machine object */
+        ComObjPtr<SnapshotMachine> pSnapshotMachine;
+        pSnapshotMachine.createObject();
+        rc = pSnapshotMachine->init(this, snapshotId.ref(), task.m_strStateFilePath);
+        AssertComRCThrowRC(rc);
+
+        /* create a snapshot object */
+        RTTIMESPEC time;
+        RTTimeNow(&time);
+        task.m_pSnapshot.createObject();
+        rc = task.m_pSnapshot->init(mParent,
+                                    snapshotId,
+                                    task.m_strName,
+                                    task.m_strDescription,
+                                    time,
+                                    pSnapshotMachine,
+                                    mData->mCurrentSnapshot);
+        AssertComRCThrowRC(rc);
+
+        /* STEP 2: create the diff images */
         LogFlowThisFunc(("Creating differencing hard disks (online=%d)...\n",
-                         aFTakingSnapshotOnline));
-
-        // backup the media data so we can recover if things goes wrong along the day;
-        // the matching commit() is in fixupMedia() during endSnapshot()
+                         task.m_fTakingSnapshotOnline));
+
+        // Backup the media data so we can recover if something goes wrong.
+        // The matching commit() is in fixupMedia() during SessionMachine::i_finishTakingSnapshot()
         i_setModified(IsModified_Storage);
         mMediaData.backup();
 
-        /* Console::fntTakeSnapshotWorker and friends expects this. */
-        if (mConsoleTaskData.mLastState == MachineState_Running)
-            i_setMachineState(MachineState_LiveSnapshotting);
-        else
-            i_setMachineState(MachineState_Saving); /** @todo Confusing! Saving is used for both online and offline snapshots. */
-
         alock.release();
         /* create new differencing hard disks and attach them to this machine */
-        rc = i_createImplicitDiffs(aConsoleProgress,
-                                   1,            // operation weight; must be the same as in Console::TakeSnapshot()
-                                   !!aFTakingSnapshotOnline);
+        rc = i_createImplicitDiffs(task.m_pProgress,
+                                   1,            // operation weight; must be the same as in Machine::TakeSnapshot()
+                                   task.m_fTakingSnapshotOnline);
         if (FAILED(rc))
             throw rc;
+        alock.acquire();
 
         // MUST NOT save the settings or the media registry here, because
         // this causes trouble with rolling back settings if the user cancels
         // taking the snapshot after the diff images have been created.
-    }
-    catch (HRESULT hrc)
-    {
-        LogThisFunc(("Caught %Rhrc [%s]\n", hrc, Global::stringifyMachineState(mData->mMachineState) ));
-        if (    mConsoleTaskData.mLastState != mData->mMachineState
-             && (   mConsoleTaskData.mLastState == MachineState_Running
-                  ? mData->mMachineState == MachineState_LiveSnapshotting
-                  : mData->mMachineState == MachineState_Saving)
-           )
-               i_setMachineState(mConsoleTaskData.mLastState);
-
-        pSnapshot->uninit();
-        pSnapshot.setNull();
-        mConsoleTaskData.mLastState = MachineState_Null;
-        mConsoleTaskData.mSnapshot.setNull();
-
-        rc = hrc;
-
-        // @todo r=dj what with the implicit diff that we created above? this is never cleaned up
-    }
-
-    if (!(aFTakingSnapshotOnline && SUCCEEDED(rc)))
-        aStateFilePath = "";
-
-    LogFlowThisFunc(("LEAVE - %Rhrc [%s]\n", rc, Global::stringifyMachineState(mData->mMachineState) ));
-    return rc;
-}
-
-/**
- * Implementation for IInternalMachineControl::endTakingSnapshot().
- *
+
+        fBeganTakingSnapshot = true;
+
+        /* Check sanity: for offline snapshots there must not be a saved state
+         * file name. All other combinations are valid (though online snapshots
+         * without saved state file seems inconsistent - there are exotic use
+         * cases, which need to be explicitly enabled, see the code above. */
+        if (   !task.m_fTakingSnapshotOnline
+            && !task.m_strStateFilePath.isEmpty())
+            throw setError(E_FAIL, "Invalid state of saved state file");
+
+        // STEP 3: save the VM state (if online)
+        if (task.m_fTakingSnapshotOnline)
+        {
+            task.m_pProgress->SetNextOperation(Bstr(tr("Saving the machine state")).raw(),
+                                               mHWData->mMemorySize);   // operation weight, same as computed
+                                                                        // when setting up progress object
+
+            if (task.m_strStateFilePath.isNotEmpty())
+            {
+                alock.release();
+                task.m_pProgress->i_setCancelCallback(i_takeSnapshotProgressCancelCallback, &task);
+                rc = task.m_pDirectControl->SaveStateWithReason(Reason_Snapshot,
+                                                                task.m_pProgress,
+                                                                Bstr(task.m_strStateFilePath).raw(),
+                                                                task.m_fPause,
+                                                                &fSuspendedBySave);
+                task.m_pProgress->i_setCancelCallback(NULL, NULL);
+                alock.acquire();
+                if (FAILED(rc))
+                    throw rc;
+            }
+            else
+                LogRel(("Machine: skipped saving state as part of online snapshot\n"));
+
+            if (!task.m_pProgress->i_notifyPointOfNoReturn())
+                throw setError(E_FAIL, tr("Canceled"));
+
+            // STEP 4: reattach hard disks
+            LogFlowThisFunc(("Reattaching new differencing hard disks...\n"));
+
+            task.m_pProgress->SetNextOperation(Bstr(tr("Reconfiguring medium attachments")).raw(),
+                                               1);       // operation weight, same as computed when setting up progress object
+
+            com::SafeIfaceArray<IMediumAttachment> atts;
+            rc = COMGETTER(MediumAttachments)(ComSafeArrayAsOutParam(atts));
+            if (FAILED(rc))
+                throw rc;
+
+            alock.release();
+            rc = task.m_pDirectControl->ReconfigureMediumAttachments(ComSafeArrayAsInParam(atts));
+            alock.acquire();
+            if (FAILED(rc))
+                throw rc;
+        }
+
+        /*
+         * Finalize the requested snapshot object. This will reset the
+         * machine state to the state it had at the beginning.
+         */
+        rc = i_finishTakingSnapshot(task, alock, true /*aSuccess*/);
+        // do not throw rc here because we can't call i_finishTakingSnapshot() twice
+        LogFlowThisFunc(("i_finishTakingSnapshot -> %Rhrc [mMachineState=%s]\n", rc, Global::stringifyMachineState(mData->mMachineState)));
+    }
+    catch (HRESULT rcThrown)
+    {
+        rc = rcThrown;
+        LogThisFunc(("Caught %Rhrc [mMachineState=%s]\n", rc, Global::stringifyMachineState(mData->mMachineState)));
+
+        // @todo r=klaus check that the implicit diffs created above are cleaned up im the relevant error cases
+
+        /* preserve existing error info */
+        ErrorInfoKeeper eik;
+
+        if (fBeganTakingSnapshot)
+            i_finishTakingSnapshot(task, alock, false /*aSuccess*/);
+
+        // have to postpone this to the end as i_finishTakingSnapshot() needs
+        // it for various cleanup steps
+        task.m_pSnapshot->uninit();
+        task.m_pSnapshot.setNull();
+    }
+    Assert(alock.isWriteLockOnCurrentThread());
+
+    {
+        // Keep all error information over the cleanup steps
+        ErrorInfoKeeper eik;
+
+        /*
+         * Fix up the machine state.
+         *
+         * For offline snapshots we just update the local copy, for the other
+         * variants do the entire work. This ensures that the state is in sync
+         * with the VM process (in particular the VM execution state).
+         */
+        bool fNeedClientMachineStateUpdate = false;
+        if (   mData->mMachineState == MachineState_LiveSnapshotting
+            || mData->mMachineState == MachineState_OnlineSnapshotting
+            || mData->mMachineState == MachineState_Snapshotting)
+        {
+            if (!task.m_fTakingSnapshotOnline)
+                i_setMachineState(task.m_machineStateBackup);
+            else
+            {
+                MachineState_T enmMachineState = MachineState_Null;
+                HRESULT rc2 = task.m_pDirectControl->COMGETTER(NominalState)(&enmMachineState);
+                if (FAILED(rc2) || enmMachineState == MachineState_Null)
+                {
+                    AssertMsgFailed(("state=%s\n", Global::stringifyMachineState(enmMachineState)));
+                    // pure nonsense, try to continue somehow
+                    enmMachineState = MachineState_Aborted;
+                }
+                if (enmMachineState == MachineState_Paused)
+                {
+                    if (fSuspendedBySave)
+                    {
+                        alock.release();
+                        rc2 = task.m_pDirectControl->ResumeWithReason(Reason_Snapshot);
+                        alock.acquire();
+                        if (SUCCEEDED(rc2))
+                            enmMachineState = task.m_machineStateBackup;
+                    }
+                    else
+                        enmMachineState = task.m_machineStateBackup;
+                }
+                if (enmMachineState != mData->mMachineState)
+                {
+                    fNeedClientMachineStateUpdate = true;
+                    i_setMachineState(enmMachineState);
+                }
+            }
+        }
+
+        /* check the remote state to see that we got it right. */
+        MachineState_T enmMachineState = MachineState_Null;
+        if (!task.m_pDirectControl.isNull())
+        {
+            ComPtr<IConsole> pConsole;
+            task.m_pDirectControl->COMGETTER(RemoteConsole)(pConsole.asOutParam());
+            if (!pConsole.isNull())
+                pConsole->COMGETTER(State)(&enmMachineState);
+        }
+        LogFlowThisFunc(("local mMachineState=%s remote mMachineState=%s\n",
+                         Global::stringifyMachineState(mData->mMachineState),
+                         Global::stringifyMachineState(enmMachineState)));
+
+        if (fNeedClientMachineStateUpdate)
+            i_updateMachineStateOnClient();
+    }
+
+    task.m_pProgress->i_notifyComplete(rc);
+
+    LogFlowThisFuncLeave();
+}
+
+
+/**
+ * Progress cancelation callback employed by SessionMachine::i_takeSnapshotHandler.
+ */
+/*static*/
+void SessionMachine::i_takeSnapshotProgressCancelCallback(void *pvUser)
+{
+    TakeSnapshotTask *pTask = (TakeSnapshotTask *)pvUser;
+    AssertPtrReturnVoid(pTask);
+    AssertReturnVoid(!pTask->m_pDirectControl.isNull());
+    pTask->m_pDirectControl->CancelSaveStateWithReason();
+}
+
+
+/**
  * Called by the Console when it's done saving the VM state into the snapshot
  * (if online) and reconfiguring the hard disks. See BeginTakingSnapshot() above.
@@ -1593 +1829 @@
  * @return
  */
-HRESULT SessionMachine::endTakingSnapshot(BOOL aSuccess)
+HRESULT SessionMachine::i_finishTakingSnapshot(TakeSnapshotTask &task, AutoWriteLock &alock, bool aSuccess)
 {
     LogFlowThisFunc(("\n"));
 
-    AutoWriteLock machineLock(this COMMA_LOCKVAL_SRC_POS);
+    Assert(alock.isWriteLockOnCurrentThread());
 
     AssertReturn(   !aSuccess
-                 || (    (    mData->mMachineState == MachineState_Saving
-                           || mData->mMachineState == MachineState_LiveSnapshotting)
-                      && mConsoleTaskData.mLastState != MachineState_Null
-                      && !mConsoleTaskData.mSnapshot.isNull()
-                    )
-                 , E_FAIL);
-
-    /*
-     * Restore the state we had when BeginTakingSnapshot() was called,
-     * Console::fntTakeSnapshotWorker restores its local copy when we return.
-     * If the state was Running, then let Console::fntTakeSnapshotWorker do it
-     * all to avoid races.
-     */
-    if (    mData->mMachineState != mConsoleTaskData.mLastState
-         && mConsoleTaskData.mLastState != MachineState_Running
-       )
-           i_setMachineState(mConsoleTaskData.mLastState);
+                 || mData->mMachineState == MachineState_Snapshotting
+                 || mData->mMachineState == MachineState_OnlineSnapshotting
+                 || mData->mMachineState == MachineState_LiveSnapshotting, E_FAIL);
 
     ComObjPtr<Snapshot> pOldFirstSnap = mData->mFirstSnapshot;
     ComObjPtr<Snapshot> pOldCurrentSnap = mData->mCurrentSnapshot;
 
-    bool fOnline = Global::IsOnline(mConsoleTaskData.mLastState);
+    bool fOnline = Global::IsOnline(task.m_machineStateBackup);
 
     HRESULT rc = S_OK;
@@ -1628 +1850 @@
     {
         // new snapshot becomes the current one
-        mData->mCurrentSnapshot = mConsoleTaskData.mSnapshot;
+        mData->mCurrentSnapshot = task.m_pSnapshot;
 
         /* memorize the first snapshot if necessary */
@@ -1651 +1873 @@
         /* inform callbacks */
         mParent->i_onSnapshotTaken(mData->mUuid,
-                                   mConsoleTaskData.mSnapshot->i_getId());
-        machineLock.release();
+                                   task.m_pSnapshot->i_getId());
+        alock.release();
     }
     else
@@ -1658 +1880 @@
         /* delete all differencing hard disks created (this will also attach
          * their parents back by rolling back mMediaData) */
-        machineLock.release();
+        alock.release();
 
         i_rollbackMedia();
@@ -1670 +1892 @@
             // snapshot means that a new saved state file was created, which we must
             // clean up now
-            RTFileDelete(mConsoleTaskData.mSnapshot->i_getStateFilePath().c_str());
-            machineLock.acquire();
-
-
-        mConsoleTaskData.mSnapshot->uninit();
-        machineLock.release();
+            RTFileDelete(task.m_pSnapshot->i_getStateFilePath().c_str());
+
+        alock.acquire();
+
+        task.m_pSnapshot->uninit();
+        alock.release();
 
     }
 
     /* clear out the snapshot data */
-    mConsoleTaskData.mLastState = MachineState_Null;
-    mConsoleTaskData.mSnapshot.setNull();
-
-    /* machineLock has been released already */
-
+    task.m_pSnapshot.setNull();
+
+    /* alock has been released already */
     mParent->i_saveModifiedRegistries();
+
+    alock.acquire();
 
     return rc;
@@ -1692 +1914 @@
 ////////////////////////////////////////////////////////////////////////////////
 //
-// RestoreSnapshot methods (SessionMachine and related tasks)
+// RestoreSnapshot methods (Machine and related tasks)
 //
 ////////////////////////////////////////////////////////////////////////////////
 
-/**
- * Implementation for IInternalMachineControl::restoreSnapshot().
- *
- * Gets called from Console::RestoreSnapshot(), and that's basically the
- * only thing Console does. Restoring a snapshot happens entirely on the
- * server side since the machine cannot be running.
- *
- * This creates a new thread that does the work and returns a progress
- * object to the client which is then returned to the caller of
- * Console::RestoreSnapshot().
- *
+HRESULT Machine::restoreSnapshot(const ComPtr<ISnapshot> &aSnapshot,
+                                 ComPtr<IProgress> &aProgress)
+{
+    NOREF(aSnapshot);
+    NOREF(aProgress);
+    ReturnComNotImplemented();
+}
+
+/**
+ * Restoring a snapshot happens entirely on the server side, the machine cannot be running.
+ *
+ * This creates a new thread that does the work and returns a progress object to the client.
  * Actual work then takes place in RestoreSnapshotTask::handler().
  *
  * @note Locks this + children objects for writing!
  *
- * @param aInitiator in: rhe console on which Console::RestoreSnapshot was called.
  * @param aSnapshot in: the snapshot to restore.
- * @param aMachineState in: client-side machine state.
  * @param aProgress out: progress object to monitor restore thread.
  * @return
  */
-HRESULT SessionMachine::restoreSnapshot(const ComPtr<IConsole> &aInitiator,
-                                        const ComPtr<ISnapshot> &aSnapshot,
-                                        MachineState_T *aMachineState,
+HRESULT SessionMachine::restoreSnapshot(const ComPtr<ISnapshot> &aSnapshot,
                                         ComPtr<IProgress> &aProgress)
 {
@@ -1727 +1946 @@
 
     // machine must not be running
-    ComAssertRet(!Global::IsOnlineOrTransient(mData->mMachineState),
-                 E_FAIL);
+    if (Global::IsOnlineOrTransient(mData->mMachineState))
+        return setError(VBOX_E_INVALID_VM_STATE,
+                        tr("Cannot delete the current state of the running machine (machine state: %s)"),
+                        Global::stringifyMachineState(mData->mMachineState));
 
     ISnapshot* iSnapshot = aSnapshot;
     ComObjPtr<Snapshot> pSnapshot(static_cast<Snapshot*>(iSnapshot));
     ComObjPtr<SnapshotMachine> pSnapMachine = pSnapshot->i_getSnapshotMachine();
+
+    HRESULT rc = S_OK;
 
     // create a progress object. The number of operations is:
@@ -1758 +1981 @@
     ComObjPtr<Progress> pProgress;
     pProgress.createObject();
-    pProgress->init(mParent, aInitiator,
+    pProgress->init(mParent, static_cast<IMachine*>(this),
                     BstrFmt(tr("Restoring snapshot '%s'"), pSnapshot->i_getName().c_str()).raw(),
                     FALSE /* aCancelable */,
@@ -1768 +1991 @@
     /* create and start the task on a separate thread (note that it will not
      * start working until we release alock) */
-    RestoreSnapshotTask *task = new RestoreSnapshotTask(this,
-                                                        pProgress,
-                                                        pSnapshot);
-    int vrc = RTThreadCreate(NULL,
-                             taskHandler,
-                             (void*)task,
-                             0,
-                             RTTHREADTYPE_MAIN_WORKER,
-                             0,
-                             "RestoreSnap");
-    if (RT_FAILURE(vrc))
-    {
-        delete task;
-        ComAssertRCRet(vrc, E_FAIL);
-    }
+    RestoreSnapshotTask *pTask = new RestoreSnapshotTask(this,
+                                                         pProgress,
+                                                         "RestoreSnap",
+                                                         pSnapshot);
+    rc = pTask->createThread();
+    if (FAILED(rc))
+        return rc;
 
     /* set the proper machine state (note: after creating a Task instance) */
@@ -1789 +2004 @@
     /* return the progress to the caller */
     pProgress.queryInterfaceTo(aProgress.asOutParam());
-
-    /* return the new state to the caller */
-    *aMachineState = mData->mMachineState;
 
     LogFlowThisFuncLeave();
@@ -1808 +2020 @@
  * @note Locks mParent + this object for writing.
  *
- * @param aTask Task data.
- */
-void SessionMachine::i_restoreSnapshotHandler(RestoreSnapshotTask &aTask)
+ * @param pTask Task data.
+ */
+void SessionMachine::i_restoreSnapshotHandler(RestoreSnapshotTask &task)
 {
     LogFlowThisFuncEnter();
@@ -1821 +2033 @@
         /* we might have been uninitialized because the session was accidentally
          * closed by the client, so don't assert */
-        aTask.pProgress->i_notifyComplete(E_FAIL,
-                                          COM_IIDOF(IMachine),
-                                          getComponentName(),
-                                          tr("The session has been accidentally closed"));
+        task.m_pProgress->i_notifyComplete(E_FAIL,
+                                           COM_IIDOF(IMachine),
+                                           getComponentName(),
+                                           tr("The session has been accidentally closed"));
 
         LogFlowThisFuncLeave();
@@ -1846 +2058 @@
         /* Delete the saved state file if the machine was Saved prior to this
          * operation */
-        if (aTask.machineStateBackup == MachineState_Saved)
+        if (task.m_machineStateBackup == MachineState_Saved)
         {
             Assert(!mSSData->strStateFilePath.isEmpty());
@@ -1856 +2068 @@
             i_releaseSavedStateFile(strStateFile, NULL /* pSnapshotToIgnore */ );
 
-            aTask.modifyBackedUpState(MachineState_PoweredOff);
+            task.modifyBackedUpState(MachineState_PoweredOff);
 
             rc = i_saveStateSettings(SaveSTS_StateFilePath);
@@ -1867 +2079 @@
 
         {
-            AutoReadLock snapshotLock(aTask.pSnapshot COMMA_LOCKVAL_SRC_POS);
+            AutoReadLock snapshotLock(task.m_pSnapshot COMMA_LOCKVAL_SRC_POS);
 
             /* remember the timestamp of the snapshot we're restoring from */
-            snapshotTimeStamp = aTask.pSnapshot->i_getTimeStamp();
-
-            ComPtr<SnapshotMachine> pSnapshotMachine(aTask.pSnapshot->i_getSnapshotMachine());
+            snapshotTimeStamp = task.m_pSnapshot->i_getTimeStamp();
+
+            ComPtr<SnapshotMachine> pSnapshotMachine(task.m_pSnapshot->i_getSnapshotMachine());
 
             /* copy all hardware data from the snapshot */
@@ -1897 +2109 @@
             alock.release();
 
-            rc = i_createImplicitDiffs(aTask.pProgress,
+            rc = i_createImplicitDiffs(task.m_pProgress,
                                        1,
                                        false /* aOnline */);
@@ -1907 +2119 @@
 
             /* Note: on success, current (old) hard disks will be
-             * deassociated/deleted on #commit() called from #saveSettings() at
+             * deassociated/deleted on #commit() called from #i_saveSettings() at
              * the end. On failure, newly created implicit diffs will be
              * deleted by #rollback() at the end. */
@@ -1914 +2126 @@
             Assert(mSSData->strStateFilePath.isEmpty());
 
-            const Utf8Str &strSnapshotStateFile = aTask.pSnapshot->i_getStateFilePath();
+            const Utf8Str &strSnapshotStateFile = task.m_pSnapshot->i_getStateFilePath();
 
             if (strSnapshotStateFile.isNotEmpty())
@@ -1920 +2132 @@
                 mSSData->strStateFilePath = strSnapshotStateFile;
 
-            LogFlowThisFunc(("Setting new current snapshot {%RTuuid}\n", aTask.pSnapshot->i_getId().raw()));
+            LogFlowThisFunc(("Setting new current snapshot {%RTuuid}\n", task.m_pSnapshot->i_getId().raw()));
             /* make the snapshot we restored from the current snapshot */
-            mData->mCurrentSnapshot = aTask.pSnapshot;
+            mData->mCurrentSnapshot = task.m_pSnapshot;
         }
 
@@ -1969 +2181 @@
 
         // detach the current-state diffs that we detected above and build a list of
-        // image files to delete _after_ saveSettings()
+        // image files to delete _after_ i_saveSettings()
 
         MediaList llDiffsToDelete;
@@ -1985 +2197 @@
 
             // Normally we "detach" the medium by removing the attachment object
-            // from the current machine data; saveSettings() below would then
+            // from the current machine data; i_saveSettings() below would then
             // compare the current machine data with the one in the backup
             // and actually call Medium::removeBackReference(). But that works only half
@@ -1991 +2203 @@
             // remove from machine data
             mMediaData->mAttachments.remove(pAttach);
-            // Remove it from the backup or else saveSettings will try to detach
+            // Remove it from the backup or else i_saveSettings will try to detach
             // it again and assert. The paranoia check avoids crashes (see
             // assert above) if this code is buggy and saves settings in the
@@ -2009 +2221 @@
         if (FAILED(rc))
             throw rc;
-        // unconditionally add the parent registry. We do similar in SessionMachine::EndTakingSnapshot
-        // (mParent->saveSettings())
 
         // release the locks before updating registry and deleting image files
         alock.release();
 
+        // unconditionally add the parent registry.
         mParent->i_markRegistryModified(mParent->i_getGlobalRegistryId());
 
@@ -2028 +2239 @@
             HRESULT rc2 = pMedium->i_deleteStorage(NULL /* aProgress */,
                                                    true /* aWait */);
-            // ignore errors here because we cannot roll back after saveSettings() above
+            // ignore errors here because we cannot roll back after i_saveSettings() above
             if (SUCCEEDED(rc2))
                 pMedium->uninit();
@@ -2049 +2260 @@
         {
             /* restore the machine state */
-            i_setMachineState(aTask.machineStateBackup);
+            i_setMachineState(task.m_machineStateBackup);
             i_updateMachineStateOnClient();
         }
@@ -2057 +2268 @@
 
     /* set the result (this will try to fetch current error info on failure) */
-    aTask.pProgress->i_notifyComplete(rc);
+    task.m_pProgress->i_notifyComplete(rc);
 
     if (SUCCEEDED(rc))
@@ -2073 +2284 @@
 ////////////////////////////////////////////////////////////////////////////////
 
-/**
- * Implementation for IInternalMachineControl::deleteSnapshot().
- *
- * Gets called from Console::DeleteSnapshot(), and that's basically the
- * only thing Console does initially. Deleting a snapshot happens entirely on
- * the server side if the machine is not running, and if it is running then
- * the individual merges are done via internal session callbacks.
+HRESULT Machine::deleteSnapshot(const com::Guid &aId, ComPtr<IProgress> &aProgress)
+{
+    NOREF(aId);
+    NOREF(aProgress);
+    ReturnComNotImplemented();
+}
+
+HRESULT SessionMachine::deleteSnapshot(const com::Guid &aId, ComPtr<IProgress> &aProgress)
+{
+    return i_deleteSnapshot(aId, aId,
+                            FALSE /* fDeleteAllChildren */,
+                            aProgress);
+}
+
+HRESULT Machine::deleteSnapshotAndAllChildren(const com::Guid &aId, ComPtr<IProgress> &aProgress)
+{
+    NOREF(aId);
+    NOREF(aProgress);
+    ReturnComNotImplemented();
+}
+
+HRESULT SessionMachine::deleteSnapshotAndAllChildren(const com::Guid &aId, ComPtr<IProgress> &aProgress)
+{
+    return i_deleteSnapshot(aId, aId,
+                            TRUE /* fDeleteAllChildren */,
+                            aProgress);
+}
+
+HRESULT Machine::deleteSnapshotRange(const com::Guid &aStartId, const com::Guid &aEndId, ComPtr<IProgress> &aProgress)
+{
+    NOREF(aStartId);
+    NOREF(aEndId);
+    NOREF(aProgress);
+    ReturnComNotImplemented();
+}
+
+HRESULT SessionMachine::deleteSnapshotRange(const com::Guid &aStartId, const com::Guid &aEndId, ComPtr<IProgress> &aProgress)
+{
+    return i_deleteSnapshot(aStartId, aEndId,
+                            FALSE /* fDeleteAllChildren */,
+                            aProgress);
+}
+
+
+/**
+ * Implementation for SessionMachine::i_deleteSnapshot().
+ *
+ * Gets called from SessionMachine::DeleteSnapshot(). Deleting a snapshot
+ * happens entirely on the server side if the machine is not running, and
+ * if it is running then the merges are done via internal session callbacks.
  *
  * This creates a new thread that does the work and returns a progress
- * object to the client which is then returned to the caller of
- * Console::DeleteSnapshot().
- *
- * Actual work then takes place in DeleteSnapshotTask::handler().
+ * object to the client.
+ *
+ * Actual work then takes place in SessionMachine::i_deleteSnapshotHandler().
  *
  * @note Locks mParent + this + children objects for writing!
  */
-HRESULT SessionMachine::deleteSnapshot(const ComPtr<IConsole> &aInitiator,
-                                       const com::Guid &aStartId,
-                                       const com::Guid &aEndId,
-                                       BOOL aDeleteAllChildren,
-                                       MachineState_T *aMachineState,
-                                       ComPtr<IProgress> &aProgress)
+HRESULT SessionMachine::i_deleteSnapshot(const com::Guid &aStartId,
+                                         const com::Guid &aEndId,
+                                         BOOL aDeleteAllChildren,
+                                         ComPtr<IProgress> &aProgress)
 {
     LogFlowThisFuncEnter();
 
-    AssertReturn(aInitiator && !aStartId.isZero() && !aEndId.isZero() && aStartId.isValid() && aEndId.isValid(), E_INVALIDARG);
+    AssertReturn(!aStartId.isZero() && !aEndId.isZero() && aStartId.isValid() && aEndId.isValid(), E_INVALIDARG);
 
 
@@ -2106 +2357 @@
 
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    if (Global::IsTransient(mData->mMachineState))
+        return setError(VBOX_E_INVALID_VM_STATE,
+                        tr("Cannot delete a snapshot of the machine while it is changing the state (machine state: %s)"),
+                        Global::stringifyMachineState(mData->mMachineState));
 
     // be very picky about machine states
@@ -2200 +2456 @@
     ComObjPtr<Progress> pProgress;
     pProgress.createObject();
-    pProgress->init(mParent, aInitiator,
+    pProgress->init(mParent, static_cast<IMachine*>(this),
                     BstrFmt(tr("Deleting snapshot '%s'"), pSnapshot->i_getName().c_str()).raw(),
                     FALSE /* aCancelable */,
@@ -2212 +2468 @@
 
     /* create and start the task on a separate thread */
-    DeleteSnapshotTask *task = new DeleteSnapshotTask(this, pProgress,
-                                                      fDeleteOnline, pSnapshot);
-    int vrc = RTThreadCreate(NULL,
-                             taskHandler,
-                             (void*)task,
-                             0,
-                             RTTHREADTYPE_MAIN_WORKER,
-                             0,
-                             "DeleteSnapshot");
-    if (RT_FAILURE(vrc))
-    {
-        delete task;
-        return E_FAIL;
-    }
+    DeleteSnapshotTask *pTask = new DeleteSnapshotTask(this, pProgress,
+                                                       "DeleteSnap",
+                                                       fDeleteOnline,
+                                                       pSnapshot);
+    rc = pTask->createThread();
+    if (FAILED(rc))
+        return rc;
 
     // the task might start running but will block on acquiring the machine's write lock
@@ -2236 +2485 @@
     else
         i_setMachineState(MachineState_DeletingSnapshot);
+    i_updateMachineStateOnClient();
 
     /* return the progress to the caller */
     pProgress.queryInterfaceTo(aProgress.asOutParam());
-
-    /* return the new state to the caller */
-    *aMachineState = mData->mMachineState;
 
     LogFlowThisFuncLeave();
@@ -2341 +2588 @@
  * @note Locks the machine + the snapshot + the media tree for writing!
  *
- * @param aTask Task data.
- */
-
-void SessionMachine::i_deleteSnapshotHandler(DeleteSnapshotTask &aTask)
+ * @param pTask Task data.
+ */
+void SessionMachine::i_deleteSnapshotHandler(DeleteSnapshotTask &task)
 {
     LogFlowThisFuncEnter();
 
+    HRESULT rc = S_OK;
     AutoCaller autoCaller(this);
-
     LogFlowThisFunc(("state=%d\n", getObjectState().getState()));
-    if (!autoCaller.isOk())
+    if (FAILED(autoCaller.rc()))
     {
         /* we might have been uninitialized because the session was accidentally
          * closed by the client, so don't assert */
-        aTask.pProgress->i_notifyComplete(E_FAIL,
-                                          COM_IIDOF(IMachine),
-                                          getComponentName(),
-                                          tr("The session has been accidentally closed"));
+        rc = setError(E_FAIL,
+                      tr("The session has been accidentally closed"));
+        task.m_pProgress->i_notifyComplete(rc);
         LogFlowThisFuncLeave();
         return;
     }
 
-    HRESULT rc = S_OK;
     MediumDeleteRecList toDelete;
     Guid snapshotId;
@@ -2371 +2615 @@
         /* Locking order:  */
         AutoMultiWriteLock2 multiLock(this->lockHandle(),                   // machine
-                                      aTask.pSnapshot->lockHandle()         // snapshot
+                                      task.m_pSnapshot->lockHandle()        // snapshot
                                       COMMA_LOCKVAL_SRC_POS);
         // once we have this lock, we know that SessionMachine::DeleteSnapshot()
@@ -2379 +2623 @@
                                COMMA_LOCKVAL_SRC_POS);
 
-        ComObjPtr<SnapshotMachine> pSnapMachine = aTask.pSnapshot->i_getSnapshotMachine();
+        ComObjPtr<SnapshotMachine> pSnapMachine = task.m_pSnapshot->i_getSnapshotMachine();
         // no need to lock the snapshot machine since it is const by definition
         Guid machineId = pSnapMachine->i_getId();
 
         // save the snapshot ID (for callbacks)
-        snapshotId = aTask.pSnapshot->i_getId();
+        snapshotId = task.m_pSnapshot->i_getId();
 
         // first pass:
@@ -2428 +2672 @@
             MediumLockList *pChildrenToReparent = NULL;
             bool fNeedsOnlineMerge = false;
-            bool fOnlineMergePossible = aTask.m_fDeleteOnline;
+            bool fOnlineMergePossible = task.m_fDeleteOnline;
             MediumLockList *pMediumLockList = NULL;
             MediumLockList *pVMMALockList = NULL;
@@ -2666 +2910 @@
             AutoWriteLock machineLock(this COMMA_LOCKVAL_SRC_POS);
 
-            Utf8Str stateFilePath = aTask.pSnapshot->i_getStateFilePath();
+            Utf8Str stateFilePath = task.m_pSnapshot->i_getStateFilePath();
             if (!stateFilePath.isEmpty())
             {
-                aTask.pProgress->SetNextOperation(Bstr(tr("Deleting the execution state")).raw(),
-                                                  1);        // weight
-
-                i_releaseSavedStateFile(stateFilePath, aTask.pSnapshot /* pSnapshotToIgnore */);
+                task.m_pProgress->SetNextOperation(Bstr(tr("Deleting the execution state")).raw(),
+                                                   1);        // weight
+
+                i_releaseSavedStateFile(stateFilePath, task.m_pSnapshot /* pSnapshotToIgnore */);
 
                 // machine will need saving now
@@ -2697 +2941 @@
             }
 
-            aTask.pProgress->SetNextOperation(BstrFmt(tr("Merging differencing image '%s'"),
-                                              pMedium->i_getName().c_str()).raw(),
-                                              ulWeight);
+            task.m_pProgress->SetNextOperation(BstrFmt(tr("Merging differencing image '%s'"),
+                                               pMedium->i_getName().c_str()).raw(),
+                                               ulWeight);
 
             bool fNeedSourceUninit = false;
@@ -2722 +2966 @@
                     /* No need to hold the lock any longer. */
                     mLock.release();
-                    rc = pMedium->i_deleteStorage(&aTask.pProgress,
+                    rc = pMedium->i_deleteStorage(&task.m_pProgress,
                                                   true /* aWait */);
                     if (FAILED(rc))
@@ -2740 +2984 @@
                     // This callback will arrive while onlineMergeMedium is
                     // still executing, and there can't be two tasks.
+                    /// @todo r=klaus this hack needs to go, and the logic needs to be "unconvoluted", putting SessionMachine in charge of coordinating the reconfig/resume.
                     mConsoleTaskData.mDeleteSnapshotInfo = (void *)&(*it);
                     // online medium merge, in the direction decided earlier
@@ -2749 +2994 @@
                                              it->mpChildrenToReparent,
                                              it->mpMediumLockList,
-                                             aTask.pProgress,
+                                             task.m_pProgress,
                                              &fNeedsSave);
                     mConsoleTaskData.mDeleteSnapshotInfo = NULL;
@@ -2761 +3006 @@
                                                  it->mpChildrenToReparent,
                                                  it->mpMediumLockList,
-                                                 &aTask.pProgress,
+                                                 &task.m_pProgress,
                                                  true /* aWait */);
                 }
@@ -2826 +3071 @@
                 ComObjPtr<Machine> pMachine = this;
                 Guid childSnapshotId;
-                ComObjPtr<Snapshot> pChildSnapshot = aTask.pSnapshot->i_getFirstChild();
+                ComObjPtr<Snapshot> pChildSnapshot = task.m_pSnapshot->i_getFirstChild();
                 if (pChildSnapshot)
                 {
@@ -2871 +3116 @@
             AutoWriteLock machineLock(this COMMA_LOCKVAL_SRC_POS);
 
-            aTask.pSnapshot->i_beginSnapshotDelete();
-            aTask.pSnapshot->uninit();
+            task.m_pSnapshot->i_beginSnapshotDelete();
+            task.m_pSnapshot->uninit();
 
             machineLock.release();
@@ -2912 +3157 @@
         // restore the machine state that was saved when the
         // task was started
-        i_setMachineState(aTask.machineStateBackup);
+        i_setMachineState(task.m_machineStateBackup);
         i_updateMachineStateOnClient();
 
@@ -2919 +3164 @@
 
     // report the result (this will try to fetch current error info on failure)
-    aTask.pProgress->i_notifyComplete(rc);
+    task.m_pProgress->i_notifyComplete(rc);
 
     if (SUCCEEDED(rc))
@@ -3061 +3306 @@
             aSource = aHD;
             aTarget = pChild;
-            LogFlowFunc(("Forward merging selected\n"));
+            LogFlowThisFunc(("Forward merging selected\n"));
         }
         else
@@ -3067 +3312 @@
             aSource = pChild;
             aTarget = aHD;
-            LogFlowFunc(("Backward merging selected\n"));
+            LogFlowThisFunc(("Backward merging selected\n"));
         }
     }
@@ -3580 +3825 @@
             AssertComRC(rc);
 
+            treeLock.release();
             pMedium->uninit();
+            treeLock.acquire();
         }