Make CrosUsbDetector observe VM starts and reconnect devices

Rather than having CrostiniManager explicitly attach USB devices when
termina starts, CrosUsbDetector can be a Concierge::VmObserver and do
this for all VMs whether they are started by chrome, or by vmc.

It also observes VmPluginDispatcherClient::OnVmStateChanged RUNNING to
reconnect devices for Plugin VM.

Moved USB functions from CrostiniManager to CrosUsbDetector since
it is the only client and USB is not specific to crostini, but applies
to all VMs.

Bug: b/150341671
Change-Id: Ic14765d3b4d2911ae297b59ab5fbba7212c9957d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2386441
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803793}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -527,17 +527,8 @@ class CrostiniManager::CrostiniRestarter
       FinishRestart(result);
       return;
     }
-    // If default termina/penguin, then do device sharing, sshfs mount and
-    // reshare folders, else we are finished.
-    auto vm_info = crostini_manager_->GetVmInfo(container_id_.vm_name);
-    if (vm_info && !vm_info->usb_devices_shared &&
-        container_id_.vm_name == kCrostiniDefaultVmName &&
-        chromeos::CrosUsbDetector::Get()) {
-      // Connect shared devices to the vm.
-      chromeos::CrosUsbDetector::Get()->ConnectSharedDevicesOnVmStartup(
-          container_id_.vm_name);
-      vm_info->usb_devices_shared = true;
-    }
+    // If default termina/penguin, then sshfs mount and reshare folders, else we
+    // are finished.
     // If arc sideloading is enabled, configure the container for that.
     crostini_manager_->ConfigureForArcSideload();
     auto info = crostini_manager_->GetContainerInfo(container_id_);
@@ -1946,82 +1937,6 @@ void CrostiniManager::OnDBusShuttingDownForTesting() {
   RemoveDBusObservers();
 }
 
-void CrostiniManager::AttachUsbDevice(const std::string& vm_name,
-                                      device::mojom::UsbDeviceInfoPtr device,
-                                      base::ScopedFD fd,
-                                      AttachUsbDeviceCallback callback) {
-  vm_tools::concierge::AttachUsbDeviceRequest request;
-  request.set_vm_name(vm_name);
-  request.set_owner_id(CryptohomeIdForProfile(profile_));
-  request.set_bus_number(device->bus_number);
-  request.set_port_number(device->port_number);
-  request.set_vendor_id(device->vendor_id);
-  request.set_product_id(device->product_id);
-
-  GetConciergeClient()->AttachUsbDevice(
-      std::move(fd), std::move(request),
-      base::BindOnce(&CrostiniManager::OnAttachUsbDevice,
-                     weak_ptr_factory_.GetWeakPtr(), vm_name, std::move(device),
-                     std::move(callback)));
-}
-
-void CrostiniManager::OnAttachUsbDevice(
-    const std::string& vm_name,
-    device::mojom::UsbDeviceInfoPtr device,
-    AttachUsbDeviceCallback callback,
-    base::Optional<vm_tools::concierge::AttachUsbDeviceResponse> response) {
-  if (!response) {
-    LOG(ERROR) << "Failed to attach USB device, empty dbus response";
-    std::move(callback).Run(/*success=*/false, chromeos::kInvalidUsbPortNumber);
-    return;
-  }
-
-  if (!response->success()) {
-    LOG(ERROR) << "Failed to attach USB device, " << response->reason();
-    std::move(callback).Run(/*success=*/false, chromeos::kInvalidUsbPortNumber);
-    return;
-  }
-
-  std::move(callback).Run(/*success=*/true, response->guest_port());
-}
-
-void CrostiniManager::DetachUsbDevice(const std::string& vm_name,
-                                      device::mojom::UsbDeviceInfoPtr device,
-                                      uint8_t guest_port,
-                                      BoolCallback callback) {
-  vm_tools::concierge::DetachUsbDeviceRequest request;
-  request.set_vm_name(vm_name);
-  request.set_owner_id(CryptohomeIdForProfile(profile_));
-  request.set_guest_port(guest_port);
-
-  GetConciergeClient()->DetachUsbDevice(
-      std::move(request),
-      base::BindOnce(&CrostiniManager::OnDetachUsbDevice,
-                     weak_ptr_factory_.GetWeakPtr(), vm_name, guest_port,
-                     std::move(device), std::move(callback)));
-}
-
-void CrostiniManager::OnDetachUsbDevice(
-    const std::string& vm_name,
-    uint8_t guest_port,
-    device::mojom::UsbDeviceInfoPtr device,
-    BoolCallback callback,
-    base::Optional<vm_tools::concierge::DetachUsbDeviceResponse> response) {
-  if (!response) {
-    LOG(ERROR) << "Failed to detach USB device, empty dbus response";
-    std::move(callback).Run(/*success=*/false);
-    return;
-  }
-
-  if (!response->success()) {
-    LOG(ERROR) << "Failed to detach USB device, " << response->reason();
-    std::move(callback).Run(/*success=*/false);
-    return;
-  }
-
-  std::move(callback).Run(/*success=*/true);
-}
-
 void CrostiniManager::AddFileWatch(const ContainerId& container_id,
                                    const base::FilePath& path,
                                    BoolCallback callback) {

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -428,22 +428,6 @@ class CrostiniManager : public KeyedService,
   void GetContainerSshKeys(const ContainerId& container_id,
                            GetContainerSshKeysCallback callback);
 
-  // Called when a USB device should be attached into the VM. Should only ever
-  // be called on user action. The guest_port is only valid on success.
-  using AttachUsbDeviceCallback =
-      base::OnceCallback<void(bool success, uint8_t guest_port)>;
-  void AttachUsbDevice(const std::string& vm_name,
-                       device::mojom::UsbDeviceInfoPtr device,
-                       base::ScopedFD fd,
-                       AttachUsbDeviceCallback callback);
-
-  // Called when a USB device should be detached from the VM.
-  // May be called on user action or on USB removal.
-  void DetachUsbDevice(const std::string& vm_name,
-                       device::mojom::UsbDeviceInfoPtr device,
-                       uint8_t guest_port,
-                       BoolCallback callback);
-
   // Add a relative path to watch within the container homedir. Register as a
   // CrostiniFileChangeObserver to be notified when changes occur. Used by
   // FilesApp.
@@ -828,21 +812,6 @@ class CrostiniManager : public KeyedService,
       GetContainerSshKeysCallback callback,
       base::Optional<vm_tools::concierge::ContainerSshKeysResponse> response);
 
