Hi Ray, Please share your thoughts on this.
Thanks, Meenakshi > -----Original Message----- > From: Carsey, Jaben [mailto:jaben.car...@intel.com] > Sent: Tuesday, February 13, 2018 9:07 PM > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Leif Lindholm > <leif.lindh...@linaro.org>; Ni, Ruiyu <ruiyu...@intel.com> > Cc: Ye, Ting <ting...@intel.com>; edk2-devel@lists.01.org; Fu, Siyuan > <siyuan...@intel.com>; ard.biesheu...@linaro.org > Subject: RE: [PATCH] Tftp assert fix for openfile failure case > > So I thought we are keeping the command, but I do agree with Leif that > better error path logic would be good. We can wait for Ray to confirm if he > has different plans. > > -Jaben > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > > Meenakshi Aggarwal > > Sent: Tuesday, February 13, 2018 7:19 AM > > To: Carsey, Jaben <jaben.car...@intel.com>; Leif Lindholm > > <leif.lindh...@linaro.org>; Ni, Ruiyu <ruiyu...@intel.com> > > Cc: Ye, Ting <ting...@intel.com>; edk2-devel@lists.01.org; Fu, Siyuan > > <siyuan...@intel.com>; ard.biesheu...@linaro.org > > Subject: Re: [edk2] [PATCH] Tftp assert fix for openfile failure case > > Importance: High > > > > Jaben, > > > > The patch was not accepted last time because as per Leif > > " > > > > > > > > This shell command was introduced in the heyday of "let's > > > > > > > > reimplement U-Boot in the EDK2 tree". Mainly, from my > > impression, > > > it > > > > > > > > seems to be used in order that people don't need to learn how > > boot > > > > > > > > managers and device paths work. > > > > > > > > > > > > > > When you say about complete boot, then this may not be useful. > > > > > > >" > > > > > > So, if we are maintaining tftp command, then i will resend the patch with > > inclusion of one comment of Leif > > " > > > > > > > > If the code is to be kept, I think (from a quick glance) that I > > > > > > > > would also like to see > > > > > > > > *Data = NULL > > > > > > > > in the error path of DownloadFile(). > > " > > > > Thanks, > > Meenakshi > > > > > -----Original Message----- > > > From: Carsey, Jaben [mailto:jaben.car...@intel.com] > > > Sent: Tuesday, February 13, 2018 8:44 PM > > > To: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Leif Lindholm > > > <leif.lindh...@linaro.org>; Ni, Ruiyu <ruiyu...@intel.com> > > > Cc: ard.biesheu...@linaro.org; Ye, Ting <ting...@intel.com>; edk2- > > > de...@lists.01.org; Fu, Siyuan <siyuan...@intel.com>; Udit Kumar > > > <udit.ku...@nxp.com> > > > Subject: RE: [PATCH] Tftp assert fix for openfile failure case > > > > > > Meenakshi, > > > > > > The TFTP command is outside the UEFI Shell specification, therefore it is > > > included as a DynamicCommand, not a command built into the shell itself. > > > > > > I a little confused by your last sentence. Do you want to send a new > patch? > > > or do you have a branch to pick changes from ? > > > > > > -Jaben > > > > > > > > > > -----Original Message----- > > > > From: Meenakshi Aggarwal [mailto:meenakshi.aggar...@nxp.com] > > > > Sent: Tuesday, February 13, 2018 1:43 AM > > > > To: Leif Lindholm <leif.lindh...@linaro.org>; Ni, Ruiyu > > > <ruiyu...@intel.com> > > > > Cc: ard.biesheu...@linaro.org; Ye, Ting <ting...@intel.com>; edk2- > > > > de...@lists.01.org; Carsey, Jaben <jaben.car...@intel.com>; Fu, > Siyuan > > > > <siyuan...@intel.com>; Udit Kumar <udit.ku...@nxp.com> > > > > Subject: RE: [PATCH] Tftp assert fix for openfile failure case > > > > Importance: High > > > > > > > > Hi, > > > > > > > > As per commit 0961002352e9115b72f544dded239ad226efe87b > > > > > > > > Tftp command will be maintained to extend internal commands and > > > > > > > > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c > > > > > > > > Looks like a copy of ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > > > > > > > So, below fix is needed in this case as well. > > > > > > > > Please suggest, so we can send the updated patch [incorporating Leif's > > > > comments] > > > > > > > > > > > > Thanks, > > > > Meenakshi > > > > > > > > > -----Original Message----- > > > > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On > Behalf > > Of > > > > > Udit Kumar > > > > > Sent: Thursday, November 09, 2017 10:13 AM > > > > > To: Leif Lindholm <leif.lindh...@linaro.org> > > > > > Cc: ruiyu...@intel.com; ard.biesheu...@linaro.org; > ting...@intel.com; > > > > edk2- > > > > > de...@lists.01.org; jaben.car...@intel.com; siyuan...@intel.com > > > > > Subject: Re: [edk2] [PATCH] Tftp assert fix for openfile failure case > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Leif Lindholm [mailto:leif.lindh...@linaro.org] > > > > > > Sent: Wednesday, November 08, 2017 8:52 PM > > > > > > To: Udit Kumar <udit.ku...@nxp.com> > > > > > > Cc: Vabhav Sharma <vabhav.sha...@nxp.com>; edk2- > > > > de...@lists.01.org; > > > > > > ruiyu...@intel.com; jaben.car...@intel.com; > > > ard.biesheu...@linaro.org; > > > > > > siyuan...@intel.com; ting...@intel.com > > > > > > Subject: Re: [PATCH] Tftp assert fix for openfile failure case > > > > > > > > > > > > On Wed, Nov 08, 2017 at 05:15:49AM +0000, Udit Kumar wrote: > > > > > > > > > diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > > > > > > > > b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > > > > > > > > index fbde3bf..6425fc5 100755 > > > > > > > > > --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > > > > > > > > +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > > > > > > > > @@ -509,6 +509,7 @@ ShellCommandRunTftp ( > > > > > > > > > ); > > > > > > > > > goto NextHandle; > > > > > > > > > > > > > > > > Wow, a goto in a foor loop in a 320-line function. > > > > > > > > What could possibly go wrong? > > > > > > > > > > > > > > Instead of being on some volume, if you are on Shell. > > > > > > > Then file open will fail. > > > > > > > > > > > > Sure. The above was a snarky comment on the state of the existing > > > code. > > > > > > > > > > > > > > > } > > > > > > > > > + DataSize = FileSize; > > > > > > > > > > > > > > > > > > Status = DownloadFile (Mtftp4, RemoteFilePath, > > > > > > > > > AsciiRemoteFilePath, > > > > > > > > FileSize, BlockSize, &Data); > > > > > > > > > if (EFI_ERROR (Status)) { > > > > > > > > > @@ -539,7 +540,6 @@ ShellCommandRunTftp ( > > > > > > > > > goto NextHandle; > > > > > > > > > } > > > > > > > > > > > > > > > > > > - DataSize = FileSize; > > > > > > > > > Status = ShellWriteFile (FileHandle, &FileSize, Data); > > > > > > > > > if (!EFI_ERROR (Status)) { > > > > > > > > > ShellStatus = SHELL_SUCCESS; > > > > > > > > > -- > > > > > > > > > 1.9.1 > > > > > > > > > > > > > > > > So, a wider question: > > > > > > > > This shell command was introduced in the heyday of "let's > > > > > > > > reimplement U-Boot in the EDK2 tree". Mainly, from my > > impression, > > > it > > > > > > > > seems to be used in order that people don't need to learn how > > boot > > > > > > > > managers and device paths work. > > > > > > > > > > > > > > When you say about complete boot, then this may not be useful. > > > > > > > > > > > > > > > Am I being too harsh? > > > > > > > > Are there practical uses for this? > > > > > > > > > > > > > > For doing some sort of unit testing of given interface. I found > > > > > > > this > > > > > > > useful. During development, this is useful to transfer generic > > > > > > > file > to > > > > > > > development board. > > > > > > > > > > > > OK, I can see how it could be useful. > > > > > > My opposition is based on three things: > > > > > > 1) people _are_ trying to use it for boot > > > > > > > > > > I agree with this, please see my previous comments, > > > > > ' When you say about complete boot, then this may not be useful.' > > > > > > > > > > > 2) not a command described by UEFI Shell spec, but I keep seeing > > > > > > platforms including it even in RELEASE builds (most likely > > > > > > because > 1) > > > > > > 3) code quality/maintainability > > > > > > > > > > > > > If the code is to be kept, I think (from a quick glance) that I > > > > > > > > would also like to see > > > > > > > > *Data = NULL > > > > > > > > in the error path of DownloadFile(). > > > > > > > > > > > > OK, so we don't need to drop it right now, but what's your take on > > this > > > > > > comment? > > > > > > > > > > I am fine, if you prefer to remove this then we will develop some test > > > > > application > > > > > for unit tests. > > > > > In case, we need to maintain this piece of code then above needs to > fix > > > as > > > > > well. > > > > > > > > > > > > > > > > / > > > > > > Leif > > > > > _______________________________________________ > > > > > edk2-devel mailing list > > > > > edk2-devel@lists.01.org > > > > > > > > > > > > > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist > > > > > s.01.org%2Fmailman%2Flistinfo%2Fedk2- > > > > > > > > > > > > > > > devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cc2673b1a07e > > > > > > > > > > > > > > > 94e9f937b08d5272c6e5b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0 > > > > > > > > > > > > > > > %7C636457994167308954&sdata=gBy9RA5d1NpsvxQkmET0HFzsJB8FK7KLueE > > > > > NXFTjSHY%3D&reserved=0 > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist > s.01.org%2Fmailman%2Flistinfo%2Fedk2- > devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C6f1efaa8f701 > 48a23c7b08d572f79ec1%7Cbd8a2a2207224ec7b35f1c4f0497e341%7C0%7C0% > 7C636541330242351234&sdata=qOkb3XaVjUhsi3cVkoskQN6z%2Bv8ySuCq1W > gFk90mZh4%3D&reserved=0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel