[bt][gatt] Clean up MTU exchange result handling

Instead of returning a tuple of (att::Result<>, mtu) from gatt::Client::
ExchangeMTU's MTUCallback, return an att::Result<mtu>. This represents
the result of the procedure more accurately - if the MTU exchange fails,
no MTU is negotiated.

The one edge case is if the MTU exchange fails because the peer does not
support the procedure. This procedure is optional per v5.3 Vol. 3 Part F
Table 4.1, and in this case we should continue initialization with the
understanding that the MTU is the default, minimum LE MTU. This is the
only behavior change from the prior code, the rest of the change is
effectively a refactor.

Also, change some EXPECT_EQ->ASSERT_EQ in RemoteServiceManagerTests so
that failed expectations do not cause invalid array accesses and process
termination.

Bug: 36375
Test: `fx test bt-host-gatt-tests`, refactored gatt/client_unittests.cc
      and added
      RemoteServiceManagerTest.InitializeMtuExchangeNotSupportedSucceeds
Change-Id: Ife0cb82eb70066cd8fc3988ac936d97fad71b8fd
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/689574
Reviewed-by: Ben Lawson <[email protected]>
Commit-Queue: Lucas Jenkins <[email protected]>
Fuchsia-Auto-Submit: Lucas Jenkins <[email protected]>

Author: Lucas Jenkins

src/connectivity/bluetooth/core/bt-host/gatt/client.cc

@@ -137,15 +137,16 @@ class Impl final : public Client {
   void ExchangeMTU(MTUCallback mtu_cb) override {
     auto pdu = NewPDU(sizeof(att::ExchangeMTURequestParams));
     if (!pdu) {
-      mtu_cb(ToResult(HostError::kOutOfMemory), 0);
+      mtu_cb(fitx::error(att::Error(HostError::kOutOfMemory)));
       return;
     }
 
     att::PacketWriter writer(att::kExchangeMTURequest, pdu.get());
     auto params = writer.mutable_payload<att::ExchangeMTURequestParams>();
     params->client_rx_mtu = htole16(att_->preferred_mtu());
 
-    auto rsp_cb = [this, mtu_cb = std::move(mtu_cb)](att::Bearer::TransactionResult result) {
+    auto rsp_cb = [this,
+                   mtu_cb = std::move(mtu_cb)](att::Bearer::TransactionResult result) mutable {
       if (result.is_ok()) {
         const att::PacketReader& rsp = result.value();
         ZX_DEBUG_ASSERT(rsp.opcode() == att::kExchangeMTUResponse);
@@ -154,7 +155,7 @@ class Impl final : public Client {
           // Received a malformed response. Disconnect the link.
           att_->ShutDown();
 
-          mtu_cb(ToResult(HostError::kPacketMalformed), 0);
+          mtu_cb(fitx::error(att::Error(HostError::kPacketMalformed)));
           return;
         }
 
@@ -166,7 +167,7 @@ class Impl final : public Client {
         uint16_t final_mtu = std::max(att::kLEMinMTU, std::min(server_mtu, att_->preferred_mtu()));
         att_->set_mtu(final_mtu);
 
-        mtu_cb(fitx::ok(), final_mtu);
+        mtu_cb(fitx::ok(final_mtu));
         return;
       }
       const auto& [error, handle] = result.error_value();
@@ -177,12 +178,12 @@ class Impl final : public Client {
       if (error.is(att::ErrorCode::kRequestNotSupported)) {
         bt_log(DEBUG, "gatt", "peer does not support MTU exchange: using default");
         att_->set_mtu(att::kLEMinMTU);
-        mtu_cb(fitx::error(error), att::kLEMinMTU);
+        mtu_cb(fitx::error(error));
         return;
       }
 
       bt_log(DEBUG, "gatt", "MTU exchange failed: %s", bt_str(error));
-      mtu_cb(fitx::error(error), 0);
+      mtu_cb(fitx::error(error));
     };
 
     att_->StartTransaction(std::move(pdu), BindCallback(std::move(rsp_cb)));

src/connectivity/bluetooth/core/bt-host/gatt/client.h

@@ -35,17 +35,19 @@ class Client {
   // Returns the current ATT MTU.
   virtual uint16_t mtu() const = 0;
 
-  // Initiates an MTU exchange and adjusts the MTU of the bearer according to
-  // what the peer is capable of. The request will be initiated using the
-  // bearer's preferred MTU.
+  // Initiates an MTU exchange and adjusts the bearer's MTU as outlined in Core Spec v5.3 Vol. 3
+  // Part F 3.4.2. The request will be made using the locally-preferred MTU.
   //
-  // After the exchange is complete, the bearer will be updated to use the
-  // resulting MTU. The resulting MTU will be notified via |callback|.
+  // Upon successful exchange, the bearer will be updated to use the resulting MTU and |callback|
+  // will be notified with the the resulting MTU.
   //
-  // |status| will be set to an error if the MTU exchange fails. The |mtu|
-  // parameter will be set to 0 and the underlying bearer's MTU will remain
-  // unmodified.
-  using MTUCallback = fit::function<void(att::Result<> status, uint16_t mtu)>;
+  // MTU exchange support is optional per v5.3 Vol. 3 Part F Table 4.1, so if the exchange fails
+  // because the peer doesn't support it, the bearer's MTU will be |kLEMinMTU| and initialization
+  // should proceed. In this case, |mtu_result| will be |att::Error(att::kRequestNotSupported)|.
+  //
+  // If the MTU exchange otherwise fails, |mtu_result| will be an error and the bearer's MTU will
+  // not change.
+  using MTUCallback = fit::callback<void(att::Result<uint16_t> mtu_result)>;
   virtual void ExchangeMTU(MTUCallback callback) = 0;
 
   // Performs a modified version of the "Discover All Primary Services" procedure defined in v5.0,

src/connectivity/bluetooth/core/bt-host/gatt/client_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "client.h"
 
+#include "src/connectivity/bluetooth/core/bt-host/att/att.h"
 #include "src/connectivity/bluetooth/core/bt-host/common/test_helpers.h"
 #include "src/connectivity/bluetooth/core/bt-host/l2cap/fake_channel_test.h"
 
@@ -14,6 +15,14 @@ constexpr UUID kTestUuid1(uint16_t{0xDEAD});
 constexpr UUID kTestUuid2(uint16_t{0xBEEF});
 constexpr UUID kTestUuid3({0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15});
 
+att::Result<uint16_t> MtuResultFromErrCode(att::ErrorCode ecode) {
+  return fitx::error(att::Error(ecode));
+}
+
+att::Result<uint16_t> MtuResultFromHostErrCode(HostError ecode) {
+  return fitx::error(att::Error(ecode));
+}
+
 // clang-format off
 const StaticByteBuffer kDiscoverPrimaryRequest(
     0x10,        // opcode: read by group type request
@@ -98,13 +107,8 @@ TEST_F(ClientTest, ExchangeMTUMalformedResponse) {
                                           kPreferredMTU, 0x00  // client rx mtu: kPreferredMTU
   );
 
-  // Initialize to a non-zero value.
-  uint16_t final_mtu = kPreferredMTU;
-  att::Result<> status = fitx::ok();
-  auto mtu_cb = [&](att::Result<> cb_status, uint16_t val) {
-    final_mtu = val;
-    status = cb_status;
-  };
+  std::optional<att::Result<uint16_t>> result;
+  auto mtu_cb = [&](att::Result<uint16_t> cb_result) { result = cb_result; };
 
   att()->set_preferred_mtu(kPreferredMTU);
 
@@ -122,8 +126,8 @@ TEST_F(ClientTest, ExchangeMTUMalformedResponse) {
 
   RunLoopUntilIdle();
 
-  EXPECT_EQ(ToResult(HostError::kPacketMalformed), status);
-  EXPECT_EQ(0, final_mtu);
+  ASSERT_TRUE(result.has_value());
+  EXPECT_EQ(MtuResultFromHostErrCode(HostError::kPacketMalformed), *result);
   EXPECT_TRUE(fake_chan()->link_error());
 }
 
@@ -135,12 +139,8 @@ TEST_F(ClientTest, ExchangeMTUErrorNotSupported) {
                                           kPreferredMTU, 0x00  // client rx mtu: kPreferredMTU
   );
 
-  uint16_t final_mtu = 0;
-  att::Result<> status = fitx::ok();
-  auto mtu_cb = [&](att::Result<> cb_status, uint16_t val) {
-    final_mtu = val;
-    status = cb_status;
-  };
+  std::optional<att::Result<uint16_t>> result;
+  auto mtu_cb = [&](att::Result<uint16_t> cb_result) { result = cb_result; };
 
   // Set the initial MTU to something other than the default LE MTU since we
   // want to confirm that the MTU changes to the default.
@@ -154,16 +154,15 @@ TEST_F(ClientTest, ExchangeMTUErrorNotSupported) {
 
   // Respond with "Request Not Supported". This will cause us to switch to the
   // default MTU.
-  fake_chan()->Receive(StaticByteBuffer(0x01,        // opcode: error response
-                                        0x02,        // request: exchange MTU
-                                        0x00, 0x00,  // handle: 0
-                                        0x06         // error: Request Not Supported
-                                        ));
+  fake_chan()->Receive(StaticByteBuffer(att::kErrorResponse,       // opcode
+                                        att::kExchangeMTURequest,  // request opcode
+                                        0x00, 0x00,                // handle: 0
+                                        att::ErrorCode::kRequestNotSupported));
 
   RunLoopUntilIdle();
 
-  EXPECT_EQ(ToResult(att::ErrorCode::kRequestNotSupported), status);
-  EXPECT_EQ(att::kLEMinMTU, final_mtu);
+  ASSERT_TRUE(result.has_value());
+  EXPECT_EQ(MtuResultFromErrCode(att::ErrorCode::kRequestNotSupported), *result);
   EXPECT_EQ(att::kLEMinMTU, att()->mtu());
 }
 
@@ -174,12 +173,8 @@ TEST_F(ClientTest, ExchangeMTUErrorOther) {
                        kPreferredMTU, 0x00  // client rx mtu: kPreferredMTU
       );
 
-  uint16_t final_mtu = kPreferredMTU;
-  att::Result<> status = fitx::ok();
-  auto mtu_cb = [&](att::Result<> cb_status, uint16_t val) {
-    final_mtu = val;
-    status = cb_status;
-  };
+  std::optional<att::Result<uint16_t>> result;
+  auto mtu_cb = [&](att::Result<uint16_t> cb_result) { result = cb_result; };
 
   att()->set_preferred_mtu(kPreferredMTU);
   EXPECT_EQ(att::kLEMinMTU, att()->mtu());
@@ -190,16 +185,15 @@ TEST_F(ClientTest, ExchangeMTUErrorOther) {
   ASSERT_TRUE(Expect(kExpectedRequest));
 
   // Respond with an error. The MTU should remain unchanged.
-  fake_chan()->Receive(StaticByteBuffer(0x01,        // opcode: error response
-                                        0x02,        // request: exchange MTU
-                                        0x00, 0x00,  // handle: 0
-                                        0x0E         // error: Unlikely Error
-                                        ));
+  fake_chan()->Receive(StaticByteBuffer(att::kErrorResponse,       // opcode
+                                        att::kExchangeMTURequest,  // request opcode
+                                        0x00, 0x00,                // handle: 0
+                                        att::ErrorCode::kUnlikelyError));
 
   RunLoopUntilIdle();
 
-  EXPECT_EQ(ToResult(att::ErrorCode::kUnlikelyError), status);
-  EXPECT_EQ(0, final_mtu);
+  ASSERT_TRUE(result.has_value());
+  EXPECT_EQ(MtuResultFromErrCode(att::ErrorCode::kUnlikelyError), *result);
   EXPECT_EQ(att::kLEMinMTU, att()->mtu());
 }
 
@@ -213,12 +207,8 @@ TEST_F(ClientTest, ExchangeMTUSelectLocal) {
                        kPreferredMTU, 0x00  // client rx mtu: kPreferredMTU
       );
 
-  uint16_t final_mtu = 0;
-  att::Result<> status = fitx::ok();
-  auto mtu_cb = [&](att::Result<> cb_status, uint16_t val) {
-    final_mtu = val;
-    status = cb_status;
-  };
+  std::optional<att::Result<uint16_t>> result;
+  auto mtu_cb = [&](att::Result<uint16_t> cb_result) { result = cb_result; };
 
   att()->set_preferred_mtu(kPreferredMTU);
 
@@ -234,9 +224,8 @@ TEST_F(ClientTest, ExchangeMTUSelectLocal) {
                                         ));
 
   RunLoopUntilIdle();
-
-  EXPECT_EQ(fitx::ok(), status);
-  EXPECT_EQ(kPreferredMTU, final_mtu);
+  ASSERT_TRUE(result.has_value());
+  EXPECT_EQ(att::Result<uint16_t>(fitx::ok(kPreferredMTU)), *result);
   EXPECT_EQ(kPreferredMTU, att()->mtu());
 }
 
@@ -250,12 +239,8 @@ TEST_F(ClientTest, ExchangeMTUSelectRemote) {
                        kPreferredMTU, 0x00  // client rx mtu: kPreferredMTU
       );
 
-  uint16_t final_mtu = 0;
-  att::Result<> status = fitx::ok();
-  auto mtu_cb = [&](att::Result<> cb_status, uint16_t val) {
-    final_mtu = val;
-    status = cb_status;
-  };
+  std::optional<att::Result<uint16_t>> result;
+  auto mtu_cb = [&](att::Result<uint16_t> cb_result) { result = cb_result; };
 
   att()->set_preferred_mtu(kPreferredMTU);
 
@@ -272,8 +257,8 @@ TEST_F(ClientTest, ExchangeMTUSelectRemote) {
 
   RunLoopUntilIdle();
 
-  EXPECT_EQ(fitx::ok(), status);
-  EXPECT_EQ(kServerRxMTU, final_mtu);
+  ASSERT_TRUE(result.has_value());
+  EXPECT_EQ(att::Result<uint16_t>(fitx::ok(kServerRxMTU)), *result);
   EXPECT_EQ(kServerRxMTU, att()->mtu());
 }
 
@@ -287,12 +272,8 @@ TEST_F(ClientTest, ExchangeMTUSelectDefault) {
                        kPreferredMTU, 0x00  // client rx mtu: kPreferredMTU
       );
 
-  uint16_t final_mtu = 0;
-  att::Result<> status = fitx::ok();
-  auto mtu_cb = [&](att::Result<> cb_status, uint16_t val) {
-    final_mtu = val;
-    status = cb_status;
-  };
+  std::optional<att::Result<uint16_t>> result;
+  auto mtu_cb = [&](att::Result<uint16_t> cb_result) { result = cb_result; };
 
   att()->set_preferred_mtu(kPreferredMTU);
 
@@ -309,8 +290,8 @@ TEST_F(ClientTest, ExchangeMTUSelectDefault) {
 
   RunLoopUntilIdle();
 
-  EXPECT_EQ(fitx::ok(), status);
-  EXPECT_EQ(att::kLEMinMTU, final_mtu);
+  ASSERT_TRUE(result.has_value());
+  EXPECT_EQ(att::Result<uint16_t>(fitx::ok(att::kLEMinMTU)), *result);
   EXPECT_EQ(att::kLEMinMTU, att()->mtu());
 }
 
@@ -3213,12 +3194,8 @@ TEST_F(ClientTest, ReadRequestSuccessNotTruncatedWhenMtuAllowsMaxValueLength) {
                        LowerBits(kPreferredMTU), UpperBits(kPreferredMTU)  // client rx mtu
       );
 
-  uint16_t final_mtu = 0;
-  att::Result<> mtu_status = fitx::ok();
-  auto mtu_cb = [&](att::Result<> cb_status, uint16_t val) {
-    final_mtu = val;
-    mtu_status = cb_status;
-  };
+  std::optional<att::Result<uint16_t>> result;
+  auto mtu_cb = [&](att::Result<uint16_t> cb_result) { result = cb_result; };
 
   // Initiate the request on the loop since Expect() below blocks.
   async::PostTask(dispatcher(), [this, mtu_cb] { client()->ExchangeMTU(mtu_cb); });
@@ -3232,8 +3209,8 @@ TEST_F(ClientTest, ReadRequestSuccessNotTruncatedWhenMtuAllowsMaxValueLength) {
                                         ));
 
   RunLoopUntilIdle();
-  EXPECT_EQ(fitx::ok(), mtu_status);
-  EXPECT_EQ(kPreferredMTU, final_mtu);
+  ASSERT_TRUE(result.has_value());
+  EXPECT_EQ(att::Result<uint16_t>(fitx::ok(kPreferredMTU)), *result);
   EXPECT_EQ(kPreferredMTU, att()->mtu());
 
   constexpr att::Handle kHandle = 0x0001;
@@ -3386,6 +3363,7 @@ TEST_F(ClientTest, ReadByTypeRequestSuccess128BitUUID) {
   EXPECT_TRUE(cb_called);
   EXPECT_FALSE(fake_chan()->link_error());
 }
+
 TEST_F(ClientTest, ReadByTypeRequestError) {
   constexpr att::Handle kStartHandle = 0x0001;
   constexpr att::Handle kEndHandle = 0xFFFF;

src/connectivity/bluetooth/core/bt-host/gatt/fake_client.cc

@@ -23,8 +23,13 @@ uint16_t FakeClient::mtu() const {
 }
 
 void FakeClient::ExchangeMTU(MTUCallback callback) {
-  auto task = [status = exchange_mtu_status_, mtu = server_mtu_, callback = std::move(callback)] {
-    callback(status, mtu);
+  auto task = [status = exchange_mtu_status_, mtu = server_mtu_,
+               callback = std::move(callback)]() mutable {
+    if (status.is_error()) {
+      callback(fitx::error(status.error_value()));
+    } else {
+      callback(fitx::ok(mtu));
+    }
   };
   async::PostTask(dispatcher_, std::move(task));
 }

src/connectivity/bluetooth/core/bt-host/gatt/remote_service_manager.cc

@@ -96,15 +96,20 @@ void RemoteServiceManager::Initialize(att::ResultFunction<> cb, std::vector<UUID
     user_init_cb(status);
   };
 
-  client_->ExchangeMTU([self, init_cb = std::move(init_cb), services = std::move(services)](
-                           att::Result<> status, uint16_t mtu) mutable {
+  client_->ExchangeMTU([self, init_cb = std::move(init_cb),
+                        services = std::move(services)](att::Result<uint16_t> mtu_result) mutable {
     // The Client's Bearer may outlive this object.
     if (!self) {
       return;
     }
 
-    if (bt_is_error(status, INFO, "gatt", "MTU exchange failed")) {
-      init_cb(status);
+    // Support for the MTU exchange is optional, so if the peer indicated they don't support it, we
+    // continue with initialization.
+    if (mtu_result.is_ok() || mtu_result.error_value().is(att::ErrorCode::kRequestNotSupported)) {
+      bt_is_error(mtu_result, DEBUG, "gatt", "MTU exchange not supported");
+    } else {
+      bt_log(INFO, "gatt", "MTU exchange failed: %s", bt_str(mtu_result.error_value()));
+      init_cb(fitx::error(mtu_result.error_value()));
       return;
     }