From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=40.107.0.61; helo=eur02-am5-obe.outbound.protection.outlook.com; envelope-from=meenakshi.aggarwal@nxp.com; receiver=edk2-devel@lists.01.org Received: from EUR02-AM5-obe.outbound.protection.outlook.com (mail-eopbgr00061.outbound.protection.outlook.com [40.107.0.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1F41621CF1CE1 for ; Tue, 13 Feb 2018 07:13:35 -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=QyVgcn/DwxFHheI1zgxLvlZEgD1pEQhiBVk1KwDUjyg=; b=nFin8c/0Kee3Bg5tQpS5aHtY5/QgSJuNA7JcD6gUkipuaYqvchkN0XalAyja6Hnii77RgOcyki+hYEAfAuQP6O3SNh8GtwguIOlyrDHqh1IhjrsxbyrR/jtu7nkZLXtJANnV6b7i4JrOtFbYV+7ypSWRASKbHpbmR8maZApGg70= Received: from DB5PR04MB0998.eurprd04.prod.outlook.com (10.161.199.12) by DB5PR04MB1175.eurprd04.prod.outlook.com (10.162.155.17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.485.10; Tue, 13 Feb 2018 15:19:23 +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.0485.015; Tue, 13 Feb 2018 15:19:22 +0000 From: Meenakshi Aggarwal To: "Carsey, Jaben" , 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: AQHTWRVOLqxIhGxCkkG6Y1yGC2kEr6OiqepQgABeh4CAAAA58A== Date: Tue, 13 Feb 2018 15:19:22 +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; DB5PR04MB1175; 6:p1kByOp97Fl3iFXbn9Nvz+zt6Qy/gdJUdfVC93JtO8rrCiw38UPz/KIVN2cRkPuA5tmIkOwYxXv+IAASPMskbOoPPhqUJffEoULN8T6SWr1kmGcB7fDtwpaRLdGsRgsfmTekMUTC8iUMblPtfDjTNYjm0LGwpFhaPrMDvz6crCJprRkPMnV+F0ed+xVaBKUHJMOFU8e78pChd+dWX/p4eh2Mg289GMYrYhzkDpB2OvXRc1sRJuDc1gg8BMPqHpp1L7cNsVAyeiZPqaYle9kUbgC1tT8sl9uTUZ+QE2vU/4Ugx5vPCnmoM2KQdJlFaGXGkCcIALvWN1vXXqoYGH822lYB4CjqgW7rmoos9aQDwlV1qoZYTwdVivo8U86ZGN9Y; 5:QagvIJhtJa54naokonWkLD1At7ltFb7Xg5KW5w5TGhNbC1er7azI49RlOqnSmCXyHCbZoutSySXbwBbyQJDy1HWmmTc/dkndDrsTt+PXLWfFpuc+HJH3/ICt3MlHxrFDbMNlcf57FEm/vimUZ9FiAeLI3p3zvvJt7sLrmWepIAU=; 24:nC1Pdcy5GO+RDThTcC/4J2BDidhkpcQWp6Dp6u1ise0w+Yt2yFl9F1bx7PoDDD9lDit35XmyAT0lv9sbM0QxWPaKp1dZb0yvrAqUmYaMDBA=; 7:4XpsDNokb1yMVJdskpWhMrngATUoYzHjvqNLSArbv/lkxA7sDiVnIO7IHtPiY8J0YsSE9Ji+77A9XOhIA7gK6p5Z2wfekY/nCSzx10x3M4gpdLUMta/oH1I4HemPkQ0ThKQ4P+u9JPRS3MK8OD/xPdocEJ4ywMJ3wwCJ6HWR3MVRwcU88CSbMJnFM0hTvGNTpkQH8JVLVtWHZpk+WDrtWRMteADpAks4QlL9dYKHPKry7DinghzWA4/saXIHx7Fj x-ms-exchange-antispam-srfa-diagnostics: SSOS;SSOR; x-forefront-antispam-report: SFV:SKI; SCL:-1; SFV:NSPM; SFS:(10009020)(346002)(376002)(39860400002)(396003)(39380400002)(366004)(189003)(199004)(13464003)(6246003)(6306002)(81156014)(76176011)(33656002)(4326008)(7696005)(3660700001)(25786009)(86362001)(575784001)(186003)(6506007)(53546011)(2906002)(9686003)(81166006)(55016002)(8936002)(2900100001)(5250100002)(74316002)(59450400001)(229853002)(6436002)(66066001)(305945005)(102836004)(5660300001)(53936002)(106356001)(54906003)(7736002)(8676002)(14454004)(105586002)(99286004)(93886005)(316002)(68736007)(3280700002)(97736004)(26005)(6116002)(3846002)(478600001)(966005)(45080400002)(2950100002)(110136005); DIR:OUT; SFP:1101; SCL:1; SRVR:DB5PR04MB1175; H:DB5PR04MB0998.eurprd04.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 1547dd8b-8109-434d-30bb-08d572f5294d x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(48565401081)(5600026)(4604075)(3008032)(4534165)(4627221)(201703031133081)(201702281549075)(2017052603307)(7153060)(7193020); SRVR:DB5PR04MB1175; x-ms-traffictypediagnostic: DB5PR04MB1175: 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:(6040501)(2401047)(5005006)(8121501046)(10201501046)(3002001)(93006095)(93001095)(3231101)(2400082)(944501161)(6055026)(6041288)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123564045)(20161123560045)(20161123562045)(20161123558120)(6072148)(201708071742011); SRVR:DB5PR04MB1175; BCL:0; PCL:0; RULEID:; SRVR:DB5PR04MB1175; x-forefront-prvs: 0582641F53 received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: iRgGVZVHvbWiKFvD9yowC9TUUTWPR+ghstTzdGNqVivMy9J2ALS5ZErWsdSjIonJOX7m4f7uLDHVcO579qca+g== spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1547dd8b-8109-434d-30bb-08d572f5294d X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Feb 2018 15:19:22.7646 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5PR04MB1175 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:13:36 -0000 Content-Language: en-US Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable 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 impressio= n, > it > > > > > > seems to be used in order that people don't need to learn how b= oot > > > > > > 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 i= nclusion 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 =3D NULL > > > > > > in the error path of DownloadFile(). " Thanks, Meenakshi=20 > -----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 >=20 > Meenakshi, >=20 > The TFTP command is outside the UEFI Shell specification, therefore it is > included as a DynamicCommand, not a command built into the shell itself. >=20 > I a little confused by your last sentence. Do you want to send a new pat= ch? > or do you have a branch to pick changes from ? >=20 > -Jaben >=20 >=20 > > -----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 Leif's > > comments] > > > > > > Thanks, > > Meenakshi > > > > > -----Original Message----- > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf O= f > > > 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 > 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, 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 impressio= n, > it > > > > > > seems to be used in order that people don't need to learn how b= oot > > > > > > 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 t= his > > > > > useful. During development, this is useful to transfer generic fi= le 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 becau= se 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 = this > > > > comment? > > > > > > I am fine, if you prefer to remove this then we will develop some tes= t > > > application > > > for unit tests. > > > In case, we need to maintain this piece of code then above needs to f= ix > 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