From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=michael.d.kinney@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 856E121959CB2 for ; Tue, 22 Jan 2019 09:49:58 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Jan 2019 09:49:57 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,507,1539673200"; d="scan'208";a="127993940" Received: from orsmsx105.amr.corp.intel.com ([10.22.225.132]) by orsmga002.jf.intel.com with ESMTP; 22 Jan 2019 09:49:57 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.240]) by ORSMSX105.amr.corp.intel.com ([169.254.2.219]) with mapi id 14.03.0415.000; Tue, 22 Jan 2019 09:49:57 -0800 From: "Kinney, Michael D" To: "afish@apple.com" , Felix Poludov , "Kinney, Michael D" CC: "edk2-devel@lists.01.org" Thread-Topic: [edk2] History question about Base.h and its alternate parallel name space.... Should we change it? Thread-Index: AQHUrd6ShywsZwEiGEavur1WJbzcbqWyZKPwgACYcICAAA57gIAA2A7ggAHnqoCAAARoAIAFydjQ Date: Tue, 22 Jan 2019 17:49:57 +0000 Message-ID: References: <37D3156D-0434-4A64-BF0C-9883A4B88838@apple.com> <9333E191E0D52B4999CE63A99BA663A00302C56A66@atlms1.us.megatrends.com> <9333E191E0D52B4999CE63A99BA663A00302C573F1@atlms1.us.megatrends.com> <6388D0B3-9465-40E6-8CDF-11001D321637@apple.com> In-Reply-To: <6388D0B3-9465-40E6-8CDF-11001D321637@apple.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Subject: Re: History question about Base.h and its alternate parallel name space.... Should we change it? X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Jan 2019 17:49:58 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Andrew, If we move all of those, then what are the code review rules for a lib of t= ype BASE? Is there a preference to use the current BASE types? Under what conditions= are EFI type names allowed? Is the a preference to stop using the current BASE types all together? Thanks, Mike > -----Original Message----- > From: afish@apple.com [mailto:afish@apple.com] > Sent: Friday, January 18, 2019 9:24 AM > To: Felix Poludov > Cc: Kinney, Michael D ; > edk2-devel@lists.01.org > Subject: Re: [edk2] History question about Base.h and > its alternate parallel name space.... Should we change > it? >=20 >=20 >=20 > > On Jan 18, 2019, at 9:08 AM, Felix Polyudov > wrote: > > > > Mike, > > > > I think EFI_GUID and EFI_STATUS should cover most of > the use cases. > > >=20 > I think I missed a couple in my original mail.... >=20 > But here are the typedef and #define names that get > remapped (or redone) in > MdePkg/Include/Uefi/UefiBaseType.h >=20 > typedef struct { > UINT32 Data1; > UINT16 Data2; > UINT16 Data3; > UINT8 Data4[8]; > } GUID; >=20 > typedef struct { > UINT8 Addr[4]; > } IPv4_ADDRESS; >=20 > typedef struct { > UINT8 Addr[16]; > } IPv6_ADDRESS; >=20 > typedef UINT64 PHYSICAL_ADDRESS; > typedef UINTN RETURN_STATUS; >=20 > #define ENCODE_ERROR(StatusCode) > ((RETURN_STATUS)(MAX_BIT | (StatusCode))) > #define ENCODE_WARNING(StatusCode) > ((RETURN_STATUS)(StatusCode)) > #define RETURN_ERROR(StatusCode) > (((INTN)(RETURN_STATUS)(StatusCode)) < 0) > #define RETURN_SUCCESS 0 > #define RETURN_LOAD_ERROR ENCODE_ERROR (1) > #define RETURN_INVALID_PARAMETER ENCODE_ERROR (2) > .... >=20 >=20 > I think the cleanup would be as simple as moving things > from MdePkg/Include/Uefi/UefiBaseType.h to > MdePkg/Include/Base.h. >=20 > So: >=20 > typedef struct { > UINT32 Data1; > UINT16 Data2; > UINT16 Data3; > UINT8 Data4[8]; > } GUID; >=20 > Becomes: >=20 > typedef struct { > UINT32 Data1; > UINT16 Data2; > UINT16 Data3; > UINT8 Data4[8]; > } GUID, EFI_GUID; >=20 > Thanks, >=20 > Andrew Fish >=20 >=20 > > -----Original Message----- > > From: Kinney, Michael D > [mailto:michael.d.kinney@intel.com] > > Sent: Thursday, January 17, 2019 3:04 PM > > To: Felix Polyudov; 'Andrew Fish'; Kinney, Michael D > > Cc: edk2-devel@lists.01.org > > Subject: RE: [edk2] History question about Base.h and > its alternate parallel name space.... Should we change > it? > > > > Felix, > > > > Is there a specific set that would have the most > benefit? > > > > Is EFI_GUID sufficient? > > > > Mike > > > >> -----Original Message----- > >> From: Felix Polyudov [mailto:Felixp@ami.com] > >> Sent: Wednesday, January 16, 2019 3:10 PM > >> To: 'Andrew Fish' ; Kinney, Michael > D > >> > >> Cc: edk2-devel@lists.01.org > >> Subject: RE: [edk2] History question about Base.h and > >> its alternate parallel name space.... Should we > change > >> it? > >> > >> I agree with Andrew. > >> In my opinion, moving alias types to Base.h will help > to > >> overcome certain practical inconveniences without > >> a significant impact on the ability to detect poorly > >> written Base libraries. > >> > >> -----Original Message----- > >> From: edk2-devel [mailto:edk2-devel- > >> bounces@lists.01.org] On Behalf Of Andrew Fish via > edk2- > >> devel > >> Sent: Wednesday, January 16, 2019 5:18 PM > >> To: Mike Kinney > >> Cc: edk2-devel@lists.01.org > >> Subject: Re: [edk2] History question about Base.h and > >> its alternate parallel name space.... Should we > change > >> it? > >> > >> > >> > >>> On Jan 16, 2019, at 1:19 PM, Kinney, Michael D > >> wrote: > >>> > >>> Hi Andrew, > >>> > >>> I though the reason was a bit more technical. We > have > >> a > >>> MODULE_TYPE of BASE. Library instances that use the > >> BASE > >>> MODULE_TYPE are declaring that the library > interfaces > >> are > >>> safe to be linked against a module of any other type > >> (SEC, > >>> PEI, DXE, SMM, DXE_RUNTIME, UEFI_DRIVER, UEFI_APP). > >>> > >>> We needed to make sure that a lib of type BASE that > >>> includes Base.h as its top level include file only > has > >>> visibility to the types that are safe for all the > >> other > >>> module types. It is up to the top level include > files > >>> for these other module types to provide the gasket > to > >>> the types in Base.h. > >>> > >>> If we add aliases in Base.h, then we may not get > build > >>> breaks when a lib of type BASE includes files that > are > >>> not compatible with BASE. > >>> > >> > >> Mike, > >> > >> I don't think having aliases for return types really > >> helps Base code quality as RETURN_SUCCESS is almost > >> always just a comment in a header file, and only > exists > >> in a .c file. Thus RETURN_* seem like a needless > >> duplication, best case it is a comment that the code > is > >> Base. > >> > >> I will agree that not having EFI_GUID defined does > case > >> all the PPI and Protocol files to blow up if you try > to > >> include them. The failure case I was helping explain > was > >> some one trying to include a PPI, that included a > >> Protocol that contained a data structure that was > >> needed. But I would posit that the definition of a > >> (EFI_)GUID is state agnostic. Having access to a PPI > or > >> Protocol definition does not break Base code, what > >> breaks Base code is trying to access some service > that > >> does not exist. To get more that EFI_GUID you are > going > >> to need to include Uefi.h, PiPei.h, PiDxe.h, etc. and > >> that will block doing anything that is not Base. > >> > >> So I'm asking if redefining the name for EFI_GUID to > >> GUID for Base is really that helpful? > >> > >> Thanks, > >> > >> Andrew Fish > >> > >> > >>> Thanks, > >>> > >>> Mike > >>> > >>>> -----Original Message----- > >>>> From: edk2-devel [mailto:edk2-devel- > >>>> bounces@lists.01.org] On Behalf Of Andrew Fish via > >> edk2- > >>>> devel > >>>> Sent: Wednesday, January 16, 2019 1:00 PM > >>>> To: edk2-devel > >>>> Subject: [edk2] History question about Base.h and > its > >>>> alternate parallel name space.... Should we change > >> it? > >>>> > >>>> I had some one ask me recently why EFI_GUID does > not > >>>> work with #include . I explained they > needed > >> to > >>>> use GUID vs. EFI_GUID. That prompted the question > of > >> why > >>>> we have 2 names for the same thing..... Well the > >>>> historical answer was kind of political as some > team > >>>> wanted to use edk2, but not implement EFI. Thus we > >> have > >>>> EFI types without the EFI_ prefix in Base.h. > >>>> > >>>> So all this got me thinking.... Maybe it makes > sense > >> to > >>>> move some of the renaming from > >>>> MdePkg/Include/Uefi/UefiBaseType.h to Base.h? > >> Removing > >>>> the Base.h duplicate types would potentially hit > lots > >> of > >>>> code [1] and break merges with other code bases > >> (break > >>>> other peoples Base libs etc.). > >>>> > >>>> These lines in MdePkg/Include/Uefi/UefiBaseType.h > >> would > >>>> get moved to MdePkg/Include/Base.h: > >>>> typedef GUID EFI_GUID; > >>>> typedef RETURN_STATUS EFI_STATUS; > >>>> #define EFIERR(_a) ENCODE_ERROR(_a) > >>>> #define EFI_ERROR(A) RETURN_ERROR(A) > >>>> > >>>> #define EFI_SUCCESS RETURN_SUCCESS > >>>> #define EFI_LOAD_ERROR RETURN_LOAD_ERROR > >>>> #define EFI_INVALID_PARAMETER > >>>> RETURN_INVALID_PARAMETER > >>>> #define EFI_UNSUPPORTED > RETURN_UNSUPPORTED > >>>> #define EFI_BAD_BUFFER_SIZE > >> RETURN_BAD_BUFFER_SIZE > >>>> #define EFI_BUFFER_TOO_SMALL > >>>> RETURN_BUFFER_TOO_SMALL > >>>> #define EFI_NOT_READY RETURN_NOT_READY > >>>> #define EFI_DEVICE_ERROR > RETURN_DEVICE_ERROR > >>>> #define EFI_WRITE_PROTECTED > >> RETURN_WRITE_PROTECTED > >>>> #define EFI_OUT_OF_RESOURCES > >>>> RETURN_OUT_OF_RESOURCES > >>>> #define EFI_VOLUME_CORRUPTED > >>>> RETURN_VOLUME_CORRUPTED > >>>> #define EFI_VOLUME_FULL > RETURN_VOLUME_FULL > >>>> #define EFI_NO_MEDIA RETURN_NO_MEDIA > >>>> #define EFI_MEDIA_CHANGED > >> RETURN_MEDIA_CHANGED > >>>> #define EFI_NOT_FOUND RETURN_NOT_FOUND > >>>> #define EFI_ACCESS_DENIED > >> RETURN_ACCESS_DENIED > >>>> #define EFI_NO_RESPONSE > RETURN_NO_RESPONSE > >>>> #define EFI_NO_MAPPING RETURN_NO_MAPPING > >>>> #define EFI_TIMEOUT RETURN_TIMEOUT > >>>> #define EFI_NOT_STARTED > RETURN_NOT_STARTED > >>>> #define EFI_ALREADY_STARTED > >> RETURN_ALREADY_STARTED > >>>> #define EFI_ABORTED RETURN_ABORTED > >>>> #define EFI_ICMP_ERROR RETURN_ICMP_ERROR > >>>> #define EFI_TFTP_ERROR RETURN_TFTP_ERROR > >>>> #define EFI_PROTOCOL_ERROR > >> RETURN_PROTOCOL_ERROR > >>>> #define EFI_INCOMPATIBLE_VERSION > >>>> RETURN_INCOMPATIBLE_VERSION > >>>> #define EFI_SECURITY_VIOLATION > >>>> RETURN_SECURITY_VIOLATION > >>>> #define EFI_CRC_ERROR RETURN_CRC_ERROR > >>>> #define EFI_END_OF_MEDIA > RETURN_END_OF_MEDIA > >>>> #define EFI_END_OF_FILE > RETURN_END_OF_FILE > >>>> #define EFI_INVALID_LANGUAGE > >>>> RETURN_INVALID_LANGUAGE > >>>> #define EFI_COMPROMISED_DATA > >>>> RETURN_COMPROMISED_DATA > >>>> #define EFI_HTTP_ERROR RETURN_HTTP_ERROR > >>>> > >>>> #define EFI_WARN_UNKNOWN_GLYPH > >>>> RETURN_WARN_UNKNOWN_GLYPH > >>>> #define EFI_WARN_DELETE_FAILURE > >>>> RETURN_WARN_DELETE_FAILURE > >>>> #define EFI_WARN_WRITE_FAILURE > >>>> RETURN_WARN_WRITE_FAILURE > >>>> #define EFI_WARN_BUFFER_TOO_SMALL > >>>> RETURN_WARN_BUFFER_TOO_SMALL > >>>> #define EFI_WARN_STALE_DATA > >> RETURN_WARN_STALE_DATA > >>>> #define EFI_WARN_FILE_SYSTEM > >>>> RETURN_WARN_FILE_SYSTEM > >>>> > >>>> I'm interested what folks think about a change like > >>>> this? This change makes the alternate names > optional. > >>>> > >>>> I guess we could also leave the old Base.h > >> definitions > >>>> in Base.h and cleanup the code to only use the EFI > >> form, > >>>> but that is a much bigger change? > >>>> > >>>> [1] RETURN_SUCCSS usage: git grep -w RETURN_SUCCESS > >>>> > >>>> Thanks, > >>>> > >>>> Andrew Fish > >>>> > >>>> _______________________________________________ > >>>> edk2-devel mailing list > >>>> edk2-devel@lists.01.org > >>>> https://lists.01.org/mailman/listinfo/edk2-devel > >> > >> _______________________________________________ > >> edk2-devel mailing list > >> edk2-devel@lists.01.org > >> https://lists.01.org/mailman/listinfo/edk2-devel > >> > >> Please consider the environment before printing this > >> email. > >> > >> The information contained in this message may be > >> confidential and proprietary to American Megatrends, > >> Inc. This communication is intended to be read only > by > >> the individual or entity to whom it is addressed or > by > >> their designee. If the reader of this message is not > the > >> intended recipient, you are on notice that any > >> distribution of this message, in any form, is > strictly > >> prohibited. Please promptly notify the sender by > reply > >> e-mail or by telephone at 770-246-8600, and then > delete > >> or destroy all copies of the transmission. > > > > Please consider the environment before printing this > email. > > > > The information contained in this message may be > confidential and proprietary to American Megatrends, > Inc. This communication is intended to be read only by > the individual or entity to whom it is addressed or by > their designee. If the reader of this message is not the > intended recipient, you are on notice that any > distribution of this message, in any form, is strictly > prohibited. Please promptly notify the sender by reply > e-mail or by telephone at 770-246-8600, and then delete > or destroy all copies of the transmission.