Changeset 5627 in vbox

Timestamp:

Nov 5, 2007 12:45:05 PM (17 years ago)

Author:

vboxsync

Message:

Main: Improved Machine incapsulation by removing data(), userData(), hwData(), hdData() and other similar methods which also fixed the VBoxSVC crash when changing a setting of a valid VM through VBoxManage if there were one or more inaccessible VMs (#2466).

Location:

trunk/src/VBox/Main

Files:

• HostImpl.cpp (modified) (2 diffs)
• MachineImpl.cpp (modified) (11 diffs)
• SnapshotImpl.cpp (modified) (5 diffs)
• VirtualBoxImpl.cpp (modified) (12 diffs)
• include/MachineImpl.h (modified) (5 diffs)

trunk/src/VBox/Main/HostImpl.cpp

@@ -1380 +1380 @@
 
     if (device->state() == USBDeviceState_USBDeviceCaptured)
+    {
+        /* Machine::name() requires a read lock */
+        AutoReaderLock machLock (device->machine());
+
         return setError (E_INVALIDARG,
             tr ("USB device '%s' with UUID {%Vuuid} is already captured by the virtual "
                 "machine '%ls'"),
             device->name().raw(), id.raw(),
-            aMachine->userData()->mName.raw());
+            device->machine()->name());
+    }
 
     /* try to capture the device */
@@ -2186 +2191 @@
     AssertReturn (aDevice->isStatePending() == false, false);
 
-    bool hasMatch = false;
-
-    {
-        /* We're going to use aMachine which is not our child/parent, add a
-         * caller */
-        AutoCaller autoCaller (aMachine);
-        if (!autoCaller.isOk())
-        {
-            /* silently return, the machine might be not running any more */
-            return false;
-        }
-
-        /* enter the machine's lock because we want to access its USB controller */
-        AutoReaderLock machineLock (aMachine);
-        hasMatch = aMachine->usbController()->hasMatchingFilter (aDevice);
-    }
+    bool hasMatch = aMachine->hasMatchingUSBFilter (aDevice);
 
     if (hasMatch)

trunk/src/VBox/Main/MachineImpl.cpp

@@ -2515 +2515 @@
  *  The returned session machine is null if no direct session is currently open.
  *
- *  @Note locks this object for reading.
+ *  @note locks this object for reading.
  */
 ComObjPtr <SessionMachine> Machine::sessionMachine()
@@ -2521 +2521 @@
     ComObjPtr <SessionMachine> sm;
 
-    AutoCaller autoCaller (this);
-    /* the machine may be inaccessible, so don't assert below */
-    if (FAILED (autoCaller.rc()))
+    AutoLimitedCaller autoCaller (this);
+    AssertComRCReturn (autoCaller.rc(), sm);
+
+    /* return null for inaccessible machines */
+    if (autoCaller.state() != Ready)
         return sm;
 
@@ -2533 +2535 @@
 
     return  sm;
+}
+
+/** 
+ *  Saves the registry entry of this machine to the given configuration node.
+ * 
+ *  @param aEntryNode Node to save the registry entry to.
+ *
+ *  @note locks this object for reading.
+ */
+HRESULT Machine::saveRegistryEntry (CFGNODE aEntryNode)
+{
+    AssertReturn (aEntryNode, E_FAIL);
+
+    AutoLimitedCaller autoCaller (this);
+    AssertComRCReturnRC (autoCaller.rc());
+
+    AutoReaderLock alock (this);
+
+    /* UUID */
+    CFGLDRSetUUID (aEntryNode, "uuid", mData->mUuid.raw());
+    /* settings file name (possibly, relative) */
+    CFGLDRSetBSTR (aEntryNode, "src", mData->mConfigFile);
+
+    return S_OK;
 }
 
