From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.51392.1595839180582102076 for ; Mon, 27 Jul 2020 01:39:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=beuJM/yF; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1595839179; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=C6IUGde5vHxT37OtEvKxdWDOYDGkp6Ocg5mERZEyxAM=; b=beuJM/yFn7gbZw5G+UnWsn2hx2q5fJftyntHYIDvMyTkb+U/HXxSXRBZFQ9XqDlJZBbdmZ kYWX4utQYC4w3Xrgu4mWb73eZpkYdyL7dacH2C/Czi5zZ0Gz8dKx3QuHvFqDsC+4lUoToU y22Nicsi0eBmo+JqhScnMFXBRc+RwTk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-230-ap0KcPPAOW6W3d4uRLdB-Q-1; Mon, 27 Jul 2020 04:39:32 -0400 X-MC-Unique: ap0KcPPAOW6W3d4uRLdB-Q-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CFE16800473; Mon, 27 Jul 2020 08:39:30 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-235.ams2.redhat.com [10.36.113.235]) by smtp.corp.redhat.com (Postfix) with ESMTP id 308B3610F3; Mon, 27 Jul 2020 08:39:24 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 0/1] ShellPkg/DynamicCommand: add HttpDynamicCommand To: vladimir.olovyannikov@broadcom.com, Zhichao Gao , Ray Ni Cc: devel@edk2.groups.io, Samer El-Haj-Mahmoud , Maciej Rabeda , Jiaxin Wu , Siyuan Fu , Liming Gao , Nd References: <20200723205052.22500-1-vladimir.olovyannikov@broadcom.com> From: "Laszlo Ersek" Message-ID: <220cd1ed-a856-5a20-fdf8-8a12d1b9ca69@redhat.com> Date: Mon, 27 Jul 2020 10:39:20 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200723205052.22500-1-vladimir.olovyannikov@broadcom.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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. (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. (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). ... 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/HttpDynamicCommand.inf] [X64] > consumed by module [ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.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-Format > 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".) 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). 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. 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