I have no preference. Please go ahead.
> -----Original Message----- > From: Marvin H?user [mailto:[email protected]] > Sent: Thursday, August 3, 2017 11:43 PM > To: [email protected] > Cc: Gao, Liming <[email protected]>; Kinney, Michael D > <[email protected]> > Subject: RE: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList > verifications. > > Hey Liming, > > I noticed that and assumed that a second call is not going to hurt much (only > used in DEBUG afterall). > To work around that, the macro could be modified to either call > IsNodeInList() or InternalBaseLibIsListValid() based on the PCD value, > though one would have to know/check the PCD's value to know what actually > triggered the ASSERT(), so I opted to separate the > ASSERTs (this also looks cleaner in my opinion). > The macro could also be modified to carry two separate ASSERT calls, which > would solve the issue above, > though I would need to think of a proper name for a macro that checks list > membership *and* whether the list is valid. > If you have a preference, please let me know, otherwise I'll come up with a > V3 soon. > > Thanks and regards, > Marvin. > > > -----Original Message----- > > From: Gao, Liming [mailto:[email protected]] > > Sent: Thursday, August 3, 2017 5:28 PM > > To: [email protected]; [email protected] > > Cc: Kinney, Michael D <[email protected]> > > Subject: RE: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList > > verifications. > > > > 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
