From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f50.google.com (mail-ej1-f50.google.com [209.85.218.50]) by mx.groups.io with SMTP id smtpd.web10.53637.1600058263331062775 for ; Sun, 13 Sep 2020 21:37:44 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@broadcom.com header.s=google header.b=IOaiIIDl; 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.218.50, mailfrom: vladimir.olovyannikov@broadcom.com) Received: by mail-ej1-f50.google.com with SMTP id r7so21184976ejs.11 for ; Sun, 13 Sep 2020 21:37:43 -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=JGlmWFhb4L76J9QoTmKkqxrWhOlZptCH4NQEqqjCKa4=; b=IOaiIIDlGF754/zAKnKdZhjuZoz7KX0VvEQkcMF0fjTmujnv3gx2rIrpyqI1S9h0gP SBALgl1+UoqHxdctvWOKO/QnuPW8bRH9v0Am+bZb8Z9bK7Y5JQFrShGgZelC75N/fnTh 2GKfl8PSjhcHqiXVDJi6UmWKrH1zO8aGggg7s= 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=JGlmWFhb4L76J9QoTmKkqxrWhOlZptCH4NQEqqjCKa4=; b=cHrOVK3bGgW6Gi0OML7KhieXRAZYNG8RpZPRaXwlTFcKoaz92oWuaJpoCKWquNUI3w HJkddzfmymx0rJDZ178ijTJQT80ur6xB0bAtgc5hlodCXDhAms++x2fKQh3sftihropF jkPRxGxxW+7IBKKxA6wOKZUBz4MCwzHzP4Xm/oZIU403cEMvH0S+29yiCJCgRgPnlB4E 6Vsy+uLZJRdXbr1ytnnNTBKHQszCCxAhQS2mU2fdNZlgxGZURcqDWWN3e8BgdWfJf00u SEsWmeRL1hcJ1D5MtFyOk0fOxJysgdsTuGco9jNKEVkS+tOS6npufJ7EFuCXJwZHBtJh hRwg== X-Gm-Message-State: AOAM532TBJwLpQXuHfHTYsBbAZfQWv45sDGP+Xd+FqxlsAut5esoqZLf lz7zohZkg9pN/Nrzy3CyMtkXpfbqqlGnuc66kHa8PA== X-Google-Smtp-Source: ABdhPJwHr/fhGTBVwfTwBecKD4fbI6JI7qJR+xw5xrJc1xJz46pdajtTli22s9F6Y1ewtUQqljbrVTideVfu4PegMEg= X-Received: by 2002:a17:906:c1c6:: with SMTP id bw6mr13355679ejb.374.1600058261622; Sun, 13 Sep 2020 21:37:41 -0700 (PDT) From: "Vladimir Olovyannikov" References: <20200909184904.11129-1-vladimir.olovyannikov@broadcom.com> In-Reply-To: MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQHg3JNJWCcY2YJbD7AEaGyhEZCY1wHe2KTlAVY+060CMMVIwwEKIAwzAVNwfG+pFMtuEA== Date: Sun, 13 Sep 2020 21:37:41 -0700 Message-ID: <066d9426ea1b5f9eb025ed50ee41ab1d@mail.gmail.com> 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, 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 found i= t 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 ca= n > 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 th= e > 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? 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