From 8b185ac0504a3b0a24a76c5b8486cea7534d9d2e Mon Sep 17 00:00:00 2001
From: Chen Chen <cncn@google.com>
Date: Tue, 24 Mar 2020 14:52:52 -0700
Subject: [PATCH] BluetoothMetrics: Return true if SaveDevice is called twice.
 Add more logging

Test: atest BluetoothMetricIdAllocatorTest
Bug:152251473

Change-Id: I3d5ee8d086a4e589e84a766d3d0923d3ae2278ba
---
 common/metric_id_allocator.cc          | 41 +++++++++++++++++++++++++---------
 common/metric_id_allocator_unittest.cc | 12 +++++-----
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/common/metric_id_allocator.cc b/common/metric_id_allocator.cc
index 652803b28..005ef3ab3 100644
--- a/common/metric_id_allocator.cc
+++ b/common/metric_id_allocator.cc
@@ -146,24 +146,45 @@ int MetricIdAllocator::AllocateId(const RawAddress& mac_address) {
 bool MetricIdAllocator::SaveDevice(const RawAddress& mac_address) {
   std::lock_guard<std::mutex> lock(id_allocator_mutex_);
   int id = 0;
-  bool success = temporary_device_cache_.Get(mac_address, &id);
-  success &= temporary_device_cache_.Remove(mac_address);
-  if (success) {
-    paired_device_cache_.Put(mac_address, id);
-    success = save_id_callback_(mac_address, id);
+  if (paired_device_cache_.Get(mac_address, &id)) {
+    return true;
+  }
+  if (!temporary_device_cache_.Get(mac_address, &id)) {
+    LOG(ERROR) << LOGGING_TAG
+               << "Failed to save device because device is not in "
+               << "temporary_device_cache_";
+    return false;
+  }
+  if (!temporary_device_cache_.Remove(mac_address)) {
+    LOG(ERROR) << LOGGING_TAG
+               << "Failed to remove device from temporary_device_cache_";
+    return false;
   }
-  return success;
+  paired_device_cache_.Put(mac_address, id);
+  if (!save_id_callback_(mac_address, id)) {
+    LOG(ERROR) << LOGGING_TAG
+               << "Callback returned false after saving the device";
+    return false;
+  }
+  return true;
 }
 
 // call this function when a device is forgotten
 void MetricIdAllocator::ForgetDevice(const RawAddress& mac_address) {
   std::lock_guard<std::mutex> lock(id_allocator_mutex_);
   int id = 0;
-  bool success = paired_device_cache_.Get(mac_address, &id);
-  success &= paired_device_cache_.Remove(mac_address);
-  if (success) {
-    ForgetDevicePostprocess(mac_address, id);
+  if (!paired_device_cache_.Get(mac_address, &id)) {
+    LOG(ERROR) << LOGGING_TAG
+               << "Failed to forget device because device is not in "
+               << "paired_device_cache_";
+    return;
+  }
+  if (!paired_device_cache_.Remove(mac_address)) {
+    LOG(ERROR) << LOGGING_TAG
+               << "Failed to remove device from paired_device_cache_";
+    return;
   }
+  ForgetDevicePostprocess(mac_address, id);
 }
 
 bool MetricIdAllocator::IsValidId(const int id) {
diff --git a/common/metric_id_allocator_unittest.cc b/common/metric_id_allocator_unittest.cc
index eae2ffee8..ccc157647 100644
--- a/common/metric_id_allocator_unittest.cc
+++ b/common/metric_id_allocator_unittest.cc
@@ -165,8 +165,8 @@ TEST(BluetoothMetricIdAllocatorTest, MetricIdAllocatorMainTest1) {
   EXPECT_TRUE(allocator.SaveDevice(RawAddress({0, 0, 0, 0, 0, 3})));
   EXPECT_EQ(dummy, 176);
 
-  // should fail, since id had been saved
-  EXPECT_FALSE(allocator.SaveDevice(RawAddress({0, 0, 0, 0, 0, 0})));
+  // should be true but callback won't be called, since id had been saved
+  EXPECT_TRUE(allocator.SaveDevice(RawAddress({0, 0, 0, 0, 0, 0})));
   EXPECT_EQ(dummy, 176);
 
   // forget
@@ -262,8 +262,8 @@ TEST(BluetoothMetricIdAllocatorTest, MetricIdAllocatorFullPairedMap) {
   // paired: 4 ... 199, 200, 201, 202, 203
   // scanned: 0, 1
 
-  // should fail, since id had been saved
-  EXPECT_FALSE(allocator.SaveDevice(kthAddress(key + 2)));
+  // should be true but callback won't be called, since id had been saved
+  EXPECT_TRUE(allocator.SaveDevice(kthAddress(key + 2)));
   EXPECT_EQ(dummy, 4);
 
   dummy = 27;
@@ -284,7 +284,9 @@ TEST(BluetoothMetricIdAllocatorTest, MetricIdAllocatorFullPairedMap) {
 
   EXPECT_TRUE(allocator.SaveDevice(kthAddress(key + 2)));
   EXPECT_EQ(dummy, 18);  // no key is evicted, a key is saved so *2,
-  EXPECT_FALSE(allocator.SaveDevice(kthAddress(key + 3)));
+
+  // should be true but callback won't be called, since id had been saved
+  EXPECT_TRUE(allocator.SaveDevice(kthAddress(key + 3)));
   EXPECT_EQ(dummy, 18);  // no such a key in scanned
   EXPECT_TRUE(allocator.SaveDevice(kthAddress(key + 4)));
   EXPECT_EQ(dummy, 12);  // one key is evicted, another key is saved so *2/3,
-- 
2.11.0