Index: /trunk/src/VBox/Main/HardDiskImpl.cpp =================================================================== --- /trunk/src/VBox/Main/HardDiskImpl.cpp (revision 350) +++ /trunk/src/VBox/Main/HardDiskImpl.cpp (revision 351) @@ -67,8 +67,8 @@ ComObjPtr progress; - // for CreateDynamic, CreateStatic + /* for CreateDynamic, CreateStatic */ uint64_t size; - // for CloneToImage + /* for CloneToImage */ ComObjPtr source; }; @@ -84,5 +84,5 @@ Progress *progress = static_cast (pvUser); - // update the progress object + /* update the progress object */ if (progress) progress->notifyProgress (uPercent); @@ -172,5 +172,5 @@ Assert (isReady()); - // uninit all children + /* uninit all children */ uninitDependentChildren(); @@ -332,5 +332,5 @@ HRESULT rc = S_OK; - // check the accessibility state of all ancestors + /* check the accessibility state of all ancestors */ ComObjPtr parent = (HardDisk *) mParent; while (parent) @@ -413,5 +413,5 @@ HRESULT rc = S_OK; - // create a project object + /* create a project object */ ComObjPtr progress; progress.createObject(); @@ -421,5 +421,5 @@ CheckComRCReturnRC (rc); - // create an imageless resulting object + /* create an imageless resulting object */ ComObjPtr image; image.createObject(); @@ -427,5 +427,5 @@ 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,9 +441,9 @@ } - // 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,17 +462,17 @@ } - // 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,5 +487,5 @@ } - // return interfaces to the caller + /* return interfaces to the caller */ image.queryInterfaceTo (aImage); progress.queryInterfaceTo (aProgress); @@ -706,5 +706,5 @@ AssertReturn (!mMachineId.isEmpty(), false); - // check all children + /* check all children */ AutoLock chLock (childrenLock()); for (HardDiskList::const_iterator it = children().begin(); @@ -746,5 +746,5 @@ 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,5 +906,5 @@ 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,5 +914,5 @@ } - // first ensure the directory exists + /* first ensure the directory exists */ { Utf8Str dir = aFolder; @@ -932,5 +932,5 @@ alock.leave(); - // call storage type specific diff creation method + /* call storage type specific diff creation method */ HRESULT rc = createDiffImage (id, filePathTo, aProgress); @@ -945,5 +945,5 @@ CheckComRCReturnRC (rc); - // associate the created hard disk with the given machine + /* associate the created hard disk with the given machine */ vdi->setMachineId (aMachineId); @@ -1011,5 +1011,5 @@ AssertReturn (aHDNode, E_FAIL); - Guid uuid; // uuid (required) + Guid uuid; /* uuid (required) */ CFGLDRQueryUUID (aHDNode, "uuid", uuid.ptr()); mId = uuid; @@ -1017,5 +1017,5 @@ if (!isDifferencing()) { - Bstr type; // type (required for nodes only) + Bstr type; /* type (required for nodes only) */ CFGLDRQueryBSTR (aHDNode, "type", type.asOutParam()); if (type == L"normal") @@ -1036,5 +1036,5 @@ return rc; - // load all children + /* load all children */ unsigned count = 0; CFGLDRCountChildren (aHDNode, "DiffHardDisk", &count); @@ -1083,10 +1083,10 @@ 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,5 +1107,5 @@ HRESULT rc = S_OK; - // save all children + /* save all children */ AutoLock chLock (childrenLock()); for (HardDiskList::const_iterator it = children().begin(); @@ -1152,4 +1152,7 @@ mState = NotCreated; + + mStateCheckSem = NIL_RTSEMEVENTMULTI; + mStateCheckWaiters = 0; mSize = 0; @@ -1347,5 +1350,5 @@ 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,5 +1524,29 @@ 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,8 +1556,8 @@ { 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,5 +1730,5 @@ 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,5 +1784,5 @@ 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,5 +1792,5 @@ } - // first ensure the directory exists + /* first ensure the directory exists */ { Utf8Str dir = aFolder; @@ -1791,5 +1818,5 @@ 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,5 +1842,5 @@ return rc; - // associate the created hard disk with the given machine + /* associate the created hard disk with the given machine */ vdi->setMachineId (aMachineId); @@ -1900,10 +1927,10 @@ 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,5 +1960,5 @@ } - // detach all our children to avoid their uninit in #uninit() + /* detach all our children to avoid their uninit in #uninit() */ removeDependentChildren(); @@ -1977,5 +2004,5 @@ CHECK_READY(); - // this must be a diff image + /* this must be a diff image */ AssertReturn (isDifferencing(), E_FAIL); @@ -1991,5 +2018,5 @@ 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,10 +2050,10 @@ } - // 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,8 +2078,8 @@ } - // 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,5 +2121,5 @@ 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,5 +2147,5 @@ 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,12 +2168,20 @@ /** - * 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,6 +2189,17 @@ 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,4 +2211,5 @@ vrc = VDICheckImage (filePath, NULL, NULL, NULL, id.ptr(), parentId.ptr(), NULL, 0); + if (VBOX_FAILURE (vrc)) { @@ -2252,12 +2299,13 @@ 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,4 +2322,23 @@ 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,5 +2369,5 @@ mFilePathFull.raw()); - // first ensure the directory exists + /* first ensure the directory exists */ { Utf8Str imageDir = mFilePathFull; @@ -2319,5 +2386,5 @@ } - // 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,5 +2408,5 @@ } - // check VDI size limits + /* check VDI size limits */ { HRESULT rc; @@ -2359,5 +2426,5 @@ HRESULT rc; - // create a project object + /* create a project object */ ComObjPtr progress; progress.createObject(); @@ -2370,16 +2437,16 @@ } - // 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,5 +2460,5 @@ else { - // get one interface for the caller + /* get one interface for the caller */ progress.queryInterfaceTo (aProgress); } @@ -2400,5 +2467,5 @@ } -// static +/* static */ DECLCALLBACK(int) HVirtualDiskImage::vdiTaskThread (RTTHREAD thread, void *pvUser) { @@ -2431,5 +2498,5 @@ task->progress); - // release reader added in HardDisk::CloneToImage() + /* release reader added in HardDisk::CloneToImage() */ task->source->releaseReader(); } @@ -2444,5 +2511,5 @@ 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,6 +2529,6 @@ 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,7 +2536,7 @@ { 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,5 +2547,5 @@ task->vdi->mState = HVirtualDiskImage::NotCreated; - // complete the progress object + /* complete the progress object */ if (errorMsg) task->progress->notifyComplete ( @@ -2554,19 +2621,19 @@ 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,5 +2642,5 @@ mLun)); - // load basic settings and children + /* load basic settings and children */ rc = loadSettings (aHDNode); CheckComRCBreakRC (rc); @@ -2618,8 +2685,8 @@ 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,5 +2989,5 @@ CHECK_READY(); - // check the basic accessibility + /* check the basic accessibility */ HRESULT rc = HardDisk::getAccessible (aAccessError); if (FAILED (rc) || !aAccessError.isNull()) @@ -2944,25 +3011,25 @@ 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,5 +3037,5 @@ CFGLDRDeleteAttribute (aStorageNode, "password"); - // save basic settings and children + /* save basic settings and children */ return HardDisk::saveSettings (aHDNode); } Index: /trunk/src/VBox/Main/ProgressImpl.cpp =================================================================== --- /trunk/src/VBox/Main/ProgressImpl.cpp (revision 350) +++ /trunk/src/VBox/Main/ProgressImpl.cpp (revision 351) @@ -393,5 +393,5 @@ return rc; - mCompletedSem = NIL_RTSEMEVENT; + mCompletedSem = NIL_RTSEMEVENTMULTI; mWaitersCount = 0; Index: /trunk/src/VBox/Main/include/HardDiskImpl.h =================================================================== --- /trunk/src/VBox/Main/include/HardDiskImpl.h (revision 350) +++ /trunk/src/VBox/Main/include/HardDiskImpl.h (revision 351) @@ -28,4 +28,6 @@ #include +#include + #include @@ -61,7 +63,4 @@ HRESULT FinalConstruct(); void FinalRelease(); - - /// @todo (dmik) remove - enum InitMode { Init_New, Init_Existing, Init_Registered }; protected: @@ -168,6 +167,7 @@ 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,8 +295,12 @@ NotCreated, Created, - Accessible, // must be greater than Created + /* the following must be greater than Created */ + Accessible, }; State mState; + + RTSEMEVENTMULTI mStateCheckSem; + ULONG mStateCheckWaiters; Bstr mDescription;