From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by mx.groups.io with SMTP id smtpd.web12.4482.1594855873741302307 for ; Wed, 15 Jul 2020 16:31:14 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=tNKb5DGx; spf=pass (domain: gmail.com, ip: 209.85.218.44, mailfrom: matthewfcarlson@gmail.com) Received: by mail-ej1-f44.google.com with SMTP id br7so4277485ejb.5 for ; Wed, 15 Jul 2020 16:31:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=DOxKyR/L6B2DJdLp6umInoOqVcaKQz3FZavlTLiyjWM=; b=tNKb5DGxi7YOIU2QqcROtu9CnIAhkUikfg/Nas/ecv2fent7QaoavKzvFyb+2sFaQv onka6/ZdlGZ/yILAPtwhYwVq1E1LLrgn2CcVKY9WSdVa1GWKreCPfseLmOAJGXmy8KdR tWokxhtdbVvXQ/oz0ncmTA+MKjLmxpqz6gkMRUnRjh3lEzDdCPgpwcCs9LP/CPW2bLZl pN+1IpwdCN6SUMiOV9UCOiM/eSKWdZDBc2yOk7QZN9veuF9H4o2s1CYeX6qoQvoqvwej CYV1SShmRyLE8nCQJpV4JiL1YTQQxGUh7tKvZCcQYSX5+mv8MGiBFc3JikV840uLIO+6 PaFw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=DOxKyR/L6B2DJdLp6umInoOqVcaKQz3FZavlTLiyjWM=; b=RZJwDhEgt/I0GL9Ckmtf1plEnqJYrWCBgHQT5bNyFEs7S2zqpL263NA/EODbmXfVbo UsV0ZMjhr3aroN0iuoww1/FPQ0mkreZ5v0D+pS0axl2OPk7iPObCTx4Jr4Sv/PBJyu6n +5BCPXS6FaqVm3ieAcMUR5EDahAybwjJ7CzHsoHrPTe1Y0ZFJSzHNISBogzKGlstbQ+2 jLayHAHmxdWEHI+4QZ5ycERRQXj1pfQ29LOoNFeAxDHUHl/6tm6ZLUrW7FfYBh6tvnrT G+CgVKrcI8a9WN3Qlp/JbvHSnEKa9osCZNEtJ+UCpi5IpOzg7hmcrGLqM7F7lSqcRsRN xPkQ== X-Gm-Message-State: AOAM531GnCZo4Zquzq8YJiGqroQYoHWH0lTtgRj1N+2YNrIeV6C59h+w G/FN+FuaB4PPBsY0JOuGhXnALEGQruxLYAzEqMA= X-Google-Smtp-Source: ABdhPJw9C8P7GX7+PCysF900lyXGtUou9+N4dElzipCaR6YE6otBgXL1Jctcg8tsMG+IlE37s7aHdeWN6Z1yhAGGH48= X-Received: by 2002:a17:906:7115:: with SMTP id x21mr1248138ejj.86.1594855872225; Wed, 15 Jul 2020 16:31:12 -0700 (PDT) MIME-Version: 1.0 References: <20200714182317.1605-1-matthewfcarlson@gmail.com> <20200714182317.1605-3-matthewfcarlson@gmail.com> In-Reply-To: From: "Matthew Carlson" Date: Wed, 15 Jul 2020 16:31:00 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH v3 2/3] CryptoPkg: BaseCryptLib: Add unit tests (Host and Shell based) To: "Kinney, Michael D" Cc: "devel@edk2.groups.io" , "Wang, Jian J" , "Lu, XiaoyuX" Content-Type: multipart/alternative; boundary="000000000000bb63b805aa835136" --000000000000bb63b805aa835136 Content-Type: text/plain; charset="UTF-8" You hit the nail on the head. I think it's a reasonable limitation to not test the CrtWrapper functions. Our goal is to test the crypto functions and I think the current tests do a decent job of that. The idea of adding some of these limitations to the inf and header files could be useful, I'll do a v5 and add it in. -Matthew Carlson On Wed, Jul 15, 2020 at 12:03 PM Kinney, Michael D < michael.d.kinney@intel.com> wrote: > Matt, > > > > For (4) and (5) I think I see the issue. Host based unit tests always > link against the standard C lib for the host OS env and > > those are in conflict with some if the wrappers that are provided to make > OpenSSL work in an EDK II FW build env. > > > > So the approach to unit testing BaseCryptLib is not to test the actually > lib instances used in FW, but to instead test the > > source code of the BaseCryptLib that is used by the FW specific instances. > There is a small chance of some differences > > that may not be caught, but we can get really good unit test coverage for > the source code that is identical in both host > > and FW instances. If this is a correct assessment, you may want to add > some of this information to the INF file header of > > and source file headers for the unit tests. > > > > Thanks, > > > > Mike > > > > *From:* Matthew Carlson > *Sent:* Wednesday, July 15, 2020 11:16 AM > *To:* Kinney, Michael D > *Cc:* devel@edk2.groups.io; Wang, Jian J ; Lu, > XiaoyuX > *Subject:* Re: [edk2-devel] [PATCH v3 2/3] CryptoPkg: BaseCryptLib: Add > unit tests (Host and Shell based) > > > > Hey Mike, > > > > 1. I'll move it for v4. > > 2. I'll remove that, thanks! > > 3. I'll fix that as well, good spot. > > 4. It's mostly the same except for the different CrtWrappers > > 5. We need a host-specific CrtWrapper since the regular one has a bunch of > conflicts with regular C98, but there are a few functions that since we're > building no std mode of OpenSSL we still need to provide. > > 6. That would be my guess- this is largely based on the edk2-staging HBFA > branch and I've had to reverse engineer some of the steps for > generating the test data. I fixed the instructions. > > > -Matthew Carlson > > > > > > On Tue, Jul 14, 2020 at 7:53 PM Kinney, Michael D < > michael.d.kinney@intel.com> wrote: > > Hi Matt, > > I think the BaseCryptLib unit tests may need some more work. > > 1) The DSC file for host based tests is in the wrong directory. > It should be in CryptoPkg/Test to match the MdePkg, MdeModulePkg > and UnitTestFrameworkPkg location. > > 2) CryptoPkg/CryptoPkg.dsc includes a reference to the host based > library UnitTestHostBaseCryptLib.inf. This should only be > listed in the host based DSC file. > > 3) CryptoPkg\Library\BaseCryptLib\UnitTestHostBaseCryptLib.inf > This file appears to only be for host based tests, but it > lists compatibility with DXE_DRIVER DXE_CORE UEFI_APPLICATION > UEFI_DRIVER > and does not list HOST_APPLICATION as expected. > > 4) Why do we need a new lib instance of the BaseCryptLib for > host based unit testing. I would think we would want to perform > unit tests on the BaseCryptLib instances that would actually be > used in FW components. Can we update the unit tests to test > the services provided in the Base, Pei, Runtime, and Smm > instances of the BaseCryptLib? > > 5) Why do we need a host based specific version of the CrtWrapper, > UnitTestHostCrtWrapper.c? > > 6) The file CryptoPkg/Test/UnitTest/Librray/BaseCryptLib/TestEKUCerts/ > ChainCreationInstructions.txt makes reference to a bintohex tool > and putting the results in a file called AllTestSignatures.h. > But I do not see this file in the patch. Are these instructions > out of date? > > Thanks, > > Mike > > --000000000000bb63b805aa835136 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
You hit the nail on the head. I think it&= #39;s a reasonable=C2=A0limitation to not test the CrtWrapper functions. Ou= r goal is to test the crypto functions and I think the current tests do a d= ecent job of that. The idea of adding some of these limitations to the inf = and header files could be useful, I'll do a v5 and add it in.

