From c6f3f1ff314d517ee7deeb03cdfde9701106ff55 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Mon, 23 Jun 2025 13:15:53 -0700 Subject: [PATCH] [message] disallow `SetPriority()` on enqueued messages in `PriorityQueue` (#11624) This commit modifies `Message::SetPriority()` to prevent changing a message's priority after it has been enqueued in a `PriorityQueue`. Attempting to do so will now return `kErrorInvalidState`. The functionality for altering the priority of an already-enqueued message is not currently used or required. Should this behavior become necessary in the future, the recommended approach is to explicitly dequeue the message first, then change its priority, and finally re-add it to the queue. This makes the intended behavior clearer and more explicit within the code. This commit also updates the `test_priority_queue` unit test to reflect this change. --- src/core/common/message.cpp | 22 ++------ src/core/common/message.hpp | 8 ++- tests/unit/test_priority_queue.cpp | 82 +++--------------------------- 3 files changed, 17 insertions(+), 95 deletions(-) diff --git a/src/core/common/message.cpp b/src/core/common/message.cpp index 6a627257d..e2ff22982 100644 --- a/src/core/common/message.cpp +++ b/src/core/common/message.cpp @@ -328,31 +328,17 @@ bool Message::IsMleCommand(Mle::Command aMleCommand) const Error Message::SetPriority(Priority aPriority) { - Error error = kErrorNone; - uint8_t priority = static_cast(aPriority); - PriorityQueue *priorityQueue; + Error error = kErrorNone; + uint8_t priority = static_cast(aPriority); static_assert(kNumPriorities <= 4, "`Metadata::mPriority` as a 2-bit field cannot fit all `Priority` values"); VerifyOrExit(priority < kNumPriorities, error = kErrorInvalidArgs); - VerifyOrExit(IsInAQueue(), GetMetadata().mPriority = priority); - VerifyOrExit(GetMetadata().mPriority != priority); - - priorityQueue = GetPriorityQueue(); - - if (priorityQueue != nullptr) - { - priorityQueue->Dequeue(*this); - } + VerifyOrExit(!IsInAPriorityQueue(), error = kErrorInvalidState); GetMetadata().mPriority = priority; - if (priorityQueue != nullptr) - { - priorityQueue->Enqueue(*this); - } - exit: return error; } @@ -869,7 +855,7 @@ void Message::SetMessageQueue(MessageQueue *aMessageQueue) void Message::SetPriorityQueue(PriorityQueue *aPriorityQueue) { GetMetadata().mQueue = aPriorityQueue; - GetMetadata().mInPriorityQ = true; + GetMetadata().mInPriorityQ = aPriorityQueue != nullptr; } //--------------------------------------------------------------------------------------------------------------------- diff --git a/src/core/common/message.hpp b/src/core/common/message.hpp index f8cde13a8..17679ab9f 100644 --- a/src/core/common/message.hpp +++ b/src/core/common/message.hpp @@ -633,13 +633,15 @@ public: /** * Sets the messages priority. - * If the message is already queued in a priority queue, changing the priority ensures to - * update the message in the associated queue. + * + * If the message is already enqueued in a priority queue, the priority cannot be changed and `kErrorInvalidState` + * is returned. * * @param[in] aPriority The message priority level. * * @retval kErrorNone Successfully set the priority for the message. * @retval kErrorInvalidArgs Priority level is not invalid. + * @retval kErrorInvalidState Message is already queued in a priority queue. */ Error SetPriority(Priority aPriority); @@ -1569,6 +1571,8 @@ private: } bool IsInAQueue(void) const { return (GetMetadata().mQueue != nullptr); } + bool IsInAPriorityQueue(void) const { return GetMetadata().mInPriorityQ; } + void SetMessageQueue(MessageQueue *aMessageQueue); void SetPriorityQueue(PriorityQueue *aPriorityQueue); diff --git a/tests/unit/test_priority_queue.cpp b/tests/unit/test_priority_queue.cpp index 4642ee1e1..5cbf39334 100644 --- a/tests/unit/test_priority_queue.cpp +++ b/tests/unit/test_priority_queue.cpp @@ -135,44 +135,11 @@ void VerifyPriorityQueueContent(PriorityQueue &aPriorityQueue, int aExpectedLeng VerifyOrQuit(message == nullptr, "`for` loop iteration resulted in fewer entries than expected"); } -// This function verifies the content of the message queue to match the passed in messages -void VerifyMsgQueueContent(MessageQueue &aMessageQueue, int aExpectedLength, ...) -{ - va_list args; - Message *message; - Message *msgArg; - - va_start(args, aExpectedLength); - - if (aExpectedLength == 0) - { - message = aMessageQueue.GetHead(); - VerifyOrQuit(message == nullptr, "is not empty when expected len is zero."); - } - else - { - for (message = aMessageQueue.GetHead(); message != nullptr; message = message->GetNext()) - { - VerifyOrQuit(aExpectedLength != 0, "contains more entries than expected"); - - msgArg = va_arg(args, Message *); - VerifyOrQuit(msgArg == message, "content does not match what is expected."); - - aExpectedLength--; - } - - VerifyOrQuit(aExpectedLength == 0, "contains less entries than expected"); - } - - va_end(args); -} - void TestPriorityQueue(void) { Instance *instance; MessagePool *messagePool; PriorityQueue queue; - MessageQueue messageQueue; Message *msgNet[kNumTestMessages]; Message *msgHigh[kNumTestMessages]; Message *msgNor[kNumTestMessages]; @@ -264,7 +231,7 @@ void TestPriorityQueue(void) queue.Dequeue(*msgHigh[3]); VerifyPriorityQueueContent(queue, 0); - // Change the priority of an already queued message and check the order change in the queue. + // Validate that priority of an already queued message in a priority queue cannot change queue.Enqueue(*msgNor[0]); VerifyPriorityQueueContent(queue, 1, msgNor[0]); queue.Enqueue(*msgHigh[0]); @@ -272,54 +239,19 @@ void TestPriorityQueue(void) queue.Enqueue(*msgLow[0]); VerifyPriorityQueueContent(queue, 3, msgHigh[0], msgNor[0], msgLow[0]); - SuccessOrQuit(msgNor[0]->SetPriority(Message::kPriorityNet)); - VerifyPriorityQueueContent(queue, 3, msgNor[0], msgHigh[0], msgLow[0]); - SuccessOrQuit(msgLow[0]->SetPriority(Message::kPriorityLow)); - VerifyPriorityQueueContent(queue, 3, msgNor[0], msgHigh[0], msgLow[0]); - SuccessOrQuit(msgLow[0]->SetPriority(Message::kPriorityNormal)); - VerifyPriorityQueueContent(queue, 3, msgNor[0], msgHigh[0], msgLow[0]); - SuccessOrQuit(msgLow[0]->SetPriority(Message::kPriorityHigh)); - VerifyPriorityQueueContent(queue, 3, msgNor[0], msgHigh[0], msgLow[0]); - SuccessOrQuit(msgLow[0]->SetPriority(Message::kPriorityNet)); - VerifyPriorityQueueContent(queue, 3, msgNor[0], msgLow[0], msgHigh[0]); - SuccessOrQuit(msgNor[0]->SetPriority(Message::kPriorityNormal)); - SuccessOrQuit(msgLow[0]->SetPriority(Message::kPriorityLow)); + VerifyOrQuit(msgNor[0]->SetPriority(Message::kPriorityNet) == kErrorInvalidState); + VerifyOrQuit(msgLow[0]->SetPriority(Message::kPriorityLow) == kErrorInvalidState); + VerifyOrQuit(msgLow[0]->SetPriority(Message::kPriorityNormal) == kErrorInvalidState); + VerifyOrQuit(msgHigh[0]->SetPriority(Message::kPriorityNormal) == kErrorInvalidState); VerifyPriorityQueueContent(queue, 3, msgHigh[0], msgNor[0], msgLow[0]); - messageQueue.Enqueue(*msgNor[1]); - messageQueue.Enqueue(*msgHigh[1]); - messageQueue.Enqueue(*msgNet[1]); - VerifyMsgQueueContent(messageQueue, 3, msgNor[1], msgHigh[1], msgNet[1]); - - // Change priority of message and check for not in messageQueue. - SuccessOrQuit(msgNor[1]->SetPriority(Message::kPriorityNet)); - VerifyMsgQueueContent(messageQueue, 3, msgNor[1], msgHigh[1], msgNet[1]); - - SuccessOrQuit(msgLow[0]->SetPriority(Message::kPriorityHigh)); - VerifyPriorityQueueContent(queue, 3, msgHigh[0], msgLow[0], msgNor[0]); - VerifyMsgQueueContent(messageQueue, 3, msgNor[1], msgHigh[1], msgNet[1]); - - // Remove messages from the two queues + // Remove messages from the queue queue.Dequeue(*msgHigh[0]); - VerifyPriorityQueueContent(queue, 2, msgLow[0], msgNor[0]); - VerifyMsgQueueContent(messageQueue, 3, msgNor[1], msgHigh[1], msgNet[1]); - - messageQueue.Dequeue(*msgNet[1]); - VerifyPriorityQueueContent(queue, 2, msgLow[0], msgNor[0]); - VerifyMsgQueueContent(messageQueue, 2, msgNor[1], msgHigh[1]); - - messageQueue.Dequeue(*msgHigh[1]); - VerifyPriorityQueueContent(queue, 2, msgLow[0], msgNor[0]); - VerifyMsgQueueContent(messageQueue, 1, msgNor[1]); - + VerifyPriorityQueueContent(queue, 2, msgNor[0], msgLow[0]); queue.Dequeue(*msgLow[0]); VerifyPriorityQueueContent(queue, 1, msgNor[0]); - VerifyMsgQueueContent(messageQueue, 1, msgNor[1]); - queue.Dequeue(*msgNor[0]); VerifyPriorityQueueContent(queue, 0); - messageQueue.Dequeue(*msgNor[1]); - VerifyMsgQueueContent(messageQueue, 0); for (Message *message : msgNor) {