Re: [PATCH] ShellPkg: Create a homefilesystem environment variable

[Laszlo Ersek <lersek@redhat.com>]

On 10/03/18 20:17, Carsey, Jaben wrote:
> Pushed.
> c0b1f749ef1304810ed4ea58ded65b7f41d79d3e

Please give other reviewers a bit more time than ~2 hours, to comment on
the patch. :)

I think I would have suggested an improvement (or a clarification about)
the commit message. It says:

>> -----Original Message-----
>> From: Carsey, Jaben
>> Sent: Wednesday, October 03, 2018 11:15 AM
>> To: 'Jim.Dailey@dell.com' <Jim.Dailey@dell.com>; edk2-devel@lists.01.org
>> Subject: RE: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment
>> variable
>>
>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
>>
>>> -----Original Message-----
>>> From: Jim.Dailey@dell.com [mailto:Jim.Dailey@dell.com]
>>> Sent: Wednesday, October 03, 2018 9:02 AM
>>> To: edk2-devel@lists.01.org
>>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu
>> <ruiyu.ni@intel.com>
>>> Subject: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment
>>> variable
>>> Importance: High
>>>
>>> Create a homefilesystem environment variable whose value is the file
>>> system on which the executing shell is located. For example: "FS14:".

that the file system in question contains the *shell*.

So my first question would have been, what if the shell is memory mapped
(from a firmware volume), but the platform doesn't expose firmware
filesystems (FFSs) as EFI simple file system protocol instances? In that
case, the "file system on which the executing shell is located" seems
ill-defined.

>>>
>>> This eliminates the need for people to have to try and find the "boot"
>>> file system in their startup script.  After this change they can simply

Note, here the commit message refers to the startup script, not the
shell itself.

>>> execute %homefilesystem% to set the cwd to the root of the file system
>>> where the shell is located.

I think the commit message here misses a "CD" command.

>>>
>>> A future enhancement could be to add "homefilesystem" to the list of
>>> predefined, read-only variables listed in the EfiShellSetEnv function of
>>> file ShellProtocol.c

Is it OK with the UEFI shell spec to define a shell variable called
"homefilesystem"? I seem to remember that edk2-specific options for
standard UEFI shell commands generally start with an underscore, to
avoid clashing with the standard namespace. Does that apply to shell
variables perhaps? (This is mostly for my own education.)

>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Jim Dailey <jim_dailey@dell.com>
>>> ---
>>>  ShellPkg/Application/Shell/Shell.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/ShellPkg/Application/Shell/Shell.c
>>> b/ShellPkg/Application/Shell/Shell.c
>>> index 3f3bcbb4b0..6185b6ac80 100644
>>> --- a/ShellPkg/Application/Shell/Shell.c
>>> +++ b/ShellPkg/Application/Shell/Shell.c
>>> @@ -1169,6 +1169,8 @@ LocateStartupScript (
>>>        *TempSpot = CHAR_NULL;
>>>      }
>>>
>>> +    InternalEfiShellSetEnv(L"homefilesystem", StartupScriptPath, TRUE);
>>> +

Again, this refers to the startup script, not the shell itself.

>>>      StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size,
>>> ((FILEPATH_DEVICE_PATH *)FileDevicePath)->PathName, 0);
>>>      PathRemoveLastItem (StartupScriptPath);
>>>      StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size,
>>> mStartupScript, 0);
>>> --
>>> 2.17.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Thanks
Laszlo