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

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

Hi Zhichao,

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Monday, September 14, 2020 1:19 AM
> To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>; Laszlo
> Ersek <lersek@redhat.com>; devel@edk2.groups.io
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Gao, Liming <liming.gao@intel.com>; Nd
> <nd@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>
> Subject: RE: [PATCH v11 0/1] ShellPkg/DynamicCommand: add
> HttpDynamicCommand
>
> Hi Vladimir,
>
> > -----Original Message-----
> > From: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> > Sent: Monday, September 14, 2020 12:38 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; devel@edk2.groups.io
> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Wu, Jiaxin
> > <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Gao, Liming <liming.gao@intel.com>; Nd
> > <nd@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>
> > Subject: RE: [PATCH v11 0/1] ShellPkg/DynamicCommand: add
> > HttpDynamicCommand
> >
> > Hi Zhichao,
> > Thank you for reviewing.
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > Sent: Sunday, September 13, 2020 5:52 PM
> > > To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
> > > Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> > > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Wu, Jiaxin
> > > <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ni, Ray
> > > <ray.ni@intel.com>; Gao, Liming <liming.gao@intel.com>; Nd
> > > <nd@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>
> > > Subject: RE: [PATCH v11 0/1] ShellPkg/DynamicCommand: add
> > > HttpDynamicCommand
> > >
> > > Hi Vladimir/Laszlo,
> > >
> > > Sorry for the late response. Recently, I am busy with other works
> > > for recent weeks. So I cannot spend much time on EDK2 open source.
> > > Apologize for the inconvenient.
> > >
> > > I didn’t give the comments on the time function because I found it
> > > is copied from EmbeddedPkg's TimeBaseLib. And I assumes it works
> > > fine without any issue as it has been in the trunk for a long time.
> > > But actually it cannot pass the MS VS X64 build. The lib was not
> > > added in the package dsc file so the build error was not found
> > > before. I hope we can directly use the TimeBaseLib instead of just
> > > use its header file and keep the duplicated code. This can be a
> > > future fix/optimization.
> > Yes, this initially was the intention, but x64 build of
> > ShellPkg/HttpDynamicCommand failed, so I switched to the hybrid: Use
> > constants from TimeBaseLib, and duplicate the function in the
> > HttpDynamicCommand itself.
> > >
> > > Other code doesn't change the logic since V9. So I have no comments
> > > on the implementation except the new time function. With the time
> > > function issue fixed, I am glad to give the R-B and help to merge the
> patch.
> > Can you please let me know what the issue is? The return now
> > corresponds to TimeBaseLib return values.
> > TimeBaseLib library itself needs to be fixed to return proper type of
> > EfiTimeToEpoch.
> > Am I missing anything?
>
> No. I think you can fix the time related code as Laszlo's suggestion. The
> optimization can be done in the future.
> But my point is, we should record the build error in the EmbeddedPkg
> BaseTimeLib. With that fixed, this driver can be optimized to remove the
> duplicated code. I have file a BZ for it:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2962. I would fix it when I
> am free.
OK, thanks, sounds good. This is basically what I had with PATCH v10.
I will submit PATCH v12 with no using of TimeBaseLib.h until it is fixed.
Then, when TimeBaseLib is fixed, we'll start using TimeBaseLib constants and
functions instead of duplicates.

Thank you,
Vladimir
>
> Thanks,
> Zhichao
>
> >
> > Thank you,
> > Vladimir
> > >
> > > Thanks,
> > > Zhichao
> > >
> > > > -----Original Message-----
> > > > From: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> > > > Sent: Saturday, September 12, 2020 1:04 AM
> > > > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> > > > Cc: Gao, Zhichao <zhichao.gao@intel.com>; Maciej Rabeda
> > > > <maciej.rabeda@linux.intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>;
> > > > Fu, Siyuan <siyuan.fu@intel.com>; Ni, Ray <ray.ni@intel.com>; Gao,
> > > > Liming <liming.gao@intel.com>; Nd <nd@arm.com>; Samer
> > > > El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> > > > Subject: RE: [PATCH v11 0/1] ShellPkg/DynamicCommand: add
> > > > HttpDynamicCommand
> > > >
> > > > > -----Original Message-----
> > > > > From: Laszlo Ersek <lersek@redhat.com>
> > > > > Sent: Friday, September 11, 2020 12:20 AM
> > > > > To: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>;
> > > > > devel@edk2.groups.io
> > > > > Cc: 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>; Samer El-Haj-
> > > Mahmoud
> > > > > <Samer.El-Haj-Mahmoud@arm.com>
> > > > > Subject: Re: [PATCH v11 0/1] ShellPkg/DynamicCommand: add
> > > > > HttpDynamicCommand
> > > > >
> > > > > On 09/10/20 22:33, Vladimir Olovyannikov wrote:
> > > > > > Hi Laszlo,
> > > > > >
> > > > > >> -----Original Message-----
> > > > > >> From: Laszlo Ersek <lersek@redhat.com>
> > > > > >> Sent: Wednesday, September 9, 2020 11:33 PM
> > > > >
> > > > > >>> 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.
> > > > >
> > > > > > Sorry, I must have misunderstood. Do you want me to resubmit
> > > > > > the patch? I am open to ideas.
> > > > >
> > > > > Ideally:
> > > > >
> > > > > - change the return type of EfiTimeToEpoch() to UNITN
> > > > >
> > > > > - drop the final UINT32 cast from EfiTimeToEpoch()
> > > > >
> > > > > - change the type of ElapsedSeconds to UINTN
> > > > >
> > > > > - change the expression
> > > > >
> > > > >     ElapsedSeconds > 1 ? ElapsedSeconds : 1
> > > > >
> > > > >   to
> > > > >
> > > > >     ElapsedSeconds > 1 ? (UINT64)ElapsedSeconds : 1
> > > > >
> > > > > - print the expression mentioned above with the format specifier
> > > > > %Lu
> > > > I see. Basically, it is PATCH v10. I just wanted to reuse
> > > > TimeBaseLib.h constants in PATCH v11.
> > > >
> > > > >
> > > > > *BUT*. These are really just small details. It would be OK to
> > > > > fix these up later, incrementally. Where I see a real problem is
> > > > > the lack of timely feedback from the ShellPkg maintainers.
> > > > Agreed. Hopefully, it can be reviewed sometime soon.
> > > >
> > > > Thank you,
> > > > Vladimir
> > > > >
> > > > > Laszlo