Changeset 3034 in vbox

Timestamp:

Jun 4, 2007 3:51:26 PM (18 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

21789

Message:

Main: Fixed async USB (added proper detection of device re-cycle on Win32; changed manual release sequence so that a detach is made in VMM first, the USB proxy is notified afterwards on success; some other small fixes).

Location:

trunk/src/VBox/Main

Files:

• ConsoleImpl.cpp (modified) (7 diffs)
• HostImpl.cpp (modified) (5 diffs)
• HostUSBDeviceImpl.cpp (modified) (16 diffs)
• MachineImpl.cpp (modified) (5 diffs)
• USBProxyService.cpp (modified) (4 diffs)
• include/ConsoleImpl.h (modified) (1 diff)
• include/HostImpl.h (modified) (1 diff)
• include/HostUSBDeviceImpl.h (modified) (3 diffs)

trunk/src/VBox/Main/ConsoleImpl.cpp

@@ -1947 +1947 @@
             tr ("The virtual machine does not have a USB controller"));
 
+    /* leave the lock because the USB Proxy service may call us back
+     * (via onUSBDeviceAttach()) */
+    alock.leave();
+
     /* Request the device capture */
     HRESULT rc = mControl->CaptureUSBDevice (aId);
@@ -1982 +1986 @@
             Guid (aId).raw());
 
-    /* Request the device release */
-    HRESULT rc = mControl->ReleaseUSBDevice (aId);
-    CheckComRCReturnRC (rc);
+    /* First, request VMM to detach the device */
+    HRESULT rc = detachUSBDevice (it);
+
+    if (SUCCEEDED (rc))
+    {
+        /* leave the lock since we don't need it any more (note though that
+         * the USB Proxy service must not call us back here) */
+        alock.leave();
+
+        /* Request the device release. Even if it fails, the device will
+         * remain as held by proxy, which is OK for us (the VM process). */
+        rc = mControl->ReleaseUSBDevice (aId);
+    }
 
     return rc;
@@ -3217 +3231 @@
     ComAssertRet (pRhConfig, E_FAIL);
 
+    HRESULT rc = attachUSBDevice (aDevice, pRhConfig);
+
     /// @todo notify listeners of IConsoleCallback in case of error
-    return attachUSBDevice (aDevice, pRhConfig);
+    return rc;
 }
 
@@ -3273 +3289 @@
     }
 
-    /* protect mpVM */
-    AutoVMCaller autoVMCaller (this);
-    CheckComRCReturnRC (autoVMCaller.rc());
-
-    PPDMIBASE pBase = NULL;
-    int vrc = PDMR3QueryLun (mpVM, "usb-ohci", 0, 0, &pBase);
-
-    /* if the device is attached, then there must be a USB controller */
-    AssertRCReturn (vrc, E_FAIL);
-
-    PVUSBIRHCONFIG pRhConfig = (PVUSBIRHCONFIG) pBase->
-        pfnQueryInterface (pBase, PDMINTERFACE_VUSB_RH_CONFIG);
-    AssertReturn (pRhConfig, E_FAIL);
-
-    LogFlowThisFunc (("Detaching USB proxy device {%Vuuid}...\n", Uuid.raw()));
-
-    /* leave the lock before a VMR3* call (EMT will call us back)! */
-    alock.leave();
-
-    PVMREQ pReq;
-    vrc = VMR3ReqCall (mpVM, &pReq, RT_INDEFINITE_WAIT,
-                       (PFNRT) usbDetachCallback, 5,
-                       this, &it, pRhConfig, Uuid.raw());
-    if (VBOX_SUCCESS (vrc))
-        vrc = pReq->iStatus;
-    VMR3ReqFree (pReq);
-
-    AssertRC (vrc);
+    HRESULT rc = detachUSBDevice (it);
 
     /// @todo notify listeners of IConsoleCallback in case of error
-    return VBOX_SUCCESS (vrc) ? S_OK : E_FAIL;
+    return rc;
 }
 
@@ -4178 +4167 @@
  *  @param aHostDevice  device to attach
  *
- *  @note Locks this object for writing.
  *  @note Synchronously calls EMT.
+ *  @note Must be called from under this object's lock.
  */
 HRESULT Console::attachUSBDevice (IUSBDevice *aHostDevice, PVUSBIRHCONFIG aConfig)
@@ -4185 +4174 @@
     AssertReturn (aHostDevice && aConfig, E_FAIL);
 
+    AssertReturn (isLockedOnCurrentThread(), E_FAIL);
+
+    /* still want a lock object because we need to leave it */
     AutoLock alock (this);
 
@@ -4328 +4320 @@
     LogFlowFuncLeave();
     return vrc;
+}
+
+/**
+ *  Sends a request to VMM to detach the given host device.  After this method
+ *  succeeds, the detached device will disappear from the mUSBDevices
+ *  collection.
+ *
+ *  @param aIt  Iterator pointing to the device to detach.
+ *
+ *  @note Synchronously calls EMT.
+ *  @note Must be called from under this object's lock.
+ */
+HRESULT Console::detachUSBDevice (USBDeviceList::iterator &aIt)
+{
+    AssertReturn (isLockedOnCurrentThread(), E_FAIL);
+
+    /* still want a lock object because we need to leave it */
+    AutoLock alock (this);
+
+    /* protect mpVM */
+    AutoVMCaller autoVMCaller (this);
+    CheckComRCReturnRC (autoVMCaller.rc());
+
+    PPDMIBASE pBase = NULL;
+    int vrc = PDMR3QueryLun (mpVM, "usb-ohci", 0, 0, &pBase);
+
+    /* if the device is attached, then there must be a USB controller */
+    AssertRCReturn (vrc, E_FAIL);
+
+    PVUSBIRHCONFIG pRhConfig = (PVUSBIRHCONFIG) pBase->
+        pfnQueryInterface (pBase, PDMINTERFACE_VUSB_RH_CONFIG);
+    AssertReturn (pRhConfig, E_FAIL);
+
+    LogFlowThisFunc (("Detaching USB proxy device {%Vuuid}...\n",
+                      (*aIt)->id().raw()));
+
+    /* leave the lock before a VMR3* call (EMT will call us back)! */
+    alock.leave();
+
+    PVMREQ pReq;
+    vrc = VMR3ReqCall (mpVM, &pReq, RT_INDEFINITE_WAIT,
+                       (PFNRT) usbDetachCallback, 5,
+                       this, &aIt, pRhConfig, (*aIt)->id().raw());
+    if (VBOX_SUCCESS (vrc))
+        vrc = pReq->iStatus;
+    VMR3ReqFree (pReq);
+
+    ComAssertRCRet (vrc, E_FAIL);
+
+    return S_OK;
 }
 

trunk/src/VBox/Main/HostImpl.cpp

@@ -1315 +1315 @@
     /* re-apply filters on the device before giving it back to the host */
     device->setHeld();
-    HRESULT rc = applyAllUSBFilters (device);
+    HRESULT rc = applyAllUSBFilters (device, aMachine);
     ComAssertComRC (rc);
 
@@ -1397 +1397 @@
                  * host */
                 device->setHeld();
-                HRESULT rc = applyAllUSBFilters (device);
+                HRESULT rc = applyAllUSBFilters (device, aMachine);
                 AssertComRC (rc);
             }
@@ -1845 +1845 @@
  *
  *  @param aDevice  USB device to apply filters to.
+ *  @param aMachine Machine the device was released by or @c NULL.
  *
  *  @note the method must be called from under this object's write lock and
  *  from the aDevice's write lock.
  */
-HRESULT Host::applyAllUSBFilters (ComObjPtr <HostUSBDevice> &aDevice)
+HRESULT Host::applyAllUSBFilters (ComObjPtr <HostUSBDevice> &aDevice,
+                                  SessionMachine *aMachine /* = NULL */)
 {
     LogFlowThisFunc (("\n"));
@@ -1901 +1903 @@
     {
         /* skip the machine the device was just detached from */
-        if (aDevice->machine() && machines [i] == aDevice->machine())
+        if (aMachine && machines [i] == aMachine)
             continue;
 
@@ -2051 +2053 @@
     mUSBDevices.erase (it);
 
-    /* reset all data */
+    /* reset all data and uninitialize the device object */
     device->reset();
 }

trunk/src/VBox/Main/HostUSBDeviceImpl.cpp

@@ -332 +332 @@
         name = Utf8StrFmt ("%s %s", mUsb->pszManufacturer,
                                      mUsb->pszProduct);
-    else if(haveManufacturer)
+    else if (haveManufacturer)
         name = Utf8StrFmt ("%s", mUsb->pszManufacturer);
-    else if(haveProduct)
-        name = Utf8StrFmt ("%s", mUsb->pszManufacturer);
+    else if (haveProduct)
+        name = Utf8StrFmt ("%s", mUsb->pszProduct);
     else
         name = "<unknown>";
@@ -389 +389 @@
 
         HRESULT rc = aMachine->onUSBDeviceAttach (d, NULL);
+
+        /* The VM process has a legal reason to fail (for example, incorrect
+         * usbfs permissions or out of virtual USB ports). More over, the VM
+         * process might have been accidentially crashed and not accessible
+         * any more (so that calling an uninitialized SessionMachine returns a
+         * failure). So don't assert. */
 
         LogFlowThisFunc (("Done machine->onUSBDeviceAttach()=%08X\n", rc));
@@ -481 +487 @@
 
 /**
- *  Sets the device state from Captured to Held and preserves the machine
+ *  Sets the device state from Captured to Held and resets the machine
  *  association (if any). Usually called before applying filters.
  *
@@ -497 +503 @@
 
     mState = USBDeviceState_USBDeviceHeld;
+    mMachine.setNull();
 }
 
@@ -525 +532 @@
 
         HRESULT rc = mMachine->onUSBDeviceDetach (mId, NULL);
-        AssertComRC (rc);
+
+        /* This call may expectedly fail with rc = NS_ERROR_FAILURE (on XPCOM)
+         * if the VM process requests device release right before termination
+         * and then terminates before onUSBDeviceDetach() reached
+         * it. Therefore, we don't assert here. On MS COM, there should be
+         * something similar (with the different error code). More over, the
+         * VM process might have been accidentially crashed and not accessible
+         * any more (so that calling an uninitialized SessionMachine returns a
+         * failure). So don't assert. */
 
         LogFlowThisFunc (("Done machine->onUSBDeviceDetach()=%08X\n", rc));
@@ -531 +546 @@
         alock.enter();
 
+        /* Reset all fields. Tthe object should have been
+         * uninitialized after this method returns, so it doesn't really
+         * matter what state we put it in. */
         mIsStatePending = false;
         mState = mPendingState = USBDeviceState_USBDeviceNotSupported;
+        mMachine.setNull();
     }
 }
@@ -552 +571 @@
 
     bool wasCapture = false;
-    bool wasRelease = false;
 
     HRESULT requestRC = S_OK;
@@ -594 +612 @@
         case USBDeviceState_USBDeviceAvailable:
         {
+            Assert (mMachine.isNull());
+
             if (mState == USBDeviceState_USBDeviceHeld)
             {
-                /* couldn't release the device */
-                wasRelease = true;
-
-                Assert (!mMachine.isNull());
-
-                /* return the captured state back */
-                mState = USBDeviceState_USBDeviceCaptured;
-
-                /// @todo more detailed error message depending on the state?
-                //  probably need some error code/string from the USB proxy itself
-
-                requestRC = E_FAIL;
-                errorText = Utf8StrFmt (
-                    tr ("USB device '%s' with UUID {%Vuuid} is being accessed by the guest "
-                        "computer and cannot be attached from the virtual machine."
-                        "Please try later"),
-                    name().raw(), id().raw());
+                /* couldn't release the device (give it back to the host).
+                 * there is nobody to report an error to (the machine has
+                 * already been deassociated because VMM has already detached
+                 * the device before requesting a release). */
             }
             else
             {
-                if (!mMachine.isNull())
-                    wasRelease = true;
-                else
-                {
-                    /* it is a canceled release request. Leave at the host */
-                    /// @todo we may want to re-run all filters in this case
-                }
+                /* it is a canceled release request. Leave at the host */
+                /// @todo we may want to re-run all filters in this case
             }
             break;
@@ -630 +632 @@
             if (mState == USBDeviceState_USBDeviceHeld)
             {
+                /* All right, the device is now held (due to some global
+                 * filter). */
                 break;
             }
@@ -670 +674 @@
 
         /* The VM process has a legal reason to fail (for example, incorrect
-         * usbfs permissions or out of virtual USB ports), so don't assert */
+         * usbfs permissions or out of virtual USB ports). More over, the VM
+         * process might have been accidentially crashed and not accessible
+         * any more (so that calling an uninitialized SessionMachine returns a
+         * failure). So don't assert. */
 
         /// @todo we will probably want to re-run all filters on failure
@@ -689 +696 @@
         mMachine.setNull();
     }
-    else if (wasRelease)
-    {
-        /* inform the VM process */
-
-        /* the VM process will query the object, so leave the lock */
-        AutoLock alock (this);
-        alock.leave();
-
-        LogFlowThisFunc (("Calling machine->onUSBDeviceDetach()...\n"));
-
-        HRESULT rc = mMachine->onUSBDeviceDetach (mId, error);
-
-        /* This call may expectedly fail with rc = NS_ERROR_FAILURE (on XPCOM)
-         * if the VM process requests device release right before termination
-         * and then terminates before onUSBDeviceDetach() is reached
-         * it. Therefore, we don't assert here. On MS COM, there should be
-         * something similar (with the different error code). */
-
-        LogFlowThisFunc (("Done machine->onUSBDeviceDetach()=%08X\n", rc));
-
-        alock.enter();
-
-        if (SUCCEEDED (requestRC))
-        {
-            /* deassociate from the machine */
-            mMachine.setNull();
-        }
-    }
 
     mIsStatePending = false;
@@ -739 +718 @@
     {
         case USBDeviceState_USBDeviceCaptured:
-        {
-            /* reset mMachine to deassociate it from the filter and tell
-             * handlePendingStateChange() what to do */
-            mMachine.setNull();
-            break;
-        }
-        case USBDeviceState_USBDeviceAvailable:
         {
             /* reset mMachine to deassociate it from the filter and tell
@@ -859 +831 @@
  *  Compares this device with a USBDEVICE and decides which comes first.
  *
- *  @return < 0 if this should come before pDev2.
- *  @return   0 if this and pDev2 are equal.
- *  @return > 0 if this should come after pDev2.
- *
- *  @param   pDev2   Device 2.
+ *  If the device has a pending state request, a non-strict comparison is made
+ *  (to properly detect a re-attached device). Otherwise, a strict comparison
+ *  is performed.
+ *
+ *  @param   aDev2   Device 2.
+ *
+ *  @return < 0 if this should come before aDev2.
+ *  @return   0 if this and aDev2 are equal.
+ *  @return > 0 if this should come after aDev2.
  *
  *  @note Must be called from under the object write lock.
  */
-int HostUSBDevice::compare (PCUSBDEVICE pDev2)
+int HostUSBDevice::compare (PCUSBDEVICE aDev2)
 {
     AssertReturn (isLockedOnCurrentThread(), -1);
 
-    return compare (mUsb, pDev2);
-}
-
+    return compare (mUsb, aDev2, !isStatePending());
+}
 
 /**
  *  Compares two USB devices and decides which comes first.
- *
- *  @return < 0 if pDev1 should come before pDev2.
- *  @return   0 if pDev1 and pDev2 are equal.
- *  @return > 0 if pDev1 should come after pDev2.
- *
- *  @param   pDev1   Device 1.
- *  @param   pDev2   Device 2.
- */
-/*static*/ int HostUSBDevice::compare (PCUSBDEVICE pDev1, PCUSBDEVICE pDev2)
-{
-    int iDiff = pDev1->idVendor - pDev2->idVendor;
+ * 
+ *  If @a aIsStrict is @c true then the comparison will indicate a difference
+ *  even if the same physical device (represented by @a aDev1) has been just
+ *  re-attached to the host computer (represented by @a aDev2) and got a
+ *  different address from the host OS, etc.
+ *
+ *  If @a aIsStrict is @c false, then such a re-attached device will be
+ *  considered equal to the previous device definition and this function will
+ *  retrun 0.
+ *
+ *  @param   aDev1      Device 1.
+ *  @param   aDev2      Device 2.
+ *  @param   aIsStrict  @c true to do a strict check and @c false otherwise.
+
+ *  @return < 0 if aDev1 should come before aDev2.
+ *  @return   0 if aDev1 and aDev2 are equal.
+ *  @return > 0 if aDev1 should come after aDev2.
+ */
+/*static*/
+int HostUSBDevice::compare (PCUSBDEVICE aDev1, PCUSBDEVICE aDev2,
+                            bool aIsStrict /* = true */)
+{
+    int iDiff = aDev1->idVendor - aDev2->idVendor;
     if (iDiff)
         return iDiff;
 
-    iDiff = pDev1->idProduct - pDev2->idProduct;
+    iDiff = aDev1->idProduct - aDev2->idProduct;
     if (iDiff)
         return iDiff;
 
-    /** @todo Sander, will this work on windows as well? Linux won't reuse an address for quite a while. */
-    return strcmp(pDev1->pszAddress, pDev2->pszAddress);
+    if (!aIsStrict)
+        return 0;
+
+    /* The rest is considered as a strict check. */
+
+    return strcmp (aDev1->pszAddress, aDev2->pszAddress);
 }
 
@@ -956 +947 @@
                 case USBDeviceState_USBDeviceAvailable:
                     isImportant = false;
+                    break;
                 default:
                     isImportant = true;
@@ -973 +965 @@
                 case USBDeviceState_USBDeviceAvailable:
                     isImportant = false;
+                    break;
                 default:
                     isImportant = true;
@@ -997 +990 @@
                 case USBDeviceState_USBDeviceBusy:
                     isImportant = false;
+                    break;
                 default:
                     isImportant = true;

trunk/src/VBox/Main/MachineImpl.cpp

@@ -7715 +7715 @@
 
     AutoCaller autoCaller (this);
-    AssertComRCReturn (autoCaller.rc(), autoCaller.rc());
-
-    // if cautureUSBDevice() fails, it must have set extended error info
+    AssertComRCReturnRC (autoCaller.rc());
+
+    /* if cautureUSBDevice() fails, it must have set extended error info */
     return mParent->host()->captureUSBDevice (this, aId);
 }
@@ -8587 +8587 @@
 
     AutoCaller autoCaller (this);
-    AssertComRCReturn (autoCaller.rc(), autoCaller.rc());
+
+    /* This notification may happen after the machine object has been
+     * uninitialized (the session was closed), so don't assert. */
+    CheckComRCReturnRC (autoCaller.rc());
 
     ComPtr <IInternalSessionControl> directControl;
@@ -8595 +8598 @@
     }
 
-    /* ignore notifications sent after #OnSessionEnd() is called */
+    /* fail on notifications sent after #OnSessionEnd() is called, it is
+     * expected by the caller */
     if (!directControl)
-        return S_OK;
+        return E_FAIL;
 
     return directControl->OnUSBDeviceAttach (aDevice, aError);
@@ -8611 +8615 @@
 
     AutoCaller autoCaller (this);
-    AssertComRCReturn (autoCaller.rc(), autoCaller.rc());
+
+    /* This notification may happen after the machine object has been
+     * uninitialized (the session was closed), so don't assert. */
+    CheckComRCReturnRC (autoCaller.rc());
 
     ComPtr <IInternalSessionControl> directControl;
@@ -8619 +8626 @@
     }
 
-    /* ignore notifications sent after #OnSessionEnd() is called */
+    /* fail on notifications sent after #OnSessionEnd() is called, it is
+     * expected by the caller */
     if (!directControl)
-        return S_OK;
+        return E_FAIL;
 
     return directControl->OnUSBDeviceDetach (aId, aError);

trunk/src/VBox/Main/USBProxyService.cpp

@@ -218 +218 @@
                 DevPtr = *It;
 
-            /// @todo we want to AddCaller here to make sure the device hasn't
-            //  been uninitialized (needs support for the no-op AddCaller when
-            //  its argument is NULL)
+            /* assert that the object is still alive (we still reference it in
+             * the collection and we're the only one who calls uninit() on it */
+            HostUSBDevice::AutoCaller devCaller (DevPtr.isNull() ? NULL : DevPtr);
+            AssertComRC (devCaller.rc());
 
             /* Lock the device object since we will read/write it's
@@ -290 +291 @@
                         It = mDevices.erase (It);
                         mHost->onUSBDeviceDetached (DevPtr);
-                        Log (("USBProxyService::processChanges: detached %p\n", (HostUSBDevice *)DevPtr)); /** @todo add details .*/
+                        Log (("USBProxyService::processChanges: detached %p\n",
+                              (HostUSBDevice *)DevPtr)); /** @todo add details .*/
+
+                        /* from now on, the object is no more valid,
+                         * uninitialize to avoid abuse */
+                        devCaller.release();
+                        DevPtr->uninit();
                     }
                     else
@@ -321 +328 @@
             ComObjPtr <HostUSBDevice> DevPtr = *It;
 
+            /* assert that the object is still alive (we still reference it in
+             * the collection and we're the only one who calls uninit() on it */
+            HostUSBDevice::AutoCaller devCaller (DevPtr);
+            AssertComRC (devCaller.rc());
+
             AutoLock devLock (DevPtr);
 
@@ -328 +340 @@
             It = mDevices.erase (It);
             mHost->onUSBDeviceDetached (DevPtr);
-            Log (("USBProxyService::processChanges: detached %p\n", (HostUSBDevice *)DevPtr)); /** @todo add details .*/
+            Log (("USBProxyService::processChanges: detached %p\n",
+                  (HostUSBDevice *)DevPtr)); /** @todo add details .*/
+
+            /* from now on, the object is no more valid,
+             * uninitialize to avoid abuse */
+            devCaller.release();
+            DevPtr->uninit();
         }
     }

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

@@ -372 +372 @@
 
     HRESULT attachUSBDevice (IUSBDevice *aHostDevice, PVUSBIRHCONFIG aConfig);
+    HRESULT detachUSBDevice (USBDeviceList::iterator &aIt);
 
     static DECLCALLBACK(int)

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

@@ -147 +147 @@
     }
 
-    HRESULT applyAllUSBFilters (ComObjPtr <HostUSBDevice> &aDevice);
+    HRESULT applyAllUSBFilters (ComObjPtr <HostUSBDevice> &aDevice,
+                                SessionMachine *aMachine = NULL);
 
     bool applyMachineUSBFilters (SessionMachine *aMachine,

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

@@ -95 +95 @@
 
     /* @note Must be called from under the object read lock. */
-    ComObjPtr <SessionMachine, ComWeakRef> &machine() { return mMachine; }
+    ComObjPtr <SessionMachine> &machine() { return mMachine; }
 
     /* @note Must be called from under the object read lock. */
@@ -117 +117 @@
     bool isMatch (const USBDeviceFilter::Data &aData);
 
-    int compare (PCUSBDEVICE pDev2);
-    static int compare (PCUSBDEVICE pDev1, PCUSBDEVICE pDev2);
+    int compare (PCUSBDEVICE aDev2);
+    static int compare (PCUSBDEVICE aDev1, PCUSBDEVICE aDev2,
+                        bool aIsStrict = true);
 
     bool updateState (PCUSBDEVICE aDev);
@@ -130 +131 @@
     USBDeviceState_T mState;
     USBDeviceState_T mPendingState;
-    ComObjPtr <SessionMachine, ComWeakRef> mMachine;
+    ComObjPtr <SessionMachine> mMachine;
     bool mIsStatePending : 1;