Changeset 29394 in vbox for trunk/src/VBox/HostServices/GuestProperties

Timestamp:

May 12, 2010 1:27:04 AM (15 years ago)

Author:

vboxsync

Message:

GuestProperties: Try make sure the timestamp is somewhat unique because the code is making assumptions about this (RTTimeNow granularity is very coarse on windows). Fixed several derefernces of rend in getOldNotificationInternal that VC++'s strictness checks pointed out. Also cleaned up the method, dropping the unncessary and somewhat historical/puzzling RT_SUCCESS(rc) bits.

Location:

trunk/src/VBox/HostServices/GuestProperties

Files:

• service.cpp (modified) (9 diffs)
• testcase/tstGuestPropSvc.cpp (modified) (4 diffs)

trunk/src/VBox/HostServices/GuestProperties/service.cpp

@@ -97 +97 @@
 
     /** Are two properties equal? */
-    bool operator== (const Property &prop)
-    {
-        return (   mName == prop.mName
-                && mValue == prop.mValue
-                && mTimestamp == prop.mTimestamp
-                && mFlags == prop.mFlags
-               );
+    bool operator==(const Property &prop)
+    {
+        if (mTimestamp != prop.mTimestamp)
+            return false;
+        if (mFlags != prop.mFlags)
+            return false;
+        if (mName != prop.mName)
+            return false;
+        if (mValue != prop.mValue)
+            return false;
+        return true;
     }
 
@@ -164 +168 @@
     /** User data pointer to be supplied to the host callback function */
     void *mpvHostData;
+    /** The previous timestamp.
+     * This is used by getCurrentTimestamp() to decrease the chance of
+     * generating duplicate timestamps.  */
+    uint64_t mPrevTimestamp;
+    /** The number of consecutive timestamp adjustments that we've made.
+     * Together with mPrevTimestamp, this defines a set of obsolete timestamp
+     * values: {(mPrevTimestamp - mcTimestampAdjustments), ..., mPrevTimestamp} */
+    uint64_t mcTimestampAdjustments;
 
     /**
@@ -193 +205 @@
          *  - Matches the pszPatterns
          */
+        /** @todo r=bird: This incorrectly ASSUMES that mTimestamp is unique.
+         *  The timestamp resolution can be very coarse on windows for instance. */
         PropertyList::const_iterator it = mGuestNotifications.begin();
         for (;    it != mGuestNotifications.end()
-               && it->mTimestamp != u64Timestamp; ++it) {}
+               && it->mTimestamp != u64Timestamp; ++it)
+            {}
         if (it == mGuestNotifications.end())  /* Not found */
             it = mGuestNotifications.begin();
@@ -239 +254 @@
         , mpfnHostCallback(NULL)
         , mpvHostData(NULL)
+        , mPrevTimestamp(0)
+        , mcTimestampAdjustments(0)
     { }
 
@@ -319 +336 @@
 private:
     static DECLCALLBACK(int) reqThreadFn(RTTHREAD ThreadSelf, void *pvUser);
+    uint64_t getCurrentTimestamp(void);
     int validateName(const char *pszName, uint32_t cbName);
     int validateValue(const char *pszValue, uint32_t cbValue);
@@ -344 +362 @@
 
 /**
+ * Gets the current timestamp.
+ *
+ * Since the RTTimeNow resolution can be very coarse, this method takes some
+ * simple steps to try avoid returning the same timestamp for two consecutive
+ * calls.  Code like getOldNotification() more or less assumes unique
+ * timestamps.
+ *
+ * @returns Nanosecond timestamp.
+ */
+uint64_t Service::getCurrentTimestamp(void)
+{
+    RTTIMESPEC time;
+    uint64_t u64NanoTS = RTTimeSpecGetNano(RTTimeNow(&time));
+    if (mPrevTimestamp - u64NanoTS > mcTimestampAdjustments)
+        mcTimestampAdjustments = 0;
+    else
+    {
+        mcTimestampAdjustments++;
+        u64NanoTS = mPrevTimestamp + 1;
+    }
+    this->mPrevTimestamp = u64NanoTS;
+    return u64NanoTS;
+}
+
+/**
  * Check that a string fits our criteria for a property name.
  *
@@ -572 +615 @@
     uint32_t cchFlags = 0;
     uint32_t fFlags = NILFLAG;
-    RTTIMESPEC time;
-    uint64_t u64TimeNano = RTTimeSpecGetNano(RTTimeNow(&time));
+    uint64_t u64TimeNano = getCurrentTimestamp();
 
     LogFlowThisFunc(("\n"));
@@ -702 +744 @@
         if (rc == VINF_SUCCESS && found)
         {
-            RTTIMESPEC time;
-            uint64_t u64Timestamp = RTTimeSpecGetNano(RTTimeNow(&time));
+            uint64_t u64Timestamp = getCurrentTimestamp();
             mProperties.erase(it);
             // if (isGuest)  /* Notify the host even for properties that the host
@@ -808 +849 @@
                                         Property *pProp)
 {
-    int rc = VINF_SUCCESS;
-    bool warn = false;
-
     /* We count backwards, as the guest should normally be querying the
      * most recent events. */
+    int rc = VWRN_NOT_FOUND;
     PropertyList::reverse_iterator it = mGuestNotifications.rbegin();
-    for (; it->mTimestamp != u64Timestamp && it != mGuestNotifications.rend();
-         ++it) {}
-    /* Warn if the timestamp was not found. */
-    if (it->mTimestamp != u64Timestamp)
-        warn = true;
+    for (; it != mGuestNotifications.rend(); ++it)
+        if (it->mTimestamp == u64Timestamp)
+        {
+            rc = VINF_SUCCESS;
+            break;
+        }
+
     /* Now look for an event matching the patterns supplied.  The base()
      * member conveniently points to the following element. */
     PropertyList::iterator base = it.base();
-    for (; !base->Matches(pszPatterns) && base != mGuestNotifications.end();
-         ++base) {}
-    if (RT_SUCCESS(rc) && base != mGuestNotifications.end())
-        *pProp = *base;
-    else if (RT_SUCCESS(rc))
-        *pProp = Property();
-    if (warn)
-        rc = VWRN_NOT_FOUND;
+    for (; base != mGuestNotifications.end(); ++base)
+        if (base->Matches(pszPatterns))
+        {
+            *pProp = *base;
+            return rc;
+        }
+    *pProp = Property();
     return rc;
 }

trunk/src/VBox/HostServices/GuestProperties/testcase/tstGuestPropSvc.cpp

@@ -727 +727 @@
     int rc = VINF_SUCCESS;
     VBOXHGCMCALLHANDLE_TYPEDEF callHandle = { VINF_SUCCESS };
-    char chBuffer[MAX_NAME_LEN + MAX_VALUE_LEN + MAX_FLAGS_LEN];
+    char achBuffer[MAX_NAME_LEN + MAX_VALUE_LEN + MAX_FLAGS_LEN];
     static char szPattern[] = "";
 
@@ -739 +739 @@
     paParms[0].setPointer ((void *) szPattern, sizeof(szPattern));
     paParms[1].setUInt64 (u64Timestamp);
-    paParms[2].setPointer ((void *) chBuffer, getNotifications[0].cchBuffer - 1);
+    paParms[2].setPointer ((void *) achBuffer, getNotifications[0].cchBuffer - 1);
     pTable->pfnCall(pTable->pvService, &callHandle, 0, NULL,
                     GET_NOTIFICATION, 4, paParms);
@@ -760 +760 @@
         paParms[0].setPointer ((void *) szPattern, sizeof(szPattern));
         paParms[1].setUInt64 (u64Timestamp);
-        paParms[2].setPointer ((void *) chBuffer, sizeof(chBuffer));
+        paParms[2].setPointer ((void *) achBuffer, sizeof(achBuffer));
         pTable->pfnCall(pTable->pvService, &callHandle, 0, NULL,
                         GET_NOTIFICATION, 4, paParms);
@@ -768 +768 @@
             || RT_FAILURE(paParms[3].getUInt32 (&u32Size))
             || u32Size != getNotifications[i].cchBuffer
-            || memcmp(chBuffer, getNotifications[i].pchBuffer, u32Size) != 0
+            || memcmp(achBuffer, getNotifications[i].pchBuffer, u32Size) != 0
            )
         {
-            RTPrintf("Failed to get notification for property '%s'.\n",
-                     getNotifications[i].pchBuffer);
+            RTPrintf("Failed to get notification for property '%s' (rc=%Rrc).\n",
+                     getNotifications[i].pchBuffer, rc);
             rc = VERR_UNRESOLVED_ERROR;
         }