From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B92CD21AEB0A3 for ; Thu, 3 Aug 2017 08:25:37 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga105.fm.intel.com with ESMTP; 03 Aug 2017 08:27:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,316,1498546800"; d="scan'208";a="135701798" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga006.fm.intel.com with ESMTP; 03 Aug 2017 08:27:49 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 3 Aug 2017 08:27:48 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 3 Aug 2017 08:27:48 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.146]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.122]) with mapi id 14.03.0319.002; Thu, 3 Aug 2017 23:27:46 +0800 From: "Gao, Liming" To: "Marvin.Haeuser@outlook.com" , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" Thread-Topic: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications. Thread-Index: AQHTC5DsjyJXg/Rms0aldga9ZPjpbqJywjWg Date: Thu, 3 Aug 2017 15:27:45 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75F52F@shsmsx102.ccr.corp.intel.com> References: <20170802131136.3128-1-Marvin.Haeuser@outlook.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Aug 2017 15:25:38 -0000 Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Marvin: If PcdVerifyNodeInList is set TRUE, InternalBaseLibIsListValid() will be = checked twice in two ASSERT(). In the original logic, InternalBaseLibIsList= Valid 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 Ma= rvin H?user > Sent: Wednesday, August 2, 2017 9:12 PM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D ; Gao, Liming > Subject: [edk2] [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList= verifications. >=20 > 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. >=20 > 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. >=20 > V2: > - Fix IsListEmpty() to ASSERT when the passed list is invalid. > - Introduce the VERIFY_IS_NODE_IN_LIST() macro to only verify whether t= he > passed node is part of the list when PcdVerifyNodeInList is TRUE. >=20 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marvin Haeuser > --- > MdePkg/Library/BaseLib/LinkedList.c | 105 +++++++++----------- > 1 file changed, 47 insertions(+), 58 deletions(-) >=20 > 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" >=20 > /** > - Worker function that locates the Node in the List. > + If PcdVerifyNodeInList is TRUE, checks whether FirstNode and SecondNod= e are > + part of the same doubly-linked list. >=20 > - 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 FirstE= ntry > + or PcdVerifyNodeInList is FALSE. > + @retval FALSE SecondEntry isn't in the same doubly-linked list as Fir= stEntry, > + 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. >=20 > 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(). >=20 > @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 Nod= e is a > - member of List. FALSE if no membership test= should > - be performed. >=20 > @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 !=3D NULL); > ASSERT (List->ForwardLink !=3D NULL); > ASSERT (List->BackLink !=3D NULL); > - ASSERT (Node !=3D NULL); > - > - Count =3D 0; > - Ptr =3D List; > - > - if (FeaturePcdGet (PcdVerifyNodeInList) && VerifyNodeInList) { > - // > - // Check to see if Node is a member of List. > - // Exit early if the number of nodes in List >=3D PcdMaximumLinkedLi= stLength > - // > - do { > - Ptr =3D 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 >=3D PcdGet32 (PcdMaximumLinkedListLength)) { > - return (BOOLEAN)(Ptr =3D=3D Node); > - } > - } > - } while ((Ptr !=3D List) && (Ptr !=3D Node)); > - > - if (Ptr !=3D Node) { > - return FALSE; > - } > - } >=20 > if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { > + Count =3D 0; > + Ptr =3D List; > + > // > // Count the total number of nodes in List. > // Exit early if the number of nodes in List >=3D PcdMaximumLinkedLi= stLength > @@ -104,9 +86,9 @@ InternalBaseLibIsNodeInList ( > } while ((Ptr !=3D List) && (Count < PcdGet32 (PcdMaximumLinkedListL= ength))); >=20 > // > - // ASSERT() if the linked list is too long > + // return whether linked list is too long > // > - ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength)); > + return (BOOLEAN)(Count < PcdGet32 (PcdMaximumLinkedListLength)); > } >=20 > 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)); >=20 > Entry->ForwardLink =3D ListHead->ForwardLink; > Entry->BackLink =3D 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)); >=20 > Entry->ForwardLink =3D ListHead; > Entry->BackLink =3D ListHead->BackLink; > @@ -323,7 +307,7 @@ GetFirstNode ( > // > // ASSERT List not too long > // > - ASSERT (InternalBaseLibIsNodeInList (List, List, FALSE)); > + ASSERT (InternalBaseLibIsListValid (List)); >=20 > 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)); >=20 > 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)); >=20 > return Node->BackLink; > } > @@ -428,7 +414,7 @@ IsListEmpty ( > // > // ASSERT List not too long > // > - ASSERT (InternalBaseLibIsNodeInList (ListHead, ListHead, FALSE)); > + ASSERT (InternalBaseLibIsListValid (ListHead)); >=20 > return (BOOLEAN)(ListHead->ForwardLink =3D=3D 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)); >=20 > return (BOOLEAN)(Node =3D=3D 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)); >=20 > return (BOOLEAN)(!IsNull (List, Node) && List->BackLink =3D=3D 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)); >=20 > // > // Ptr is the node pointed to by FirstEntry->ForwardLink > -- > 2.12.2.windows.2 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel