Hi Laszlo, Thank you for the comments. I agree with them, and as you suggest, I will address them along with comments from maintainers.
Thank you, Vladimir > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo > Ersek > Sent: Monday, July 27, 2020 3:08 PM > To: devel@edk2.groups.io; vladimir.olovyanni...@broadcom.com > Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>; Zhichao > Gao <zhichao....@intel.com>; Maciej Rabeda > <maciej.rab...@linux.intel.com>; Jiaxin Wu <jiaxin...@intel.com>; Siyuan > Fu <siyuan...@intel.com>; Ray Ni <ray...@intel.com>; Liming Gao > <liming....@intel.com>; Nd <n...@arm.com> > Subject: Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add > HttpDynamicCommand > > Just some quick remarks after a comparison with v3: > > On 07/27/20 18:48, Vladimir Olovyannikov via groups.io wrote: > > Introduce an http client utilizing EDK2 HTTP protocol, to allow fast > > image downloading from http/https servers. > > HTTP download speed is usually faster than tftp. > > The client is based on the same approach as tftp dynamic command, and > > uses the same UEFI Shell command line parameters. This makes it easy > > integrating http into existing UEFI Shell scripts. > > Note that to enable HTTP download, feature Pcd > > gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections must be set to > > TRUE. > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860 > > > > Signed-off-by: Vladimir Olovyannikov > > <vladimir.olovyanni...@broadcom.com> > > CC: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com> > > CC: Laszlo Ersek <ler...@redhat.com> > > (1) These "CC" lines are not formatted correctly -- they might do the job > as > far as git-send-email is concerned, but they don't satisfy > "PatchCheck.py": > > > ShellPkg/DynamicCommand: add HttpDynamicCommand The commit > message > > format is not valid: > > * 'CC' should be 'Cc' > > * 'CC' should be 'Cc' > > https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-F > > ormat > > The exit status is 255, so this would break the CI run again. > > Please run "PatchCheck.py" locally before posting, and/or submit a > personal > CI build. > > [...] > > > + if (UserNicName != NULL) { > > + if (StrCmp (NicName, UserNicName) != 0) { > > + Status = EFI_NOT_FOUND; > > Change is new since v4, but not documented in the v5 changelog. > > (The code may be OK, but please help reviewers with the v(n) -> v(n+1) > changelogs.) > > [...] > > > + if (ShellCommandLineGetFlag (CheckPackage, L"-m")) { > > + Context.Flags |= DL_FLAG_TIME; > > + } > > (2) The "-m" flag has not been removed from here > > [...] > > > +// Download Flags > > +#define DL_FLAG_TIME BIT0 // Show elapsed time. > > (3) and here > > > +".SH SYNOPSIS\r\n" > > +" \r\n" > > +"HTTP [-i interface] [-l port] [-t timeout] [-s size] [-m] [-k]\r\n" > > (4) and here > > [...] > > > +" -m Measure and report download time (in seconds). > > \r\n" > > (5) and here. > > I suggest waiting for ShellPkg owner feedback before posting v6. > > Thanks! > Laszlo > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#63704): https://edk2.groups.io/g/devel/message/63704 Mute This Topic: https://groups.io/mt/75826968/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
