From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::236; helo=mail-wr0-x236.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x236.google.com (mail-wr0-x236.google.com [IPv6:2a00:1450:400c:c0c::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2B88D2034AB28 for ; Wed, 8 Nov 2017 07:18:28 -0800 (PST) Received: by mail-wr0-x236.google.com with SMTP id 15so2755531wrb.5 for ; Wed, 08 Nov 2017 07:22:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=KSQW4Xf5VtSHNuGTiQjtMjMNVGIuEt5ZOTPS/kyg5mo=; b=e3OaysmxNhyiI6Xko+ytPd9v6obIYa2ZCTq8045taxZ9Oy1R8d9UHXEtTvwR553c49 k/Zyiz73tVkQBTggLwIKvGFHJvz5IDUlnoqNFxI1ztiLlT18WSTgZ7RmNXmVX3De0H9Z wwvI93pA4I7+lK6HiQviqn0rg3JtLFSDPwTsY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=KSQW4Xf5VtSHNuGTiQjtMjMNVGIuEt5ZOTPS/kyg5mo=; b=SgqtfcYOPqpV79p5ZEBXEQWwmH2V+1Cmm4Qf1oEn42FFQzm/Bal9tuvlBD1YBbk2Am lyb0qU/+QVz3fl21Xud5WXJLrokSoLEEN9aVo+CjD6SNl7yAmIaQ1RWgWV6HgbWZuogk Gr4N1xaGuch426y9RG4gt26Cckz3PqD/HbJ2TpSLXSGtn1gQOY/xr63V5mzCT2Iugr1z LcnINr+s0i7kKQ8cLl+bnQvcD2ZxcvWKtpCdI0wVJclXPbO/8CdZ5ymUcjqymjS4e7hv B+nBbXkw+m1TSo0KcGdm1xPnyoF5ah5pPWxUg3/c4pNDrkuck1/zB8DU5qDVgBUKYM6+ F9vQ== X-Gm-Message-State: AJaThX7x8jf0aiKLHrZLofBxajHPdgdriY9ZQFsHw9K6EmxAkS9lHYMH jgiY4BfQHjKx/4W4gvOQ6xkuKg== X-Google-Smtp-Source: ABhQp+SICMVrSIN9iDMEVO/2IxUu0bD+d93kAST+gSl4Y5WWCVf6X4XxxDVX4l2iMIGaOxKaqLYIfw== X-Received: by 10.223.153.100 with SMTP id x91mr763590wrb.189.1510154548278; Wed, 08 Nov 2017 07:22:28 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id x142sm4954659wme.34.2017.11.08.07.22.26 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 08 Nov 2017 07:22:26 -0800 (PST) Date: Wed, 8 Nov 2017 15:22:25 +0000 From: Leif Lindholm 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" Message-ID: <20171108152225.gy6te7rcroqqjr3j@bivouac.eciton.net> References: <1509719192-16582-1-git-send-email-vabhav.sharma@nxp.com> <20171107175458.f4pgne4rkri7lzch@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] Tftp assert fix for openfile failure case X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Nov 2017 15:18:29 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 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? / Leif