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

["Kun Qin" <kuqin12@gmail.com>]

Hi Mike,

It has been a week since the last movement on this. Could you please
provide feedback on this patch when you have a chance? Any input is
appreciated.

Regards,
Kun

On 6/30/2023 4:00 PM, Michael Kubacki wrote:
> 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/UnitTestPersistenceLibSimpleFileSystem.c 
>> | 232 +++++++++++++-------
>>   1 file changed, 157 insertions(+), 75 deletions(-)
>>
>> diff --git 
>> a/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c 
>> b/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c 
>>
>> index b59991683f48..8f7f03646cd5 100644
>> --- 
>> a/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.c
>> +++ 
>> b/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTestPersistenceLibSimpleFileSystem.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;
>>
>> +}
>>