public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Michael D Kinney" <michael.d.kinney@intel.com>
To: Matthew Carlson <matthewfcarlson@gmail.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "devel@edk2.groups.io" <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: Add unit tests (Host and Shell based)
Date: Wed, 15 Jul 2020 19:03:02 +0000	[thread overview]
Message-ID: <MN2PR11MB4461DC43BB52B8855477C162D27E0@MN2PR11MB4461.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CALSfKnmwyqG7sSODonA=d_j=p1n4wq1WdJDZzRPajZWBceqPQg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3328 bytes --]

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 <matthewfcarlson@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: 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<mailto: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

[-- Attachment #2: Type: text/html, Size: 45341 bytes --]

  reply	other threads:[~2020-07-15 19:03 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-14 18:23 [PATCH v3 0/3] Add Unit Tests for BaseCryptLib to CryptoPkg matthewfcarlson
2020-07-14 18:23 ` [PATCH v3 1/3] UnitTestFrameworkPkg : BaseTimerLibPosix: Adds a host-based timer Lib matthewfcarlson
2020-07-15  2:38   ` Michael D Kinney
2020-07-14 18:23 ` [PATCH v3 2/3] CryptoPkg: BaseCryptLib: Add unit tests (Host and Shell based) matthewfcarlson
2020-07-15  2:52   ` [edk2-devel] " Michael D Kinney
2020-07-15 18:16     ` matthewfcarlson
2020-07-15 19:03       ` Michael D Kinney [this message]
2020-07-15 23:31         ` Matthew Carlson
2020-07-14 18:23 ` [PATCH v3 3/3] AzurePipelines : Pr Gate: Turn on HBUT for CryptoPkg matthewfcarlson
2020-07-15  2:39   ` Michael D Kinney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=MN2PR11MB4461DC43BB52B8855477C162D27E0@MN2PR11MB4461.namprd11.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox