Re: [edk2-devel] [PATCH v3 1/1] UnitTestFrameworkPkg: UnitTestPersistenceLib: Save Unit Test Cache Option

["Michael D Kinney" <michael.d.kinney@intel.com>]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>


> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Friday, June 30, 2023 4:00 PM
> To: devel@edk2.groups.io; kuqin12@gmail.com
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 1/1] UnitTestFrameworkPkg:
> UnitTestPersistenceLib: Save Unit Test Cache Option
> 
> Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> I haven't seen Mike Kinney acknowledge the changes made in response to
> his v1 request so we'll need to get that before merging the patch.
> 
> Thanks,
> Michael
> 
> On 6/30/2023 2:14 PM, Kun Qin wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4467
> >
> > Current implementation of UnitTestFrameworkPkg for shell-based unit
> test
> > will save the unit test cache to the same volume as the test
> application
> > itself. This works as long as the test application is on a writable
> > volume, such as USB or EFI partition.
> >
> > Instead of saving the files to the same file system of unit test
> > application, this change will save the cache file to the path where
> the
> > user ran this test application.
> >
> > This change then added an input argument to allow user to specify
> where
> > to save such cache file through `--CachePath` shell argument to allow
> > even more flexibility.
> >
> > This change was tested on proprietary physical hardware platforms and
> > QEMU based virtual platform.
> >
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >
> > Signed-off-by: Kun Qin <kuqin12@gmail.com>
> > ---
> >
> > Notes:
> >      v2:
> >      - Adding input argument [Mike Kinney]
> >      v3:
> >      - No review, no change
> >
> >
> UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/Unit
> TestPersistenceLibSimpleFileSystem.c | 232 +++++++++++++-------
> >   1 file changed, 157 insertions(+), 75 deletions(-)
> >
> > diff --git
> a/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/Un
> itTestPersistenceLibSimpleFileSystem.c
> b/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/Un
> itTestPersistenceLibSimpleFileSystem.c
> > index b59991683f48..8f7f03646cd5 100644
> > ---
> a/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/Un
> itTestPersistenceLibSimpleFileSystem.c
> > +++
> b/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/Un
> itTestPersistenceLibSimpleFileSystem.c
> > @@ -16,13 +16,16 @@
> >   #include <Library/UefiBootServicesTableLib.h>
> >
> >   #include <Library/DevicePathLib.h>
> >
> >   #include <Library/ShellLib.h>
> >
> > +#include <Library/UefiLib.h>
> >
> >   #include <Protocol/LoadedImage.h>
> >
> >   #include <UnitTestFrameworkTypes.h>
> >
> >
> >
> >   #define CACHE_FILE_SUFFIX  L"_Cache.dat"
> >
> >
> >
> > +CHAR16  *mCachePath = NULL;
> >
> > +
> >
> >   /**
> >
> > -  Generate the device path to the cache file.
> >
> > +  Generate the file name and path to the cache file.
> >
> >
> >
> >     @param[in]  FrameworkHandle  A pointer to the framework that is
> being persisted.
> >
> >
> >
> > @@ -31,8 +34,8 @@
> >
> >
> >   **/
> >
> >   STATIC
> >
> > -EFI_DEVICE_PATH_PROTOCOL *
> >
> > -GetCacheFileDevicePath (
> >
> > +CHAR16 *
> >
> > +GetCacheFileName (
> >
> >     IN UNIT_TEST_FRAMEWORK_HANDLE  FrameworkHandle
> >
> >     )
> >
> >   {
> >
> > @@ -44,13 +47,11 @@ GetCacheFileDevicePath (
> >     CHAR16                     *TestName;
> >
> >     UINTN                      DirectorySlashOffset;
> >
> >     UINTN                      CacheFilePathLength;
> >
> > -  EFI_DEVICE_PATH_PROTOCOL   *CacheFileDevicePath;
> >
> >
> >
> > -  Framework           = (UNIT_TEST_FRAMEWORK *)FrameworkHandle;
> >
> > -  AppPath             = NULL;
> >
> > -  CacheFilePath       = NULL;
> >
> > -  TestName            = NULL;
> >
> > -  CacheFileDevicePath = NULL;
> >
> > +  Framework     = (UNIT_TEST_FRAMEWORK *)FrameworkHandle;
> >
> > +  AppPath       = NULL;
> >
> > +  CacheFilePath = NULL;
> >
> > +  TestName      = NULL;
> >
> >
> >
> >     //
> >
> >     // First, we need to get some information from the loaded image.
> >
> > @@ -85,35 +86,62 @@ GetCacheFileDevicePath (
> >     // PathCleanUpDirectories (FileNameCopy);
> >
> >     //     if (PathRemoveLastItem (FileNameCopy)) {
> >
> >     //
> >
> > -  AppPath              = ConvertDevicePathToText (LoadedImage-
> >FilePath, TRUE, TRUE); // NOTE: This must be freed.
> >
> > -  DirectorySlashOffset = StrLen (AppPath);
> >
> > -  //
> >
> > -  // Make sure we didn't get any weird data.
> >
> > -  //
> >
> > -  if (DirectorySlashOffset == 0) {
> >
> > -    DEBUG ((DEBUG_ERROR, "%a - Weird 0-length string when processing
> app path.\n", __func__));
> >
> > -    goto Exit;
> >
> > -  }
> >
> > +  if (mCachePath == NULL) {
> >
> > +    AppPath = ConvertDevicePathToText (LoadedImage->FilePath, TRUE,
> TRUE); // NOTE: This must be freed.
> >
> > +    if (AppPath == NULL) {
> >
> > +      goto Exit;
> >
> > +    }
> >
> >
> >
> > -  //
> >
> > -  // Now that we know we have a decent string, let's take a deeper
> look.
> >
> > -  //
> >
> > -  do {
> >
> > -    if (AppPath[DirectorySlashOffset] == L'\\') {
> >
> > -      break;
> >
> > +    DirectorySlashOffset = StrLen (AppPath);
> >
> > +    //
> >
> > +    // Make sure we didn't get any weird data.
> >
> > +    //
> >
> > +    if (DirectorySlashOffset == 0) {
> >
> > +      DEBUG ((DEBUG_ERROR, "%a - Weird 0-length string when
> processing app path.\n", __func__));
> >
> > +      goto Exit;
> >
> >       }
> >
> >
> >
> > -    DirectorySlashOffset--;
> >
> > -  } while (DirectorySlashOffset > 0);
> >
> > +    //
> >
> > +    // Now that we know we have a decent string, let's take a deeper
> look.
> >
> > +    //
> >
> > +    do {
> >
> > +      if (AppPath[DirectorySlashOffset] == L'\\') {
> >
> > +        break;
> >
> > +      }
> >
> > +
> >
> > +      DirectorySlashOffset--;
> >
> > +    } while (DirectorySlashOffset > 0);
> >
> >
> >
> > -  //
> >
> > -  // After that little maneuver, DirectorySlashOffset should be
> pointing at the last '\' in AppString.
> >
> > -  // That would be the path to the parent directory that the test app
> is executing from.
> >
> > -  // Let's check and make sure that's right.
> >
> > -  //
> >
> > -  if (AppPath[DirectorySlashOffset] != L'\\') {
> >
> > -    DEBUG ((DEBUG_ERROR, "%a - Could not find a single directory
> separator in app path.\n", __func__));
> >
> > -    goto Exit;
> >
> > +    //
> >
> > +    // After that little maneuver, DirectorySlashOffset should be
> pointing at the last '\' in AppString.
> >
> > +    // That would be the path to the parent directory that the test
> app is executing from.
> >
> > +    // Let's check and make sure that's right.
> >
> > +    //
> >
> > +    if (AppPath[DirectorySlashOffset] != L'\\') {
> >
> > +      DEBUG ((DEBUG_ERROR, "%a - Could not find a single directory
> separator in app path.\n", __func__));
> >
> > +      goto Exit;
> >
> > +    }
> >
> > +  } else {
> >
> > +    AppPath = FullyQualifyPath (mCachePath); // NOTE: This must be
> freed.
> >
> > +    if (AppPath == NULL) {
> >
> > +      goto Exit;
> >
> > +    }
> >
> > +
> >
> > +    DirectorySlashOffset = StrLen (AppPath);
> >
> > +
> >
> > +    if (AppPath[DirectorySlashOffset - 1] != L'\\') {
> >
> > +      // Set the slash if user did not specify it on the newly
> allocated pool
> >
> > +      AppPath = ReallocatePool (
> >
> > +                  (DirectorySlashOffset + 1) * sizeof (CHAR16),
> >
> > +                  (DirectorySlashOffset + 2) * sizeof (CHAR16),
> >
> > +                  AppPath
> >
> > +                  );
> >
> > +      AppPath[DirectorySlashOffset]     = L'\\';
> >
> > +      AppPath[DirectorySlashOffset + 1] = L'\0';
> >
> > +    } else {
> >
> > +      // Otherwise the user input is good enough to go, mostly
> >
> > +      DirectorySlashOffset--;
> >
> > +    }
> >
> >     }
> >
> >
> >
> >     //
> >
> > @@ -135,11 +163,6 @@ GetCacheFileDevicePath (
> >     StrCatS (CacheFilePath, CacheFilePathLength, TestName);
> // Copy the base name for the test cache.
> >
> >     StrCatS (CacheFilePath, CacheFilePathLength, CACHE_FILE_SUFFIX);
> // Copy the file suffix.
> >
> >
> >
> > -  //
> >
> > -  // Finally, try to create the device path for the thing thing.
> >
> > -  //
> >
> > -  CacheFileDevicePath = FileDevicePath (LoadedImage->DeviceHandle,
> CacheFilePath);
> >
> > -
> >
> >   Exit:
> >
> >     //
> >
> >     // Free allocated buffers.
> >
> > @@ -148,15 +171,11 @@ Exit:
> >       FreePool (AppPath);
> >
> >     }
> >
> >
> >
> > -  if (CacheFilePath != NULL) {
> >
> > -    FreePool (CacheFilePath);
> >
> > -  }
> >
> > -
> >
> >     if (TestName != NULL) {
> >
> >       FreePool (TestName);
> >
> >     }
> >
> >
> >
> > -  return CacheFileDevicePath;
> >
> > +  return CacheFilePath;
> >
> >   }
> >
> >
> >
> >   /**
> >
> > @@ -175,21 +194,24 @@ DoesCacheExist (
> >     IN UNIT_TEST_FRAMEWORK_HANDLE  FrameworkHandle
> >
> >     )
> >
> >   {
> >
> > -  EFI_DEVICE_PATH_PROTOCOL  *FileDevicePath;
> >
> > -  EFI_STATUS                Status;
> >
> > -  SHELL_FILE_HANDLE         FileHandle;
> >
> > +  CHAR16             *FileName;
> >
> > +  EFI_STATUS         Status;
> >
> > +  SHELL_FILE_HANDLE  FileHandle;
> >
> >
> >
> >     //
> >
> >     // NOTE: This devpath is allocated and must be freed.
> >
> >     //
> >
> > -  FileDevicePath = GetCacheFileDevicePath (FrameworkHandle);
> >
> > +  FileName = GetCacheFileName (FrameworkHandle);
> >
> > +  if (FileName == NULL) {
> >
> > +    return FALSE;
> >
> > +  }
> >
> >
> >
> >     //
> >
> >     // Check to see whether the file exists.  If the file can be
> opened for
> >
> >     // reading, it exists.  Otherwise, probably not.
> >
> >     //
> >
> > -  Status = ShellOpenFileByDevicePath (
> >
> > -             &FileDevicePath,
> >
> > +  Status = ShellOpenFileByName (
> >
> > +             FileName,
> >
> >                &FileHandle,
> >
> >                EFI_FILE_MODE_READ,
> >
> >                0
> >
> > @@ -198,8 +220,8 @@ DoesCacheExist (
> >       ShellCloseFile (&FileHandle);
> >
> >     }
> >
> >
> >
> > -  if (FileDevicePath != NULL) {
> >
> > -    FreePool (FileDevicePath);
> >
> > +  if (FileName != NULL) {
> >
> > +    FreePool (FileName);
> >
> >     }
> >
> >
> >
> >     DEBUG ((DEBUG_VERBOSE, "%a - Returning %d\n", __func__, !EFI_ERROR
> (Status)));
> >
> > @@ -229,10 +251,10 @@ SaveUnitTestCache (
> >     IN UINTN                       SaveStateSize
> >
> >     )
> >
> >   {
> >
> > -  EFI_DEVICE_PATH_PROTOCOL  *FileDevicePath;
> >
> > -  EFI_STATUS                Status;
> >
> > -  SHELL_FILE_HANDLE         FileHandle;
> >
> > -  UINTN                     WriteCount;
> >
> > +  CHAR16             *FileName;
> >
> > +  EFI_STATUS         Status;
> >
> > +  SHELL_FILE_HANDLE  FileHandle;
> >
> > +  UINTN              WriteCount;
> >
> >
> >
> >     //
> >
> >     // Check the inputs for sanity.
> >
> > @@ -245,13 +267,16 @@ SaveUnitTestCache (
> >     // Determine the path for the cache file.
> >
> >     // NOTE: This devpath is allocated and must be freed.
> >
> >     //
> >
> > -  FileDevicePath = GetCacheFileDevicePath (FrameworkHandle);
> >
> > +  FileName = GetCacheFileName (FrameworkHandle);
> >
> > +  if (FileName == NULL) {
> >
> > +    return EFI_INVALID_PARAMETER;
> >
> > +  }
> >
> >
> >
> >     //
> >
> >     // First lets open the file if it exists so we can delete
> it...This is the work around for truncation
> >
> >     //
> >
> > -  Status = ShellOpenFileByDevicePath (
> >
> > -             &FileDevicePath,
> >
> > +  Status = ShellOpenFileByName (
> >
> > +             FileName,
> >
> >                &FileHandle,
> >
> >                (EFI_FILE_MODE_READ | EFI_FILE_MODE_WRITE),
> >
> >                0
> >
> > @@ -270,8 +295,8 @@ SaveUnitTestCache (
> >     //
> >
> >     // Now that we know the path to the file... let's open it for
> writing.
> >
> >     //
> >
> > -  Status = ShellOpenFileByDevicePath (
> >
> > -             &FileDevicePath,
> >
> > +  Status = ShellOpenFileByName (
> >
> > +             FileName,
> >
> >                &FileHandle,
> >
> >                (EFI_FILE_MODE_READ | EFI_FILE_MODE_WRITE |
> EFI_FILE_MODE_CREATE),
> >
> >                0
> >
> > @@ -304,8 +329,8 @@ SaveUnitTestCache (
> >     ShellCloseFile (&FileHandle);
> >
> >
> >
> >   Exit:
> >
> > -  if (FileDevicePath != NULL) {
> >
> > -    FreePool (FileDevicePath);
> >
> > +  if (FileName != NULL) {
> >
> > +    FreePool (FileName);
> >
> >     }
> >
> >
> >
> >     return Status;
> >
> > @@ -334,13 +359,13 @@ LoadUnitTestCache (
> >     OUT UINTN                       *SaveStateSize
> >
> >     )
> >
> >   {
> >
> > -  EFI_STATUS                Status;
> >
> > -  EFI_DEVICE_PATH_PROTOCOL  *FileDevicePath;
> >
> > -  SHELL_FILE_HANDLE         FileHandle;
> >
> > -  BOOLEAN                   IsFileOpened;
> >
> > -  UINT64                    LargeFileSize;
> >
> > -  UINTN                     FileSize;
> >
> > -  VOID                      *Buffer;
> >
> > +  EFI_STATUS         Status;
> >
> > +  CHAR16             *FileName;
> >
> > +  SHELL_FILE_HANDLE  FileHandle;
> >
> > +  BOOLEAN            IsFileOpened;
> >
> > +  UINT64             LargeFileSize;
> >
> > +  UINTN              FileSize;
> >
> > +  VOID               *Buffer;
> >
> >
> >
> >     IsFileOpened = FALSE;
> >
> >     Buffer       = NULL;
> >
> > @@ -356,13 +381,16 @@ LoadUnitTestCache (
> >     // Determine the path for the cache file.
> >
> >     // NOTE: This devpath is allocated and must be freed.
> >
> >     //
> >
> > -  FileDevicePath = GetCacheFileDevicePath (FrameworkHandle);
> >
> > +  FileName = GetCacheFileName (FrameworkHandle);
> >
> > +  if (FileName == NULL) {
> >
> > +    return EFI_INVALID_PARAMETER;
> >
> > +  }
> >
> >
> >
> >     //
> >
> >     // Now that we know the path to the file... let's open it for
> writing.
> >
> >     //
> >
> > -  Status = ShellOpenFileByDevicePath (
> >
> > -             &FileDevicePath,
> >
> > +  Status = ShellOpenFileByName (
> >
> > +             FileName,
> >
> >                &FileHandle,
> >
> >                EFI_FILE_MODE_READ,
> >
> >                0
> >
> > @@ -407,8 +435,8 @@ Exit:
> >     //
> >
> >     // Free allocated buffers
> >
> >     //
> >
> > -  if (FileDevicePath != NULL) {
> >
> > -    FreePool (FileDevicePath);
> >
> > +  if (FileName != NULL) {
> >
> > +    FreePool (FileName);
> >
> >     }
> >
> >
> >
> >     if (IsFileOpened) {
> >
> > @@ -426,3 +454,57 @@ Exit:
> >     *SaveData = Buffer;
> >
> >     return Status;
> >
> >   }
> >
> > +
> >
> > +/**
> >
> > +  Shell based UnitTestPersistenceLib library constructor.
> >
> > +
> >
> > +  @param[in]  ImageHandle  The firmware allocated handle for the EFI
> image.
> >
> > +  @param[in]  SystemTable  A pointer to the EFI System Table.
> >
> > +
> >
> > +  @retval EFI_SUCCESS      The constructor finished successfully.
> >
> > +  @retval Others           Error codes returned from gBS-
> >HandleProtocol.
> >
> > + **/
> >
> > +EFI_STATUS
> >
> > +EFIAPI
> >
> > +UnitTestPersistenceLibConstructor (
> >
> > +  IN EFI_HANDLE        ImageHandle,
> >
> > +  IN EFI_SYSTEM_TABLE  *SystemTable
> >
> > +  )
> >
> > +{
> >
> > +  UINTN                          Index;
> >
> > +  UINTN                          Argc;
> >
> > +  CHAR16                         **Argv;
> >
> > +  EFI_STATUS                     Status;
> >
> > +  EFI_SHELL_PARAMETERS_PROTOCOL  *ShellParameters;
> >
> > +
> >
> > +  Status = gBS->HandleProtocol (
> >
> > +                  gImageHandle,
> >
> > +                  &gEfiShellParametersProtocolGuid,
> >
> > +                  (VOID **)&ShellParameters
> >
> > +                  );
> >
> > +  if (EFI_ERROR (Status)) {
> >
> > +    ASSERT_EFI_ERROR (Status);
> >
> > +    goto Done;
> >
> > +  }
> >
> > +
> >
> > +  Argc = ShellParameters->Argc;
> >
> > +  Argv = ShellParameters->Argv;
> >
> > +
> >
> > +  Status = EFI_SUCCESS;
> >
> > +  if ((Argc > 1) && (Argv != NULL)) {
> >
> > +    // This might be our cue, check for whether we need to do
> anything
> >
> > +    for (Index = 1; Index < Argc; Index++) {
> >
> > +      if (StrCmp (Argv[Index], L"--CachePath") == 0) {
> >
> > +        // Need to update the potential cache path to designated path
> >
> > +        if (Index < Argc - 1) {
> >
> > +          mCachePath = Argv[Index + 1];
> >
> > +        } else {
> >
> > +          Print (L"  --CachePath <Path of where to save unit test
> cache files, i.e. FS0:TestFolder>\n");
> >
> > +        }
> >
> > +      }
> >
> > +    }
> >
> > +  }
> >
> > +
> >
> > +Done:
> >
> > +  return Status;
> >
> > +}
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107077): https://edk2.groups.io/g/devel/message/107077
Mute This Topic: https://groups.io/mt/99878682/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-