From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=104.47.2.57; helo=eur01-db5-obe.outbound.protection.outlook.com; envelope-from=meenakshi.aggarwal@nxp.com; receiver=edk2-devel@lists.01.org Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0057.outbound.protection.outlook.com [104.47.2.57]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D9928223C1766 for ; Mon, 19 Feb 2018 22:13:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=o5p6HWyLhUY/Hhw9+lCjosDwqcpK+/Loon0LRU7o5ls=; b=D1tNe6fP09GnSEyLN45FQ7zwhdIdwnyF7U4R66i65RHg2ZDnYwpDlwXhKnOsJU5fcXhc1NRpBhws64BvTKZaf3i951J9wA58JCYizNfKCt1+LRKpgPuN3VKhD8NeHjAN24tbWV2ykmMSv9hmvfb3lquxdYFj+bqrLQQ8YPc0nyM= Received: from DB5PR04MB0998.eurprd04.prod.outlook.com (10.161.199.12) by DB5PR04MB0936.eurprd04.prod.outlook.com (10.161.195.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.506.18; Tue, 20 Feb 2018 06:19:39 +0000 Received: from DB5PR04MB0998.eurprd04.prod.outlook.com ([fe80::5b4:dfb7:891f:32ce]) by DB5PR04MB0998.eurprd04.prod.outlook.com ([fe80::5b4:dfb7:891f:32ce%13]) with mapi id 15.20.0506.023; Tue, 20 Feb 2018 06:19:38 +0000 From: Meenakshi Aggarwal To: "Carsey, Jaben" , Leif Lindholm , "Ni, Ruiyu" CC: "Ye, Ting" , "edk2-devel@lists.01.org" , "Fu, Siyuan" , "ard.biesheuvel@linaro.org" Thread-Topic: [PATCH] Tftp assert fix for openfile failure case Thread-Index: AQHTWRVOLqxIhGxCkkG6Y1yGC2kEr6OiqepQgABeh4CAAAA58IAABh+AgApkhdA= Date: Tue, 20 Feb 2018 06:19:38 +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-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=meenakshi.aggarwal@nxp.com; x-originating-ip: [192.88.169.1] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB5PR04MB0936; 6:vsojioVRE9sYc+Nu2y4l+pvjswMBXfchVQD0TAoMxXDeYoRLB2wM2MZH3YD5Xgc8RjVaWy96CB1iZJyOuWLFMaM3mBaETSxiPG3U8EHqZhJ64rNIjHmG8NCQD+DL/1Bk034OhQM6uRFPyUBDmkiLf4oxGnp6kWMzixz7wXpY5P/GFu+sa5YjMkBBabfCoO9y+XsZ9fXnlZa8ZeaRv8ZdNtYJZHeWzv+m6c0dx8BoXMvfeb4q1Qel+LGv8J6DLTrydiyiNHHTRBHIr1dh9uXihJzZo8ZvQyEu4KJS372/QWrpFsSKJzWNqXSKHPDuumTBRr7eINTGwN2LlMYuJ2TUExi9tQT7xwRg1y4uK/o4dhIF3RzFBgExwRkm2E/xJU/9; 5:XzmLsLUS0KKe61qsxsvfJ2iJO0r6kWPYSRkdmOWZAqVLxLszMLGb1+cdsn/u9VA+t+68V1vqoQHJZp1PNbQIUqoBRhl6FEx1Hy6A0hVwWIE74ITARmJ1vplaRfbam5tYm9015WJw+k19a1JxDK7MYCf6/mRsZRiE6WgMNebQWO0=; 24:beRPdj5gv/7xdi/evVhzPZzXQc4P9GGHMQnlL3IR5kOvs/1OZ9rM3sHcfvKb3Q27MvcH42Vrz+1g6mLJAoBI2rxuvAslkSv3k1YOR3o5Rtc=; 7:nXf5Fb6fwAuyNehqtONqTrTeyBlwP4JOR7tqvX4ZJypfI6nU49bjLJl3Xt+VBALo8Emuv7GtEYKeDnDCgLH5Htll84bFUhPgFz4zh4V8Rhwt0MGoY93TIxfAPfb4AxNnkrGIPSZ4bCHZFBhDTPGogz2FNiyt2yDtlmMPnTo+WlzbCQN3JWQcWELLEgCJ1Xcd31km32IWAj7b/vIPghYAN+0w8ThliDHdazBCcRML8TsuZpFKDHg8jLqVNe8r2frj x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: fb9187cd-c160-4733-6374-08d57829ebc4 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(5600026)(4604075)(3008032)(48565401081)(4534165)(4627221)(201703031133081)(201702281549075)(2017052603307)(7153060)(7193020); SRVR:DB5PR04MB0936; x-ms-traffictypediagnostic: DB5PR04MB0936: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(189930954265078)(185117386973197)(162533806227266)(45079756050767)(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001056)(6040501)(2401047)(5005006)(8121501046)(10201501046)(3002001)(3231101)(944501161)(93006095)(93001095)(6055026)(6041288)(20161123562045)(20161123564045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123560045)(20161123558120)(6072148)(201708071742011); SRVR:DB5PR04MB0936; BCL:0; PCL:0; RULEID:; SRVR:DB5PR04MB0936; x-forefront-prvs: 05891FB07F x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(346002)(366004)(39380400002)(396003)(39860400002)(376002)(13464003)(199004)(189003)(305945005)(74316002)(5250100002)(102836004)(53546011)(3280700002)(105586002)(3846002)(3660700001)(7736002)(6506007)(2906002)(59450400001)(25786009)(5660300001)(7696005)(93886005)(6116002)(86362001)(575784001)(76176011)(53936002)(4326008)(6246003)(68736007)(106356001)(14454004)(966005)(478600001)(2900100001)(33656002)(186003)(316002)(110136005)(99286004)(26005)(54906003)(8676002)(81166006)(81156014)(9686003)(55016002)(97736004)(45080400002)(229853002)(2950100002)(66066001)(6306002)(6436002)(8936002); DIR:OUT; SFP:1101; SCL:1; SRVR:DB5PR04MB0936; H:DB5PR04MB0998.eurprd04.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: qJNPANfUgWzGjqcEtm0xDZV8e9S5Bn/aYrwWNNt1MvBk4d2OC3CZgw6C9ZzJUlIBfW1GvXqmPE95KpZfx2fUO2fQoRWbfPcxtBQaKgD+tA7550eNg/F4GvCkmZVJU3g8nWWOd/x+QBZbgPfFDUHhB+fLd4xfGnj41TEi2rkdtXY= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: fb9187cd-c160-4733-6374-08d57829ebc4 X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Feb 2018 06:19:38.6520 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5PR04MB0936 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, 20 Feb 2018 06:13:44 -0000 Content-Language: en-US Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable Hi Ray, Please share your thoughts on this. Thanks, Meenakshi > -----Original Message----- > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > Sent: Tuesday, February 13, 2018 9:07 PM > To: Meenakshi Aggarwal ; Leif Lindholm > ; Ni, Ruiyu > Cc: Ye, Ting ; edk2-devel@lists.01.org; Fu, Siyuan > ; ard.biesheuvel@linaro.org > Subject: RE: [PATCH] Tftp assert fix for openfile failure case >=20 > 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. >=20 > -Jaben >=20 > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Meenakshi Aggarwal > > Sent: Tuesday, February 13, 2018 7:19 AM > > To: Carsey, Jaben ; Leif Lindholm > > ; Ni, Ruiyu > > Cc: Ye, Ting ; edk2-devel@lists.01.org; Fu, Siyuan > > ; ard.biesheuvel@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 h= ow > > 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 wi= th > > inclusion of one comment of Leif > > " > > > > > > > > If the code is to be kept, I think (from a quick glance) th= at I > > > > > > > > would also like to see > > > > > > > > *Data =3D NULL > > > > > > > > in the error path of DownloadFile(). > > " > > > > Thanks, > > Meenakshi > > > > > -----Original Message----- > > > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > > > Sent: Tuesday, February 13, 2018 8:44 PM > > > To: Meenakshi Aggarwal ; Leif Lindholm > > > ; Ni, Ruiyu > > > Cc: ard.biesheuvel@linaro.org; Ye, Ting ; edk2- > > > devel@lists.01.org; Fu, Siyuan ; Udit Kumar > > > > > > Subject: RE: [PATCH] Tftp assert fix for openfile failure case > > > > > > Meenakshi, > > > > > > The TFTP command is outside the UEFI Shell specification, therefore i= t is > > > included as a DynamicCommand, not a command built into the shell itse= lf. > > > > > > 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 > > > > > > > > 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 Lei= f's > > > > comments] > > > > > > > > > > > > Thanks, > > > > Meenakshi > > > > > > > > > -----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/Tft= p.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 existi= ng > > > code. > > > > > > > > > > > > > > > } > > > > > > > > > + 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, Da= ta); > > > > > > > > > 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 h= ow > > 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 fou= nd this > > > > > > > useful. During development, this is useful to transfer generi= c 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 seein= g > > > > > > platforms including it even in RELEASE builds (most likely b= ecause > 1) > > > > > > 3) code quality/maintainability > > > > > > > > > > > > > If the code is to be kept, I think (from a quick glance) th= at 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 > > 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=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 > > _______________________________________________ > > 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%7C6f1efaa8f701 > 48a23c7b08d572f79ec1%7Cbd8a2a2207224ec7b35f1c4f0497e341%7C0%7C0% > 7C636541330242351234&sdata=3DqOkb3XaVjUhsi3cVkoskQN6z%2Bv8ySuCq1W > gFk90mZh4%3D&reserved=3D0