Reviewed-by: Michael D Kinney <michael.d.kin...@intel.com>
> -----Original Message----- > From: Michael Kubacki <mikub...@linux.microsoft.com> > Sent: Friday, June 30, 2023 4:00 PM > To: devel@edk2.groups.io; kuqi...@gmail.com > Cc: Sean Brogan <sean.bro...@microsoft.com>; Kinney, Michael D > <michael.d.kin...@intel.com> > Subject: Re: [edk2-devel] [PATCH v3 1/1] UnitTestFrameworkPkg: > UnitTestPersistenceLib: Save Unit Test Cache Option > > Reviewed-by: Michael Kubacki <michael.kuba...@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.bro...@microsoft.com> > > Cc: Michael Kubacki <mikub...@linux.microsoft.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > > > Signed-off-by: Kun Qin <kuqi...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
