requester: instance-id: Release read lock on conflict

36af84cdbb66 ("requester: Add new APIs for instance ID allocation and
freeing") introduced the new instance ID allocation APIs, and some
unbalanced locking along with it. When a conflict arose on an instance
ID, the read lock was not released by the non-owning caller.

Release the lock on conflict and on error, and add a test case to
prevent regression.

gitlint-ignore: UC1, B1
Fixes: 36af84cdbb66 ("requester: Add new APIs for instance ID allocation and freeing")
Reported-by: Jerry Chen <jerry_c_chen@wiwynn.com>
Change-Id: Iecd1583c6b8863b458cc4fbf1ac42b20ca2a3433
Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
diff --git a/tests/instance-id.cpp b/tests/instance-id.cpp
index ae4102d..ad290ac 100644
--- a/tests/instance-id.cpp
+++ b/tests/instance-id.cpp
@@ -237,6 +237,84 @@
     ASSERT_EQ(pldm_instance_db_destroy(db), 0);
 }
 
+TEST_F(PldmInstanceDbTest, releaseConflictedSameTid)
+{
+    static constexpr pldm_tid_t tid = 1;
+    struct
+    {
+        struct pldm_instance_db* db;
+        pldm_instance_id_t iid;
+    } connections[] = {
+        {nullptr, 0},
+        {nullptr, 0},
+    };
+    pldm_instance_id_t iid;
+
+    /* Allocate IID 0 for the TID to the first connection */
+    ASSERT_EQ(pldm_instance_db_init(&connections[0].db, dbPath.c_str()), 0);
+    EXPECT_EQ(
+        pldm_instance_id_alloc(connections[0].db, tid, &connections[0].iid), 0);
+
+    /*
+     * On the second connection, allocate the first available IID for the TID.
+     * This should generate a conflict on IID 0 (allocated to the first
+     * connection), and result in IID 1 being provided.
+     *
+     * There should now be one read lock held on each of IID 0 and IID 1 for TID
+     * 1 (by the first and second connections respectively).
+     */
+    ASSERT_EQ(pldm_instance_db_init(&connections[1].db, dbPath.c_str()), 0);
+    EXPECT_EQ(
+        pldm_instance_id_alloc(connections[1].db, tid, &connections[1].iid), 0);
+
+    /*
+     * Make sure the implementation hasn't allocated the connections a
+     * conflicting IID for the TID.
+     */
+    EXPECT_NE(connections[0].iid, connections[1].iid);
+
+    /*
+     * Now free the IID allocated to the first connection.
+     *
+     * We should be able to re-acquire this later.
+     */
+    EXPECT_EQ(pldm_instance_id_free(connections[0].db, tid, connections[0].iid),
+              0);
+
+    /*
+     * Iterate through the IID space on the first connection to wrap it back
+     * around to IID 0.
+     *
+     * Note that:
+     *
+     * 1. The first connection has already allocated (and released) IID 0,
+     *    eliminating one iteration
+     *
+     * 2. IID 1 is held by the second connection. This eliminates a second
+     *    iteration as it must be skipped to avoid a conflict.
+     */
+    for (int i = 0; i < (pldmMaxInstanceIds - 1 - 1); i++)
+    {
+        EXPECT_EQ(pldm_instance_id_alloc(connections[0].db, tid, &iid), 0);
+        EXPECT_EQ(pldm_instance_id_free(connections[0].db, tid, iid), 0);
+    }
+
+    /*
+     * The next IID allocated to the first connection should be the IID it
+     * allocated initially (which should be 0).
+     */
+    EXPECT_EQ(pldm_instance_id_alloc(connections[0].db, tid, &iid), 0);
+    EXPECT_EQ(iid, connections[0].iid);
+
+    /* Now tidy up */
+    ASSERT_EQ(pldm_instance_id_free(connections[0].db, tid, iid), 0);
+
+    EXPECT_EQ(pldm_instance_id_free(connections[1].db, tid, connections[1].iid),
+              0);
+    ASSERT_EQ(pldm_instance_db_destroy(connections[1].db), 0);
+    ASSERT_EQ(pldm_instance_db_destroy(connections[0].db), 0);
+}
+
 TEST_F(PldmInstanceDbTest, freeUnallocatedInstanceId)
 {
     struct pldm_instance_db* db = nullptr;