Changeset 8388 in vbox for trunk/src/VBox

Timestamp:

Apr 25, 2008 2:27:15 PM (17 years ago)

Author:

vboxsync

Message:

Main: Fixed very old regression: Failure to lock the machine settings file was not properly reported which led to assertions in debug build or to misbehavior in release builds. Examples of such failure were renaming the settings file manually or moving the machine directory w/o unregistering.

File:

• trunk/src/VBox/Main/MachineImpl.cpp (modified) (10 diffs)

trunk/src/VBox/Main/MachineImpl.cpp

@@ -121 +121 @@
 {
     mRegistered = FALSE;
+    mAccessible = FALSE;
     /* mUuid is initialized in Machine::init() */
 
@@ -400 +401 @@
     mData->mConfigFileFull = configFileFull;
 
-    /* start with accessible */
-    mData->mAccessible = TRUE;
-
-    if (aMode != Init_New)
-    {
-        /* lock the settings file */
-        rc = lockConfig();
-
-        if (aMode == Init_Registered && FAILED (rc))
-        {
-            /* If the machine is registered, then, instead of returning a
-             * failure, we mark it as inaccessible and set the result to
-             * success to give it a try later */
-            mData->mAccessible = FALSE;
-            /* fetch the current error info */
-            mData->mAccessError = com::ErrorInfo();
-            LogWarning (("Machine {%Vuuid} is inaccessible! [%ls]\n",
-                         mData->mUuid.raw(),
-                         mData->mAccessError.getText().raw()));
-            rc = S_OK;
-        }
-    }
-    else
-    {
-        /* check for the file existence */
-        RTFILE f = NIL_RTFILE;
-        int vrc = RTFileOpen (&f, configFileFull, RTFILE_O_READ);
-        if (VBOX_SUCCESS (vrc) || vrc == VERR_SHARING_VIOLATION)
-        {
-            rc = setError (E_FAIL,
-                tr ("Settings file '%s' already exists"), configFileFull);
-            if (VBOX_SUCCESS (vrc))
-                RTFileClose (f);
-        }
-        else
-        {
-            if (vrc != VERR_FILE_NOT_FOUND && vrc != VERR_PATH_NOT_FOUND)
-                rc = setError (E_FAIL,
-                    tr ("Invalid settings file name: '%ls' (%Vrc)"),
-                        mData->mConfigFileFull.raw(), vrc);
-        }
-
-        /* reset mAccessible to make sure uninit() called by the AutoInitSpan
-         * destructor will not call uninitDataAndChildObjects() (we haven't
-         * initialized anything yet) */
-        if (FAILED (rc))
-            mData->mAccessible = FALSE;
-    }
-
-    CheckComRCReturnRC (rc);
-
     if (aMode == Init_Registered)
     {
+        mData->mRegistered = TRUE;
+
         /* store the supplied UUID (will be used to check for UUID consistency
          * in loadSettings() */
         unconst (mData->mUuid) = *aId;
-        /* try to  load settings only if the settings file is accessible */
-        if (mData->mAccessible)
-            rc = registeredInit();
+        rc = registeredInit();
     }
     else
     {
-        rc = initDataAndChildObjects();
+        if (aMode == Init_Existing)
+        {
+            /* lock the settings file */
+            rc = lockConfig();
+        }
+        else if (aMode == Init_New)
+        {
+            /* check for the file existence */
+            RTFILE f = NIL_RTFILE;
+            int vrc = RTFileOpen (&f, configFileFull, RTFILE_O_READ);
+            if (VBOX_SUCCESS (vrc) || vrc == VERR_SHARING_VIOLATION)
+            {
+                rc = setError (E_FAIL,
+                    tr ("Settings file '%s' already exists"), configFileFull);
+                if (VBOX_SUCCESS (vrc))
+                    RTFileClose (f);
+            }
+            else
+            {
+                if (vrc != VERR_FILE_NOT_FOUND && vrc != VERR_PATH_NOT_FOUND)
+                    rc = setError (E_FAIL,
+                        tr ("Invalid settings file name: '%ls' (%Vrc)"),
+                            mData->mConfigFileFull.raw(), vrc);
+            }
+        }
+        else
+            AssertFailed();
 
         if (SUCCEEDED (rc))
-        {
+            rc = initDataAndChildObjects();
+
+        if (SUCCEEDED (rc))
+        {
+            /* set to true now to cause uninit() to call
+             * uninitDataAndChildObjects() on failure */
+            mData->mAccessible = TRUE;
+
             if (aMode != Init_New)
             {
@@ -529 +512 @@
     AssertReturn (mType == IsMachine, E_FAIL);
     AssertReturn (!mData->mUuid.isEmpty(), E_FAIL);
-
-    HRESULT rc = initDataAndChildObjects();
-    CheckComRCReturnRC (rc);
-
-    if (!mData->mAccessible)
-        rc = lockConfig();
-
-    /* Temporarily reset the registered flag in order to let setters potentially
-     * called from loadSettings() succeed (isMutable() used in all setters
-     * will return FALSE for a Machine instance if mRegistered is TRUE). */
-    mData->mRegistered = FALSE;
+    AssertReturn (!mData->mAccessible, E_FAIL);
+
+    HRESULT rc = lockConfig();
 
     if (SUCCEEDED (rc))
-    {
+        rc = initDataAndChildObjects();
+
+    if (SUCCEEDED (rc))
+    {
+        /* Temporarily reset the registered flag in order to let setters
+         * potentially called from loadSettings() succeed (isMutable() used in
+         * all setters will return FALSE for a Machine instance if mRegistered
+         * is TRUE). */
+        mData->mRegistered = FALSE;
+
         rc = loadSettings (true /* aRegistered */);
+
+        /* Restore the registered flag (even on failure) */
+        mData->mRegistered = TRUE;
 
         if (FAILED (rc))
@@ -551 +538 @@
     if (SUCCEEDED (rc))
     {
+        /* Set mAccessible to TRUE only if we successfully locked and loaded
+         * the settings file */
         mData->mAccessible = TRUE;
 
@@ -565 +554 @@
          * failure, we mark it as inaccessible and set the result to
          * success to give it a try later */
-        mData->mAccessible = FALSE;
+
         /* fetch the current error info */
         mData->mAccessError = com::ErrorInfo();
@@ -581 +570 @@
         rc = S_OK;
     }
-
-    /* Restore the registered flag (even on failure) */
-    mData->mRegistered = TRUE;
 
     return rc;
@@ -3670 +3656 @@
 
 /**
- *  Helper to initialize all associated child objects
- *  and allocate data structures.
- *
- *  This method must be called as a part of the object's initialization
- *  procedure (usually done in the #init() method).
- *
- *  @note Must be called only from #init() or from #registeredInit().
+ * Helper to initialize all associated child objects and allocate data
+ * structures.
+ *
+ * This method must be called as a part of the object's initialization procedure
+ * (usually done in the #init() method).
+ *
+ * @note Must be called only from #init() or from #registeredInit().
  */
 HRESULT Machine::initDataAndChildObjects()
@@ -3684 +3670 @@
     AssertComRCReturn (autoCaller.state() == InInit ||
                        autoCaller.state() == Limited, E_FAIL);
+
+    AssertReturn (!mData->mAccessible, E_FAIL);
 
     /* allocate data structures */
@@ -3749 +3737 @@
 
 /**
- *  Helper to uninitialize all associated child objects
- *  and to free all data structures.
- *
- *  This method must be called as a part of the object's uninitialization
- *  procedure (usually done in the #uninit() method).
- *
- *  @note Must be called only from #uninit() or from #registeredInit().
+ * Helper to uninitialize all associated child objects and to free all data
+ * structures.
+ *
+ * This method must be called as a part of the object's uninitialization
+ * procedure (usually done in the #uninit() method).
+ *
+ * @note Must be called only from #uninit() or from #registeredInit().
  */
 void Machine::uninitDataAndChildObjects()
@@ -6700 +6688 @@
                               RTFILE_O_DENY_WRITE);
         if (VBOX_FAILURE (vrc))
+        {
             mData->mHandleCfgFile = NIL_RTFILE;
+
+            rc = setError (E_FAIL,
+                tr ("Could not lock the settings file '%ls' (%Vrc)"),
+                mData->mConfigFileFull.raw(), vrc);
+        }
     }