-Matthew Carlson


On Wed, Jul 15, 2020 at 12:03 PM Kinney, Michael D <<= a href=3D"mailto:michael.d.kinney@intel.com">michael.d.kinney@intel.com= > wrote:

Matt,

=C2=A0

For (4) and (5) I think I see the issue.= =C2=A0 Host based unit tests always link against the standard C lib for the= host OS env and

those are in conflict with some if the wrapper= s that are provided to make OpenSSL work in an EDK II FW build env.<= u>

=C2=A0

So the approach to unit testing BaseCryptLib is n= ot to test the actually lib instances used in FW, but to instead test the

source code of the BaseCryptLib that= is used by the FW specific instances.=C2=A0 There is a small chance of some differences

that may not be caught, but we can get really = good unit test coverage for the source code that is identical in both host<= u>

and FW instances.=C2=A0 If this is a correct assessment, you may want to add some of this in= formation to the INF file header of

and source file headers for the unit tests.=

=C2=A0

Thanks,

=C2=A0

Mike

=C2=A0

From: Matthew Carlson <= matthewfcarl= son@gmail.com>
Sent: Wednesday, July 15, 2020 11:16 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@= edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com= >
Subject: Re: [edk2-devel] [PATCH v3 2/3] CryptoPkg: BaseCryptLib: Ad= d unit tests (Host and Shell based)

