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

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

On 07/23/20 16:54, Laszlo Ersek wrote:
> On 07/13/20 20:31, 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.
>>
>> Signed-off-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
>> Tested-By: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.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>
>> ---
>>  .../DynamicCommand/HttpDynamicCommand/Http.c  | 1700 +++++++++++++++++
>>  .../DynamicCommand/HttpDynamicCommand/Http.h  |   84 +
>>  .../HttpDynamicCommand/Http.uni               |  113 ++
>>  .../HttpDynamicCommand/HttpApp.c              |   53 +
>>  .../HttpDynamicCommand/HttpApp.inf            |   58 +
>>  .../HttpDynamicCommand/HttpDynamicCommand.c   |  134 ++
>>  .../HttpDynamicCommand/HttpDynamicCommand.inf |   63 +
>>  ShellPkg/Include/Guid/ShellLibHiiGuid.h       |    5 +
>>  ShellPkg/ShellPkg.dec                         |    1 +
>>  ShellPkg/ShellPkg.dsc                         |    5 +
>>  10 files changed, 2216 insertions(+)
>>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.c
>>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.h
>>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/Http.uni
>>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.c
>>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpApp.inf
>>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.c
>>  create mode 100644 ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
> 
> I tested this, in an OVMF IA32X64 VM, and in an ArmVirtQemu (AARCH64) VM.
> 
> I tested HTTP download, HTTPS download, and also willfully triggered a
> Volume Full failure (checked free space on the FS with "vol" first, then
> attempted to download a larger file than that). Also attempted a
> download to read-only media (refused correctly with "Write Protected").
> 
> I tested FQDNs and IPv4 addresses in the URLs.
> 
> I used "wget-style" URLs, mainly.
> 
> For setting the CA certificates, I used the method described in
> OvmfPkg/README, in section "HTTPS Boot".
> 
> Compared the local files downloaded by this command vs. local files
> downloaded from the same URLs using other tools (browser, wget etc). I
> used a few hundred KB large files for this.
> 
> One suggestion: if the download fails for some reason, but the local
> file creation succeeded, then upon exit, the local file should be
> deleted. Otherwise an incomplete (possibly zero size) file is left in
> the filesystem.
> 
> Tested-by: Laszlo Ersek <lersek@redhat.com>

Another comment relating to usage. (This is by no means an observation
for the code, at best it could be included in some developer
documentation or in some wiki page.)

Having access to the DebugLib outputs of the drivers that provide the
underlying protocols for this shell command is very useful. For example
those logs can report certificate validation issues.

Such access is usually not difficult with virtual machines, but on
physical machines, it could be more challenging. I guess one way around
that is to use such DebugLib instances in the platform firmware (i.e. in
the underlying protocol drivers) that write to the UEFI consoles. That
tends to mess up the display in the shell window, but it does provide
more information.

Thanks
Laszlo