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

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

Hi Andrew,

I think it makes sense for new code to use the
UEFI type names and for existing code to remain unchanged.

>From looking at UefiBaseTypes.h, I do not think we should
move all types from that file into Base.h.  If we really
wanted to do that, we could simple add #include of 
UefiBaseTypes.h to Base.h.

Moving a few lines from UefiBaseTypes.h to Base.h at the
bottom of Base.h with a comment block that explains why
the UEFI specific types/defines are available from Base.h
would be another way to add a subset of types/defines.  
That way, if we decide to move more in the future, there
is a place to move them that is easy to review.

Do you want to propose the specific types/defines that 
should be moved?

Best regards,

Mike

> -----Original Message-----
> From: afish@apple.com [mailto:afish@apple.com]
> Sent: Tuesday, January 22, 2019 5:00 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Felix Poludov <Felixp@ami.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 22, 2019, at 9:49 AM, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > 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?
> >
> 
> Mike,
> 
> it seems messy to add types to Base.h to support Base
> Libs. So for example IPv4_ADDRESS and IPv6_ADDRESS got
> sucked into Base.h since an MdePkg Base Lib got added
> that used it. That does not really seem to scale to Base
> Libs in other packages. Thus I guess the preference
> would be to use the common EFI_* names. I'd not advocate
> removing the old names as that is going to break other
> folks Base LIbs. We could do the cleanup in TianoCore.
> If we want to deprecate the old names form, I guess that
> could be an ifdef to allow some depreciation in the
> future?
> 
> Thanks,
> 
> Andrew Fish
> 
> > 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.
> >