From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f177.google.com (mail-pf1-f177.google.com [209.85.210.177]) by mx.groups.io with SMTP id smtpd.web11.50.1688744713049726664 for ; Fri, 07 Jul 2023 08:45:13 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@gmail.com header.s=20221208 header.b=QfWlu3kC; spf=pass (domain: gmail.com, ip: 209.85.210.177, mailfrom: kuqin12@gmail.com) Received: by mail-pf1-f177.google.com with SMTP id d2e1a72fcca58-666683eb028so1245234b3a.0 for ; Fri, 07 Jul 2023 08:45:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1688744712; x=1691336712; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=Akz/uB1MvFYZutPIUCYj9XYiwS6DxVwnmnrsDnPnzhY=; b=QfWlu3kC6SbgekpqUQBhkcChpsRVeRfDNzLTIT32sFQOfI+sAILoysdWG3dGFECa5r MxZ/e/l4PGWf4kLYKVtzBnV1xyFDaLyrx+QUUJUcb75opeXzcc8cERezomCAA+XHzwcB FVAa8XiGILtaIStsha1Ao0E6lpO66b80WeFAhBhq/hALPUIYcaOgc7QIWJ6c6OnLGKPi YAHvJJud5qr69caManTMPND2HuQgQTtLJJwBpid/PWEqoygCU7bZV2CSv6z9TSXJEJOz CL3lglb5bhme61RXimC32BM/qu2pufZQnOCnPQtv2/zbHYIGK1q0e3cAO2exmhyTH36O GvCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688744712; x=1691336712; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Akz/uB1MvFYZutPIUCYj9XYiwS6DxVwnmnrsDnPnzhY=; b=LKXN3u96pKwwBEcwY61omOW6q70AVQ6Q7WEIf97axxBIRQo61yVbwIDTJY/RlSjClF /YHfwTb1GYoLQYakPjfKBWEDS7v6sAxCpAdByhIejsIGAlPwDDk7htts7ggpKKdx52dr myWcBOhEbRouj9jfFCKwSdfYOPkB16lXN9M1SRo8/3hzLN7Fujn0oKh3tzM7hKbnDIWX MX1E/xezJ2J9aFb2YzSw/CUvDDLibmo2+EcnJ/R4PgRbDGOCUBCt+tnGWc/tUqQxGNjr rdK8olpY6B9UP/Kvzghs2mt2a4/XXUuFFOf5AKm6+h6YymxF3f8/kWoZ3621nfqFS95X 8BTg== X-Gm-Message-State: ABy/qLaYg2S+OjJCWoJDG3MBZ5MjZKH5eSApZrobXtDv5mukykfrgV3o pXq6OTubXhV44d0XNQnb2cU= X-Google-Smtp-Source: APBJJlH5Nx6nkSKImbd+xLtjh6zxNhyxcJU2n2f8AG12HDR6pL0PeIKUEH7TZluanSnI9pan84kSkQ== X-Received: by 2002:a05:6a00:39a1:b0:67e:bf65:ae61 with SMTP id fi33-20020a056a0039a100b0067ebf65ae61mr5017502pfb.28.1688744712344; Fri, 07 Jul 2023 08:45:12 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:3458:2b91:2cc3:27da? ([2001:4898:80e8:2:b478:2b91:2cc3:27da]) by smtp.gmail.com with ESMTPSA id e6-20020aa78246000000b00682bbb65852sm3136272pfn.176.2023.07.07.08.45.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 07 Jul 2023 08:45:11 -0700 (PDT) Message-ID: <13d47b57-29dd-007c-a9f2-e5bde74480db@gmail.com> Date: Fri, 7 Jul 2023 08:45:09 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [edk2-devel] [PATCH v3 1/1] UnitTestFrameworkPkg: UnitTestPersistenceLib: Save Unit Test Cache Option To: Michael Kubacki , devel@edk2.groups.io, Michael D Kinney Cc: Sean Brogan References: <20230630181430.2128-1-kuqin12@gmail.com> <20230630181430.2128-2-kuqin12@gmail.com> <06c10e6c-bca1-4266-1c77-e73127077c97@linux.microsoft.com> From: "Kun Qin" In-Reply-To: <06c10e6c-bca1-4266-1c77-e73127077c97@linux.microsoft.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > > 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 >> Cc: Michael Kubacki >> Cc: Michael D Kinney >> >> Signed-off-by: Kun Qin >> --- >> >> 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 >> >>   #include >> >>   #include >> >> +#include >> >>   #include >> >>   #include >> >> >>   #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 > cache files, i.e. FS0:TestFolder>\n"); >> >> +        } >> >> +      } >> >> +    } >> >> +  } >> >> + >> >> +Done: >> >> +  return Status; >> >> +} >>