Merge "libbinder: Handle transaction failures correctly"

am: 823fa55aec

* commit '823fa55aec55684605e6f4c764ec20901066553c':
  libbinder: Handle transaction failures correctly

2 files changed, 59 insertions, 53 deletions

diff --git a/include/binder/Status.h b/include/binder/Status.h
index 04738f8dd1..d19824d894 100644
--- a/include/binder/Status.h
+++ b/include/binder/Status.h
@@ -60,31 +60,31 @@ public:
         EX_ILLEGAL_STATE = -5,
         EX_NETWORK_MAIN_THREAD = -6,
         EX_UNSUPPORTED_OPERATION = -7,
-        EX_TRANSACTION_FAILED = -8,
 
         // This is special and Java specific; see Parcel.java.
         EX_HAS_REPLY_HEADER = -128,
+        // This is special, and indicates to C++ binder proxies that the
+        // transaction has failed at a low level.
+        EX_TRANSACTION_FAILED = -129,
     };
 
+    // A more readable alias for the default constructor.
+    static Status ok();
     // Allow authors to explicitly pick whether their integer is a status_t or
     // exception code.
-    static Status fromExceptionCode(int32_t exception_code);
+    static Status fromExceptionCode(int32_t exceptionCode);
+    static Status fromExceptionCode(int32_t exceptionCode,
+                                    const String8& message);
     static Status fromStatusT(status_t status);
-    // A more readable alias for the default constructor.
-    static Status ok();
 
     Status() = default;
-    Status(int32_t exception_code, const String8& message);
-    Status(int32_t exception_code, const char* message);
-
+    ~Status() = default;
 
     // Status objects are copyable and contain just simple data.
     Status(const Status& status) = default;
     Status(Status&& status) = default;
     Status& operator=(const Status& status) = default;
 
-    ~Status() = default;
-
     // Bear in mind that if the client or service is a Java endpoint, this
     // is not the logic which will provide/interpret the data here.
     status_t readFromParcel(const Parcel& parcel);
@@ -92,16 +92,17 @@ public:
 
     // Set one of the pre-defined exception types defined above.
     void setException(int32_t ex, const String8& message);
-    // A few of the status_t values map to exception codes, but most of them
-    // simply map to "transaction failed."
+    // Setting a |status| != OK causes generated code to return |status|
+    // from Binder transactions, rather than writing an exception into the
+    // reply Parcel.
     void setFromStatusT(status_t status);
 
     // Get information about an exception.
-    // Any argument may be given as nullptr.
-    void getException(int32_t* returned_exception,
-                      String8* returned_message) const;
     int32_t exceptionCode() const  { return mException; }
     const String8& exceptionMessage() const { return mMessage; }
+    status_t transactionError() const {
+        return mException == EX_TRANSACTION_FAILED ? mErrorCode : OK;
+    }
 
     bool isOk() const { return mException == EX_NONE; }
 
@@ -109,9 +110,17 @@ public:
     String8 toString8() const;
 
 private:
-    // We always write |mException| to the parcel.
-    // If |mException| !=  EX_NONE, we write message as well.
+    Status(int32_t exception_code);
+    Status(int32_t exception_code, const String8& message);
+
+    // If |mException| == EX_TRANSACTION_FAILED, generated code will return
+    // |mErrorCode| as the result of the transaction rather than write an
+    // exception to the reply parcel.
+    //
+    // Otherwise, we always write |mException| to the parcel.
+    // If |mException| !=  EX_NONE, we write |mMessage| as well.
     int32_t mException = EX_NONE;
+    int32_t mErrorCode = 0;
     String8 mMessage;
 };  // class Status
 
diff --git a/libs/binder/Status.cpp b/libs/binder/Status.cpp
index 41fff3d4cd..67f0d59669 100644
--- a/libs/binder/Status.cpp
+++ b/libs/binder/Status.cpp
@@ -19,8 +19,17 @@
 namespace android {
 namespace binder {
 
-Status Status::fromExceptionCode(int32_t exception_code) {
-    return Status(exception_code, "");
+Status Status::ok() {
+    return Status();
+}
+
+Status Status::fromExceptionCode(int32_t exceptionCode) {
+    return Status(exceptionCode);
+}
+
+Status Status::fromExceptionCode(int32_t exceptionCode,
+                                 const String8& message) {
+    return Status(exceptionCode, message);
 }
 
 Status Status::fromStatusT(status_t status) {
@@ -29,16 +38,9 @@ Status Status::fromStatusT(status_t status) {
     return ret;
 }
 
-Status Status::ok() {
-    return Status();
-}
-
-Status::Status(int32_t exception_code, const String8& message)
-    : mException(exception_code),
-      mMessage(message) {}
-
-Status::Status(int32_t exception_code, const char* message)
-    : mException(exception_code),
+Status::Status(int32_t exceptionCode) : mException(exceptionCode) {}
+Status::Status(int32_t exceptionCode, const String8& message)
+    : mException(exceptionCode),
       mMessage(message) {}
 
 status_t Status::readFromParcel(const Parcel& parcel) {
@@ -69,12 +71,24 @@ status_t Status::readFromParcel(const Parcel& parcel) {
     }
 
     // The remote threw an exception.  Get the message back.
-    mMessage = String8(parcel.readString16());
+    String16 message;
+    status = parcel.readString16(&message);
+    if (status != OK) {
+        setFromStatusT(status);
+        return status;
+    }
+    mMessage = String8(message);
 
     return status;
 }
 
 status_t Status::writeToParcel(Parcel* parcel) const {
+    // Something really bad has happened, and we're not going to even
+    // try returning rich error data.
+    if (mException == EX_TRANSACTION_FAILED) {
+        return mErrorCode;
+    }
+
     status_t status = parcel->writeInt32(mException);
     if (status != OK) { return status; }
     if (mException == EX_NONE) {
@@ -86,43 +100,26 @@ status_t Status::writeToParcel(Parcel* parcel) const {
 }
 
 void Status::setFromStatusT(status_t status) {
-    switch (status) {
-        case NO_ERROR:
-            mException = EX_NONE;
-            mMessage.clear();
-            break;
-        case UNEXPECTED_NULL:
-            mException = EX_NULL_POINTER;
-            mMessage.setTo("Unexpected null reference in Parcel");
-            break;
-        default:
-            mException = EX_TRANSACTION_FAILED;
-            mMessage.setTo("Transaction failed");
-            break;
-    }
+    mException = (status == NO_ERROR) ? EX_NONE : EX_TRANSACTION_FAILED;
+    mErrorCode = status;
+    mMessage.clear();
 }
 
 void Status::setException(int32_t ex, const String8& message) {
     mException = ex;
+    mErrorCode = NO_ERROR;  // an exception, not a transaction failure.
     mMessage.setTo(message);
 }
 
-void Status::getException(int32_t* returned_exception,
-                          String8* returned_message) const {
-    if (returned_exception) {
-        *returned_exception = mException;
-    }
-    if (returned_message) {
-        returned_message->setTo(mMessage);
-    }
-}
-
 String8 Status::toString8() const {
     String8 ret;
     if (mException == EX_NONE) {
         ret.append("No error");
     } else {
         ret.appendFormat("Status(%d): '", mException);
+        if (mException == EX_TRANSACTION_FAILED) {
+            ret.appendFormat("%d: ", mErrorCode);
+        }
         ret.append(String8(mMessage));
         ret.append("'");
     }