-  // Callback for CrostiniManager::OnAttachUsbDeviceOpen
-  void OnAttachUsbDevice(
-      const std::string& vm_name,
-      device::mojom::UsbDeviceInfoPtr device,
-      AttachUsbDeviceCallback callback,
-      base::Optional<vm_tools::concierge::AttachUsbDeviceResponse> response);
-
-  // Callback for CrostiniManager::DetachUsbDevice
-  void OnDetachUsbDevice(
-      const std::string& vm_name,
-      uint8_t guest_port,
-      device::mojom::UsbDeviceInfoPtr device,
-      BoolCallback callback,
-      base::Optional<vm_tools::concierge::DetachUsbDeviceResponse> response);
-
   // Callback for AnsibleManagementService::ConfigureDefaultContainer
   void OnDefaultContainerConfigured(bool success);
 

chrome/browser/chromeos/crostini/crostini_manager_unittest.cc

@@ -60,7 +60,6 @@ const char kVmName[] = "vm_name";
 const char kContainerName[] = "container_name";
 const char kPackageID[] = "package;1;;";
 constexpr int64_t kDiskSizeBytes = 4ll * 1024 * 1024 * 1024;  // 4 GiB
-const uint8_t kUsbPort = 0x01;
 const char kTerminaKernelVersion[] =
     "4.19.56-05556-gca219a5b1086 #3 SMP PREEMPT Mon Jul 1 14:36:38 CEST 2019";
 const char kCrostiniCorruptionHistogram[] = "Crostini.FilesystemCorruption";
@@ -606,96 +605,6 @@ TEST_F(CrostiniManagerTest, UninstallPackageOwningFileSignalOperationBlocked) {
   run_loop()->Run();
 }
 
