Re: [PATCH v11 0/1] ShellPkg/DynamicCommand: add HttpDynamicCommand

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

On 09/09/20 20:49, Vladimir Olovyannikov wrote:
> Signed-off-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Nd <nd@arm.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> 
> This patchset introduces 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
> 
> PATCH v11 changes:
> Address comments from Laszlo:
>   - use TimeBaseLib.h header to get rid of duplicated constants;
>   - explicitly return UINT32 in EfiTimeToEpoch().

to be clear, I explicitly *disagree* with returning UINT32 from
EfiTimeToEpoch().

I'm not "demanding" (or even suggesting) that you update the
EfiTimeToEpoch() implementation in this patch to return UINTN, but I'd
like to be very clear that, IMO, for EfiTimeToEpoch() to suffer from a
year 2106 problem on 64-bit systems too, is bad design. So please don't
list the UINT32 return type as my suggestion -- that's the exact
opposite of what I'd actually suggest.

Zhichao: are you ready to merge this patch? If so, please let me know;
I'll test it then.

Thanks
Laszlo

> 
> 
> Vladimir Olovyannikov (1):
>   ShellPkg/DynamicCommand: add HttpDynamicCommand
> 
>  ShellPkg/ShellPkg.dec                         |    1 +
>  ShellPkg/ShellPkg.dsc                         |    5 +
>  .../HttpDynamicCommand/HttpApp.inf            |   58 +
>  .../HttpDynamicCommand/HttpDynamicCommand.inf |   63 +
>  .../DynamicCommand/HttpDynamicCommand/Http.h  |   91 +
>  ShellPkg/Include/Guid/ShellLibHiiGuid.h       |    5 +
>  .../DynamicCommand/HttpDynamicCommand/Http.c  | 1823 +++++++++++++++++
>  .../HttpDynamicCommand/HttpApp.c              |   61 +
>  .../HttpDynamicCommand/HttpDynamicCommand.c   |  137 ++
>  .../HttpDynamicCommand/Http.uni               |  117 ++
>  10 files changed, 2361 insertions(+)
>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf
>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.h
>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.c
>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.c
>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.uni
>