=C2=A0

Hey Mike,

=C2=A0

1. I'll move it for v4.=C2=A0

2. I'll remove that, thanks!

3. I'll fix that as well, good spot.=C2=A0

4. It's mostly the same except for the different= CrtWrappers

5. We need a host-specific CrtWrapper since the regu= lar one has a bunch of conflicts with regular C98, but there are a few func= tions that since we're building no std mode of OpenSSL we still need to= provide.

6. That would be my guess- this is largely based on = the edk2-staging HBFA branch and I've had to reverse engineer some of t= he steps for generating=C2=A0the test data. I fixed the instructions.


-Matthew Carlson

=C2=A0

=C2=A0

On Tue, Jul 14, 2020 at 7:53 PM Kinney, Michael D &l= t;michael.d= .kinney@intel.com> wrote:

Hi Matt,

I think the BaseCryptLib unit tests may need some more work.

1) The DSC file for host based tests is in the wrong directory.
=C2=A0 =C2=A0It should be in CryptoPkg/Test to match the MdePkg, MdeModuleP= kg
=C2=A0 =C2=A0and UnitTestFrameworkPkg location.

2) CryptoPkg/CryptoPkg.dsc includes a reference to the host based
=C2=A0 =C2=A0library UnitTestHostBaseCryptLib.inf.=C2=A0 This should only b= e
=C2=A0 =C2=A0listed in the host based DSC file.

3) CryptoPkg\Library\BaseCryptLib\UnitTestHostBaseCryptLib.inf
=C2=A0 =C2=A0This file appears to only be for host based tests, but it
=C2=A0 =C2=A0lists compatibility with DXE_DRIVER DXE_CORE UEFI_APPLICATION = UEFI_DRIVER
=C2=A0 =C2=A0and does not list HOST_APPLICATION as expected.

4) Why do we need a new lib instance of the BaseCryptLib for
=C2=A0 =C2=A0host based unit testing.=C2=A0 I would think we would want to = perform
=C2=A0 =C2=A0unit tests on the BaseCryptLib instances that would actually b= e
=C2=A0 =C2=A0used in FW components.=C2=A0 Can we update the unit tests to t= est
=C2=A0 =C2=A0the services provided in the Base, Pei, Runtime, and Smm
=C2=A0 =C2=A0instances of the BaseCryptLib?

5) Why do we need a host based specific version of the CrtWrapper,
=C2=A0 =C2=A0UnitTestHostCrtWrapper.c?

6) The file CryptoPkg/Test/UnitTest/Librray/BaseCryptLib/TestEKUCerts/
=C2=A0 =C2=A0ChainCreationInstructions.txt makes reference to a bintohex to= ol
=C2=A0 =C2=A0and putting the results in a file called AllTestSignatures.h.<= br> =C2=A0 =C2=A0But I do not see this file in the patch.=C2=A0 Are these instr= uctions
=C2=A0 =C2=A0out of date?

Thanks,

Mike

--000000000000bb63b805aa835136--