Index: /trunk/src/VBox/Main/include/GuestCtrlImplPrivate.h =================================================================== --- /trunk/src/VBox/Main/include/GuestCtrlImplPrivate.h (revision 75860) +++ /trunk/src/VBox/Main/include/GuestCtrlImplPrivate.h (revision 75861) @@ -1078,5 +1078,5 @@ int GetGuestError(void) const { return mGuestRc; } int SignalExternal(IEvent *pEvent); - const GuestEventTypes Types(void) { return mEventTypes; } + const GuestEventTypes &Types(void) { return mEventTypes; } size_t TypeCount(void) { return mEventTypes.size(); } @@ -1140,5 +1140,5 @@ * for HGCM (VMMDev) communication. */ Console *mConsole; - /** The next upcoming context ID for this object. */ + /** The next context ID counter component for this object. */ uint32_t mNextContextID; /** Local listener for handling the waiting events Index: /trunk/src/VBox/Main/include/GuestSessionImpl.h =================================================================== --- /trunk/src/VBox/Main/include/GuestSessionImpl.h (revision 75860) +++ /trunk/src/VBox/Main/include/GuestSessionImpl.h (revision 75861) @@ -29,4 +29,6 @@ #include "GuestSessionImplTasks.h" +#include /** @todo r=bird: Needed for ASMBitSet() in GuestSession::Data constructor. Removed when + * that is moved into the class implementation file as it should be. */ #include @@ -36,7 +38,7 @@ * Guest session implementation. */ -class ATL_NO_VTABLE GuestSession : - public GuestSessionWrap, - public GuestBase +class ATL_NO_VTABLE GuestSession + : public GuestSessionWrap + , public GuestBase { public: @@ -245,6 +247,7 @@ struct SessionObject { - /** Creation timestamp (in ms). */ - uint64_t tsCreatedMs; + /** Creation timestamp (in ms). + * @note not used by anyone at the moment. */ + uint64_t msBirth; /** The object type. */ SESSIONOBJECTTYPE enmType; @@ -305,6 +308,5 @@ Guest *i_getParent(void) { return mParent; } uint32_t i_getProtocolVersion(void) { return mData.mProtocolVersion; } - int i_objectRegister(SESSIONOBJECTTYPE enmType, uint32_t *puObjectID); - int i_objectRegisterEx(SESSIONOBJECTTYPE enmType, uint32_t fFlags, uint32_t *puObjectID); + int i_objectRegister(SESSIONOBJECTTYPE enmType, uint32_t *pidObject); int i_objectUnregister(uint32_t uObjectID); int i_pathRename(const Utf8Str &strSource, const Utf8Str &strDest, uint32_t uFlags, int *pGuestRc); @@ -341,4 +343,10 @@ const ComObjPtr mEventSource; + /** @todo r=bird: One of the core points of the DATA sub-structures in Main is + * hinding implementation details and stuff that requires including iprt/asm.h. + * The way it's used here totally defeats that purpose. You need to make it + * a pointer to a anynmous Data struct and define that structure in + * GuestSessionImpl.cpp and allocate it in the Init() function. + */ struct Data { @@ -367,7 +375,4 @@ /** Map of registered session objects (files, directories, ...). */ SessionObjects mObjects; - /** Queue of object IDs which are not used anymore (free list). - * Acts as a "free list" for the mObjects map. */ - SessionObjectsFree mObjectsFree; /** Guest control protocol version to be used. * Guest Additions < VBox 4.3 have version 1, @@ -379,8 +384,14 @@ * returned from the guest side. */ int mRC; + /** Object ID allocation bitmap; clear bits are free, set bits are busy. */ + uint64_t bmObjectIds[VBOX_GUESTCTRL_MAX_OBJECTS / sizeof(uint64_t) / 8]; Data(void) : mpBaseEnvironment(NULL) - { } + { + RT_ZERO(bmObjectIds); + ASMBitSet(&bmObjectIds, VBOX_GUESTCTRL_MAX_OBJECTS - 1); /* Reserved for the session itself? */ + ASMBitSet(&bmObjectIds, 0); /* Let's reserve this too. */ + } Data(const Data &rThat) : mCredentials(rThat.mCredentials) @@ -393,9 +404,10 @@ , mProcesses(rThat.mProcesses) , mObjects(rThat.mObjects) - , mObjectsFree(rThat.mObjectsFree) , mProtocolVersion(rThat.mProtocolVersion) , mTimeout(rThat.mTimeout) , mRC(rThat.mRC) - { } + { + memcpy(&bmObjectIds, &rThat.bmObjectIds, sizeof(bmObjectIds)); + } ~Data(void) { Index: /trunk/src/VBox/Main/src-client/GuestCtrlImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-client/GuestCtrlImpl.cpp (revision 75860) +++ /trunk/src/VBox/Main/src-client/GuestCtrlImpl.cpp (revision 75861) @@ -160,6 +160,6 @@ alock.release(); +#ifdef DEBUG bool fDispatch = true; -#ifdef DEBUG /* * Pre-check: If we got a status message with an error and VERR_TOO_MUCH_DATA @@ -188,6 +188,6 @@ } } + if (fDispatch) #endif - if (fDispatch) { switch (pCtxCb->uFunction) Index: /trunk/src/VBox/Main/src-client/GuestCtrlPrivate.cpp =================================================================== --- /trunk/src/VBox/Main/src-client/GuestCtrlPrivate.cpp (revision 75860) +++ /trunk/src/VBox/Main/src-client/GuestCtrlPrivate.cpp (revision 75861) @@ -37,4 +37,5 @@ #endif /* DEBUG */ #include +#include #include @@ -648,6 +649,6 @@ GuestBase::GuestBase(void) - : mConsole(NULL), - mNextContextID(0) + : mConsole(NULL) + , mNextContextID(RTRandU32() % VBOX_GUESTCTRL_MAX_CONTEXTS) { } @@ -791,9 +792,8 @@ uint32_t uCount = ASMAtomicIncU32(&mNextContextID); - if (uCount == VBOX_GUESTCTRL_MAX_CONTEXTS) + if (uCount >= VBOX_GUESTCTRL_MAX_CONTEXTS) uCount = 0; - uint32_t uNewContextID = - VBOX_GUESTCTRL_CONTEXTID_MAKE(uSessionID, uObjectID, uCount); + uint32_t uNewContextID = VBOX_GUESTCTRL_CONTEXTID_MAKE(uSessionID, uObjectID, uCount); *puContextID = uNewContextID; @@ -812,6 +812,8 @@ * around once at a time. * - * @returns IPRT status code. VERR_ALREADY_EXISTS if an event with the given session - * and object ID already has been registered. + * @returns IPRT status code. + * @retval VERR_ALREADY_EXISTS if an event with the given session and object ID + * already has been registered. r=bird: Incorrect, see explanation in + * registerWaitEventEx(). * * @param uSessionID Session ID to register wait event for. @@ -827,12 +829,20 @@ /** - * Registers (creates) a new wait event based on a given session, object ID - * and a list of event types to wait for. - * - * From those IDs an unique context ID (CID) will be built, which only can be - * around once at a time. - * - * @returns IPRT status code. VERR_ALREADY_EXISTS if an event with the given session - * and object ID already has been registered. + * Creates and registers a new wait event object that waits on a set of events + * related to a given object within the session. + * + * From the session ID and object ID a one-time unique context ID (CID) is built + * for this wait object. Normally the CID is then passed to the guest along + * with a request, and the guest passed the CID back with the reply. The + * handler for the reply then emits a signal on the event type associated with + * the reply, which includes signalling the object returned by this method and + * the waking up the thread waiting on it. + * + * @returns VBox status code. + * @retval VERR_ALREADY_EXISTS if an event with the given session and object ID + * already has been registered. r=bird: No, this isn't when this is + * returned, it is returned when generateContextID() generates a + * duplicate. The duplicate being in the count part (bits 15:0) of the + * session ID. So, VERR_DUPLICATE would be more appropraite. * * @param uSessionID Session ID to register wait event for. @@ -845,11 +855,12 @@ GuestWaitEvent **ppEvent) { + AssertReturn(!lstEvents.empty(), VERR_INVALID_PARAMETER); AssertPtrReturn(ppEvent, VERR_INVALID_POINTER); - uint32_t uContextID; - int rc = generateContextID(uSessionID, uObjectID, &uContextID); - if (RT_FAILURE(rc)) - return rc; - + uint32_t idContext; + int rc = generateContextID(uSessionID, uObjectID, &idContext); + AssertRCReturn(rc, rc); + +#if 1 /** @todo r=bird: Incorrect exception and memory handling, no strategy for dealing with duplicate IDs: */ rc = RTCritSectEnter(&mWaitEventCritSect); if (RT_SUCCESS(rc)) @@ -857,8 +868,8 @@ try { - GuestWaitEvent *pEvent = new GuestWaitEvent(uContextID, lstEvents); + GuestWaitEvent *pEvent = new GuestWaitEvent(idContext, lstEvents); AssertPtr(pEvent); - LogFlowThisFunc(("New event=%p, CID=%RU32\n", pEvent, uContextID)); + LogFlowThisFunc(("New event=%p, CID=%RU32\n", pEvent, idContext)); /* Insert event into matching event group. This is for faster per-group @@ -869,9 +880,9 @@ /* Check if the event group already has an event with the same * context ID in it (collision). */ - GuestWaitEvents eventGroup = mWaitEventGroups[(*itEvents)]; - if (eventGroup.find(uContextID) == eventGroup.end()) + GuestWaitEvents eventGroup = mWaitEventGroups[(*itEvents)]; /** @todo r=bird: Why copy it? */ + if (eventGroup.find(idContext) == eventGroup.end()) { /* No, insert. */ - mWaitEventGroups[(*itEvents)].insert(std::pair(uContextID, pEvent)); + mWaitEventGroups[(*itEvents)].insert(std::pair(idContext, pEvent)); } else @@ -885,7 +896,7 @@ { /* Register event in regular event list. */ - if (mWaitEvents.find(uContextID) == mWaitEvents.end()) + if (mWaitEvents.find(idContext) == mWaitEvents.end()) { - mWaitEvents[uContextID] = pEvent; + mWaitEvents[idContext] = pEvent; } else @@ -905,6 +916,92 @@ rc = rc2; } - return rc; + +#else /** @todo r=bird: Version with proper exception handling, no leaks and limited duplicate CID handling: */ + + GuestWaitEvent *pEvent = new GuestWaitEvent(idContext, lstEvents); + AssertReturn(pEvent, VERR_NO_MEMORY); + LogFlowThisFunc(("New event=%p, CID=%RU32\n", pEvent, idContext)); + + rc = RTCritSectEnter(&mWaitEventCritSect); + if (RT_SUCCESS(rc)) + { + /* + * Check that we don't have any context ID collisions (should be very unlikely). + * + * The ASSUMPTION here is that mWaitEvents has all the same events as + * mWaitEventGroups, so it suffices to check one of the two. + */ + if (mWaitEvents.find(idContext) != mWaitEvents.end()) + { + uint32_t cTries = 0; + do + { + rc = generateContextID(uSessionID, uObjectID, &idContext); + AssertRCBreak(rc); + Log(("Duplicate! Trying a different context ID: %#x\n", idContext)); + if (mWaitEvents.find(idContext) != mWaitEvents.end()) + rc = VERR_ALREADY_EXISTS; + } while (RT_FAILURE_NP(rc) && cTries++ < 10); + } + if (RT_SUCCESS(rc)) + { + /* + * Insert event into matching event group. This is for faster per-group lookup of all events later. + */ + uint32_t cInserts = 0; + for (GuestEventTypes::const_iterator ItType = lstEvents.begin(); ItType != lstEvents.end(); ++ItType) + { + GuestWaitEvents &eventGroup = mWaitEventGroups[*ItType]; + if (eventGroup.find(idContext) == eventGroup.end()) + { + try + { + eventGroup.insert(std::pair(idContext, pEvent)); + cInserts++; + } + catch (std::bad_alloc &) + { + while (ItType != lstEvents.begin()) + { + --ItType; + mWaitEventGroups[*ItType].erase(idContext); + } + rc = VERR_NO_MEMORY; + break; + } + } + else + Assert(cInserts > 0); /* else: lstEvents has duplicate entries. */ + } + if (RT_SUCCESS(rc)) + { + Assert(cInserts > 0); + NOREF(cInserts); + + /* + * Register event in the regular event list. + */ + try + { + mWaitEvents[idContext] = pEvent; + } + catch (std::bad_alloc &) + { + for (GuestEventTypes::const_iterator ItType = lstEvents.begin(); ItType != lstEvents.end(); ++ItType) + mWaitEventGroups[*ItType].erase(idContext); + rc = VERR_NO_MEMORY; + } + } + } + + RTCritSectLeave(&mWaitEventCritSect); + } + if (RT_SUCCESS(rc)) + return rc; + + delete pEvent; + return rc; +#endif } @@ -920,4 +1017,5 @@ if (itGroup != mWaitEventGroups.end()) { +#if 1 /** @todo r=bird: consider the other variant. */ GuestWaitEvents::iterator itEvents = itGroup->second.begin(); while (itEvents != itGroup->second.end()) @@ -930,12 +1028,20 @@ VBOX_GUESTCTRL_CONTEXTID_GET_COUNT(itEvents->first))); #endif - ComPtr pThisEvent = aEvent; + ComPtr pThisEvent = aEvent; /** @todo r=bird: This means addref/release for each iteration. Isn't that silly? */ Assert(!pThisEvent.isNull()); int rc2 = itEvents->second->SignalExternal(aEvent); if (RT_SUCCESS(rc)) - rc = rc2; + rc = rc2; /** @todo r=bird: This doesn't make much sense since it will only fail if not + * properly initialized or major memory corruption. And if it's broken, why + * don't you just remove it instead of leaving it in the group??? It would + * make life so much easier here as you could just change the while condition + * to while ((itEvents = itGroup->second.begin() != itGroup->second.end()) + * and skip all this two step removal below. I'll put this in a #if 0 and show what I mean... */ if (RT_SUCCESS(rc2)) { + /** @todo r=bird: I don't follow the logic here. Why don't you just remove + * it from all groups, including this one? You just have move the */ + /* Remove the event from all other event groups (except the * original one!) because it was signalled. */ @@ -975,4 +1081,38 @@ #endif } +#else + /* Signal all events in the group, leaving the group empty afterwards. */ + GuestWaitEvents::iterator ItWaitEvt; + while ((ItWaitEvt = itGroup->second.begin()) != itGroup->second.end()) + { + LogFlowThisFunc(("Signalling event=%p, type=%ld (CID %#x: Session=%RU32, Object=%RU32, Count=%RU32) ...\n", + ItWaitEvt->second, aType, ItWaitEvt->first, VBOX_GUESTCTRL_CONTEXTID_GET_SESSION(ItWaitEvt->first), + VBOX_GUESTCTRL_CONTEXTID_GET_OBJECT(ItWaitEvt->first), VBOX_GUESTCTRL_CONTEXTID_GET_COUNT(ItWaitEvt->first))); + + int rc2 = ItWaitEvt->second->SignalExternal(aEvent); + AssertRC(rc2); + + /* Take down the wait event object details before we erase it from this list and invalid ItGrpEvt. */ + const GuestEventTypes &EvtTypes = ItWaitEvt->second->Types(); + uint32_t idContext = ItWaitEvt->first; + itGroup->second.erase(ItWaitEvt); + + for (GuestEventTypes::const_iterator ItType = EvtTypes.begin(); ItType != EvtTypes.end(); ++ItType) + { + GuestEventGroup::iterator EvtTypeGrp = mWaitEventGroups.find(*ItType); + if (EvtTypeGrp != mWaitEventGroups.end()) + { + ItWaitEvt = EvtTypeGrp->second.find(idContext); + if (ItWaitEvt != EvtTypeGrp->second.end()) + { + LogFlowThisFunc(("Removing event %p (CID %#x) from type %d group\n", ItWaitEvt->second, idContext, *ItType)); + EvtTypeGrp->second.erase(ItWaitEvt); + LogFlowThisFunc(("%zu events left for type %d\n", EvtTypeGrp->second.size(), *ItType)); + Assert(EvtTypeGrp->second.find(idContext) == EvtTypeGrp->second.end()); /* no duplicates */ + } + } + } + } +#endif } @@ -1035,9 +1175,9 @@ * * @returns IPRT status code. - * @param pEvent Event to unregister (delete). - */ -int GuestBase::unregisterWaitEvent(GuestWaitEvent *pEvent) -{ - if (!pEvent) /* Nothing to unregister. */ + * @param pWaitEvt Wait event to unregister (delete). + */ +int GuestBase::unregisterWaitEvent(GuestWaitEvent *pWaitEvt) +{ + if (!pWaitEvt) /* Nothing to unregister. */ return VINF_SUCCESS; @@ -1045,10 +1185,29 @@ if (RT_SUCCESS(rc)) { - LogFlowThisFunc(("pEvent=%p\n", pEvent)); - + LogFlowThisFunc(("pWaitEvt=%p\n", pWaitEvt)); + +/** @todo r=bird: One way of optimizing this would be to use the pointer + * instead of the context ID as index into the groups, i.e. revert the value + * pair for the GuestWaitEvents type. + * + * An even more efficent way, would be to not use sexy std::xxx containers for + * the types, but iprt/list.h, as that would just be a RTListNodeRemove call for + * each type w/o needing to iterate much at all. I.e. add a struct { + * RTLISTNODE, GuestWaitEvent *pSelf} array to GuestWaitEvent, and change + * GuestEventGroup to std::map + * (RTListAnchorClass == RTLISTANCHOR wrapper with a constructor)). + * + * P.S. the try/catch is now longer needed after I changed pWaitEvt->Types() to + * return a const reference rather than a copy of the type list (and it think it + * is safe to assume iterators are not hitting the heap). Copy vs reference is + * an easy mistake to make in C++. + * + * P.P.S. The mWaitEventGroups optimization is probably just a lot of extra work + * with little payoff. + */ try { /* Remove the event from all event type groups. */ - const GuestEventTypes lstTypes = pEvent->Types(); + const GuestEventTypes &lstTypes = pWaitEvt->Types(); for (GuestEventTypes::const_iterator itType = lstTypes.begin(); itType != lstTypes.end(); ++itType) @@ -1058,24 +1217,23 @@ while (itCurEvent != mWaitEventGroups[(*itType)].end()) { - if (itCurEvent->second == pEvent) + if (itCurEvent->second == pWaitEvt) { mWaitEventGroups[(*itType)].erase(itCurEvent); break; } - else - ++itCurEvent; + ++itCurEvent; } } /* Remove the event from the general event list as well. */ - GuestWaitEvents::iterator itEvent = mWaitEvents.find(pEvent->ContextID()); + GuestWaitEvents::iterator itEvent = mWaitEvents.find(pWaitEvt->ContextID()); Assert(itEvent != mWaitEvents.end()); - Assert(itEvent->second == pEvent); + Assert(itEvent->second == pWaitEvt); mWaitEvents.erase(itEvent); - delete pEvent; - pEvent = NULL; + delete pWaitEvt; + pWaitEvt = NULL; } catch (const std::exception &ex) @@ -1094,9 +1252,9 @@ /** - * Waits for a formerly registered guest event. + * Waits for an already registered guest wait event. * * @return IPRT status code. - * @param pEvent Pointer to event to wait for. - * @param uTimeoutMS Timeout (in ms) for waiting. + * @param pWaitEvt Pointer to event to wait for. + * @param msTimeout Timeout (in ms) for waiting. * @param pType Event type of following IEvent. * Optional. @@ -1104,6 +1262,5 @@ * for this event. Optional. */ -int GuestBase::waitForEvent(GuestWaitEvent *pEvent, uint32_t uTimeoutMS, - VBoxEventType_T *pType, IEvent **ppEvent) +int GuestBase::waitForEvent(GuestWaitEvent *pEvent, uint32_t msTimeout, VBoxEventType_T *pType, IEvent **ppEvent) { AssertPtrReturn(pEvent, VERR_INVALID_POINTER); @@ -1111,9 +1268,9 @@ /* ppEvent is optional. */ - int vrc = pEvent->Wait(uTimeoutMS); + int vrc = pEvent->Wait(msTimeout); if (RT_SUCCESS(vrc)) { const ComPtr pThisEvent = pEvent->Event(); - if (!pThisEvent.isNull()) /* Having a VBoxEventType_ event is optional. */ + if (pThisEvent.isNotNull()) /* Having a VBoxEventType_ event is optional. */ /** @todo r=bird: misplaced comment? */ { if (pType) @@ -1269,5 +1426,5 @@ } -int GuestWaitEventBase::Wait(RTMSINTERVAL uTimeoutMS) +int GuestWaitEventBase::Wait(RTMSINTERVAL msTimeout) { int rc = VINF_SUCCESS; @@ -1280,8 +1437,5 @@ AssertReturn(mEventSem != NIL_RTSEMEVENT, VERR_CANCELLED); - RTMSINTERVAL msInterval = uTimeoutMS; - if (!uTimeoutMS) - msInterval = RT_INDEFINITE_WAIT; - rc = RTSemEventWait(mEventSem, msInterval); + rc = RTSemEventWait(mEventSem, msTimeout ? msTimeout : RT_INDEFINITE_WAIT); if (ASMAtomicReadBool(&mfAborted)) rc = VERR_CANCELLED; @@ -1301,5 +1455,5 @@ { int rc2 = Init(uCID); - AssertRC(rc2); /** @todo Throw exception here. */ + AssertRC(rc2); /** @todo Throw exception here. */ /** @todo r=bird: add+use Init() instead. Will cause weird VERR_CANCELLED errors in GuestBase::signalWaitEvent. */ mEventTypes = lstEvents; @@ -1309,5 +1463,5 @@ { int rc2 = Init(uCID); - AssertRC(rc2); /** @todo Throw exception here. */ + AssertRC(rc2); /** @todo Throw exception here. */ /** @todo r=bird: add+use Init() instead. Will cause weird VERR_CANCELLED errors in GuestBase::signalWaitEvent. */ } @@ -1345,4 +1499,6 @@ int GuestWaitEvent::SignalExternal(IEvent *pEvent) { + /** @todo r=bird: VERR_CANCELLED is misleading. mEventSem can only be NIL if + * not successfully initialized! */ AssertReturn(mEventSem != NIL_RTSEMEVENT, VERR_CANCELLED); Index: /trunk/src/VBox/Main/src-client/GuestFileImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-client/GuestFileImpl.cpp (revision 75860) +++ /trunk/src/VBox/Main/src-client/GuestFileImpl.cpp (revision 75861) @@ -295,4 +295,8 @@ AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS); +/** @todo r=bird: Why do you have both a offset and a tell() function? + * After a ReadAt or WriteAt with a non-current offset, the tell() result will + * differ from this value, because mOffCurrent is only ever incremented with + * data read or written. */ *aOffset = mData.mOffCurrent; Index: /trunk/src/VBox/Main/src-client/GuestSessionImpl.cpp =================================================================== --- /trunk/src/VBox/Main/src-client/GuestSessionImpl.cpp (revision 75860) +++ /trunk/src/VBox/Main/src-client/GuestSessionImpl.cpp (revision 75861) @@ -46,4 +46,5 @@ #include /* For CopyTo/From. */ #include +#include #include @@ -341,5 +342,4 @@ mData.mObjects.clear(); - mData.mObjectsFree.clear(); mData.mEnvironmentChanges.reset(); @@ -1017,13 +1017,13 @@ AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - const uint32_t uObjectID = pDirectory->getObjectID(); - - LogFlowFunc(("Removing directory (objectID=%RU32) ...\n", uObjectID)); - - int rc = i_objectUnregister(uObjectID); + const uint32_t idObject = pDirectory->getObjectID(); + + LogFlowFunc(("Removing directory (objectID=%RU32) ...\n", idObject)); + + int rc = i_objectUnregister(idObject); if (RT_FAILURE(rc)) return rc; - SessionDirectories::iterator itDirs = mData.mDirectories.find(uObjectID); + SessionDirectories::iterator itDirs = mData.mDirectories.find(idObject); AssertReturn(itDirs != mData.mDirectories.end(), VERR_NOT_FOUND); @@ -1032,5 +1032,5 @@ LogFlowFunc(("Removing directory ID=%RU32 (session %RU32, now total %zu directories)\n", - uObjectID, mData.mSession.mID, mData.mDirectories.size())); + idObject, mData.mSession.mID, mData.mDirectories.size())); rc = pDirConsumed->i_onRemove(); @@ -1162,6 +1162,6 @@ /* Register a new object ID. */ - uint32_t uObjectID; - int rc = i_objectRegister(SESSIONOBJECTTYPE_DIRECTORY, &uObjectID); + uint32_t idObject; + int rc = i_objectRegister(SESSIONOBJECTTYPE_DIRECTORY, &idObject); if (RT_FAILURE(rc)) return rc; @@ -1175,5 +1175,5 @@ AssertPtr(pConsole); - int vrc = pDirectory->init(pConsole, this /* Parent */, uObjectID, openInfo); + int vrc = pDirectory->init(pConsole, this /* Parent */, idObject, openInfo); if (RT_FAILURE(vrc)) return vrc; @@ -1189,5 +1189,5 @@ { /* Add the created directory to our map. */ - mData.mDirectories[uObjectID] = pDirectory; + mData.mDirectories[idObject] = pDirectory; LogFlowFunc(("Added new guest directory \"%s\" (Session: %RU32) (now total %zu directories)\n", @@ -1416,13 +1416,13 @@ AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - const uint32_t uObjectID = pFile->getObjectID(); - - LogFlowFunc(("Removing file (objectID=%RU32) ...\n", uObjectID)); - - int rc = i_objectUnregister(uObjectID); + const uint32_t idObject = pFile->getObjectID(); + + LogFlowFunc(("Removing file (objectID=%RU32) ...\n", idObject)); + + int rc = i_objectUnregister(idObject); if (RT_FAILURE(rc)) return rc; - SessionFiles::iterator itFiles = mData.mFiles.find(uObjectID); + SessionFiles::iterator itFiles = mData.mFiles.find(idObject); AssertReturn(itFiles != mData.mFiles.end(), VERR_NOT_FOUND); @@ -1520,6 +1520,6 @@ /* Register a new object ID. */ - uint32_t uObjectID; - int rc = i_objectRegister(SESSIONOBJECTTYPE_FILE, &uObjectID); + uint32_t idObject; + int rc = i_objectRegister(SESSIONOBJECTTYPE_FILE, &idObject); if (RT_FAILURE(rc)) return rc; @@ -1533,5 +1533,5 @@ AssertPtr(pConsole); - rc = pFile->init(pConsole, this /* GuestSession */, uObjectID, openInfo); + rc = pFile->init(pConsole, this /* GuestSession */, idObject, openInfo); if (RT_FAILURE(rc)) return rc; @@ -1547,5 +1547,5 @@ { /* Add the created file to our vector. */ - mData.mFiles[uObjectID] = pFile; + mData.mFiles[idObject] = pFile; LogFlowFunc(("Added new guest file \"%s\" (Session: %RU32) (now total %zu files)\n", @@ -2035,95 +2035,77 @@ /** - * Registers an object to a session. + * Registers an object with the session, i.e. allocates an object ID. * - * @return VBox status code. - * @param enmType Session object type to register. - * @param puObjectID Returns registered object ID on success. Optional. + * @return VBox status code. + * @retval VERR_GSTCTL_MAX_OBJECTS_REACHED if the maximum of concurrent objects + * is reached. + * @param enmType Session object type to register. + * @param pidObject Where to return the object ID on success. */ -int GuestSession::i_objectRegister(SESSIONOBJECTTYPE enmType, uint32_t *puObjectID) -{ - return i_objectRegisterEx(enmType, 0 /* fFlags */, puObjectID); +int GuestSession::i_objectRegister(SESSIONOBJECTTYPE enmType, uint32_t *pidObject) +{ + /* + * Pick a random bit as starting point. If it's in use, search forward + * for a free one, wrapping around. We've reserved both the zero'th and + * max-1 IDs (see Data constructor). + */ + uint32_t idObject = RTRandU32Ex(1, VBOX_GUESTCTRL_MAX_OBJECTS - 2); + AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); + if (!ASMBitTestAndSet(&mData.bmObjectIds[0], idObject)) + { /* likely */ } + else if (mData.mObjects.size() < VBOX_GUESTCTRL_MAX_OBJECTS - 2 /* First and last are not used */) + { + /* Forward search. */ + int iHit = ASMBitNextClear(&mData.bmObjectIds[0], VBOX_GUESTCTRL_MAX_OBJECTS, idObject); + if (iHit < 0) + iHit = ASMBitFirstClear(&mData.bmObjectIds[0], VBOX_GUESTCTRL_MAX_OBJECTS); + AssertLogRelMsgReturn(iHit >= 0, ("object count: %#zu\n", mData.mObjects.size()), VERR_GSTCTL_MAX_OBJECTS_REACHED); + idObject = iHit; + AssertLogRelMsgReturn(!ASMBitTestAndSet(&mData.bmObjectIds[0], idObject), ("idObject=%#x\n", idObject), VERR_INTERNAL_ERROR_2); + } + else + { + LogFunc(("enmType=%RU32 -> VERR_GSTCTL_MAX_OBJECTS_REACHED!! (%zu objects)\n", enmType, mData.mObjects.size())); + return VERR_GSTCTL_MAX_OBJECTS_REACHED; + } + + Log2Func(("enmType=%RU32 -> idObject=%RU32 (%zu objects)\n", enmType, idObject, mData.mObjects.size())); + + try + { + mData.mObjects[idObject].enmType = enmType; + mData.mObjects[idObject].msBirth = RTTimeMilliTS(); + } + catch (std::bad_alloc &) + { + ASMBitClear(&mData.bmObjectIds[0], idObject); + return VERR_NO_MEMORY; + } + + alock.release(); + + *pidObject = idObject; + return VINF_SUCCESS; } /** - * Registers an object to a session, extended version. + * Unregisters an object from a session. * - * @return VBox status code. VERR_GSTCTL_MAX_OBJECTS_REACHED if the maximum of concurrent objects is reached. - * @param enmType Session object type to register. - * @param fFlags Registration flags. Not used yet and must be 0. - * @param puObjectID Returns registered object ID on success. Optional. + * @retval VINF_SUCCESS on success. + * @retval VERR_NOT_FOUND if the object ID was not found. + * @param idObject Object ID to unregister. */ -int GuestSession::i_objectRegisterEx(SESSIONOBJECTTYPE enmType, uint32_t fFlags, uint32_t *puObjectID) -{ - RT_NOREF(fFlags); - AssertReturn(fFlags == 0, VERR_INVALID_PARAMETER); - +int GuestSession::i_objectUnregister(uint32_t idObject) +{ AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); int rc = VINF_SUCCESS; - - uint32_t uObjectID = 0; /* Shut up MSVC. */ - - if (mData.mObjects.size() < VBOX_GUESTCTRL_MAX_OBJECTS - 1 /* Minus 1 for the session itself */) - { - SessionObjects::reverse_iterator itRend = mData.mObjects.rbegin(); - if (itRend != mData.mObjects.rend()) - uObjectID = itRend->first + 1; /* Last key plus 1. */ - else - uObjectID = 0; - } - else - { - /* Utilize our "free list" to get the next free object ID in the map. */ - if (mData.mObjectsFree.size()) - { - /* Always re-use the oldest object ID to avoid trouble. */ - uObjectID = mData.mObjectsFree.front(); - mData.mObjectsFree.pop_front(); - } - else - rc = VERR_GSTCTL_MAX_OBJECTS_REACHED; - } - - Log2Func(("enmType=%RU32, fFlags=%RU32 -> uObjectID=%RU32 (%zu objects, %zu on free list), rc=%Rrc\n", - enmType, fFlags, uObjectID, mData.mObjects.size(), mData.mObjectsFree.size(), rc)); - - if (RT_SUCCESS(rc)) - { - mData.mObjects[uObjectID].enmType = enmType; - mData.mObjects[uObjectID].tsCreatedMs = RTTimeMilliTS(); - - if (puObjectID) - *puObjectID = uObjectID; - } + AssertMsgStmt(ASMBitTestAndClear(&mData.bmObjectIds, idObject), ("idObject=%#x\n", idObject), rc = VERR_NOT_FOUND); + + SessionObjects::const_iterator ItObj = mData.mObjects.find(idObject); + AssertMsgReturn(ItObj != mData.mObjects.end(), ("idObject=%#x\n", idObject), VERR_NOT_FOUND); + mData.mObjects.erase(ItObj); return rc; -} - -/** - * Unregisters a formerly registered object from a session. - * - * @return VBox status code. VERR_NOT_FOUND if object to unregister was not found. - * @param uObjectID Object ID to unregister. - */ -int GuestSession::i_objectUnregister(uint32_t uObjectID) -{ - AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - - SessionObjects::const_iterator itObj = mData.mObjects.find(uObjectID); - if (itObj != mData.mObjects.end()) - { - Log2Func(("uObjectID=%RU32 (now %zu objects in free list)\n", uObjectID, mData.mObjectsFree.size())); - - /* Note: Do not remove object from object list (mData.mObjects) here, as we continue operating - * on the free list (mData.mObjectsFree) if we reached the object list's maximum. */ - - mData.mObjectsFree.push_back(uObjectID); - Assert(mData.mObjectsFree.size() <= VBOX_GUESTCTRL_MAX_OBJECTS); - return VINF_SUCCESS; - } - - AssertFailed(); - return VERR_NOT_FOUND; } @@ -2283,13 +2265,13 @@ AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - const uint32_t uObjectID = pProcess->getObjectID(); - - LogFlowFunc(("Removing process (objectID=%RU32) ...\n", uObjectID)); - - int rc = i_objectUnregister(uObjectID); + const uint32_t idObject = pProcess->getObjectID(); + + LogFlowFunc(("Removing process (objectID=%RU32) ...\n", idObject)); + + int rc = i_objectUnregister(idObject); if (RT_FAILURE(rc)) return rc; - SessionProcesses::iterator itProcs = mData.mProcesses.find(uObjectID); + SessionProcesses::iterator itProcs = mData.mProcesses.find(idObject); AssertReturn(itProcs != mData.mProcesses.end(), VERR_NOT_FOUND); @@ -2302,5 +2284,5 @@ LogFlowFunc(("Removing process ID=%RU32 (session %RU32, guest PID %RU32, now total %zu processes)\n", - uObjectID, mData.mSession.mID, uPID, mData.mProcesses.size())); + idObject, mData.mSession.mID, uPID, mData.mProcesses.size())); rc = pProcess->i_onRemove(); @@ -2380,6 +2362,6 @@ /* Register a new object ID. */ - uint32_t uObjectID; - int rc = i_objectRegister(SESSIONOBJECTTYPE_PROCESS, &uObjectID); + uint32_t idObject; + int rc = i_objectRegister(SESSIONOBJECTTYPE_PROCESS, &idObject); if (RT_FAILURE(rc)) return rc; @@ -2390,5 +2372,5 @@ return VERR_COM_UNEXPECTED; - rc = pProcess->init(mParent->i_getConsole() /* Console */, this /* Session */, uObjectID, + rc = pProcess->init(mParent->i_getConsole() /* Console */, this /* Session */, idObject, procInfo, mData.mpBaseEnvironment); if (RT_FAILURE(rc)) @@ -2398,8 +2380,8 @@ try { - mData.mProcesses[uObjectID] = pProcess; + mData.mProcesses[idObject] = pProcess; LogFlowFunc(("Added new process (Session: %RU32) with process ID=%RU32 (now total %zu processes)\n", - mData.mSession.mID, uObjectID, mData.mProcesses.size())); + mData.mSession.mID, idObject, mData.mProcesses.size())); alock.release(); /* Release lock before firing off event. */