Changeset 351 in vbox for trunk

Timestamp:

Jan 26, 2007 10:49:57 AM (18 years ago)

Author:

vboxsync

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

17908

Message:

Main: Fixed: HVirtualDiskImage::geAccessible() blocked all other methods, including simple getters [defect #1789].

Location:

trunk/src/VBox/Main

Files:

• HardDiskImpl.cpp (modified) (61 diffs)
• ProgressImpl.cpp (modified) (1 diff)
• include/HardDiskImpl.h (modified) (4 diffs)

trunk/src/VBox/Main/HardDiskImpl.cpp

@@ -67 +67 @@
     ComObjPtr <Progress> progress;
 
-    // for CreateDynamic, CreateStatic
+    /* for CreateDynamic, CreateStatic */
     uint64_t size;
 
-    // for CloneToImage
+    /* for CloneToImage */
     ComObjPtr <HardDisk> source;
 };
@@ -84 +84 @@
     Progress *progress = static_cast <Progress *> (pvUser);
 
-    // update the progress object
+    /* update the progress object */
     if (progress)
         progress->notifyProgress (uPercent);
@@ -172 +172 @@
     Assert (isReady());
 
-    // uninit all children
+    /* uninit all children */
     uninitDependentChildren();
 
@@ -332 +332 @@
         HRESULT rc = S_OK;
 
-        // check the accessibility state of all ancestors
+        /* check the accessibility state of all ancestors */
         ComObjPtr <HardDisk> parent = (HardDisk *) mParent;
         while (parent)
@@ -413 +413 @@
     HRESULT rc = S_OK;
 
-    // create a project object
+    /* create a project object */
     ComObjPtr <Progress> progress;
     progress.createObject();
@@ -421 +421 @@
     CheckComRCReturnRC (rc);
 
-    // create an imageless resulting object
+    /* create an imageless resulting object */
     ComObjPtr <HVirtualDiskImage> image;
     image.createObject();
@@ -427 +427 @@
     CheckComRCReturnRC (rc);
 
-    // append the default path if only a name is given
+    /* append the default path if only a name is given */
     Bstr path = aFilePath;
     {
@@ -441 +441 @@
     }
 
-    // set the desired path
+    /* set the desired path */
     rc = image->setFilePath (path);
     CheckComRCReturnRC (rc);
 
-    // ensure the directory exists
+    /* ensure the directory exists */
     {
         Utf8Str imageDir = image->filePath();
@@ -462 +462 @@
     }
 
-    // mark as busy (being created)
-    // (VDI task thread will unmark it)
+    /* mark as busy (being created)
+     * (VDI task thread will unmark it) */
     image->setBusy();
 
-    // fill in a VDI task data
+    /* fill in a VDI task data */
     VDITask *task = new VDITask (VDITask::CloneToImage, image, progress);
     task->source = this;
 
-    // increase readers until finished
-    // (VDI task thread will decrease them)
+    /* increase readers until finished
+     * (VDI task thread will decrease them) */
     addReader();
 
-    // create the hard disk creation thread, pass operation data
+    /* create the hard disk creation thread, pass operation data */
     int vrc = RTThreadCreate (NULL, HVirtualDiskImage::vdiTaskThread,
                               (void *) task, 0, RTTHREADTYPE_MAIN_HEAVY_WORKER,
@@ -487 +487 @@
     }
 
-    // return interfaces to the caller
+    /* return interfaces to the caller */
     image.queryInterfaceTo (aImage);
     progress.queryInterfaceTo (aProgress);
@@ -706 +706 @@
     AssertReturn (!mMachineId.isEmpty(), false);
 
