Re: [RFC] Fine-grained review ownership for MdeModulePkg

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Wed, Jun 19, 2019 at 05:09:40AM +0000, Wu, Hao A wrote:
> Hello all,
> 
> As suggested by Ray and Leif, modules (with wildcard) in MdeModulePkg are
> classified to a list of features.
> 
> Please note that:
> * The below list is a draft at this moment, please help to provide
>   feedbacks/comments;
> * Modules with no clear classification are listed under the 'Misc' section
>   at the bottom of the list.

Thank you for doing the heavy lifting.

A few comments below on how this could be more easily described in the
Maintainer.txt syntax.

> ACPI:
> MdeModulePkg/Include/*/*Acpi*.h
> MdeModulePkg/Universal/Acpi/
>
> BDS:
> MdeModulePkg/Include/Library/PlatformBootManagerLib.h
> MdeModulePkg/Include/Library/UefiBootManagerLib.h
> MdeModulePkg/Library/PlatformBootManagerLibNull/
> MdeModulePkg/Library/UefiBootManagerLib/

MdeModulePkg/*BootManagerLib*/

> MdeModulePkg/Universal/BdsDxe/
> MdeModulePkg/Universal/BootManagerPolicyDxe/

Or maybe even
MdeModulePkg/*BootManager*/, which would also match the line above.

> MdeModulePkg/Universal/LoadFileOnFv2/
> MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.*
> 
> Console:
> MdeModulePkg/Include/Guid/ConnectConInEvent.h
> MdeModulePkg/Include/Guid/ConsoleInDevice.h
> MdeModulePkg/Include/Guid/ConsoleOutDevice.h
> MdeModulePkg/Include/Guid/StandardErrorDevice.h
> MdeModulePkg/Include/Guid/TtyTerm.h
> MdeModulePkg/Universal/Console/ConPlatformDxe/
> MdeModulePkg/Universal/Console/ConSplitterDxe/
> MdeModulePkg/Universal/Console/GraphicsConsoleDxe/
> MdeModulePkg/Universal/Console/TerminalDxe/

I was intrigued as to why this did not specify
MdeModulePkg/Universal/Console/
See [1] below.

However, even if suggestions included below were not implemented, the
situation could be described as:
F: MdeModulePkg/Universal/Console/
X: MdeModulePkg/Universal/Console/GraphicsOutputDxe/

> Core (PEI, DXE and Runtime):
> MdeModulePkg/Core/Dxe/*
> MdeModulePkg/Core/Dxe/Dispatcher/
> MdeModulePkg/Core/Dxe/DxeMain/
> MdeModulePkg/Core/Dxe/Event/
> MdeModulePkg/Core/Dxe/FwVol*/
> MdeModulePkg/Core/Dxe/Hand/
> MdeModulePkg/Core/Dxe/Image/
> MdeModulePkg/Core/Dxe/Library/
> MdeModulePkg/Core/Dxe/Misc/
> MdeModulePkg/Core/Dxe/SectionExtraction/

F: MdeModulePkg/Core/Dxe/
X: MdeModulePkg/Core/Dxe/Mem/

> MdeModulePkg/Core/DxeIplPeim/
> MdeModulePkg/Core/Pei/*
> MdeModulePkg/Core/Pei/BootMode/
> MdeModulePkg/Core/Pei/CpuIo/
> MdeModulePkg/Core/Pei/Dependency/
> MdeModulePkg/Core/Pei/Dispatcher/
> MdeModulePkg/Core/Pei/FwVol/
> MdeModulePkg/Core/Pei/Hob/
> MdeModulePkg/Core/Pei/Image/
> MdeModulePkg/Core/Pei/PeiMain/
> MdeModulePkg/Core/Pei/Ppi/
> MdeModulePkg/Core/Pei/Security/

F: MdeModulePkg/Core/Pei/
X: MdeModulePkg/Core/Pei/Memory/
X: MdeModulePkg/Core/Pei/PciCfg2/
X: MdeModulePkg/Core/Pei/Reset/
X: MdeModulePkg/Core/Pei/StatusCode/

I'm going to stop there, because I'm lazy, and I realise this is about
the responsibility areas rather than an actual patch to
Maintainers.txt - and I think my point is made.

Further comments below.

> MdeModulePkg/Core/RuntimeDxe/
> MdeModulePkg/Include/Guid/Crc32GuidedSectionExtraction.h
> MdeModulePkg/Include/Guid/EventExitBootServiceFailed.h
> MdeModulePkg/Include/Guid/IdleLoopEvent.h
> MdeModulePkg/Include/Guid/LoadModuleAtFixedAddress.h
> MdeModulePkg/Include/Library/SecurityManagementLib.h
> MdeModulePkg/Library/*SectionExtract*/
> MdeModulePkg/Library/DxeSecurityManagementLib/
> MdeModulePkg/Universal/PlatformDriOverrideDxe/
> MdeModulePkg/Universal/SectionExtraction*/
> MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c
> 
> Debug:
> MdeModulePkg/Include/Guid/DebugMask.h
> MdeModulePkg/Include/Library/DebugAgentLib.h
> MdeModulePkg/Include/Ppi/Debug.h
> MdeModulePkg/Library/*Debug*/
> MdeModulePkg/Universal/Debug*/
> 
> Decompress:
> MdeModulePkg/Include/Guid/LzmaDecompress.h
> MdeModulePkg/Library/*Decompress*/
> 
> Device:
> MdeModulePkg/Bus/Ata/
> MdeModulePkg/Bus/I2c/
> MdeModulePkg/Bus/Isa/
> MdeModulePkg/Bus/Pci/Ehci*/
> MdeModulePkg/Bus/Pci/IdeBusPei/
> MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/
> MdeModulePkg/Bus/Pci/NvmExpress*/
> MdeModulePkg/Bus/Pci/PciSioSerialDxe/
> MdeModulePkg/Bus/Pci/SataControllerDxe/
> MdeModulePkg/Bus/Pci/SdMmc*/
> MdeModulePkg/Bus/Pci/Ufs*/
> MdeModulePkg/Bus/Pci/Uhci*/
> MdeModulePkg/Bus/Pci/Xhci*/
> MdeModulePkg/Bus/Scsi/
> MdeModulePkg/Bus/Sd/
> MdeModulePkg/Bus/Ufs/
> MdeModulePkg/Bus/Usb/
> MdeModulePkg/Include/*/*Ata*.h
> MdeModulePkg/Include/*/*NonDiscoverableDevice*.h
> MdeModulePkg/Include/*/*NvmExpress*.h
> MdeModulePkg/Include/*/*SerialPort*.h
> MdeModulePkg/Include/*/*SdMmc*.h
> MdeModulePkg/Include/*/*Ufs*.h
> MdeModulePkg/Include/*/*Usb*.h
> MdeModulePkg/Include/Guid/S3StorageDeviceInitList.h
> MdeModulePkg/Include/Guid/RecoveryDevice.h
> MdeModulePkg/Include/Guid/UsbKeyBoardLayout.h
> MdeModulePkg/Include/Ppi/StorageSecurityCommand.h
> MdeModulePkg/Include/Protocol/Ps2Policy.h
> MdeModulePkg/Library/BaseSerialPortLib16550/
> MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/
> MdeModulePkg/Universal/SerialDxe/
> 
> Disk:
> MdeModulePkg/Universal/Disk/
> 
> EBC:
> MdeModulePkg/Include/*/*Ebc*.h
> MdeModulePkg/Include/Protocol/DebuggerConfiguration.h
> MdeModulePkg/Universal/EbcDxe/
> 
> Firmware Update:
> MdeModulePkg/Application/CapsuleApp/
> MdeModulePkg/Include/*/*Capsule*.h
> MdeModulePkg/Include/Library/DisplayUpdateProgressLib.h
> MdeModulePkg/Include/Library/FmpAuthenticationLib.h
> MdeModulePkg/Include/Protocol/EsrtManagement.h
> MdeModulePkg/Include/Protocol/FirmwareManagementProgress.h
> MdeModulePkg/Library/DisplayUpdateProgressLib*/
> MdeModulePkg/Library/DxeCapsuleLib*/
> MdeModulePkg/Library/FmpAuthenticationLibNull/
> MdeModulePkg/Universal/Capsule*/
> MdeModulePkg/Universal/Esrt*/
> 
> Graphic:
> MdeModulePkg/Include/*/*Logo*.h
> MdeModulePkg/Include/Library/BmpSupportLib.h
> MdeModulePkg/Include/Library/FrameBufferBltLib.h
> MdeModulePkg/Library/BaseBmpSupportLib/
> MdeModulePkg/Library/BootLogoLib/
> MdeModulePkg/Library/FrameBufferBltLib/
> MdeModulePkg/Logo/
> MdeModulePkg/Universal/Console/GraphicsOutputDxe/

[1] I would say this suggests GraphicsOutputDxe could be moved to,
    say, MdeModulePkg/Universal/Graphics - and Logo be moved there
    too.

> 
> HII/UI:
> MdeModulePkg/Application/BootManagerMenuApp/
> MdeModulePkg/Application/UiApp/
> MdeModulePkg/Include/*/*FileExplorer*.h
> MdeModulePkg/Include/*/*FormBrowser*.h
> MdeModulePkg/Include/*/*Hii*.h
> MdeModulePkg/Include/Library/CustomizedDisplayLib.h
> MdeModulePkg/Include/Protocol/DisplayProtocol.h
> MdeModulePkg/Library/*FileExplorer*/
> MdeModulePkg/Library/*Hii*/
> MdeModulePkg/Library/*UiLib/
> MdeModulePkg/Library/CustomizedDisplayLib/
> MdeModulePkg/Universal/DisplayEngineDxe/
> MdeModulePkg/Universal/FileExplorerDxe/
> MdeModulePkg/Universal/Hii*/
> MdeModulePkg/Universal/SetupBrowserDxe/
> 
> IPMI:
> MdeModulePkg/Include/*/*Ipmi*.h
> MdeModulePkg/Library/*Ipmi*/
> 
> Memory Management:
> MdeModulePkg/Application/MemoryProfileInfo/
> MdeModulePkg/Core/Dxe/Gcd/
> MdeModulePkg/Core/Dxe/Mem/
> MdeModulePkg/Core/Pei/Memory/
> MdeModulePkg/Include/*/*Mem*.h
> MdeModulePkg/Include/*/*IoMmu*.h
> MdeModulePkg/Library/*MemoryAllocation*/
> MdeModulePkg/Universal/MemoryTest/
> 
> PCD:
> MdeModulePkg/Application/DumpDynPcd/
> MdeModulePkg/Include/*/*Pcd*.h
> MdeModulePkg/Universal/PCD/
> 
> PCI Bus:
> MdeModulePkg/Bus/Pci/IncompatiblePciDeviceSupportDxe/
> MdeModulePkg/Bus/Pci/PciBusDxe/
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/
> MdeModulePkg/Core/Pei/PciCfg2/
> MdeModulePkg/Include/Library/PciHostBridgeLib.h
> MdeModulePkg/Library/PciHostBridgeLibNull/
> MdeModulePkg/Universal/PcatSingleSegmentPciCfg2Pei/
> 
> Performance:
> MdeModulePkg/Include/*/*Perf*.h
> MdeModulePkg/Library/*Perf*/
> 
> Reset:
> MdeModulePkg/Core/Pei/Reset/
> MdeModulePkg/Include/*/*Reset*.h
> MdeModulePkg/Library/*Reset*/
> MdeModulePkg/Universal/ResetSystem*/
> 
> S3:
> MdeModulePkg/Include/*/*BootScript*.h
> MdeModulePkg/Include/*/*LockBox*.h
> MdeModulePkg/Include/*/*S3*.h
> MdeModulePkg/Library/*LockBox*/
> MdeModulePkg/Library/*S3*/
> MdeModulePkg/Universal/LockBox/
> 
> SMBIOS:
> MdeModulePkg/Universal/Smbios*/
> 
> SMM:
> MdeModulePkg/Application/SmiHandlerProfileInfo/
> MdeModulePkg/Core/PiSmmCore/
> MdeModulePkg/Include/*/*Smi*.h
> MdeModulePkg/Include/*/*Smm*.h
> MdeModulePkg/Library/*Smi*/
> MdeModulePkg/Library/*Smm*/
> MdeModulePkg/Universal/SmmCommunicationBufferDxe/
> 
> Status Code:
> MdeModulePkg/Core/Pei/StatusCode/
> MdeModulePkg/Include/*/*StatusCode*.h
> MdeModulePkg/Library/*StatusCode*/
> MdeModulePkg/Universal/*StatusCode*/
> 
> Variable:
> MdeModulePkg/Application/VariableInfo/
> MdeModulePkg/Include/*/*FaultTolerantWrite*.h
> MdeModulePkg/Include/*/*Var*.h
> MdeModulePkg/Include/Guid/SystemNvDataGuid.h
> MdeModulePkg/Include/Protocol/SwapAddressRange.h
> MdeModulePkg/Library/*Var*/
> MdeModulePkg/Universal/FaultTolerantWrite*/
> MdeModulePkg/Universal/Variable/
> 
> Misc:

I have no issue with the below, but just wanted to point out that if
there was an overarching MdeModulePkg/
section, these would automatically match against that anyway - we
don't need to explicitly tag everything.

> MdeModulePkg/Application/HelloWorld/
> MdeModulePkg/Include/Guid/MdeModulePkgTokenSpace.h
> MdeModulePkg/Include/Guid/MtcVendor.h
> MdeModulePkg/Include/Guid/ZeroGuid.h
> MdeModulePkg/Include/Library/CpuExceptionHandlerLib.h
> MdeModulePkg/Include/Library/PlatformHookLib.h
> MdeModulePkg/Include/Library/RecoveryLib.h
> MdeModulePkg/Include/Library/SortLib.h
> MdeModulePkg/Include/Library/TpmMeasurementLib.h
> MdeModulePkg/Include/Protocol/Dpc.h
> MdeModulePkg/Include/Protocol/LoadPe32Image.h
> MdeModulePkg/Include/Protocol/PeCoffImageEmulator.h
> MdeModulePkg/Include/Protocol/Print2.h
> MdeModulePkg/Library/BaseHobLibNull/
> MdeModulePkg/Library/BasePlatformHookLibNull/
> MdeModulePkg/Library/BaseSortLib/
> MdeModulePkg/Library/CpuExceptionHandlerLibNull/
> MdeModulePkg/Library/DxePrintLibPrint2Protocol/
> MdeModulePkg/Library/PeiRecoveryLibNull/
> MdeModulePkg/Library/PlatformHookLibSerialPortPpi/
> MdeModulePkg/Library/TpmMeasurementLibNull/
> MdeModulePkg/Library/UefiSortLib/
> MdeModulePkg/Universal/DevicePathDxe/
> MdeModulePkg/Universal/DriverHealthManagerDxe/
> MdeModulePkg/Universal/DriverSampleDxe/
> MdeModulePkg/Universal/FvSimpleFileSystemDxe/
> MdeModulePkg/Universal/LegacyRegion2Dxe/
> MdeModulePkg/Universal/Metronome/
> MdeModulePkg/Universal/MonotonicCounterRuntimeDxe/
> MdeModulePkg/Universal/PrintDxe/
> MdeModulePkg/Universal/RegularExpressionDxe/
> MdeModulePkg/Universal/TimestampDxe/
> MdeModulePkg/Universal/WatchdogTimerDxe/

Looking through this made me realise it would be useful to have a way
of quickly looking up section matches for a given path. So I added a
command line option -l/--lookup to GetMaintainer.py - inline below:

/
    Leif

>From 11a04f4690e7b8bc348483a26bb3840b74b1fd9d Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Wed, 19 Jun 2019 10:07:26 +0100
Subject: [RFC PATCH 1/1] BaseTools: add lookup function to GetMaintainer.py

Add command line option -l/--lookup, which lets you specify a path
(existent or not in the current tree) to look up maintainers for.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 BaseTools/Scripts/GetMaintainer.py | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Scripts/GetMaintainer.py b/BaseTools/Scripts/GetMaintainer.py
index 616342cdb434..444e601c55d1 100644
--- a/BaseTools/Scripts/GetMaintainer.py
+++ b/BaseTools/Scripts/GetMaintainer.py
@@ -160,15 +160,22 @@ if __name__ == '__main__':
                         help='git revision to examine (default: HEAD)',
                         nargs='?',
                         default='HEAD')
+    PARSER.add_argument('-l', '--lookup',
+                        help='Find section matches for path LOOKUP',
+                        required=False)
     ARGS = PARSER.parse_args()
 
     REPO = SetupGit.locate_repo()
 
-    FILES = get_modified_files(REPO, ARGS)
     CONFIG_FILE = os.path.join(REPO.working_dir, 'Maintainers.txt')
 
     SECTIONS = parse_maintainers_file(CONFIG_FILE)
 
+    if ARGS.lookup:
+        FILES = [ARGS.lookup]
+    else:
+        FILES = get_modified_files(REPO, ARGS)
+
     ADDRESSES = []
 
     for file in FILES:
-- 
2.11.0