Re: Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform

[Laszlo Ersek <lersek@redhat.com>]

On 10/05/16 22:24, Shah, Tapan wrote:
> How about moving protocol locate from constructor to the actual function level and returning an error if it fails to locate (See attached patch file).

What function is that precisely? The hunk headers in the patch don't show.

If it is a function that is part of some file operation, then, although
the patch is an improvement because the shell won't ASSERT(), it's still
not perfect, because lack of the protocol will break that file operation
completely.

... The containing function seems to be ShellOpenFileByName(). Seems
like a quite central function to render inoperable if the collation
protocol is not present.

Actually, I'm quite confused about conformance to specific versions of
the UEFI spec. In UEFI v2.6, section "2.6.2 Platform-Specific Elements",
point 5, we have

    If a platform includes the ability to boot from a disk device, then
    the EFI_BLOCK_IO_PROTOCOL, the EFI_DISK_IO_PROTOCOL, the
    EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, and the
    EFI_UNICODE_COLLATION_PROTOCOL are required.

In fact this requirement goes back to UEFI v2.3.1c at least. (I didn't
bother to check earlier releases of the spec.) By depending on the
protocol in the shell, we say that the current upstream shell is not
required to open files by name on pre-v2.3.1c systems (or perhaps on
systems older than v2.3.1c), and on systems that cannot boot from a disk
device.

Is this something we can say? How is this kind of compatibility tracked
in edk2?

Thanks
Laszlo

> 
> Tapan
> 
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Wednesday, October 05, 2016 3:18 PM
> To: Andrew Fish <afish@apple.com>; Daniil Egranov <daniil.egranov@arm.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel-01 <edk2-devel@ml01.01.org>; Leif Lindholm <Leif.Lindholm@arm.com>; Shah, Tapan <tapandshah@hpe.com>
> Subject: Re: [edk2] Assert in ShellPkg with latest tianocore edk2 source on the Reference Platform
> 
> On 10/05/16 21:48, Andrew Fish wrote:
>> > 
>>> >> On Oct 5, 2016, at 12:24 PM, Daniil Egranov <daniil.egranov@arm.com> wrote:
>>> >>
>>> >> I have the same ASSERT issue on Juno platform even the EnglishDxe.inf is included to the platform build. If UefiShellLib has such dependency on the protocol then according to EDKII Module Writer's Guide you need to specify the dependency on protocol in the module .inf to ensure the drivers proper load sequence.
>>> >>
>>> >> 8.6 Dependency Expressions
>>> >> A dependency expression specifies the protocols that the DXE driver 
>>> >> requires to execute. In EDK II, it is specified in the [Depex] section of INF file.
>>> >>
>> > 
>> > The Dependency Expression is for DXE Drivers that are dispatched by the DXE Core. A UEFI Driver from an option ROM or an Application does not get dispatched by the dispatch and the Depex will not help. The Depex ends up being a section in the FV and it has nothing to do with the PE/COFF image of the an application or option ROM. 
>> > 
>> > IMHO the shell should try as hard as possible to function and should not ASSERT if some newer Protocol is missing.  
> (Background: commit 583448b441650.)
> 
> In this specific case, the protocol dependency seems possible to eliminate:
> 
> - Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c
> includes the CharToUpper() function, which is minimal -- could even be copied or moved over -- and can translate the ASCII subset of UCS-2,
> 
> - once the ASCII characters of the input string have been translated to upper case, the result could be searched for / compared against L"NULL"
> using BaseLib.h APIs.
> 
> (Given that L"NULL" only contains characters from the ASCII subset, it suffices to upper-case ASCII chars in the input string. No non-ASCII character in the BMP can translate to ASCII N/U/L via upcasing, even with the real collation protocol (I trust), and no other ASCII characters than n/u/l will translate to N/U/L. This means that we won't miss an instance of NULL because we didn't upcate it (because it was non-ascii), and it also means we won't mistakenly match something non-NULL as NULL.)
> 
> Just my two cents.
> 
> Laszlo
> 
> 
> uefishelllib_fix.diff
> 
> 
> Index: ShellPkg/Library/UefiShellLib/UefiShellLib.c
> ===================================================================
> --- ShellPkg/Library/UefiShellLib/UefiShellLib.c	(revision 22692)
> +++ ShellPkg/Library/UefiShellLib/UefiShellLib.c	(working copy)
> @@ -292,8 +292,6 @@
>    IN EFI_SYSTEM_TABLE  *SystemTable
>    )
>  {
> -  EFI_STATUS  Status;
> -
>    mEfiShellEnvironment2       = NULL;
>    gEfiShellProtocol           = NULL;
>    gEfiShellParametersProtocol = NULL;
> @@ -300,11 +298,6 @@
>    mEfiShellInterface          = NULL;
>    mEfiShellEnvironment2Handle = NULL;
>  
> -  if (mUnicodeCollationProtocol == NULL) {
> -    Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid, NULL, (VOID**)&mUnicodeCollationProtocol);
> -    ASSERT_EFI_ERROR (Status);
> -  }
> -
>    //
>    // verify that auto initialize is not set false
>    //
> @@ -730,6 +723,14 @@
>                                                 FileHandle,
>                                                 OpenMode);
>  
> +    if (mUnicodeCollationProtocol == NULL) {
> +      Status = gBS->LocateProtocol (&gEfiUnicodeCollation2ProtocolGuid, NULL, (VOID**)&mUnicodeCollationProtocol);
> +      if (EFI_ERROR (Status)) {
> +        gEfiShellProtocol->CloseFile (*FileHandle);
> +        return Status;
> +      }
> +    }
> +
>      if ((mUnicodeCollationProtocol->StriColl (mUnicodeCollationProtocol, (CHAR16*)FileName, L"NUL") != 0) &&
>          (mUnicodeCollationProtocol->StriColl (mUnicodeCollationProtocol, (CHAR16*)FileName, L"NULL") != 0) &&
>           !EFI_ERROR(Status) && ((OpenMode & EFI_FILE_MODE_CREATE) != 0)){
>