[PATCH] ShellPkg: Create a homefilesystem environment variable

[PATCH] ShellPkg: Create a homefilesystem environment variable

[Jim.Dailey]

Create a homefilesystem environment variable whose value is the file
system on which the executing shell is located. For example: "FS14:".

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
execute %homefilesystem% to set the cwd to the root of the file system
where the shell is located.

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

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);
+
     StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size, ((FILEPATH_DEVICE_PATH *)FileDevicePath)->PathName, 0);
     PathRemoveLastItem (StartupScriptPath);
     StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size, mStartupScript, 0);
-- 
2.17.0.windows.1

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

[Carsey, Jaben]

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:".
> 
> 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
> execute %homefilesystem% to set the cwd to the root of the file system
> where the shell is located.
> 
> 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
> 
> 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);
> +
>      StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size,
> ((FILEPATH_DEVICE_PATH *)FileDevicePath)->PathName, 0);
>      PathRemoveLastItem (StartupScriptPath);
>      StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size,
> mStartupScript, 0);
> --
> 2.17.0.windows.1

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

[Carsey, Jaben]

Pushed.
c0b1f749ef1304810ed4ea58ded65b7f41d79d3e

> -----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:".
> >
> > 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
> > execute %homefilesystem% to set the cwd to the root of the file system
> > where the shell is located.
> >
> > 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
> >
> > 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);
> > +
> >      StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size,
> > ((FILEPATH_DEVICE_PATH *)FileDevicePath)->PathName, 0);
> >      PathRemoveLastItem (StartupScriptPath);
> >      StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size,
> > mStartupScript, 0);
> > --
> > 2.17.0.windows.1

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

[Laszlo Ersek]

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

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

[Jim.Dailey]

>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek

I'll attempt answer some of your questions, but Jaben may have to
answer some of them (like his commit speed :-) or questions about what
the shell spec allows).

>
>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.

In such a case homefilesystem will not get defined, I think.

>>>> 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.

The shell does not handle "cd fsN:".  But "fsN:" does work.  I suppose
one could always add a "cd \" after "%hoemfilesystem%", but I think it
will not have any effect in most (all?) cases where homefilesystem is
defined.

>>>> +    InternalEfiShellSetEnv(L"homefilesystem", StartupScriptPath, TRUE);
>>>> +
>
>Again, this refers to the startup script, not the shell itself.

The variable's name implies the startup script, but at the point it is
used, it contains only the file system where the shell itself was found.
Code following this continues to modify the variable's value until it
eventually does point to where the startup script *might* be.

>Thanks
>Laszlo
>

Regards,
Jim

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

[Andrew Fish]

> On Oct 4, 2018, at 10:07 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> 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.
> 

Same if the Shell was network booted. 

Thanks,

Andrew Fish

>>>> 
>>>> 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 <mailto:edk2-devel@lists.01.org>
>> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>
>> 
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>

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

[Laszlo Ersek]

On 10/04/18 21:05, Jim.Dailey@dell.com wrote:
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> 
> I'll attempt answer some of your questions, but Jaben may have to
> answer some of them (like his commit speed :-) or questions about what
> the shell spec allows).
> 
>>
>> 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.
> 
> In such a case homefilesystem will not get defined, I think.

OK, thanks.

> 
>>>>> 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.
> 
> The shell does not handle "cd fsN:".  But "fsN:" does work.  I suppose
> one could always add a "cd \" after "%hoemfilesystem%", but I think it
> will not have any effect in most (all?) cases where homefilesystem is
> defined.

My mistake.

>>>>> +    InternalEfiShellSetEnv(L"homefilesystem", StartupScriptPath, TRUE);
>>>>> +
>>
>> Again, this refers to the startup script, not the shell itself.
> 
> The variable's name implies the startup script, but at the point it is
> used, it contains only the file system where the shell itself was found.
> Code following this continues to modify the variable's value until it
> eventually does point to where the startup script *might* be.

Thanks for explaining!
Laszlo

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

[Carsey, Jaben]

Laszlo,

The leading "_" was required for out of spec, but built in, commands.  The spec has no restrictions on environment variables except some have special meaning and may be read only.

I can certainly work on slowing down the process.  I have been complaining about that same thing and should have been more aware.  I would like to have a community minimum amount of time before commits are done that we all agree to.  Something like 1 full day would be nice I think.

