Marvin: If PcdVerifyNodeInList is set TRUE, InternalBaseLibIsListValid() will be checked twice in two ASSERT(). In the original logic, InternalBaseLibIsListValid only runs once. Could we work out the same check logic?
Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of Marvin > H?user > Sent: Wednesday, August 2, 2017 9:12 PM > To: [email protected] > Cc: Kinney, Michael D <[email protected]>; Gao, Liming > <[email protected]> > Subject: [edk2] [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList > verifications. > > 1) Replace InternalBaseLibIsNodeInList() with > InternalBaseLibIsListValid(). > - The verification whether Node is within the doubly-linked List > is now done by IsNodeInList(). > - Whether the list is valid is returned. > > 2) The comments within InsertHeadList() and InsertTailList() stated > that it is checked whether Entry is not part of the doubly-linked > list. This was not done as argument 3 of > InternalBaseLibIsNodeInList() indicated whether the check is done, > not whether to check if the node is or is not in the list. This > has been fixed by using IsNodeInList() for the ASSERTs. > > V2: > - Fix IsListEmpty() to ASSERT when the passed list is invalid. > - Introduce the VERIFY_IS_NODE_IN_LIST() macro to only verify whether the > passed node is part of the list when PcdVerifyNodeInList is TRUE. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marvin Haeuser <[email protected]> > --- > MdePkg/Library/BaseLib/LinkedList.c | 105 +++++++++----------- > 1 file changed, 47 insertions(+), 58 deletions(-) > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c > b/MdePkg/Library/BaseLib/LinkedList.c > index 484ee836b8c2..6864fae7d939 100644 > --- a/MdePkg/Library/BaseLib/LinkedList.c > +++ b/MdePkg/Library/BaseLib/LinkedList.c > @@ -15,25 +15,38 @@ > #include "BaseLibInternals.h" > > /** > - Worker function that locates the Node in the List. > + If PcdVerifyNodeInList is TRUE, checks whether FirstNode and SecondNode are > + part of the same doubly-linked list. > > - By searching the List, finds the location of the Node in List. At the same > time, > - verifies the validity of this list. > + If FirstEntry is NULL, then ASSERT(). > + If FirstEntry->ForwardLink is NULL, then ASSERT(). > + If FirstEntry->BackLink is NULL, then ASSERT(). > + If SecondEntry is NULL, then ASSERT(); > + If PcdMaximumLinkedListLength is not zero, and List contains more than > + PcdMaximumLinkedListLength nodes, then ASSERT(). > + > + @param FirstEntry A pointer to a node in a linked list. > + @param SecondEntry A pointer to the node to locate. > + > + @retval TRUE SecondEntry is in the same doubly-linked list as FirstEntry > + or PcdVerifyNodeInList is FALSE. > + @retval FALSE SecondEntry isn't in the same doubly-linked list as > FirstEntry, > + or FirstEntry is invalid. > + > +**/ > +#define VERIFY_IS_NODE_IN_LIST(FirstEntry, SecondEntry) \ > + (!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList ((FirstEntry), > (SecondEntry))) > + > +/** > + Worker function that verifies the validity of this list. > > If List is NULL, then ASSERT(). > If List->ForwardLink is NULL, then ASSERT(). > - If List->backLink is NULL, then ASSERT(). > - If Node is NULL, then ASSERT(). > - If PcdVerifyNodeInList is TRUE and DoMembershipCheck is TRUE and Node > - is in not a member of List, then return FALSE > + If List->BackLink is NULL, then ASSERT(). > If PcdMaximumLinkedListLength is not zero, and List contains more than > PcdMaximumLinkedListLength nodes, then ASSERT(). > > @param List A pointer to a node in a linked list. > - @param Node A pointer to a node in a linked list. > - @param VerifyNodeInList TRUE if a check should be made to see if Node is > a > - member of List. FALSE if no membership test > should > - be performed. > > @retval TRUE if PcdVerifyNodeInList is FALSE > @retval TRUE if DoMembershipCheck is FALSE > @@ -45,10 +58,8 @@ > **/ > BOOLEAN > EFIAPI > -InternalBaseLibIsNodeInList ( > - IN CONST LIST_ENTRY *List, > - IN CONST LIST_ENTRY *Node, > - IN BOOLEAN VerifyNodeInList > +InternalBaseLibIsListValid ( > + IN CONST LIST_ENTRY *List > ) > { > UINTN Count; > @@ -60,40 +71,11 @@ InternalBaseLibIsNodeInList ( > ASSERT (List != NULL); > ASSERT (List->ForwardLink != NULL); > ASSERT (List->BackLink != NULL); > - ASSERT (Node != NULL); > - > - Count = 0; > - Ptr = List; > - > - if (FeaturePcdGet (PcdVerifyNodeInList) && VerifyNodeInList) { > - // > - // Check to see if Node is a member of List. > - // Exit early if the number of nodes in List >= > PcdMaximumLinkedListLength > - // > - do { > - Ptr = Ptr->ForwardLink; > - if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { > - Count++; > - // > - // ASSERT() if the linked list is too long > - // > - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); > - > - // > - // Return if the linked list is too long > - // > - if (Count >= PcdGet32 (PcdMaximumLinkedListLength)) { > - return (BOOLEAN)(Ptr == Node); > - } > - } > - } while ((Ptr != List) && (Ptr != Node)); > - > - if (Ptr != Node) { > - return FALSE; > - } > - } > > if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { > + Count = 0; > + Ptr = List; > + > // > // Count the total number of nodes in List. > // Exit early if the number of nodes in List >= > PcdMaximumLinkedListLength > @@ -104,9 +86,9 @@ InternalBaseLibIsNodeInList ( > } while ((Ptr != List) && (Count < PcdGet32 > (PcdMaximumLinkedListLength))); > > // > - // ASSERT() if the linked list is too long > + // return whether linked list is too long > // > - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); > + return (BOOLEAN)(Count < PcdGet32 (PcdMaximumLinkedListLength)); > } > > return TRUE; > @@ -244,7 +226,8 @@ InsertHeadList ( > // > // ASSERT List not too long and Entry is not one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > + ASSERT (InternalBaseLibIsListValid (ListHead)); > + ASSERT (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); > > Entry->ForwardLink = ListHead->ForwardLink; > Entry->BackLink = ListHead; > @@ -285,7 +268,8 @@ InsertTailList ( > // > // ASSERT List not too long and Entry is not one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > + ASSERT (InternalBaseLibIsListValid (ListHead)); > + ASSERT (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); > > Entry->ForwardLink = ListHead; > Entry->BackLink = ListHead->BackLink; > @@ -323,7 +307,7 @@ GetFirstNode ( > // > // ASSERT List not too long > // > - ASSERT (InternalBaseLibIsNodeInList (List, List, FALSE)); > + ASSERT (InternalBaseLibIsListValid (List)); > > return List->ForwardLink; > } > @@ -359,7 +343,8 @@ GetNextNode ( > // > // ASSERT List not too long and Node is one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > + ASSERT (InternalBaseLibIsListValid (List)); > + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); > > return Node->ForwardLink; > } > @@ -395,7 +380,8 @@ GetPreviousNode ( > // > // ASSERT List not too long and Node is one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > + ASSERT (InternalBaseLibIsListValid (List)); > + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); > > return Node->BackLink; > } > @@ -428,7 +414,7 @@ IsListEmpty ( > // > // ASSERT List not too long > // > - ASSERT (InternalBaseLibIsNodeInList (ListHead, ListHead, FALSE)); > + ASSERT (InternalBaseLibIsListValid (ListHead)); > > return (BOOLEAN)(ListHead->ForwardLink == ListHead); > } > @@ -469,7 +455,8 @@ IsNull ( > // > // ASSERT List not too long and Node is one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > + ASSERT (InternalBaseLibIsListValid (List)); > + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); > > return (BOOLEAN)(Node == List); > } > @@ -507,7 +494,8 @@ IsNodeAtEnd ( > // > // ASSERT List not too long and Node is one of the nodes of List > // > - ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE)); > + ASSERT (InternalBaseLibIsListValid (List)); > + ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node)); > > return (BOOLEAN)(!IsNull (List, Node) && List->BackLink == Node); > } > @@ -554,7 +542,8 @@ SwapListEntries ( > // > // ASSERT Entry1 and Entry2 are in the same linked list > // > - ASSERT (InternalBaseLibIsNodeInList (FirstEntry, SecondEntry, TRUE)); > + ASSERT (InternalBaseLibIsListValid (FirstEntry)); > + ASSERT (VERIFY_IS_NODE_IN_LIST (FirstEntry, SecondEntry)); > > // > // Ptr is the node pointed to by FirstEntry->ForwardLink > -- > 2.12.2.windows.2 > > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
