Cleanup Return object.
- Expose isOk() instead of having to say getStatus().isOk()
- Add 'get' function which checks to make sure content is valid.
- Mark implicit cast operator as deprecated.
Bug: 32744406
Bug: 31348667
Test: hidl_test
Change-Id: I13bfe33b6c1f9b94a372161064a9e913825df959
diff --git a/base/Android.bp b/base/Android.bp
index d990140..f2f5ab0 100644
--- a/base/Android.bp
+++ b/base/Android.bp
@@ -33,6 +33,7 @@
},
srcs: [
+ "HidlInternal.cpp",
"HidlSupport.cpp",
"Status.cpp",
"TaskRunner.cpp",
diff --git a/base/HidlInternal.cpp b/base/HidlInternal.cpp
new file mode 100644
index 0000000..45afda3
--- /dev/null
+++ b/base/HidlInternal.cpp
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#define LOG_TAG "HidlInternal"
+
+#include <hidl/HidlInternal.h>
+
+#include <android-base/logging.h>
+
+namespace android {
+namespace hardware {
+namespace details {
+
+void hidl_log_base::logAlwaysFatal(const char *message) const {
+ LOG(FATAL) << message;
+}
+
+} // namespace details
+} // namespace hardware
+} // namespace android
diff --git a/base/HidlSupport.cpp b/base/HidlSupport.cpp
index 1d87d4c..35e1672 100644
--- a/base/HidlSupport.cpp
+++ b/base/HidlSupport.cpp
@@ -14,8 +14,6 @@
* limitations under the License.
*/
-#define LOG_TAG "HidlSupport"
-
#include <hidl/HidlSupport.h>
#include <android-base/logging.h>
@@ -28,14 +26,6 @@
namespace android {
namespace hardware {
-namespace details {
-
-void hidl_log_base::logAlwaysFatal(const char *message) {
- LOG(FATAL) << message;
-}
-
-} // namespace details
-
static const char *const kEmptyString = "";
hidl_string::hidl_string()
diff --git a/base/include/hidl/HidlInternal.h b/base/include/hidl/HidlInternal.h
new file mode 100644
index 0000000..5739137
--- /dev/null
+++ b/base/include/hidl/HidlInternal.h
@@ -0,0 +1,99 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef ANDROID_HIDL_INTERNAL_H
+#define ANDROID_HIDL_INTERNAL_H
+
+#include <cstdint>
+#include <utility>
+
+namespace android {
+namespace hardware {
+namespace details {
+
+// hidl_log_base is a base class that templatized
+// classes implemented in a header can inherit from,
+// to avoid creating dependencies on liblog.
+struct hidl_log_base {
+ void logAlwaysFatal(const char *message) const;
+};
+
+// HIDL client/server code should *NOT* use this class.
+//
+// hidl_pointer wraps a pointer without taking ownership,
+// and stores it in a union with a uint64_t. This ensures
+// that we always have enough space to store a pointer,
+// regardless of whether we're running in a 32-bit or 64-bit
+// process.
+template<typename T>
+struct hidl_pointer {
+ hidl_pointer()
+ : mPointer(nullptr) {
+ }
+ hidl_pointer(T* ptr)
+ : mPointer(ptr) {
+ }
+ hidl_pointer(const hidl_pointer<T>& other) {
+ mPointer = other.mPointer;
+ }
+ hidl_pointer(hidl_pointer<T>&& other) {
+ *this = std::move(other);
+ }
+
+ hidl_pointer &operator=(const hidl_pointer<T>& other) {
+ mPointer = other.mPointer;
+ return *this;
+ }
+ hidl_pointer &operator=(hidl_pointer<T>&& other) {
+ mPointer = other.mPointer;
+ other.mPointer = nullptr;
+ return *this;
+ }
+ hidl_pointer &operator=(T* ptr) {
+ mPointer = ptr;
+ return *this;
+ }
+
+ operator T*() const {
+ return mPointer;
+ }
+ explicit operator void*() const { // requires explicit cast to avoid ambiguity
+ return mPointer;
+ }
+ T& operator*() const {
+ return *mPointer;
+ }
+ T* operator->() const {
+ return mPointer;
+ }
+ T &operator[](size_t index) {
+ return mPointer[index];
+ }
+ const T &operator[](size_t index) const {
+ return mPointer[index];
+ }
+private:
+ union {
+ T* mPointer;
+ uint64_t _pad;
+ };
+};
+
+} // namespace details
+} // namespace hardware
+} // namespace android
+
+#endif // ANDROID_HIDL_INTERNAL_H
diff --git a/base/include/hidl/HidlSupport.h b/base/include/hidl/HidlSupport.h
index f4a216a..6a1f4e5 100644
--- a/base/include/hidl/HidlSupport.h
+++ b/base/include/hidl/HidlSupport.h
@@ -24,6 +24,7 @@
#include <cutils/native_handle.h>
#include <cutils/properties.h>
#include <functional>
+#include <hidl/HidlInternal.h>
#include <hidl/Status.h>
#include <map>
#include <tuple>
@@ -35,78 +36,6 @@
namespace android {
namespace hardware {
-namespace details {
-
-// hidl_log_base is a base class that templatized
-// classes implemented in a header can inherit from,
-// to avoid creating dependencies on liblog.
-struct hidl_log_base {
- void logAlwaysFatal(const char *message);
-};
-
-// HIDL client/server code should *NOT* use this class.
-//
-// hidl_pointer wraps a pointer without taking ownership,
-// and stores it in a union with a uint64_t. This ensures
-// that we always have enough space to store a pointer,
-// regardless of whether we're running in a 32-bit or 64-bit
-// process.
-template<typename T>
-struct hidl_pointer {
- hidl_pointer()
- : mPointer(nullptr) {
- }
- hidl_pointer(T* ptr)
- : mPointer(ptr) {
- }
- hidl_pointer(const hidl_pointer<T>& other) {
- mPointer = other.mPointer;
- }
- hidl_pointer(hidl_pointer<T>&& other) {
- *this = std::move(other);
- }
-
- hidl_pointer &operator=(const hidl_pointer<T>& other) {
- mPointer = other.mPointer;
- return *this;
- }
- hidl_pointer &operator=(hidl_pointer<T>&& other) {
- mPointer = other.mPointer;
- other.mPointer = nullptr;
- return *this;
- }
- hidl_pointer &operator=(T* ptr) {
- mPointer = ptr;
- return *this;
- }
-
- operator T*() const {
- return mPointer;
- }
- explicit operator void*() const { // requires explicit cast to avoid ambiguity
- return mPointer;
- }
- T& operator*() const {
- return *mPointer;
- }
- T* operator->() const {
- return mPointer;
- }
- T &operator[](size_t index) {
- return mPointer[index];
- }
- const T &operator[](size_t index) const {
- return mPointer[index];
- }
-private:
- union {
- T* mPointer;
- uint64_t _pad;
- };
-};
-} // namespace details
-
-
// hidl_handle wraps a pointer to a native_handle_t in a hidl_pointer,
// so that it can safely be transferred between 32-bit and 64-bit processes.
struct hidl_handle {
diff --git a/base/include/hidl/Status.h b/base/include/hidl/Status.h
index 248d51c..1246ae0 100644
--- a/base/include/hidl/Status.h
+++ b/base/include/hidl/Status.h
@@ -20,8 +20,9 @@
#include <cstdint>
#include <sstream>
-#include <utils/String8.h>
#include <android-base/macros.h>
+#include <hidl/HidlInternal.h>
+#include <utils/String8.h>
namespace android {
namespace hardware {
@@ -140,28 +141,49 @@
// For gtest output logging
std::stringstream& operator<< (std::stringstream& stream, const Status& s);
-template<typename T> class Return {
+template<typename T> class Return : private details::hidl_log_base {
private:
- T val {};
- Status status {};
+ T mVal {};
+ Status mStatus {};
public:
- Return(T v) : val{v} {}
- Return(Status s) : status(s) {}
- operator T() const { return val; }
- const Status& getStatus() const {
- return status;
- }
+ Return(T v) : mVal{v} {}
+ Return(Status s) : mStatus(s) {}
+
+ T get() const {
+ if (!mStatus.isOk()) {
+ logAlwaysFatal("Attempted to retrieve value from hidl service, "
+ "but there was a transport error.");
+ }
+ return mVal;
+ }
+ bool isOk() const {
+ return mStatus.isOk();
+ }
+
+ // TODO(b/31348667) deprecate, remove all usage, remove function
+ // Can't mark as deprecated yet because of all the projects built with -Werror.
+ // [[deprecated("Replaced by get()")]]
+ operator T() const { return mVal; }
+
+ const Status& getStatus() const {
+ return mStatus;
+ }
};
template<> class Return<void> {
private:
- Status status {};
+ Status mStatus {};
public:
- Return() = default;
- Return(Status s) : status(s) {}
- const Status& getStatus() const {
- return status;
- }
+ Return() = default;
+ Return(Status s) : mStatus(s) {}
+
+ bool isOk() const {
+ return mStatus.isOk();
+ }
+
+ const Status& getStatus() const {
+ return mStatus;
+ }
};
static inline Return<void> Void() {
diff --git a/transport/include/hidl/HidlBinderSupport.h b/transport/include/hidl/HidlBinderSupport.h
index b4a2aa2..84ba966 100644
--- a/transport/include/hidl/HidlBinderSupport.h
+++ b/transport/include/hidl/HidlBinderSupport.h
@@ -363,7 +363,9 @@
::android::hardware::Return<void> ret = \
this->interfaceChain( \
[&success, &sm, &serviceName, &binderIface](const auto &chain) { \
- success = sm->add(chain, serviceName.c_str(), binderIface); \
+ ::android::hardware::Return<bool> addRet = \
+ sm->add(chain, serviceName.c_str(), binderIface); \
+ success = addRet.isOk() && addRet.get(); \
}); \
success = success && ret.getStatus().isOk(); \
return success ? ::android::OK : ::android::UNKNOWN_ERROR; \
@@ -380,9 +382,11 @@
if (sm == nullptr) { \
return false; \
} \
- return sm->registerForNotifications(PACKAGE "::I" #INTERFACE, \
- serviceName, \
- notification); \
+ ::android::hardware::Return<bool> success = \
+ sm->registerForNotifications(PACKAGE "::I" #INTERFACE, \
+ serviceName, \
+ notification); \
+ return success.isOk() && success.get(); \
}
