[fdf] Update device groups in driver_index.fidl Update how device groups are represented in driver_index.fidl to match the redesign. Bug: 103188 Tests: Updated existing tests Change-Id: I014a5916d828cd802ea538c619611997a55541cc Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/693423 Reviewed-by: Jocelyn Dang <jocelyndang@google.com> API-Review: Jocelyn Dang <jocelyndang@google.com> Commit-Queue: Sarah Chan <spqchan@google.com> Reviewed-by: David Gilhooley <dgilhooley@google.com>
diff --git a/sdk/fidl/fuchsia.driver.index/driver_index.fidl b/sdk/fidl/fuchsia.driver.index/driver_index.fidl index cae2a40..16ea2c9 100644 --- a/sdk/fidl/fuchsia.driver.index/driver_index.fidl +++ b/sdk/fidl/fuchsia.driver.index/driver_index.fidl
@@ -59,23 +59,19 @@ 5: driver_info MatchedDriverInfo; }; -/// Information for a device group matched by the driver index. +/// Information for a device group that contains a matched node. type MatchedDeviceGroupInfo = table { - /// Topological path for a device group. + /// Topological path of the node's device group. 1: topological_path string:MAX; - /// Node index for the nodes in the device group. + /// The node's index in its device group. 2: node_index uint32; +}; - /// Total number of nodes in the device group. - 3: num_nodes uint32; - - /// A list of all the node names in the device group. These are ordered - /// according to index. - 4: node_names vector<string:MAX>:MAX; - - /// The name of the device group. - 5: composite_name string:MAX; +/// Information for a device group node that's matched by the driver index. +type MatchedDeviceGroupNodeInfo = table { + /// A list of device groups that contain the matched node. + 1: device_groups vector<MatchedDeviceGroupInfo>:MAX; }; /// Driver matched by the driver index. @@ -86,8 +82,8 @@ /// Information for a composite driver. 2: composite_driver MatchedCompositeInfo; - /// Information for a device group. - 3: device_group MatchedDeviceGroupInfo; + /// Information for a device group node. + 3: device_group_node MatchedDeviceGroupNodeInfo; }; /// Protocol through which the driver index can be queried.
diff --git a/src/devices/bin/driver-index/src/device_group.rs b/src/devices/bin/driver-index/src/device_group.rs index af34dee..9f055ec 100644 --- a/src/devices/bin/driver-index/src/device_group.rs +++ b/src/devices/bin/driver-index/src/device_group.rs
@@ -47,17 +47,22 @@ }) } + // TODO(fxb/103206): Return a list of device groups in a MatchedDeviceGroupNode. pub fn matches(&self, properties: &DeviceProperties) -> Option<fdi::MatchedDriver> { for (index, node) in self.nodes.iter().enumerate() { if match_node(&node, properties) { - let node_names = self.nodes.iter().map(|node| node.name.clone()).collect(); - return Some(fdi::MatchedDriver::DeviceGroup(fdi::MatchedDeviceGroupInfo { + let node_info = fdi::MatchedDeviceGroupInfo { topological_path: Some(self.topological_path.clone()), node_index: Some(index as u32), - num_nodes: Some(self.nodes.len() as u32), - node_names: Some(node_names), ..fdi::MatchedDeviceGroupInfo::EMPTY - })); + }; + + return Some(fdi::MatchedDriver::DeviceGroupNode( + fdi::MatchedDeviceGroupNodeInfo { + device_groups: Some(vec![node_info]), + ..fdi::MatchedDeviceGroupNodeInfo::EMPTY + }, + )); } } @@ -207,13 +212,15 @@ Symbol::StringValue("plover".to_string()), ); + let expected_device_group = fdi::MatchedDeviceGroupInfo { + topological_path: Some("test/path".to_string()), + node_index: Some(0), + ..fdi::MatchedDeviceGroupInfo::EMPTY + }; assert_eq!( - Some(fdi::MatchedDriver::DeviceGroup(fdi::MatchedDeviceGroupInfo { - topological_path: Some("test/path".to_string()), - node_index: Some(0), - num_nodes: Some(2), - node_names: Some(vec!["whimbrel".to_string(), "godwit".to_string()]), - ..fdi::MatchedDeviceGroupInfo::EMPTY + Some(fdi::MatchedDriver::DeviceGroupNode(fdi::MatchedDeviceGroupNodeInfo { + device_groups: Some(vec![expected_device_group]), + ..fdi::MatchedDeviceGroupNodeInfo::EMPTY })), device_group.matches(&device_properties_1) ); @@ -231,13 +238,15 @@ Symbol::EnumValue("Moon.Half".to_string()), ); + let expected_device_group_2 = fdi::MatchedDeviceGroupInfo { + topological_path: Some("test/path".to_string()), + node_index: Some(1), + ..fdi::MatchedDeviceGroupInfo::EMPTY + }; assert_eq!( - Some(fdi::MatchedDriver::DeviceGroup(fdi::MatchedDeviceGroupInfo { - topological_path: Some("test/path".to_string()), - node_index: Some(1), - num_nodes: Some(2), - node_names: Some(vec!["whimbrel".to_string(), "godwit".to_string()]), - ..fdi::MatchedDeviceGroupInfo::EMPTY + Some(fdi::MatchedDriver::DeviceGroupNode(fdi::MatchedDeviceGroupNodeInfo { + device_groups: Some(vec![expected_device_group_2]), + ..fdi::MatchedDeviceGroupNodeInfo::EMPTY })), device_group.matches(&device_properties_2) ); @@ -359,13 +368,16 @@ PropertyKey::StringKey("plover".to_string()), Symbol::StringValue("lapwing".to_string()), ); + + let expected_device_group = fdi::MatchedDeviceGroupInfo { + topological_path: Some("test/path".to_string()), + node_index: Some(0), + ..fdi::MatchedDeviceGroupInfo::EMPTY + }; assert_eq!( - Some(fdi::MatchedDriver::DeviceGroup(fdi::MatchedDeviceGroupInfo { - topological_path: Some("test/path".to_string()), - node_index: Some(0), - num_nodes: Some(2), - node_names: Some(vec!["whimbrel".to_string(), "godwit".to_string()]), - ..fdi::MatchedDeviceGroupInfo::EMPTY + Some(fdi::MatchedDriver::DeviceGroupNode(fdi::MatchedDeviceGroupNodeInfo { + device_groups: Some(vec![expected_device_group]), + ..fdi::MatchedDeviceGroupNodeInfo::EMPTY })), device_group.matches(&device_properties_1) ); @@ -374,13 +386,15 @@ let mut device_properties_2: DeviceProperties = HashMap::new(); device_properties_2.insert(PropertyKey::NumberKey(5), Symbol::NumberValue(20)); + let expected_device_group_2 = fdi::MatchedDeviceGroupInfo { + topological_path: Some("test/path".to_string()), + node_index: Some(1), + ..fdi::MatchedDeviceGroupInfo::EMPTY + }; assert_eq!( - Some(fdi::MatchedDriver::DeviceGroup(fdi::MatchedDeviceGroupInfo { - topological_path: Some("test/path".to_string()), - node_index: Some(1), - num_nodes: Some(2), - node_names: Some(vec!["whimbrel".to_string(), "godwit".to_string()]), - ..fdi::MatchedDeviceGroupInfo::EMPTY + Some(fdi::MatchedDriver::DeviceGroupNode(fdi::MatchedDeviceGroupNodeInfo { + device_groups: Some(vec![expected_device_group_2]), + ..fdi::MatchedDeviceGroupNodeInfo::EMPTY })), device_group.matches(&device_properties_2) );
diff --git a/src/devices/bin/driver-index/src/main.rs b/src/devices/bin/driver-index/src/main.rs index d39b13c..9d6fc29 100644 --- a/src/devices/bin/driver-index/src/main.rs +++ b/src/devices/bin/driver-index/src/main.rs
@@ -2151,12 +2151,13 @@ let result = proxy.match_driver(match_args).await.unwrap().unwrap(); assert_eq!( - fdi::MatchedDriver::DeviceGroup(fdi::MatchedDeviceGroupInfo { - topological_path: Some("test/path".to_string()), - node_index: Some(0), - num_nodes: Some(1), - node_names: Some(vec!["whimbrel".to_string()]), - ..fdi::MatchedDeviceGroupInfo::EMPTY + fdi::MatchedDriver::DeviceGroupNode(fdi::MatchedDeviceGroupNodeInfo { + device_groups: Some(vec![fdi::MatchedDeviceGroupInfo { + topological_path: Some("test/path".to_string()), + node_index: Some(0), + ..fdi::MatchedDeviceGroupInfo::EMPTY + }]), + ..fdi::MatchedDeviceGroupNodeInfo::EMPTY }), result ); @@ -2263,6 +2264,7 @@ let result = proxy.match_drivers_v1(match_args).await.unwrap().unwrap(); assert_eq!(2, result.len()); + assert_eq!( vec![ fdi::MatchedDriver::Driver(fdi::MatchedDriverInfo { @@ -2274,12 +2276,13 @@ is_fallback: Some(false), ..fdi::MatchedDriverInfo::EMPTY }), - fdi::MatchedDriver::DeviceGroup(fdi::MatchedDeviceGroupInfo { - topological_path: Some("test/path".to_string()), - node_index: Some(0), - num_nodes: Some(1), - node_names: Some(vec!["whimbrel".to_string()]), - ..fdi::MatchedDeviceGroupInfo::EMPTY + fdi::MatchedDriver::DeviceGroupNode(fdi::MatchedDeviceGroupNodeInfo { + device_groups: Some(vec![fdi::MatchedDeviceGroupInfo { + topological_path: Some("test/path".to_string()), + node_index: Some(0), + ..fdi::MatchedDeviceGroupInfo::EMPTY + }]), + ..fdi::MatchedDeviceGroupNodeInfo::EMPTY }) ], result
diff --git a/src/devices/bin/driver_manager/bind_driver_manager.cc b/src/devices/bin/driver_manager/bind_driver_manager.cc index de3baae..05dffb3 100644 --- a/src/devices/bin/driver_manager/bind_driver_manager.cc +++ b/src/devices/bin/driver_manager/bind_driver_manager.cc
@@ -337,7 +337,7 @@ // Bind the matched fragment to the device. auto& composite = device_groups_[path]; - auto status = composite->BindFragment(device_group.composite.node, dev); + auto status = composite->BindFragment(device_group.node_index, dev); if (status != ZX_OK) { LOGF(ERROR, "Failed to BindFragment for '%.*s': %s", static_cast<uint32_t>(dev->name().size()), dev->name().data(), zx_status_get_string(status));
diff --git a/src/devices/bin/driver_manager/driver.h b/src/devices/bin/driver_manager/driver.h index 4e72500..a6f6a26 100644 --- a/src/devices/bin/driver_manager/driver.h +++ b/src/devices/bin/driver_manager/driver.h
@@ -39,7 +39,7 @@ struct MatchedDeviceGroupInfo { std::string topological_path; - MatchedCompositeDevice composite; + uint32_t node_index; }; using MatchedDriver =
diff --git a/src/devices/bin/driver_manager/driver_loader.cc b/src/devices/bin/driver_manager/driver_loader.cc index acd22c2..36e0622 100644 --- a/src/devices/bin/driver_manager/driver_loader.cc +++ b/src/devices/bin/driver_manager/driver_loader.cc
@@ -63,7 +63,7 @@ } zx::status<fdi::wire::MatchedDriverInfo> GetFidlMatchedDriverInfo(fdi::wire::MatchedDriver driver) { - if (driver.is_device_group()) { + if (driver.is_device_group_node()) { return zx::error(ZX_ERR_NOT_FOUND); } @@ -105,41 +105,30 @@ return composite; } -// TODO(fxb/95268): Move common composite device fields in driver_index.fidl -// into a separate struct. This will allow us to reduce the duplicate code -// between CreateMatchedDeviceGroupInfo() and CreateMatchedCompositeDevice(). zx::status<MatchedDeviceGroupInfo> CreateMatchedDeviceGroupInfo( - fdi::wire::MatchedDeviceGroupInfo device_group_info) { - if (!device_group_info.has_topological_path()) { + fdi::wire::MatchedDeviceGroupNodeInfo device_group_node_info) { + // Currently we only support only one device group in the list. + // TODO(fxb/103208): Update the DFv1 device group implementation so it + // can handle multiple device groups. + if (!device_group_node_info.has_device_groups() || + device_group_node_info.device_groups().count() != 1) { return zx::error(ZX_ERR_INVALID_ARGS); } - MatchedCompositeDevice composite = {}; - if (device_group_info.has_num_nodes()) { - composite.num_nodes = device_group_info.num_nodes(); + auto device_group = device_group_node_info.device_groups().at(0); + + if (!device_group.has_topological_path()) { + return zx::error(ZX_ERR_INVALID_ARGS); } - if (device_group_info.has_node_index()) { - composite.node = device_group_info.node_index(); - } - - if (device_group_info.has_composite_name()) { - composite.name = std::string(device_group_info.composite_name().data(), - device_group_info.composite_name().size()); - } - - if (device_group_info.has_node_names()) { - std::vector<std::string> names; - for (auto& name : device_group_info.node_names()) { - names.push_back(std::string(name.data(), name.size())); - } - composite.node_names = std::move(names); + if (!device_group.has_node_index()) { + return zx::error(ZX_ERR_INVALID_ARGS); } return zx::ok(MatchedDeviceGroupInfo{ - .topological_path = std::string(device_group_info.topological_path().data(), - device_group_info.topological_path().size()), - .composite = composite, + .topological_path = std::string(device_group.topological_path().data(), + device_group.topological_path().size()), + .node_index = device_group.node_index(), }); } @@ -408,8 +397,8 @@ const auto& drivers = result->value()->drivers; for (auto driver : drivers) { - if (driver.is_device_group()) { - auto device_group = CreateMatchedDeviceGroupInfo(driver.device_group()); + if (driver.is_device_group_node()) { + auto device_group = CreateMatchedDeviceGroupInfo(driver.device_group_node()); if (device_group.is_error()) { LOGF(ERROR, "DriverIndex: MatchDriverV1 response is missing fields in MatchedDeviceGroupInfo");
diff --git a/src/devices/bin/driver_manager/driver_loader_test.cc b/src/devices/bin/driver_manager/driver_loader_test.cc index 8c9304f..c1e86e944 100644 --- a/src/devices/bin/driver_manager/driver_loader_test.cc +++ b/src/devices/bin/driver_manager/driver_loader_test.cc
@@ -77,8 +77,8 @@ } for (auto& device_group : device_groups) { - drivers[index] = fdi::wire::MatchedDriver::WithDeviceGroup( - fidl::ObjectView<fdi::wire::MatchedDeviceGroupInfo>(allocator, device_group)); + drivers[index] = fdi::wire::MatchedDriver::WithDeviceGroupNode( + fidl::ObjectView<fdi::wire::MatchedDeviceGroupNodeInfo>(allocator, device_group)); index++; } @@ -91,7 +91,7 @@ } std::vector<FakeDriver> fake_drivers; - std::vector<fdi::wire::MatchedDeviceGroupInfo> device_groups; + std::vector<fdi::wire::MatchedDeviceGroupNodeInfo> device_groups; }; class DriverLoaderTest : public zxtest::Test { @@ -310,35 +310,34 @@ fidl::Arena allocator; // Add first device group. - fidl::VectorView<fidl::StringView> node_names_1(allocator, 2); - node_names_1[0] = fidl::StringView(allocator, "whimbrel"); - node_names_1[1] = fidl::StringView(allocator, "curlew"); - auto device_group_1 = fdi::wire::MatchedDeviceGroupInfo(allocator); + device_group_1.set_node_index(1); device_group_1.set_topological_path( fidl::ObjectView<fidl::StringView>(allocator, allocator, "test/path/device_group_1")); - device_group_1.set_composite_name( - fidl::ObjectView<fidl::StringView>(allocator, allocator, "device_group_1")); - device_group_1.set_node_names( - fidl::ObjectView<fidl::VectorView<fidl::StringView>>(allocator, node_names_1)); - device_group_1.set_num_nodes(2); - device_group_1.set_node_index(1); - driver_index_server.device_groups.push_back(device_group_1); + + fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo> device_groups_1(allocator, 1); + device_groups_1[0] = device_group_1; + + auto device_group_node_1 = fdi::wire::MatchedDeviceGroupNodeInfo(allocator); + device_group_node_1.set_device_groups( + fidl::ObjectView<fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo>>(allocator, + device_groups_1)); + driver_index_server.device_groups.push_back(device_group_node_1); // Add second device group. - fidl::VectorView<fidl::StringView> node_names_2(allocator, 1); - node_names_2[0] = fidl::StringView(allocator, "sanderling"); - auto device_group_2 = fdi::wire::MatchedDeviceGroupInfo(allocator); + device_group_2.set_node_index(0); device_group_2.set_topological_path( fidl::ObjectView<fidl::StringView>(allocator, allocator, "test/path/device_group_2")); - device_group_2.set_composite_name( - fidl::ObjectView<fidl::StringView>(allocator, allocator, "device_group_2")); - device_group_2.set_node_names( - fidl::ObjectView<fidl::VectorView<fidl::StringView>>(allocator, node_names_1)); - device_group_2.set_num_nodes(1); - device_group_2.set_node_index(0); - driver_index_server.device_groups.push_back(device_group_2); + + fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo> device_groups_2(allocator, 1); + device_groups_2[0] = device_group_2; + + auto device_group_node_2 = fdi::wire::MatchedDeviceGroupNodeInfo(allocator); + device_group_node_2.set_device_groups( + fidl::ObjectView<fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo>>(allocator, + device_groups_2)); + driver_index_server.device_groups.push_back(device_group_node_2); DriverLoader driver_loader(nullptr, std::move(driver_index), &resolver, loop.dispatcher(), false, nullptr); @@ -352,36 +351,31 @@ auto device_group_result_1 = std::get<MatchedDeviceGroupInfo>(drivers[0]); ASSERT_STREQ("test/path/device_group_1", device_group_result_1.topological_path); - ASSERT_EQ(1, device_group_result_1.composite.node); - ASSERT_EQ(2, device_group_result_1.composite.num_nodes); - ASSERT_STREQ("device_group_1", device_group_result_1.composite.name); + ASSERT_EQ(1, device_group_result_1.node_index); auto device_group_result_2 = std::get<MatchedDeviceGroupInfo>(drivers[1]); ASSERT_STREQ("test/path/device_group_2", device_group_result_2.topological_path); - ASSERT_EQ(0, device_group_result_2.composite.node); - ASSERT_EQ(1, device_group_result_2.composite.num_nodes); - ASSERT_STREQ("device_group_2", device_group_result_2.composite.name); + ASSERT_EQ(0, device_group_result_2.node_index); } TEST_F(DriverLoaderTest, TestReturnDriversAndDeviceGroups) { fidl::Arena allocator; - fidl::VectorView<fidl::StringView> node_names(allocator, 2); - node_names[0] = fidl::StringView(allocator, "whimbrel"); - node_names[1] = fidl::StringView(allocator, "curlew"); - auto device_group = fdi::wire::MatchedDeviceGroupInfo(allocator); + device_group.set_node_index(1); device_group.set_topological_path( fidl::ObjectView<fidl::StringView>(allocator, allocator, "test/path/device_group")); - device_group.set_composite_name( - fidl::ObjectView<fidl::StringView>(allocator, allocator, "device_group")); - device_group.set_node_names( - fidl::ObjectView<fidl::VectorView<fidl::StringView>>(allocator, node_names)); - device_group.set_num_nodes(2); - device_group.set_node_index(1); + + fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo> device_groups(allocator, 1); + device_groups[0] = device_group; + + auto device_group_node = fdi::wire::MatchedDeviceGroupNodeInfo(allocator); + device_group_node.set_device_groups( + fidl::ObjectView<fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo>>(allocator, + device_groups)); auto driver_name = "fuchsia_boot:///#driver.so"; - driver_index_server.device_groups.push_back(device_group); + driver_index_server.device_groups.push_back(device_group_node); driver_index_server.fake_drivers.emplace_back(driver_name, fdi::wire::DriverPackageType::kBoot); auto driver = std::make_unique<Driver>(); @@ -404,23 +398,76 @@ // Check device group. auto device_group_result = std::get<MatchedDeviceGroupInfo>(drivers[1]); ASSERT_STREQ("test/path/device_group", device_group_result.topological_path); - ASSERT_EQ(1, device_group_result.composite.node); - ASSERT_EQ(2, device_group_result.composite.num_nodes); - ASSERT_STREQ("device_group", device_group_result.composite.name); + ASSERT_EQ(1, device_group_result.node_index); } TEST_F(DriverLoaderTest, TestReturnDeviceGroupNoTopologicalPath) { fidl::Arena allocator; - fidl::VectorView<fidl::StringView> node_names(allocator, 2); - node_names[0] = fidl::StringView(allocator, "whimbrel"); - auto device_group = fdi::wire::MatchedDeviceGroupInfo(allocator); - device_group.set_composite_name( - fidl::ObjectView<fidl::StringView>(allocator, allocator, "device_group")); - device_group.set_num_nodes(1); - device_group.set_node_index(0); - driver_index_server.device_groups.push_back(device_group); + device_group.set_node_index(1); + + fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo> device_groups(allocator, 1); + device_groups[0] = device_group; + + auto device_group_node = fdi::wire::MatchedDeviceGroupNodeInfo(allocator); + device_group_node.set_device_groups( + fidl::ObjectView<fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo>>(allocator, + device_groups)); + driver_index_server.device_groups.push_back(device_group_node); + + DriverLoader driver_loader(nullptr, std::move(driver_index), &resolver, loop.dispatcher(), false, + nullptr); + loop.StartThread("fidl-thread"); + + DriverLoader::MatchDeviceConfig config; + fidl::VectorView<fdf::wire::NodeProperty> props{}; + auto drivers = driver_loader.MatchPropertiesDriverIndex(props, config); + ASSERT_EQ(drivers.size(), 0); +} + +TEST_F(DriverLoaderTest, TestReturnDeviceGroupNoNodes) { + fidl::Arena allocator; + + fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo> device_groups(allocator, 0); + auto device_group_node = fdi::wire::MatchedDeviceGroupNodeInfo(allocator); + device_group_node.set_device_groups( + fidl::ObjectView<fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo>>(allocator, + device_groups)); + driver_index_server.device_groups.push_back(device_group_node); + + DriverLoader driver_loader(nullptr, std::move(driver_index), &resolver, loop.dispatcher(), false, + nullptr); + loop.StartThread("fidl-thread"); + + DriverLoader::MatchDeviceConfig config; + fidl::VectorView<fdf::wire::NodeProperty> props{}; + auto drivers = driver_loader.MatchPropertiesDriverIndex(props, config); + ASSERT_EQ(drivers.size(), 0); +} + +TEST_F(DriverLoaderTest, TestReturnDeviceGroupMultipleNodes) { + fidl::Arena allocator; + + auto device_group_1 = fdi::wire::MatchedDeviceGroupInfo(allocator); + device_group_1.set_node_index(1); + device_group_1.set_topological_path( + fidl::ObjectView<fidl::StringView>(allocator, allocator, "test/path/device_group_1")); + + auto device_group_2 = fdi::wire::MatchedDeviceGroupInfo(allocator); + device_group_2.set_node_index(3); + device_group_2.set_topological_path( + fidl::ObjectView<fidl::StringView>(allocator, allocator, "test/path/device_group_2")); + + fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo> device_groups(allocator, 2); + device_groups[0] = device_group_1; + device_groups[1] = device_group_2; + + auto device_group_node = fdi::wire::MatchedDeviceGroupNodeInfo(allocator); + device_group_node.set_device_groups( + fidl::ObjectView<fidl::VectorView<fdi::wire::MatchedDeviceGroupInfo>>(allocator, + device_groups)); + driver_index_server.device_groups.push_back(device_group_node); DriverLoader driver_loader(nullptr, std::move(driver_index), &resolver, loop.dispatcher(), false, nullptr);