Commit c50922a2 authored by Joshua Pawlicki's avatar Joshua Pawlicki Committed by Commit Bot

Revert "USB devices are only shared with one VM at a time"

This reverts commit 4e4ea3fd.

Reason for revert: Speculative revert to fix BrowserTest.CloseWithAppMenuOpen failures on linux-lacros-tester-rel bot.

[ RUN      ] BrowserTest.CloseWithAppMenuOpen
[27984:27984:0917/013814.891745:WARNING:ozone_platform_wayland.cc(203)] Failed to find drm render node path.
[27813:28015:0917/013814.933599:ERROR:object_proxy.cc(621)] Failed to call method: org.freedesktop.DBus.Properties.Get: object_path= /org/freedesktop/UPower: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.UPower was not provided by any .service files
[27813:28015:0917/013814.933632:WARNING:property.cc(144)] DaemonVersion: GetAndBlock: failed.
[27813:28015:0917/013814.933859:ERROR:object_proxy.cc(621)] Failed to call method: org.freedesktop.UPower.GetDisplayDevice: object_path= /org/freedesktop/UPower: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.UPower was not provided by any .service files
[27813:28015:0917/013814.934033:ERROR:object_proxy.cc(621)] Failed to call method: org.freedesktop.UPower.EnumerateDevices: object_path= /org/freedesktop/UPower: org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.UPower was not provided by any .service files
[destroyed object]: error 1: popup parent not constructed

Original change's description:
> USB devices are only shared with one VM at a time
> 
> Remove map of VmSharingInfo from CrosUsbDeviceInfo since device
> is only shared with one VM at a time.
> 
> TBR=khorimoto@chromium.org
> 
> Bug: 1129266
> Change-Id: I2025091bf63eeed538c62d8b39cfbb744339aa64
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2413716
> Commit-Queue: Joel Hockey <joelhockey@chromium.org>
> Reviewed-by: Nicholas Verne <nverne@chromium.org>
> Auto-Submit: Joel Hockey <joelhockey@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#807786}

TBR=joelhockey@chromium.org,nverne@chromium.org

