From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f53.google.com (mail-pj1-f53.google.com [209.85.216.53]) by mx.groups.io with SMTP id smtpd.web11.3636.1686100453224455694 for ; Tue, 06 Jun 2023 18:14:13 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@gmail.com header.s=20221208 header.b=sDE7uh6D; spf=pass (domain: gmail.com, ip: 209.85.216.53, mailfrom: kuqin12@gmail.com) Received: by mail-pj1-f53.google.com with SMTP id 98e67ed59e1d1-2563a4b6285so3244122a91.2 for ; Tue, 06 Jun 2023 18:14:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686100452; x=1688692452; 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=rOEG4F9qJ3AcE1GZeBn1xurOqByAd9t+eHuejnZJbAo=; b=sDE7uh6D1ENqmEcl0dtxoCdc1nAx6ZYC9l1U/2VxgSIUBpA06QiSHDJ7IKMuNM9Ws6 IfvpsjDv7JTGMsbfQz+1m3GK4aAefa118GeGiis2OP6nq5oqwinUNdIyf6lE/FaovSzn awoL7mG7opEA+MQnvJjoDzITpkRdeNb7Yb+pO1LlosIDeoO6zxZlB389F3Lk0pHx2hZI OnZkPHVsn1BTfNyXU8/mmexC2kZ0L0ORwkKSEAUdNZjAFARMHL0DXa1PtuH46qnWr/77 xJUzZRX9n2CVIbIeVI5zxLGvvV5RFrKNQi2Ftl/Ju34ER0cpvqPncVnROAJWW4XZ4m5I 6qfA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686100452; x=1688692452; 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=rOEG4F9qJ3AcE1GZeBn1xurOqByAd9t+eHuejnZJbAo=; b=OnoPnf/n6S4lX462KRMjV+v7OPX1NjEJZs7Gh8zdQIqFJUXNTDyjr5T0GY5m7ad4qA fTx+5ajNcx9D7q9+KxMaa2mynCx/SCYJqGbITKchjxRHG0THvd+hrhhBVY3hxGGTwmhf AJOVKbF48/g/TXImG17RIqgSW+REkDZxpONZxmXf2v3fvnuxYzgfKhlpYP+C8p7WMFqq wWQZvZbUcsp8FoNPfxq7JZTrFNp5ZS/7Jq80oXBRqnS+VrZKDD6TZEJPO9EUYz3SNGqA XGzdo+c5eNk9BtXod0T4MS7QvJ4big6bCelT931dbhdnBjePBe2iTyZlZ+kk8VlW0Dw5 t8Yw== X-Gm-Message-State: AC+VfDz5dno8pXeAIuSgKwQnbcj/3i0+hS8dud3Jrc7UBEsp1jWOGy48 S6sGwr/8d1V3zpUHo0VBRXyMdPhMqSg= X-Google-Smtp-Source: ACHHUZ79aE2narjY+90xG5ofZXDRqPP94jz28pyeBe34RofOPAV5TCQEY3WppJeOVfYKnksVp86pIw== X-Received: by 2002:a17:90b:4003:b0:258:8731:4e39 with SMTP id ie3-20020a17090b400300b0025887314e39mr2263101pjb.40.1686100452239; Tue, 06 Jun 2023 18:14:12 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:e490:d74e:193b:f53c? ([2001:4898:80e8:3:64b0:d74e:193b:f53c]) by smtp.gmail.com with ESMTPSA id v7-20020a17090a898700b00256dff5f8e3sm138375pjn.49.2023.06.06.18.14.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 06 Jun 2023 18:14:11 -0700 (PDT) Message-ID: Date: Tue, 6 Jun 2023 18:14:11 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [edk2-devel] [PATCH v1 1/1] UnitTestFrameworkPkg: UnitTestPersistenceLib: Save Unit Test Cache to Drive To: devel@edk2.groups.io, michael.d.kinney@intel.com Cc: Sean Brogan , Michael Kubacki References: <20230531191358.587-1-kuqin12@gmail.com> <20230531191358.587-2-kuqin12@gmail.com> <6093b465-6d20-1ab7-6b84-fba55c3fb0df@gmail.com> From: "Kun Qin" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi Mike, I reworked the patch and sent a v2 here: https://edk2.groups.io/g/devel/message/105827 Note that I did not end up putting the shell based argument checks in UnitTestLib.inf because that library has multi-phase support. Adding Shell argument checks will break it unless we add more library instances to maintain the compatibility. I think adding argument support here would be more self-contained and we do not need to pass the path around through changing framework handle structure. Please let me know if you have any feedback. Thanks, Kun On 6/1/2023 9:48 AM, Michael D Kinney wrote: > I think your use case is limited to the UnitTestLib based tests running from the UEFI Shell. > > In that case, the library UnitTestFrameworkPkg\Library\UnitTestLib\UnitTestLib.inf is always > linked to the unit test application. You could add a CONSTRUCTOR to UnitTestLib.inf and the > CONSTRUCTOR function can look for UEFI Shell arguments and if args specify alternate path > for unit test cache then save some state for that path. > > Mike > > >> -----Original Message----- >> From: Kun Qin >> Sent: Thursday, June 1, 2023 8:50 AM >> To: Kinney, Michael D ; devel@edk2.groups.io >> Cc: Sean Brogan ; Michael Kubacki >> >> Subject: Re: [PATCH v1 1/1] UnitTestFrameworkPkg: UnitTestPersistenceLib: >> Save Unit Test Cache to Drive >> >> Hi Mike, >> >> Yes, that would indeed serve the purpose. Could you please show me a >> pointer on how to add that >> commonly? I would be open to change the code the other way. >> >> The reason I would like to change this commonly is because if the input >> argument has to be added >> per test, that will prevent the existing tests to be used in the >> aforementioned scenarios. Please let >> me how you think. >> >> Thanks in advance. >> >> Regards, >> Kun >> >> On 5/31/2023 1:39 PM, Kinney, Michael D wrote: >>> Would it be more flexible for unit tests apps to support an optional >> command line arg >>> to specify the volume to write results? >>> >>> Mike >>> >>>> -----Original Message----- >>>> From: Kun Qin >>>> Sent: Wednesday, May 31, 2023 12:14 PM >>>> To: devel@edk2.groups.io >>>> Cc: Sean Brogan ; Michael Kubacki >>>> ; Kinney, Michael D >>>> >>>> Subject: [PATCH v1 1/1] UnitTestFrameworkPkg: UnitTestPersistenceLib: >> Save >>>> Unit Test Cache to Drive >>>> >>>> From: kuqin12 <42554914+kuqin12@users.noreply.github.com> >>>> >>>> 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 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 >>>> --- >>>> >>>> >> UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitTes >>>> tPersistenceLibSimpleFileSystem.c | 81 +++++++++++--------- >>>> 1 file changed, 43 insertions(+), 38 deletions(-) >>>> >>>> diff --git >>>> >> a/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitT >>>> estPersistenceLibSimpleFileSystem.c >>>> >> b/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitT >>>> estPersistenceLibSimpleFileSystem.c >>>> index b59991683f48..d4181808b2be 100644 >>>> --- >>>> >> a/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitT >>>> estPersistenceLibSimpleFileSystem.c >>>> +++ >>>> >> b/UnitTestFrameworkPkg/Library/UnitTestPersistenceLibSimpleFileSystem/UnitT >>>> estPersistenceLibSimpleFileSystem.c >>>> @@ -22,7 +22,7 @@ >>>> #define CACHE_FILE_SUFFIX L"_Cache.dat" >>>> >>>> >>>> >>>> /** >>>> >>>> - 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 +31,8 @@ >>>> >>>> >>>> **/ >>>> >>>> STATIC >>>> >>>> -EFI_DEVICE_PATH_PROTOCOL * >>>> >>>> -GetCacheFileDevicePath ( >>>> >>>> +CHAR16 * >>>> >>>> +GetCacheFileName ( >>>> >>>> IN UNIT_TEST_FRAMEWORK_HANDLE FrameworkHandle >>>> >>>> ) >>>> >>>> { >>>> >>>> @@ -85,7 +85,12 @@ GetCacheFileDevicePath ( >>>> // PathCleanUpDirectories (FileNameCopy); >>>> >>>> // if (PathRemoveLastItem (FileNameCopy)) { >>>> >>>> // >>>> >>>> - AppPath = ConvertDevicePathToText (LoadedImage- >>> FilePath, >>>> TRUE, TRUE); // NOTE: This must be freed. >>>> >>>> + AppPath = ConvertDevicePathToText (LoadedImage->FilePath, TRUE, >> TRUE); >>>> // NOTE: This must be freed. >>>> >>>> + if (AppPath == NULL) { >>>> >>>> + DEBUG ((DEBUG_ERROR, "%a - Failed to convert device path to >> text.\n", >>>> __func__)); >>>> >>>> + goto Exit; >>>> >>>> + } >>>> >>>> + >>>> >>>> DirectorySlashOffset = StrLen (AppPath); >>>> >>>> // >>>> >>>> // Make sure we didn't get any weird data. >>>> >>>> @@ -148,15 +153,15 @@ Exit: >>>> FreePool (AppPath); >>>> >>>> } >>>> >>>> >>>> >>>> - if (CacheFilePath != NULL) { >>>> >>>> - FreePool (CacheFilePath); >>>> >>>> + if (CacheFileDevicePath != NULL) { >>>> >>>> + FreePool (CacheFileDevicePath); >>>> >>>> } >>>> >>>> >>>> >>>> if (TestName != NULL) { >>>> >>>> FreePool (TestName); >>>> >>>> } >>>> >>>> >>>> >>>> - return CacheFileDevicePath; >>>> >>>> + return CacheFilePath; >>>> >>>> } >>>> >>>> >>>> >>>> /** >>>> >>>> @@ -175,21 +180,21 @@ 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); >>>> >>>> >>>> >>>> // >>>> >>>> // 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 +203,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 +234,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 +250,13 @@ SaveUnitTestCache ( >>>> // Determine the path for the cache file. >>>> >>>> // NOTE: This devpath is allocated and must be freed. >>>> >>>> // >>>> >>>> - FileDevicePath = GetCacheFileDevicePath (FrameworkHandle); >>>> >>>> + FileName = GetCacheFileName (FrameworkHandle); >>>> >>>> >>>> >>>> // >>>> >>>> // 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 +275,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 +309,8 @@ SaveUnitTestCache ( >>>> ShellCloseFile (&FileHandle); >>>> >>>> >>>> >>>> Exit: >>>> >>>> - if (FileDevicePath != NULL) { >>>> >>>> - FreePool (FileDevicePath); >>>> >>>> + if (FileName != NULL) { >>>> >>>> + FreePool (FileName); >>>> >>>> } >>>> >>>> >>>> >>>> return Status; >>>> >>>> @@ -334,13 +339,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 +361,13 @@ LoadUnitTestCache ( >>>> // Determine the path for the cache file. >>>> >>>> // NOTE: This devpath is allocated and must be freed. >>>> >>>> // >>>> >>>> - FileDevicePath = GetCacheFileDevicePath (FrameworkHandle); >>>> >>>> + FileName = GetCacheFileName (FrameworkHandle); >>>> >>>> >>>> >>>> // >>>> >>>> // 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 +412,8 @@ Exit: >>>> // >>>> >>>> // Free allocated buffers >>>> >>>> // >>>> >>>> - if (FileDevicePath != NULL) { >>>> >>>> - FreePool (FileDevicePath); >>>> >>>> + if (FileName != NULL) { >>>> >>>> + FreePool (FileName); >>>> >>>> } >>>> >>>> >>>> >>>> if (IsFileOpened) { >>>> >>>> -- >>>> 2.40.1.windows.1 > > > >