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

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

Hi Laszlo,
Thank you for testing!

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, July 23, 2020 8:03 AM
> 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 v3 1/1] ShellPkg/DynamicCommand: add
> HttpDynamicCommand
>
> 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.
I was thinking about this.
Sometimes, an error file returned by HTTP server (say, on 404 or 503) has
more information than just 404, 503, etc.
That was the only reason I do not delete it.
I still return error code to the caller, so the caller can read it (maybe,
to provide more information to the user), and/or delete it right away.
What do you think?
> >
> > 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.
I agree. Note that usually those libs have DEBUG(()) messages which are
compiled out in RELEASE builds.
>
> Thanks
> Laszlo