From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR02-VE1-obe.outbound.protection.outlook.com (mail-oln040092069085.outbound.protection.outlook.com [40.92.69.85]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1C4B22095DB87 for ; Thu, 3 Aug 2017 08:40:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=EzPEVl/WjRUYWo7P8GmEDz+A4C5kAmnoquWvpCSSjRs=; b=q7vAsElDWkkAOuX6D5aYIiQRTm+JriT1nGXdIwlbVY2HAl+5uI2ZMqdPvRVr/MM8DaApKdt7cLFaP82bDnImr741mJ5L1vItdxv4j/9U65n8aMqpDZ6rUfqE3NmEbZ0iDSXHOvWlkbHEUcQRrot/EJ3fO3FOcBrR9csqFuv+NZxCNrJ7gb2lqo9cTNuDhkrEDoodZCcD2eAMQDAwMPMGwJ+GCpx5u1e+OBU6QI6pSWZYkjG6GLMKu5tJckpGyEb9ENTs5rswcvd38oYBOekU2e/zz/p8o+eJApn4nGP+VXLWQeiivYBKtQfPbQ3qvVxpfm/MeBGy3Fc4h4SOZVZpqw== Received: from AM5EUR02FT019.eop-EUR02.prod.protection.outlook.com (10.152.8.59) by AM5EUR02HT091.eop-EUR02.prod.protection.outlook.com (10.152.9.129) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.1304.16; Thu, 3 Aug 2017 15:42:57 +0000 Received: from AM4PR06MB1491.eurprd06.prod.outlook.com (10.152.8.57) by AM5EUR02FT019.mail.protection.outlook.com (10.152.8.169) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1304.16 via Frontend Transport; Thu, 3 Aug 2017 15:42:57 +0000 Received: from AM4PR06MB1491.eurprd06.prod.outlook.com ([fe80::dcd0:60e6:90e0:871b]) by AM4PR06MB1491.eurprd06.prod.outlook.com ([fe80::dcd0:60e6:90e0:871b%13]) with mapi id 15.01.1304.025; Thu, 3 Aug 2017 15:42:57 +0000 From: Marvin H?user To: "edk2-devel@lists.01.org" CC: "Gao, Liming" , "michael.d.kinney@intel.com" Thread-Topic: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications. Thread-Index: AQHTC5DsjyJXg/Rms0aldga9ZPjpbqJywjWggAAB5/A= Date: Thu, 3 Aug 2017 15:42:56 +0000 Message-ID: References: <20170802131136.3128-1-Marvin.Haeuser@outlook.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D75F52F@shsmsx102.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75F52F@shsmsx102.ccr.corp.intel.com> Accept-Language: de-DE, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: lists.01.org; dkim=none (message not signed) header.d=none;lists.01.org; dmarc=none action=none header.from=outlook.com; x-incomingtopheadermarker: OriginalChecksum:9B01C7381DB359787E15F5B3F16EE8D5A40C364D17E9C03B36E13A690BFBBFA6; UpperCasedChecksum:06FDF12A0DB9C28B70A7BE26DAABE32017FDFD5661D722A1AAEDA7F05CE478CB; SizeAsReceived:7595; Count:45 x-tmn: [I6WsKVoK9OTDL10/WAGjSN4KYmNhMd1PC+unSUETknM01Omy6sQpNg7o1fiyqEaU] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM5EUR02HT091; 7:/qfioP296maHtqb1+JClJDpUT2FpyN8VZfdhYqe0yNojiY6oVsIjjz5Q34TOs5bLc8dAf4V1G5lxURrjc9doFEfv7V/aZioTSneHqRY66Dbf66uMnfBVLeKZxrLTyB/uyzp6fF8gKUWmGg60Ii2k85giZ2/6Mh+3rx/2B+zzahbh2qQQ1G+oQCjAXOX++xJwAxrkXVEeH1RkYV15S3mWk0XHvGlYkzUJT8fZC2/C3bvIait/2ykgqXK4WqYI9AJKK1LnTdn8o7/vCJr0urCN0hpdAPXr21TO8a5lzlP2PSCbf01WxhSC6Y8TA8hT4tyLK32iDnjaSgf+++mrWmSBERKBU0mJRLmTp8VNpTYZ9/UmJd62TqnM2k55sybk/Q/RhiwVbLVOQ6ahmiHf/b2dkqKm+EVlaHAwu6LetBgwnteyipsJrCogKj+u84kAvByAHIfcAjogEsCIpuANXNXsc91Q7tYkANoJ1dL6pjDGa+1Vo42SO5aioS0RPC6FBxx9PCVIXEBzALi+s58LVH8/TK8De4ddZNjeMyKctTIFVIKIl38Ex/EEq+7NK8jTFssiEZ59Ml4SI1GCjObviQVziCtNqGSWSZ30UR9OGuXHjLFfAhxXNLFfNq7gc6ee0abYD+n61Si+sXF5JUssBZDxnqQZaTk8OIPjtU8Yu6PaHpOux/XcPvbrpPcCamFMosOAn3qn/IaRLvsk2Opm+T8P8ivrdSes68RIh1o8mnyEDZuKbkr0sqRglHsyGUeFoXQxvKIM57YonOWokFIKkdRmPg== x-incomingheadercount: 45 x-eopattributedmessage: 0 x-forefront-antispam-report: EFV:NLI; SFV:NSPM; SFS:(7070007)(98901004); DIR:OUT; SFP:1901; SCL:1; SRVR:AM5EUR02HT091; H:AM4PR06MB1491.eurprd06.prod.outlook.com; FPR:; SPF:None; LANG:en; x-ms-office365-filtering-correlation-id: 3560fad4-b218-49cf-6123-08d4da865017 x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(300000500095)(300135000095)(300000501095)(300135300095)(22001)(300000502095)(300135100095)(300000503095)(300135400095)(201702061074)(5061506573)(5061507331)(1603103135)(2017031320274)(2017031324274)(2017031323274)(2017031322377)(1603101448)(1601125374)(1701031045)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:AM5EUR02HT091; x-ms-traffictypediagnostic: AM5EUR02HT091: x-exchange-antispam-report-test: UriScan:(189930954265078)(788757137089)(162533806227266)(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(444000031); SRVR:AM5EUR02HT091; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:AM5EUR02HT091; x-forefront-prvs: 03883BD916 spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Aug 2017 15:42:56.9327 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Internet X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5EUR02HT091 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:40:49 -0000 Content-Language: de-DE Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hey Liming, I noticed that and assumed that a second call is not going to hurt much (on= ly used in DEBUG afterall). To work around that, the macro could be modified to either call IsNodeInLis= t() or InternalBaseLibIsListValid() based on the PCD value, though one would have to know/check the PCD's value to know what actually t= riggered 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 > Subject: RE: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList > verifications. >=20 > Marvin: > If PcdVerifyNodeInList is set TRUE, InternalBaseLibIsListValid() will b= e > checked twice in two ASSERT(). In the original logic, InternalBaseLibIsLi= stValid > only runs once. Could we work out the same check logic? >=20 > 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 ; Gao, Liming > > > > Subject: [edk2] [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedLi= st > 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 > > --- > > 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 Firs= tEntry > > + 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 th= an > > 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 N= ode > is a > > - member of List. FALSE if no membership te= st 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 !=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 > PcdMaximumLinkedListLength > > - // > > - 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; > > - } > > - } > > > > 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 > > PcdMaximumLinkedListLength @@ -104,9 +86,9 @@ > InternalBaseLibIsNodeInList ( > > } while ((Ptr !=3D 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 Lis= t > > // > > - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > > + ASSERT (InternalBaseLibIsListValid (ListHead)); ASSERT > > + (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); > > > > 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 Lis= t > > // > > - ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > > + ASSERT (InternalBaseLibIsListValid (ListHead)); ASSERT > > + (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry)); > > > > 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)); > > > > 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 =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)); > > > > 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)); > > > > 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)); > > > > // > > // 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