From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web10.137.1571786176749333281 for ; Tue, 22 Oct 2019 16:16:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=OxSOhCmk; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1571786175; 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=pp5l7Mvggm5d3ccv9qTvGtwmI6Ush37rCWUUa8OHXl8=; b=OxSOhCmkdQvH3kiEni7YLw4PibIKmKCH4lTrgq+mIlvef7g7//qVm+u9duBga7o97jMvKP qMV4xMGxW4c5JD0tT6Hx0N01+SU+GslcslTzASGigguVKIpijcoOQByJjokqQW0jyDopNQ reY+f8g+pw8xGIkepZlgbM5WXVms4c4= 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-360-trnw1j8KNrm5lth1224Zdg-1; Tue, 22 Oct 2019 19:16:12 -0400 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 40EB31005509; Tue, 22 Oct 2019 23:16:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.54]) by smtp.corp.redhat.com (Postfix) with ESMTP id 300D460619; Tue, 22 Oct 2019 23:16:07 +0000 (UTC) Subject: Re: [edk2-devel] [Patch v2 1/2] SecurityPkg: Replace EFI_D_* with DEBUG_* To: "Kinney, Michael D" , "devel@edk2.groups.io" , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= Cc: "Yao, Jiewen" , "Wang, Jian J" , "Zhang, Chao B" References: <20191022173716.27700-1-michael.d.kinney@intel.com> <20191022173716.27700-2-michael.d.kinney@intel.com> From: "Laszlo Ersek" Message-ID: <291020c5-be1c-0b6a-fdb5-d0a7c27f43f8@redhat.com> Date: Wed, 23 Oct 2019 01:16:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-MC-Unique: trnw1j8KNrm5lth1224Zdg-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 10/22/19 20:27, Kinney, Michael D wrote: > Hi Laszlo, >=20 > I agree with the challenges in reviewing these types of > code changes. The spelling errors in comments are easier > to review because we know if it is in a comment there is > no change to code functionality. >=20 > The original patch only changed a single EFI_D_ to DEBUG_ > for the one DEBUG() statement that had a spelling error. >=20 > Philippe requested that be split out into its own patch. I missed that discussion (or, more likely, I must have skipped it in a hurry; sorry about that). In retrospect, Phil's request makes sense, especially if a single EFI_D_ to DEBUG_ change was hidden among hundreds of typo fixes. > Would you prefer the first patch only change the one line > with the spelling error and not update the rest of the > package? Yes, I think so. (Although, I certainly defer on this to the SecurityPkg maintainers, and Phil.) > The source of this bug is for CI checks being enabled > in edk2-staging/edk2-ci. Since we are using PatchCheck.py > as one of the checks, any updates to any package where > the patch file includes a line with EFI_D_* will fail, > so all packages will need to do the conversion as some > point. "BaseTools/Scripts/PatchCheck.py" checks for EFI_D_ with the regular expression in "old_debug_re". However, "old_debug_re" is only referenced in the check_added_line() method. I think that's the right behavior: we shouldn't reject a patch just because it has EFI_D_ in context (=3D unchanged lines) or in removed lines. EFI_D_ is only wrong in lines that a patch is introducing anew. Therefore, I don't think it's necessary to remove all EFI_D_ uses eventually. We only need to remove those uses whose lines we touch for another reason. > We need to decide if this will be done as needed > only to lines in affected patches, or if we want to do it > to whole packages so everything is cleaned up. I prefer option#1. I see value in a large audit mostly if the audit finds bugs -- semantic or actual (functional) bugs. EFI_D_* is a very-very small semantic issue and it doesn't seem to block or complicate anything; so fixing it doesn't justify the review cost, IMO. $ git grep EFI_D_ -- OvmfPkg/ ArmVirtPkg/ | wc -l 444 *shudder* Thanks! Laszlo >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Tuesday, October 22, 2019 11:06 AM >> To: devel@edk2.groups.io; Kinney, Michael D >> >> Cc: Yao, Jiewen ; Wang, Jian J >> ; Zhang, Chao B >> >> Subject: Re: [edk2-devel] [Patch v2 1/2] SecurityPkg: >> Replace EFI_D_* with DEBUG_* >> >> Hi Mike, >> >> On 10/22/19 19:37, Michael D Kinney wrote: >>> Update all DEBUG() macros in the SecurityPkg to use >> DEBUG_ instead of >>> EFI_D_. This is required to pass PatchCheck.py >> checks. >> >> [...] >> >>> 45 files changed, 410 insertions(+), 410 deletions(- >> ) >> >> ( >> >> If the SecurityPkg maintainers are happy with this >> patch, then it's not my place to complain. >> >> I'd just like to point out that I'd object to such a >> patch for OvmfPkg. >> Such sweeping conversions are difficult to review (they >> are also difficult to implement -- I think mass >> search&replace is not too safe without human review). >> >> New code should not add EFI_D_* usage, of course. >> >> I'd expect PatchCheck.py to complain about EFI_D_* only >> on lines that are added by a patch, not on lines being >> removed, or present in the context. Is that not the >> case? >> >> ... Hm, looking at patch#2, it seems that some spelling >> errors affect debug messages. Therefore, some of the >> typo fixes do turn EFI_D_* macros into new lines. Given >> that there is a huge number of typo fixes (205 lines, >> apparently), I guess it makes sense to separate out the >> EFI_D_* conversion. It's up to the package owners >> whether they prefer reviewing >> - 410 lines of EFI_D_* massaging, plus 205 lines of >> typo fixes, >> - or 205 lines of { EFI_D_* conversion, plus typo fix >> }. >> >> For OvmfPkg, my choice would likely be (assuming such a >> large diffstat): >> - fix EFI_D_*, one patch per module, and only on lines >> affected by typos, >> - fix typos, one patch per module. >> >> I could suspend and resume a review like that more >> easily. >> >> ) >> >> Thanks >> Laszlo >=20