From 6160e479f6a5aad9cabf7d831ff63f4dd14e9704 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Tue, 25 Jun 2019 12:47:31 -0700 Subject: [PATCH] usb: switch to new libusb_wrap_sys_device API Replace use of our custom, patched libusb APIs with the new libusb_wrap_sys_device() function, which has been submitted to libusb upstream. This allows us to drop the bindings for the custom APIs (and will also allow us to drop the libusb patch that introduces them). For now, keep this path behind the sandboxed-libusb feature to allow crosvm to build against older libusb versions that do not have the new API. This should be cleaned up eventually once we are comfortable with raising the minimum libusb version required. BUG=b:133773289 TEST=Attach Android device to Linux VM; deploy app via adb Change-Id: Ie249c6f3f3b4c63210dd163ca7ad03e2de8a8872 Signed-off-by: Daniel Verkamp Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/1676601 Tested-by: kokoro --- devices/src/usb/host_backend/context.rs | 24 ++++---------- .../host_backend_device_provider.rs | 21 ++---------- usb_util/src/bindings.rs | 16 ++-------- usb_util/src/device_handle.rs | 13 ++++++++ usb_util/src/libusb_context.rs | 32 ++++++------------- usb_util/src/libusb_device.rs | 12 ------- 6 files changed, 34 insertions(+), 84 deletions(-) diff --git a/devices/src/usb/host_backend/context.rs b/devices/src/usb/host_backend/context.rs index 960d066096..76ad4a46f5 100644 --- a/devices/src/usb/host_backend/context.rs +++ b/devices/src/usb/host_backend/context.rs @@ -8,8 +8,11 @@ use std::os::raw::c_short; use std::os::unix::io::RawFd; use std::sync::{Arc, Weak}; use sys_util::{error, WatchingEvents}; +#[cfg(feature = "sandboxed-libusb")] +use usb_util::device_handle::DeviceHandle; use usb_util::hotplug::UsbHotplugHandler; use usb_util::libusb_context::{LibUsbContext, LibUsbPollfdChangeHandler}; +#[cfg(not(feature = "sandboxed-libusb"))] use usb_util::libusb_device::LibUsbDevice; use vm_control::MaybeOwnedFd; @@ -22,7 +25,6 @@ pub struct Context { impl Context { /// Create a new context. - #[cfg(not(feature = "sandboxed-libusb"))] pub fn new(event_loop: Arc) -> Result { let context = LibUsbContext::new().map_err(Error::CreateLibUsbContext)?; let ctx = Context { @@ -36,20 +38,6 @@ impl Context { Ok(ctx) } - #[cfg(feature = "sandboxed-libusb")] - pub fn new(event_loop: Arc) -> Result { - let context = LibUsbContext::new_jailed().map_err(Error::CreateLibUsbContext)?; - let ctx = Context { - context: context.clone(), - event_loop, - event_handler: Arc::new(LibUsbEventHandler { - context: context.clone(), - }), - }; - ctx.init_event_handler()?; - Ok(ctx) - } - pub fn set_hotplug_handler(&self, handler: H) { if let Err(e) = self.context.set_hotplug_cb(handler) { error!("cannot set hotplug handler: {:?}", e); @@ -100,9 +88,9 @@ impl Context { } #[cfg(feature = "sandboxed-libusb")] - pub fn get_device(&self, fd: std::fs::File) -> Option { - match self.context.get_device_from_fd(fd) { - Ok(dev) => Some(dev), + pub fn get_device_handle(&self, fd: std::fs::File) -> Option { + match self.context.handle_from_file(fd) { + Ok(handle) => Some(handle), Err(e) => { error!("could not build device from fd: {:?}", e); None diff --git a/devices/src/usb/host_backend/host_backend_device_provider.rs b/devices/src/usb/host_backend/host_backend_device_provider.rs index 01fd84c913..f479bcb134 100644 --- a/devices/src/usb/host_backend/host_backend_device_provider.rs +++ b/devices/src/usb/host_backend/host_backend_device_provider.rs @@ -209,9 +209,7 @@ impl ProviderInner { } }; - let device_fd = usb_file.as_raw_fd(); - - let device = match self.ctx.get_device(usb_file) { + let device_handle = match self.ctx.get_device_handle(usb_file) { Some(d) => d, None => { error!( @@ -228,22 +226,7 @@ impl ProviderInner { } }; - let device_handle = { - // This is safe only when fd is an fd of the current device. - match unsafe { device.open_fd(device_fd) } { - Ok(handle) => handle, - Err(e) => { - error!("fail to open device: {:?}", e); - // The send failure will be logged, but event loop still think - // the event is handled. - let _ = self - .sock - .send(&UsbControlResult::FailedToOpenDevice) - .map_err(Error::WriteControlSock); - return Ok(()); - } - } - }; + let device = device_handle.get_device(); // Resetting the device is used to make sure it is in a known state, but it may // still function if the reset fails. diff --git a/usb_util/src/bindings.rs b/usb_util/src/bindings.rs index a3e71526fa..8f62a011d0 100644 --- a/usb_util/src/bindings.rs +++ b/usb_util/src/bindings.rs @@ -4011,17 +4011,14 @@ pub type libusb_log_level = u32; extern "C" { pub fn libusb_init(ctx: *mut *mut libusb_context) -> ::std::os::raw::c_int; } -extern "C" { - pub fn libusb_init_jailed(ctx: *mut *mut libusb_context) -> ::std::os::raw::c_int; -} extern "C" { pub fn libusb_exit(ctx: *mut libusb_context); } extern "C" { - pub fn libusb_get_device_from_fd( + pub fn libusb_wrap_sys_device( ctx: *mut libusb_context, - fd: ::std::os::raw::c_int, - device: *mut *mut libusb_device, + fd: __intptr_t, + handle: *mut *mut libusb_device_handle, ) -> ::std::os::raw::c_int; } extern "C" { @@ -4198,13 +4195,6 @@ extern "C" { dev_handle: *mut *mut libusb_device_handle, ) -> ::std::os::raw::c_int; } -extern "C" { - pub fn libusb_open_fd( - dev: *mut libusb_device, - fd: ::std::os::raw::c_int, - handle: *mut *mut libusb_device_handle, - ) -> ::std::os::raw::c_int; -} extern "C" { pub fn libusb_close(dev_handle: *mut libusb_device_handle); } diff --git a/usb_util/src/device_handle.rs b/usb_util/src/device_handle.rs index 32a963f804..77a91bea93 100644 --- a/usb_util/src/device_handle.rs +++ b/usb_util/src/device_handle.rs @@ -8,6 +8,7 @@ use std::sync::Arc; use crate::bindings; use crate::error::{Error, Result}; use crate::libusb_context::LibUsbContextInner; +use crate::libusb_device::LibUsbDevice; use crate::usb_transfer::{UsbTransfer, UsbTransferBuffer}; /// DeviceHandle wraps libusb_device_handle. @@ -39,6 +40,18 @@ impl DeviceHandle { } } + /// Get corresponding usb device. + pub fn get_device(&self) -> LibUsbDevice { + // Safe because 'self.handle' is a valid pointer to device handle and libusb_get_device() + // always returns a valid device. + unsafe { + LibUsbDevice::new( + self._context.clone(), + bindings::libusb_get_device(self.handle), + ) + } + } + /// Reset this usb device. pub fn reset(&self) -> Result<()> { // Safe because 'self.handle' is a valid pointer to device handle. diff --git a/usb_util/src/libusb_context.rs b/usb_util/src/libusb_context.rs index a1bab32648..17a77f0997 100644 --- a/usb_util/src/libusb_context.rs +++ b/usb_util/src/libusb_context.rs @@ -3,11 +3,14 @@ // found in the LICENSE file. use std; -use std::os::raw::{c_short, c_void}; +#[allow(unused_imports)] +use std::os::raw::{c_long, c_short, c_void}; use std::os::unix::io::RawFd; use std::sync::Arc; use crate::bindings; +#[cfg(feature = "sandboxed-libusb")] +use crate::device_handle::DeviceHandle; use crate::error::{Error, Result}; use crate::hotplug::{hotplug_cb, UsbHotplugHandler, UsbHotplugHandlerHolder}; use crate::libusb_device::LibUsbDevice; @@ -66,32 +69,17 @@ impl LibUsbContext { }) } - /// Create a new jailed LibUsbContext. #[cfg(feature = "sandboxed-libusb")] - pub fn new_jailed() -> Result { - let mut ctx: *mut bindings::libusb_context = std::ptr::null_mut(); - // Safe because '&mut ctx' points to a valid memory (on stack). - try_libusb!(unsafe { bindings::libusb_init_jailed(&mut ctx) }); - Ok(LibUsbContext { - inner: Arc::new(LibUsbContextInner { - context: ctx, - pollfd_change_handler: Mutex::new(None), - }), - }) - } - - /// Build device from File. - #[cfg(feature = "sandboxed-libusb")] - pub fn get_device_from_fd(&self, fd: std::fs::File) -> Result { + pub fn handle_from_file(&self, f: std::fs::File) -> Result { use std::os::unix::io::IntoRawFd; - let fd = fd.into_raw_fd(); - let mut device: *mut bindings::libusb_device = std::ptr::null_mut(); - // Safe because fd is valid and owned, and '&mut device' points to valid memory. + let fd = f.into_raw_fd(); + let mut handle: *mut bindings::libusb_device_handle = std::ptr::null_mut(); + // Safe because fd is valid and owned, and '&mut handle' points to valid memory. try_libusb!(unsafe { - bindings::libusb_get_device_from_fd(self.inner.context, fd, &mut device) + bindings::libusb_wrap_sys_device(self.inner.context, fd as c_long, &mut handle) }); - unsafe { Ok(LibUsbDevice::new(self.inner.clone(), device)) } + unsafe { Ok(DeviceHandle::new(self.inner.clone(), handle)) } } /// Returns a list of USB devices currently attached to the system. diff --git a/usb_util/src/libusb_device.rs b/usb_util/src/libusb_device.rs index e8f51207a2..ad87b46cbf 100644 --- a/usb_util/src/libusb_device.rs +++ b/usb_util/src/libusb_device.rs @@ -3,8 +3,6 @@ // found in the LICENSE file. use std; -#[cfg(feature = "sandboxed-libusb")] -use std::os::unix::io::RawFd; use std::sync::Arc; use crate::bindings; @@ -107,14 +105,4 @@ impl LibUsbDevice { // Safe because handle points to valid memory. Ok(unsafe { DeviceHandle::new(self._context.clone(), handle) }) } - - /// Get device handle of this device. Take an external fd. This function is only safe when fd - /// is an fd of this usb device. - #[cfg(feature = "sandboxed-libusb")] - pub unsafe fn open_fd(&self, fd: RawFd) -> Result { - let mut handle: *mut bindings::libusb_device_handle = std::ptr::null_mut(); - // Safe when 'self.device' is constructed from libusb device list and handle is on stack. - try_libusb!(bindings::libusb_open_fd(self.device, fd, &mut handle)); - Ok(DeviceHandle::new(self._context.clone(), handle)) - } }