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

["Laszlo Ersek" <lersek@redhat.com>]

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-Format

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