-Jaben

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Andrew Fish
> Sent: Thursday, October 04, 2018 12:20 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment
> variable
> Importance: High
> 
> 
> 
> > On Oct 4, 2018, at 10:07 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> >
> > 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.
> >
> 
> Same if the Shell was network booted.
> 
> Thanks,
> 
> Andrew Fish
> 
> >>>>
> >>>> 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 <mailto:edk2-devel@lists.01.org>
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> <https://lists.01.org/mailman/listinfo/edk2-devel>
> >>
> >
> > Thanks
> > Laszlo
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
> > https://lists.01.org/mailman/listinfo/edk2-devel
> <https://lists.01.org/mailman/listinfo/edk2-devel>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Laszlo Ersek]

On 10/04/18 22:54, Carsey, Jaben wrote:
> Laszlo,
> 
> The leading "_" was required for out of spec, but built in, commands.  The spec has no restrictions on environment variables except some have special meaning and may be read only.
> 
> I can certainly work on slowing down the process.  I have been complaining about that same thing and should have been more aware.  I would like to have a community minimum amount of time before commits are done that we all agree to.  Something like 1 full day would be nice I think.

Good idea! I believe 24 hours should be tolerable on all ends. It also
gives a chance to people in other time zones to comment.

I think there should be one exception: grave regressions -- build
failures, or total boot failures -- should be possible to revert (or
fix) as soon as there's one review.

Thanks!
Laszlo

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

[Tim Lewis]

Jaben --

Following on this: shouldn't this be a spec issue? If you are asking people
to depend on the behavior.

Thanks,
Tim

-----Original Message-----
From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Laszlo Ersek
Sent: Friday, October 5, 2018 4:33 AM
To: Carsey, Jaben <jaben.carsey@intel.com>; Andrew Fish <afish@apple.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment
variable

On 10/04/18 22:54, Carsey, Jaben wrote:
> Laszlo,
> 
> The leading "_" was required for out of spec, but built in, commands.  The
spec has no restrictions on environment variables except some have special
meaning and may be read only.
> 
> I can certainly work on slowing down the process.  I have been complaining
about that same thing and should have been more aware.  I would like to have
a community minimum amount of time before commits are done that we all agree
to.  Something like 1 full day would be nice I think.

Good idea! I believe 24 hours should be tolerable on all ends. It also gives
a chance to people in other time zones to comment.

I think there should be one exception: grave regressions -- build failures,
or total boot failures -- should be possible to revert (or
fix) as soon as there's one review.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Carsey, Jaben]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, October 05, 2018 4:33 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>; Andrew Fish
> <afish@apple.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment
> variable
> Importance: High
> 
> On 10/04/18 22:54, Carsey, Jaben wrote:
> > Laszlo,
> >
> > The leading "_" was required for out of spec, but built in, commands.  The
> spec has no restrictions on environment variables except some have special
> meaning and may be read only.
> >
> > I can certainly work on slowing down the process.  I have been complaining
> about that same thing and should have been more aware.  I would like to
> have a community minimum amount of time before commits are done that
> we all agree to.  Something like 1 full day would be nice I think.
> 
> Good idea! I believe 24 hours should be tolerable on all ends. It also
> gives a chance to people in other time zones to comment.

So how do we proceed?  What's the "deciding method" to have a minimum time to allow for reviews?

> 
> I think there should be one exception: grave regressions -- build
> failures, or total boot failures -- should be possible to revert (or
> fix) as soon as there's one review.

Agreed.  We need some way around when an error is made.

> 
> Thanks!
> Laszlo

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

[Laszlo Ersek]

On 10/05/18 17:00, Carsey, Jaben wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, October 05, 2018 4:33 AM
>> To: Carsey, Jaben <jaben.carsey@intel.com>; Andrew Fish
>> <afish@apple.com>
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment
>> variable
>> Importance: High
>>
>> On 10/04/18 22:54, Carsey, Jaben wrote:
>>> Laszlo,
>>>
>>> The leading "_" was required for out of spec, but built in, commands.  The
>> spec has no restrictions on environment variables except some have special
>> meaning and may be read only.
>>>
>>> I can certainly work on slowing down the process.  I have been complaining
>> about that same thing and should have been more aware.  I would like to
>> have a community minimum amount of time before commits are done that
>> we all agree to.  Something like 1 full day would be nice I think.
>>
>> Good idea! I believe 24 hours should be tolerable on all ends. It also
>> gives a chance to people in other time zones to comment.
> 
> So how do we proceed?  What's the "deciding method" to have a minimum time to allow for reviews?

