From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f50.google.com (mail-ed1-f50.google.com [209.85.208.50]) by mx.groups.io with SMTP id smtpd.web12.67949.1600109646431405223 for ; Mon, 14 Sep 2020 11:54:07 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@broadcom.com header.s=google header.b=g6Y9IRug; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: broadcom.com, ip: 209.85.208.50, mailfrom: vladimir.olovyannikov@broadcom.com) Received: by mail-ed1-f50.google.com with SMTP id ay8so609309edb.8 for ; Mon, 14 Sep 2020 11:54:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=from:references:in-reply-to:mime-version:thread-index:date :message-id:subject:to:cc:content-transfer-encoding; bh=0iRuVeLcAXCKbtmKdQ06vygnrcV3w72KZr/oDHia82A=; b=g6Y9IRugYuv6G3B3db9KMwg4UivsDdFZiQ6Ka1xtkK7mWizJ0laUrExRWyEQsVoIpF iEjx4pyIrF0hWJquqFEVj7DE4Kwf8osJGgzpO+MWloyP3eMsv3ws5cBu0cRNy3rshmv6 BTg2UlQf6WvjHS8YOyk6UUDGT3DigLAFBYYGc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:references:in-reply-to:mime-version :thread-index:date:message-id:subject:to:cc :content-transfer-encoding; bh=0iRuVeLcAXCKbtmKdQ06vygnrcV3w72KZr/oDHia82A=; b=re73EYLVOr+xSY3V1MfzMFz0zki5qcu4JqVRWXdV1bikc9HhhoaoghobE9lWOPyz7e /+E9czRHR2tVW5M69MTiOaHjVBRt+vPWX3OIegmA4NnuqrpsgOjHh45Xyu1tzDmAbwxP 3kt65a+kwi7f1PUStYbqaiDGyqwOSr4pv0RJv14Crb2xEjOtv/IYbGam0t40gFYBnRp+ nLESuDfxs31mpQ7t2rGxRJuMUL+UFfxhQ2dYQ664j2aQb2KZ1EII0sx8Nn06HmRw2nnI i0/s9zikZWB2sm3kNhRbr3xCO7o59Cz/fwZSvh/Wg7XPudSLMUXB5++LzXN0Ji7Tl7py VhHA== X-Gm-Message-State: AOAM5316aGljEb+7tLelYuw9Ij/TmWMfJCO6iN+tOb08So5ORlyJDyIq sVzRiPlUxcj8MBTQfqjztmPGzaq3YW7zuPuDiSAdcA== X-Google-Smtp-Source: ABdhPJzJ1wl5h54dIxL/YAXYKT+mLIc6RD0RWfbK73JP+ziYGBRMPxv605xszLgrL4zhnz0GYtTegJRT45JZt8Zeag4= X-Received: by 2002:aa7:d785:: with SMTP id s5mr18210482edq.154.1600109644639; Mon, 14 Sep 2020 11:54:04 -0700 (PDT) From: "Vladimir Olovyannikov" References: <20200909184904.11129-1-vladimir.olovyannikov@broadcom.com> <066d9426ea1b5f9eb025ed50ee41ab1d@mail.gmail.com> In-Reply-To: MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQHg3JNJWCcY2YJbD7AEaGyhEZCY1wHe2KTlAVY+060CMMVIwwEKIAwzAVNwfG8Bk94N3AJjE8RYqPYD+ZA= Date: Mon, 14 Sep 2020 11:54:02 -0700 Message-ID: Subject: Re: [PATCH v11 0/1] ShellPkg/DynamicCommand: add HttpDynamicCommand To: "Gao, Zhichao" , Laszlo Ersek , devel@edk2.groups.io Cc: Maciej Rabeda , "Wu, Jiaxin" , "Fu, Siyuan" , "Ni, Ray" , "Gao, Liming" , Nd , Samer El-Haj-Mahmoud Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Zhichao, > -----Original Message----- > From: Gao, Zhichao > Sent: Monday, September 14, 2020 1:19 AM > To: Vladimir Olovyannikov ; Laszlo > Ersek ; devel@edk2.groups.io > Cc: Maciej Rabeda ; Wu, Jiaxin > ; Fu, Siyuan ; Ni, Ray > ; Gao, Liming ; Nd > ; Samer El-Haj-Mahmoud Mahmoud@arm.com> > Subject: RE: [PATCH v11 0/1] ShellPkg/DynamicCommand: add > HttpDynamicCommand > > Hi Vladimir, > > > -----Original Message----- > > From: Vladimir Olovyannikov > > Sent: Monday, September 14, 2020 12:38 PM > > To: Gao, Zhichao ; Laszlo Ersek > > ; devel@edk2.groups.io > > Cc: Maciej Rabeda ; Wu, Jiaxin > > ; Fu, Siyuan ; Ni, Ray > > ; Gao, Liming ; Nd > > ; Samer El-Haj-Mahmoud Mahmoud@arm.com> > > Subject: RE: [PATCH v11 0/1] ShellPkg/DynamicCommand: add > > HttpDynamicCommand > > > > Hi Zhichao, > > Thank you for reviewing. > > > > > -----Original Message----- > > > From: Gao, Zhichao > > > Sent: Sunday, September 13, 2020 5:52 PM > > > To: Vladimir Olovyannikov ; > > > Laszlo Ersek ; devel@edk2.groups.io > > > Cc: Maciej Rabeda ; Wu, Jiaxin > > > ; Fu, Siyuan ; Ni, Ray > > > ; Gao, Liming ; Nd > > > ; Samer El-Haj-Mahmoud 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=E2=80=99t give the comments on the time function because I fou= nd 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=3D2962. I would fix it whe= n 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 an= d functions instead of duplicates. Thank you, Vladimir > > Thanks, > Zhichao > > > > > Thank you, > > Vladimir > > > > > > Thanks, > > > Zhichao > > > > > > > -----Original Message----- > > > > From: Vladimir Olovyannikov > > > > Sent: Saturday, September 12, 2020 1:04 AM > > > > To: Laszlo Ersek ; devel@edk2.groups.io > > > > Cc: Gao, Zhichao ; Maciej Rabeda > > > > ; Wu, Jiaxin ; > > > > Fu, Siyuan ; Ni, Ray ; Gao, > > > > Liming ; Nd ; Samer > > > > El-Haj-Mahmoud > > > > Subject: RE: [PATCH v11 0/1] ShellPkg/DynamicCommand: add > > > > HttpDynamicCommand > > > > > > > > > -----Original Message----- > > > > > From: Laszlo Ersek > > > > > Sent: Friday, September 11, 2020 12:20 AM > > > > > To: Vladimir Olovyannikov ; > > > > > devel@edk2.groups.io > > > > > Cc: Zhichao Gao ; Maciej Rabeda > > > > > ; Jiaxin Wu > > > > > ; Siyuan Fu ; Ray Ni > > > > > ; Liming Gao ; Nd > > > > > ; Samer El-Haj- > > > Mahmoud > > > > > > > > > > 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 > > > > > >> 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