From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=jaben.carsey@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BE9D921CF1CE2 for ; Tue, 13 Feb 2018 07:08:23 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Feb 2018 07:14:13 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,507,1511856000"; d="scan'208";a="203846390" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga006.fm.intel.com with ESMTP; 13 Feb 2018 07:14:13 -0800 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 13 Feb 2018 07:14:13 -0800 Received: from fmsmsx103.amr.corp.intel.com ([169.254.2.47]) by FMSMSX151.amr.corp.intel.com ([169.254.7.150]) with mapi id 14.03.0319.002; Tue, 13 Feb 2018 07:14:12 -0800 From: "Carsey, Jaben" To: Meenakshi Aggarwal , Leif Lindholm , "Ni, Ruiyu" CC: "ard.biesheuvel@linaro.org" , "Ye, Ting" , "edk2-devel@lists.01.org" , "Fu, Siyuan" , Udit Kumar Thread-Topic: [PATCH] Tftp assert fix for openfile failure case Thread-Index: AQHTVH8FEsf2/Ge8e0K0AE2cf6IeBqMJwCyAgAC+OYCAAKl7gIAA38qAgJczk4D//9W14A== Date: Tue, 13 Feb 2018 15:14:13 +0000 Message-ID: References: <1509719192-16582-1-git-send-email-vabhav.sharma@nxp.com> <20171107175458.f4pgne4rkri7lzch@bivouac.eciton.net> <20171108152225.gy6te7rcroqqjr3j@bivouac.eciton.net> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNWRlOWFjNDgtYjMwYS00MGU3LWE2NWUtODlhMjc2ZmQ2ZjhmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IlpPb3pCSytUXC9JUFNNR2ZOVzFhbXZaZFRxbkllbnJoMWdmVVplNHlKakZjPSJ9 x-ctpclassification: CTP_NT x-originating-ip: [10.1.200.107] MIME-Version: 1.0 Subject: Re: [PATCH] Tftp assert fix for openfile failure case X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Feb 2018 15:08:24 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Meenakshi, The TFTP command is outside the UEFI Shell specification, therefore it is i= ncluded 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.aggarwal@nxp.com] > Sent: Tuesday, February 13, 2018 1:43 AM > To: Leif Lindholm ; Ni, Ruiyu > Cc: ard.biesheuvel@linaro.org; Ye, Ting ; edk2- > devel@lists.01.org; Carsey, Jaben ; Fu, Siyuan > ; Udit Kumar > Subject: RE: [PATCH] Tftp assert fix for openfile failure case > Importance: High >=20 > Hi, >=20 > As per commit 0961002352e9115b72f544dded239ad226efe87b >=20 > Tftp command will be maintained to extend internal commands and >=20 > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c >=20 > Looks like a copy of ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c >=20 > So, below fix is needed in this case as well. >=20 > Please suggest, so we can send the updated patch [incorporating Leif's > comments] >=20 >=20 > Thanks, > Meenakshi >=20 > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Udit Kumar > > Sent: Thursday, November 09, 2017 10:13 AM > > To: Leif Lindholm > > Cc: ruiyu.ni@intel.com; ard.biesheuvel@linaro.org; ting.ye@intel.com; > edk2- > > devel@lists.01.org; jaben.carsey@intel.com; siyuan.fu@intel.com > > Subject: Re: [edk2] [PATCH] Tftp assert fix for openfile failure case > > > > > > > > > -----Original Message----- > > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > > > Sent: Wednesday, November 08, 2017 8:52 PM > > > To: Udit Kumar > > > Cc: Vabhav Sharma ; edk2- > devel@lists.01.org; > > > ruiyu.ni@intel.com; jaben.carsey@intel.com; ard.biesheuvel@linaro.org= ; > > > siyuan.fu@intel.com; ting.ye@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 cod= e. > > > > > > > > > } > > > > > > + DataSize =3D FileSize; > > > > > > > > > > > > Status =3D DownloadFile (Mtftp4, RemoteFilePath, > > > > > > AsciiRemoteFilePath, > > > > > FileSize, BlockSize, &Data); > > > > > > if (EFI_ERROR (Status)) { > > > > > > @@ -539,7 +540,6 @@ ShellCommandRunTftp ( > > > > > > goto NextHandle; > > > > > > } > > > > > > > > > > > > - DataSize =3D FileSize; > > > > > > Status =3D ShellWriteFile (FileHandle, &FileSize, Data); > > > > > > if (!EFI_ERROR (Status)) { > > > > > > ShellStatus =3D 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 boo= t > > > > > 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 thi= s > > > > 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 =3D NULL > > > > > in the error path of DownloadFile(). > > > > > > OK, so we don't need to drop it right now, but what's your take on th= is > > > 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=3Dhttps%3A%2F%2Flist > > s.01.org%2Fmailman%2Flistinfo%2Fedk2- > > > devel&data=3D02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cc2673b1a07e > > > 94e9f937b08d5272c6e5b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0 > > > %7C636457994167308954&sdata=3DgBy9RA5d1NpsvxQkmET0HFzsJB8FK7KLueE > > NXFTjSHY%3D&reserved=3D0