Re: [edk2-devel] [PATCH v5 1/1] ShellPkg/DynamicCommand: add HttpDynamicCommand

["Vladimir Olovyannikov" <vladimir.olovyannikov@broadcom.com>]

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.olovyannikov@broadcom.com
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Zhichao
> Gao <zhichao.gao@intel.com>; Maciej Rabeda
> <maciej.rabeda@linux.intel.com>; Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan
> Fu <siyuan.fu@intel.com>; Ray Ni <ray.ni@intel.com>; Liming Gao
> <liming.gao@intel.com>; Nd <nd@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.olovyannikov@broadcom.com>
> > CC: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> > CC: Laszlo Ersek <lersek@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
>
>
>