Change-Id: I96cab98518776d10ef10d7c8376eb7379c92d6e3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1129266
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2416389Reviewed-by: default avatarJoshua Pawlicki <waffles@chromium.org>
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807879}
parent 822e825f
......@@ -291,6 +291,10 @@ CrosUsbDeviceInfo::CrosUsbDeviceInfo() = default;
CrosUsbDeviceInfo::CrosUsbDeviceInfo(const CrosUsbDeviceInfo&) = default;
CrosUsbDeviceInfo::~CrosUsbDeviceInfo() = default;
CrosUsbDeviceInfo::VmSharingInfo::VmSharingInfo() = default;
CrosUsbDeviceInfo::VmSharingInfo::VmSharingInfo(const VmSharingInfo&) = default;
CrosUsbDeviceInfo::VmSharingInfo::~VmSharingInfo() = default;
std::string CrosUsbDetector::MakeNotificationId(const std::string& guid) {
return "cros:" + guid;
}
......@@ -506,8 +510,10 @@ void CrosUsbDetector::OnDeviceRemoved(
std::string guid = device_info->guid;
for (const auto& device : usb_devices_) {
if (device.guid == guid && device.shared_vm_name) {
DetachUsbDeviceFromVm(*device.shared_vm_name, guid, base::DoNothing());
if (device.guid == guid) {
for (const auto& sharing_info_pair : device.vm_sharing_info) {
DetachUsbDeviceFromVm(sharing_info_pair.first, guid, base::DoNothing());
}
}
}
const auto& start = std::remove_if(
......@@ -530,13 +536,15 @@ void CrosUsbDetector::ConnectSharedDevicesOnVmStartup(
const std::string& vm_name) {
// Reattach shared devices when the VM becomes available.
for (auto& device : usb_devices_) {
if (device.shared_vm_name == vm_name) {
for (auto& sharing_pair : device.vm_sharing_info) {
if (sharing_pair.second.shared && sharing_pair.first == vm_name) {
VLOG(1) << "Connecting " << device.label << " to " << vm_name;
// Clear any older guest_port setting.
device.guest_port = base::nullopt;
sharing_pair.second.guest_port = base::nullopt;
AttachUsbDeviceToVm(vm_name, device.guid, base::DoNothing());
}
}
}
}
void CrosUsbDetector::AttachUsbDeviceToVm(
......@@ -550,7 +558,7 @@ void CrosUsbDetector::AttachUsbDeviceToVm(
// restart.
// Setting this flag early also allows the UI not to flicker because of
// the notification resulting from the default VM detach below.
device.shared_vm_name = vm_name;
device.vm_sharing_info[vm_name].shared = true;
allowed_interfaces_mask = device.allowed_interfaces_mask;
// The guest port will be set on completion.
break;
......@@ -593,11 +601,14 @@ void CrosUsbDetector::DetachUsbDeviceFromVm(
base::Optional<uint8_t> guest_port;
for (const auto& device : usb_devices_) {
if (device.guid == guid && device.shared_vm_name == vm_name) {
guest_port = device.guest_port;
if (device.guid == guid) {
const auto it = device.vm_sharing_info.find(vm_name);
if (it != device.vm_sharing_info.end()) {
guest_port = it->second.guest_port;
break;
}
}
}
if (!guest_port) {
std::move(callback).Run(/*success=*/true);
......@@ -641,7 +652,8 @@ void CrosUsbDetector::OnAttachUsbDeviceOpened(
}
for (const auto& device : usb_devices_) {
if (device.guid == device_info->guid) {
if (device.shared_vm_name == vm_name && device.guest_port) {
const auto it = device.vm_sharing_info.find(vm_name);
if (it != device.vm_sharing_info.end() && it->second.guest_port) {
LOG(ERROR) << "Device " << device.label << " is already shared";
// The device is already attached.
std::move(callback).Run(/*success=*/true);
......@@ -682,8 +694,9 @@ void CrosUsbDetector::OnUsbDeviceAttachFinished(
if (success) {
for (auto& device : usb_devices_) {
if (device.guid == guid) {
device.shared_vm_name = vm_name;
device.guest_port = response->guest_port();
auto& vm_sharing_info = device.vm_sharing_info[vm_name];
vm_sharing_info.shared = true;
vm_sharing_info.guest_port = response->guest_port();
break;
}
}
......@@ -708,8 +721,7 @@ void CrosUsbDetector::OnUsbDeviceDetachFinished(
for (auto& device : usb_devices_) {
if (device.guid == guid) {
device.shared_vm_name = base::nullopt;
device.guest_port = base::nullopt;
device.vm_sharing_info.erase(vm_name);
break;
}
}
......
......@@ -51,15 +51,25 @@ struct CrosUsbDeviceInfo {
CrosUsbDeviceInfo(const CrosUsbDeviceInfo&);
~CrosUsbDeviceInfo();
struct VmSharingInfo {
VmSharingInfo();
VmSharingInfo(const VmSharingInfo&);
~VmSharingInfo();
// Whether the device is shared with the VM. Note that the device may be
// shared but not attached (yet) in which case |guest_port| below would be
// unset.
bool shared = false;
base::Optional<uint8_t> guest_port;
};
// Maps a VM name to the sharing/attach information of the device in the VM
// identified by that name.
base::flat_map<std::string, VmSharingInfo> vm_sharing_info;
std::string guid;
base::string16 label;
// Whether the device can be shared with guest OSes.
bool sharable_with_crostini = false;
// Name of VM shared with. Unset if not shared. Note that the device may be
// shared but not attached (yet) in which case |guest_port| below would be
// unset.
base::Optional<std::string> shared_vm_name;
base::Optional<uint8_t> guest_port;
// Interfaces shareable with guest OSes
uint32_t allowed_interfaces_mask = 0;
// TODO(nverne): Add current state and errors etc.
......
......@@ -198,6 +198,13 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
return devices.front();
}
static bool IsSharedWithCrostini(
const chromeos::CrosUsbDeviceInfo& device_info) {
const auto it = device_info.vm_sharing_info.find(
crostini::kCrostiniDefaultVmName);
return it != device_info.vm_sharing_info.end() && it->second.shared;
}
protected:
base::string16 connection_message(const char* product_name) {
return base::ASCIIToUTF16(base::StringPrintf(
......@@ -730,12 +737,15 @@ TEST_F(CrosUsbDetectorTest, AttachDeviceToVmSetsGuestPort) {
auto device_info = GetSingleDeviceInfo();
AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device_info.guid);
EXPECT_FALSE(device_info.guest_port.has_value());
EXPECT_FALSE(
chromeos::CrosUsbDeviceInfo::VmSharingInfo().guest_port.has_value());
device_info = GetSingleDeviceSharableWithCrostini();
EXPECT_TRUE(device_info.shared_vm_name.has_value());
EXPECT_EQ(crostini::kCrostiniDefaultVmName, *device_info.shared_vm_name);
EXPECT_TRUE(device_info.guest_port.has_value());
EXPECT_EQ(0U, *device_info.guest_port);
EXPECT_EQ(1U, device_info.vm_sharing_info.size());
auto crostini_info =
device_info.vm_sharing_info[crostini::kCrostiniDefaultVmName];
EXPECT_TRUE(crostini_info.shared);
EXPECT_TRUE(crostini_info.guest_port.has_value());
EXPECT_EQ(0U, *crostini_info.guest_port);
}
TEST_F(CrosUsbDetectorTest, AttachingAlreadyAttachedDeviceIsANoOp) {
......@@ -748,15 +758,15 @@ TEST_F(CrosUsbDetectorTest, AttachingAlreadyAttachedDeviceIsANoOp) {
base::RunLoop().RunUntilIdle();
auto device_info = GetSingleDeviceInfo();
EXPECT_FALSE(device_info.shared_vm_name.has_value());
EXPECT_EQ(0U, device_info.vm_sharing_info.size());
AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device_info.guid);
cros_usb_detector_->AddUsbDeviceObserver(&usb_device_observer_);
AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device_info.guid);
EXPECT_EQ(0, usb_device_observer_.notify_count());
device_info = GetSingleDeviceInfo();
EXPECT_TRUE(device_info.shared_vm_name.has_value());
EXPECT_EQ(crostini::kCrostiniDefaultVmName, *device_info.shared_vm_name);
EXPECT_EQ(1U, device_info.vm_sharing_info.size());
EXPECT_TRUE(IsSharedWithCrostini(device_info));
}
TEST_F(CrosUsbDetectorTest, DeviceCanBeAttachedToArcVmWhenCrostiniIsDisabled) {
......@@ -768,12 +778,8 @@ TEST_F(CrosUsbDetectorTest, DeviceCanBeAttachedToArcVmWhenCrostiniIsDisabled) {
device_manager_.AddDevice(device_1);
base::RunLoop().RunUntilIdle();
auto device_info = GetSingleDeviceInfo();
AttachDeviceToVm(arc::kArcVmName, device_info.guid);
base::RunLoop().RunUntilIdle();
device_info = GetSingleDeviceInfo();
EXPECT_TRUE(device_info.shared_vm_name.has_value());
EXPECT_EQ(arc::kArcVmName, *device_info.shared_vm_name);
AttachDeviceToVm(arc::kArcVmName, GetSingleDeviceInfo().guid);
EXPECT_TRUE(GetSingleDeviceInfo().vm_sharing_info[arc::kArcVmName].shared);
}
TEST_F(CrosUsbDetectorTest, SharedDevicesGetAttachedOnStartup) {
......@@ -790,14 +796,13 @@ TEST_F(CrosUsbDetectorTest, SharedDevicesGetAttachedOnStartup) {
base::RunLoop().RunUntilIdle();
// No device is shared with Crostini, yet.
EXPECT_EQ(0, usb_device_observer_.notify_count());
auto device_info = GetSingleDeviceInfo();
EXPECT_FALSE(device_info.shared_vm_name.has_value());
auto device = GetSingleDeviceInfo();
EXPECT_EQ(0U, device.vm_sharing_info.size());
AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device_info.guid);
device = GetSingleDeviceInfo();
AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device.guid);
base::RunLoop().RunUntilIdle();
device_info = GetSingleDeviceInfo();
EXPECT_TRUE(device_info.shared_vm_name.has_value());
EXPECT_EQ(crostini::kCrostiniDefaultVmName, *device_info.shared_vm_name);
EXPECT_TRUE(IsSharedWithCrostini(GetSingleDeviceInfo()));
// Concierge::VmStarted signal should trigger connections.
cros_usb_detector_->AddUsbDeviceObserver(&usb_device_observer_);
......@@ -806,9 +811,7 @@ TEST_F(CrosUsbDetectorTest, SharedDevicesGetAttachedOnStartup) {
fake_concierge_client_->NotifyVmStarted(vm_started_signal);
base::RunLoop().RunUntilIdle();
EXPECT_EQ(1, usb_device_observer_.notify_count());
device_info = GetSingleDeviceInfo();
EXPECT_TRUE(device_info.shared_vm_name.has_value());
EXPECT_EQ(crostini::kCrostiniDefaultVmName, *device_info.shared_vm_name);
EXPECT_TRUE(IsSharedWithCrostini(GetSingleDeviceInfo()));
// VmPluginDispatcherClient::OnVmStateChanged RUNNING should also trigger.
vm_tools::plugin_dispatcher::VmStateChangedSignal vm_state_changed_signal;
......@@ -819,9 +822,7 @@ TEST_F(CrosUsbDetectorTest, SharedDevicesGetAttachedOnStartup) {
vm_state_changed_signal);
base::RunLoop().RunUntilIdle();
EXPECT_EQ(2, usb_device_observer_.notify_count());
device_info = GetSingleDeviceInfo();
EXPECT_TRUE(device_info.shared_vm_name.has_value());
EXPECT_EQ(crostini::kCrostiniDefaultVmName, *device_info.shared_vm_name);
EXPECT_TRUE(IsSharedWithCrostini(GetSingleDeviceInfo()));
}
TEST_F(CrosUsbDetectorTest, DeviceAllowedInterfacesMaskSetCorrectly) {
......
......@@ -288,12 +288,13 @@ namespace {
base::ListValue UsbDevicesToListValue(
const std::vector<CrosUsbDeviceInfo> shared_usbs) {
base::ListValue usb_devices_list;
for (auto& device : shared_usbs) {
for (auto device : shared_usbs) {
base::Value device_info(base::Value::Type::DICTIONARY);
device_info.SetStringKey("guid", device.guid);
device_info.SetStringKey("label", device.label);
device_info.SetBoolKey(
"shared", device.shared_vm_name == crostini::kCrostiniDefaultVmName);
device_info.SetKey("guid", base::Value(device.guid));
device_info.SetKey("label", base::Value(device.label));
const bool shared_in_crostini =
device.vm_sharing_info[crostini::kCrostiniDefaultVmName].shared;
device_info.SetKey("shared", base::Value(shared_in_crostini));
usb_devices_list.Append(std::move(device_info));
}
return usb_devices_list;
......
......@@ -27,8 +27,9 @@ base::ListValue UsbDevicesToListValue(
base::Value device_info(base::Value::Type::DICTIONARY);
device_info.SetStringKey("guid", device.guid);
device_info.SetStringKey("label", device.label);
device_info.SetBoolKey("shared",
device.shared_vm_name == plugin_vm::kPluginVmName);
auto it = device.vm_sharing_info.find(plugin_vm::kPluginVmName);
bool shared = it != device.vm_sharing_info.end() && it->second.shared;
device_info.SetBoolKey("shared", shared);
usb_devices_list.Append(std::move(device_info));
}
return usb_devices_list;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment