[message] disallow SetPriority() on enqueued messages in PriorityQueue (openthread#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.

Author: abtink

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<uint8_t>(aPriority);
-    PriorityQueue *priorityQueue;
+    Error   error    = kErrorNone;
+    uint8_t priority = static_cast<uint8_t>(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;
 }
 
 //---------------------------------------------------------------------------------------------------------------------

src/core/common/message.hpp

@@ -633,13 +633,15 @@ class Message : public otMessage, public Buffer, public GetProvider<Message>
 
     /**
      * 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 @@ class Message : public otMessage, public Buffer, public GetProvider<Message>
     }
 
     bool IsInAQueue(void) const { return (GetMetadata().mQueue != nullptr); }
+    bool IsInAPriorityQueue(void) const { return GetMetadata().mInPriorityQ; }
+
     void SetMessageQueue(MessageQueue *aMessageQueue);
     void SetPriorityQueue(PriorityQueue *aPriorityQueue);
 

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,62 +231,27 @@ 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]);
     VerifyPriorityQueueContent(queue, 2, msgHigh[0], msgNor[0]);
     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)
     {