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

["Gao, Zhichao" <zhichao.gao@intel.com>]

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.

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