Hi Laszlo, Thank you for testing! > -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Thursday, July 23, 2020 8:03 AM > To: devel@edk2.groups.io; vladimir.olovyanni...@broadcom.com > Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>; Zhichao > Gao <zhichao....@intel.com>; Maciej Rabeda > <maciej.rab...@linux.intel.com>; Jiaxin Wu <jiaxin...@intel.com>; Siyuan > Fu <siyuan...@intel.com>; Ray Ni <ray...@intel.com>; Liming Gao > <liming....@intel.com>; Nd <n...@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.olovyanni...@broadcom.com> > >> Tested-By: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com> > >> Cc: Zhichao Gao <zhichao....@intel.com> > >> Cc: Maciej Rabeda <maciej.rab...@linux.intel.com> > >> Cc: Jiaxin Wu <jiaxin...@intel.com> > >> Cc: Siyuan Fu <siyuan...@intel.com> > >> Cc: Ray Ni <ray...@intel.com> > >> Cc: Liming Gao <liming....@intel.com> > >> Cc: Nd <n...@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 <ler...@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
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#63212): https://edk2.groups.io/g/devel/message/63212 Mute This Topic: https://groups.io/mt/75483573/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
