From 8d5b93eacc63a84e6a359b52973904dc742006d0 Mon Sep 17 00:00:00 2001 From: Ta-Wei Tu Date: Tue, 14 Sep 2021 16:08:40 +0800 Subject: [PATCH] [binder] Clean up parcel interfaces (#27322) Some changes: * OnTransactCb now takes a non-const ReadableParcel* so that testing codes no longer have to rely on mutable. * Remove GetReadableParcel() interface from binder since we only sent one-way transaction and the output (readable) parcel is never used. * Remove GetDataPosition() / SetDataPosition() interfaces since they are both unused. * Some changes that should've been made to #27257 but was somehow missing... --- .../transport/binder/server/binder_server.cc | 10 ++-- .../ext/transport/binder/wire_format/binder.h | 16 ++---- .../binder/wire_format/binder_android.cc | 38 ++++++------- .../binder/wire_format/binder_android.h | 24 +++----- .../binder/wire_format/wire_reader_impl.cc | 16 ++---- .../binder/wire_format/wire_reader_impl.h | 6 +- .../transport/binder/end2end/fake_binder.cc | 44 +++++--------- .../transport/binder/end2end/fake_binder.h | 27 ++++----- .../binder/end2end/fake_binder_test.cc | 57 ++++++------------- .../binder/end2end/testing_channel_create.cc | 2 +- test/core/transport/binder/mock_objects.cc | 2 - test/core/transport/binder/mock_objects.h | 16 ++---- 12 files changed, 89 insertions(+), 169 deletions(-) diff --git a/src/core/ext/transport/binder/server/binder_server.cc b/src/core/ext/transport/binder/server/binder_server.cc index dae27d31a1e..f8c070af7d2 100644 --- a/src/core/ext/transport/binder/server/binder_server.cc +++ b/src/core/ext/transport/binder/server/binder_server.cc @@ -98,10 +98,10 @@ class BinderServerListener : public Server::ListenerInterface { void Start(Server* /*server*/, const std::vector* /*pollsets*/) override { - tx_receiver_ = factory_([this](transaction_code_t code, - const grpc_binder::ReadableParcel* parcel) { - return OnSetupTransport(code, parcel); - }); + tx_receiver_ = factory_( + [this](transaction_code_t code, grpc_binder::ReadableParcel* parcel) { + return OnSetupTransport(code, parcel); + }); endpoint_binder_ = tx_receiver_->GetRawBinder(); grpc_add_endpoint_binder(addr_, endpoint_binder_); } @@ -127,7 +127,7 @@ class BinderServerListener : public Server::ListenerInterface { private: absl::Status OnSetupTransport(transaction_code_t code, - const grpc_binder::ReadableParcel* parcel) { + grpc_binder::ReadableParcel* parcel) { grpc_core::ExecCtx exec_ctx; if (grpc_binder::BinderTransportTxCode(code) != grpc_binder::BinderTransportTxCode::SETUP_TRANSPORT) { diff --git a/src/core/ext/transport/binder/wire_format/binder.h b/src/core/ext/transport/binder/wire_format/binder.h index ead21c7fd67..2b495cbae99 100644 --- a/src/core/ext/transport/binder/wire_format/binder.h +++ b/src/core/ext/transport/binder/wire_format/binder.h @@ -44,9 +44,7 @@ class Binder; class WritableParcel { public: virtual ~WritableParcel() = default; - virtual int32_t GetDataPosition() const = 0; virtual int32_t GetDataSize() const = 0; - virtual absl::Status SetDataPosition(int32_t pos) = 0; virtual absl::Status WriteInt32(int32_t data) = 0; virtual absl::Status WriteInt64(int64_t data) = 0; virtual absl::Status WriteBinder(HasRawBinder* binder) = 0; @@ -69,18 +67,17 @@ class ReadableParcel { public: virtual ~ReadableParcel() = default; virtual int32_t GetDataSize() const = 0; - virtual absl::Status ReadInt32(int32_t* data) const = 0; - virtual absl::Status ReadInt64(int64_t* data) const = 0; - virtual absl::Status ReadBinder(std::unique_ptr* data) const = 0; - // TODO(waynetu): Provide better interfaces. - virtual absl::Status ReadByteArray(std::string* data) const = 0; - virtual absl::Status ReadString(std::string* str) const = 0; + virtual absl::Status ReadInt32(int32_t* data) = 0; + virtual absl::Status ReadInt64(int64_t* data) = 0; + virtual absl::Status ReadBinder(std::unique_ptr* data) = 0; + virtual absl::Status ReadByteArray(std::string* data) = 0; + virtual absl::Status ReadString(std::string* str) = 0; }; class TransactionReceiver : public HasRawBinder { public: using OnTransactCb = - std::function; + std::function; ~TransactionReceiver() override = default; }; @@ -96,7 +93,6 @@ class Binder : public HasRawBinder { virtual absl::Status Transact(BinderTransportTxCode tx_code) = 0; virtual WritableParcel* GetWritableParcel() const = 0; - virtual ReadableParcel* GetReadableParcel() const = 0; // TODO(waynetu): Can we decouple the receiver from the binder? virtual std::unique_ptr ConstructTxReceiver( diff --git a/src/core/ext/transport/binder/wire_format/binder_android.cc b/src/core/ext/transport/binder/wire_format/binder_android.cc index c1f300a7f81..12190227831 100644 --- a/src/core/ext/transport/binder/wire_format/binder_android.cc +++ b/src/core/ext/transport/binder/wire_format/binder_android.cc @@ -215,11 +215,16 @@ absl::Status BinderAndroid::PrepareTransaction() { absl::Status BinderAndroid::Transact(BinderTransportTxCode tx_code) { AIBinder* binder = binder_.get(); - return AIBinder_transact(binder, static_cast(tx_code), - &input_parcel_->parcel_, &output_parcel_->parcel_, - FLAG_ONEWAY) == STATUS_OK - ? absl::OkStatus() - : absl::InternalError("AIBinder_transact failed"); + // We only do one-way transaction and thus the output parcel is never used. + AParcel* unused_output_parcel; + absl::Status result = + (AIBinder_transact(binder, static_cast(tx_code), + &input_parcel_->parcel_, &unused_output_parcel, + FLAG_ONEWAY) == STATUS_OK) + ? absl::OkStatus() + : absl::InternalError("AIBinder_transact failed"); + AParcel_delete(unused_output_parcel); + return result; } std::unique_ptr BinderAndroid::ConstructTxReceiver( @@ -229,10 +234,6 @@ std::unique_ptr BinderAndroid::ConstructTxReceiver( transact_cb); } -int32_t WritableParcelAndroid::GetDataPosition() const { - return AParcel_getDataPosition(parcel_); -} - int32_t WritableParcelAndroid::GetDataSize() const { if (AParcel_getDataSize) { return AParcel_getDataSize(parcel_); @@ -242,12 +243,6 @@ int32_t WritableParcelAndroid::GetDataSize() const { } } -absl::Status WritableParcelAndroid::SetDataPosition(int32_t pos) { - return AParcel_setDataPosition(parcel_, pos) == STATUS_OK - ? absl::OkStatus() - : absl::InternalError("AParcel_setDataPosition failed"); -} - absl::Status WritableParcelAndroid::WriteInt32(int32_t data) { return AParcel_writeInt32(parcel_, data) == STATUS_OK ? absl::OkStatus() @@ -286,24 +281,23 @@ int32_t ReadableParcelAndroid::GetDataSize() const { return AParcel_getDataSize(parcel_); } else { gpr_log(GPR_INFO, "[Warning] AParcel_getDataSize is not available"); - return -1; + return 0; } } -absl::Status ReadableParcelAndroid::ReadInt32(int32_t* data) const { +absl::Status ReadableParcelAndroid::ReadInt32(int32_t* data) { return AParcel_readInt32(parcel_, data) == STATUS_OK ? absl::OkStatus() : absl::InternalError("AParcel_readInt32 failed"); } -absl::Status ReadableParcelAndroid::ReadInt64(int64_t* data) const { +absl::Status ReadableParcelAndroid::ReadInt64(int64_t* data) { return AParcel_readInt64(parcel_, data) == STATUS_OK ? absl::OkStatus() : absl::InternalError("AParcel_readInt64 failed"); } -absl::Status ReadableParcelAndroid::ReadBinder( - std::unique_ptr* data) const { +absl::Status ReadableParcelAndroid::ReadBinder(std::unique_ptr* data) { AIBinder* binder; if (AParcel_readStrongBinder(parcel_, &binder) != STATUS_OK) { *data = nullptr; @@ -313,7 +307,7 @@ absl::Status ReadableParcelAndroid::ReadBinder( return absl::OkStatus(); } -absl::Status ReadableParcelAndroid::ReadByteArray(std::string* data) const { +absl::Status ReadableParcelAndroid::ReadByteArray(std::string* data) { std::vector vec; if (AParcelReadVector(parcel_, &vec) == STATUS_OK) { data->resize(vec.size()); @@ -325,7 +319,7 @@ absl::Status ReadableParcelAndroid::ReadByteArray(std::string* data) const { return absl::InternalError("AParcel_readByteArray failed"); } -absl::Status ReadableParcelAndroid::ReadString(std::string* str) const { +absl::Status ReadableParcelAndroid::ReadString(std::string* str) { return AParcelReadString(parcel_, str) == STATUS_OK ? absl::OkStatus() : absl::InternalError("AParcel_readString failed"); diff --git a/src/core/ext/transport/binder/wire_format/binder_android.h b/src/core/ext/transport/binder/wire_format/binder_android.h index 68ff764803d..27030fb1959 100644 --- a/src/core/ext/transport/binder/wire_format/binder_android.h +++ b/src/core/ext/transport/binder/wire_format/binder_android.h @@ -44,9 +44,7 @@ class WritableParcelAndroid final : public WritableParcel { explicit WritableParcelAndroid(AParcel* parcel) : parcel_(parcel) {} ~WritableParcelAndroid() override = default; - int32_t GetDataPosition() const override; int32_t GetDataSize() const override; - absl::Status SetDataPosition(int32_t pos) override; absl::Status WriteInt32(int32_t data) override; absl::Status WriteInt64(int64_t data) override; absl::Status WriteBinder(HasRawBinder* binder) override; @@ -63,19 +61,18 @@ class ReadableParcelAndroid final : public ReadableParcel { public: ReadableParcelAndroid() = default; // TODO(waynetu): Get rid of the const_cast. - explicit ReadableParcelAndroid(const AParcel* parcel) - : parcel_(const_cast(parcel)) {} + explicit ReadableParcelAndroid(const AParcel* parcel) : parcel_(parcel) {} ~ReadableParcelAndroid() override = default; int32_t GetDataSize() const override; - absl::Status ReadInt32(int32_t* data) const override; - absl::Status ReadInt64(int64_t* data) const override; - absl::Status ReadBinder(std::unique_ptr* data) const override; - absl::Status ReadByteArray(std::string* data) const override; - absl::Status ReadString(std::string* str) const override; + absl::Status ReadInt32(int32_t* data) override; + absl::Status ReadInt64(int64_t* data) override; + absl::Status ReadBinder(std::unique_ptr* data) override; + absl::Status ReadByteArray(std::string* data) override; + absl::Status ReadString(std::string* str) override; private: - AParcel* parcel_ = nullptr; + const AParcel* parcel_ = nullptr; friend class BinderAndroid; }; @@ -84,8 +81,7 @@ class BinderAndroid final : public Binder { public: explicit BinderAndroid(ndk::SpAIBinder binder) : binder_(binder), - input_parcel_(absl::make_unique()), - output_parcel_(absl::make_unique()) {} + input_parcel_(absl::make_unique()) {} ~BinderAndroid() override = default; void* GetRawBinder() override { return binder_.get(); } @@ -97,9 +93,6 @@ class BinderAndroid final : public Binder { WritableParcel* GetWritableParcel() const override { return input_parcel_.get(); } - ReadableParcel* GetReadableParcel() const override { - return output_parcel_.get(); - }; std::unique_ptr ConstructTxReceiver( grpc_core::RefCountedPtr wire_reader_ref, @@ -108,7 +101,6 @@ class BinderAndroid final : public Binder { private: ndk::SpAIBinder binder_; std::unique_ptr input_parcel_; - std::unique_ptr output_parcel_; }; class TransactionReceiverAndroid final : public TransactionReceiver { diff --git a/src/core/ext/transport/binder/wire_format/wire_reader_impl.cc b/src/core/ext/transport/binder/wire_format/wire_reader_impl.cc index 427368b54bc..39507979cc6 100644 --- a/src/core/ext/transport/binder/wire_format/wire_reader_impl.cc +++ b/src/core/ext/transport/binder/wire_format/wire_reader_impl.cc @@ -40,7 +40,7 @@ namespace grpc_binder { namespace { -absl::StatusOr parse_metadata(const ReadableParcel* reader) { +absl::StatusOr parse_metadata(ReadableParcel* reader) { int num_header; RETURN_IF_ERROR(reader->ReadInt32(&num_header)); gpr_log(GPR_INFO, "num_header = %d", num_header); @@ -109,14 +109,9 @@ void WireReaderImpl::SendSetupTransport(Binder* binder) { gpr_log(GPR_INFO, "prepare transaction = %d", binder->PrepareTransaction().ok()); WritableParcel* writable_parcel = binder->GetWritableParcel(); - gpr_log(GPR_INFO, "data position = %d", writable_parcel->GetDataPosition()); - // gpr_log(GPR_INFO, "set data position to 0 = %d", - // writer->SetDataPosition(0)); - gpr_log(GPR_INFO, "data position = %d", writable_parcel->GetDataPosition()); int32_t version = 77; gpr_log(GPR_INFO, "write int32 = %d", writable_parcel->WriteInt32(version).ok()); - gpr_log(GPR_INFO, "data position = %d", writable_parcel->GetDataPosition()); // The lifetime of the transaction receiver is the same as the wire writer's. // The transaction receiver is responsible for not calling the on-transact // callback when it's dead. @@ -125,7 +120,7 @@ void WireReaderImpl::SendSetupTransport(Binder* binder) { // callback owns a Ref() when it's being invoked. tx_receiver_ = binder->ConstructTxReceiver( /*wire_reader_ref=*/Ref(), - [this](transaction_code_t code, const ReadableParcel* readable_parcel) { + [this](transaction_code_t code, ReadableParcel* readable_parcel) { return this->ProcessTransaction(code, readable_parcel); }); @@ -146,7 +141,7 @@ std::unique_ptr WireReaderImpl::RecvSetupTransport() { } absl::Status WireReaderImpl::ProcessTransaction(transaction_code_t code, - const ReadableParcel* parcel) { + ReadableParcel* parcel) { gpr_log(GPR_INFO, __func__); gpr_log(GPR_INFO, "tx code = %u", code); if (code >= static_cast(kFirstCallId)) { @@ -225,7 +220,7 @@ absl::Status WireReaderImpl::ProcessTransaction(transaction_code_t code, } absl::Status WireReaderImpl::ProcessStreamingTransaction( - transaction_code_t code, const ReadableParcel* parcel) { + transaction_code_t code, ReadableParcel* parcel) { grpc_core::MutexLock lock(&mu_); if (!connected_) { return absl::InvalidArgumentError("Transports not connected yet"); @@ -267,8 +262,7 @@ absl::Status WireReaderImpl::ProcessStreamingTransaction( } absl::Status WireReaderImpl::ProcessStreamingTransactionImpl( - transaction_code_t code, const ReadableParcel* parcel, - int* cancellation_flags) { + transaction_code_t code, ReadableParcel* parcel, int* cancellation_flags) { GPR_ASSERT(cancellation_flags); num_incoming_bytes_ += parcel->GetDataSize(); diff --git a/src/core/ext/transport/binder/wire_format/wire_reader_impl.h b/src/core/ext/transport/binder/wire_format/wire_reader_impl.h index db1f7d53e62..d89a248c88b 100644 --- a/src/core/ext/transport/binder/wire_format/wire_reader_impl.h +++ b/src/core/ext/transport/binder/wire_format/wire_reader_impl.h @@ -67,7 +67,7 @@ class WireReaderImpl : public WireReader { std::unique_ptr binder) override; absl::Status ProcessTransaction(transaction_code_t code, - const ReadableParcel* parcel); + ReadableParcel* parcel); /// Send SETUP_TRANSPORT request through \p binder. /// @@ -93,9 +93,9 @@ class WireReaderImpl : public WireReader { private: absl::Status ProcessStreamingTransaction(transaction_code_t code, - const ReadableParcel* parcel); + ReadableParcel* parcel); absl::Status ProcessStreamingTransactionImpl(transaction_code_t code, - const ReadableParcel* parcel, + ReadableParcel* parcel, int* cancellation_flags) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_); diff --git a/test/core/transport/binder/end2end/fake_binder.cc b/test/core/transport/binder/end2end/fake_binder.cc index 37ba57b91ac..6d0d1cd3804 100644 --- a/test/core/transport/binder/end2end/fake_binder.cc +++ b/test/core/transport/binder/end2end/fake_binder.cc @@ -24,59 +24,42 @@ namespace end2end_testing { TransactionProcessor* g_transaction_processor = nullptr; -FakeWritableParcel::FakeWritableParcel() : data_(1) {} - -int32_t FakeWritableParcel::GetDataPosition() const { return data_position_; } - int32_t FakeWritableParcel::GetDataSize() const { return data_size_; } -absl::Status FakeWritableParcel::SetDataPosition(int32_t pos) { - if (data_.size() < static_cast(pos) + 1) { - data_.resize(pos + 1); - } - data_position_ = pos; - return absl::OkStatus(); -} - absl::Status FakeWritableParcel::WriteInt32(int32_t data) { - data_[data_position_] = data; - SetDataPosition(data_position_ + 1).IgnoreError(); - data_size_ += 4; + data_.push_back(data); + data_size_ += sizeof(int32_t); return absl::OkStatus(); } absl::Status FakeWritableParcel::WriteInt64(int64_t data) { - data_[data_position_] = data; - SetDataPosition(data_position_ + 1).IgnoreError(); - data_size_ += 8; + data_.push_back(data); + data_size_ += sizeof(int64_t); return absl::OkStatus(); } absl::Status FakeWritableParcel::WriteBinder(HasRawBinder* binder) { - data_[data_position_] = binder->GetRawBinder(); - SetDataPosition(data_position_ + 1).IgnoreError(); - data_size_ += 8; + data_.push_back(binder->GetRawBinder()); + data_size_ += sizeof(void*); return absl::OkStatus(); } absl::Status FakeWritableParcel::WriteString(absl::string_view s) { - data_[data_position_] = std::string(s); - SetDataPosition(data_position_ + 1).IgnoreError(); + data_.push_back(std::string(s)); data_size_ += s.size(); return absl::OkStatus(); } absl::Status FakeWritableParcel::WriteByteArray(const int8_t* buffer, int32_t length) { - data_[data_position_] = std::vector(buffer, buffer + length); - SetDataPosition(data_position_ + 1).IgnoreError(); + data_.push_back(std::vector(buffer, buffer + length)); data_size_ += length; return absl::OkStatus(); } int32_t FakeReadableParcel::GetDataSize() const { return data_size_; } -absl::Status FakeReadableParcel::ReadInt32(int32_t* data) const { +absl::Status FakeReadableParcel::ReadInt32(int32_t* data) { if (data_position_ >= data_.size() || !absl::holds_alternative(data_[data_position_])) { return absl::InternalError("ReadInt32 failed"); @@ -85,7 +68,7 @@ absl::Status FakeReadableParcel::ReadInt32(int32_t* data) const { return absl::OkStatus(); } -absl::Status FakeReadableParcel::ReadInt64(int64_t* data) const { +absl::Status FakeReadableParcel::ReadInt64(int64_t* data) { if (data_position_ >= data_.size() || !absl::holds_alternative(data_[data_position_])) { return absl::InternalError("ReadInt64 failed"); @@ -94,8 +77,7 @@ absl::Status FakeReadableParcel::ReadInt64(int64_t* data) const { return absl::OkStatus(); } -absl::Status FakeReadableParcel::ReadBinder( - std::unique_ptr* data) const { +absl::Status FakeReadableParcel::ReadBinder(std::unique_ptr* data) { if (data_position_ >= data_.size() || !absl::holds_alternative(data_[data_position_])) { return absl::InternalError("ReadBinder failed"); @@ -106,7 +88,7 @@ absl::Status FakeReadableParcel::ReadBinder( return absl::OkStatus(); } -absl::Status FakeReadableParcel::ReadString(std::string* str) const { +absl::Status FakeReadableParcel::ReadString(std::string* str) { if (data_position_ >= data_.size() || !absl::holds_alternative(data_[data_position_])) { return absl::InternalError("ReadString failed"); @@ -115,7 +97,7 @@ absl::Status FakeReadableParcel::ReadString(std::string* str) const { return absl::OkStatus(); } -absl::Status FakeReadableParcel::ReadByteArray(std::string* data) const { +absl::Status FakeReadableParcel::ReadByteArray(std::string* data) { if (data_position_ >= data_.size() || !absl::holds_alternative>(data_[data_position_])) { return absl::InternalError("ReadByteArray failed"); diff --git a/test/core/transport/binder/end2end/fake_binder.h b/test/core/transport/binder/end2end/fake_binder.h index f1c48b9b539..b629c3d3423 100644 --- a/test/core/transport/binder/end2end/fake_binder.h +++ b/test/core/transport/binder/end2end/fake_binder.h @@ -82,10 +82,7 @@ using FakeData = std::vector< // MoveData(). class FakeWritableParcel final : public WritableParcel { public: - FakeWritableParcel(); - int32_t GetDataPosition() const override; int32_t GetDataSize() const override; - absl::Status SetDataPosition(int32_t pos) override; absl::Status WriteInt32(int32_t data) override; absl::Status WriteInt64(int64_t data) override; absl::Status WriteBinder(HasRawBinder* binder) override; @@ -96,7 +93,6 @@ class FakeWritableParcel final : public WritableParcel { private: FakeData data_; - size_t data_position_ = 0; int32_t data_size_ = 0; }; @@ -109,11 +105,11 @@ class FakeReadableParcel final : public ReadableParcel { explicit FakeReadableParcel(FakeData data) : data_(std::move(data)) { for (auto& d : data_) { if (absl::holds_alternative(d)) { - data_size_ += 4; + data_size_ += sizeof(int32_t); } else if (absl::holds_alternative(d)) { - data_size_ += 8; + data_size_ += sizeof(int64_t); } else if (absl::holds_alternative(d)) { - data_size_ += 8; + data_size_ += sizeof(void*); } else if (absl::holds_alternative(d)) { data_size_ += absl::get(d).size(); } else { @@ -123,15 +119,15 @@ class FakeReadableParcel final : public ReadableParcel { } int32_t GetDataSize() const override; - absl::Status ReadInt32(int32_t* data) const override; - absl::Status ReadInt64(int64_t* data) const override; - absl::Status ReadBinder(std::unique_ptr* data) const override; - absl::Status ReadByteArray(std::string* data) const override; - absl::Status ReadString(std::string* str) const override; + absl::Status ReadInt32(int32_t* data) override; + absl::Status ReadInt64(int64_t* data) override; + absl::Status ReadBinder(std::unique_ptr* data) override; + absl::Status ReadByteArray(std::string* data) override; + absl::Status ReadString(std::string* str) override; private: const FakeData data_; - mutable size_t data_position_ = 0; + size_t data_position_ = 0; int32_t data_size_ = 0; }; @@ -186,8 +182,7 @@ class PersistentFakeTransactionReceiver { TransactionReceiver::OnTransactCb cb, std::unique_ptr tunnel); - absl::Status Receive(BinderTransportTxCode tx_code, - const ReadableParcel* parcel) { + absl::Status Receive(BinderTransportTxCode tx_code, ReadableParcel* parcel) { return callback_(static_cast(tx_code), parcel); } @@ -215,7 +210,6 @@ class FakeBinder final : public Binder { absl::Status Transact(BinderTransportTxCode tx_code) override; WritableParcel* GetWritableParcel() const override { return input_.get(); } - ReadableParcel* GetReadableParcel() const override { return output_.get(); } std::unique_ptr ConstructTxReceiver( grpc_core::RefCountedPtr wire_reader_ref, @@ -226,7 +220,6 @@ class FakeBinder final : public Binder { private: FakeEndpoint* endpoint_; std::unique_ptr input_; - std::unique_ptr output_; }; // A transaction processor. diff --git a/test/core/transport/binder/end2end/fake_binder_test.cc b/test/core/transport/binder/end2end/fake_binder_test.cc index 2fdea9b2963..6abae7f89d2 100644 --- a/test/core/transport/binder/end2end/fake_binder_test.cc +++ b/test/core/transport/binder/end2end/fake_binder_test.cc @@ -29,29 +29,6 @@ namespace grpc_binder { namespace end2end_testing { - -TEST(FakeBinderTestWithoutTransaction, WritableParcelDataPosition) { - std::unique_ptr parcel = - absl::make_unique(); - EXPECT_EQ(parcel->GetDataPosition(), 0); - EXPECT_TRUE(parcel->WriteInt32(0).ok()); - EXPECT_EQ(parcel->GetDataPosition(), 1); - EXPECT_TRUE(parcel->WriteInt32(1).ok()); - EXPECT_TRUE(parcel->WriteInt32(2).ok()); - EXPECT_EQ(parcel->GetDataPosition(), 3); - EXPECT_TRUE(parcel->WriteString("").ok()); - EXPECT_EQ(parcel->GetDataPosition(), 4); - EXPECT_TRUE(parcel->SetDataPosition(0).ok()); - const char kBuffer[] = "test"; - EXPECT_TRUE(parcel - ->WriteByteArray(reinterpret_cast(kBuffer), - strlen(kBuffer)) - .ok()); - EXPECT_EQ(parcel->GetDataPosition(), 1); - EXPECT_TRUE(parcel->SetDataPosition(100).ok()); - EXPECT_EQ(parcel->GetDataPosition(), 100); -} - namespace { class FakeBinderTest : public ::testing::TestWithParam { @@ -70,8 +47,8 @@ TEST_P(FakeBinderTest, SendInt32) { int called = 0; std::unique_ptr sender; std::unique_ptr tx_receiver; - std::tie(sender, tx_receiver) = NewBinderPair( - [&](transaction_code_t tx_code, const ReadableParcel* parcel) { + std::tie(sender, tx_receiver) = + NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) { EXPECT_EQ(tx_code, kTxCode); int value = 0; EXPECT_TRUE(parcel->ReadInt32(&value).ok()); @@ -95,8 +72,8 @@ TEST_P(FakeBinderTest, SendString) { int called = 0; std::unique_ptr sender; std::unique_ptr tx_receiver; - std::tie(sender, tx_receiver) = NewBinderPair( - [&](transaction_code_t tx_code, const ReadableParcel* parcel) { + std::tie(sender, tx_receiver) = + NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) { EXPECT_EQ(tx_code, kTxCode); std::string value; EXPECT_TRUE(parcel->ReadString(&value).ok()); @@ -120,8 +97,8 @@ TEST_P(FakeBinderTest, SendByteArray) { int called = 0; std::unique_ptr sender; std::unique_ptr tx_receiver; - std::tie(sender, tx_receiver) = NewBinderPair( - [&](transaction_code_t tx_code, const ReadableParcel* parcel) { + std::tie(sender, tx_receiver) = + NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) { EXPECT_EQ(tx_code, kTxCode); std::string value; EXPECT_TRUE(parcel->ReadByteArray(&value).ok()); @@ -150,8 +127,8 @@ TEST_P(FakeBinderTest, SendMultipleItems) { int called = 0; std::unique_ptr sender; std::unique_ptr tx_receiver; - std::tie(sender, tx_receiver) = NewBinderPair( - [&](transaction_code_t tx_code, const ReadableParcel* parcel) { + std::tie(sender, tx_receiver) = + NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) { int value_result; EXPECT_EQ(tx_code, kTxCode); EXPECT_TRUE(parcel->ReadInt32(&value_result).ok()); @@ -186,8 +163,8 @@ TEST_P(FakeBinderTest, SendBinder) { int called = 0; std::unique_ptr sender; std::unique_ptr tx_receiver; - std::tie(sender, tx_receiver) = NewBinderPair( - [&](transaction_code_t tx_code, const ReadableParcel* parcel) { + std::tie(sender, tx_receiver) = + NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) { EXPECT_EQ(tx_code, kTxCode); std::unique_ptr binder; EXPECT_TRUE(parcel->ReadBinder(&binder).ok()); @@ -202,8 +179,7 @@ TEST_P(FakeBinderTest, SendBinder) { int called2 = 0; std::unique_ptr tx_receiver2 = absl::make_unique( - nullptr, - [&](transaction_code_t tx_code, const ReadableParcel* parcel) { + nullptr, [&](transaction_code_t tx_code, ReadableParcel* parcel) { int value; EXPECT_TRUE(parcel->ReadInt32(&value).ok()); EXPECT_EQ(value, kValue); @@ -228,8 +204,8 @@ TEST_P(FakeBinderTest, SendTransactionAfterDestruction) { int called = 0; { std::unique_ptr tx_receiver; - std::tie(sender, tx_receiver) = NewBinderPair( - [&](transaction_code_t tx_code, const ReadableParcel* parcel) { + std::tie(sender, tx_receiver) = + NewBinderPair([&](transaction_code_t tx_code, ReadableParcel* parcel) { EXPECT_EQ(tx_code, kTxCode); int value; EXPECT_TRUE(parcel->ReadInt32(&value).ok()); @@ -299,10 +275,9 @@ TEST_P(FakeBinderTest, StressTest) { std::unique_ptr tx_receiver; int expected_tx_code = th_arg->tx_code; std::vector>* cnt = th_arg->global_cnts; - std::tie(binder, tx_receiver) = - NewBinderPair([tid, p, cnt, expected_tx_code]( - transaction_code_t tx_code, - const ReadableParcel* parcel) mutable { + std::tie(binder, tx_receiver) = NewBinderPair( + [tid, p, cnt, expected_tx_code](transaction_code_t tx_code, + ReadableParcel* parcel) mutable { EXPECT_EQ(tx_code, expected_tx_code); int value; EXPECT_TRUE(parcel->ReadInt32(&value).ok()); diff --git a/test/core/transport/binder/end2end/testing_channel_create.cc b/test/core/transport/binder/end2end/testing_channel_create.cc index 04f09f57341..6c761ea2efd 100644 --- a/test/core/transport/binder/end2end/testing_channel_create.cc +++ b/test/core/transport/binder/end2end/testing_channel_create.cc @@ -35,7 +35,7 @@ class ServerSetupTransportHelper { : wire_reader_(absl::make_unique( /*transport_stream_receiver=*/nullptr, /*is_client=*/false)) { std::tie(endpoint_binder_, tx_receiver_) = NewBinderPair( - [this](transaction_code_t tx_code, const ReadableParcel* parcel) { + [this](transaction_code_t tx_code, ReadableParcel* parcel) { return this->wire_reader_->ProcessTransaction(tx_code, parcel); }); } diff --git a/test/core/transport/binder/mock_objects.cc b/test/core/transport/binder/mock_objects.cc index 98261b0db4a..4fb03555f4e 100644 --- a/test/core/transport/binder/mock_objects.cc +++ b/test/core/transport/binder/mock_objects.cc @@ -33,7 +33,6 @@ MockReadableParcel::MockReadableParcel() { } MockWritableParcel::MockWritableParcel() { - ON_CALL(*this, SetDataPosition).WillByDefault(Return(absl::OkStatus())); ON_CALL(*this, WriteInt32).WillByDefault(Return(absl::OkStatus())); ON_CALL(*this, WriteBinder).WillByDefault(Return(absl::OkStatus())); ON_CALL(*this, WriteString).WillByDefault(Return(absl::OkStatus())); @@ -44,7 +43,6 @@ MockBinder::MockBinder() { ON_CALL(*this, PrepareTransaction).WillByDefault(Return(absl::OkStatus())); ON_CALL(*this, Transact).WillByDefault(Return(absl::OkStatus())); ON_CALL(*this, GetWritableParcel).WillByDefault(Return(&mock_input_)); - ON_CALL(*this, GetReadableParcel).WillByDefault(Return(&mock_output_)); ON_CALL(*this, ConstructTxReceiver) .WillByDefault( [this](grpc_core::RefCountedPtr /*wire_reader_ref*/, diff --git a/test/core/transport/binder/mock_objects.h b/test/core/transport/binder/mock_objects.h index c5562eb866a..7e22add0e21 100644 --- a/test/core/transport/binder/mock_objects.h +++ b/test/core/transport/binder/mock_objects.h @@ -26,9 +26,7 @@ namespace grpc_binder { class MockWritableParcel : public WritableParcel { public: - MOCK_METHOD(int32_t, GetDataPosition, (), (const, override)); MOCK_METHOD(int32_t, GetDataSize, (), (const, override)); - MOCK_METHOD(absl::Status, SetDataPosition, (int32_t), (override)); MOCK_METHOD(absl::Status, WriteInt32, (int32_t), (override)); MOCK_METHOD(absl::Status, WriteInt64, (int64_t), (override)); MOCK_METHOD(absl::Status, WriteBinder, (HasRawBinder*), (override)); @@ -42,12 +40,11 @@ class MockWritableParcel : public WritableParcel { class MockReadableParcel : public ReadableParcel { public: MOCK_METHOD(int32_t, GetDataSize, (), (const, override)); - MOCK_METHOD(absl::Status, ReadInt32, (int32_t*), (const, override)); - MOCK_METHOD(absl::Status, ReadInt64, (int64_t*), (const, override)); - MOCK_METHOD(absl::Status, ReadBinder, (std::unique_ptr*), - (const, override)); - MOCK_METHOD(absl::Status, ReadByteArray, (std::string*), (const, override)); - MOCK_METHOD(absl::Status, ReadString, (std::string*), (const, override)); + MOCK_METHOD(absl::Status, ReadInt32, (int32_t*), (override)); + MOCK_METHOD(absl::Status, ReadInt64, (int64_t*), (override)); + MOCK_METHOD(absl::Status, ReadBinder, (std::unique_ptr*), (override)); + MOCK_METHOD(absl::Status, ReadByteArray, (std::string*), (override)); + MOCK_METHOD(absl::Status, ReadString, (std::string*), (override)); MockReadableParcel(); }; @@ -58,7 +55,6 @@ class MockBinder : public Binder { MOCK_METHOD(absl::Status, PrepareTransaction, (), (override)); MOCK_METHOD(absl::Status, Transact, (BinderTransportTxCode), (override)); MOCK_METHOD(WritableParcel*, GetWritableParcel, (), (const, override)); - MOCK_METHOD(ReadableParcel*, GetReadableParcel, (), (const, override)); MOCK_METHOD(std::unique_ptr, ConstructTxReceiver, (grpc_core::RefCountedPtr, TransactionReceiver::OnTransactCb), @@ -80,7 +76,7 @@ class MockTransactionReceiver : public TransactionReceiver { public: explicit MockTransactionReceiver(OnTransactCb transact_cb, BinderTransportTxCode code, - const ReadableParcel* output) { + ReadableParcel* output) { transact_cb(static_cast(code), output).IgnoreError(); }