@@ -2638 +2664 @@
 bool Machine::isDVDImageUsed (const Guid &aId, ResourceUsage_T aUsage)
 {
-    AutoCaller autoCaller (this);
-    AssertComRCReturn (autoCaller.rc() || autoCaller.state() == Limited, false);
+    AutoLimitedCaller autoCaller (this);
+    AssertComRCReturn (autoCaller.rc(), false);
 
     /* answer 'not attached' if the VM is limited */
@@ -2655 +2681 @@
     /* first, check the current state */
     {
-        const ComObjPtr <DVDDrive> &dvd = m->dvdDrive();
+        const ComObjPtr <DVDDrive> &dvd = m->mDVDDrive;
         AssertReturn (!dvd.isNull(), false);
 
@@ -2715 +2741 @@
 {
     AutoCaller autoCaller (this);
-    AssertComRCReturn (autoCaller.rc() || autoCaller.state() == Limited, false);
+    AssertComRCReturn (autoCaller.rc(), false);
 
     /* answer 'not attached' if the VM is limited */
@@ -2731 +2757 @@
     /* first, check the current state */
     {
-        const ComObjPtr <FloppyDrive> &floppy = m->floppyDrive();
+        const ComObjPtr <FloppyDrive> &floppy = m->mFloppyDrive;
         AssertReturn (!floppy.isNull(), false);
 
@@ -7167 +7193 @@
 
                         const HDData::HDAttachmentList &snapAtts =
-                            snap->data().mMachine->hdData()->mHDAttachments;
+                            snap->data().mMachine->mHDData->mHDAttachments;
 
                         HDData::HDAttachmentList::const_iterator foundIt = snapAtts.end();
@@ -8017 +8043 @@
     Task (SessionMachine *m, Progress *p)
         : machine (m), progress (p)
-        , state (m->data()->mMachineState) // save the current machine state
+        , state (m->mData->mMachineState) // save the current machine state
         , subTask (false), settingsChanged (false)
     {}
@@ -9475 +9501 @@
 
     AutoCaller autoCaller (this);
-    AssertComRCReturn (autoCaller.rc(), autoCaller.rc());
+    AssertComRCReturnRC (autoCaller.rc());
 
     ComPtr <IInternalSessionControl> directControl;
@@ -9488 +9514 @@
 
     return directControl->OnSharedFolderChange (FALSE /* aGlobal */);
+}
+
+/** 
+ *  Returns @c true if this machine's USB controller reports it has a matching
+ *  filter for the given USB device and @c false otherwise.
+ *
+ *  @note Locks this object for reading.
+ */
+bool SessionMachine::hasMatchingUSBFilter (const ComObjPtr <HostUSBDevice> &aDevice)
+{
+    AutoCaller autoCaller (this);
+    /* silently return if not ready -- this method may be called after the
+     * direct machine session has been called */
+    if (!autoCaller.isOk())
+        return false;
+
+    AutoReaderLock alock (this);
+
+    return mUSBController->hasMatchingFilter (aDevice);
 }
 

trunk/src/VBox/Main/SnapshotImpl.cpp

@@ -316 +316 @@
 const Bstr &Snapshot::stateFilePath() const
 {
-    return mData.mMachine->ssData()->mStateFilePath;
+    return mData.mMachine->mSSData->mStateFilePath;
 }
 
@@ -409 +409 @@
 
     AssertReturn (!mData.mMachine.isNull(), false);
-    AssertReturn (!mData.mMachine->dvdDrive().isNull(), false);
-
-    DVDDrive::Data *d = mData.mMachine->dvdDrive()->data().data();
+    AssertReturn (!mData.mMachine->mDVDDrive.isNull(), false);
+
+    DVDDrive::Data *d = mData.mMachine->mDVDDrive->data().data();
 
     if (d &&
@@ -448 +448 @@
 
     AssertReturn (!mData.mMachine.isNull(), false);
-    AssertReturn (!mData.mMachine->dvdDrive().isNull(), false);
-
-    FloppyDrive::Data *d = mData.mMachine->floppyDrive()->data().data();
+    AssertReturn (!mData.mMachine->mFloppyDrive.isNull(), false);
+
+    FloppyDrive::Data *d = mData.mMachine->mFloppyDrive->data().data();
 
     if (d &&
@@ -494 +494 @@
     AssertReturnVoid (isReady());
 
-    Utf8Str path = mData.mMachine->ssData()->mStateFilePath;
+    Utf8Str path = mData.mMachine->mSSData->mStateFilePath;
     LogFlowThisFunc (("Snap[%ls].statePath={%s}\n", mData.mName.raw(), path.raw()));
 
@@ -501 +501 @@
     {
         path = Utf8StrFmt ("%s%s", aNewPath, path.raw() + strlen (aOldPath));
-        mData.mMachine->ssData()->mStateFilePath = path;
+        mData.mMachine->mSSData->mStateFilePath = path;
 
         LogFlowThisFunc (("-> updated: {%s}\n", path.raw()));

trunk/src/VBox/Main/VirtualBoxImpl.cpp

@@ -806 +806 @@
     }
 
-    /*
-     *  Machine::trySetRegistered() will commit and save machine settings, and
-     *  will also call #registerMachine() on success.
-     */
+    AutoCaller machCaller (machine);
+    ComAssertComRCRetRC (machCaller.rc());
+
     rc = registerMachine (machine);
 
     /* fire an event */
     if (SUCCEEDED (rc))
-        onMachineRegistered (machine->data()->mUuid, TRUE);
+        onMachineRegistered (machine->uuid(), TRUE);
 
     return rc;
@@ -864 +863 @@
          ++ it)
     {
-        AutoReaderLock machineLock (*it);
-        if ((*it)->userData()->mName == aName)
-            machine = *it;
+        AutoLimitedCaller machCaller (*it);
+        AssertComRC (machCaller.rc());
+
+        /* skip inaccessible machines */
+        if (machCaller.state() == Machine::Ready)
+        {
+            AutoReaderLock machLock (*it);
+            if ((*it)->name() == aName)
+                machine = *it;
+        }
     }
 
@@ -2775 +2781 @@
         {
             ComObjPtr <Machine> m = *mit;
-            if (m->isDVDImageUsed (aId, aUsage))
+
+            AutoLimitedCaller machCaller (m);
+            AssertComRC (machCaller.rc());
+
+            /* ignore inaccessible machines */
+            if (machCaller.state() == Machine::Ready)
             {
-                /* if not interested in the list, return shortly */
-                if (aMachineIDs == NULL)
-                    return true;
-
-                idSet.insert (m->data()->mUuid);
+                if (m->isDVDImageUsed (aId, aUsage))
+                {
+                    /* if not interested in the list, return shortly */
+                    if (aMachineIDs == NULL)
+                        return true;
+
+                    idSet.insert (m->uuid());
+                }
             }
         }
@@ -2854 +2868 @@
         {
             ComObjPtr <Machine> m = *mit;
-            if (m->isFloppyImageUsed (aId, aUsage))
+
+            AutoLimitedCaller machCaller (m);
+            AssertComRC (machCaller.rc());
+
+            /* ignore inaccessible machines */
+            if (machCaller.state() == Machine::Ready)
             {
-                /* if not interested in the list, return shortly */
-                if (aMachineIDs == NULL)
-                    return true;
-
-                idSet.insert (m->data()->mUuid);
+                if (m->isFloppyImageUsed (aId, aUsage))
+                {
+                    /* if not interested in the list, return shortly */
+                    if (aMachineIDs == NULL)
+                        return true;
+
+                    idSet.insert (m->uuid());
+                }
             }
         }
@@ -2961 +2983 @@
              ++ it)
         {
-            /* mUuid is constant, no need to lock */
-            found = (*it)->data()->mUuid == aId;
+            AutoLimitedCaller machCaller (*it);
+            AssertComRC (machCaller.rc());
+
+            found = (*it)->uuid() == aId;
             if (found && aMachine)
                 *aMachine = *it;
@@ -3653 +3677 @@
                 /// @todo (dmik) move this part to Machine for better incapsulation
 
-                ComObjPtr <Machine> m = *it;
-                AutoReaderLock machineLock (m);
-
-                AssertMsg (m->data(), ("Machine data must not be NULL"));
-                CFGNODE entryNode;
+                CFGNODE entryNode = NULL;
                 CFGLDRAppendChildNode (registryNode, Entry, &entryNode);
-                /* UUID + curly brackets */
-                CFGLDRSetUUID (entryNode, "uuid", unconst (m->data()->mUuid).ptr());
-                /* source */
-                CFGLDRSetBSTR (entryNode, "src", m->data()->mConfigFile);
-                /* done */
+                rc = (*it)->saveRegistryEntry (entryNode);
                 CFGLDRReleaseNode (entryNode);
+                CheckComRCBreakRC (rc);
             }
 
@@ -3670 +3687 @@
         }
         while (0);
-        if (FAILED (rc))
-            break;
+        CheckComRCBreakRC (rc);
 
         /* disk images */
@@ -3691 +3707 @@
                 rc = saveHardDisks (imagesNode);
                 CFGLDRReleaseNode (imagesNode);
-                if (FAILED (rc))
-                    break;
+                CheckComRCBreakRC (rc);
             }
 
@@ -3740 +3755 @@
         }
         while (0);
-        if (FAILED (rc))
-            break;
+        CheckComRCBreakRC (rc);
 
         do
@@ -3747 +3761 @@
             /* host data (USB filters) */
             rc = mData.mHost->saveSettings (global);
-            if (FAILED (rc))
-                break;
+            CheckComRCBreakRC (rc);
 
             rc = mData.mSystemProperties->saveSettings (global);
-            if (FAILED (rc))
-                break;
+            CheckComRCBreakRC (rc);
         }
         while (0);
@@ -3866 +3878 @@
  *  saves global settings.
  *
+ *  @note The caller must have added itself as a caller of the @a aMachine
+ *  object if calls this method not on VirtualBox startup.
+ *
  *  @param aMachine     machine to register
  *
@@ -3879 +3894 @@
     AutoLock alock (this);
 
-    ComAssertRet (findMachine (aMachine->data()->mUuid,
+    ComAssertRet (findMachine (aMachine->uuid(),
                   false /* aDoSetError  */, NULL) == E_INVALIDARG,
                   E_FAIL);

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

@@ -60 +60 @@
 class Snapshot;
 class SharedFolder;
+class HostUSBDevice;
 
 class SessionMachine;
@@ -510 +511 @@
     ComObjPtr <SessionMachine> sessionMachine();
 
-    // Note: the below methods are intended to be called only after adding
-    // a caller to the Machine instance and, when necessary, from under
-    // the Machine lock in appropriate mode
-
-    /// @todo (dmik) revise code using these methods: improving incapsulation
-    //  should make them not necessary
-
-    const ComObjPtr <VirtualBox, ComWeakRef> &virtualBox() { return mParent; }
-
-    const Shareable <Data> &data() const { return mData; }
-    const Backupable <UserData> &userData() const { return mUserData; }
-    const Backupable <HDData> &hdData() const { return mHDData; }
-
-    const Shareable <SSData> &ssData() const { return mSSData; }
-
-    const ComObjPtr <DVDDrive> &dvdDrive() { return mDVDDrive; }
-    const ComObjPtr <FloppyDrive> &floppyDrive() { return mFloppyDrive; }
-    const ComObjPtr <USBController> &usbController() { return mUSBController; }
-
+    /** 
+     *  Returns the VirtualBox object this machine belongs to.
+     *
+     *  @note This method doesn't check this object's readiness as it is
+     *  intended to be used only by Machine children where it is guaranteed
+     *  that this object still exists in memory.
+     */
+    const ComObjPtr <VirtualBox, ComWeakRef> &virtualBox() const { return mParent; }
+
+    /** 
+     *  Returns this machine's name.
+     *
+     *  @note This method doesn't check this object's readiness as it is
+     *  intended to be used only after adding a caller to this object (that
+     *  guarantees that the object is ready or at least limited).
+     */
+    const Guid &uuid() const { return mData->mUuid; }
+
+    /** 
+     *  Returns this machine's name.
+     *
+     *  @note This method doesn't lock this object or check its readiness as
+     *  it is intended to be used only after adding a caller to this object
+     *  (that guarantees that the object is ready) and locking it for reading.
+     */
+    const Bstr &name() const { return mUserData->mName; }
+
+    // callback handlers
     virtual HRESULT onDVDDriveChange() { return S_OK; }
     virtual HRESULT onFloppyDriveChange() { return S_OK; }
@@ -537 +547 @@
     virtual HRESULT onUSBControllerChange() { return S_OK; }
     virtual HRESULT onSharedFolderChange() { return S_OK; }
+
+    HRESULT saveRegistryEntry (CFGNODE aEntryNode);
 
     int calculateFullPath (const char *aPath, Utf8Str &aResult);
@@ -794 +806 @@
     HRESULT onSharedFolderChange();
 
+    bool hasMatchingUSBFilter (const ComObjPtr <HostUSBDevice> &aDevice);
+
 private:
 
@@ -909 +923 @@
 
     Guid mSnapshotId;
+
+    friend class Snapshot;
 };