* History question about Base.h and its alternate parallel name space.... Should we change it?
@ 2019-01-16 21:00 Andrew Fish
2019-01-16 21:19 ` Kinney, Michael D
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Fish @ 2019-01-16 21:00 UTC (permalink / raw)
To: edk2-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: History question about Base.h and its alternate parallel name space.... Should we change it?
2019-01-16 21:00 History question about Base.h and its alternate parallel name space.... Should we change it? Andrew Fish
@ 2019-01-16 21:19 ` Kinney, Michael D
2019-01-16 22:18 ` Andrew Fish
0 siblings, 1 reply; 10+ messages in thread
From: Kinney, Michael D @ 2019-01-16 21:19 UTC (permalink / raw)
To: Andrew Fish, edk2-devel@lists.01.org, Kinney, Michael D
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.
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: History question about Base.h and its alternate parallel name space.... Should we change it?
2019-01-16 21:19 ` Kinney, Michael D
@ 2019-01-16 22:18 ` Andrew Fish
2019-01-16 23:09 ` Felix Polyudov
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Fish @ 2019-01-16 22:18 UTC (permalink / raw)
To: Mike Kinney; +Cc: edk2-devel@lists.01.org
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: History question about Base.h and its alternate parallel name space.... Should we change it?
2019-01-16 22:18 ` Andrew Fish
@ 2019-01-16 23:09 ` Felix Polyudov
2019-01-17 20:04 ` Kinney, Michael D
0 siblings, 1 reply; 10+ messages in thread
From: Felix Polyudov @ 2019-01-16 23:09 UTC (permalink / raw)
To: 'Andrew Fish', Mike Kinney; +Cc: edk2-devel@lists.01.org
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: History question about Base.h and its alternate parallel name space.... Should we change it?
2019-01-16 23:09 ` Felix Polyudov
@ 2019-01-17 20:04 ` Kinney, Michael D
2019-01-18 17:08 ` Felix Polyudov
0 siblings, 1 reply; 10+ messages in thread
From: Kinney, Michael D @ 2019-01-17 20:04 UTC (permalink / raw)
To: Felix Polyudov, 'Andrew Fish', Kinney, Michael D
Cc: edk2-devel@lists.01.org
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: History question about Base.h and its alternate parallel name space.... Should we change it?
2019-01-17 20:04 ` Kinney, Michael D
@ 2019-01-18 17:08 ` Felix Polyudov
2019-01-18 17:24 ` Andrew Fish
0 siblings, 1 reply; 10+ messages in thread
From: Felix Polyudov @ 2019-01-18 17:08 UTC (permalink / raw)
To: 'Kinney, Michael D', 'Andrew Fish'
Cc: edk2-devel@lists.01.org
Mike,
I think EFI_GUID and EFI_STATUS should cover most of the use cases.
-----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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: History question about Base.h and its alternate parallel name space.... Should we change it?
2019-01-18 17:08 ` Felix Polyudov
@ 2019-01-18 17:24 ` Andrew Fish
2019-01-22 17:49 ` Kinney, Michael D
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Fish @ 2019-01-18 17:24 UTC (permalink / raw)
To: Felix Poludov; +Cc: Mike Kinney, edk2-devel@lists.01.org
> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: History question about Base.h and its alternate parallel name space.... Should we change it?
2019-01-18 17:24 ` Andrew Fish
@ 2019-01-22 17:49 ` Kinney, Michael D
2019-01-23 1:00 ` Andrew Fish
0 siblings, 1 reply; 10+ messages in thread
From: Kinney, Michael D @ 2019-01-22 17:49 UTC (permalink / raw)
To: afish@apple.com, Felix Poludov, Kinney, Michael D; +Cc: edk2-devel@lists.01.org
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: History question about Base.h and its alternate parallel name space.... Should we change it?
2019-01-22 17:49 ` Kinney, Michael D
@ 2019-01-23 1:00 ` Andrew Fish
2019-01-23 18:27 ` Kinney, Michael D
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Fish @ 2019-01-23 1:00 UTC (permalink / raw)
To: Mike Kinney; +Cc: Felix Poludov, edk2-devel@lists.01.org
> 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.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: History question about Base.h and its alternate parallel name space.... Should we change it?
2019-01-23 1:00 ` Andrew Fish
@ 2019-01-23 18:27 ` Kinney, Michael D
0 siblings, 0 replies; 10+ messages in thread
From: Kinney, Michael D @ 2019-01-23 18:27 UTC (permalink / raw)
To: afish@apple.com, Kinney, Michael D; +Cc: Felix Poludov, edk2-devel@lists.01.org
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.
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-01-23 18:27 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-16 21:00 History question about Base.h and its alternate parallel name space.... Should we change it? Andrew Fish
2019-01-16 21:19 ` Kinney, Michael D
2019-01-16 22:18 ` Andrew Fish
2019-01-16 23:09 ` Felix Polyudov
2019-01-17 20:04 ` Kinney, Michael D
2019-01-18 17:08 ` Felix Polyudov
2019-01-18 17:24 ` Andrew Fish
2019-01-22 17:49 ` Kinney, Michael D
2019-01-23 1:00 ` Andrew Fish
2019-01-23 18:27 ` Kinney, Michael D
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox