Changeset 27779 in vbox for trunk/src/VBox/Main/ApplianceImpl.cpp

Timestamp:

Mar 29, 2010 11:28:45 AM (15 years ago)

Author:

vboxsync

Message:

Main: fix locking in Appliance import/export

File:

• trunk/src/VBox/Main/ApplianceImpl.cpp (modified) (25 diffs)

trunk/src/VBox/Main/ApplianceImpl.cpp

@@ -69 +69 @@
 struct Appliance::Data
 {
+    enum ApplianceState { ApplianceIdle, ApplianceImporting, ApplianceExporting };
+
     Data()
-      : pReader(NULL) {}
+      : state(ApplianceIdle),
+        pReader(NULL)
+    {
+    }
 
     ~Data()
@@ -81 +86 @@
     }
 
-    LocationInfo locInfo; /* The location info for the currently processed OVF */
-
-    OVFReader *pReader;
-
-    list< ComObjPtr<VirtualSystemDescription> > virtualSystemDescriptions;
-
-    list<Utf8Str> llWarnings;
-
-    ULONG ulWeightPerOperation;
-    Utf8Str strOVFSHA1Digest;
+    ApplianceState  state;
+
+    LocationInfo    locInfo;       // location info for the currently processed OVF
+
+    OVFReader       *pReader;
+
+    bool            fBusyWriting;          // state protection; while this is true nobody else can call methods
+
+    list< ComObjPtr<VirtualSystemDescription> >
+                    virtualSystemDescriptions;
+
+    list<Utf8Str>   llWarnings;
+
+    ULONG           ulWeightPerOperation;
+    Utf8Str         strOVFSHA1Digest;
 };
 
@@ -371 +381 @@
 ////////////////////////////////////////////////////////////////////////////////
 
+/**
+ * Returns true if the appliance is in "idle" state. This should always be the
+ * case unless an import or export is currently in progress. Similar to machine
+ * states, this permits the Appliance implementation code to let go of the
+ * Appliance object lock while a time-consuming disk conversion is in progress
+ * without exposing the appliance to conflicting calls.
+ *
+ * This sets an error on "this" (the appliance) and returns false if the appliance
+ * is busy. The caller should then return E_ACCESSDENIED.
+ *
+ * Must be called from under the object lock!
+ *
+ * @return
+ */
+bool Appliance::isApplianceIdle() const
+{
+    if (m->state == Data::ApplianceImporting)
+        setError(VBOX_E_INVALID_OBJECT_STATE, "The appliance is busy importing files");
+    else if (m->state == Data::ApplianceExporting)
+        setError(VBOX_E_INVALID_OBJECT_STATE, "The appliance is busy exporting files");
+    else
+        return true;
+
+    return false;
+}
+
 HRESULT Appliance::searchUniqueVMName(Utf8Str& aName) const
 {
@@ -885 +921 @@
     HRESULT rc = S_OK;
 
-    switch(task->taskType)
+    switch (task->taskType)
     {
         case TaskImportOVF::Read:
@@ -1103 +1139 @@
 
     AutoWriteLock appLock(this COMMA_LOCKVAL_SRC_POS);
+
+    if (!isApplianceIdle())
+        return VERR_ACCESS_DENIED;
+
+    // Change the appliance state so we can safely leave the lock while doing time-consuming
+    // disk imports; also the below method calls do all kinds of locking which conflicts with
+    // the appliance object lock
+    m->state = Data::ApplianceImporting;
+    appLock.release();
 
     HRESULT rc = S_OK;
@@ -1906 +1951 @@
     }
 
+    // restore the appliance state
+    appLock.acquire();
+    m->state = Data::ApplianceIdle;
+
     pTask->rc = rc;
 
@@ -2257 +2306 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoWriteLock appLock(this COMMA_LOCKVAL_SRC_POS);
-
     HRESULT rc = S_OK;
 
     try
     {
+        AutoMultiWriteLock2 multiLock(&mVirtualBox->getMediaTreeLockHandle(), this->lockHandle() COMMA_LOCKVAL_SRC_POS);
+
         xml::Document doc;
         xml::ElementNode *pelmRoot = doc.createRootElement("Envelope");
@@ -2935 +2984 @@
         }
 
-        // finally, fill in the network section we set up empty above according
+        // now, fill in the network section we set up empty above according
         // to the networks we found with the hardware items
         map<Utf8Str, bool>::const_iterator itN;
@@ -2948 +2997 @@
         }
 
+        // Finally, write out the disks!
+
+        // This can take a very long time so leave the locks; in particular, we have the media tree
+        // lock which Medium::CloneTo() will request, and that would deadlock. Instead, protect
+        // the appliance by resetting its state so we can safely leave the lock
+        m->state = Data::ApplianceExporting;
+        multiLock.release();
+
         list<Utf8Str> diskList;
         map<Utf8Str, const VirtualSystemDescriptionEntry*>::const_iterator itS;