I think we can handle this as part of the following:

https://lists.01.org/pipermail/edk2-devel/2018-October/030385.html

Thanks
Laszlo

> 
>>
>> I think there should be one exception: grave regressions -- build
>> failures, or total boot failures -- should be possible to revert (or
>> fix) as soon as there's one review.
> 
> Agreed.  We need some way around when an error is made.
> 
>>
>> Thanks!
>> Laszlo

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

[Ni, Ruiyu]

On 10/4/2018 12:02 AM, Jim.Dailey@dell.com wrote:
> Create a homefilesystem environment variable whose value is the file
> system on which the executing shell is located. For example: "FS14:".
> 
> 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
> execute %homefilesystem% to set the cwd to the root of the file system
> where the shell is located.
> 
> 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
> 
> 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);
> +
>       StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size, ((FILEPATH_DEVICE_PATH *)FileDevicePath)->PathName, 0);
>       PathRemoveLastItem (StartupScriptPath);
>       StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size, mStartupScript, 0);
> 
Jim,
Creating spec-undefined "homefilesystem" ENV variable is not a good idea 
in my opinion.
Can you submit a Shell Spec change and change the implementation once 
the spec change is approved?

-- 
Thanks,
Ray

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

[Jim.Dailey]

>-----Original Message-----
>From: Ni, Ruiyu [mailto:ruiyu.ni@Intel.com]
>Sent: Monday, October 8, 2018 1:42 AM
>To: Dailey, Jim; edk2-devel@lists.01.org
>Cc: jaben.carsey@intel.com
>Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment variable
>
>
>On 10/4/2018 12:02 AM, Jim.Dailey@dell.com wrote:
>> Create a homefilesystem environment variable whose value is the file
>> system on which the executing shell is located. For example: "FS14:".
>>
>Jim,
>Creating spec-undefined "homefilesystem" ENV variable is not a good idea
>in my opinion.
>Can you submit a Shell Spec change and change the implementation once
>the spec change is approved?

Ray,

I think you and Jaben should get together on this:

>>-----Original Message-----
>>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>>Sent: Thursday, October 4, 2018 12:07 PM
>>To: Carsey, Jaben; Dailey, Jim; edk2-devel@lists.01.org
>>Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment variable
>>
>>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.)
>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Carsey, Jaben
>>> Sent: Thursday, October 4, 2018 3:54 PM
>>> To: Andrew Fish; Laszlo Ersek
>>> Cc: edk2-devel@lists.01.org
>>> Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment variable
>>>
>>> Laszlo,
>>>
>>> The leading "_" was required for out of spec, but built in, commands.
>>> The spec has no restrictions on environment variables except some have
>>> special meaning and may be read only.
>>>

Besides, if I were you and didn't have a lot of free time, I wouldn't unleash
me on shell spec changes! :-)

>--
>Thanks,
>Ray

Regards,
Jim

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

[Ni, Ruiyu]

On 10/5/2018 8:19 PM, Tim Lewis wrote:
> Jaben --
> 
> Following on this: shouldn't this be a spec issue? If you are asking people
> to depend on the behavior.

I agree. So I suggest submit a Spec ECR for this change.
Since it was checked in, we can revert it if the ECR is rejected.

> 
> Thanks,
> Tim
> 
> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Laszlo Ersek
> Sent: Friday, October 5, 2018 4:33 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>; Andrew Fish <afish@apple.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment
> variable
> 
> On 10/04/18 22:54, Carsey, Jaben wrote:
>> Laszlo,
>>
>> The leading "_" was required for out of spec, but built in, commands.  The
> spec has no restrictions on environment variables except some have special
> meaning and may be read only.
>>
>> I can certainly work on slowing down the process.  I have been complaining
> about that same thing and should have been more aware.  I would like to have
> a community minimum amount of time before commits are done that we all agree
> to.  Something like 1 full day would be nice I think.
> 
> Good idea! I believe 24 hours should be tolerable on all ends. It also gives
> a chance to people in other time zones to comment.
> 
> I think there should be one exception: grave regressions -- build failures,
> or total boot failures -- should be possible to revert (or
> fix) as soon as there's one review.
> 
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


-- 
Thanks,
Ray