* [PATCH] ShellPkg: Create a homefilesystem environment variable @ 2018-10-03 16:02 Jim.Dailey 2018-10-03 18:15 ` Carsey, Jaben ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Jim.Dailey @ 2018-10-03 16:02 UTC (permalink / raw) To: edk2-devel; +Cc: jaben.carsey, ruiyu.ni 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-03 16:02 [PATCH] ShellPkg: Create a homefilesystem environment variable Jim.Dailey @ 2018-10-03 18:15 ` Carsey, Jaben 2018-10-03 18:17 ` Carsey, Jaben 2018-10-08 6:42 ` Ni, Ruiyu 2 siblings, 0 replies; 15+ messages in thread From: Carsey, Jaben @ 2018-10-03 18:15 UTC (permalink / raw) To: Jim.Dailey@dell.com, edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-03 16:02 [PATCH] ShellPkg: Create a homefilesystem environment variable Jim.Dailey 2018-10-03 18:15 ` Carsey, Jaben @ 2018-10-03 18:17 ` Carsey, Jaben 2018-10-04 17:07 ` Laszlo Ersek 2018-10-08 6:42 ` Ni, Ruiyu 2 siblings, 1 reply; 15+ messages in thread From: Carsey, Jaben @ 2018-10-03 18:17 UTC (permalink / raw) To: Jim.Dailey@dell.com, edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-03 18:17 ` Carsey, Jaben @ 2018-10-04 17:07 ` Laszlo Ersek 2018-10-04 19:05 ` Jim.Dailey 2018-10-04 19:19 ` Andrew Fish 0 siblings, 2 replies; 15+ messages in thread From: Laszlo Ersek @ 2018-10-04 17:07 UTC (permalink / raw) To: Carsey, Jaben, Jim.Dailey@dell.com, edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-04 17:07 ` Laszlo Ersek @ 2018-10-04 19:05 ` Jim.Dailey 2018-10-04 19:20 ` Laszlo Ersek 2018-10-04 19:19 ` Andrew Fish 1 sibling, 1 reply; 15+ messages in thread From: Jim.Dailey @ 2018-10-04 19:05 UTC (permalink / raw) To: lersek; +Cc: jaben.carsey, edk2-devel >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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-04 19:05 ` Jim.Dailey @ 2018-10-04 19:20 ` Laszlo Ersek 0 siblings, 0 replies; 15+ messages in thread From: Laszlo Ersek @ 2018-10-04 19:20 UTC (permalink / raw) To: Jim.Dailey; +Cc: jaben.carsey, edk2-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-04 17:07 ` Laszlo Ersek 2018-10-04 19:05 ` Jim.Dailey @ 2018-10-04 19:19 ` Andrew Fish 2018-10-04 20:54 ` Carsey, Jaben 1 sibling, 1 reply; 15+ messages in thread From: Andrew Fish @ 2018-10-04 19:19 UTC (permalink / raw) To: Laszlo Ersek; +Cc: Carsey, Jaben, Jim.Dailey@dell.com, edk2-devel@lists.01.org > 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-04 19:19 ` Andrew Fish @ 2018-10-04 20:54 ` Carsey, Jaben 2018-10-05 11:32 ` Laszlo Ersek 0 siblings, 1 reply; 15+ messages in thread From: Carsey, Jaben @ 2018-10-04 20:54 UTC (permalink / raw) To: Andrew Fish, Laszlo Ersek; +Cc: edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-04 20:54 ` Carsey, Jaben @ 2018-10-05 11:32 ` Laszlo Ersek 2018-10-05 12:19 ` Tim Lewis 2018-10-05 15:00 ` Carsey, Jaben 0 siblings, 2 replies; 15+ messages in thread From: Laszlo Ersek @ 2018-10-05 11:32 UTC (permalink / raw) To: Carsey, Jaben, Andrew Fish; +Cc: edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-05 11:32 ` Laszlo Ersek @ 2018-10-05 12:19 ` Tim Lewis 2018-10-09 4:33 ` Ni, Ruiyu 2018-10-05 15:00 ` Carsey, Jaben 1 sibling, 1 reply; 15+ messages in thread From: Tim Lewis @ 2018-10-05 12:19 UTC (permalink / raw) To: 'Laszlo Ersek', 'Carsey, Jaben', 'Andrew Fish' Cc: edk2-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-05 12:19 ` Tim Lewis @ 2018-10-09 4:33 ` Ni, Ruiyu 0 siblings, 0 replies; 15+ messages in thread From: Ni, Ruiyu @ 2018-10-09 4:33 UTC (permalink / raw) To: Tim Lewis, 'Laszlo Ersek', 'Carsey, Jaben', 'Andrew Fish' Cc: edk2-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-05 11:32 ` Laszlo Ersek 2018-10-05 12:19 ` Tim Lewis @ 2018-10-05 15:00 ` Carsey, Jaben 2018-10-05 17:47 ` Laszlo Ersek 1 sibling, 1 reply; 15+ messages in thread From: Carsey, Jaben @ 2018-10-05 15:00 UTC (permalink / raw) To: Laszlo Ersek, Andrew Fish; +Cc: edk2-devel@lists.01.org > -----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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-05 15:00 ` Carsey, Jaben @ 2018-10-05 17:47 ` Laszlo Ersek 0 siblings, 0 replies; 15+ messages in thread From: Laszlo Ersek @ 2018-10-05 17:47 UTC (permalink / raw) To: Carsey, Jaben, Andrew Fish; +Cc: edk2-devel@lists.01.org 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-03 16:02 [PATCH] ShellPkg: Create a homefilesystem environment variable Jim.Dailey 2018-10-03 18:15 ` Carsey, Jaben 2018-10-03 18:17 ` Carsey, Jaben @ 2018-10-08 6:42 ` Ni, Ruiyu 2018-10-08 13:58 ` Jim.Dailey 2 siblings, 1 reply; 15+ messages in thread From: Ni, Ruiyu @ 2018-10-08 6:42 UTC (permalink / raw) To: Jim.Dailey, edk2-devel; +Cc: jaben.carsey 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ShellPkg: Create a homefilesystem environment variable 2018-10-08 6:42 ` Ni, Ruiyu @ 2018-10-08 13:58 ` Jim.Dailey 0 siblings, 0 replies; 15+ messages in thread From: Jim.Dailey @ 2018-10-08 13:58 UTC (permalink / raw) To: ruiyu.ni; +Cc: jaben.carsey, edk2-devel >-----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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-10-09 4:32 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-03 16:02 [PATCH] ShellPkg: Create a homefilesystem environment variable Jim.Dailey 2018-10-03 18:15 ` Carsey, Jaben 2018-10-03 18:17 ` Carsey, Jaben 2018-10-04 17:07 ` Laszlo Ersek 2018-10-04 19:05 ` Jim.Dailey 2018-10-04 19:20 ` Laszlo Ersek 2018-10-04 19:19 ` Andrew Fish 2018-10-04 20:54 ` Carsey, Jaben 2018-10-05 11:32 ` Laszlo Ersek 2018-10-05 12:19 ` Tim Lewis 2018-10-09 4:33 ` Ni, Ruiyu 2018-10-05 15:00 ` Carsey, Jaben 2018-10-05 17:47 ` Laszlo Ersek 2018-10-08 6:42 ` Ni, Ruiyu 2018-10-08 13:58 ` Jim.Dailey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox