Re: [edk2-devel] [Patch v2 1/2] SecurityPkg: Replace EFI_D_* with DEBUG_*

["Laszlo Ersek" <lersek@redhat.com>]

On 10/22/19 20:27, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> 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.
> 
> The original patch only changed a single EFI_D_ to DEBUG_
> for the one DEBUG() statement that had a spelling error.
> 
> 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 (= 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 <lersek@redhat.com>
>> Sent: Tuesday, October 22, 2019 11:06 AM
>> To: devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Zhang, Chao B
>> <chao.b.zhang@intel.com>
>> 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
>