Changeset 2805 in vbox for trunk/src/VBox

Timestamp:

May 23, 2007 1:45:31 PM (18 years ago)

Author:

vboxsync

Message:

Main: Fixed: Machine-level USB filters are now inserted to the USB proxy only upon VM startup and removed upon termination (defect #1975).

Location:

trunk/src/VBox/Main

Files:

• ConsoleImpl.cpp (modified) (3 diffs)
• HostImpl.cpp (modified) (8 diffs)
• USBControllerImpl.cpp (modified) (17 diffs)
• include/USBControllerImpl.h (modified) (1 diff)

trunk/src/VBox/Main/ConsoleImpl.cpp

@@ -3799 +3799 @@
         }
 
-        releaseAllUSBDevices();
+        /* If the machine has an USB comtroller, release all USB devices
+         * (symmectric to the code in captureUSBDevices()) */
+        {
+            PPDMIBASE pBase;
+            int vrc = PDMR3QueryLun (mpVM, "usb-ohci", 0, 0, &pBase);
+            if (VBOX_SUCCESS (vrc))
+                releaseAllUSBDevices();
+        }
 
         /*
@@ -4288 +4295 @@
 
 /**
- *  USB device attack callback used by AttachUSBDevice().
+ *  USB device attach callback used by AttachUSBDevice().
  *  Note that AttachUSBDevice() doesn't return until this callback is executed,
  *  so we don't use AutoCaller and don't care about reference counters of
@@ -4349 +4356 @@
 
 /**
- *  USB device attack callback used by AttachUSBDevice().
- *  Note that AttachUSBDevice() doesn't return until this callback is executed,
+ *  USB device detach callback used by DetachUSBDevice().
+ *  Note that DetachUSBDevice() doesn't return until this callback is executed,
  *  so we don't use AutoCaller and don't care about reference counters of
  *  interface pointers passed in.

trunk/src/VBox/Main/HostImpl.cpp

@@ -1294 +1294 @@
  *  of IUSBDevice instances.
  *
- *  Called by Console from the VM process (through IInternalMachineControl).
+ *  Called by Console from the VM process (through IInternalMachineControl)
+ *  upon VM startup.
+ *
+ *  @note Locks this object for reading (@todo for writing now, until switched
+ *  to the new locking scheme).
  */
 HRESULT Host::autoCaptureUSBDevices (SessionMachine *aMachine,
                                      IUSBDeviceCollection **aHostDevices)
 {
-    LogFlowMember (("Host::autoCaptureUSBDevices: aMachine=%p\n", aMachine));
+    LogFlowThisFunc (("aMachine=%p\n", aMachine));
 
     AutoLock lock (this);
@@ -1321 +1325 @@
             if (device->state() == USBDeviceState_USBDeviceCaptured)
             {
-                // put the device to the return list
+                /* put the device to the return list */
                 ComPtr <IUSBDevice> d;
                 device.queryInterfaceTo (d.asOutParam());
@@ -1343 +1347 @@
  *  by the given machine (excluding this machine's filters).
  *
- *  Called by Console from the VM process (throug IInternalMachineControl).
+ *  Called by Console from the VM process (throug IInternalMachineControl)
+ *  upon normal VM termination or by SessionMachine::uninit() upon abnormal
+ *  VM termination (from under the Machine/SessionMachine lock).
+ *
+ *  @note Locks this object for reading (@todo for writing now, until switched
+ *  to the new locking scheme).
  */
 HRESULT Host::releaseAllUSBDevices (SessionMachine *aMachine)
@@ -1356 +1365 @@
         if (device->machine() == aMachine)
         {
-            // reset the device and apply filters
+            /* reset the device and apply filters */
             device->reset();
             HRESULT rc = applyAllUSBFilters (device, aMachine);
@@ -1511 +1520 @@
     LogFlowMember (("Host::applyAllUSBFilters: \n"));
 
-    // ignore unsupported devices
+    /* ignore unsupported devices */
     if (aDevice->state() == USBDeviceState_USBDeviceNotSupported)
         return S_OK;
-    // ignore unavailable devices as well
+    /* ignore unavailable devices as well */
     if (aDevice->state() == USBDeviceState_USBDeviceUnavailable)
         return S_OK;
@@ -1521 +1530 @@
     mParent->getOpenedMachines (machines);
 
-    // apply global filters
+    /* apply global filters */
     USBDeviceFilterList::const_iterator it = mUSBDeviceFilters.begin();
     while (it != mUSBDeviceFilters.end())
@@ -1537 +1546 @@
     }
 
-    // apply machine filters
+    /* apply machine filters */
     for (size_t i = 0; i < machines.size(); i++)
     {
@@ -1559 +1568 @@
     }
 
-    // no machine filters were matched.
-    // if no global filters were matched as well, release the device
-    // to make it available on the host
+    /* no machine filters were matched.
+     * if no global filters were matched as well, release the device
+     * to make it available on the host */
     if (    it == mUSBDeviceFilters.end()
         &&  aDevice->state() == USBDeviceState_USBDeviceHeld)

trunk/src/VBox/Main/USBControllerImpl.cpp

@@ -295 +295 @@
     CHECK_READY();
 
-    CHECK_MACHINE_MUTABILITY (m_Parent);
+    /* the machine needs to be mutable */
+    Machine::AutoStateDependency <Machine::MutableStateDep> adep (m_Parent);
+    CheckComRCReturnRC (adep.rc());
 
     ComObjPtr <USBDeviceFilter> filter = getDependentChild (aFilter);
@@ -307 +309 @@
             tr ("The given USB device filter is already in the list"));
 
-    // backup the list before modification
+    /* backup the list before modification */
     m_DeviceFilters.backup();
 
-    // iterate to the position...
+    /* iterate to the position... */
     DeviceFilterList::iterator it;
     if (aPosition < m_DeviceFilters->size())
@@ -319 +321 @@
     else
         it = m_DeviceFilters->end();
-    // ...and insert
+    /* ...and insert */
     m_DeviceFilters->insert (it, filter);
     filter->mInList = true;
 
-    // notify the proxy (only when the filter is active)
+    /// @todo After rewriting Win32 USB support, no more necessary;
+    //  a candidate for removal.
+#if 0
+    /* notify the proxy (only when the filter is active) */
     if (filter->data().mActive)
+#else
+    /* notify the proxy (only when it makes sense) */
+    if (filter->data().mActive && adep.machineState() >= MachineState_Running)
+#endif
     {
         USBProxyService *service = m_Parent->virtualBox()->host()->usbProxyService();
@@ -345 +354 @@
     CHECK_READY();
 
-    CHECK_MACHINE_MUTABILITY (m_Parent);
+    /* the machine needs to be mutable */
+    Machine::AutoStateDependency <Machine::MutableStateDep> adep (m_Parent);
+    CheckComRCReturnRC (adep.rc());
 
     if (!m_DeviceFilters->size())
@@ -356 +367 @@
             aPosition, m_DeviceFilters->size() - 1);
 
-    // backup the list before modification
+    /* backup the list before modification */
     m_DeviceFilters.backup();
 
     ComObjPtr <USBDeviceFilter> filter;
     {
-        // iterate to the position...
+        /* iterate to the position... */
         DeviceFilterList::iterator it = m_DeviceFilters->begin();
         std::advance (it, aPosition);
-        // ...get an element from there...
+        /* ...get an element from there... */
         filter = *it;
-        // ...and remove
+        /* ...and remove */
         filter->mInList = false;
         m_DeviceFilters->erase (it);
     }
 
-    // cancel sharing (make an independent copy of data)
+    /* cancel sharing (make an independent copy of data) */
     filter->unshare();
 
     filter.queryInterfaceTo (aFilter);
 
-    // notify the proxy (only when the filter is active)
+    /// @todo After rewriting Win32 USB support, no more necessary;
+    //  a candidate for removal.
+#if 0
+    /* notify the proxy (only when the filter is active) */
     if (filter->data().mActive)
+#else
+    /* notify the proxy (only when it makes sense) */
+    if (filter->data().mActive && adep.machineState() >= MachineState_Running)
+#endif
     {
         USBProxyService *service = m_Parent->virtualBox()->host()->usbProxyService();
@@ -607 +625 @@
 bool USBController::rollback()
 {
-    AutoLock alock(this);
+    AutoLock alock (this);
+
+    /* we need the machine state */
+    Machine::AutoStateDependency <Machine::MutableStateDep> adep (m_Parent);
+    AssertComRCReturn (adep.rc(), false);
 
     bool dataChanged = false;
@@ -613 +635 @@
     if (m_Data.isBackedUp())
     {
-        // we need to check all data to see whether anything will be changed
-        // after rollback
+        /* we need to check all data to see whether anything will be changed
+         * after rollback */
         dataChanged = m_Data.hasActualChanges();
         m_Data.rollback();
@@ -624 +646 @@
         ComAssertRet (service, false);
 
-        // uninitialize all new filters (absent in the backed up list)
+        /* uninitialize all new filters (absent in the backed up list) */
         DeviceFilterList::const_iterator it = m_DeviceFilters->begin();
         DeviceFilterList *backedList = m_DeviceFilters.backedUpData();
@@ -632 +654 @@
                 backedList->end())
             {
-                // notify the proxy (only when the filter is active)
+    /// @todo After rewriting Win32 USB support, no more necessary;
+    //  a candidate for removal.
+#if 0
+                /* notify the proxy (only when the filter is active) */
                 if ((*it)->data().mActive)
+#else
+                /* notify the proxy (only when it makes sense) */
+                if ((*it)->data().mActive &&
+                    adep.machineState() >= MachineState_Running)
+#endif
                 {
                     USBDeviceFilter *filter = *it;
@@ -646 +676 @@
         }
 
-        // find all removed old filters (absent in the new list)
-        // and insert them back to the USB proxy
-        it = backedList->begin();
-        while (it != backedList->end())
-        {
-            if (std::find (m_DeviceFilters->begin(), m_DeviceFilters->end(), *it) ==
-                m_DeviceFilters->end())
+    /// @todo After rewriting Win32 USB support, no more necessary;
+    //  a candidate for removal.
+#if 0
+#else
+        if (adep.machineState() >= MachineState_Running)
+#endif
+        {
+            /* find all removed old filters (absent in the new list)
+             * and insert them back to the USB proxy */
+            it = backedList->begin();
+            while (it != backedList->end())
             {
-                // notify the proxy (only when the filter is active)
-                if ((*it)->data().mActive)
+                if (std::find (m_DeviceFilters->begin(), m_DeviceFilters->end(), *it) ==
+                    m_DeviceFilters->end())
                 {
-                    USBDeviceFilter *flt = *it; // resolve ambiguity
-                    ComAssertRet (flt->id() == NULL, false);
-                    flt->id() = service->insertFilter (ComPtr <IUSBDeviceFilter> (flt));
+                    /* notify the proxy (only when necessary) */
+                    if ((*it)->data().mActive)
+                    {
+                        USBDeviceFilter *flt = *it; /* resolve ambiguity */
+                        ComAssertRet (flt->id() == NULL, false);
+                        flt->id() = service->insertFilter
+                            (ComPtr <IUSBDeviceFilter> (flt));
+                    }
                 }
+                ++ it;
             }
-            ++ it;
-        }
-
-        // restore the list
+        }
+
+        /* restore the list */
         m_DeviceFilters.rollback();
     }
 
-    // rollback any changes to filters after restoring the list
+    /* here we don't depend on the machine state any more */
+    adep.release();
+
+    /* rollback any changes to filters after restoring the list */
     DeviceFilterList::const_iterator it = m_DeviceFilters->begin();
     while (it != m_DeviceFilters->end())
@@ -676 +718 @@
         {
             (*it)->rollback();
-            // call this to notify the USB proxy about changes
+            /* call this to notify the USB proxy about changes */
             onDeviceFilterChange (*it);
         }
@@ -821 +863 @@
     CHECK_READY();
 
-    USBProxyService *service = m_Parent->virtualBox()->host()->usbProxyService();
-    ComAssertRet (service, E_FAIL);
-
-    // iterate over the filter list and notify the proxy accordingly
-
-    DeviceFilterList::const_iterator it = m_DeviceFilters->begin();
-    while (it != m_DeviceFilters->end())
-    {
-        USBDeviceFilter *flt = *it; // resolve ambiguity (for ComPtr below)
-
-        // notify the proxy (only if the filter is active)
-        if (flt->data().mActive)
-        {
-            if (aRegistered)
-            {
-                ComAssertRet (flt->id() == NULL, E_FAIL);
-                flt->id() = service->insertFilter (ComPtr <IUSBDeviceFilter> (flt));
-            }
-            else
-            {
-                ComAssertRet (flt->id() != NULL, E_FAIL);
-                service->removeFilter (flt->id());
-                flt->id() = NULL;
-            }
-        }
-        ++ it;
-    }
+    /// @todo After rewriting Win32 USB support, no more necessary;
+    //  a candidate for removal.
+#if 0
+    notifyProxy (!!aRegistered);
+#endif
 
     return S_OK;
@@ -861 +881 @@
     CHECK_READY();
 
+    /// @todo After rewriting Win32 USB support, no more necessary;
+    //  a candidate for removal.
+#if 0
+#else
+    /* we need the machine state */
+    Machine::AutoStateDependency <Machine::MutableStateDep> adep (m_Parent);
+    AssertComRCReturnRC (adep.rc());
+
+    /* nothing to do if the machine isn't running */
+    if (adep.machineState() < MachineState_Running)
+        return S_OK;
+#endif
+
     if (aFilter->mInList && m_Parent->isRegistered())
     {
@@ -868 +901 @@
         if (aActiveChanged)
         {
-            // insert/remove the filter from the proxy
+            /* insert/remove the filter from the proxy */
             if (aFilter->data().mActive)
             {
                 ComAssertRet (aFilter->id() == NULL, E_FAIL);
-                aFilter->id() =
-                    service->insertFilter (ComPtr <IUSBDeviceFilter> (aFilter));
+                aFilter->id() = service->insertFilter
+                    (ComPtr <IUSBDeviceFilter> (aFilter));
             }
             else
@@ -886 +919 @@
             if (aFilter->data().mActive)
             {
-                // update the filter in the proxy
+                /* update the filter in the proxy */
                 ComAssertRet (aFilter->id() != NULL, E_FAIL);
                 service->removeFilter (aFilter->id());
-                aFilter->id() =
-                    service->insertFilter (ComPtr <IUSBDeviceFilter> (aFilter));
+                aFilter->id() = service->insertFilter
+                    (ComPtr <IUSBDeviceFilter> (aFilter));
             }
         }
@@ -936 +969 @@
 bool USBController::hasMatchingFilter (IUSBDevice *aUSBDevice)
 {
-    LogFlowMember (("USBController::hasMatchingFilter()\n"));
+    LogFlowThisFunc (("\n"));
 
     AutoLock alock (this);
@@ -1027 +1060 @@
 }
 
+HRESULT USBController::notifyProxy (bool aInsertFilters)
+{
+    LogFlowThisFunc (("aInsertFilters=%RTbool\n", aInsertFilters));
+
+    AutoLock alock (this);
+    if (!isReady())
+        return false;
+
+    USBProxyService *service = m_Parent->virtualBox()->host()->usbProxyService();
+    AssertReturn (service, E_FAIL);
+
+    DeviceFilterList::const_iterator it = m_DeviceFilters->begin();
+    while (it != m_DeviceFilters->end())
+    {
+        USBDeviceFilter *flt = *it; /* resolve ambiguity (for ComPtr below) */
+
+        /* notify the proxy (only if the filter is active) */
+        if (flt->data().mActive)
+        {
+            if (aInsertFilters)
+            {
+                AssertReturn (flt->id() == NULL, E_FAIL);
+                flt->id() = service->insertFilter
+                    (ComPtr <IUSBDeviceFilter> (flt));
+            }
+            else
+            {
+                AssertReturn (flt->id() != NULL, E_FAIL);
+                service->removeFilter (flt->id());
+                flt->id() = NULL;
+            }
+        }
+        ++ it;
+    }
+
+    return S_OK; 
+}
+
 // private methods
 /////////////////////////////////////////////////////////////////////////////

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

@@ -111 +111 @@
     bool hasMatchingFilter (IUSBDevice *aUSBDevice);
 
+    HRESULT notifyProxy (bool aInsertFilters);
+
     // for VirtualBoxSupportErrorInfoImpl
     static const wchar_t *getComponentName() { return L"USBController"; }