pdr: Fix for Entity Association PDR looping
While normalizing the Entity association PDR, record handle
is passed to the library function. Considering PDR contains
logical and physical contained entities, two pdrs with same
record handle are created.
To fix the issue, we are propagating the recently used record
handle from the pldm_pdr_add to entity_association_pdr_add.
Then incrementing the record handle before creating next PDR.
gitlint-ignore: B1, UC1
Fixes: 25ddbccfae0e ("pdr: Add pldm_entity_association_pdr_add_from_node_with_record_handle()")
Change-Id: Ifaa5694cabe7ad38d8881f0b7be0a79d9d3e06c0
Signed-off-by: Archana Kakani <archana.kakani@ibm.com>
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f46152a..f2fde9c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -56,6 +56,9 @@
No new error values will be returned, but existing error values may be
returned under new conditions.
+- pdr: Indicates success or failure depending on the outcome of the entity
+ association PDR creation
+
### Deprecated
### Removed
@@ -64,6 +67,7 @@
- pdr: Remove PDR if the contained entity to be removed is the last one
- meson: sizes.h: add includedir to install path
+- pdr: Create entity association PDRs with unique record handle
### Security
diff --git a/src/dsp/pdr.c b/src/dsp/pdr.c
index 142c4f7..95526ff 100644
--- a/src/dsp/pdr.c
+++ b/src/dsp/pdr.c
@@ -795,14 +795,14 @@
return false;
}
-static int entity_association_pdr_add_children(
+static int64_t entity_association_pdr_add_children(
pldm_entity_node *curr, pldm_pdr *repo, uint16_t size,
uint8_t contained_count, uint8_t association_type, bool is_remote,
uint16_t terminus_handle, uint32_t record_handle)
{
uint8_t *start;
uint8_t *pdr;
- int rc;
+ int64_t rc;
pdr = calloc(1, size);
if (!pdr) {
@@ -851,19 +851,26 @@
rc = pldm_pdr_add(repo, pdr, size, is_remote, terminus_handle,
&record_handle);
free(pdr);
- return rc;
+ return (rc < 0) ? rc : record_handle;
}
-static int entity_association_pdr_add_entry(pldm_entity_node *curr,
- pldm_pdr *repo, bool is_remote,
- uint16_t terminus_handle,
- uint32_t record_handle)
+static int64_t entity_association_pdr_add_entry(pldm_entity_node *curr,
+ pldm_pdr *repo, bool is_remote,
+ uint16_t terminus_handle,
+ uint32_t record_handle)
{
uint8_t num_logical_children = pldm_entity_get_num_children(
curr, PLDM_ENTITY_ASSOCIAION_LOGICAL);
uint8_t num_physical_children = pldm_entity_get_num_children(
curr, PLDM_ENTITY_ASSOCIAION_PHYSICAL);
- int rc;
+ int64_t rc;
+
+ if (!num_logical_children && !num_physical_children) {
+ if (record_handle == 0) {
+ return -EINVAL;
+ }
+ return record_handle - 1;
+ }
if (num_logical_children) {
uint16_t logical_pdr_size =
@@ -878,6 +885,12 @@
if (rc < 0) {
return rc;
}
+ if (num_physical_children) {
+ if (rc >= UINT32_MAX) {
+ return -EOVERFLOW;
+ }
+ record_handle = rc + 1;
+ }
}
if (num_physical_children) {
@@ -893,9 +906,10 @@
if (rc < 0) {
return rc;
}
+ record_handle = rc;
}
- return 0;
+ return record_handle;
}
static bool is_present(pldm_entity entity, pldm_entity **entities,
@@ -914,36 +928,56 @@
return false;
}
-static int entity_association_pdr_add(pldm_entity_node *curr, pldm_pdr *repo,
- pldm_entity **entities,
- size_t num_entities, bool is_remote,
- uint16_t terminus_handle,
- uint32_t record_handle)
+static int64_t entity_association_pdr_add(pldm_entity_node *curr,
+ pldm_pdr *repo,
+ pldm_entity **entities,
+ size_t num_entities, bool is_remote,
+ uint16_t terminus_handle,
+ uint32_t record_handle)
{
- int rc;
+ int64_t rc;
if (curr == NULL) {
- return 0;
+ // entity_association_pdr_add function gets called
+ // recursively for the siblings and children of the
+ // entity. This causes NULL current entity node, and the
+ // record handle is returned
+ return record_handle;
}
if (is_present(curr->entity, entities, num_entities)) {
rc = entity_association_pdr_add_entry(
curr, repo, is_remote, terminus_handle, record_handle);
- if (rc) {
+ if (rc < 0) {
return rc;
}
+ if (rc >= UINT32_MAX) {
+ return -EOVERFLOW;
+ }
+ record_handle = rc + 1;
}
rc = entity_association_pdr_add(curr->next_sibling, repo, entities,
num_entities, is_remote,
terminus_handle, record_handle);
- if (rc) {
+ if (rc < 0) {
return rc;
}
+ // entity_association_pdr_add return record handle in success
+ // case. If the pdr gets added to the repo, new record handle
+ // will be returned. Below check confirms if the pdr is added
+ // to the repo and increments the record handle
+ if (record_handle != rc) {
+ if (rc >= UINT32_MAX) {
+ return -EOVERFLOW;
+ }
+ record_handle = rc + 1;
+ }
- return entity_association_pdr_add(curr->first_child, repo, entities,
- num_entities, is_remote,
- terminus_handle, record_handle);
+ rc = entity_association_pdr_add(curr->first_child, repo, entities,
+ num_entities, is_remote,
+ terminus_handle, record_handle);
+ return rc;
}
LIBPLDM_ABI_STABLE
@@ -954,9 +988,10 @@
if (!tree || !repo) {
return 0;
}
-
- return entity_association_pdr_add(tree->root, repo, NULL, 0, is_remote,
- terminus_handle, 0);
+ int64_t rc = entity_association_pdr_add(tree->root, repo, NULL, 0,
+ is_remote, terminus_handle, 0);
+ assert(rc >= INT_MIN);
+ return (rc < 0) ? (int)rc : 0;
}
LIBPLDM_ABI_STABLE
@@ -979,9 +1014,12 @@
return -EINVAL;
}
- return entity_association_pdr_add(node, repo, entities, num_entities,
- is_remote, terminus_handle,
- record_handle);
+ int64_t rc = entity_association_pdr_add(node, repo, entities,
+ num_entities, is_remote,
+ terminus_handle, record_handle);
+
+ assert(rc >= INT_MIN);
+ return (rc < 0) ? (int)rc : 0;
}
static void find_entity_ref_in_tree(pldm_entity_node *tree_node,
@@ -1343,22 +1381,27 @@
sizeof(struct pldm_pdr_entity_association)) {
return;
}
+
struct pldm_pdr_entity_association *entity_association_pdr =
(struct pldm_pdr_entity_association *)start;
- if (entity_association_pdr->num_children == UINT8_MAX) {
+ size_t l_num_entities = entity_association_pdr->num_children;
+
+ if (l_num_entities == 0) {
return;
}
- size_t l_num_entities = entity_association_pdr->num_children + 1;
- if (l_num_entities < 2) {
+ if ((pdr_len - sizeof(struct pldm_pdr_hdr)) / sizeof(pldm_entity) <
+ l_num_entities) {
return;
}
- if (UINT8_MAX < l_num_entities) {
+ if (l_num_entities >= (size_t)UINT8_MAX) {
return;
}
+ l_num_entities++;
+
pldm_entity *l_entities = calloc(l_num_entities, sizeof(pldm_entity));
if (!l_entities) {
return;
diff --git a/tests/dsp/pdr.cpp b/tests/dsp/pdr.cpp
index 2ca79b7..e8e2256 100644
--- a/tests/dsp/pdr.cpp
+++ b/tests/dsp/pdr.cpp
@@ -11,6 +11,83 @@
#include <gtest/gtest.h>
+typedef struct pldm_association_pdr_test
+{
+ uint32_t record_handle;
+ uint8_t version;
+ uint8_t type;
+ uint16_t record_change_num;
+ uint16_t length;
+ uint16_t container_id;
+ uint8_t association_type;
+ uint8_t num_children;
+
+ inline bool operator==(pldm_association_pdr_test pdr) const
+ {
+ return (record_handle == pdr.record_handle) && (type == pdr.type) &&
+ (length == pdr.length) && (container_id == pdr.container_id) &&
+ (association_type == pdr.association_type) &&
+ (num_children == pdr.num_children);
+ }
+} pldm_association_pdr_test;
+
+typedef struct pldm_entity_test
+{
+ uint16_t entity_type;
+ uint16_t entity_instance_num;
+ uint16_t entity_container_id;
+
+ inline bool operator==(pldm_entity_test entity) const
+ {
+ return (entity_type == entity.entity_type) &&
+ (entity_instance_num == entity.entity_instance_num) &&
+ (entity_container_id == entity.entity_container_id);
+ }
+} pldm_entity_test;
+
+static void getEntity(struct pldm_msgbuf* buf, pldm_entity_test& entity)
+{
+ pldm_msgbuf_extract_uint16(buf, entity.entity_type);
+ pldm_msgbuf_extract_uint16(buf, entity.entity_instance_num);
+ pldm_msgbuf_extract_uint16(buf, entity.entity_container_id);
+}
+
+static void
+ getAssociationPdrDetails(struct pldm_msgbuf* buf,
+ pldm_association_pdr_test& association_pdr_test)
+{
+ pldm_msgbuf_extract_uint32(buf, association_pdr_test.record_handle);
+ pldm_msgbuf_extract_uint8(buf, association_pdr_test.version);
+ pldm_msgbuf_extract_uint8(buf, association_pdr_test.type);
+ pldm_msgbuf_extract_uint16(buf, association_pdr_test.record_change_num);
+ pldm_msgbuf_extract_uint16(buf, association_pdr_test.length);
+
+ pldm_msgbuf_extract_uint16(buf, association_pdr_test.container_id);
+ pldm_msgbuf_extract_uint8(buf, association_pdr_test.association_type);
+}
+
+static void
+ verifyEntityAssociationPdr(struct pldm_msgbuf* buf,
+ const pldm_association_pdr_test& association_pdr,
+ const pldm_entity_test& container_entity1,
+ const pldm_entity_test& child_entity1)
+{
+ struct pldm_entity_test container_entity = {};
+ struct pldm_entity_test child_entity = {};
+ pldm_association_pdr_test association_pdr_test{};
+
+ getAssociationPdrDetails(buf, association_pdr_test);
+ getEntity(buf, container_entity);
+ pldm_msgbuf_extract_uint8(buf, association_pdr_test.num_children);
+ getEntity(buf, child_entity);
+
+ ASSERT_EQ(pldm_msgbuf_destroy_consumed(buf), 0);
+
+ EXPECT_TRUE(association_pdr_test == association_pdr);
+ EXPECT_TRUE(container_entity == container_entity1);
+ EXPECT_TRUE(child_entity == child_entity1);
+}
+
TEST(PDRAccess, testInit)
{
auto repo = pldm_pdr_init();
@@ -1426,6 +1503,103 @@
pldm_entity_association_tree_destroy(tree);
}
+TEST(EntityAssociationPDR, testPDRWithRecordHandle)
+{
+ // e = entity type, c = container id, i = instance num
+
+ // INPUT
+ // 1(e=1)
+ // |
+ // 2(e=2)--3(e=2)
+
+ // Expected OUTPUT
+ // 1(e=1,c=0,i=1)
+ // |
+ // 2(e=2,c=1,i=1)--3(e=2,c=1,i=2)
+
+ pldm_entity entities[3] = {{1, 1, 0}, {2, 1, 1}, {3, 1, 1}};
+ pldm_entity_test testEntities[3] = {{1, 1, 0}, {2, 1, 1}, {3, 1, 1}};
+
+ auto tree = pldm_entity_association_tree_init();
+
+ auto l1 = pldm_entity_association_tree_add(
+ tree, &entities[0], 0xffff, nullptr, PLDM_ENTITY_ASSOCIAION_PHYSICAL);
+ ASSERT_NE(l1, nullptr);
+
+ auto l2a = pldm_entity_association_tree_add(
+ tree, &entities[1], 0xffff, l1, PLDM_ENTITY_ASSOCIAION_PHYSICAL);
+ ASSERT_NE(l2a, nullptr);
+ pldm_entity_association_tree_add(tree, &entities[2], 0xffff, l1,
+ PLDM_ENTITY_ASSOCIAION_LOGICAL);
+ auto repo = pldm_pdr_init();
+ pldm_entity* l_entities = entities;
+
+ pldm_entity_node* node = nullptr;
+ pldm_find_entity_ref_in_tree(tree, entities[0], &node);
+
+ ASSERT_NE(node, nullptr);
+
+ int numEntities = 3;
+ pldm_entity_association_pdr_add_from_node_with_record_handle(
+ node, repo, &l_entities, numEntities, true, 1, (10));
+
+ EXPECT_EQ(pldm_pdr_get_record_count(repo), 2u);
+
+ uint32_t currRecHandle{};
+ uint32_t nextRecHandle{};
+ uint8_t* data = nullptr;
+ uint32_t size{};
+
+ pldm_pdr_find_record(repo, currRecHandle, &data, &size, &nextRecHandle);
+
+ struct pldm_msgbuf _buf;
+ struct pldm_msgbuf* buf = &_buf;
+
+ auto rc =
+ pldm_msgbuf_init_errno(buf,
+ (sizeof(struct pldm_pdr_hdr) +
+ sizeof(struct pldm_pdr_entity_association)),
+ data, size);
+ ASSERT_EQ(rc, 0);
+
+ pldm_association_pdr_test association_pdr = pldm_association_pdr_test{
+ 10,
+ 1,
+ PLDM_PDR_ENTITY_ASSOCIATION,
+ 1,
+ static_cast<uint16_t>(size - sizeof(struct pldm_pdr_hdr)),
+ 1,
+ PLDM_ENTITY_ASSOCIAION_LOGICAL,
+ 1};
+
+ verifyEntityAssociationPdr(buf, association_pdr, testEntities[0],
+ testEntities[2]);
+
+ currRecHandle = nextRecHandle;
+ pldm_pdr_find_record(repo, currRecHandle, &data, &size, &nextRecHandle);
+ rc = pldm_msgbuf_init_errno(buf,
+ (sizeof(struct pldm_pdr_hdr) +
+ sizeof(struct pldm_pdr_entity_association)),
+ data, size);
+ ASSERT_EQ(rc, 0);
+
+ pldm_association_pdr_test association_pdr1 = pldm_association_pdr_test{
+ 11,
+ 1,
+ PLDM_PDR_ENTITY_ASSOCIATION,
+ 1,
+ static_cast<uint16_t>(size - sizeof(struct pldm_pdr_hdr)),
+ 1,
+ PLDM_ENTITY_ASSOCIAION_PHYSICAL,
+ 1};
+
+ verifyEntityAssociationPdr(buf, association_pdr1, testEntities[0],
+ testEntities[1]);
+
+ pldm_pdr_destroy(repo);
+ pldm_entity_association_tree_destroy(tree);
+}
+
TEST(EntityAssociationPDR, testFind)
{
// 1
@@ -1562,12 +1736,12 @@
{
std::vector<uint8_t> pdr{};
pdr.resize(sizeof(pldm_pdr_hdr) + sizeof(pldm_pdr_entity_association) +
- sizeof(pldm_entity) * 5);
+ sizeof(pldm_entity) * 4);
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
pldm_pdr_hdr* hdr = reinterpret_cast<pldm_pdr_hdr*>(pdr.data());
hdr->type = PLDM_PDR_ENTITY_ASSOCIATION;
hdr->length =
- htole16(sizeof(pldm_pdr_entity_association) + sizeof(pldm_entity) * 5);
+ htole16(sizeof(pldm_pdr_entity_association) + sizeof(pldm_entity) * 4);
pldm_pdr_entity_association* e =
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)