Unregistering HIDL interfaces breaks VTS
Based on some back and forth on the internal IDL discuss group, it
sounds like we can't unregister interfaces anymore if they're defined in
the manifest. The CAN interface behind the HIDL interface will still be
taken down correctly, but this will get rid of the failing attempt to
bring down the HIDL interface.
Bug: 213841914
Bug: 213843613
Test: VtsHalCanBusV1_0TargetTest
Test: VtsHalCanControllerV1_0TargetTest
Test: VtsHalCanBusVirtualV1_0TargetTest
Change-Id: Iae4d6a650c30adacdc7c51140e0295915b516833
(cherry picked from commit d349f1d5213dc14d90016efb5e4934a80888064c)
diff --git a/automotive/can/1.0/default/CanController.cpp b/automotive/can/1.0/default/CanController.cpp
index 9c0f2c5..1b5bf5b 100644
--- a/automotive/can/1.0/default/CanController.cpp
+++ b/automotive/can/1.0/default/CanController.cpp
@@ -292,14 +292,6 @@
return ICanController::Result::OK;
}
-static bool unregisterCanBusService(const hidl_string& name, sp<CanBus> busService) {
- auto manager = hidl::manager::V1_2::IServiceManager::getService();
- if (!manager) return false;
- const auto res = manager->tryUnregister(ICanBus::descriptor, name, busService);
- if (!res.isOk()) return false;
- return res;
-}
-
Return<bool> CanController::downInterface(const hidl_string& name) {
LOG(VERBOSE) << "Attempting to bring interface down: " << name;
@@ -313,12 +305,6 @@
auto success = true;
- if (!unregisterCanBusService(name, busEntry.mapped())) {
- LOG(ERROR) << "Couldn't unregister " << name;
- // don't return yet, let's try to do best-effort cleanup
- success = false;
- }
-
if (!busEntry.mapped()->down()) {
LOG(ERROR) << "Couldn't bring " << name << " down";
success = false;
diff --git a/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp b/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp
index 294cd17..f8ae871 100644
--- a/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp
+++ b/automotive/can/1.0/vts/functional/VtsHalCanControllerV1_0TargetTest.cpp
@@ -33,18 +33,45 @@
using InterfaceType = ICanController::InterfaceType;
using IfId = ICanController::BusConfig::InterfaceId;
+struct Bus {
+ DISALLOW_COPY_AND_ASSIGN(Bus);
+
+ Bus(sp<ICanController> controller, const ICanController::BusConfig& config)
+ : mIfname(config.name), mController(controller) {
+ // Don't bring up the bus, we just need a wrapper for the ICanBus object
+ /* Not using ICanBus::getService here, since it ignores interfaces not in the manifest
+ * file -- this is a test, so we don't want to add fake services to a device manifest. */
+ auto manager = hidl::manager::V1_2::IServiceManager::getService();
+ auto service = manager->get(ICanBus::descriptor, config.name);
+ mBus = ICanBus::castFrom(service);
+ }
+
+ ICanBus* operator->() const { return mBus.get(); }
+ sp<ICanBus> get() { return mBus; }
+
+ Return<Result> send(const CanMessage& msg) { return mBus->send(msg); }
+
+ private:
+ const std::string mIfname;
+ sp<ICanController> mController;
+ sp<ICanBus> mBus;
+};
+
class CanControllerHalTest : public ::testing::TestWithParam<std::string> {
protected:
virtual void SetUp() override;
virtual void TearDown() override;
static void SetUpTestCase();
+ Bus makeBus(const std::string ifaceName);
+
hidl_vec<InterfaceType> getSupportedInterfaceTypes();
bool isSupported(InterfaceType iftype);
bool up(InterfaceType iftype, const std::string srvname, std::string ifname,
ICanController::Result expected);
- void assertRegistered(const std::string srvname, bool expectRegistered);
+ void assertRegistered(const std::string srvname, const std::string ifaceName,
+ bool expectRegistered);
sp<ICanController> mCanController;
static hidl_vec<hidl_string> mBusNames;
@@ -117,16 +144,33 @@
return true;
}
-void CanControllerHalTest::assertRegistered(std::string srvname, bool expectRegistered) {
+void CanControllerHalTest::assertRegistered(const std::string srvname, const std::string ifaceName,
+ bool expectRegistered) {
/* Not using ICanBus::tryGetService here, since it ignores interfaces not in the manifest
* file -- this is a test, so we don't want to add fake services to a device manifest. */
auto manager = hidl::manager::V1_2::IServiceManager::getService();
auto busService = manager->get(ICanBus::descriptor, srvname);
+ if (!expectRegistered) {
+ /* We can't unregister a HIDL interface defined in the manifest, so we'll just check to make
+ * sure that the interface behind it is down */
+ auto bus = makeBus(ifaceName);
+ const auto result = bus->send({});
+ ASSERT_EQ(Result::INTERFACE_DOWN, result);
+ return;
+ }
ASSERT_EQ(expectRegistered, busService.withDefault(nullptr) != nullptr)
<< "ICanBus/" << srvname << (expectRegistered ? " is not " : " is ") << "registered"
<< " (should be otherwise)";
}
+Bus CanControllerHalTest::makeBus(const std::string ifaceName) {
+ ICanController::BusConfig config = {};
+ config.name = mBusNames[0];
+ config.interfaceId.virtualif({ifaceName});
+
+ return Bus(mCanController, config);
+}
+
TEST_P(CanControllerHalTest, SupportsSomething) {
const auto supported = getSupportedInterfaceTypes();
ASSERT_GT(supported.size(), 0u);
@@ -134,15 +178,17 @@
TEST_P(CanControllerHalTest, BringUpDown) {
const std::string name = mBusNames[0];
+ const std::string iface = "vcan57";
+ mCanController->downInterface(name);
- assertRegistered(name, false);
- if (!up(InterfaceType::VIRTUAL, name, "vcan57", ICanController::Result::OK)) GTEST_SKIP();
- assertRegistered(name, true);
+ assertRegistered(name, iface, false);
+
+ if (!up(InterfaceType::VIRTUAL, name, iface, ICanController::Result::OK)) GTEST_SKIP();
+ assertRegistered(name, iface, true);
const auto dnresult = mCanController->downInterface(name);
ASSERT_TRUE(dnresult);
-
- assertRegistered(name, false);
+ assertRegistered(name, iface, false);
}
TEST_P(CanControllerHalTest, DownFake) {
@@ -152,18 +198,19 @@
TEST_P(CanControllerHalTest, UpTwice) {
const std::string name = mBusNames[0];
+ const std::string iface = "vcan72";
- assertRegistered(name, false);
- if (!up(InterfaceType::VIRTUAL, name, "vcan72", ICanController::Result::OK)) GTEST_SKIP();
- assertRegistered(name, true);
+ assertRegistered(name, iface, false);
+ if (!up(InterfaceType::VIRTUAL, name, iface, ICanController::Result::OK)) GTEST_SKIP();
+ assertRegistered(name, iface, true);
if (!up(InterfaceType::VIRTUAL, name, "vcan73", ICanController::Result::INVALID_STATE)) {
GTEST_SKIP();
}
- assertRegistered(name, true);
+ assertRegistered(name, iface, true);
const auto result = mCanController->downInterface(name);
ASSERT_TRUE(result);
- assertRegistered(name, false);
+ assertRegistered(name, iface, false);
}
TEST_P(CanControllerHalTest, ConfigCompatibility) {
@@ -230,63 +277,67 @@
TEST_P(CanControllerHalTest, FailEmptyName) {
const std::string name = "";
+ const std::string iface = "vcan57";
- assertRegistered(name, false);
- if (!up(InterfaceType::VIRTUAL, name, "vcan57", ICanController::Result::BAD_SERVICE_NAME)) {
+ assertRegistered(name, iface, false);
+ if (!up(InterfaceType::VIRTUAL, name, iface, ICanController::Result::BAD_SERVICE_NAME)) {
GTEST_SKIP();
}
- assertRegistered(name, false);
+ assertRegistered(name, iface, false);
}
TEST_P(CanControllerHalTest, FailBadName) {
// 33 characters (name can be at most 32 characters long)
const std::string name = "ab012345678901234567890123456789c";
+ const std::string iface = "vcan57";
- assertRegistered(name, false);
- if (!up(InterfaceType::VIRTUAL, name, "vcan57", ICanController::Result::BAD_SERVICE_NAME)) {
+ assertRegistered(name, iface, false);
+ if (!up(InterfaceType::VIRTUAL, name, iface, ICanController::Result::BAD_SERVICE_NAME)) {
GTEST_SKIP();
}
- assertRegistered(name, false);
+ assertRegistered(name, iface, false);
}
TEST_P(CanControllerHalTest, FailBadVirtualAddress) {
const std::string name = mBusNames[0];
+ const std::string iface = "";
- assertRegistered(name, false);
- if (!up(InterfaceType::VIRTUAL, name, "", ICanController::Result::BAD_INTERFACE_ID)) {
+ assertRegistered(name, iface, false);
+ if (!up(InterfaceType::VIRTUAL, name, iface, ICanController::Result::BAD_INTERFACE_ID)) {
GTEST_SKIP();
}
- assertRegistered(name, false);
+ assertRegistered(name, iface, false);
}
TEST_P(CanControllerHalTest, FailBadSocketcanAddress) {
const std::string name = mBusNames[0];
+ const std::string iface = "can87";
- assertRegistered(name, false);
- if (!up(InterfaceType::SOCKETCAN, name, "can87", ICanController::Result::BAD_INTERFACE_ID)) {
+ assertRegistered(name, iface, false);
+ if (!up(InterfaceType::SOCKETCAN, name, iface, ICanController::Result::BAD_INTERFACE_ID)) {
GTEST_SKIP();
}
- assertRegistered(name, false);
+ assertRegistered(name, iface, false);
auto supported =
up(InterfaceType::SOCKETCAN, name, "", ICanController::Result::BAD_INTERFACE_ID);
ASSERT_TRUE(supported);
- assertRegistered(name, false);
+ assertRegistered(name, iface, false);
}
TEST_P(CanControllerHalTest, FailBadSlcanAddress) {
const std::string name = mBusNames[0];
+ const std::string iface = "/dev/shouldnotexist123";
- assertRegistered(name, false);
- if (!up(InterfaceType::SLCAN, name, "/dev/shouldnotexist123",
- ICanController::Result::BAD_INTERFACE_ID)) {
+ assertRegistered(name, iface, false);
+ if (!up(InterfaceType::SLCAN, name, iface, ICanController::Result::BAD_INTERFACE_ID)) {
GTEST_SKIP();
}
- assertRegistered(name, false);
+ assertRegistered(name, iface, false);
auto supported = up(InterfaceType::SLCAN, name, "", ICanController::Result::BAD_INTERFACE_ID);
ASSERT_TRUE(supported);
- assertRegistered(name, false);
+ assertRegistered(name, iface, false);
}
/**