Merge "Unregistering HIDL interfaces breaks VTS" into tm-dev
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);
}
/**