Changeset 10305 in vbox for trunk/src/VBox/Main

Timestamp:

Jul 7, 2008 10:33:33 AM (16 years ago)

Author:

vboxsync

Message:

Main: Lock the object before switching on non-const data members.

Location:

trunk/src/VBox/Main

Files:

• ConsoleImpl.cpp (modified) (6 diffs)
• MachineImpl.cpp (modified) (6 diffs)
• SessionImpl.cpp (modified) (2 diffs)
• idl/VirtualBox.xidl (modified) (2 diffs)
• include/SessionImpl.h (modified) (1 diff)

trunk/src/VBox/Main/ConsoleImpl.cpp

@@ -3533 +3533 @@
 }
 
+/**
+ * @note Temporarily locks this object for writing.
+ */
 HRESULT Console::getGuestProperty (INPTR BSTR aKey, BSTR *aValue)
 {
-#ifndef VBOX_WITH_INFO_SVC
-    HRESULT hrc = E_NOTIMPL;
+#if !defined (VBOX_WITH_INFO_SVC)
+    return E_NOTIMPL;
 #else
-    if (!VALID_PTR(aKey))
+    if (!VALID_PTR (aKey))
+        return E_INVALIDARG;
+    if (!VALID_PTR (aValue))
         return E_POINTER;
-    if (!VALID_PTR(aValue))
-        return E_POINTER;
 
     AutoCaller autoCaller (this);
-    CheckComRCReturnRC (autoCaller.rc());
+    AssertComRCReturnRC (autoCaller.rc());
 
     /* protect mpVM (if not NULL) */
@@ -3550 +3553 @@
     CheckComRCReturnRC (autoVMCaller.rc());
 
-    HRESULT hrc = E_UNEXPECTED;
+    /* Note: validity of mVMMDev which is bound to uninit() is guaranteed by
+     * autoVMCaller, so there is no need to hold a lock of this */
+
+    HRESULT rc = E_UNEXPECTED;
     using namespace svcInfo;
 
     VBOXHGCMSVCPARM parm[3];
     Utf8Str Utf8Key = aKey;
-    Utf8Str Utf8Value(KEY_MAX_VALUE_LEN);
+    Utf8Str Utf8Value (KEY_MAX_VALUE_LEN);
 
     parm[0].type = VBOX_HGCM_SVC_PARM_PTR;
@@ -3565 +3571 @@
     parm[1].u.pointer.addr = Utf8Value.mutableRaw();
     parm[1].u.pointer.size = KEY_MAX_VALUE_LEN;
-    int rc = mVMMDev->hgcmHostCall ("VBoxSharedInfoSvc", GET_CONFIG_KEY_HOST, 3, &parm[0]);
+    int vrc = mVMMDev->hgcmHostCall ("VBoxSharedInfoSvc", GET_CONFIG_KEY_HOST,
+                                     3, &parm[0]);
     /* The returned string should never be able to be greater than our buffer */
-    AssertLogRel(rc != VERR_BUFFER_OVERFLOW);
-    if (RT_SUCCESS(rc) || (VERR_NOT_FOUND == rc))
-    {
-        hrc = S_OK;
-        if (rc != VERR_NOT_FOUND)
-            Utf8Value.cloneTo(aValue);
+    AssertLogRel (vrc != VERR_BUFFER_OVERFLOW);
+    if (RT_SUCCESS (vrc) || (VERR_NOT_FOUND == vrc))
+    {
+        rc = S_OK;
+        if (vrc != VERR_NOT_FOUND)
+            Utf8Value.cloneTo (aValue);
         else
             aValue = NULL;
     }
     else
-        hrc = setError (E_UNEXPECTED, tr ("hgcmHostCall to VBoxSharedInfoSvc failed: %Rrc"), rc);
-#endif
-    return hrc;
-}
-
+        rc = setError (E_UNEXPECTED,
+            tr ("Failed to call the VBoxSharedInfoSvc service (%Rrc)"), vrc);
+    return rc;
+#endif /* else !defined (VBOX_WITH_INFO_SVC) */
+}
+
+/**
+ * @note Temporarily locks this object for writing.
+ */
 HRESULT Console::setGuestProperty (INPTR BSTR aKey, INPTR BSTR aValue)
 {
-#ifndef VBOX_WITH_INFO_SVC
-    HRESULT hrc = E_NOTIMPL;
+#if !defined (VBOX_WITH_INFO_SVC)
+    return E_NOTIMPL;
 #else
-    if (!VALID_PTR(aKey))
-        return E_POINTER;
-    if ((aValue != NULL) && !VALID_PTR(aValue))
-        return E_POINTER;
+    if (!VALID_PTR (aKey))
+        return E_INVALIDARG;
+    if ((aValue != NULL) && !VALID_PTR (aValue))
+        return E_INVALIDARG;
 
     AutoCaller autoCaller (this);
-    CheckComRCReturnRC (autoCaller.rc());
+    AssertComRCReturnRC (autoCaller.rc());
 
     /* protect mpVM (if not NULL) */
@@ -3599 +3610 @@
     CheckComRCReturnRC (autoVMCaller.rc());
 
-    HRESULT hrc = E_UNEXPECTED;
+    /* Note: validity of mVMMDev which is bound to uninit() is guaranteed by
+     * autoVMCaller, so there is no need to hold a lock of this */
+
+    HRESULT rc = E_UNEXPECTED;
     using namespace svcInfo;
 
     VBOXHGCMSVCPARM parm[2];
     Utf8Str Utf8Key = aKey;
-    int rc = VINF_SUCCESS;
+    int vrc = VINF_SUCCESS;
 
     parm[0].type = VBOX_HGCM_SVC_PARM_PTR;
@@ -3619 +3633 @@
         /* The + 1 is the null terminator */
         parm[1].u.pointer.size = Utf8Value.length() + 1;
-        rc = mVMMDev->hgcmHostCall ("VBoxSharedInfoSvc", SET_CONFIG_KEY_HOST, 2, &parm[0]);
+        vrc = mVMMDev->hgcmHostCall ("VBoxSharedInfoSvc", SET_CONFIG_KEY_HOST,
+                                     2, &parm[0]);
     }
     else
-        rc = mVMMDev->hgcmHostCall ("VBoxSharedInfoSvc", DEL_CONFIG_KEY_HOST, 1, &parm[0]);
-    if (RT_SUCCESS(rc))
-        hrc = S_OK;
+        vrc = mVMMDev->hgcmHostCall ("VBoxSharedInfoSvc", DEL_CONFIG_KEY_HOST,
+                                     1, &parm[0]);
+    if (RT_SUCCESS (vrc))
+        rc = S_OK;
     else
-        hrc = setError (E_UNEXPECTED, tr ("hgcmHostCall to VBoxSharedInfoSvc failed: %Rrc"), rc);
-#endif
-    return hrc;
+        rc = setError (E_UNEXPECTED,
+            tr ("Failed to call the VBoxSharedInfoSvc service (%Rrc)"), vrc);
+    return rc;
+#endif /* else !defined (VBOX_WITH_INFO_SVC) */
 }
 
@@ -4181 +4198 @@
             pValue = CFGMR3GetNextValue (pValue);
     }
-    /* In a second stage, we remove any extra data keys corresponding to 
+    /* In a second stage, we remove any extra data keys corresponding to
      * properties which aren't in the CFGM node. */
     Bstr strExtraDataKey;

trunk/src/VBox/Main/MachineImpl.cpp

@@ -2678 +2678 @@
 }
 
-/**
- * Read a value from the host/guest property store.  If a session is
- * currently open for the guest then query the session object for the value,
- * since the current values of the property store will be held in RAM in the
- * session.  Otherwise read the value from machine extra data, where it is
- * stored between sessions.  Returns E_FAIL if we are currently transitioning
- * between states.
- */
 STDMETHODIMP Machine::GetGuestProperty (INPTR BSTR aKey, BSTR *aValue)
 {
-#ifndef VBOX_WITH_INFO_SVC
-    HRESULT hrc = E_NOTIMPL;
+#if !defined (VBOX_WITH_INFO_SVC)
+    return E_NOTIMPL;
 #else
-    if (!VALID_PTR(aKey))
+    if (!VALID_PTR (aKey))
+        return E_INVALIDARG;
+    if (!VALID_PTR (aValue))
         return E_POINTER;
-    if (!VALID_PTR(aValue))
-        return E_POINTER;
 
     AutoCaller autoCaller (this);
     CheckComRCReturnRC (autoCaller.rc());
 
+    AutoReadLock alock (this);
+
     using namespace svcInfo;
-    HRESULT hrc = E_FAIL;
+    HRESULT rc = E_FAIL;
 
     switch (mData->mSession.mState)
@@ -2706 +2700 @@
         case SessionState_Closed:
         {
-            AutoReadLock alock (this);
-
             /* The "+ 1" in the length is the null terminator. */
-            Bstr strKey(Bstr(aKey).length() + VBOX_SHARED_INFO_PREFIX_LEN + 1);
+            Bstr strKey (Bstr (aKey).length() + VBOX_SHARED_INFO_PREFIX_LEN + 1);
             BSTR strKeyRaw = strKey.mutableRaw();
 
@@ -2716 +2708 @@
             for (unsigned i = 0; i < VBOX_SHARED_INFO_PREFIX_LEN; ++i)
                 /* I take it this is legal, at least g++ accepts it. */
-                strKeyRaw[i] = VBOX_SHARED_INFO_KEY_PREFIX[i];
+                strKeyRaw [i] = VBOX_SHARED_INFO_KEY_PREFIX[i];
             /* The "+ 1" in the length is the null terminator. */
-            for (unsigned i = 0, len = Bstr(aKey).length() + 1; i < len; ++i)
-                strKeyRaw[i + VBOX_SHARED_INFO_PREFIX_LEN] = aKey[i];
-            hrc = GetExtraData(strKey, aValue);
+            for (unsigned i = 0, len = Bstr (aKey).length() + 1; i < len; ++i)
+                strKeyRaw [i + VBOX_SHARED_INFO_PREFIX_LEN] = aKey [i];
+            rc = GetExtraData (strKey, aValue);
             break;
         }
         case SessionState_Open:
         {
-            ComPtr <IInternalSessionControl> directControl;
+            if (mData->mSession.mState != SessionState_Open)
             {
-                AutoReadLock alock (this);
-        
-                if (mData->mSession.mState != SessionState_Open)
-                    return setError (E_FAIL,
-                        tr ("Machine session is not open (session state: %d) - please retry."),
-                        mData->mSession.mState);
-        
-                directControl = mData->mSession.mDirectControl;
+                rc = setError (E_FAIL,
+                    tr ("Session is not open (session state: %d)"),
+                    mData->mSession.mState);
+                break;
             }
 
-            hrc = directControl->AccessGuestProperty (aKey, NULL,
-                                                      false /* isSetter */,
-                                                      aValue);
+            ComPtr <IInternalSessionControl> directControl =
+                mData->mSession.mDirectControl;
+
+            /* just be on the safe side when calling another process */
+            alock.unlock();
+
+            rc = directControl->AccessGuestProperty (aKey, NULL,
+                                                     false /* isSetter */,
+                                                     aValue);
             break;
         }
         default:
-            hrc = setError (E_FAIL, tr ("Machine session is currently transitioning (session state: %d) - please retry."),
-                            mData->mSession.mState);
-    }
-#endif  /* VBOX_WITH_INFO_SVC not defined */
-    return hrc;
+            rc = setError (E_FAIL,
+                tr ("Session is currently transitioning (session state: %d)"),
+                mData->mSession.mState);
+    }
+    return rc;
+#endif /* else !defined (VBOX_WITH_INFO_SVC) */
 }
 
@@ -2760 +2755 @@
 STDMETHODIMP Machine::SetGuestProperty (INPTR BSTR aKey, INPTR BSTR aValue)
 {
-#ifndef VBOX_WITH_INFO_SVC
-    HRESULT hrc = E_NOTIMPL;
+#if !defined (VBOX_WITH_INFO_SVC)
+    return E_NOTIMPL;
 #else
-    if (!VALID_PTR(aKey))
-        return E_POINTER;
-    if ((aValue != NULL) && !VALID_PTR(aValue))
-        return E_POINTER;
+    if (!VALID_PTR (aKey))
+        return E_INVALIDARG;
+    if ((aValue != NULL) && !VALID_PTR (aValue))
+        return E_INVALIDARG;
 
     AutoCaller autoCaller (this);
     CheckComRCReturnRC (autoCaller.rc());
 
+    /* SetExtraData() needs a write lock */
+    AutoWriteLock alock (this);
+
     using namespace svcInfo;
-    HRESULT hrc = E_FAIL;
+    HRESULT rc = E_FAIL;
 
     switch (mData->mSession.mState)
@@ -2778 +2776 @@
         case SessionState_Closed:
         {
-            AutoWriteLock alock (this);
-
             /* The "+ 1" in the length is the null terminator. */
-            Bstr strKey(Bstr(aKey).length() + VBOX_SHARED_INFO_PREFIX_LEN + 1);
+            Bstr strKey (Bstr (aKey).length() + VBOX_SHARED_INFO_PREFIX_LEN + 1);
             BSTR strKeyRaw = strKey.mutableRaw();
 
@@ -2788 +2784 @@
             for (unsigned i = 0; i < VBOX_SHARED_INFO_PREFIX_LEN; ++i)
                 /* I take it this is legal, at least g++ accepts it. */
-                strKeyRaw[i] = VBOX_SHARED_INFO_KEY_PREFIX[i];
+                strKeyRaw [i] = VBOX_SHARED_INFO_KEY_PREFIX[i];
             /* The "+ 1" in the length is the null terminator. */
-            for (unsigned i = 0, len = Bstr(aKey).length() + 1; i < len; ++i)
-                strKeyRaw[i + VBOX_SHARED_INFO_PREFIX_LEN] = aKey[i];
-            hrc = SetExtraData(strKey, aValue);
+            for (unsigned i = 0, len = Bstr (aKey).length() + 1; i < len; ++i)
+                strKeyRaw [i + VBOX_SHARED_INFO_PREFIX_LEN] = aKey [i];
+            rc = SetExtraData (strKey, aValue);
             break;
         }
         case SessionState_Open:
         {
-            ComPtr <IInternalSessionControl> directControl;
+            if (mData->mSession.mState != SessionState_Open)
             {
-                AutoReadLock alock (this);
-        
-                if (mData->mSession.mState != SessionState_Open)
-                    return setError (E_FAIL,
-                        tr ("Machine session is not open (session state: %d) - please retry."),
-                        mData->mSession.mState);
-        
-                directControl = mData->mSession.mDirectControl;
+                rc = setError (E_FAIL,
+                    tr ("Session is not open (session state: %d)"),
+                    mData->mSession.mState);
+                break;
             }
 
-            BSTR dummy;
-            hrc = directControl->AccessGuestProperty (aKey, aValue,
-                                                      true /* isSetter */,
-                                                      &dummy);
+            ComPtr <IInternalSessionControl> directControl =
+                mData->mSession.mDirectControl;
+
+            /* just be on the safe side when calling another process */
+            alock.leave();
+
+            BSTR dummy = NULL;
+            rc = directControl->AccessGuestProperty (aKey, aValue,
+                                                     true /* isSetter */,
+                                                     &dummy);
             break;
         }
         default:
-            hrc = setError (E_FAIL, tr ("Machine session is currently transitioning (session state: %d) - please retry."),
-                            mData->mSession.mState);
-    }
-#endif  /* VBOX_WITH_INFO_SVC not defined */
-    return hrc;
+            rc = setError (E_FAIL,
+                tr ("Session is currently transitioning (session state: %d)"),
+                mData->mSession.mState);
+    }
+    return rc;
+#endif /* else !defined (VBOX_WITH_INFO_SVC) */
 }
 

trunk/src/VBox/Main/SessionImpl.cpp

@@ -655 +655 @@
 
 STDMETHODIMP Session::AccessGuestProperty (INPTR BSTR aKey, INPTR BSTR aValue,
-                                           BOOL isSetter, BSTR *retValue)
+                                           BOOL aIsSetter, BSTR *aRetValue)
 {
 #ifdef VBOX_WITH_INFO_SVC
@@ -663 +663 @@
     if (mState != SessionState_Open)
         return setError (E_FAIL,
-                         tr ("Machine session is not open (session state: %d) - please retry."),
-                         mState);
+            tr ("Machine session is not open (session state: %d)."),
+            mState);
     AssertReturn (mType == SessionType_Direct, E_UNEXPECTED);
-    if (!VALID_PTR(aKey))
+    if (!VALID_PTR (aKey))
         return E_POINTER;
-    if (!isSetter && !VALID_PTR(retValue))
+    if (!aIsSetter && !VALID_PTR (aRetValue))
         return E_POINTER;
     /* aValue can be NULL for a setter call if the property is to be deleted. */
-    if (isSetter && (aValue != NULL) && !VALID_PTR(aValue))
-        return E_POINTER;
-    if (!isSetter)
-        return mConsole->getGuestProperty(aKey, retValue);
+    if (aIsSetter && (aValue != NULL) && !VALID_PTR (aValue))
+        return E_INVALIDARG;
+    if (!aIsSetter)
+        return mConsole->getGuestProperty (aKey, aRetValue);
     else
-        return mConsole->setGuestProperty(aKey, aValue);
+        return mConsole->setGuestProperty (aKey, aValue);
 #else
     return E_NOTIMPL;

trunk/src/VBox/Main/idl/VirtualBox.xidl

@@ -3471 +3471 @@
       <desc>
         Reads a value from the machine's host/guest property store.
+
+        Reads a value from the machine's host/guest property store.
+
+        If a session for this virtual machine is currently open then queries the
+        session object for the value, since the current values of the property
+        store will be held in RAM in the session. Otherwise reads the value from
+        machine extra data, where it is stored between sessions.
+
+        This method will fail if we are currently transitioning between session
+        states.
       </desc>
       <param name="key" type="wstring" dir="in">
@@ -3489 +3499 @@
         Sets, changes or deletes a value in the machine's host/guest
         property store.
+
+        If a session for this virtual machine is currently open then changes the
+        value on the session object, since the current values of the property
+        store will be held in RAM in the session. Otherwise changes the value
+        in machine extra data, where it is stored between sessions.
+
+        This method will fail if we are currently transitioning between session
+        states.
       </desc>
       <param name="key" type="wstring" dir="in">

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

@@ -106 +106 @@
     STDMETHOD(OnUSBDeviceDetach) (INPTR GUIDPARAM aId, IVirtualBoxErrorInfo *aError);
     STDMETHOD(OnShowWindow) (BOOL aCheck, BOOL *aCanShow, ULONG64 *aWinId);
-    STDMETHOD(AccessGuestProperty) (INPTR BSTR aKey, INPTR BSTR aValue, BOOL isSetter, BSTR *retValue);
+    STDMETHOD(AccessGuestProperty) (INPTR BSTR aKey, INPTR BSTR aValue,
+                                    BOOL aIsSetter, BSTR *aRetValue);
 
     // for VirtualBoxSupportErrorInfoImpl