@@ -3003 +3060 @@
                 if (!pTask->progress.isNull())
                     pTask->progress->SetNextOperation(BstrFmt(tr("Exporting virtual disk image '%s'"), strSrcFilePath.c_str()),
-                                                     pDiskEntry->ulSizeMB);     // operation's weight, as set up with the IProgress originally);
+                                                      pDiskEntry->ulSizeMB);     // operation's weight, as set up with the IProgress originally);
 
                 // now wait for the background disk operation to complete; this throws HRESULTs on error
@@ -3079 +3136 @@
         rc = aRC;
     }
+
+    AutoWriteLock appLock(this COMMA_LOCKVAL_SRC_POS);
+    // reset the state so others can call methods again
+    m->state = Data::ApplianceIdle;
 
     pTask->rc = rc;
@@ -3278 +3339 @@
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
+    if (!isApplianceIdle())
+        return E_ACCESSDENIED;
+
     Bstr bstrPath(m->locInfo.strPath);
     bstrPath.cloneTo(aPath);
@@ -3297 +3361 @@
 
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    if (!isApplianceIdle())
+        return E_ACCESSDENIED;
 
     if (m->pReader) // OVFReader instantiated?
@@ -3356 +3423 @@
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
+    if (!isApplianceIdle())
+        return E_ACCESSDENIED;
+
     SafeIfaceArray<IVirtualSystemDescription> sfaVSD(m->virtualSystemDescriptions);
     sfaVSD.detachTo(ComSafeArrayOutArg(aVirtualSystemDescriptions));
@@ -3377 +3447 @@
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
+    if (!isApplianceIdle())
+        return E_ACCESSDENIED;
+
     if (m->pReader)
     {
@@ -3422 +3495 @@
 
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    if (!isApplianceIdle())
+        return E_ACCESSDENIED;
 
     HRESULT rc = S_OK;
@@ -3439 +3515 @@
         return setError(E_FAIL,
                         tr("Cannot interpret appliance without reading it first (call read() before interpret())"));
+
+    // Change the appliance state so we can safely leave the lock while doing time-consuming
+    // disk imports; also the below method calls do all kinds of locking which conflicts with
+    // the appliance object lock
+    m->state = Data::ApplianceImporting;
+    alock.release();
 
     /* Try/catch so we can clean up on error */
@@ -3853 +3935 @@
     }
 
+    // reset the appliance state
+    alock.acquire();
+    m->state = Data::ApplianceIdle;
+
     return rc;
 }
@@ -3870 +3956 @@
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
 
+    if (!isApplianceIdle())
+        return E_ACCESSDENIED;
+
     if (!m->pReader)
         return setError(E_FAIL,
@@ -3900 +3989 @@
 
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    if (!isApplianceIdle())
+        return E_ACCESSDENIED;
 
     ComObjPtr<VFSExplorer> explorer;
@@ -3934 +4026 @@
 
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    if (!isApplianceIdle())
+        return E_ACCESSDENIED;
 
     // see if we can handle this file; for now we insist it has an ".ovf" extension
@@ -3984 +4079 @@
 
     AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    if (!isApplianceIdle())
+        return E_ACCESSDENIED;
 
     com::SafeArray<BSTR> sfaWarnings(m->llWarnings.size());
@@ -4406 +4504 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    AutoReadLock alock1(this COMMA_LOCKVAL_SRC_POS);
-
     ComObjPtr<VirtualSystemDescription> pNewDesc;
 
@@ -4423 +4519 @@
         ComPtr<IUSBController> pUsbController;
         ComPtr<IAudioAdapter> pAudioAdapter;
+
+        // first, call the COM methods, as they request locks
+        rc = COMGETTER(USBController)(pUsbController.asOutParam());
+        if (FAILED(rc))
+            fUSBEnabled = false;
+        else
+            rc = pUsbController->COMGETTER(Enabled)(&fUSBEnabled);
+
+        // request the machine lock while acessing internal members
+        AutoReadLock alock1(this COMMA_LOCKVAL_SRC_POS);
+
+        pAudioAdapter = mAudioAdapter;
+        rc = pAudioAdapter->COMGETTER(Enabled)(&fAudioEnabled);
+        if (FAILED(rc)) throw rc;
+        rc = pAudioAdapter->COMGETTER(AudioController)(&audioController);
+        if (FAILED(rc)) throw rc;
 
         // get name
@@ -4443 +4555 @@
         // snapshotFolder?
         // VRDPServer?
-
-        // this is more tricky so use the COM method
-        rc = COMGETTER(USBController)(pUsbController.asOutParam());
-        if (FAILED(rc))
-            fUSBEnabled = false;
-        else
-            rc = pUsbController->COMGETTER(Enabled)(&fUSBEnabled);
-
-        pAudioAdapter = mAudioAdapter;
-        rc = pAudioAdapter->COMGETTER(Enabled)(&fAudioEnabled);
-        if (FAILED(rc)) throw rc;
-        rc = pAudioAdapter->COMGETTER(AudioController)(&audioController);
-        if (FAILED(rc)) throw rc;
 
         // create a new virtual system