-TEST_F(CrostiniManagerTest, AttachUsbDeviceSuccess) {
-  vm_tools::concierge::AttachUsbDeviceResponse response;
-  response.set_success(true);
-  fake_concierge_client_->set_attach_usb_device_response(response);
-
-  auto fake_usb = fake_usb_manager_.CreateAndAddDevice(0, 0);
-  auto guid = fake_usb->guid;
-
-  crostini_manager()->AttachUsbDevice(
-      kVmName, std::move(fake_usb), TestFileDescriptor(),
-      base::BindOnce(&CrostiniManagerTest::AttachUsbDeviceCallback,
-                     base::Unretained(this), run_loop()->QuitClosure(),
-                     /*expected_success=*/true));
-  run_loop()->Run();
-  fake_usb_manager_.RemoveDevice(guid);
-}
-
-TEST_F(CrostiniManagerTest, AttachUsbDeviceFailure) {
-  vm_tools::concierge::AttachUsbDeviceResponse response;
-  response.set_success(false);
-  fake_concierge_client_->set_attach_usb_device_response(response);
-
-  auto fake_usb = fake_usb_manager_.CreateAndAddDevice(0, 0);
-  auto guid = fake_usb->guid;
-
-  crostini_manager()->AttachUsbDevice(
-      kVmName, std::move(fake_usb), TestFileDescriptor(),
-      base::BindOnce(&CrostiniManagerTest::AttachUsbDeviceCallback,
-                     base::Unretained(this), run_loop()->QuitClosure(),
-                     /*expected_success=*/false));
-  run_loop()->Run();
-  fake_usb_manager_.RemoveDevice(guid);
-}
-
-TEST_F(CrostiniManagerTest, DetachUsbDeviceSuccess) {
-  vm_tools::concierge::AttachUsbDeviceResponse attach_response;
-  attach_response.set_success(true);
-  fake_concierge_client_->set_attach_usb_device_response(attach_response);
-
-  vm_tools::concierge::DetachUsbDeviceResponse detach_response;
-  detach_response.set_success(true);
-  fake_concierge_client_->set_detach_usb_device_response(detach_response);
-
-  auto fake_usb = fake_usb_manager_.CreateAndAddDevice(0, 0);
-  auto guid = fake_usb->guid;
-
-  auto detach_usb = base::BindOnce(
-      &CrostiniManager::DetachUsbDevice, base::Unretained(crostini_manager()),
-      kVmName, fake_usb.Clone(), kUsbPort,
-      base::BindOnce(&CrostiniManagerTest::DetachUsbDeviceCallback,
-                     base::Unretained(this), run_loop()->QuitClosure(), true,
-                     /*expected_success=*/true));
-
-  crostini_manager()->AttachUsbDevice(
-      kVmName, std::move(fake_usb), TestFileDescriptor(),
-      base::BindOnce(&CrostiniManagerTest::AttachUsbDeviceCallback,
-                     base::Unretained(this), std::move(detach_usb),
-                     /*expected_success=*/true));
-  run_loop()->Run();
-  fake_usb_manager_.RemoveDevice(guid);
-}
-
-TEST_F(CrostiniManagerTest, DetachUsbDeviceFailure) {
-  vm_tools::concierge::AttachUsbDeviceResponse attach_response;
-  attach_response.set_success(true);
-  fake_concierge_client_->set_attach_usb_device_response(attach_response);
-
-  vm_tools::concierge::DetachUsbDeviceResponse detach_response;
-  detach_response.set_success(false);
-  fake_concierge_client_->set_detach_usb_device_response(detach_response);
-
-  auto fake_usb = fake_usb_manager_.CreateAndAddDevice(0, 0);
-  auto guid = fake_usb->guid;
-
-  auto detach_usb = base::BindOnce(
-      &CrostiniManager::DetachUsbDevice, base::Unretained(crostini_manager()),
-      kVmName, fake_usb.Clone(), kUsbPort,
-      base::BindOnce(&CrostiniManagerTest::DetachUsbDeviceCallback,
-                     base::Unretained(this), run_loop()->QuitClosure(), true,
-                     /*expected_success=*/false));
-
-  crostini_manager()->AttachUsbDevice(
-      kVmName, std::move(fake_usb), TestFileDescriptor(),
-      base::BindOnce(&CrostiniManagerTest::AttachUsbDeviceCallback,
-                     base::Unretained(this), std::move(detach_usb),
-                     /*expected_success=*/true));
-  run_loop()->Run();
-  fake_usb_manager_.RemoveDevice(guid);
-}
-
 class CrostiniManagerRestartTest : public CrostiniManagerTest,
                                    public CrostiniManager::RestartObserver {
  public:

chrome/browser/chromeos/crostini/crostini_simple_types.h

@@ -137,7 +137,6 @@ enum class ContainerVersion {
 struct VmInfo {
   VmState state;
   vm_tools::concierge::VmInfo info;
-  bool usb_devices_shared = false;
 };
 
 struct StreamingExportStatus {

chrome/browser/chromeos/usb/cros_usb_detector.cc

@@ -11,9 +11,12 @@
 #include "base/bind_helpers.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "chrome/browser/chromeos/crostini/crostini_features.h"
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
+#include "chrome/browser/chromeos/plugin_vm/plugin_vm_util.h"
 #include "chrome/browser/notifications/system_notification_helper.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/settings_window_manager_chromeos.h"
@@ -21,6 +24,8 @@
 #include "chrome/common/webui_url_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "chromeos/dbus/concierge_client.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/arc/arc_util.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/vector_icons/vector_icons.h"
@@ -145,7 +150,9 @@ class CrosUsbNotificationDelegate
              const base::Optional<base::string16>& reply) override {
     disposition_ = CrosUsbNotificationClosed::kUnknown;
     if (button_index && button_index.value() == 0) {
-      HandleConnectToVm();
+      HandleConnectToVm(crostini::kCrostiniDefaultVmName);
+    } else if (button_index && button_index.value() == 1) {
+      HandleConnectToVm(plugin_vm::kPluginVmName);
     } else {
       HandleShowSettings();
     }
@@ -159,12 +166,12 @@ class CrosUsbNotificationDelegate
 
  private:
   ~CrosUsbNotificationDelegate() override = default;
-  void HandleConnectToVm() {
+  void HandleConnectToVm(const std::string& vm_name) {
     disposition_ = CrosUsbNotificationClosed::kConnectToLinux;
     chromeos::CrosUsbDetector* detector = chromeos::CrosUsbDetector::Get();
     if (detector) {
-      detector->AttachUsbDeviceToVm(crostini::kCrostiniDefaultVmName,
-                                    device_info_->guid, base::DoNothing());
+      detector->AttachUsbDeviceToVm(vm_name, device_info_->guid,
+                                    base::DoNothing());
       return;
     }
     Close(false);
@@ -226,6 +233,8 @@ void ShowNotificationForDevice(device::mojom::UsbDeviceInfoPtr device_info) {
   rich_notification_data.buttons.emplace_back(
       message_center::ButtonInfo(l10n_util::GetStringUTF16(
           IDS_CROSUSB_NOTIFICATION_BUTTON_CONNECT_TO_LINUX)));
+  rich_notification_data.buttons.emplace_back(message_center::ButtonInfo(
+      l10n_util::GetStringUTF16(IDS_PLUGIN_VM_APP_NAME)));
 
   std::string notification_id =
       CrosUsbDetector::MakeNotificationId(device_info->guid);
@@ -254,7 +263,7 @@ CrosUsbDeviceInfo::VmSharingInfo::VmSharingInfo(const VmSharingInfo&) = default;
 CrosUsbDeviceInfo::VmSharingInfo::~VmSharingInfo() = default;
 
 std::string CrosUsbDetector::MakeNotificationId(const std::string& guid) {
-  return "cros:" + guid;
+  return "cros:" + base::HexEncode(guid.data(), guid.size());
 }
 
 // static
@@ -300,10 +309,20 @@ CrosUsbDetector::CrosUsbDetector() {
   fastboot_device_filter_->subclass_code = kAdbSubclass;
   fastboot_device_filter_->has_protocol_code = true;
   fastboot_device_filter_->protocol_code = kFastbootProtocol;
+
+  chromeos::DBusThreadManager::Get()->GetConciergeClient()->AddVmObserver(this);
+  chromeos::DBusThreadManager::Get()
+      ->GetVmPluginDispatcherClient()
+      ->AddObserver(this);
 }
 
 CrosUsbDetector::~CrosUsbDetector() {
   DCHECK_EQ(this, g_cros_usb_detector);
+  chromeos::DBusThreadManager::Get()->GetConciergeClient()->RemoveVmObserver(
+      this);
+  chromeos::DBusThreadManager::Get()
+      ->GetVmPluginDispatcherClient()
+      ->RemoveObserver(this);
   g_cros_usb_detector = nullptr;
 }
 
@@ -380,6 +399,25 @@ bool CrosUsbDetector::ShouldShowNotification(
   return false;
 }
 
+void CrosUsbDetector::OnVmStarted(
+    const vm_tools::concierge::VmStartedSignal& signal) {
+  ConnectSharedDevicesOnVmStartup(signal.name());
+}
+
+void CrosUsbDetector::OnVmStopped(
+    const vm_tools::concierge::VmStoppedSignal& signal) {}
+
+void CrosUsbDetector::OnVmToolsStateChanged(
+    const vm_tools::plugin_dispatcher::VmToolsStateChangedSignal& signal) {}
+
+void CrosUsbDetector::OnVmStateChanged(
+    const vm_tools::plugin_dispatcher::VmStateChangedSignal& signal) {
+  if (signal.vm_state() ==
+      vm_tools::plugin_dispatcher::VmState::VM_STATE_RUNNING) {
+    ConnectSharedDevicesOnVmStartup(signal.vm_name());
+  }
+}
+
 void CrosUsbDetector::OnDeviceChecked(
     device::mojom::UsbDeviceInfoPtr device_info,
     bool hide_notification,
@@ -502,8 +540,8 @@ void CrosUsbDetector::AttachUsbDeviceToVm(
   SystemNotificationHelper::GetInstance()->Close(
       CrosUsbDetector::MakeNotificationId(guid));
 
-  VLOG(1) << "Opening " << std::hex << guid << " with mask "
-          << allowed_interfaces_mask;
+  VLOG(1) << "Opening " << base::HexEncode(guid.data(), guid.size())
+          << " with mask " << std::hex << allowed_interfaces_mask;
   // Open a file descriptor to pass to CrostiniManager & Concierge.
   device_manager_->OpenFileDescriptor(
       guid, allowed_interfaces_mask,
@@ -523,7 +561,6 @@ void CrosUsbDetector::DetachUsbDeviceFromVm(
     std::move(callback).Run(/*success=*/true);
     return;
   }
-  const auto& device_info = it->second;
 
   base::Optional<uint8_t> guest_port;
   for (const auto& device : usb_devices_) {
@@ -540,8 +577,14 @@ void CrosUsbDetector::DetachUsbDeviceFromVm(
     std::move(callback).Run(/*success=*/true);
     return;
   }
-  manager()->DetachUsbDevice(
-      vm_name, device_info.Clone(), *guest_port,
+
+  vm_tools::concierge::DetachUsbDeviceRequest request;
+  request.set_vm_name(vm_name);
+  request.set_owner_id(crostini::CryptohomeIdForProfile(profile()));
+  request.set_guest_port(*guest_port);
+
+  chromeos::DBusThreadManager::Get()->GetConciergeClient()->DetachUsbDevice(
+      std::move(request),
       base::BindOnce(&CrosUsbDetector::OnUsbDeviceDetachFinished,
                      weak_ptr_factory_.GetWeakPtr(), vm_name, guid,
                      std::move(callback)));
@@ -581,13 +624,18 @@ void CrosUsbDetector::OnAttachUsbDeviceOpened(
       }
     }
   }
-  // TODO(b/123374026): Ideally CrostiniManager wouldn't be used for
-  // attaching/detaching USB devices from non-Crostini VMs, e.g. ARCVM. It works
-  // currently since CrostiniManager is mostly delegating to ConciergeClient but
-  // it's a little confusing and fragile.
+
   const std::string guid = device_info->guid;
-  manager()->AttachUsbDevice(
-      vm_name, std::move(device_info), std::move(fd),
+  vm_tools::concierge::AttachUsbDeviceRequest request;
+  request.set_vm_name(vm_name);
+  request.set_owner_id(crostini::CryptohomeIdForProfile(profile()));
+  request.set_bus_number(device_info->bus_number);
+  request.set_port_number(device_info->port_number);
+  request.set_vendor_id(device_info->vendor_id);
+  request.set_product_id(device_info->product_id);
+
+  chromeos::DBusThreadManager::Get()->GetConciergeClient()->AttachUsbDevice(
+      std::move(fd), std::move(request),
       base::BindOnce(&CrosUsbDetector::OnUsbDeviceAttachFinished,
                      weak_ptr_factory_.GetWeakPtr(), vm_name, guid,
                      std::move(callback)));
@@ -597,14 +645,22 @@ void CrosUsbDetector::OnUsbDeviceAttachFinished(
     const std::string& vm_name,
     const std::string& guid,
     base::OnceCallback<void(bool success)> callback,
-    bool success,
-    uint8_t guest_port) {
+    base::Optional<vm_tools::concierge::AttachUsbDeviceResponse> response) {
+  bool success = true;
+  if (!response) {
+    LOG(ERROR) << "Failed to attach USB device, empty dbus response";
+    success = false;
+  } else if (!response->success()) {
+    LOG(ERROR) << "Failed to attach USB device, " << response->reason();
+    success = false;
+  }
+
   if (success) {
     for (auto& device : usb_devices_) {
       if (device.guid == guid) {
         auto& vm_sharing_info = device.vm_sharing_info[vm_name];
         vm_sharing_info.shared = true;
-        vm_sharing_info.guest_port = guest_port;
+        vm_sharing_info.guest_port = response->guest_port();
         break;
       }
     }
@@ -617,7 +673,16 @@ void CrosUsbDetector::OnUsbDeviceDetachFinished(
     const std::string& vm_name,
     const std::string& guid,
     base::OnceCallback<void(bool success)> callback,
-    bool success) {
+    base::Optional<vm_tools::concierge::DetachUsbDeviceResponse> response) {
+  bool success = true;
+  if (!response) {
+    LOG(ERROR) << "Failed to detach USB device, empty dbus response";
+    success = false;
+  } else if (!response->success()) {
+    LOG(ERROR) << "Failed to detach USB device, " << response->reason();
+    success = false;
+  }
+
   for (auto& device : usb_devices_) {
     if (device.guid == guid) {
       device.vm_sharing_info.erase(vm_name);

chrome/browser/chromeos/usb/cros_usb_detector.h

@@ -15,6 +15,8 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
+#include "chromeos/dbus/concierge_client.h"
+#include "chromeos/dbus/vm_plugin_dispatcher_client.h"
 #include "mojo/public/cpp/bindings/associated_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -81,7 +83,9 @@ class CrosUsbDeviceObserver : public base::CheckedObserver {
 
 // Detects USB Devices for Chrome OS and manages UI for controlling their use
 // with CrOS, Web or GuestOSs.
-class CrosUsbDetector : public device::mojom::UsbDeviceManagerClient {
+class CrosUsbDetector : public device::mojom::UsbDeviceManagerClient,
+                        public chromeos::ConciergeClient::VmObserver,
+                        public chromeos::VmPluginDispatcherClient::Observer {
  public:
   // Used to namespace USB notifications to avoid clashes with WebUsbDetector.
   static std::string MakeNotificationId(const std::string& guid);
@@ -133,6 +137,17 @@ class CrosUsbDetector : public device::mojom::UsbDeviceManagerClient {
   std::vector<CrosUsbDeviceInfo> GetDevicesSharableWithCrostini() const;
 
  private:
+  // chromeos::ConciergeClient::VmObserver:
+  void OnVmStarted(const vm_tools::concierge::VmStartedSignal& signal) override;
+  void OnVmStopped(const vm_tools::concierge::VmStoppedSignal& signal) override;
+
+  // chromeos::VmPluginDispatcherClient::Observer:
+  void OnVmToolsStateChanged(
+      const vm_tools::plugin_dispatcher::VmToolsStateChangedSignal& signal)
+      override;
+  void OnVmStateChanged(
+      const vm_tools::plugin_dispatcher::VmStateChangedSignal& signal) override;
+
   // Called after USB device access has been checked.
   void OnDeviceChecked(device::mojom::UsbDeviceInfoPtr device,
                        bool hide_notification,
@@ -159,13 +174,13 @@ class CrosUsbDetector : public device::mojom::UsbDeviceManagerClient {
       const std::string& vm_name,
       const std::string& guid,
       base::OnceCallback<void(bool success)> callback,
-      bool success,
-      uint8_t guest_port);
+      base::Optional<vm_tools::concierge::AttachUsbDeviceResponse> response);
+
   void OnUsbDeviceDetachFinished(
       const std::string& vm_name,
       const std::string& guid,
       base::OnceCallback<void(bool success)> callback,
-      bool success);
+      base::Optional<vm_tools::concierge::DetachUsbDeviceResponse> response);
 
   // Returns true when a device should show a notification when attached.
   bool ShouldShowNotification(const device::mojom::UsbDeviceInfo& device_info,

chrome/browser/chromeos/usb/cros_usb_detector_unittest.cc

@@ -28,6 +28,7 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_cicerone_client.h"
 #include "chromeos/dbus/fake_concierge_client.h"
+#include "chromeos/dbus/fake_vm_plugin_dispatcher_client.h"
 #include "components/arc/arc_util.h"
 #include "services/device/public/cpp/test/fake_usb_device_info.h"
 #include "services/device/public/cpp/test/fake_usb_device_manager.h"
@@ -121,7 +122,9 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
         chromeos::DBusThreadManager::Get()->GetCiceroneClient());
     fake_concierge_client_ = static_cast<chromeos::FakeConciergeClient*>(
         chromeos::DBusThreadManager::Get()->GetConciergeClient());
-    cros_usb_detector_ = std::make_unique<chromeos::CrosUsbDetector>();
+    fake_vm_plugin_dispatcher_client_ =
+        static_cast<chromeos::FakeVmPluginDispatcherClient*>(
+            chromeos::DBusThreadManager::Get()->GetVmPluginDispatcherClient());
   }
 
   ~CrosUsbDetectorTest() override { chromeos::DBusThreadManager::Shutdown(); }
@@ -131,6 +134,7 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
   }
 
   void SetUp() override {
+    cros_usb_detector_ = std::make_unique<chromeos::CrosUsbDetector>();
     BrowserWithTestWindowTest::SetUp();
     crostini_test_helper_.reset(new crostini::CrostiniTestHelper(profile()));
 
@@ -153,6 +157,7 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
   void TearDown() override {
     crostini_test_helper_.reset();
     BrowserWithTestWindowTest::TearDown();
+    cros_usb_detector_.reset();
   }
 
   void ConnectToDeviceManager() {
@@ -213,6 +218,7 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
   // Owned by chromeos::DBusThreadManager
   chromeos::FakeCiceroneClient* fake_cicerone_client_;
   chromeos::FakeConciergeClient* fake_concierge_client_;
+  chromeos::FakeVmPluginDispatcherClient* fake_vm_plugin_dispatcher_client_;
 
   TestCrosUsbDeviceObserver usb_device_observer_;
   std::unique_ptr<chromeos::CrosUsbDetector> cros_usb_detector_;
@@ -754,13 +760,25 @@ TEST_F(CrosUsbDetectorTest, SharedDevicesGetAttachedOnStartup) {
   base::RunLoop().RunUntilIdle();
   EXPECT_TRUE(IsSharedWithCrostini(GetSingleDeviceInfo()));
 
+  // Concierge::VmStarted signal should trigger connections.
   cros_usb_detector_->AddUsbDeviceObserver(&usb_device_observer_);
-  cros_usb_detector_->ConnectSharedDevicesOnVmStartup(
-      crostini::kCrostiniDefaultVmName);
+  vm_tools::concierge::VmStartedSignal vm_started_signal;
+  vm_started_signal.set_name(crostini::kCrostiniDefaultVmName);
+  fake_concierge_client_->NotifyVmStarted(vm_started_signal);
   base::RunLoop().RunUntilIdle();
-  // On VM startup, we have to re-attach devices.
   EXPECT_EQ(1, usb_device_observer_.notify_count());
   EXPECT_TRUE(IsSharedWithCrostini(GetSingleDeviceInfo()));
+
+  // VmPluginDispatcherClient::OnVmStateChanged RUNNING should also trigger.
+  vm_tools::plugin_dispatcher::VmStateChangedSignal vm_state_changed_signal;
+  vm_state_changed_signal.set_vm_name(crostini::kCrostiniDefaultVmName);
+  vm_state_changed_signal.set_vm_state(
+      vm_tools::plugin_dispatcher::VmState::VM_STATE_RUNNING);
+  fake_vm_plugin_dispatcher_client_->NotifyVmStateChanged(
+      vm_state_changed_signal);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(2, usb_device_observer_.notify_count());
+  EXPECT_TRUE(IsSharedWithCrostini(GetSingleDeviceInfo()));
 }
 
 TEST_F(CrosUsbDetectorTest, DeviceAllowedInterfacesMaskSetCorrectly) {