-    // check all children
+    /* check all children */
     AutoLock chLock (childrenLock());
     for (HardDiskList::const_iterator it = children().begin();
@@ -746 +746 @@
         if (child->mReaders > 0 || child->mBusy)
         {
-            // reset the busy flag of all previous children
+            /* reset the busy flag of all previous children */
             while (it != children().begin())
                 (*(-- it))->clearBusy();
@@ -906 +906 @@
                                      aFolder.raw(), RTPATH_DELIMITER, id.ptr());
 
-    // try to make the path relative to the vbox home dir
+    /* try to make the path relative to the vbox home dir */
     const char *filePathToRel = filePathTo;
     {
@@ -914 +914 @@
     }
 
-    // first ensure the directory exists
+    /* first ensure the directory exists */
     {
         Utf8Str dir = aFolder;
@@ -932 +932 @@
     alock.leave();
 
-    // call storage type specific diff creation method
+    /* call storage type specific diff creation method */
     HRESULT rc = createDiffImage (id, filePathTo, aProgress);
 
@@ -945 +945 @@
     CheckComRCReturnRC (rc);
 
-    // associate the created hard disk with the given machine
+    /* associate the created hard disk with the given machine */
     vdi->setMachineId (aMachineId);
 
@@ -1011 +1011 @@
     AssertReturn (aHDNode, E_FAIL);
 
-    Guid uuid; // uuid (required)
+    Guid uuid; /* uuid (required) */
     CFGLDRQueryUUID (aHDNode, "uuid", uuid.ptr());
     mId = uuid;
@@ -1017 +1017 @@
     if (!isDifferencing())
     {
-        Bstr type; // type (required for <HardDisk> nodes only)
+        Bstr type; /* type (required for <HardDisk> nodes only) */
         CFGLDRQueryBSTR (aHDNode, "type", type.asOutParam());
         if (type == L"normal")
@@ -1036 +1036 @@
         return rc;
 
-    // load all children
+    /* load all children */
     unsigned count = 0;
     CFGLDRCountChildren (aHDNode, "DiffHardDisk", &count);
@@ -1083 +1083 @@
     AssertReturn (aHDNode, E_FAIL);
 
-    // uuid (required)
+    /* uuid (required) */
     CFGLDRSetUUID (aHDNode, "uuid", mId.ptr());
 
     if (!isDifferencing())
     {
-        // type (required)
+        /* type (required) */
         const char *type = NULL;
         switch (mType)
@@ -1107 +1107 @@
     HRESULT rc = S_OK;
 
-    // save all children
+    /* save all children */
     AutoLock chLock (childrenLock());
     for (HardDiskList::const_iterator it = children().begin();
@@ -1152 +1152 @@
 
     mState = NotCreated;
+
+    mStateCheckSem = NIL_RTSEMEVENTMULTI;
+    mStateCheckWaiters = 0;
 
     mSize = 0;
@@ -1347 +1350 @@
     CHECK_READY();
 
-    // only a non-differencing image knows the logical size
+    /* only a non-differencing image knows the logical size */
     if (isDifferencing())
         return root()->COMGETTER(Size) (aSize);
@@ -1521 +1524 @@
     CHECK_READY();
 
-    // check the basic accessibility
+    if (mStateCheckSem != NIL_RTSEMEVENTMULTI)
+    {
+        /* An accessibility check in progress on some other thread,
+         * wait for it to finish. */
+
+        ComAssertRet (mStateCheckWaiters != ~0, E_FAIL);
+        ++ mStateCheckWaiters;
+        alock.leave();
+        
+        int vrc = RTSemEventMultiWait (mStateCheckSem, RT_INDEFINITE_WAIT);
+
+        alock.enter();
+        AssertReturn (mStateCheckWaiters != 0, E_FAIL);
+        -- mStateCheckWaiters;
+        if (mStateCheckWaiters == 0)
+        {
+            RTSemEventMultiDestroy (mStateCheckSem);
+            mStateCheckSem = NIL_RTSEMEVENTMULTI;
+        }
+        
+        /* don't touch aAccessError, it has been already set */
+        return S_OK;
+    }
+
+    /* check the basic accessibility */
     HRESULT rc = HardDisk::getAccessible (aAccessError);
     if (FAILED (rc) || !aAccessError.isNull())
@@ -1529 +1556 @@
     {
         return queryInformation (&aAccessError);
-        // if we fail here, this means something like UUID mismatch.
-        // Do nothing, just return the failure (error info is already
-        // set by queryInformation()), in hope that one of subsequent
-        // attempts to check for acessibility will succeed
+        /* if we fail here, this means something like UUID mismatch.
+         * Do nothing, just return the failure (error info is already
+         * set by queryInformation()), in hope that one of subsequent
+         * attempts to check for acessibility will succeed */
     }
 
@@ -1703 +1730 @@
     releaseReader();
 
-    // update the UUID to correspond to the file name
+    /* update the UUID to correspond to the file name */
     if (VBOX_SUCCESS (vrc))
         vrc = VDISetImageUUIDs (aTargetPath, aId, NULL, NULL, NULL);
@@ -1757 +1784 @@
                                      aFolder.raw(), RTPATH_DELIMITER, id.ptr());
 
-    // try to make the path relative to the vbox home dir
+    /* try to make the path relative to the vbox home dir */
     const char *filePathToRel = filePathTo;
     {
@@ -1765 +1792 @@
     }
 
-    // first ensure the directory exists
+    /* first ensure the directory exists */
     {
         Utf8Str dir = aFolder;
@@ -1791 +1818 @@
     alock.enter();
 
-    // get modification and parent UUIDs of this image
+    /* get modification and parent UUIDs of this image */
     RTUUID modUuid, parentUuid, parentModUuid;
     if (VBOX_SUCCESS (vrc))
@@ -1815 +1842 @@
         return rc;
 
-    // associate the created hard disk with the given machine
+    /* associate the created hard disk with the given machine */
     vdi->setMachineId (aMachineId);
 
@@ -1900 +1927 @@
             AutoLock childLock (child);
 
-            // reparent the child
+            /* reparent the child */
             child->mParent = mParent;
             if (mParent)
                 mParent->addDependentChild (child);
 
-            // change the parent UUID in the image as well
+            /* change the parent UUID in the image as well */
             RTUUID parentUuid, parentModUuid;
             vrc = VDIGetImageUUIDs (Utf8Str (mParent->asVDI()->mFilePathFull),
@@ -1933 +1960 @@
     }
 
-    // detach all our children to avoid their uninit in #uninit()
+    /* detach all our children to avoid their uninit in #uninit() */
     removeDependentChildren();
 
@@ -1977 +2004 @@
     CHECK_READY();
 
-    // this must be a diff image
+    /* this must be a diff image */
     AssertReturn (isDifferencing(), E_FAIL);
 
@@ -1991 +2018 @@
         AutoLock chLock (childrenLock());
 
-        // iterate over a copy since we will modify the list
+        /* iterate over a copy since we will modify the list */
         HardDiskList list = children();
 
@@ -2023 +2050 @@
             }
 
-            // reparent the child
+            /* reparent the child */
             child->mParent = mParent;
             if (mParent)
                 mParent->addDependentChild (child);
 
-            // change the parent UUID in the image as well
+            /* change the parent UUID in the image as well */
             RTUUID parentUuid, parentModUuid;
             vrc = VDIGetImageUUIDs (Utf8Str (mParent->asVDI()->mFilePathFull),
@@ -2051 +2078 @@
             }
 
-            // detach child to avoid its uninit in #uninit()
+            /* detach child to avoid its uninit in #uninit() */
             removeDependentChild (child);
 
-            // remove the busy flag
+            /* remove the busy flag */
             child->clearBusy();
         }
@@ -2094 +2121 @@
                                     Utf8Str (mParent->asVDI()->mFilePathFull),
                                     NULL, NULL, NULL);
-    // update the UUID to correspond to the file name
+    /* update the UUID to correspond to the file name */
     if (VBOX_SUCCESS (vrc))
         vrc = VDISetImageUUIDs (filePathFull, mId, NULL, NULL, NULL);
@@ -2120 +2147 @@
     if (aFilePath && *aFilePath)
     {
-        // get the full file name
+        /* get the full file name */
         char filePathFull [RTPATH_MAX];
         int vrc = RTPathAbsEx (mVirtualBox->homeDir(), Utf8Str (aFilePath),
@@ -2141 +2168 @@
 
 /**
- *  Helper to query information about the VDI hard disk
+ *  Helper to query information about the VDI hard disk.
  *
  *  @param aAccessError not used when NULL, otherwise see #getAccessible()
- *  @note
- *      Must be called from under the object's lock!
+ *
+ *  @note Must be called from under the object's lock, only after
+ *        CHECK_BUSY_AND_READERS() succeeds.
  */
 HRESULT HVirtualDiskImage::queryInformation (Bstr *aAccessError)
 {
+    AssertReturn (isLockedOnCurrentThread(), E_FAIL);
+
+    /* create a lock object to completely release it later */
+    AutoLock alock (this);
+
+    AssertReturn (mStateCheckWaiters == 0, E_FAIL);
+
     ComAssertRet (mState >= Created, E_FAIL);
 
@@ -2154 +2189 @@
     int vrc = VINF_SUCCESS;
 
-    /* stupid-stupid-stupid code. VBoxVHDD management is sick.
-     * we're opening a file three times to get three bits of information */
+    /* lazily create a semaphore */
+    vrc = RTSemEventMultiCreate (&mStateCheckSem);
+    ComAssertRCRet (vrc, E_FAIL);
+
+    /* go to Busy state to prevent any concurrent modifications
+     * after releasing the lock below (to unblock getters before 
+     * a lengthy operation) */
+    setBusy();
+
+    alock.leave();
+    
+    /* VBoxVHDD management interface needs to be optimized: we're opening a
+     * file three times in a raw to get three bits of information. */
 
     Utf8Str filePath = mFilePathFull;
@@ -2165 +2211 @@
         vrc = VDICheckImage (filePath, NULL, NULL, NULL,
                              id.ptr(), parentId.ptr(), NULL, 0);
+
         if (VBOX_FAILURE (vrc))
         {
@@ -2252 +2299 @@
                 break;
         }
-
-        if (aAccessError)
-            aAccessError->setNull();
-
-        mState = Accessible;
     }
     while (0);
 
+    /* enter the lock again */
+    alock.enter();
+    
+    /* remove the busy flag */
+    clearBusy();
+    
     if (VBOX_FAILURE (vrc) || FAILED (rc))
     {
@@ -2274 +2322 @@
         mState = Created;
     }
+    else
+    {
+        if (aAccessError)
+            aAccessError->setNull();
+
+        mState = Accessible;
+    }
+    
+    /* inform waiters if there are any */
+    if (mStateCheckWaiters > 0)
+    {
+        RTSemEventMultiSignal (mStateCheckSem);
+    }
+    else
+    {
+        /* delete the semaphore ourselves */
+        RTSemEventMultiDestroy (mStateCheckSem);
+        mStateCheckSem = NIL_RTSEMEVENTMULTI;
+    }
 
     return rc;
@@ -2302 +2369 @@
             mFilePathFull.raw());
 
-    // first ensure the directory exists
+    /* first ensure the directory exists */
     {
         Utf8Str imageDir = mFilePathFull;
@@ -2319 +2386 @@
     }
 
-    // check whether the given file exists or not
+    /* check whether the given file exists or not */
     RTFILE file;
     int vrc = RTFileOpen (&file, Utf8Str (mFilePathFull),
@@ -2341 +2408 @@
     }
 
-    // check VDI size limits
+    /* check VDI size limits */
     {
         HRESULT rc;
@@ -2359 +2426 @@
     HRESULT rc;
 
-    // create a project object
+    /* create a project object */
     ComObjPtr <Progress> progress;
     progress.createObject();
@@ -2370 +2437 @@
     }
 
-    // mark as busy (being created)
-    // (VDI task thread will unmark it)
+    /* mark as busy (being created)
+     * (VDI task thread will unmark it) */
     setBusy();
 
-    // fill in a VDI task data
+    /* fill in VDI task data */
     VDITask *task = new VDITask (aDynamic ? VDITask::CreateDynamic
                                           : VDITask::CreateStatic,
                                  this, progress);
     task->size = aSize;
-    task->size *= 1024 * 1024; // convert to bytes
-
-    // create the hard disk creation thread, pass operation data
+    task->size *= 1024 * 1024; /* convert to bytes */
+
+    /* create the hard disk creation thread, pass operation data */
     vrc = RTThreadCreate (NULL, vdiTaskThread, (void *) task, 0,
                           RTTHREADTYPE_MAIN_HEAVY_WORKER, 0, "VDITask");
@@ -2393 +2460 @@
     else
     {
-        // get one interface for the caller
+        /* get one interface for the caller */
         progress.queryInterfaceTo (aProgress);
     }
@@ -2400 +2467 @@
 }
 
-// static
+/* static */
 DECLCALLBACK(int) HVirtualDiskImage::vdiTaskThread (RTTHREAD thread, void *pvUser)
 {
@@ -2431 +2498 @@
                                          task->progress);
 
-        // release reader added in HardDisk::CloneToImage()
+        /* release reader added in HardDisk::CloneToImage() */
         task->source->releaseReader();
     }
@@ -2444 +2511 @@
         if (VBOX_SUCCESS (vrc) && task->vdi->id())
         {
-            // we have a non-null UUID, update the created image
+            /* we have a non-null UUID, update the created image */
             vrc = VDISetImageUUIDs (Utf8Str (task->vdi->filePathFull()),
                                     task->vdi->id().raw(), NULL, NULL, NULL);
@@ -2462 +2529 @@
     AutoLock alock (task->vdi);
 
-    // clear busy set in in HardDisk::CloneToImage() or
-    // in HVirtualDiskImage::createImage()
+    /* clear busy set in in HardDisk::CloneToImage() or
+     * in HVirtualDiskImage::createImage() */
     task->vdi->clearBusy();
 
@@ -2469 +2536 @@
     {
         task->vdi->mState = HVirtualDiskImage::Created;
-        // update VDI data fields
+        /* update VDI data fields */
         rc = task->vdi->queryInformation (NULL);
-        // complete the progress object
+        /* complete the progress object */
         task->progress->notifyComplete (rc);
     }
@@ -2480 +2547 @@
 
         task->vdi->mState = HVirtualDiskImage::NotCreated;
-        // complete the progress object
+        /* complete the progress object */
         if (errorMsg)
             task->progress->notifyComplete (
@@ -2554 +2621 @@
         CheckComRCBreakRC (rc);
 
-        // set ready to let protectedUninit() be called on failure
+        /* set ready to let protectedUninit() be called on failure */
         setReady (true);
 
-        // server (required)
+        /* server (required) */
         CFGLDRQueryBSTR (aISCSINode, "server", mServer.asOutParam());
-        // target (required)
+        /* target (required) */
         CFGLDRQueryBSTR (aISCSINode, "target", mTarget.asOutParam());
 
-        // port (optional)
+        /* port (optional) */
         CFGLDRQueryUInt16 (aISCSINode, "port", &mPort);
-        // lun (optional)
+        /* lun (optional) */
         CFGLDRQueryUInt64 (aISCSINode, "lun", &mLun);
-        // userName (optional)
+        /* userName (optional) */
         CFGLDRQueryBSTR (aISCSINode, "userName", mUserName.asOutParam());
-        // password (optional)
+        /* password (optional) */
         CFGLDRQueryBSTR (aISCSINode, "password", mPassword.asOutParam());
 
@@ -2575 +2642 @@
                         mLun));
 
-        // load basic settings and children
+        /* load basic settings and children */
         rc = loadSettings (aHDNode);
         CheckComRCBreakRC (rc);
@@ -2618 +2685 @@
         CheckComRCBreakRC (rc);
 
-        // set ready to let protectedUninit() be called on failure
+        /* set ready to let protectedUninit() be called on failure */
         setReady (true);
 
-        // we have to generate a new UUID
+        /* we have to generate a new UUID */
         mId.create();
         mType = HardDiskType_WritethroughHardDisk;
@@ -2922 +2989 @@
     CHECK_READY();
 
-    // check the basic accessibility
+    /* check the basic accessibility */
     HRESULT rc = HardDisk::getAccessible (aAccessError);
     if (FAILED (rc) || !aAccessError.isNull())
@@ -2944 +3011 @@
     CHECK_READY();
 
-    // server (required)
+    /* server (required) */
     CFGLDRSetBSTR (aStorageNode, "server", mServer);
-    // target (required)
+    /* target (required) */
     CFGLDRSetBSTR (aStorageNode, "target", mTarget);
 
-    // port (optional)
+    /* port (optional) */
     if (mPort != 0)
         CFGLDRSetUInt16 (aStorageNode, "port", mPort);
     else
         CFGLDRDeleteAttribute (aStorageNode, "port");
-    // lun (optional)
+    /* lun (optional) */
     if (mLun != 0)
         CFGLDRSetUInt64 (aStorageNode, "lun", mLun);
     else
         CFGLDRDeleteAttribute (aStorageNode, "lun");
-    // userName (optional)
+    /* userName (optional) */
     if (!mUserName.isNull())
         CFGLDRSetBSTR (aStorageNode, "userName", mUserName);
     else
         CFGLDRDeleteAttribute (aStorageNode, "userName");
-    // password (optional)
+    /* password (optional) */
     if (!mPassword.isNull())
         CFGLDRSetBSTR (aStorageNode, "password", mPassword);
@@ -2970 +3037 @@
         CFGLDRDeleteAttribute (aStorageNode, "password");
 
-    // save basic settings and children
+    /* save basic settings and children */
     return HardDisk::saveSettings (aHDNode);
 }

