From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) by mx.groups.io with SMTP id smtpd.web12.58188.1595863924085100875 for ; Mon, 27 Jul 2020 08:32:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@broadcom.com header.s=google header.b=bvFiFElW; 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.167.50, mailfrom: vladimir.olovyannikov@broadcom.com) Received: by mail-lf1-f50.google.com with SMTP id d2so3609622lfj.1 for ; Mon, 27 Jul 2020 08:32:03 -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; bh=2lOdalw5LONuIesVtH1umZE1z4pvLWnb7yQ2s+hYYOA=; b=bvFiFElWNOqAv4kguBxy/QMiZ/hMdw7aV+ArU+URiNCdUFYypIZNrgGHuZ0MywmQsc 9pS8U6LhwN79hZoQbaKxlJ8wZJyi14dHL+8VXRaEPvPedBbgPr2p/UB4aHySgBhj9+oD OkyxyQkH2JNaYwQWu68Aesg/UHpnaI5msgrA4= 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; bh=2lOdalw5LONuIesVtH1umZE1z4pvLWnb7yQ2s+hYYOA=; b=R7vEUGRLcK5uGObIMSv4kL/DKwacIYgS2Z2Q6HDQvjGwszCsod6MABcR+4kLgZPapH bZSkH7lmnhZotzAXSvJ74VYigUEdkzocxM5OPeT0satEiu8RVCH3dRi7HpjmN/YWmrKD meltHwtn8r83IzhWelkhH9xkoNNQc2oLD+TKehuv94QxemZucJNeGrQMQ3AmYVHDwZ8c HMfaFByJ77OoVks97Y/b4/+U4/x5Nvi1ge9488Y4KLZ53XDPXHupJ9JLqj7sXW20jOf5 GT4Kf5ycFKAyibcmgUtKoJMymOqnyDcO1Jx6VHb+pzm0p9FWOi1jIoD4yvAL0w0BZ7YS j9Lw== X-Gm-Message-State: AOAM5337qQoq3WGcYMpOI9e6MlrfY5vdg4rkyheaNSJnC9S2poQQA6mY cMzwGw/zpkfsL2yvWwqpqRMuARr8jc0WRxaq5F+TgA== X-Google-Smtp-Source: ABdhPJy0+UC+tB5iCLpA1LaTWFhl5909dUohC2emCIYolSUku2KaKBUZ4ad0kf/wwLutnPCaURlaA0UrTUuwZFoJeg4= X-Received: by 2002:a05:6512:2082:: with SMTP id t2mr12473154lfr.142.1595863922070; Mon, 27 Jul 2020 08:32:02 -0700 (PDT) From: "Vladimir Olovyannikov" References: <20200723205052.22500-1-vladimir.olovyannikov@broadcom.com> <220cd1ed-a856-5a20-fdf8-8a12d1b9ca69@redhat.com> In-Reply-To: <220cd1ed-a856-5a20-fdf8-8a12d1b9ca69@redhat.com> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQJP+AdnqzowNaUZkbApSdGA8Og7kgHSj4ApqBnQPCA= Date: Mon, 27 Jul 2020 08:31:59 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH v4 0/1] ShellPkg/DynamicCommand: add HttpDynamicCommand To: Laszlo Ersek , Zhichao Gao , Ray Ni Cc: devel@edk2.groups.io, Samer El-Haj-Mahmoud , Maciej Rabeda , Jiaxin Wu , Siyuan Fu , Liming Gao , Nd Content-Type: text/plain; charset="UTF-8" Hi Laszlo, Thank you for valuable comments, I did not realize TimerLib should not be used at all for ShellPkg. I will remove -m option for now and then will replace with gRT time functions. > -----Original Message----- > From: Laszlo Ersek > Sent: Monday, July 27, 2020 1:39 AM > To: vladimir.olovyannikov@broadcom.com; Zhichao Gao > ; Ray Ni > Cc: devel@edk2.groups.io; Samer El-Haj-Mahmoud Mahmoud@arm.com>; Maciej Rabeda ; > Jiaxin Wu ; Siyuan Fu ; Liming > Gao ; Nd > Subject: Re: [edk2-devel] [PATCH v4 0/1] ShellPkg/DynamicCommand: add > HttpDynamicCommand > > Hi Vladimir, > > On 07/23/20 22:50, Vladimir Olovyannikov via groups.io wrote: > > Signed-off-by: Vladimir Olovyannikov > > > > Tested-By: Samer El-Haj-Mahmoud > > Tested-By: Laszlo Ersek > > Cc: Zhichao Gao > > Cc: Maciej Rabeda > > Cc: Jiaxin Wu > > Cc: Siyuan Fu > > Cc: Ray Ni > > Cc: Liming Gao > > Cc: Nd > > > > This patchset introduces 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. > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2860 > > > > PATCH v4 changes: > > Address comments based on Laszlo's testing: > > - Fix .uni help file missing "\r\n" before RETURNVALUES section; > > - delete the downloaded file in case of an error, unless > > -k ("keep bad") option is provided on command line. > > > > Allow download time measurement in seconds by providing -m parameter > > on command line. > > (1) "Tested-by" tags cannot be carried forward on a patch if there are any > changes to the patch. If you update a patch and people would like to have > their T-b's on the patch, then they'll have to retest the patch. So every > time > you update a patch, please drop the previously given Tested-by tags from > it. > > OK, > (2) I was about to retest this patch, but I also compared it with the > previous > version (v3). I think the "-m" option should not be added, for now anyway. > For two reasons: > > - The patch is already large, and I think there has been no ShellPkg > reviewer / maintainer feedback so far. I think we shouldn't make the > patch even larger, with new features. "-m" looks like an addition that > can be done separately, once the core feature is upstream. OK, makes sense. > > (I consider "-k" a bugfix on the other hand. More precisely, I > consider the new behavior *without "-k"* a bugfix. So I certainly > welcome that.) > > - The other reason is that the "-m" feature seems to introduce a > TimerLib dependency. Depending on TimerLib in a shell application is > wrong, IMO; in particular because this application / dynamic command > is extremely useful, so some people might easily want to download a > pre-built binary, and run it on their system for whatever purposes. > But TimerLib is platform-dependent, and that would break this binary > portability. > > For some more background, please refer to commit 7a141b1306f6 > ("ShellPkg: remove superfluous TimerLib resolution", 2018-02-13). I read it, thanks. Now I understand the complications. > > > ... In fact, upon re-reading the above commit, I'm realizing the current > patch > could break the "ShellPkg.dsc" build, because the patch > (incorrectly) introduces a TimerLib dependency in a ShellPkg module, but > "ShellPkg.dsc" (correctly) does not resolve the TimerLib class to any > instance. > > And it does break: > > $ build -a X64 -b NOOPT -t GCC48 -p ShellPkg/ShellPkg.dsc > > > ShellPkg/ShellPkg.dsc(...): error 4000: Instance of library class > > [TimerLib] is > not found > > in > [ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicComman > d.inf] [X64] > > consumed by module > > > [ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicComman > d.inf] > > One consequence of the above is that this patch would not pass a CI build. > > > ... Another reason this patch would not pass a CI build is a > "PatchCheck.py" > failure: > > > ShellPkg/DynamicCommand: add HttpDynamicCommand The commit > message > > format is not valid: > > * 'Tested-By' should be 'Tested-by' > > * 'Tested-By' should be 'Tested-by' > > https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-F > > ormat > > The code passed all checks. > > (Side comment: please use the clipboard for cutting and pasting feedback > tags from emails to commit messages. I have a keyboard shortcut for > inserting "Tested-by: Laszlo Ersek ", and so I know for > a fact that I never send an upper-case "By".) OK, will do. > > I suggest running a personal CI build on github.com before submitting the > patch to the list (just open a pull request -- if it fails, you'll get the > error > reports, and if it succeeds, then the mergify bot will auto-close the > successful > personal build, without actually merging the patch). Thanks, will do that. > > > In summary, I suggest posting v5: > > - with the Tested-by flags dropped (Samer and myself will have to > re-test), > > - with the "-k" option preserved, but the "-m" option (and the > associated TimerLib dependency) removed. > > Later on, I think "-m" could be added based on gRT->GetTime(), instead of > TimerLib. Sure, totally makes sense. > > > Ray, Zhichao, when do you intend to review this patch? It does not make > much sense for Samer and myself to keep testing this patch if you're going > to > start the review only later. The review feedback will probably necessitate > updates to the patch, which will invalidate Samer's testing and mine. > We've > done a few rounds of testing; the patch certainly deserves package > maintainer review. Once no more changes looked necessary from the > reviewer side, I'd be happy to test the patch again. > > Thanks > Laszlo Thank you, Vladimir