Changeset 75861 in vbox

Timestamp:

Dec 2, 2018 12:26:36 AM (6 years ago)

Author:

vboxsync

Message:

Main/Guest: Added a few codereview comments, mainly in the guest base class area. Replaced the object ID management in GuestSession with a classic allocation bitmap approach as that's much less memory intensive than 16 byte structs in a insert-only std::map and an additional map tracking free 32-bit IDs after we run out. bugref:9313

Location:

trunk/src/VBox/Main

Files:

• include/GuestCtrlImplPrivate.h (modified) (2 diffs)
• include/GuestSessionImpl.h (modified) (8 diffs)
• src-client/GuestCtrlImpl.cpp (modified) (2 diffs)
• src-client/GuestCtrlPrivate.cpp (modified) (24 diffs)
• src-client/GuestFileImpl.cpp (modified) (1 diff)
• src-client/GuestSessionImpl.cpp (modified) (17 diffs)

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

@@ -1078 +1078 @@
     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 +1140 @@
      *  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

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

@@ -29 +29 @@
 #include "GuestSessionImplTasks.h"
 
+#include <iprt/asm.h> /** @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 <deque>
 
@@ -36 +38 @@
  * Guest session implementation.
  */
-class ATL_NO_VTABLE GuestSession :
-    public GuestSessionWrap,
-    public GuestBase
+class ATL_NO_VTABLE GuestSession
+    : public GuestSessionWrap
+    , public GuestBase
 {
 public:
@@ -245 +247 @@
     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 +308 @@
     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 +343 @@
     const ComObjPtr<EventSource>    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 +375 @@
         /** 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 +384 @@
          *  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 +404 @@
             , 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)
         {

trunk/src/VBox/Main/src-client/GuestCtrlImpl.cpp

@@ -160 +160 @@
         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 +188 @@
             }
         }
+        if (fDispatch)
 #endif
-        if (fDispatch)
         {
             switch (pCtxCb->uFunction)

trunk/src/VBox/Main/src-client/GuestCtrlPrivate.cpp

@@ -37 +37 @@
 #endif /* DEBUG */
 #include <iprt/fs.h>
+#include <iprt/rand.h>
 #include <iprt/time.h>
 
@@ -648 +649 @@
 
 GuestBase::GuestBase(void)
-    : mConsole(NULL),
-      mNextContextID(0)
+    : mConsole(NULL)
+    , mNextContextID(RTRandU32() % VBOX_GUESTCTRL_MAX_CONTEXTS)
 {
 }
@@ -791 +792 @@
 
     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 +812 @@
  * 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 +829 @@
 
 /**
- * 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 +855 @@
                                    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 +868 @@
         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 +880 @@
                 /* 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<uint32_t, GuestWaitEvent *>(uContextID, pEvent));
+                    mWaitEventGroups[(*itEvents)].insert(std::pair<uint32_t, GuestWaitEvent *>(idContext, pEvent));
                 }
                 else
@@ -885 +896 @@
             {
                 /* 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 +916 @@
             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<uint32_t, GuestWaitEvent *>(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 +1017 @@
         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 +1028 @@
                                  VBOX_GUESTCTRL_CONTEXTID_GET_COUNT(itEvents->first)));
 #endif
-                ComPtr<IEvent> pThisEvent = aEvent;
+                ComPtr<IEvent> 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 +1081 @@
 #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 +1175 @@
  *
  * @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 +1185 @@
     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<VBoxEventType_T, RTListAnchorClass>
+ * (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 +1217 @@
                 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 +1252 @@
 
 /**
- * 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 +1262 @@
  *                                  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 +1268 @@
     /* ppEvent is optional. */
 
-    int vrc = pEvent->Wait(uTimeoutMS);
+    int vrc = pEvent->Wait(msTimeout);
     if (RT_SUCCESS(vrc))
     {
         const ComPtr<IEvent> 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 +1426 @@
 }
 
-int GuestWaitEventBase::Wait(RTMSINTERVAL uTimeoutMS)
+int GuestWaitEventBase::Wait(RTMSINTERVAL msTimeout)
 {
     int rc = VINF_SUCCESS;
@@ -1280 +1437 @@
         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 +1455 @@
 {
     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 +1463 @@
 {
     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 +1499 @@
 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);
 

trunk/src/VBox/Main/src-client/GuestFileImpl.cpp

@@ -295 +295 @@
     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;
 

trunk/src/VBox/Main/src-client/GuestSessionImpl.cpp

@@ -46 +46 @@
 #include <iprt/file.h> /* For CopyTo/From. */
 #include <iprt/path.h>
+#include <iprt/rand.h>
 
 #include <VBox/com/array.h>
@@ -341 +342 @@
 
     mData.mObjects.clear();
-    mData.mObjectsFree.clear();
 
     mData.mEnvironmentChanges.reset();
@@ -1017 +1017 @@
     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 +1032 @@
 
     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 +1162 @@
 
     /* 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 +1175 @@
     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 +1189 @@
     {
         /* 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 +1416 @@
     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 +1520 @@
 
     /* 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 +1533 @@
     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 +1547 @@
     {
         /* 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 +2035 @@
 
 /**
- * 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 +2265 @@
     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 +2284 @@
 
     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 +2362 @@
 
     /* 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 +2372 @@
         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 +2380 @@
     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. */