trunk/src/VBox/Main/ProgressImpl.cpp

@@ -393 +393 @@
         return rc;
 
-    mCompletedSem = NIL_RTSEMEVENT;
+    mCompletedSem = NIL_RTSEMEVENTMULTI;
     mWaitersCount = 0;
 

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

@@ -28 +28 @@
 #include <VBox/cfgldr.h>
 
+#include <iprt/semaphore.h>
+
 #include <list>
 
@@ -61 +63 @@
     HRESULT FinalConstruct();
     void FinalRelease();
-
-    /// @todo (dmik) remove
-    enum InitMode { Init_New, Init_Existing, Init_Registered };
 
 protected:
@@ -168 +167 @@
     void updatePaths (const char *aOldPath, const char *aNewPath);
 
-    bool isBusy() { AutoLock alock (this); return mBusy; }
-    unsigned readers() { AutoLock alock (this); return mReaders; }
+    /* these must be are called from under the lock */
+    bool isBusy() { isLockedOnCurrentThread(); return mBusy; }
+    unsigned readers() { isLockedOnCurrentThread(); return mReaders; }
 
     // for VirtualBoxSupportErrorInfoImpl
@@ -295 +295 @@
         NotCreated,
         Created,
-        Accessible, // must be greater than Created
+        /* the following must be greater than Created */
+        Accessible,
     };
 
     State mState;
+    
+    RTSEMEVENTMULTI mStateCheckSem;
+    ULONG mStateCheckWaiters;
 
     Bstr mDescription;