Re: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications.

["Gao, Liming" <liming.gao@intel.com>]

I have no preference. Please go ahead. 

> -----Original Message-----
> From: Marvin H?user [mailto:Marvin.Haeuser@outlook.com]
> Sent: Thursday, August 3, 2017 11:43 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> 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:liming.gao@intel.com]
> > Sent: Thursday, August 3, 2017 5:28 PM
> > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> > 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:edk2-devel-bounces@lists.01.org] On Behalf Of
> > > Marvin H?user
> > > Sent: Wednesday, August 2, 2017 9:12 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>
> > > 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 <Marvin.Haeuser@outlook.com>
> > > ---
> > >  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
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel