Re: History question about Base.h and its alternate parallel name space.... Should we change it?

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Andrew,

If we move all of those, then what are the code review rules for a lib of type 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 <Felixp@ami.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> 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 18, 2019, at 9:08 AM, Felix Polyudov
> <Felixp@ami.com> wrote:
> >
> > Mike,
> >
> > I think EFI_GUID and EFI_STATUS should cover most of
> the use cases.
> >
> 
> I think I missed a couple in my original mail....
> 
> But here are the typedef and #define names that get
> remapped (or redone) in
> MdePkg/Include/Uefi/UefiBaseType.h
> 
> typedef struct {
>   UINT32  Data1;
>   UINT16  Data2;
>   UINT16  Data3;
>   UINT8   Data4[8];
> } GUID;
> 
> typedef struct {
>   UINT8 Addr[4];
> } IPv4_ADDRESS;
> 
> typedef struct {
>   UINT8 Addr[16];
> } IPv6_ADDRESS;
> 
> typedef UINT64 PHYSICAL_ADDRESS;
> typedef UINTN RETURN_STATUS;
> 
> #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)
> ....
> 
> 
> I think the cleanup would be as simple as moving things
> from MdePkg/Include/Uefi/UefiBaseType.h to
> MdePkg/Include/Base.h.
> 
> So:
> 
> typedef struct {
>   UINT32  Data1;
>   UINT16  Data2;
>   UINT16  Data3;
>   UINT8   Data4[8];
> } GUID;
> 
> Becomes:
> 
> typedef struct {
>   UINT32  Data1;
>   UINT16  Data2;
>   UINT16  Data3;
>   UINT8   Data4[8];
> } GUID, EFI_GUID;
> 
> Thanks,
> 
> Andrew Fish
> 
> 
> > -----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' <afish@apple.com>; Kinney, Michael
> D
> >> <michael.d.kinney@intel.com>
> >> 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
> >> <michael.d.kinney@intel.com> 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 <edk2-devel@lists.01.org>
> >>>> 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 <Base.h>. 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.