* [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 20:59 ` Laszlo Ersek
2016-10-21 21:27 ` [PATCH 02/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls Laszlo Ersek
` (20 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Liming Gao, Michael D Kinney
ASSERT_EFI_ERROR() cannot be used in BASE type modules because
- the replacement text calls EFI_ERROR(),
- EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
- the inclusion of "UefiBaseType.h" is not required for BASE type modules.
While
ASSERT (!RETURN_ERROR (StatusParameter))
would be a functional statement in BASE type modules, it would be less
convenient and less informative: ASSERT_EFI_ERROR() prints the actual
StatusParameter.
Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
original macro definition and update it as follows:
- replace EFI with RETURN,
- wrap overlong lines in the comment block and in the code,
- EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
one such BASE module.
MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
index 81904325703f..3a910e6a208b 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
#define ASSERT_EFI_ERROR(StatusParameter)
#endif
+/**
+ Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
+
+ If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
+ bit of PcdDebugProperyMask is set, then this macro evaluates the
+ RETURN_STATUS value specified by StatusParameter. If StatusParameter is an
+ error code, then DebugAssert() is called passing in the source filename,
+ source line number, and StatusParameter.
+
+ @param StatusParameter RETURN_STATUS value to evaluate.
+
+**/
+#if !defined(MDEPKG_NDEBUG)
+ #define ASSERT_RETURN_ERROR(StatusParameter) \
+ do { \
+ if (DebugAssertEnabled ()) { \
+ if (RETURN_ERROR (StatusParameter)) { \
+ DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
+ StatusParameter)); \
+ _ASSERT (!RETURN_ERROR (StatusParameter)); \
+ } \
+ } \
+ } while (FALSE)
+#else
+ #define ASSERT_RETURN_ERROR(StatusParameter)
+#endif
+
/**
Macro that calls DebugAssert() if a protocol is already installed in the
handle database.
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
2016-10-21 21:27 ` [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() Laszlo Ersek
@ 2016-10-24 20:59 ` Laszlo Ersek
2016-10-24 23:05 ` Kinney, Michael D
0 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-24 20:59 UTC (permalink / raw)
To: Michael D Kinney, Liming Gao; +Cc: edk2-devel-01
Mike, Liming,
On 10/21/16 23:27, Laszlo Ersek wrote:
> ASSERT_EFI_ERROR() cannot be used in BASE type modules because
> - the replacement text calls EFI_ERROR(),
> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
> - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
>
> While
>
> ASSERT (!RETURN_ERROR (StatusParameter))
>
> would be a functional statement in BASE type modules, it would be less
> convenient and less informative: ASSERT_EFI_ERROR() prints the actual
> StatusParameter.
>
> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
> original macro definition and update it as follows:
> - replace EFI with RETURN,
> - wrap overlong lines in the comment block and in the code,
> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
> one such BASE module.
>
> MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
> index 81904325703f..3a910e6a208b 100644
> --- a/MdePkg/Include/Library/DebugLib.h
> +++ b/MdePkg/Include/Library/DebugLib.h
> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
> #define ASSERT_EFI_ERROR(StatusParameter)
> #endif
>
> +/**
> + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
> +
> + If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
> + bit of PcdDebugProperyMask is set, then this macro evaluates the
> + RETURN_STATUS value specified by StatusParameter. If StatusParameter is an
> + error code, then DebugAssert() is called passing in the source filename,
> + source line number, and StatusParameter.
> +
> + @param StatusParameter RETURN_STATUS value to evaluate.
> +
> +**/
> +#if !defined(MDEPKG_NDEBUG)
> + #define ASSERT_RETURN_ERROR(StatusParameter) \
> + do { \
> + if (DebugAssertEnabled ()) { \
> + if (RETURN_ERROR (StatusParameter)) { \
> + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
> + StatusParameter)); \
> + _ASSERT (!RETURN_ERROR (StatusParameter)); \
> + } \
> + } \
> + } while (FALSE)
> +#else
> + #define ASSERT_RETURN_ERROR(StatusParameter)
> +#endif
> +
> /**
> Macro that calls DebugAssert() if a protocol is already installed in the
> handle database.
>
can I please get a maintainer review for this patch? The rest of the
series is ready to go, but it depends on this patch.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
2016-10-24 20:59 ` Laszlo Ersek
@ 2016-10-24 23:05 ` Kinney, Michael D
2016-10-25 7:54 ` Laszlo Ersek
2016-10-25 8:22 ` Zeng, Star
0 siblings, 2 replies; 54+ messages in thread
From: Kinney, Michael D @ 2016-10-24 23:05 UTC (permalink / raw)
To: Laszlo Ersek, Gao, Liming, Kinney, Michael D; +Cc: edk2-devel-01
Hi Laszlo,
Sorry for the delay. I was traveling last week.
I did see this and I have been thinking about it.
I think it does make sense to add this new macro
for libraries of type BASE. I am surprised we did
not run into an issue before that would have required
the introduction of this macro earlier. Unless the
workaround has been to add #include of
<Uefi/UefiBaseTypes.h>, which makes me think we should
review BASE libraries to make sure that extra include
is not present.
The EFI_* error codes are mapped to RETURN_* error
codes. So the only feedback I was considering was
to implement ASSERT_EFI_ERROR() using
ASSERT_RETURN_ERROR(), but that might not always be
the right mapping because the RETURN_* codes are
a subset of EFI_* error codes.
Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>
Best regards,
Mike
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, October 24, 2016 2:00 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
>
> Mike, Liming,
>
> On 10/21/16 23:27, Laszlo Ersek wrote:
> > ASSERT_EFI_ERROR() cannot be used in BASE type modules because
> > - the replacement text calls EFI_ERROR(),
> > - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
> > - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
> >
> > While
> >
> > ASSERT (!RETURN_ERROR (StatusParameter))
> >
> > would be a functional statement in BASE type modules, it would be less
> > convenient and less informative: ASSERT_EFI_ERROR() prints the actual
> > StatusParameter.
> >
> > Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
> > original macro definition and update it as follows:
> > - replace EFI with RETURN,
> > - wrap overlong lines in the comment block and in the code,
> > - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
> >
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >
> > Notes:
> > OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
> > one such BASE module.
> >
> > MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
> > index 81904325703f..3a910e6a208b 100644
> > --- a/MdePkg/Include/Library/DebugLib.h
> > +++ b/MdePkg/Include/Library/DebugLib.h
> > @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
> > #define ASSERT_EFI_ERROR(StatusParameter)
> > #endif
> >
> > +/**
> > + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
> > +
> > + If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
> > + bit of PcdDebugProperyMask is set, then this macro evaluates the
> > + RETURN_STATUS value specified by StatusParameter. If StatusParameter is an
> > + error code, then DebugAssert() is called passing in the source filename,
> > + source line number, and StatusParameter.
> > +
> > + @param StatusParameter RETURN_STATUS value to evaluate.
> > +
> > +**/
> > +#if !defined(MDEPKG_NDEBUG)
> > + #define ASSERT_RETURN_ERROR(StatusParameter) \
> > + do { \
> > + if (DebugAssertEnabled ()) { \
> > + if (RETURN_ERROR (StatusParameter)) { \
> > + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
> > + StatusParameter)); \
> > + _ASSERT (!RETURN_ERROR (StatusParameter)); \
> > + } \
> > + } \
> > + } while (FALSE)
> > +#else
> > + #define ASSERT_RETURN_ERROR(StatusParameter)
> > +#endif
> > +
> > /**
> > Macro that calls DebugAssert() if a protocol is already installed in the
> > handle database.
> >
>
> can I please get a maintainer review for this patch? The rest of the
> series is ready to go, but it depends on this patch.
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
2016-10-24 23:05 ` Kinney, Michael D
@ 2016-10-25 7:54 ` Laszlo Ersek
2016-10-25 10:32 ` Gao, Liming
2016-10-25 8:22 ` Zeng, Star
1 sibling, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-25 7:54 UTC (permalink / raw)
To: Kinney, Michael D, Gao, Liming
Cc: edk2-devel-01, Jordan Justen (Intel address), Ard Biesheuvel
On 10/25/16 01:05, Kinney, Michael D wrote:
> Hi Laszlo,
>
> Sorry for the delay. I was traveling last week.
>
> I did see this and I have been thinking about it.
> I think it does make sense to add this new macro
> for libraries of type BASE. I am surprised we did
> not run into an issue before that would have required
> the introduction of this macro earlier. Unless the
> workaround has been to add #include of
> <Uefi/UefiBaseTypes.h>, which makes me think we should
> review BASE libraries to make sure that extra include
> is not present.
I spent a few minutes on the following shell script, to identify such
libraries:
{
# Locate the INF files that have a LIBRARY_CLASS define with a client
# module type list that explicitly includes BASE
git grep -l -E '\<LIBRARY_CLASS *= *[A-Za-z0-9_]+ *\|.*\<BASE\>' -- \
'*.inf'
# Locate the INF files that have MODULE_TYPE=BASE, and a LIBRARY_CLASS
# define without a client type list.
git grep -l -E '\<MODULE_TYPE *= *BASE\>' -- '*inf' \
| xargs -r -- grep -l -E '\<LIBRARY_CLASS\>[^|]+$' --
} \
| {
# Cut off the last pathname component, in order to get the pathname of
# the directory containing the INF file
rev | cut -f 2- -d / | rev
} \
| {
# If a directory has several matching INF files, list the directory
# only once.
sort -u
} \
| {
# Check if any file in these directories includes
# "Uefi/UefiBaseType.h".
xargs -r -- grep -r -l Uefi/UefiBaseType.h --
}
It prints the following files:
CorebootModulePkg/Library/CbParseLib/CbParseLib.c
CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.c
MdePkg/Library/BaseLib/FilePaths.c
OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
QuarkSocPkg/QuarkNorthCluster/Library/QNCSmmLib/QNCSmmLib.c
QuarkSocPkg/QuarkNorthCluster/Library/ResetSystemLib/ResetSystemLib.c
We should likely investigate them.
I'll handle the OvmfPkg one.
>
> The EFI_* error codes are mapped to RETURN_* error
> codes. So the only feedback I was considering was
> to implement ASSERT_EFI_ERROR() using
> ASSERT_RETURN_ERROR(), but that might not always be
> the right mapping because the RETURN_* codes are
> a subset of EFI_* error codes.
>
> Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>
Thank you!
Laszlo
> Best regards,
>
> Mike
>
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, October 24, 2016 2:00 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>
>> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
>> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
>>
>> Mike, Liming,
>>
>> On 10/21/16 23:27, Laszlo Ersek wrote:
>>> ASSERT_EFI_ERROR() cannot be used in BASE type modules because
>>> - the replacement text calls EFI_ERROR(),
>>> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
>>> - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
>>>
>>> While
>>>
>>> ASSERT (!RETURN_ERROR (StatusParameter))
>>>
>>> would be a functional statement in BASE type modules, it would be less
>>> convenient and less informative: ASSERT_EFI_ERROR() prints the actual
>>> StatusParameter.
>>>
>>> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
>>> original macro definition and update it as follows:
>>> - replace EFI with RETURN,
>>> - wrap overlong lines in the comment block and in the code,
>>> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
>>>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>> OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
>>> one such BASE module.
>>>
>>> MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
>>> 1 file changed, 27 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
>>> index 81904325703f..3a910e6a208b 100644
>>> --- a/MdePkg/Include/Library/DebugLib.h
>>> +++ b/MdePkg/Include/Library/DebugLib.h
>>> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
>>> #define ASSERT_EFI_ERROR(StatusParameter)
>>> #endif
>>>
>>> +/**
>>> + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
>>> +
>>> + If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
>>> + bit of PcdDebugProperyMask is set, then this macro evaluates the
>>> + RETURN_STATUS value specified by StatusParameter. If StatusParameter is an
>>> + error code, then DebugAssert() is called passing in the source filename,
>>> + source line number, and StatusParameter.
>>> +
>>> + @param StatusParameter RETURN_STATUS value to evaluate.
>>> +
>>> +**/
>>> +#if !defined(MDEPKG_NDEBUG)
>>> + #define ASSERT_RETURN_ERROR(StatusParameter) \
>>> + do { \
>>> + if (DebugAssertEnabled ()) { \
>>> + if (RETURN_ERROR (StatusParameter)) { \
>>> + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
>>> + StatusParameter)); \
>>> + _ASSERT (!RETURN_ERROR (StatusParameter)); \
>>> + } \
>>> + } \
>>> + } while (FALSE)
>>> +#else
>>> + #define ASSERT_RETURN_ERROR(StatusParameter)
>>> +#endif
>>> +
>>> /**
>>> Macro that calls DebugAssert() if a protocol is already installed in the
>>> handle database.
>>>
>>
>> can I please get a maintainer review for this patch? The rest of the
>> series is ready to go, but it depends on this patch.
>>
>> Thanks!
>> Laszlo
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
2016-10-25 7:54 ` Laszlo Ersek
@ 2016-10-25 10:32 ` Gao, Liming
2016-10-27 2:53 ` Kinney, Michael D
0 siblings, 1 reply; 54+ messages in thread
From: Gao, Liming @ 2016-10-25 10:32 UTC (permalink / raw)
To: Laszlo Ersek, Kinney, Michael D
Cc: edk2-devel-01, Justen, Jordan L, Ard Biesheuvel
Laszlo:
Thanks for your report them. I just investigate MdePkg and MdeModulePkg ones. MdePkg BaseLib should be BASE type. It doesn't depend on UEFI. I will clean up it. MdeModulePkg FrameBufferBltLib is designed for UEFI GOP BLT operation. This library instance type should be UEFI_DRIVER.
I will provide the patch to clean up them.
Thanks
Liming
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Tuesday, October 25, 2016 3:54 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
Cc: edk2-devel-01 <edk2-devel@ml01.01.org>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
On 10/25/16 01:05, Kinney, Michael D wrote:
> Hi Laszlo,
>
> Sorry for the delay. I was traveling last week.
>
> I did see this and I have been thinking about it.
> I think it does make sense to add this new macro
> for libraries of type BASE. I am surprised we did
> not run into an issue before that would have required
> the introduction of this macro earlier. Unless the
> workaround has been to add #include of
> , which makes me think we should
> review BASE libraries to make sure that extra include
> is not present.
I spent a few minutes on the following shell script, to identify such
libraries:
{
# Locate the INF files that have a LIBRARY_CLASS define with a client
# module type list that explicitly includes BASE
git grep -l -E '\' -- \
'*.inf'
# Locate the INF files that have MODULE_TYPE=BASE, and a LIBRARY_CLASS
# define without a client type list.
git grep -l -E '\' -- '*inf' \
| xargs -r -- grep -l -E '\[^|]+$' --
} \
| {
# Cut off the last pathname component, in order to get the pathname of
# the directory containing the INF file
rev | cut -f 2- -d / | rev
} \
| {
# If a directory has several matching INF files, list the directory
# only once.
sort -u
} \
| {
# Check if any file in these directories includes
# "Uefi/UefiBaseType.h".
xargs -r -- grep -r -l Uefi/UefiBaseType.h --
}
It prints the following files:
CorebootModulePkg/Library/CbParseLib/CbParseLib.c
CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.c
MdePkg/Library/BaseLib/FilePaths.c
OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
QuarkSocPkg/QuarkNorthCluster/Library/QNCSmmLib/QNCSmmLib.c
QuarkSocPkg/QuarkNorthCluster/Library/ResetSystemLib/ResetSystemLib.c
We should likely investigate them.
I'll handle the OvmfPkg one.
>
> The EFI_* error codes are mapped to RETURN_* error
> codes. So the only feedback I was considering was
> to implement ASSERT_EFI_ERROR() using
> ASSERT_RETURN_ERROR(), but that might not always be
> the right mapping because the RETURN_* codes are
> a subset of EFI_* error codes.
>
> Reviewed-by: Michael Kinney
Thank you!
Laszlo
> Best regards,
>
> Mike
>
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, October 24, 2016 2:00 PM
>> To: Kinney, Michael D ; Gao, Liming
>>
>> Cc: edk2-devel-01
>> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
>>
>> Mike, Liming,
>>
>> On 10/21/16 23:27, Laszlo Ersek wrote:
>>> ASSERT_EFI_ERROR() cannot be used in BASE type modules because
>>> - the replacement text calls EFI_ERROR(),
>>> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
>>> - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
>>>
>>> While
>>>
>>> ASSERT (!RETURN_ERROR (StatusParameter))
>>>
>>> would be a functional statement in BASE type modules, it would be less
>>> convenient and less informative: ASSERT_EFI_ERROR() prints the actual
>>> StatusParameter.
>>>
>>> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
>>> original macro definition and update it as follows:
>>> - replace EFI with RETURN,
>>> - wrap overlong lines in the comment block and in the code,
>>> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
>>>
>>> Cc: Liming Gao
>>> Cc: Michael D Kinney
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek
>>> ---
>>>
>>> Notes:
>>> OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
>>> one such BASE module.
>>>
>>> MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
>>> 1 file changed, 27 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
>>> index 81904325703f..3a910e6a208b 100644
>>> --- a/MdePkg/Include/Library/DebugLib.h
>>> +++ b/MdePkg/Include/Library/DebugLib.h
>>> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
>>> #define ASSERT_EFI_ERROR(StatusParameter)
>>> #endif
>>>
>>> +/**
>>> + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
>>> +
>>> + If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
>>> + bit of PcdDebugProperyMask is set, then this macro evaluates the
>>> + RETURN_STATUS value specified by StatusParameter. If StatusParameter is an
>>> + error code, then DebugAssert() is called passing in the source filename,
>>> + source line number, and StatusParameter.
>>> +
>>> + @param StatusParameter RETURN_STATUS value to evaluate.
>>> +
>>> +**/
>>> +#if !defined(MDEPKG_NDEBUG)
>>> + #define ASSERT_RETURN_ERROR(StatusParameter) \
>>> + do { \
>>> + if (DebugAssertEnabled ()) { \
>>> + if (RETURN_ERROR (StatusParameter)) { \
>>> + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
>>> + StatusParameter)); \
>>> + _ASSERT (!RETURN_ERROR (StatusParameter)); \
>>> + } \
>>> + } \
>>> + } while (FALSE)
>>> +#else
>>> + #define ASSERT_RETURN_ERROR(StatusParameter)
>>> +#endif
>>> +
>>> /**
>>> Macro that calls DebugAssert() if a protocol is already installed in the
>>> handle database.
>>>
>>
>> can I please get a maintainer review for this patch? The rest of the
>> series is ready to go, but it depends on this patch.
>>
>> Thanks!
>> Laszlo
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
2016-10-25 10:32 ` Gao, Liming
@ 2016-10-27 2:53 ` Kinney, Michael D
0 siblings, 0 replies; 54+ messages in thread
From: Kinney, Michael D @ 2016-10-27 2:53 UTC (permalink / raw)
To: Gao, Liming, Laszlo Ersek, Kinney, Michael D
Cc: edk2-devel-01, Justen, Jordan L, Ard Biesheuvel
Hi Laszlo,
I investigated the QuarkSocPkg ones.
The extra #include of BaseType.h should be removed from:
QuarkSocPkg/QuarkNorthCluster/Library/QNCSmmLib/QNCSmmLib.c
However, it should not be removed from the other one:
QuarkSocPkg/QuarkNorthCluster/Library/ResetSystemLib/ResetSystemLib.c
The ResetSystemLib uses EFI_TIME that is defined in BaseType.h.
I will send patch for QNCSmmLib.
Mike
From: Gao, Liming
Sent: Tuesday, October 25, 2016 3:33 AM
To: Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: edk2-devel-01 <edk2-devel@ml01.01.org>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: RE: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
Laszlo:
Thanks for your report them. I just investigate MdePkg and MdeModulePkg ones. MdePkg BaseLib should be BASE type. It doesn't depend on UEFI. I will clean up it. MdeModulePkg FrameBufferBltLib is designed for UEFI GOP BLT operation. This library instance type should be UEFI_DRIVER.
I will provide the patch to clean up them.
Thanks
Liming
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Tuesday, October 25, 2016 3:54 PM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
Cc: edk2-devel-01 <edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>
Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
On 10/25/16 01:05, Kinney, Michael D wrote:
> Hi Laszlo,
>
> Sorry for the delay. I was traveling last week.
>
> I did see this and I have been thinking about it.
> I think it does make sense to add this new macro
> for libraries of type BASE. I am surprised we did
> not run into an issue before that would have required
> the introduction of this macro earlier. Unless the
> workaround has been to add #include of
> , which makes me think we should
> review BASE libraries to make sure that extra include
> is not present.
I spent a few minutes on the following shell script, to identify such
libraries:
{
# Locate the INF files that have a LIBRARY_CLASS define with a client
# module type list that explicitly includes BASE
git grep -l -E '\' -- \
'*.inf'
# Locate the INF files that have MODULE_TYPE=BASE, and a LIBRARY_CLASS
# define without a client type list.
git grep -l -E '\' -- '*inf' \
| xargs -r -- grep -l -E '\[^|]+$' --
} \
| {
# Cut off the last pathname component, in order to get the pathname of
# the directory containing the INF file
rev | cut -f 2- -d / | rev
} \
| {
# If a directory has several matching INF files, list the directory
# only once.
sort -u
} \
| {
# Check if any file in these directories includes
# "Uefi/UefiBaseType.h".
xargs -r -- grep -r -l Uefi/UefiBaseType.h --
}
It prints the following files:
CorebootModulePkg/Library/CbParseLib/CbParseLib.c
CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.c
MdePkg/Library/BaseLib/FilePaths.c
OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
QuarkSocPkg/QuarkNorthCluster/Library/QNCSmmLib/QNCSmmLib.c
QuarkSocPkg/QuarkNorthCluster/Library/ResetSystemLib/ResetSystemLib.c
We should likely investigate them.
I'll handle the OvmfPkg one.
>
> The EFI_* error codes are mapped to RETURN_* error
> codes. So the only feedback I was considering was
> to implement ASSERT_EFI_ERROR() using
> ASSERT_RETURN_ERROR(), but that might not always be
> the right mapping because the RETURN_* codes are
> a subset of EFI_* error codes.
>
> Reviewed-by: Michael Kinney
Thank you!
Laszlo
> Best regards,
>
> Mike
>
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, October 24, 2016 2:00 PM
>> To: Kinney, Michael D ; Gao, Liming
>>
>> Cc: edk2-devel-01
>> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
>>
>> Mike, Liming,
>>
>> On 10/21/16 23:27, Laszlo Ersek wrote:
>>> ASSERT_EFI_ERROR() cannot be used in BASE type modules because
>>> - the replacement text calls EFI_ERROR(),
>>> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
>>> - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
>>>
>>> While
>>>
>>> ASSERT (!RETURN_ERROR (StatusParameter))
>>>
>>> would be a functional statement in BASE type modules, it would be less
>>> convenient and less informative: ASSERT_EFI_ERROR() prints the actual
>>> StatusParameter.
>>>
>>> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
>>> original macro definition and update it as follows:
>>> - replace EFI with RETURN,
>>> - wrap overlong lines in the comment block and in the code,
>>> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
>>>
>>> Cc: Liming Gao
>>> Cc: Michael D Kinney
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek
>>> ---
>>>
>>> Notes:
>>> OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
>>> one such BASE module.
>>>
>>> MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
>>> 1 file changed, 27 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
>>> index 81904325703f..3a910e6a208b 100644
>>> --- a/MdePkg/Include/Library/DebugLib.h
>>> +++ b/MdePkg/Include/Library/DebugLib.h
>>> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
>>> #define ASSERT_EFI_ERROR(StatusParameter)
>>> #endif
>>>
>>> +/**
>>> + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
>>> +
>>> + If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
>>> + bit of PcdDebugProperyMask is set, then this macro evaluates the
>>> + RETURN_STATUS value specified by StatusParameter. If StatusParameter is an
>>> + error code, then DebugAssert() is called passing in the source filename,
>>> + source line number, and StatusParameter.
>>> +
>>> + @param StatusParameter RETURN_STATUS value to evaluate.
>>> +
>>> +**/
>>> +#if !defined(MDEPKG_NDEBUG)
>>> + #define ASSERT_RETURN_ERROR(StatusParameter) \
>>> + do { \
>>> + if (DebugAssertEnabled ()) { \
>>> + if (RETURN_ERROR (StatusParameter)) { \
>>> + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
>>> + StatusParameter)); \
>>> + _ASSERT (!RETURN_ERROR (StatusParameter)); \
>>> + } \
>>> + } \
>>> + } while (FALSE)
>>> +#else
>>> + #define ASSERT_RETURN_ERROR(StatusParameter)
>>> +#endif
>>> +
>>> /**
>>> Macro that calls DebugAssert() if a protocol is already installed in the
>>> handle database.
>>>
>>
>> can I please get a maintainer review for this patch? The rest of the
>> series is ready to go, but it depends on this patch.
>>
>> Thanks!
>> Laszlo
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
2016-10-24 23:05 ` Kinney, Michael D
2016-10-25 7:54 ` Laszlo Ersek
@ 2016-10-25 8:22 ` Zeng, Star
2016-10-25 8:33 ` Laszlo Ersek
1 sibling, 1 reply; 54+ messages in thread
From: Zeng, Star @ 2016-10-25 8:22 UTC (permalink / raw)
To: Kinney, Michael D, Laszlo Ersek, Gao, Liming; +Cc: edk2-devel-01, Zeng, Star
In fact, EFI_* codes are a subset of RETURN_* codes, so it seems work to implement ASSERT_EFI_ERROR() using new ASSERT_RETURN_ERROR().
Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Kinney, Michael D
Sent: Tuesday, October 25, 2016 7:05 AM
To: Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
Hi Laszlo,
Sorry for the delay. I was traveling last week.
I did see this and I have been thinking about it.
I think it does make sense to add this new macro for libraries of type BASE. I am surprised we did not run into an issue before that would have required the introduction of this macro earlier. Unless the workaround has been to add #include of <Uefi/UefiBaseTypes.h>, which makes me think we should review BASE libraries to make sure that extra include is not present.
The EFI_* error codes are mapped to RETURN_* error codes. So the only feedback I was considering was to implement ASSERT_EFI_ERROR() using ASSERT_RETURN_ERROR(), but that might not always be the right mapping because the RETURN_* codes are a subset of EFI_* error codes.
Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>
Best regards,
Mike
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, October 24, 2016 2:00 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add
> ASSERT_RETURN_ERROR()
>
> Mike, Liming,
>
> On 10/21/16 23:27, Laszlo Ersek wrote:
> > ASSERT_EFI_ERROR() cannot be used in BASE type modules because
> > - the replacement text calls EFI_ERROR(),
> > - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
> > - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
> >
> > While
> >
> > ASSERT (!RETURN_ERROR (StatusParameter))
> >
> > would be a functional statement in BASE type modules, it would be
> > less convenient and less informative: ASSERT_EFI_ERROR() prints the
> > actual StatusParameter.
> >
> > Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR().
> > Copy the original macro definition and update it as follows:
> > - replace EFI with RETURN,
> > - wrap overlong lines in the comment block and in the code,
> > - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
> >
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >
> > Notes:
> > OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
> > one such BASE module.
> >
> > MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/MdePkg/Include/Library/DebugLib.h
> > b/MdePkg/Include/Library/DebugLib.h
> > index 81904325703f..3a910e6a208b 100644
> > --- a/MdePkg/Include/Library/DebugLib.h
> > +++ b/MdePkg/Include/Library/DebugLib.h
> > @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
> > #define ASSERT_EFI_ERROR(StatusParameter) #endif
> >
> > +/**
> > + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
> > +
> > + If MDEPKG_NDEBUG is not defined and the
> > + DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
> > + bit of PcdDebugProperyMask is set, then this macro evaluates the
> > + RETURN_STATUS value specified by StatusParameter. If
> > + StatusParameter is an error code, then DebugAssert() is called
> > + passing in the source filename, source line number, and StatusParameter.
> > +
> > + @param StatusParameter RETURN_STATUS value to evaluate.
> > +
> > +**/
> > +#if !defined(MDEPKG_NDEBUG)
> > + #define ASSERT_RETURN_ERROR(StatusParameter) \
> > + do { \
> > + if (DebugAssertEnabled ()) { \
> > + if (RETURN_ERROR (StatusParameter)) { \
> > + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
> > + StatusParameter)); \
> > + _ASSERT (!RETURN_ERROR (StatusParameter)); \
> > + } \
> > + } \
> > + } while (FALSE)
> > +#else
> > + #define ASSERT_RETURN_ERROR(StatusParameter)
> > +#endif
> > +
> > /**
> > Macro that calls DebugAssert() if a protocol is already installed in the
> > handle database.
> >
>
> can I please get a maintainer review for this patch? The rest of the
> series is ready to go, but it depends on this patch.
>
> Thanks!
> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
2016-10-25 8:22 ` Zeng, Star
@ 2016-10-25 8:33 ` Laszlo Ersek
0 siblings, 0 replies; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-25 8:33 UTC (permalink / raw)
To: Zeng, Star, Kinney, Michael D, Gao, Liming; +Cc: edk2-devel-01
On 10/25/16 10:22, Zeng, Star wrote:
> In fact, EFI_* codes are a subset of RETURN_* codes, so it seems work to implement ASSERT_EFI_ERROR() using new ASSERT_RETURN_ERROR().
Let me commit the patch as-is for now, with Mike's review, and let's
continue the discussion on the above-suggested code sharing. If everyone
agrees, I can submit a separate patch that reuses ASSERT_RETURN_ERROR in
ASSERT_EFI_ERROR.
One thing I'm not so sure about (regarding the code sharing) is that
each of these macros prints its own name, as a literal string. If we
simply redefine ASSERT_EFI_ERROR with ASSERT_RETURN_ERROR, the error
message won't match the source code any longer for the former.
We can get around this by introducing a common base macro that also
takes the name to print as a parameter. But, I think that justifies a
separate patch even more.
Thanks!
Laszlo
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Kinney, Michael D
> Sent: Tuesday, October 25, 2016 7:05 AM
> To: Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
>
> Hi Laszlo,
>
> Sorry for the delay. I was traveling last week.
>
> I did see this and I have been thinking about it.
> I think it does make sense to add this new macro for libraries of type BASE. I am surprised we did not run into an issue before that would have required the introduction of this macro earlier. Unless the workaround has been to add #include of <Uefi/UefiBaseTypes.h>, which makes me think we should review BASE libraries to make sure that extra include is not present.
>
> The EFI_* error codes are mapped to RETURN_* error codes. So the only feedback I was considering was to implement ASSERT_EFI_ERROR() using ASSERT_RETURN_ERROR(), but that might not always be the right mapping because the RETURN_* codes are a subset of EFI_* error codes.
>
> Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>
>
> Best regards,
>
> Mike
>
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, October 24, 2016 2:00 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>
>> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
>> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add
>> ASSERT_RETURN_ERROR()
>>
>> Mike, Liming,
>>
>> On 10/21/16 23:27, Laszlo Ersek wrote:
>>> ASSERT_EFI_ERROR() cannot be used in BASE type modules because
>>> - the replacement text calls EFI_ERROR(),
>>> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
>>> - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
>>>
>>> While
>>>
>>> ASSERT (!RETURN_ERROR (StatusParameter))
>>>
>>> would be a functional statement in BASE type modules, it would be
>>> less convenient and less informative: ASSERT_EFI_ERROR() prints the
>>> actual StatusParameter.
>>>
>>> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR().
>>> Copy the original macro definition and update it as follows:
>>> - replace EFI with RETURN,
>>> - wrap overlong lines in the comment block and in the code,
>>> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
>>>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>> OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
>>> one such BASE module.
>>>
>>> MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
>>> 1 file changed, 27 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Library/DebugLib.h
>>> b/MdePkg/Include/Library/DebugLib.h
>>> index 81904325703f..3a910e6a208b 100644
>>> --- a/MdePkg/Include/Library/DebugLib.h
>>> +++ b/MdePkg/Include/Library/DebugLib.h
>>> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
>>> #define ASSERT_EFI_ERROR(StatusParameter) #endif
>>>
>>> +/**
>>> + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
>>> +
>>> + If MDEPKG_NDEBUG is not defined and the
>>> + DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
>>> + bit of PcdDebugProperyMask is set, then this macro evaluates the
>>> + RETURN_STATUS value specified by StatusParameter. If
>>> + StatusParameter is an error code, then DebugAssert() is called
>>> + passing in the source filename, source line number, and StatusParameter.
>>> +
>>> + @param StatusParameter RETURN_STATUS value to evaluate.
>>> +
>>> +**/
>>> +#if !defined(MDEPKG_NDEBUG)
>>> + #define ASSERT_RETURN_ERROR(StatusParameter) \
>>> + do { \
>>> + if (DebugAssertEnabled ()) { \
>>> + if (RETURN_ERROR (StatusParameter)) { \
>>> + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
>>> + StatusParameter)); \
>>> + _ASSERT (!RETURN_ERROR (StatusParameter)); \
>>> + } \
>>> + } \
>>> + } while (FALSE)
>>> +#else
>>> + #define ASSERT_RETURN_ERROR(StatusParameter)
>>> +#endif
>>> +
>>> /**
>>> Macro that calls DebugAssert() if a protocol is already installed in the
>>> handle database.
>>>
>>
>> can I please get a maintainer review for this patch? The rest of the
>> series is ready to go, but it depends on this patch.
>>
>> Thanks!
>> Laszlo
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 02/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
2016-10-21 21:27 ` [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 4:44 ` Gary Lin
2016-10-21 21:27 ` [PATCH 03/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls Laszlo Ersek
` (19 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Anthony PERARD, Gary Lin, Jordan Justen
AsciiStrCpy() is deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
not used in my setup, testing would be appreciated
OvmfPkg/XenBusDxe/XenStore.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index fb00e7393bb1..aa3ff7d3017b 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -1298,11 +1298,8 @@ XenStoreTransactionEnd (
{
CHAR8 AbortStr[2];
- if (Abort) {
- AsciiStrCpy (AbortStr, "F");
- } else {
- AsciiStrCpy (AbortStr, "T");
- }
+ AbortStr[0] = Abort ? 'F' : 'T';
+ AbortStr[1] = '\0';
return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL);
}
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 02/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls
2016-10-21 21:27 ` [PATCH 02/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls Laszlo Ersek
@ 2016-10-24 4:44 ` Gary Lin
0 siblings, 0 replies; 54+ messages in thread
From: Gary Lin @ 2016-10-24 4:44 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Anthony PERARD, Jordan Justen
On Fri, Oct 21, 2016 at 11:27:20PM +0200, Laszlo Ersek wrote:
> AsciiStrCpy() is deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> not used in my setup, testing would be appreciated
>
> OvmfPkg/XenBusDxe/XenStore.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index fb00e7393bb1..aa3ff7d3017b 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -1298,11 +1298,8 @@ XenStoreTransactionEnd (
> {
> CHAR8 AbortStr[2];
>
> - if (Abort) {
> - AsciiStrCpy (AbortStr, "F");
> - } else {
> - AsciiStrCpy (AbortStr, "T");
> - }
> + AbortStr[0] = Abort ? 'F' : 'T';
> + AbortStr[1] = '\0';
>
> return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL);
> }
> --
> 2.9.2
>
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 03/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
2016-10-21 21:27 ` [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() Laszlo Ersek
2016-10-21 21:27 ` [PATCH 02/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 4:44 ` Gary Lin
2016-10-21 21:27 ` [PATCH 04/19] OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls Laszlo Ersek
` (18 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Anthony PERARD, Gary Lin, Jordan Justen
AsciiStrCat() is deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Replace AsciiStrCat() with AsciiSPrint(). Spell out the (already existent)
PrintLib dependency in the INF file. Add an explicit ASSERT() to document
that XenStoreJoin() assumes that the pool allocation always succeeds.
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
not used in my setup, testing would be appreciated
OvmfPkg/XenBusDxe/XenBusDxe.inf | 1 +
OvmfPkg/XenBusDxe/XenStore.c | 15 +++++++++------
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
index f0c5db98b1f4..5ff1cd04840c 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
@@ -56,6 +56,7 @@ [LibraryClasses]
DebugLib
XenHypercallLib
SynchronizationLib
+ PrintLib
[Protocols]
gEfiDriverBindingProtocolGuid
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index aa3ff7d3017b..b7ae1d04863d 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -303,14 +303,17 @@ XenStoreJoin (
)
{
CHAR8 *Buf;
+ UINTN BufSize;
/* +1 for '/' and +1 for '\0' */
- Buf = AllocateZeroPool (
- AsciiStrLen (DirectoryPath) + AsciiStrLen (Node) + 2);
- AsciiStrCat (Buf, DirectoryPath);
- if (Node[0] != '\0') {
- AsciiStrCat (Buf, "/");
- AsciiStrCat (Buf, Node);
+ BufSize = AsciiStrLen (DirectoryPath) + AsciiStrLen (Node) + 2;
+ Buf = AllocatePool (BufSize);
+ ASSERT (Buf != NULL);
+
+ if (Node[0] == '\0') {
+ AsciiSPrint (Buf, BufSize, "%a", DirectoryPath);
+ } else {
+ AsciiSPrint (Buf, BufSize, "%a/%a", DirectoryPath, Node);
}
return Buf;
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 03/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls
2016-10-21 21:27 ` [PATCH 03/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls Laszlo Ersek
@ 2016-10-24 4:44 ` Gary Lin
0 siblings, 0 replies; 54+ messages in thread
From: Gary Lin @ 2016-10-24 4:44 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Anthony PERARD, Jordan Justen
On Fri, Oct 21, 2016 at 11:27:21PM +0200, Laszlo Ersek wrote:
> AsciiStrCat() is deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Replace AsciiStrCat() with AsciiSPrint(). Spell out the (already existent)
> PrintLib dependency in the INF file. Add an explicit ASSERT() to document
> that XenStoreJoin() assumes that the pool allocation always succeeds.
>
Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> not used in my setup, testing would be appreciated
>
> OvmfPkg/XenBusDxe/XenBusDxe.inf | 1 +
> OvmfPkg/XenBusDxe/XenStore.c | 15 +++++++++------
> 2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> index f0c5db98b1f4..5ff1cd04840c 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> @@ -56,6 +56,7 @@ [LibraryClasses]
> DebugLib
> XenHypercallLib
> SynchronizationLib
> + PrintLib
>
> [Protocols]
> gEfiDriverBindingProtocolGuid
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index aa3ff7d3017b..b7ae1d04863d 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -303,14 +303,17 @@ XenStoreJoin (
> )
> {
> CHAR8 *Buf;
> + UINTN BufSize;
>
> /* +1 for '/' and +1 for '\0' */
> - Buf = AllocateZeroPool (
> - AsciiStrLen (DirectoryPath) + AsciiStrLen (Node) + 2);
> - AsciiStrCat (Buf, DirectoryPath);
> - if (Node[0] != '\0') {
> - AsciiStrCat (Buf, "/");
> - AsciiStrCat (Buf, Node);
> + BufSize = AsciiStrLen (DirectoryPath) + AsciiStrLen (Node) + 2;
> + Buf = AllocatePool (BufSize);
> + ASSERT (Buf != NULL);
> +
> + if (Node[0] == '\0') {
> + AsciiSPrint (Buf, BufSize, "%a", DirectoryPath);
> + } else {
> + AsciiSPrint (Buf, BufSize, "%a/%a", DirectoryPath, Node);
> }
>
> return Buf;
> --
> 2.9.2
>
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 04/19] OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (2 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 03/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 4:45 ` Gary Lin
2016-10-21 21:27 ` [PATCH 05/19] OvmfPkg/PlatformBootManagerLib: " Laszlo Ersek
` (17 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Anthony PERARD, Gary Lin, Jordan Justen
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
not used in my setup, testing would be appreciated
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
index 7a8beb3354b9..dec6d4af50df 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
@@ -803,6 +803,7 @@ FvbInitialize (
BOOLEAN Initialize;
EFI_HANDLE Handle;
EFI_PHYSICAL_ADDRESS Address;
+ RETURN_STATUS PcdStatus;
DEBUG ((EFI_D_INFO, "EMU Variable FVB Started\n"));
@@ -862,19 +863,24 @@ FvbInitialize (
SetMem (Ptr, EMU_FVB_SIZE, ERASED_UINT8);
InitializeFvAndVariableStoreHeaders (Ptr);
}
- PcdSet64 (PcdFlashNvStorageVariableBase64, (UINT32)(UINTN) Ptr);
+ PcdStatus = PcdSet64S (PcdFlashNvStorageVariableBase64, (UINT32)(UINTN) Ptr);
+ ASSERT_RETURN_ERROR (PcdStatus);
//
// Initialize the Fault Tolerant Write data area
//
SubPtr = (VOID*) ((UINT8*) Ptr + PcdGet32 (PcdVariableStoreSize));
- PcdSet32 (PcdFlashNvStorageFtwWorkingBase, (UINT32)(UINTN) SubPtr);
+ PcdStatus = PcdSet32S (PcdFlashNvStorageFtwWorkingBase,
+ (UINT32)(UINTN) SubPtr);
+ ASSERT_RETURN_ERROR (PcdStatus);
//
// Initialize the Fault Tolerant Write spare block
//
SubPtr = (VOID*) ((UINT8*) Ptr + EMU_FVB_BLOCK_SIZE);
- PcdSet32 (PcdFlashNvStorageFtwSpareBase, (UINT32)(UINTN) SubPtr);
+ PcdStatus = PcdSet32S (PcdFlashNvStorageFtwSpareBase,
+ (UINT32)(UINTN) SubPtr);
+ ASSERT_RETURN_ERROR (PcdStatus);
//
// Setup FVB device path
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 04/19] OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 ` [PATCH 04/19] OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls Laszlo Ersek
@ 2016-10-24 4:45 ` Gary Lin
0 siblings, 0 replies; 54+ messages in thread
From: Gary Lin @ 2016-10-24 4:45 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Anthony PERARD, Jordan Justen
On Fri, Oct 21, 2016 at 11:27:22PM +0200, Laszlo Ersek wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> not used in my setup, testing would be appreciated
>
> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> index 7a8beb3354b9..dec6d4af50df 100644
> --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> @@ -803,6 +803,7 @@ FvbInitialize (
> BOOLEAN Initialize;
> EFI_HANDLE Handle;
> EFI_PHYSICAL_ADDRESS Address;
> + RETURN_STATUS PcdStatus;
>
> DEBUG ((EFI_D_INFO, "EMU Variable FVB Started\n"));
>
> @@ -862,19 +863,24 @@ FvbInitialize (
> SetMem (Ptr, EMU_FVB_SIZE, ERASED_UINT8);
> InitializeFvAndVariableStoreHeaders (Ptr);
> }
> - PcdSet64 (PcdFlashNvStorageVariableBase64, (UINT32)(UINTN) Ptr);
> + PcdStatus = PcdSet64S (PcdFlashNvStorageVariableBase64, (UINT32)(UINTN) Ptr);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> //
> // Initialize the Fault Tolerant Write data area
> //
> SubPtr = (VOID*) ((UINT8*) Ptr + PcdGet32 (PcdVariableStoreSize));
> - PcdSet32 (PcdFlashNvStorageFtwWorkingBase, (UINT32)(UINTN) SubPtr);
> + PcdStatus = PcdSet32S (PcdFlashNvStorageFtwWorkingBase,
> + (UINT32)(UINTN) SubPtr);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> //
> // Initialize the Fault Tolerant Write spare block
> //
> SubPtr = (VOID*) ((UINT8*) Ptr + EMU_FVB_BLOCK_SIZE);
> - PcdSet32 (PcdFlashNvStorageFtwSpareBase, (UINT32)(UINTN) SubPtr);
> + PcdStatus = PcdSet32S (PcdFlashNvStorageFtwSpareBase,
> + (UINT32)(UINTN) SubPtr);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> //
> // Setup FVB device path
> --
> 2.9.2
>
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 05/19] OvmfPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (3 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 04/19] OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 4:45 ` Gary Lin
2016-10-21 21:27 ` [PATCH 06/19] OvmfPkg/SmbiosVersionLib: " Laszlo Ersek
` (16 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Anthony PERARD, Gary Lin, Jordan Justen
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
the emulated variable part of the patch is un-exercised on my setup,
testing would be appreciated
OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index bd1a4396f1d3..29ce21282595 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -353,8 +353,9 @@ Returns:
--*/
{
- EFI_HANDLE Handle;
- EFI_STATUS Status;
+ EFI_HANDLE Handle;
+ EFI_STATUS Status;
+ RETURN_STATUS PcdStatus;
DEBUG ((EFI_D_INFO, "PlatformBootManagerBeforeConsole\n"));
InstallDevicePathCallback ();
@@ -394,7 +395,9 @@ Returns:
ASSERT_EFI_ERROR (Status);
PlatformInitializeConsole (gPlatformConsole);
- PcdSet16 (PcdPlatformBootTimeOut, GetFrontPageTimeoutFromQemu ());
+ PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
+ GetFrontPageTimeoutFromQemu ());
+ ASSERT_RETURN_ERROR (PcdStatus);
PlatformRegisterOptionsAndKeys ();
}
@@ -1281,6 +1284,7 @@ VisitingFileSystemInstance (
{
EFI_STATUS Status;
STATIC BOOLEAN ConnectedToFileSystem = FALSE;
+ RETURN_STATUS PcdStatus;
if (ConnectedToFileSystem) {
return EFI_ALREADY_STARTED;
@@ -1300,7 +1304,9 @@ VisitingFileSystemInstance (
NULL,
&mEmuVariableEventReg
);
- PcdSet64 (PcdEmuVariableEvent, (UINT64)(UINTN) mEmuVariableEvent);
+ PcdStatus = PcdSet64S (PcdEmuVariableEvent,
+ (UINT64)(UINTN) mEmuVariableEvent);
+ ASSERT_RETURN_ERROR (PcdStatus);
return EFI_SUCCESS;
}
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 05/19] OvmfPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 ` [PATCH 05/19] OvmfPkg/PlatformBootManagerLib: " Laszlo Ersek
@ 2016-10-24 4:45 ` Gary Lin
0 siblings, 0 replies; 54+ messages in thread
From: Gary Lin @ 2016-10-24 4:45 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Anthony PERARD, Jordan Justen
On Fri, Oct 21, 2016 at 11:27:23PM +0200, Laszlo Ersek wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> the emulated variable part of the patch is un-exercised on my setup,
> testing would be appreciated
>
> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index bd1a4396f1d3..29ce21282595 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -353,8 +353,9 @@ Returns:
>
> --*/
> {
> - EFI_HANDLE Handle;
> - EFI_STATUS Status;
> + EFI_HANDLE Handle;
> + EFI_STATUS Status;
> + RETURN_STATUS PcdStatus;
>
> DEBUG ((EFI_D_INFO, "PlatformBootManagerBeforeConsole\n"));
> InstallDevicePathCallback ();
> @@ -394,7 +395,9 @@ Returns:
> ASSERT_EFI_ERROR (Status);
>
> PlatformInitializeConsole (gPlatformConsole);
> - PcdSet16 (PcdPlatformBootTimeOut, GetFrontPageTimeoutFromQemu ());
> + PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
> + GetFrontPageTimeoutFromQemu ());
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> PlatformRegisterOptionsAndKeys ();
> }
> @@ -1281,6 +1284,7 @@ VisitingFileSystemInstance (
> {
> EFI_STATUS Status;
> STATIC BOOLEAN ConnectedToFileSystem = FALSE;
> + RETURN_STATUS PcdStatus;
>
> if (ConnectedToFileSystem) {
> return EFI_ALREADY_STARTED;
> @@ -1300,7 +1304,9 @@ VisitingFileSystemInstance (
> NULL,
> &mEmuVariableEventReg
> );
> - PcdSet64 (PcdEmuVariableEvent, (UINT64)(UINTN) mEmuVariableEvent);
> + PcdStatus = PcdSet64S (PcdEmuVariableEvent,
> + (UINT64)(UINTN) mEmuVariableEvent);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> return EFI_SUCCESS;
> }
> --
> 2.9.2
>
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 06/19] OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (4 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 05/19] OvmfPkg/PlatformBootManagerLib: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 8:00 ` Ard Biesheuvel
2016-10-21 21:27 ` [PATCH 07/19] OvmfPkg/PlatformDxe: " Laszlo Ersek
` (15 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
tested with OVMF (smbios v2) and ArmVirtQemu (smbios v3)
OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
index 950c3f7e0a31..58180412bd1a 100644
--- a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
+++ b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
@@ -40,6 +40,7 @@ DetectSmbiosVersion (
UINTN AnchorSize, TablesSize;
QEMU_SMBIOS_ANCHOR QemuAnchor;
UINT16 SmbiosVersion;
+ RETURN_STATUS PcdStatus;
if (PcdGetBool (PcdQemuSmbiosValidated)) {
//
@@ -87,7 +88,8 @@ DetectSmbiosVersion (
DEBUG ((EFI_D_INFO, "%a: SMBIOS 3.x DocRev from QEMU: 0x%02x\n",
__FUNCTION__, QemuAnchor.V3.DocRev));
- PcdSet8 (PcdSmbiosDocRev, QemuAnchor.V3.DocRev);
+ PcdStatus = PcdSet8S (PcdSmbiosDocRev, QemuAnchor.V3.DocRev);
+ ASSERT_RETURN_ERROR (PcdStatus);
break;
default:
@@ -96,12 +98,14 @@ DetectSmbiosVersion (
DEBUG ((EFI_D_INFO, "%a: SMBIOS version from QEMU: 0x%04x\n", __FUNCTION__,
SmbiosVersion));
- PcdSet16 (PcdSmbiosVersion, SmbiosVersion);
+ PcdStatus = PcdSet16S (PcdSmbiosVersion, SmbiosVersion);
+ ASSERT_RETURN_ERROR (PcdStatus);
//
// SMBIOS platform drivers can now fetch and install
// "etc/smbios/smbios-tables" from QEMU.
//
- PcdSetBool (PcdQemuSmbiosValidated, TRUE);
+ PcdStatus = PcdSetBoolS (PcdQemuSmbiosValidated, TRUE);
+ ASSERT_RETURN_ERROR (PcdStatus);
return RETURN_SUCCESS;
}
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 06/19] OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 ` [PATCH 06/19] OvmfPkg/SmbiosVersionLib: " Laszlo Ersek
@ 2016-10-24 8:00 ` Ard Biesheuvel
0 siblings, 0 replies; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 8:00 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> Notes:
> tested with OVMF (smbios v2) and ArmVirtQemu (smbios v3)
>
> OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
> index 950c3f7e0a31..58180412bd1a 100644
> --- a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
> +++ b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
> @@ -40,6 +40,7 @@ DetectSmbiosVersion (
> UINTN AnchorSize, TablesSize;
> QEMU_SMBIOS_ANCHOR QemuAnchor;
> UINT16 SmbiosVersion;
> + RETURN_STATUS PcdStatus;
>
> if (PcdGetBool (PcdQemuSmbiosValidated)) {
> //
> @@ -87,7 +88,8 @@ DetectSmbiosVersion (
>
> DEBUG ((EFI_D_INFO, "%a: SMBIOS 3.x DocRev from QEMU: 0x%02x\n",
> __FUNCTION__, QemuAnchor.V3.DocRev));
> - PcdSet8 (PcdSmbiosDocRev, QemuAnchor.V3.DocRev);
> + PcdStatus = PcdSet8S (PcdSmbiosDocRev, QemuAnchor.V3.DocRev);
> + ASSERT_RETURN_ERROR (PcdStatus);
> break;
>
> default:
> @@ -96,12 +98,14 @@ DetectSmbiosVersion (
>
> DEBUG ((EFI_D_INFO, "%a: SMBIOS version from QEMU: 0x%04x\n", __FUNCTION__,
> SmbiosVersion));
> - PcdSet16 (PcdSmbiosVersion, SmbiosVersion);
> + PcdStatus = PcdSet16S (PcdSmbiosVersion, SmbiosVersion);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> //
> // SMBIOS platform drivers can now fetch and install
> // "etc/smbios/smbios-tables" from QEMU.
> //
> - PcdSetBool (PcdQemuSmbiosValidated, TRUE);
> + PcdStatus = PcdSetBoolS (PcdQemuSmbiosValidated, TRUE);
> + ASSERT_RETURN_ERROR (PcdStatus);
> return RETURN_SUCCESS;
> }
> --
> 2.9.2
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 07/19] OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (5 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 06/19] OvmfPkg/SmbiosVersionLib: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-21 21:27 ` [PATCH 08/19] OvmfPkg/PlatformPei: " Laszlo Ersek
` (14 subsequent siblings)
21 siblings, 0 replies; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/PlatformDxe/Platform.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
index 4ec327e763f6..126d8e73823b 100644
--- a/OvmfPkg/PlatformDxe/Platform.c
+++ b/OvmfPkg/PlatformDxe/Platform.c
@@ -663,6 +663,7 @@ ExecutePlatformConfig (
EFI_STATUS Status;
PLATFORM_CONFIG PlatformConfig;
UINT64 OptionalElements;
+ RETURN_STATUS PcdStatus;
Status = PlatformConfigLoad (&PlatformConfig, &OptionalElements);
if (EFI_ERROR (Status)) {
@@ -675,10 +676,13 @@ ExecutePlatformConfig (
//
// Pass the preferred resolution to GraphicsConsoleDxe via dynamic PCDs.
//
- PcdSet32 (PcdVideoHorizontalResolution,
+ PcdStatus = PcdSet32S (PcdVideoHorizontalResolution,
PlatformConfig.HorizontalResolution);
- PcdSet32 (PcdVideoVerticalResolution,
+ ASSERT_RETURN_ERROR (PcdStatus);
+
+ PcdStatus = PcdSet32S (PcdVideoVerticalResolution,
PlatformConfig.VerticalResolution);
+ ASSERT_RETURN_ERROR (PcdStatus);
}
return EFI_SUCCESS;
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH 08/19] OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (6 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 07/19] OvmfPkg/PlatformDxe: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 4:45 ` Gary Lin
2016-10-21 21:27 ` [PATCH 09/19] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: " Laszlo Ersek
` (13 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Anthony PERARD, Gary Lin, Jordan Justen
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
The ReserveEmuVariableNvStore() and InitializeXen() hunks make no
difference in my setup, so testing for those would be appreciated.
OvmfPkg/PlatformPei/MemDetect.c | 11 +++--
OvmfPkg/PlatformPei/Platform.c | 44 +++++++++++++-------
OvmfPkg/PlatformPei/Xen.c | 5 ++-
3 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 4863eb101067..d00a570d4381 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -107,6 +107,7 @@ GetFirstNonAddress (
FIRMWARE_CONFIG_ITEM FwCfgItem;
UINTN FwCfgSize;
UINT64 HotPlugMemoryEnd;
+ RETURN_STATUS PcdStatus;
FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
@@ -154,7 +155,8 @@ GetFirstNonAddress (
if (mBootMode != BOOT_ON_S3_RESUME) {
DEBUG ((EFI_D_INFO, "%a: disabling 64-bit PCI host aperture\n",
__FUNCTION__));
- PcdSet64 (PcdPciMmio64Size, 0);
+ PcdStatus = PcdSet64S (PcdPciMmio64Size, 0);
+ ASSERT_RETURN_ERROR (PcdStatus);
}
//
@@ -202,8 +204,11 @@ GetFirstNonAddress (
// the GCD memory space map through our PciHostBridgeLib instance; here we
// only need to set the PCDs.
//
- PcdSet64 (PcdPciMmio64Base, Pci64Base);
- PcdSet64 (PcdPciMmio64Size, Pci64Size);
+ PcdStatus = PcdSet64S (PcdPciMmio64Base, Pci64Base);
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet64S (PcdPciMmio64Size, Pci64Size);
+ ASSERT_RETURN_ERROR (PcdStatus);
+
DEBUG ((EFI_D_INFO, "%a: Pci64Base=0x%Lx Pci64Size=0x%Lx\n",
__FUNCTION__, Pci64Base, Pci64Size));
}
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index ca1e6dc7e320..c6e1106c9ed0 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -156,8 +156,9 @@ MemMapInitialization (
VOID
)
{
- UINT64 PciIoBase;
- UINT64 PciIoSize;
+ UINT64 PciIoBase;
+ UINT64 PciIoSize;
+ RETURN_STATUS PcdStatus;
PciIoBase = 0xC000;
PciIoSize = 0x4000;
@@ -212,8 +213,11 @@ MemMapInitialization (
//
PciSize = 0xFC000000 - PciBase;
AddIoMemoryBaseSizeHob (PciBase, PciSize);
- PcdSet64 (PcdPciMmio32Base, PciBase);
- PcdSet64 (PcdPciMmio32Size, PciSize);
+ PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet64S (PcdPciMmio32Size, PciSize);
+ ASSERT_RETURN_ERROR (PcdStatus);
+
AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
@@ -266,8 +270,10 @@ MemMapInitialization (
PciIoBase,
PciIoSize
);
- PcdSet64 (PcdPciIoBase, PciIoBase);
- PcdSet64 (PcdPciIoSize, PciIoSize);
+ PcdStatus = PcdSet64S (PcdPciIoBase, PciIoBase);
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet64S (PcdPciIoSize, PciIoSize);
+ ASSERT_RETURN_ERROR (PcdStatus);
}
EFI_STATUS
@@ -316,11 +322,13 @@ GetNamedFwCfgBoolean (
#define UPDATE_BOOLEAN_PCD_FROM_FW_CFG(TokenName) \
do { \
- BOOLEAN Setting; \
+ BOOLEAN Setting; \
+ RETURN_STATUS PcdStatus; \
\
if (!EFI_ERROR (GetNamedFwCfgBoolean ( \
"opt/ovmf/" #TokenName, &Setting))) { \
- PcdSetBool (TokenName, Setting); \
+ PcdStatus = PcdSetBoolS (TokenName, Setting); \
+ ASSERT_RETURN_ERROR (PcdStatus); \
} \
} while (0)
@@ -379,12 +387,13 @@ MiscInitialization (
VOID
)
{
- UINTN PmCmd;
- UINTN Pmba;
- UINT32 PmbaAndVal;
- UINT32 PmbaOrVal;
- UINTN AcpiCtlReg;
- UINT8 AcpiEnBit;
+ UINTN PmCmd;
+ UINTN Pmba;
+ UINT32 PmbaAndVal;
+ UINT32 PmbaOrVal;
+ UINTN AcpiCtlReg;
+ UINT8 AcpiEnBit;
+ RETURN_STATUS PcdStatus;
//
// Disable A20 Mask
@@ -424,7 +433,8 @@ MiscInitialization (
ASSERT (FALSE);
return;
}
- PcdSet16 (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
+ PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
+ ASSERT_RETURN_ERROR (PcdStatus);
//
// If the appropriate IOspace enable bit is set, assume the ACPI PMBA
@@ -491,6 +501,7 @@ ReserveEmuVariableNvStore (
)
{
EFI_PHYSICAL_ADDRESS VariableStore;
+ RETURN_STATUS PcdStatus;
//
// Allocate storage for NV variables early on so it will be
@@ -509,7 +520,8 @@ ReserveEmuVariableNvStore (
VariableStore,
(2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
));
- PcdSet64 (PcdEmuVariableNvStoreReserved, VariableStore);
+ PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
+ ASSERT_RETURN_ERROR (PcdStatus);
}
diff --git a/OvmfPkg/PlatformPei/Xen.c b/OvmfPkg/PlatformPei/Xen.c
index 223908a4f529..ab38f97a67aa 100644
--- a/OvmfPkg/PlatformPei/Xen.c
+++ b/OvmfPkg/PlatformPei/Xen.c
@@ -210,6 +210,8 @@ InitializeXen (
VOID
)
{
+ RETURN_STATUS PcdStatus;
+
if (mXenLeaf == 0) {
return EFI_NOT_FOUND;
}
@@ -222,7 +224,8 @@ InitializeXen (
//
AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE);
- PcdSetBool (PcdPciDisableBusEnumeration, TRUE);
+ PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE);
+ ASSERT_RETURN_ERROR (PcdStatus);
return EFI_SUCCESS;
}
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 08/19] OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 ` [PATCH 08/19] OvmfPkg/PlatformPei: " Laszlo Ersek
@ 2016-10-24 4:45 ` Gary Lin
2016-10-24 11:51 ` Laszlo Ersek
0 siblings, 1 reply; 54+ messages in thread
From: Gary Lin @ 2016-10-24 4:45 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Anthony PERARD, Jordan Justen
On Fri, Oct 21, 2016 at 11:27:26PM +0200, Laszlo Ersek wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> The ReserveEmuVariableNvStore() and InitializeXen() hunks make no
> difference in my setup, so testing for those would be appreciated.
>
> OvmfPkg/PlatformPei/MemDetect.c | 11 +++--
> OvmfPkg/PlatformPei/Platform.c | 44 +++++++++++++-------
> OvmfPkg/PlatformPei/Xen.c | 5 ++-
> 3 files changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 4863eb101067..d00a570d4381 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -107,6 +107,7 @@ GetFirstNonAddress (
> FIRMWARE_CONFIG_ITEM FwCfgItem;
> UINTN FwCfgSize;
> UINT64 HotPlugMemoryEnd;
> + RETURN_STATUS PcdStatus;
>
> FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
>
> @@ -154,7 +155,8 @@ GetFirstNonAddress (
> if (mBootMode != BOOT_ON_S3_RESUME) {
> DEBUG ((EFI_D_INFO, "%a: disabling 64-bit PCI host aperture\n",
> __FUNCTION__));
> - PcdSet64 (PcdPciMmio64Size, 0);
> + PcdStatus = PcdSet64S (PcdPciMmio64Size, 0);
> + ASSERT_RETURN_ERROR (PcdStatus);
> }
>
> //
> @@ -202,8 +204,11 @@ GetFirstNonAddress (
> // the GCD memory space map through our PciHostBridgeLib instance; here we
> // only need to set the PCDs.
> //
> - PcdSet64 (PcdPciMmio64Base, Pci64Base);
> - PcdSet64 (PcdPciMmio64Size, Pci64Size);
> + PcdStatus = PcdSet64S (PcdPciMmio64Base, Pci64Base);
> + ASSERT_RETURN_ERROR (PcdStatus);
> + PcdStatus = PcdSet64S (PcdPciMmio64Size, Pci64Size);
> + ASSERT_RETURN_ERROR (PcdStatus);
> +
> DEBUG ((EFI_D_INFO, "%a: Pci64Base=0x%Lx Pci64Size=0x%Lx\n",
> __FUNCTION__, Pci64Base, Pci64Size));
> }
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index ca1e6dc7e320..c6e1106c9ed0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -156,8 +156,9 @@ MemMapInitialization (
> VOID
> )
> {
> - UINT64 PciIoBase;
> - UINT64 PciIoSize;
> + UINT64 PciIoBase;
> + UINT64 PciIoSize;
> + RETURN_STATUS PcdStatus;
>
> PciIoBase = 0xC000;
> PciIoSize = 0x4000;
> @@ -212,8 +213,11 @@ MemMapInitialization (
> //
> PciSize = 0xFC000000 - PciBase;
> AddIoMemoryBaseSizeHob (PciBase, PciSize);
> - PcdSet64 (PcdPciMmio32Base, PciBase);
> - PcdSet64 (PcdPciMmio32Size, PciSize);
> + PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
> + ASSERT_RETURN_ERROR (PcdStatus);
> + PcdStatus = PcdSet64S (PcdPciMmio32Size, PciSize);
> + ASSERT_RETURN_ERROR (PcdStatus);
> +
> AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
> AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
> if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> @@ -266,8 +270,10 @@ MemMapInitialization (
> PciIoBase,
> PciIoSize
> );
> - PcdSet64 (PcdPciIoBase, PciIoBase);
> - PcdSet64 (PcdPciIoSize, PciIoSize);
> + PcdStatus = PcdSet64S (PcdPciIoBase, PciIoBase);
> + ASSERT_RETURN_ERROR (PcdStatus);
> + PcdStatus = PcdSet64S (PcdPciIoSize, PciIoSize);
> + ASSERT_RETURN_ERROR (PcdStatus);
> }
>
> EFI_STATUS
> @@ -316,11 +322,13 @@ GetNamedFwCfgBoolean (
>
> #define UPDATE_BOOLEAN_PCD_FROM_FW_CFG(TokenName) \
> do { \
> - BOOLEAN Setting; \
> + BOOLEAN Setting; \
> + RETURN_STATUS PcdStatus; \
> \
> if (!EFI_ERROR (GetNamedFwCfgBoolean ( \
> "opt/ovmf/" #TokenName, &Setting))) { \
> - PcdSetBool (TokenName, Setting); \
> + PcdStatus = PcdSetBoolS (TokenName, Setting); \
> + ASSERT_RETURN_ERROR (PcdStatus); \
> } \
> } while (0)
>
> @@ -379,12 +387,13 @@ MiscInitialization (
> VOID
> )
> {
> - UINTN PmCmd;
> - UINTN Pmba;
> - UINT32 PmbaAndVal;
> - UINT32 PmbaOrVal;
> - UINTN AcpiCtlReg;
> - UINT8 AcpiEnBit;
> + UINTN PmCmd;
> + UINTN Pmba;
> + UINT32 PmbaAndVal;
> + UINT32 PmbaOrVal;
> + UINTN AcpiCtlReg;
> + UINT8 AcpiEnBit;
> + RETURN_STATUS PcdStatus;
>
> //
> // Disable A20 Mask
> @@ -424,7 +433,8 @@ MiscInitialization (
> ASSERT (FALSE);
> return;
> }
> - PcdSet16 (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
> + PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> //
> // If the appropriate IOspace enable bit is set, assume the ACPI PMBA
> @@ -491,6 +501,7 @@ ReserveEmuVariableNvStore (
> )
> {
> EFI_PHYSICAL_ADDRESS VariableStore;
> + RETURN_STATUS PcdStatus;
>
> //
> // Allocate storage for NV variables early on so it will be
> @@ -509,7 +520,8 @@ ReserveEmuVariableNvStore (
> VariableStore,
> (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
> ));
> - PcdSet64 (PcdEmuVariableNvStoreReserved, VariableStore);
> + PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
> + ASSERT_RETURN_ERROR (PcdStatus);
> }
>
>
> diff --git a/OvmfPkg/PlatformPei/Xen.c b/OvmfPkg/PlatformPei/Xen.c
> index 223908a4f529..ab38f97a67aa 100644
> --- a/OvmfPkg/PlatformPei/Xen.c
> +++ b/OvmfPkg/PlatformPei/Xen.c
> @@ -210,6 +210,8 @@ InitializeXen (
> VOID
> )
> {
> + RETURN_STATUS PcdStatus;
> +
> if (mXenLeaf == 0) {
> return EFI_NOT_FOUND;
> }
> @@ -222,7 +224,8 @@ InitializeXen (
> //
> AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE);
>
> - PcdSetBool (PcdPciDisableBusEnumeration, TRUE);
> + PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> return EFI_SUCCESS;
> }
> --
> 2.9.2
>
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH 08/19] OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls
2016-10-24 4:45 ` Gary Lin
@ 2016-10-24 11:51 ` Laszlo Ersek
0 siblings, 0 replies; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-24 11:51 UTC (permalink / raw)
To: Gary Lin; +Cc: edk2-devel-01, Anthony PERARD, Jordan Justen
On 10/24/16 06:45, Gary Lin wrote:
> On Fri, Oct 21, 2016 at 11:27:26PM +0200, Laszlo Ersek wrote:
>> These are deprecated / disabled under the
>> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>>
>> Introduce a variable called PcdStatus, and use it to assert the success of
>> these operations (there is no reason for them to fail here).
>>
>
> Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>
Thank you, Gary!
Laszlo
>
>> Cc: Anthony PERARD <anthony.perard@citrix.com>
>> Cc: Gary Lin <glin@suse.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>> The ReserveEmuVariableNvStore() and InitializeXen() hunks make no
>> difference in my setup, so testing for those would be appreciated.
>>
>> OvmfPkg/PlatformPei/MemDetect.c | 11 +++--
>> OvmfPkg/PlatformPei/Platform.c | 44 +++++++++++++-------
>> OvmfPkg/PlatformPei/Xen.c | 5 ++-
>> 3 files changed, 40 insertions(+), 20 deletions(-)
>>
>> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
>> index 4863eb101067..d00a570d4381 100644
>> --- a/OvmfPkg/PlatformPei/MemDetect.c
>> +++ b/OvmfPkg/PlatformPei/MemDetect.c
>> @@ -107,6 +107,7 @@ GetFirstNonAddress (
>> FIRMWARE_CONFIG_ITEM FwCfgItem;
>> UINTN FwCfgSize;
>> UINT64 HotPlugMemoryEnd;
>> + RETURN_STATUS PcdStatus;
>>
>> FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
>>
>> @@ -154,7 +155,8 @@ GetFirstNonAddress (
>> if (mBootMode != BOOT_ON_S3_RESUME) {
>> DEBUG ((EFI_D_INFO, "%a: disabling 64-bit PCI host aperture\n",
>> __FUNCTION__));
>> - PcdSet64 (PcdPciMmio64Size, 0);
>> + PcdStatus = PcdSet64S (PcdPciMmio64Size, 0);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> }
>>
>> //
>> @@ -202,8 +204,11 @@ GetFirstNonAddress (
>> // the GCD memory space map through our PciHostBridgeLib instance; here we
>> // only need to set the PCDs.
>> //
>> - PcdSet64 (PcdPciMmio64Base, Pci64Base);
>> - PcdSet64 (PcdPciMmio64Size, Pci64Size);
>> + PcdStatus = PcdSet64S (PcdPciMmio64Base, Pci64Base);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> + PcdStatus = PcdSet64S (PcdPciMmio64Size, Pci64Size);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> +
>> DEBUG ((EFI_D_INFO, "%a: Pci64Base=0x%Lx Pci64Size=0x%Lx\n",
>> __FUNCTION__, Pci64Base, Pci64Size));
>> }
>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>> index ca1e6dc7e320..c6e1106c9ed0 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -156,8 +156,9 @@ MemMapInitialization (
>> VOID
>> )
>> {
>> - UINT64 PciIoBase;
>> - UINT64 PciIoSize;
>> + UINT64 PciIoBase;
>> + UINT64 PciIoSize;
>> + RETURN_STATUS PcdStatus;
>>
>> PciIoBase = 0xC000;
>> PciIoSize = 0x4000;
>> @@ -212,8 +213,11 @@ MemMapInitialization (
>> //
>> PciSize = 0xFC000000 - PciBase;
>> AddIoMemoryBaseSizeHob (PciBase, PciSize);
>> - PcdSet64 (PcdPciMmio32Base, PciBase);
>> - PcdSet64 (PcdPciMmio32Size, PciSize);
>> + PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> + PcdStatus = PcdSet64S (PcdPciMmio32Size, PciSize);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> +
>> AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>> AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
>> if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>> @@ -266,8 +270,10 @@ MemMapInitialization (
>> PciIoBase,
>> PciIoSize
>> );
>> - PcdSet64 (PcdPciIoBase, PciIoBase);
>> - PcdSet64 (PcdPciIoSize, PciIoSize);
>> + PcdStatus = PcdSet64S (PcdPciIoBase, PciIoBase);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> + PcdStatus = PcdSet64S (PcdPciIoSize, PciIoSize);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> }
>>
>> EFI_STATUS
>> @@ -316,11 +322,13 @@ GetNamedFwCfgBoolean (
>>
>> #define UPDATE_BOOLEAN_PCD_FROM_FW_CFG(TokenName) \
>> do { \
>> - BOOLEAN Setting; \
>> + BOOLEAN Setting; \
>> + RETURN_STATUS PcdStatus; \
>> \
>> if (!EFI_ERROR (GetNamedFwCfgBoolean ( \
>> "opt/ovmf/" #TokenName, &Setting))) { \
>> - PcdSetBool (TokenName, Setting); \
>> + PcdStatus = PcdSetBoolS (TokenName, Setting); \
>> + ASSERT_RETURN_ERROR (PcdStatus); \
>> } \
>> } while (0)
>>
>> @@ -379,12 +387,13 @@ MiscInitialization (
>> VOID
>> )
>> {
>> - UINTN PmCmd;
>> - UINTN Pmba;
>> - UINT32 PmbaAndVal;
>> - UINT32 PmbaOrVal;
>> - UINTN AcpiCtlReg;
>> - UINT8 AcpiEnBit;
>> + UINTN PmCmd;
>> + UINTN Pmba;
>> + UINT32 PmbaAndVal;
>> + UINT32 PmbaOrVal;
>> + UINTN AcpiCtlReg;
>> + UINT8 AcpiEnBit;
>> + RETURN_STATUS PcdStatus;
>>
>> //
>> // Disable A20 Mask
>> @@ -424,7 +433,8 @@ MiscInitialization (
>> ASSERT (FALSE);
>> return;
>> }
>> - PcdSet16 (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
>> + PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>>
>> //
>> // If the appropriate IOspace enable bit is set, assume the ACPI PMBA
>> @@ -491,6 +501,7 @@ ReserveEmuVariableNvStore (
>> )
>> {
>> EFI_PHYSICAL_ADDRESS VariableStore;
>> + RETURN_STATUS PcdStatus;
>>
>> //
>> // Allocate storage for NV variables early on so it will be
>> @@ -509,7 +520,8 @@ ReserveEmuVariableNvStore (
>> VariableStore,
>> (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
>> ));
>> - PcdSet64 (PcdEmuVariableNvStoreReserved, VariableStore);
>> + PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>> }
>>
>>
>> diff --git a/OvmfPkg/PlatformPei/Xen.c b/OvmfPkg/PlatformPei/Xen.c
>> index 223908a4f529..ab38f97a67aa 100644
>> --- a/OvmfPkg/PlatformPei/Xen.c
>> +++ b/OvmfPkg/PlatformPei/Xen.c
>> @@ -210,6 +210,8 @@ InitializeXen (
>> VOID
>> )
>> {
>> + RETURN_STATUS PcdStatus;
>> +
>> if (mXenLeaf == 0) {
>> return EFI_NOT_FOUND;
>> }
>> @@ -222,7 +224,8 @@ InitializeXen (
>> //
>> AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE);
>>
>> - PcdSetBool (PcdPciDisableBusEnumeration, TRUE);
>> + PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE);
>> + ASSERT_RETURN_ERROR (PcdStatus);
>>
>> return EFI_SUCCESS;
>> }
>> --
>> 2.9.2
>>
>>
>>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 09/19] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (7 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 08/19] OvmfPkg/PlatformPei: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-21 21:27 ` [PATCH 10/19] OvmfPkg: disable deprecated interfaces Laszlo Ersek
` (12 subsequent siblings)
21 siblings, 0 replies; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
index c37aed1c9e7b..ff27c1100c01 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -965,6 +965,7 @@ FvbInitialize (
EFI_PHYSICAL_ADDRESS BaseAddress;
UINTN Length;
UINTN NumOfBlocks;
+ RETURN_STATUS PcdStatus;
if (EFI_ERROR (QemuFlashInitialize ())) {
//
@@ -1095,18 +1096,21 @@ FvbInitialize (
//
// Set several PCD values to point to flash
//
- PcdSet64 (
+ PcdStatus = PcdSet64S (
PcdFlashNvStorageVariableBase64,
(UINTN) PcdGet32 (PcdOvmfFlashNvStorageVariableBase)
);
- PcdSet32 (
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet32S (
PcdFlashNvStorageFtwWorkingBase,
PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase)
);
- PcdSet32 (
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet32S (
PcdFlashNvStorageFtwSpareBase,
PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase)
);
+ ASSERT_RETURN_ERROR (PcdStatus);
FwhInstance = (EFI_FW_VOL_INSTANCE *)
(
@@ -1119,6 +1123,7 @@ FvbInitialize (
//
InstallVirtualAddressChangeHandler ();
- PcdSetBool (PcdOvmfFlashVariablesEnable, TRUE);
+ PcdStatus = PcdSetBoolS (PcdOvmfFlashVariablesEnable, TRUE);
+ ASSERT_RETURN_ERROR (PcdStatus);
return EFI_SUCCESS;
}
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH 10/19] OvmfPkg: disable deprecated interfaces
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (8 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 09/19] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-21 21:27 ` [PATCH 11/19] ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls Laszlo Ersek
` (11 subsequent siblings)
21 siblings, 0 replies; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
At this point no code in OvmfPkg (and apparently no code outside of
OvmfPkg that OVMF depends on) uses the deprecated APIs, so we can disable
them in the platform DSC files:
BaseLib:
- StrCpy
- StrnCpy
- StrCat
- StrnCat
- UnicodeStrToAsciiStr
- AsciiStrCpy
- AsciiStrnCpy
- AsciiStrCat
- AsciiStrnCat
- AsciiStrToUnicodeStr
PcdLib:
- PcdSet8
- PcdSet16
- PcdSet32
- PcdSet64
- PcdSetPtr
- PcdSetBool
- PcdSetEx8
- PcdSetEx16
- PcdSetEx32
- PcdSetEx64
- PcdSetExPtr
- PcdSetExBool
UefiLib:
- GetVariable
- GetEfiGlobalVariable
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
Tested with GCC only. My VS2015 build environment was lost and will have
to be reinstalled from scratch at a later (unspecified) point, and I've
never had access to Intel compilers.
OvmfPkg/OvmfPkgIa32.dsc | 7 +++++++
OvmfPkg/OvmfPkgIa32X64.dsc | 7 +++++++
OvmfPkg/OvmfPkgX64.dsc | 7 +++++++
3 files changed, 21 insertions(+)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 4f6dbc53a9fa..3f4d42d85642 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -46,6 +46,13 @@ [BuildOptions]
MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
+ #
+ # Disable deprecated APIs.
+ #
+ MSFT:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+ INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+ GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
+
[BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index c21f88ec1a08..568847531a56 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -51,6 +51,13 @@ [BuildOptions]
INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
!endif
+ #
+ # Disable deprecated APIs.
+ #
+ MSFT:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+ INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+ GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
+
[BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index b7a39e3c4c61..dcf64b9d6aef 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -51,6 +51,13 @@ [BuildOptions]
INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
!endif
+ #
+ # Disable deprecated APIs.
+ #
+ MSFT:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+ INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+ GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
+
[BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH 11/19] ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (9 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 10/19] OvmfPkg: disable deprecated interfaces Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 8:00 ` Ard Biesheuvel
2016-10-21 21:27 ` [PATCH 12/19] ArmVirtPkg/ArmVirtPL031FdtClientLib: " Laszlo Ersek
` (10 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
index 648806126e78..dc1f5155f1fc 100644
--- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
@@ -41,6 +41,7 @@ ArmVirtGicArchLibConstructor (
UINTN GicRevision;
EFI_STATUS Status;
UINT64 DistBase, CpuBase, RedistBase;
+ RETURN_STATUS PcdStatus;
Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
(VOID **)&FdtClient);
@@ -85,8 +86,10 @@ ArmVirtGicArchLibConstructor (
RedistBase = SwapBytes64 (Reg[2]);
ASSERT (RedistBase < MAX_UINTN);
- PcdSet64 (PcdGicDistributorBase, DistBase);
- PcdSet64 (PcdGicRedistributorsBase, RedistBase);
+ PcdStatus = PcdSet64S (PcdGicDistributorBase, DistBase);
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet64S (PcdGicRedistributorsBase, RedistBase);
+ ASSERT_RETURN_ERROR (PcdStatus);
DEBUG ((EFI_D_INFO, "Found GIC v3 (re)distributor @ 0x%Lx (0x%Lx)\n",
DistBase, RedistBase));
@@ -120,8 +123,10 @@ ArmVirtGicArchLibConstructor (
ASSERT (DistBase < MAX_UINTN);
ASSERT (CpuBase < MAX_UINTN);
- PcdSet64 (PcdGicDistributorBase, DistBase);
- PcdSet64 (PcdGicInterruptInterfaceBase, CpuBase);
+ PcdStatus = PcdSet64S (PcdGicDistributorBase, DistBase);
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet64S (PcdGicInterruptInterfaceBase, CpuBase);
+ ASSERT_RETURN_ERROR (PcdStatus);
DEBUG ((EFI_D_INFO, "Found GIC @ 0x%Lx/0x%Lx\n", DistBase, CpuBase));
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 11/19] ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 ` [PATCH 11/19] ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls Laszlo Ersek
@ 2016-10-24 8:00 ` Ard Biesheuvel
0 siblings, 0 replies; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 8:00 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01
On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> index 648806126e78..dc1f5155f1fc 100644
> --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> @@ -41,6 +41,7 @@ ArmVirtGicArchLibConstructor (
> UINTN GicRevision;
> EFI_STATUS Status;
> UINT64 DistBase, CpuBase, RedistBase;
> + RETURN_STATUS PcdStatus;
>
> Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
> (VOID **)&FdtClient);
> @@ -85,8 +86,10 @@ ArmVirtGicArchLibConstructor (
> RedistBase = SwapBytes64 (Reg[2]);
> ASSERT (RedistBase < MAX_UINTN);
>
> - PcdSet64 (PcdGicDistributorBase, DistBase);
> - PcdSet64 (PcdGicRedistributorsBase, RedistBase);
> + PcdStatus = PcdSet64S (PcdGicDistributorBase, DistBase);
> + ASSERT_RETURN_ERROR (PcdStatus);
> + PcdStatus = PcdSet64S (PcdGicRedistributorsBase, RedistBase);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> DEBUG ((EFI_D_INFO, "Found GIC v3 (re)distributor @ 0x%Lx (0x%Lx)\n",
> DistBase, RedistBase));
> @@ -120,8 +123,10 @@ ArmVirtGicArchLibConstructor (
> ASSERT (DistBase < MAX_UINTN);
> ASSERT (CpuBase < MAX_UINTN);
>
> - PcdSet64 (PcdGicDistributorBase, DistBase);
> - PcdSet64 (PcdGicInterruptInterfaceBase, CpuBase);
> + PcdStatus = PcdSet64S (PcdGicDistributorBase, DistBase);
> + ASSERT_RETURN_ERROR (PcdStatus);
> + PcdStatus = PcdSet64S (PcdGicInterruptInterfaceBase, CpuBase);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> DEBUG ((EFI_D_INFO, "Found GIC @ 0x%Lx/0x%Lx\n", DistBase, CpuBase));
>
> --
> 2.9.2
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 12/19] ArmVirtPkg/ArmVirtPL031FdtClientLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (10 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 11/19] ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 8:00 ` Ard Biesheuvel
2016-10-21 21:27 ` [PATCH 13/19] ArmVirtPkg/ArmVirtPlatformLib: " Laszlo Ersek
` (9 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
index 3c4e44caa2f4..82de7a51b32e 100644
--- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
+++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
@@ -34,6 +34,7 @@ ArmVirtPL031FdtClientLibConstructor (
CONST UINT64 *Reg;
UINT32 RegSize;
UINT64 RegBase;
+ RETURN_STATUS PcdStatus;
Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
(VOID **)&FdtClient);
@@ -60,7 +61,8 @@ ArmVirtPL031FdtClientLibConstructor (
RegBase = SwapBytes64 (Reg[0]);
ASSERT (RegBase < MAX_UINT32);
- PcdSet32 (PcdPL031RtcBase, (UINT32)RegBase);
+ PcdStatus = PcdSet32S (PcdPL031RtcBase, (UINT32)RegBase);
+ ASSERT_RETURN_ERROR (PcdStatus);
DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 12/19] ArmVirtPkg/ArmVirtPL031FdtClientLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 ` [PATCH 12/19] ArmVirtPkg/ArmVirtPL031FdtClientLib: " Laszlo Ersek
@ 2016-10-24 8:00 ` Ard Biesheuvel
0 siblings, 0 replies; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 8:00 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01
On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> index 3c4e44caa2f4..82de7a51b32e 100644
> --- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> @@ -34,6 +34,7 @@ ArmVirtPL031FdtClientLibConstructor (
> CONST UINT64 *Reg;
> UINT32 RegSize;
> UINT64 RegBase;
> + RETURN_STATUS PcdStatus;
>
> Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
> (VOID **)&FdtClient);
> @@ -60,7 +61,8 @@ ArmVirtPL031FdtClientLibConstructor (
> RegBase = SwapBytes64 (Reg[0]);
> ASSERT (RegBase < MAX_UINT32);
>
> - PcdSet32 (PcdPL031RtcBase, (UINT32)RegBase);
> + PcdStatus = PcdSet32S (PcdPL031RtcBase, (UINT32)RegBase);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));
>
> --
> 2.9.2
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 13/19] ArmVirtPkg/ArmVirtPlatformLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (11 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 12/19] ArmVirtPkg/ArmVirtPL031FdtClientLib: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 7:59 ` Ard Biesheuvel
2016-10-21 21:27 ` [PATCH 14/19] ArmVirtPkg/ArmVirtTimerFdtClientLib: " Laszlo Ersek
` (8 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
index 7a0fc0e75e37..fcaf3c681a97 100644
--- a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
+++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
@@ -70,13 +70,14 @@ ArmPlatformInitializeSystemMemory (
VOID
)
{
- VOID *DeviceTreeBase;
- INT32 Node, Prev;
- UINT64 NewBase, CurBase;
- UINT64 NewSize, CurSize;
- CONST CHAR8 *Type;
- INT32 Len;
- CONST UINT64 *RegProp;
+ VOID *DeviceTreeBase;
+ INT32 Node, Prev;
+ UINT64 NewBase, CurBase;
+ UINT64 NewSize, CurSize;
+ CONST CHAR8 *Type;
+ INT32 Len;
+ CONST UINT64 *RegProp;
+ RETURN_STATUS PcdStatus;
NewBase = 0;
NewSize = 0;
@@ -131,7 +132,8 @@ ArmPlatformInitializeSystemMemory (
// Make sure the start of DRAM matches our expectation
//
ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
- PcdSet64 (PcdSystemMemorySize, NewSize);
+ PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
+ ASSERT_RETURN_ERROR (PcdStatus);
//
// We need to make sure that the machine we are running on has at least
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 13/19] ArmVirtPkg/ArmVirtPlatformLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 ` [PATCH 13/19] ArmVirtPkg/ArmVirtPlatformLib: " Laszlo Ersek
@ 2016-10-24 7:59 ` Ard Biesheuvel
0 siblings, 0 replies; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 7:59 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01
On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
> index 7a0fc0e75e37..fcaf3c681a97 100644
> --- a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
> +++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
> @@ -70,13 +70,14 @@ ArmPlatformInitializeSystemMemory (
> VOID
> )
> {
> - VOID *DeviceTreeBase;
> - INT32 Node, Prev;
> - UINT64 NewBase, CurBase;
> - UINT64 NewSize, CurSize;
> - CONST CHAR8 *Type;
> - INT32 Len;
> - CONST UINT64 *RegProp;
> + VOID *DeviceTreeBase;
> + INT32 Node, Prev;
> + UINT64 NewBase, CurBase;
> + UINT64 NewSize, CurSize;
> + CONST CHAR8 *Type;
> + INT32 Len;
> + CONST UINT64 *RegProp;
> + RETURN_STATUS PcdStatus;
>
> NewBase = 0;
> NewSize = 0;
> @@ -131,7 +132,8 @@ ArmPlatformInitializeSystemMemory (
> // Make sure the start of DRAM matches our expectation
> //
> ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
> - PcdSet64 (PcdSystemMemorySize, NewSize);
> + PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> //
> // We need to make sure that the machine we are running on has at least
> --
> 2.9.2
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 14/19] ArmVirtPkg/ArmVirtTimerFdtClientLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (12 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 13/19] ArmVirtPkg/ArmVirtPlatformLib: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 7:59 ` Ard Biesheuvel
2016-10-21 21:27 ` [PATCH 15/19] ArmVirtPkg/FdtPciPcdProducerLib: " Laszlo Ersek
` (7 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
index 6e7461c6b2e7..c40838632352 100644
--- a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
+++ b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
@@ -41,6 +41,7 @@ ArmVirtTimerFdtClientLibConstructor (
CONST INTERRUPT_PROPERTY *InterruptProp;
UINT32 PropSize;
INT32 SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum;
+ RETURN_STATUS PcdStatus;
Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
(VOID **)&FdtClient);
@@ -78,10 +79,14 @@ ArmVirtTimerFdtClientLibConstructor (
DEBUG ((EFI_D_INFO, "Found Timer interrupts %d, %d, %d, %d\n",
SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum));
- PcdSet32 (PcdArmArchTimerSecIntrNum, SecIntrNum);
- PcdSet32 (PcdArmArchTimerIntrNum, IntrNum);
- PcdSet32 (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
- PcdSet32 (PcdArmArchTimerHypIntrNum, HypIntrNum);
+ PcdStatus = PcdSet32S (PcdArmArchTimerSecIntrNum, SecIntrNum);
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet32S (PcdArmArchTimerIntrNum, IntrNum);
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet32S (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
+ ASSERT_RETURN_ERROR (PcdStatus);
+ PcdStatus = PcdSet32S (PcdArmArchTimerHypIntrNum, HypIntrNum);
+ ASSERT_RETURN_ERROR (PcdStatus);
return EFI_SUCCESS;
}
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 14/19] ArmVirtPkg/ArmVirtTimerFdtClientLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 ` [PATCH 14/19] ArmVirtPkg/ArmVirtTimerFdtClientLib: " Laszlo Ersek
@ 2016-10-24 7:59 ` Ard Biesheuvel
0 siblings, 0 replies; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 7:59 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01
On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
> index 6e7461c6b2e7..c40838632352 100644
> --- a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
> @@ -41,6 +41,7 @@ ArmVirtTimerFdtClientLibConstructor (
> CONST INTERRUPT_PROPERTY *InterruptProp;
> UINT32 PropSize;
> INT32 SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum;
> + RETURN_STATUS PcdStatus;
>
> Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
> (VOID **)&FdtClient);
> @@ -78,10 +79,14 @@ ArmVirtTimerFdtClientLibConstructor (
> DEBUG ((EFI_D_INFO, "Found Timer interrupts %d, %d, %d, %d\n",
> SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum));
>
> - PcdSet32 (PcdArmArchTimerSecIntrNum, SecIntrNum);
> - PcdSet32 (PcdArmArchTimerIntrNum, IntrNum);
> - PcdSet32 (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
> - PcdSet32 (PcdArmArchTimerHypIntrNum, HypIntrNum);
> + PcdStatus = PcdSet32S (PcdArmArchTimerSecIntrNum, SecIntrNum);
> + ASSERT_RETURN_ERROR (PcdStatus);
> + PcdStatus = PcdSet32S (PcdArmArchTimerIntrNum, IntrNum);
> + ASSERT_RETURN_ERROR (PcdStatus);
> + PcdStatus = PcdSet32S (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
> + ASSERT_RETURN_ERROR (PcdStatus);
> + PcdStatus = PcdSet32S (PcdArmArchTimerHypIntrNum, HypIntrNum);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> return EFI_SUCCESS;
> }
> --
> 2.9.2
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 15/19] ArmVirtPkg/FdtPciPcdProducerLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (13 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 14/19] ArmVirtPkg/ArmVirtTimerFdtClientLib: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 7:59 ` Ard Biesheuvel
2016-10-21 21:27 ` [PATCH 16/19] ArmVirtPkg/PlatformBootManagerLib: " Laszlo Ersek
` (6 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
index ea27cda7b77c..072f54ef7d9e 100644
--- a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
+++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
@@ -98,6 +98,7 @@ FdtPciPcdProducerLibConstructor (
INT32 Node;
RETURN_STATUS RetStatus;
UINT64 IoTranslation;
+ RETURN_STATUS PcdStatus;
PciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);
if (PciExpressBaseAddress != MAX_UINT64) {
@@ -126,12 +127,14 @@ FdtPciPcdProducerLibConstructor (
if (!EFI_ERROR (Status) && RegSize == 2 * sizeof (UINT64)) {
PciExpressBaseAddress = SwapBytes64 (*Reg);
- PcdSetBool (PcdPciDisableBusEnumeration, FALSE);
+ PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, FALSE);
+ ASSERT_RETURN_ERROR (PcdStatus);
IoTranslation = 0;
RetStatus = GetPciIoTranslation (FdtClient, Node, &IoTranslation);
if (!RETURN_ERROR (RetStatus)) {
- PcdSet64 (PcdPciIoTranslation, IoTranslation);
+ PcdStatus = PcdSet64S (PcdPciIoTranslation, IoTranslation);
+ ASSERT_RETURN_ERROR (PcdStatus);
} else {
//
// Support for I/O BARs is not mandatory, and so it does not make sense
@@ -145,7 +148,8 @@ FdtPciPcdProducerLibConstructor (
}
}
- PcdSet64 (PcdPciExpressBaseAddress, PciExpressBaseAddress);
+ PcdStatus = PcdSet64S (PcdPciExpressBaseAddress, PciExpressBaseAddress);
+ ASSERT_RETURN_ERROR (PcdStatus);
return RETURN_SUCCESS;
}
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 15/19] ArmVirtPkg/FdtPciPcdProducerLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 ` [PATCH 15/19] ArmVirtPkg/FdtPciPcdProducerLib: " Laszlo Ersek
@ 2016-10-24 7:59 ` Ard Biesheuvel
0 siblings, 0 replies; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 7:59 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01
On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
> index ea27cda7b77c..072f54ef7d9e 100644
> --- a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
> +++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
> @@ -98,6 +98,7 @@ FdtPciPcdProducerLibConstructor (
> INT32 Node;
> RETURN_STATUS RetStatus;
> UINT64 IoTranslation;
> + RETURN_STATUS PcdStatus;
>
> PciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);
> if (PciExpressBaseAddress != MAX_UINT64) {
> @@ -126,12 +127,14 @@ FdtPciPcdProducerLibConstructor (
> if (!EFI_ERROR (Status) && RegSize == 2 * sizeof (UINT64)) {
> PciExpressBaseAddress = SwapBytes64 (*Reg);
>
> - PcdSetBool (PcdPciDisableBusEnumeration, FALSE);
> + PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, FALSE);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> IoTranslation = 0;
> RetStatus = GetPciIoTranslation (FdtClient, Node, &IoTranslation);
> if (!RETURN_ERROR (RetStatus)) {
> - PcdSet64 (PcdPciIoTranslation, IoTranslation);
> + PcdStatus = PcdSet64S (PcdPciIoTranslation, IoTranslation);
> + ASSERT_RETURN_ERROR (PcdStatus);
> } else {
> //
> // Support for I/O BARs is not mandatory, and so it does not make sense
> @@ -145,7 +148,8 @@ FdtPciPcdProducerLibConstructor (
> }
> }
>
> - PcdSet64 (PcdPciExpressBaseAddress, PciExpressBaseAddress);
> + PcdStatus = PcdSet64S (PcdPciExpressBaseAddress, PciExpressBaseAddress);
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> return RETURN_SUCCESS;
> }
> --
> 2.9.2
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 16/19] ArmVirtPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (14 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 15/19] ArmVirtPkg/FdtPciPcdProducerLib: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 7:58 ` Ard Biesheuvel
2016-10-21 21:27 ` [PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS() Laszlo Ersek
` (5 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel
These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index aecad570a04f..56f4c921b513 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -578,6 +578,8 @@ PlatformBootManagerBeforeConsole (
VOID
)
{
+ RETURN_STATUS PcdStatus;
+
//
// Signal EndOfDxe PI Event
//
@@ -629,7 +631,9 @@ PlatformBootManagerBeforeConsole (
//
// Set the front page timeout from the QEMU configuration.
//
- PcdSet16 (PcdPlatformBootTimeOut, GetFrontPageTimeoutFromQemu ());
+ PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
+ GetFrontPageTimeoutFromQemu ());
+ ASSERT_RETURN_ERROR (PcdStatus);
//
// Register platform-specific boot options and keyboard shortcuts.
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 16/19] ArmVirtPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls
2016-10-21 21:27 ` [PATCH 16/19] ArmVirtPkg/PlatformBootManagerLib: " Laszlo Ersek
@ 2016-10-24 7:58 ` Ard Biesheuvel
0 siblings, 0 replies; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 7:58 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01
On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index aecad570a04f..56f4c921b513 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -578,6 +578,8 @@ PlatformBootManagerBeforeConsole (
> VOID
> )
> {
> + RETURN_STATUS PcdStatus;
> +
> //
> // Signal EndOfDxe PI Event
> //
> @@ -629,7 +631,9 @@ PlatformBootManagerBeforeConsole (
> //
> // Set the front page timeout from the QEMU configuration.
> //
> - PcdSet16 (PcdPlatformBootTimeOut, GetFrontPageTimeoutFromQemu ());
> + PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
> + GetFrontPageTimeoutFromQemu ());
> + ASSERT_RETURN_ERROR (PcdStatus);
>
> //
> // Register platform-specific boot options and keyboard shortcuts.
> --
> 2.9.2
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (15 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 16/19] ArmVirtPkg/PlatformBootManagerLib: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 7:58 ` Ard Biesheuvel
2016-10-21 21:27 ` [PATCH 18/19] ArmPkg/DefaultExceptionHandlerLib: " Laszlo Ersek
` (4 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Leif Lindholm, Michael Zimmermann
AsciiStrCat() is deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
The "Str" variable serves no particular purpose in the MRegList() and
ThumbMRegList() functions; replace it with the pointed-to "mMregListStr" /
"mThumbMregListStr" global variable (as appropriate), so that the new
AsciiStrCatS() calls are as clear as possible.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
- build-tested only
- Michael (CC'd) had posted a patch earlier for this:
<https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
but I only noticed that now that he pointed it out, in
<https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
I'll leave it to the ArmPkg maintainers to choose one; I don't feel
strongly either way.
ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c | 22 +++++++++-----------
ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c | 20 ++++++++----------
2 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
index 29a8d4438622..8551edc379c1 100644
--- a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
+++ b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
@@ -88,12 +88,10 @@ MRegList (
)
{
UINTN Index, Start, End;
- CHAR8 *Str;
BOOLEAN First;
- Str = mMregListStr;
- *Str = '\0';
- AsciiStrCat (Str, "{");
+ mMregListStr[0] = '\0';
+ AsciiStrCatS (mMregListStr, sizeof mMregListStr, "{");
for (Index = 0, First = TRUE; Index <= 15; Index++) {
if ((OpCode & (1 << Index)) != 0) {
Start = End = Index;
@@ -102,25 +100,25 @@ MRegList (
}
if (!First) {
- AsciiStrCat (Str, ",");
+ AsciiStrCatS (mMregListStr, sizeof mMregListStr, ",");
} else {
First = FALSE;
}
if (Start == End) {
- AsciiStrCat (Str, gReg[Start]);
- AsciiStrCat (Str, ", ");
+ AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[Start]);
+ AsciiStrCatS (mMregListStr, sizeof mMregListStr, ", ");
} else {
- AsciiStrCat (Str, gReg[Start]);
- AsciiStrCat (Str, "-");
- AsciiStrCat (Str, gReg[End]);
+ AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[Start]);
+ AsciiStrCatS (mMregListStr, sizeof mMregListStr, "-");
+ AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[End]);
}
}
}
if (First) {
- AsciiStrCat (Str, "ERROR");
+ AsciiStrCatS (mMregListStr, sizeof mMregListStr, "ERROR");
}
- AsciiStrCat (Str, "}");
+ AsciiStrCatS (mMregListStr, sizeof mMregListStr, "}");
// BugBug: Make caller pass in buffer it is cleaner
return mMregListStr;
diff --git a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
index 5bad3afcfbf6..86d5083cb189 100644
--- a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
+++ b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
@@ -397,12 +397,10 @@ ThumbMRegList (
)
{
UINTN Index, Start, End;
- CHAR8 *Str;
BOOLEAN First;
- Str = mThumbMregListStr;
- *Str = '\0';
- AsciiStrCat (Str, "{");
+ mThumbMregListStr[0] = '\0';
+ AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "{");
for (Index = 0, First = TRUE; Index <= 15; Index++) {
if ((RegBitMask & (1 << Index)) != 0) {
@@ -412,24 +410,24 @@ ThumbMRegList (
}
if (!First) {
- AsciiStrCat (Str, ",");
+ AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, ",");
} else {
First = FALSE;
}
if (Start == End) {
- AsciiStrCat (Str, gReg[Start]);
+ AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
} else {
- AsciiStrCat (Str, gReg[Start]);
- AsciiStrCat (Str, "-");
- AsciiStrCat (Str, gReg[End]);
+ AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
+ AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "-");
+ AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[End]);
}
}
}
if (First) {
- AsciiStrCat (Str, "ERROR");
+ AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "ERROR");
}
- AsciiStrCat (Str, "}");
+ AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "}");
// BugBug: Make caller pass in buffer it is cleaner
return mThumbMregListStr;
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()
2016-10-21 21:27 ` [PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS() Laszlo Ersek
@ 2016-10-24 7:58 ` Ard Biesheuvel
2016-10-24 18:47 ` Jordan Justen
0 siblings, 1 reply; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 7:58 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Leif Lindholm, Michael Zimmermann
On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> AsciiStrCat() is deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> The "Str" variable serves no particular purpose in the MRegList() and
> ThumbMRegList() functions; replace it with the pointed-to "mMregListStr" /
> "mThumbMregListStr" global variable (as appropriate), so that the new
> AsciiStrCatS() calls are as clear as possible.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> Notes:
> - build-tested only
>
> - Michael (CC'd) had posted a patch earlier for this:
> <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
> but I only noticed that now that he pointed it out, in
> <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
> I'll leave it to the ArmPkg maintainers to choose one; I don't feel
> strongly either way.
>
Apologies to Michael for ignoring his contribution, but I'd be happy
for Laszlo to merge this as part of the series.
> ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c | 22 +++++++++-----------
> ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c | 20 ++++++++----------
> 2 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> index 29a8d4438622..8551edc379c1 100644
> --- a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> +++ b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> @@ -88,12 +88,10 @@ MRegList (
> )
> {
> UINTN Index, Start, End;
> - CHAR8 *Str;
> BOOLEAN First;
>
> - Str = mMregListStr;
> - *Str = '\0';
> - AsciiStrCat (Str, "{");
> + mMregListStr[0] = '\0';
> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "{");
> for (Index = 0, First = TRUE; Index <= 15; Index++) {
> if ((OpCode & (1 << Index)) != 0) {
> Start = End = Index;
> @@ -102,25 +100,25 @@ MRegList (
> }
>
> if (!First) {
> - AsciiStrCat (Str, ",");
> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, ",");
> } else {
> First = FALSE;
> }
>
> if (Start == End) {
> - AsciiStrCat (Str, gReg[Start]);
> - AsciiStrCat (Str, ", ");
> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[Start]);
> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, ", ");
> } else {
> - AsciiStrCat (Str, gReg[Start]);
> - AsciiStrCat (Str, "-");
> - AsciiStrCat (Str, gReg[End]);
> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[Start]);
> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "-");
> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[End]);
> }
> }
> }
> if (First) {
> - AsciiStrCat (Str, "ERROR");
> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "ERROR");
> }
> - AsciiStrCat (Str, "}");
> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "}");
>
> // BugBug: Make caller pass in buffer it is cleaner
> return mMregListStr;
> diff --git a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> index 5bad3afcfbf6..86d5083cb189 100644
> --- a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> +++ b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> @@ -397,12 +397,10 @@ ThumbMRegList (
> )
> {
> UINTN Index, Start, End;
> - CHAR8 *Str;
> BOOLEAN First;
>
> - Str = mThumbMregListStr;
> - *Str = '\0';
> - AsciiStrCat (Str, "{");
> + mThumbMregListStr[0] = '\0';
> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "{");
>
> for (Index = 0, First = TRUE; Index <= 15; Index++) {
> if ((RegBitMask & (1 << Index)) != 0) {
> @@ -412,24 +410,24 @@ ThumbMRegList (
> }
>
> if (!First) {
> - AsciiStrCat (Str, ",");
> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, ",");
> } else {
> First = FALSE;
> }
>
> if (Start == End) {
> - AsciiStrCat (Str, gReg[Start]);
> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
> } else {
> - AsciiStrCat (Str, gReg[Start]);
> - AsciiStrCat (Str, "-");
> - AsciiStrCat (Str, gReg[End]);
> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "-");
> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[End]);
> }
> }
> }
> if (First) {
> - AsciiStrCat (Str, "ERROR");
> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "ERROR");
> }
> - AsciiStrCat (Str, "}");
> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "}");
>
> // BugBug: Make caller pass in buffer it is cleaner
> return mThumbMregListStr;
> --
> 2.9.2
>
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()
2016-10-24 7:58 ` Ard Biesheuvel
@ 2016-10-24 18:47 ` Jordan Justen
2016-10-24 19:54 ` Laszlo Ersek
0 siblings, 1 reply; 54+ messages in thread
From: Jordan Justen @ 2016-10-24 18:47 UTC (permalink / raw)
To: Ard Biesheuvel, Laszlo Ersek
Cc: edk2-devel-01, Michael Zimmermann, Leif Lindholm
On 2016-10-24 00:58:46, Ard Biesheuvel wrote:
> On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> > AsciiStrCat() is deprecated / disabled under the
> > DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
> >
> > The "Str" variable serves no particular purpose in the MRegList() and
> > ThumbMRegList() functions; replace it with the pointed-to "mMregListStr" /
> > "mThumbMregListStr" global variable (as appropriate), so that the new
> > AsciiStrCatS() calls are as clear as possible.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> > ---
> >
> > Notes:
> > - build-tested only
> >
> > - Michael (CC'd) had posted a patch earlier for this:
> > <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
> > but I only noticed that now that he pointed it out, in
> > <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
> > I'll leave it to the ArmPkg maintainers to choose one; I don't feel
> > strongly either way.
> >
>
> Apologies to Michael for ignoring his contribution, but I'd be happy
> for Laszlo to merge this as part of the series.
>
> > ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c | 22 +++++++++-----------
> > ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c | 20 ++++++++----------
> > 2 files changed, 19 insertions(+), 23 deletions(-)
> >
> > diff --git a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> > index 29a8d4438622..8551edc379c1 100644
> > --- a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> > +++ b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> > @@ -88,12 +88,10 @@ MRegList (
> > )
> > {
> > UINTN Index, Start, End;
> > - CHAR8 *Str;
> > BOOLEAN First;
> >
> > - Str = mMregListStr;
> > - *Str = '\0';
> > - AsciiStrCat (Str, "{");
> > + mMregListStr[0] = '\0';
> > + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "{");
Maybe drop the extra space after the function name throughout this
patch. (There are 2 instead of just one.)
-Jordan
> > for (Index = 0, First = TRUE; Index <= 15; Index++) {
> > if ((OpCode & (1 << Index)) != 0) {
> > Start = End = Index;
> > @@ -102,25 +100,25 @@ MRegList (
> > }
> >
> > if (!First) {
> > - AsciiStrCat (Str, ",");
> > + AsciiStrCatS (mMregListStr, sizeof mMregListStr, ",");
> > } else {
> > First = FALSE;
> > }
> >
> > if (Start == End) {
> > - AsciiStrCat (Str, gReg[Start]);
> > - AsciiStrCat (Str, ", ");
> > + AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[Start]);
> > + AsciiStrCatS (mMregListStr, sizeof mMregListStr, ", ");
> > } else {
> > - AsciiStrCat (Str, gReg[Start]);
> > - AsciiStrCat (Str, "-");
> > - AsciiStrCat (Str, gReg[End]);
> > + AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[Start]);
> > + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "-");
> > + AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[End]);
> > }
> > }
> > }
> > if (First) {
> > - AsciiStrCat (Str, "ERROR");
> > + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "ERROR");
> > }
> > - AsciiStrCat (Str, "}");
> > + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "}");
> >
> > // BugBug: Make caller pass in buffer it is cleaner
> > return mMregListStr;
> > diff --git a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> > index 5bad3afcfbf6..86d5083cb189 100644
> > --- a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> > +++ b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> > @@ -397,12 +397,10 @@ ThumbMRegList (
> > )
> > {
> > UINTN Index, Start, End;
> > - CHAR8 *Str;
> > BOOLEAN First;
> >
> > - Str = mThumbMregListStr;
> > - *Str = '\0';
> > - AsciiStrCat (Str, "{");
> > + mThumbMregListStr[0] = '\0';
> > + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "{");
> >
> > for (Index = 0, First = TRUE; Index <= 15; Index++) {
> > if ((RegBitMask & (1 << Index)) != 0) {
> > @@ -412,24 +410,24 @@ ThumbMRegList (
> > }
> >
> > if (!First) {
> > - AsciiStrCat (Str, ",");
> > + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, ",");
> > } else {
> > First = FALSE;
> > }
> >
> > if (Start == End) {
> > - AsciiStrCat (Str, gReg[Start]);
> > + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
> > } else {
> > - AsciiStrCat (Str, gReg[Start]);
> > - AsciiStrCat (Str, "-");
> > - AsciiStrCat (Str, gReg[End]);
> > + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
> > + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "-");
> > + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[End]);
> > }
> > }
> > }
> > if (First) {
> > - AsciiStrCat (Str, "ERROR");
> > + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "ERROR");
> > }
> > - AsciiStrCat (Str, "}");
> > + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "}");
> >
> > // BugBug: Make caller pass in buffer it is cleaner
> > return mThumbMregListStr;
> > --
> > 2.9.2
> >
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()
2016-10-24 18:47 ` Jordan Justen
@ 2016-10-24 19:54 ` Laszlo Ersek
0 siblings, 0 replies; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-24 19:54 UTC (permalink / raw)
To: Jordan Justen, Ard Biesheuvel
Cc: edk2-devel-01, Michael Zimmermann, Leif Lindholm
On 10/24/16 20:47, Jordan Justen wrote:
> On 2016-10-24 00:58:46, Ard Biesheuvel wrote:
>> On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
>>> AsciiStrCat() is deprecated / disabled under the
>>> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>>>
>>> The "Str" variable serves no particular purpose in the MRegList() and
>>> ThumbMRegList() functions; replace it with the pointed-to "mMregListStr" /
>>> "mThumbMregListStr" global variable (as appropriate), so that the new
>>> AsciiStrCatS() calls are as clear as possible.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>>> ---
>>>
>>> Notes:
>>> - build-tested only
>>>
>>> - Michael (CC'd) had posted a patch earlier for this:
>>> <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
>>> but I only noticed that now that he pointed it out, in
>>> <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
>>> I'll leave it to the ArmPkg maintainers to choose one; I don't feel
>>> strongly either way.
>>>
>>
>> Apologies to Michael for ignoring his contribution, but I'd be happy
>> for Laszlo to merge this as part of the series.
>>
>>> ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c | 22 +++++++++-----------
>>> ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c | 20 ++++++++----------
>>> 2 files changed, 19 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
>>> index 29a8d4438622..8551edc379c1 100644
>>> --- a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
>>> +++ b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
>>> @@ -88,12 +88,10 @@ MRegList (
>>> )
>>> {
>>> UINTN Index, Start, End;
>>> - CHAR8 *Str;
>>> BOOLEAN First;
>>>
>>> - Str = mMregListStr;
>>> - *Str = '\0';
>>> - AsciiStrCat (Str, "{");
>>> + mMregListStr[0] = '\0';
>>> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "{");
>
> Maybe drop the extra space after the function name throughout this
> patch. (There are 2 instead of just one.)
Yep, I saw that; I couldn't decide whether to remove them or to leave
them undisturbed.
I will remove them.
Thanks!
Laszlo
>
> -Jordan
>
>>> for (Index = 0, First = TRUE; Index <= 15; Index++) {
>>> if ((OpCode & (1 << Index)) != 0) {
>>> Start = End = Index;
>>> @@ -102,25 +100,25 @@ MRegList (
>>> }
>>>
>>> if (!First) {
>>> - AsciiStrCat (Str, ",");
>>> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, ",");
>>> } else {
>>> First = FALSE;
>>> }
>>>
>>> if (Start == End) {
>>> - AsciiStrCat (Str, gReg[Start]);
>>> - AsciiStrCat (Str, ", ");
>>> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[Start]);
>>> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, ", ");
>>> } else {
>>> - AsciiStrCat (Str, gReg[Start]);
>>> - AsciiStrCat (Str, "-");
>>> - AsciiStrCat (Str, gReg[End]);
>>> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[Start]);
>>> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "-");
>>> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, gReg[End]);
>>> }
>>> }
>>> }
>>> if (First) {
>>> - AsciiStrCat (Str, "ERROR");
>>> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "ERROR");
>>> }
>>> - AsciiStrCat (Str, "}");
>>> + AsciiStrCatS (mMregListStr, sizeof mMregListStr, "}");
>>>
>>> // BugBug: Make caller pass in buffer it is cleaner
>>> return mMregListStr;
>>> diff --git a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
>>> index 5bad3afcfbf6..86d5083cb189 100644
>>> --- a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
>>> +++ b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
>>> @@ -397,12 +397,10 @@ ThumbMRegList (
>>> )
>>> {
>>> UINTN Index, Start, End;
>>> - CHAR8 *Str;
>>> BOOLEAN First;
>>>
>>> - Str = mThumbMregListStr;
>>> - *Str = '\0';
>>> - AsciiStrCat (Str, "{");
>>> + mThumbMregListStr[0] = '\0';
>>> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "{");
>>>
>>> for (Index = 0, First = TRUE; Index <= 15; Index++) {
>>> if ((RegBitMask & (1 << Index)) != 0) {
>>> @@ -412,24 +410,24 @@ ThumbMRegList (
>>> }
>>>
>>> if (!First) {
>>> - AsciiStrCat (Str, ",");
>>> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, ",");
>>> } else {
>>> First = FALSE;
>>> }
>>>
>>> if (Start == End) {
>>> - AsciiStrCat (Str, gReg[Start]);
>>> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
>>> } else {
>>> - AsciiStrCat (Str, gReg[Start]);
>>> - AsciiStrCat (Str, "-");
>>> - AsciiStrCat (Str, gReg[End]);
>>> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
>>> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "-");
>>> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, gReg[End]);
>>> }
>>> }
>>> }
>>> if (First) {
>>> - AsciiStrCat (Str, "ERROR");
>>> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "ERROR");
>>> }
>>> - AsciiStrCat (Str, "}");
>>> + AsciiStrCatS (mThumbMregListStr, sizeof mThumbMregListStr, "}");
>>>
>>> // BugBug: Make caller pass in buffer it is cleaner
>>> return mThumbMregListStr;
>>> --
>>> 2.9.2
>>>
>>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 18/19] ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with AsciiStrCatS()
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (16 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS() Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-24 7:57 ` Ard Biesheuvel
2016-10-21 21:27 ` [PATCH 19/19] ArmVirtPkg: disable deprecated interfaces Laszlo Ersek
` (3 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Leif Lindholm, Michael Zimmermann
AsciiStrCat() is deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
The caller of CpsrString() is required to pass in "ReturnStr" with 32
CHAR8 elements. (DefaultExceptionHandler() complies with this.) "Str" is
used to build "ReturnStr" gradually. Just before calling AsciiStrCat(),
"Str" points to the then-terminating NUL character in "ReturnStr".
The difference (Str - ReturnStr) gives the number of non-NUL characters
we've written thus far, hence (32 - (Str - ReturnStr)) yields the number
of remaining bytes in ReturnStr, including the ultimately terminating NUL
character.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
- build-tested only
- Michael (CC'd) had posted a patch earlier for this:
<https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
but I only noticed that now that he pointed it out, in
<https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
I'll leave it to the ArmPkg maintainers to choose one; I don't feel
strongly either way.
ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
index aece26355e2e..4b2ee9a9acf7 100644
--- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
+++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
@@ -116,7 +116,10 @@ CpsrString (
break;
}
- AsciiStrCat (Str, ModeStr);
+ //
+ // See the interface contract in the leading comment block.
+ //
+ AsciiStrCatS (Str, 32 - (Str - ReturnStr), ModeStr);
return;
}
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 18/19] ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with AsciiStrCatS()
2016-10-21 21:27 ` [PATCH 18/19] ArmPkg/DefaultExceptionHandlerLib: " Laszlo Ersek
@ 2016-10-24 7:57 ` Ard Biesheuvel
2016-10-24 11:52 ` Laszlo Ersek
0 siblings, 1 reply; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 7:57 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Leif Lindholm, Michael Zimmermann
On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> AsciiStrCat() is deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> The caller of CpsrString() is required to pass in "ReturnStr" with 32
> CHAR8 elements. (DefaultExceptionHandler() complies with this.) "Str" is
> used to build "ReturnStr" gradually. Just before calling AsciiStrCat(),
> "Str" points to the then-terminating NUL character in "ReturnStr".
>
> The difference (Str - ReturnStr) gives the number of non-NUL characters
> we've written thus far, hence (32 - (Str - ReturnStr)) yields the number
> of remaining bytes in ReturnStr, including the ultimately terminating NUL
> character.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> - build-tested only
>
> - Michael (CC'd) had posted a patch earlier for this:
> <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
> but I only noticed that now that he pointed it out, in
> <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
> I'll leave it to the ArmPkg maintainers to choose one; I don't feel
> strongly either way.
>
> ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> index aece26355e2e..4b2ee9a9acf7 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> @@ -116,7 +116,10 @@ CpsrString (
> break;
> }
>
> - AsciiStrCat (Str, ModeStr);
> + //
> + // See the interface contract in the leading comment block.
> + //
> + AsciiStrCatS (Str, 32 - (Str - ReturnStr), ModeStr);
> return;
Could you please use a symbolic constant for this '32', and replace it
in the declaration of CpsrStr[] as well?
With that
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Oh, and if the bogus 'return' happens to get lost along the way as
well, I would not mind.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 18/19] ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with AsciiStrCatS()
2016-10-24 7:57 ` Ard Biesheuvel
@ 2016-10-24 11:52 ` Laszlo Ersek
0 siblings, 0 replies; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-24 11:52 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-01, Leif Lindholm, Michael Zimmermann
On 10/24/16 09:57, Ard Biesheuvel wrote:
> On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
>> AsciiStrCat() is deprecated / disabled under the
>> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>>
>> The caller of CpsrString() is required to pass in "ReturnStr" with 32
>> CHAR8 elements. (DefaultExceptionHandler() complies with this.) "Str" is
>> used to build "ReturnStr" gradually. Just before calling AsciiStrCat(),
>> "Str" points to the then-terminating NUL character in "ReturnStr".
>>
>> The difference (Str - ReturnStr) gives the number of non-NUL characters
>> we've written thus far, hence (32 - (Str - ReturnStr)) yields the number
>> of remaining bytes in ReturnStr, including the ultimately terminating NUL
>> character.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>> - build-tested only
>>
>> - Michael (CC'd) had posted a patch earlier for this:
>> <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
>> but I only noticed that now that he pointed it out, in
>> <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
>> I'll leave it to the ArmPkg maintainers to choose one; I don't feel
>> strongly either way.
>>
>> ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
>> index aece26355e2e..4b2ee9a9acf7 100644
>> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
>> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
>> @@ -116,7 +116,10 @@ CpsrString (
>> break;
>> }
>>
>> - AsciiStrCat (Str, ModeStr);
>> + //
>> + // See the interface contract in the leading comment block.
>> + //
>> + AsciiStrCatS (Str, 32 - (Str - ReturnStr), ModeStr);
>> return;
>
> Could you please use a symbolic constant for this '32', and replace it
> in the declaration of CpsrStr[] as well?
>
> With that
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Oh, and if the bogus 'return' happens to get lost along the way as
> well, I would not mind.
>
Okay, will do. :)
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 19/19] ArmVirtPkg: disable deprecated interfaces
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (17 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 18/19] ArmPkg/DefaultExceptionHandlerLib: " Laszlo Ersek
@ 2016-10-21 21:27 ` Laszlo Ersek
2016-10-25 8:25 ` Laszlo Ersek
2016-10-24 8:04 ` [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Ard Biesheuvel
` (2 subsequent siblings)
21 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-21 21:27 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel
At this point no code in ArmVirtPkg (and apparently no code outside of
ArmVirtPkg that the ArmVirt binaries depend on) uses the deprecated APIs,
so we can disable them in the common platform DSC include file:
BaseLib:
- StrCpy
- StrnCpy
- StrCat
- StrnCat
- UnicodeStrToAsciiStr
- AsciiStrCpy
- AsciiStrnCpy
- AsciiStrCat
- AsciiStrnCat
- AsciiStrToUnicodeStr
PcdLib:
- PcdSet8
- PcdSet16
- PcdSet32
- PcdSet64
- PcdSetPtr
- PcdSetBool
- PcdSetEx8
- PcdSetEx16
- PcdSetEx32
- PcdSetEx64
- PcdSetExPtr
- PcdSetExBool
UefiLib:
- GetVariable
- GetEfiGlobalVariable
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
not build-tested with RVCT
ArmVirtPkg/ArmVirt.dsc.inc | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index c071988ad8f5..dbd6678accde 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -246,6 +246,12 @@ [BuildOptions]
GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
+ #
+ # Disable deprecated APIs.
+ #
+ RVCT:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
+ GCC:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
+
################################################################################
#
# Pcd Section - list of all EDK II PCD Entries defined by this Platform
--
2.9.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 19/19] ArmVirtPkg: disable deprecated interfaces
2016-10-21 21:27 ` [PATCH 19/19] ArmVirtPkg: disable deprecated interfaces Laszlo Ersek
@ 2016-10-25 8:25 ` Laszlo Ersek
2016-10-25 8:28 ` Ard Biesheuvel
0 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-25 8:25 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel
Ard,
On 10/21/16 23:27, Laszlo Ersek wrote:
> At this point no code in ArmVirtPkg (and apparently no code outside of
> ArmVirtPkg that the ArmVirt binaries depend on) uses the deprecated APIs,
> so we can disable them in the common platform DSC include file:
>
> BaseLib:
> - StrCpy
> - StrnCpy
> - StrCat
> - StrnCat
> - UnicodeStrToAsciiStr
> - AsciiStrCpy
> - AsciiStrnCpy
> - AsciiStrCat
> - AsciiStrnCat
> - AsciiStrToUnicodeStr
>
> PcdLib:
> - PcdSet8
> - PcdSet16
> - PcdSet32
> - PcdSet64
> - PcdSetPtr
> - PcdSetBool
> - PcdSetEx8
> - PcdSetEx16
> - PcdSetEx32
> - PcdSetEx64
> - PcdSetExPtr
> - PcdSetExBool
>
> UefiLib:
> - GetVariable
> - GetEfiGlobalVariable
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> not build-tested with RVCT
>
> ArmVirtPkg/ArmVirt.dsc.inc | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index c071988ad8f5..dbd6678accde 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -246,6 +246,12 @@ [BuildOptions]
>
> GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
>
> + #
> + # Disable deprecated APIs.
> + #
> + RVCT:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
> + GCC:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
> +
> ################################################################################
> #
> # Pcd Section - list of all EDK II PCD Entries defined by this Platform
>
Can you please give an R-b for this too?
Thanks
Laszlo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 19/19] ArmVirtPkg: disable deprecated interfaces
2016-10-25 8:25 ` Laszlo Ersek
@ 2016-10-25 8:28 ` Ard Biesheuvel
0 siblings, 0 replies; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-25 8:28 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01
On 25 October 2016 at 09:25, Laszlo Ersek <lersek@redhat.com> wrote:
> Ard,
>
> On 10/21/16 23:27, Laszlo Ersek wrote:
>> At this point no code in ArmVirtPkg (and apparently no code outside of
>> ArmVirtPkg that the ArmVirt binaries depend on) uses the deprecated APIs,
>> so we can disable them in the common platform DSC include file:
>>
>> BaseLib:
>> - StrCpy
>> - StrnCpy
>> - StrCat
>> - StrnCat
>> - UnicodeStrToAsciiStr
>> - AsciiStrCpy
>> - AsciiStrnCpy
>> - AsciiStrCat
>> - AsciiStrnCat
>> - AsciiStrToUnicodeStr
>>
>> PcdLib:
>> - PcdSet8
>> - PcdSet16
>> - PcdSet32
>> - PcdSet64
>> - PcdSetPtr
>> - PcdSetBool
>> - PcdSetEx8
>> - PcdSetEx16
>> - PcdSetEx32
>> - PcdSetEx64
>> - PcdSetExPtr
>> - PcdSetExBool
>>
>> UefiLib:
>> - GetVariable
>> - GetEfiGlobalVariable
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>> not build-tested with RVCT
>>
>> ArmVirtPkg/ArmVirt.dsc.inc | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index c071988ad8f5..dbd6678accde 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -246,6 +246,12 @@ [BuildOptions]
>>
>> GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
>>
>> + #
>> + # Disable deprecated APIs.
>> + #
>> + RVCT:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
>> + GCC:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
>> +
>> ################################################################################
>> #
>> # Pcd Section - list of all EDK II PCD Entries defined by this Platform
>>
>
> Can you please give an R-b for this too?
>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
However, I am still a bit uneasy regarding the lack of explicit
confirmation that it is the responsibility of the person who moves
functions under a '#ifndef DISABLE_NEW_DEPRECATED_INTERFACES' to
ensure that all references have been removed from the *entire*
codebase, not just the intel bits.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (18 preceding siblings ...)
2016-10-21 21:27 ` [PATCH 19/19] ArmVirtPkg: disable deprecated interfaces Laszlo Ersek
@ 2016-10-24 8:04 ` Ard Biesheuvel
2016-10-24 11:50 ` Laszlo Ersek
2016-10-25 8:26 ` Laszlo Ersek
2016-10-24 18:48 ` Jordan Justen
2016-10-25 9:07 ` Laszlo Ersek
21 siblings, 2 replies; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-24 8:04 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-01, Anthony PERARD, Gary Lin, Jordan Justen,
Leif Lindholm, Liming Gao, Michael D Kinney, Michael Zimmermann
On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> This series intends to solve the following BZs:
>
> <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
> the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
> files
>
> <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
> Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
> DSC files
>
> Public branch:
> <https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.
>
> The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
> following MdePkg library class APIs:
>
> BaseLib:
> - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
> - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
> AsciiStrToUnicodeStr.
>
> PcdLib:
> - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
> - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
> PcdSetExBool.
>
> UefiLib:
> - GetVariable, GetEfiGlobalVariable.
>
> The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
> For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
> into ArmPkg a little bit, due to dependencies.
>
> I couldn't build-test some changes (for example, the only compiler
> toolchains I have access to at the moment are GCC48 for Ia32/X64, and
> GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
> test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
> flag). For all of these, I liberally sprinkled the patches with Cc's and
> Notes sections, asking for help. I did make an honest effort to build
> the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
> could think of, perusing the various !if directives in the DSC files.
>
I tried ArmVirtQemu.dsc in DEBUG mode with RVCTLINUX, and it built fine*
Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # RVCT
Thanks,
Ard.
* In general, RVCT tends to fall over quite regularly due to its
finicky diagnostics, so I did have to apply this patch
"""
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
index ca61ac5e1983..1098d9501cc7 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -891,7 +891,7 @@ NorFlashRead (
SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
// Readout the data
- AlignedCopyMem (Buffer, (VOID *)StartAddress + Offset, BufferSizeInBytes);
+ AlignedCopyMem (Buffer, (VOID *)(StartAddress + Offset), BufferSizeInBytes);
return EFI_SUCCESS;
}
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
index 812dafd065b2..0ef7b8d81bbc 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -71,3 +71,7 @@ [Depex]
# NorFlashDxe must be loaded before VariableRuntimeDxe in case
empty flash needs populating with default values
#
BEFORE gVariableRuntimeDxeFileGuid
+
+[BuildOptions]
+ RVCT:*_*_*_CC_FLAGS = --diag_suppress=6314
+
"""
but these changes are entirely unrelated to the series, and I will
follow up with some patches to fix this.
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind
2016-10-24 8:04 ` [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Ard Biesheuvel
@ 2016-10-24 11:50 ` Laszlo Ersek
2016-10-25 8:26 ` Laszlo Ersek
1 sibling, 0 replies; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-24 11:50 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel-01, Anthony PERARD, Gary Lin, Jordan Justen,
Leif Lindholm, Liming Gao, Michael D Kinney, Michael Zimmermann
On 10/24/16 10:04, Ard Biesheuvel wrote:
> On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
>> This series intends to solve the following BZs:
>>
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
>> the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
>> files
>>
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
>> Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
>> DSC files
>>
>> Public branch:
>> <https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.
>>
>> The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
>> following MdePkg library class APIs:
>>
>> BaseLib:
>> - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
>> - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
>> AsciiStrToUnicodeStr.
>>
>> PcdLib:
>> - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
>> - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
>> PcdSetExBool.
>>
>> UefiLib:
>> - GetVariable, GetEfiGlobalVariable.
>>
>> The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
>> For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
>> into ArmPkg a little bit, due to dependencies.
>>
>> I couldn't build-test some changes (for example, the only compiler
>> toolchains I have access to at the moment are GCC48 for Ia32/X64, and
>> GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
>> test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
>> flag). For all of these, I liberally sprinkled the patches with Cc's and
>> Notes sections, asking for help. I did make an honest effort to build
>> the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
>> could think of, perusing the various !if directives in the DSC files.
>>
>
> I tried ArmVirtQemu.dsc in DEBUG mode with RVCTLINUX, and it built fine*
>
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # RVCT
Thank you!
Then,
> * In general, RVCT tends to fall over quite regularly due to its
> finicky diagnostics, so I did have to apply this patch
>
> """
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index ca61ac5e1983..1098d9501cc7 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -891,7 +891,7 @@ NorFlashRead (
> SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
>
> // Readout the data
> - AlignedCopyMem (Buffer, (VOID *)StartAddress + Offset, BufferSizeInBytes);
> + AlignedCopyMem (Buffer, (VOID *)(StartAddress + Offset), BufferSizeInBytes);
>
> return EFI_SUCCESS;
> }
RVCT is correct here; standard C does not permit arithmetic on
pointers-to-void. From C99 for example,
6.5.6 Additive operators
...
Constraints
2 For addition, either both operands shall have arithmetic type, or
one operand shall be a pointer to an object type and the other
shall have integer type. [...]
"Pointer to void" is not "pointer to object type".
Although in
6.2.5 Types
we have
27 A pointer to void shall have the same representation and alignment
requirements as a pointer to a character type. [...]
but also:
19 The void type comprises an empty set of values; it is an
incomplete type that cannot be completed.
So, not an object type.
Can you submit this patch per se? It's a valid and justified change.
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> index 812dafd065b2..0ef7b8d81bbc 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> @@ -71,3 +71,7 @@ [Depex]
> # NorFlashDxe must be loaded before VariableRuntimeDxe in case
> empty flash needs populating with default values
> #
> BEFORE gVariableRuntimeDxeFileGuid
> +
> +[BuildOptions]
> + RVCT:*_*_*_CC_FLAGS = --diag_suppress=6314
> +
> """
No input on this though :)
> but these changes are entirely unrelated to the series, and I will
> follow up with some patches to fix this.
>
Yes, please.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind
2016-10-24 8:04 ` [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Ard Biesheuvel
2016-10-24 11:50 ` Laszlo Ersek
@ 2016-10-25 8:26 ` Laszlo Ersek
2016-10-25 8:45 ` Laszlo Ersek
1 sibling, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-25 8:26 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jordan Justen, edk2-devel-01, Liming Gao, Michael D Kinney,
Gary Lin, Leif Lindholm, Michael Zimmermann
On 10/24/16 10:04, Ard Biesheuvel wrote:
> On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
>> This series intends to solve the following BZs:
>>
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
>> the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
>> files
>>
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
>> Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
>> DSC files
>>
>> Public branch:
>> <https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.
>>
>> The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
>> following MdePkg library class APIs:
>>
>> BaseLib:
>> - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
>> - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
>> AsciiStrToUnicodeStr.
>>
>> PcdLib:
>> - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
>> - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
>> PcdSetExBool.
>>
>> UefiLib:
>> - GetVariable, GetEfiGlobalVariable.
>>
>> The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
>> For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
>> into ArmPkg a little bit, due to dependencies.
>>
>> I couldn't build-test some changes (for example, the only compiler
>> toolchains I have access to at the moment are GCC48 for Ia32/X64, and
>> GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
>> test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
>> flag). For all of these, I liberally sprinkled the patches with Cc's and
>> Notes sections, asking for help. I did make an honest effort to build
>> the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
>> could think of, perusing the various !if directives in the DSC files.
>>
>
> I tried ArmVirtQemu.dsc in DEBUG mode with RVCTLINUX, and it built fine*
>
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # RVCT
Okay, I'll add this tag to the ArmVirtPkg patches.
Thanks,
Laszlo
>
> Thanks,
> Ard.
>
>
> * In general, RVCT tends to fall over quite regularly due to its
> finicky diagnostics, so I did have to apply this patch
>
> """
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index ca61ac5e1983..1098d9501cc7 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -891,7 +891,7 @@ NorFlashRead (
> SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
>
> // Readout the data
> - AlignedCopyMem (Buffer, (VOID *)StartAddress + Offset, BufferSizeInBytes);
> + AlignedCopyMem (Buffer, (VOID *)(StartAddress + Offset), BufferSizeInBytes);
>
> return EFI_SUCCESS;
> }
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> index 812dafd065b2..0ef7b8d81bbc 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> @@ -71,3 +71,7 @@ [Depex]
> # NorFlashDxe must be loaded before VariableRuntimeDxe in case
> empty flash needs populating with default values
> #
> BEFORE gVariableRuntimeDxeFileGuid
> +
> +[BuildOptions]
> + RVCT:*_*_*_CC_FLAGS = --diag_suppress=6314
> +
> """
>
> but these changes are entirely unrelated to the series, and I will
> follow up with some patches to fix this.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind
2016-10-25 8:26 ` Laszlo Ersek
@ 2016-10-25 8:45 ` Laszlo Ersek
2016-10-25 8:49 ` Ard Biesheuvel
0 siblings, 1 reply; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-25 8:45 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jordan Justen, edk2-devel-01, Leif Lindholm, Gary Lin, Liming Gao,
Michael D Kinney, Michael Zimmermann
On 10/25/16 10:26, Laszlo Ersek wrote:
> On 10/24/16 10:04, Ard Biesheuvel wrote:
>> I tried ArmVirtQemu.dsc in DEBUG mode with RVCTLINUX, and it built fine*
>>
>> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # RVCT
>
> Okay, I'll add this tag to the ArmVirtPkg patches.
Actually, I'm adding it to the following non-ArmVirtPkg patches as well:
- MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
- OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
- OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls
because, according to the ArmVirtQemu build report file, these modules
are pulled into ArmVirtQemu.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind
2016-10-25 8:45 ` Laszlo Ersek
@ 2016-10-25 8:49 ` Ard Biesheuvel
0 siblings, 0 replies; 54+ messages in thread
From: Ard Biesheuvel @ 2016-10-25 8:49 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Jordan Justen, edk2-devel-01, Leif Lindholm, Gary Lin, Liming Gao,
Michael D Kinney, Michael Zimmermann
On 25 October 2016 at 09:45, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/25/16 10:26, Laszlo Ersek wrote:
>> On 10/24/16 10:04, Ard Biesheuvel wrote:
>
>>> I tried ArmVirtQemu.dsc in DEBUG mode with RVCTLINUX, and it built fine*
>>>
>>> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # RVCT
>>
>> Okay, I'll add this tag to the ArmVirtPkg patches.
>
> Actually, I'm adding it to the following non-ArmVirtPkg patches as well:
>
> - MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
> - OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
> - OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls
>
> because, according to the ArmVirtQemu build report file, these modules
> are pulled into ArmVirtQemu.
>
That's fine
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (19 preceding siblings ...)
2016-10-24 8:04 ` [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Ard Biesheuvel
@ 2016-10-24 18:48 ` Jordan Justen
2016-10-25 9:07 ` Laszlo Ersek
21 siblings, 0 replies; 54+ messages in thread
From: Jordan Justen @ 2016-10-24 18:48 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Anthony PERARD, Ard Biesheuvel, Gary Lin, Leif Lindholm,
Liming Gao, Michael D Kinney, Michael Zimmermann
Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
On 2016-10-21 14:27:18, Laszlo Ersek wrote:
> This series intends to solve the following BZs:
>
> <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
> the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
> files
>
> <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
> Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
> DSC files
>
> Public branch:
> <https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.
>
> The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
> following MdePkg library class APIs:
>
> BaseLib:
> - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
> - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
> AsciiStrToUnicodeStr.
>
> PcdLib:
> - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
> - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
> PcdSetExBool.
>
> UefiLib:
> - GetVariable, GetEfiGlobalVariable.
>
> The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
> For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
> into ArmPkg a little bit, due to dependencies.
>
> I couldn't build-test some changes (for example, the only compiler
> toolchains I have access to at the moment are GCC48 for Ia32/X64, and
> GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
> test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
> flag). For all of these, I liberally sprinkled the patches with Cc's and
> Notes sections, asking for help. I did make an honest effort to build
> the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
> could think of, perusing the various !if directives in the DSC files.
>
> I recommend the "--word-diff" option of git-show, as a tool to assist
> with the review.
>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (19):
> MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
> OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls
> OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls
> OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls
> OvmfPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls
> OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
> OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls
> OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe: eliminate unchecked PcdSetXX()
> calls
> OvmfPkg: disable deprecated interfaces
> ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls
> ArmVirtPkg/ArmVirtPL031FdtClientLib: eliminate unchecked PcdSetXX()
> calls
> ArmVirtPkg/ArmVirtPlatformLib: eliminate unchecked PcdSetXX() calls
> ArmVirtPkg/ArmVirtTimerFdtClientLib: eliminate unchecked PcdSetXX()
> calls
> ArmVirtPkg/FdtPciPcdProducerLib: eliminate unchecked PcdSetXX() calls
> ArmVirtPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX()
> calls
> ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()
> ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with
> AsciiStrCatS()
> ArmVirtPkg: disable deprecated interfaces
>
> ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c | 22 +++++-----
> ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c | 20 ++++-----
> ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 ++-
> ArmVirtPkg/ArmVirt.dsc.inc | 6 +++
> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 13 ++++--
> ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c | 4 +-
> ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c | 18 ++++----
> ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c | 13 ++++--
> ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 10 +++--
> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 6 ++-
> MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++
> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 12 ++++--
> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 14 +++++--
> OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c | 10 +++--
> OvmfPkg/OvmfPkgIa32.dsc | 7 ++++
> OvmfPkg/OvmfPkgIa32X64.dsc | 7 ++++
> OvmfPkg/OvmfPkgX64.dsc | 7 ++++
> OvmfPkg/PlatformDxe/Platform.c | 8 +++-
> OvmfPkg/PlatformPei/MemDetect.c | 11 +++--
> OvmfPkg/PlatformPei/Platform.c | 44 +++++++++++++-------
> OvmfPkg/PlatformPei/Xen.c | 5 ++-
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 13 ++++--
> OvmfPkg/XenBusDxe/XenBusDxe.inf | 1 +
> OvmfPkg/XenBusDxe/XenStore.c | 22 +++++-----
> 24 files changed, 213 insertions(+), 92 deletions(-)
>
> --
> 2.9.2
>
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind
2016-10-21 21:27 [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind Laszlo Ersek
` (20 preceding siblings ...)
2016-10-24 18:48 ` Jordan Justen
@ 2016-10-25 9:07 ` Laszlo Ersek
21 siblings, 0 replies; 54+ messages in thread
From: Laszlo Ersek @ 2016-10-25 9:07 UTC (permalink / raw)
To: edk2-devel-01
Cc: Ard Biesheuvel, Jordan Justen, Liming Gao, Michael D Kinney,
Gary Lin, Leif Lindholm, Michael Zimmermann
On 10/21/16 23:27, Laszlo Ersek wrote:
> This series intends to solve the following BZs:
>
> <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
> the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
> files
>
> <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
> Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
> DSC files
>
> Public branch:
> <https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.
>
> The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
> following MdePkg library class APIs:
>
> BaseLib:
> - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
> - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
> AsciiStrToUnicodeStr.
>
> PcdLib:
> - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
> - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
> PcdSetExBool.
>
> UefiLib:
> - GetVariable, GetEfiGlobalVariable.
>
> The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
> For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
> into ArmPkg a little bit, due to dependencies.
>
> I couldn't build-test some changes (for example, the only compiler
> toolchains I have access to at the moment are GCC48 for Ia32/X64, and
> GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
> test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
> flag). For all of these, I liberally sprinkled the patches with Cc's and
> Notes sections, asking for help. I did make an honest effort to build
> the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
> could think of, perusing the various !if directives in the DSC files.
>
> I recommend the "--word-diff" option of git-show, as a tool to assist
> with the review.
>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (19):
> MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
> OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls
> OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls
> OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls
> OvmfPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls
> OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
> OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls
> OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe: eliminate unchecked PcdSetXX()
> calls
> OvmfPkg: disable deprecated interfaces
> ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls
> ArmVirtPkg/ArmVirtPL031FdtClientLib: eliminate unchecked PcdSetXX()
> calls
> ArmVirtPkg/ArmVirtPlatformLib: eliminate unchecked PcdSetXX() calls
> ArmVirtPkg/ArmVirtTimerFdtClientLib: eliminate unchecked PcdSetXX()
> calls
> ArmVirtPkg/FdtPciPcdProducerLib: eliminate unchecked PcdSetXX() calls
> ArmVirtPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX()
> calls
> ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()
> ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with
> AsciiStrCatS()
> ArmVirtPkg: disable deprecated interfaces
>
> ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c | 22 +++++-----
> ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c | 20 ++++-----
> ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 ++-
> ArmVirtPkg/ArmVirt.dsc.inc | 6 +++
> ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 13 ++++--
> ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c | 4 +-
> ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c | 18 ++++----
> ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c | 13 ++++--
> ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 10 +++--
> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 6 ++-
> MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++
> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 12 ++++--
> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 14 +++++--
> OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c | 10 +++--
> OvmfPkg/OvmfPkgIa32.dsc | 7 ++++
> OvmfPkg/OvmfPkgIa32X64.dsc | 7 ++++
> OvmfPkg/OvmfPkgX64.dsc | 7 ++++
> OvmfPkg/PlatformDxe/Platform.c | 8 +++-
> OvmfPkg/PlatformPei/MemDetect.c | 11 +++--
> OvmfPkg/PlatformPei/Platform.c | 44 +++++++++++++-------
> OvmfPkg/PlatformPei/Xen.c | 5 ++-
> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 13 ++++--
> OvmfPkg/XenBusDxe/XenBusDxe.inf | 1 +
> OvmfPkg/XenBusDxe/XenStore.c | 22 +++++-----
> 24 files changed, 213 insertions(+), 92 deletions(-)
>
Thanks for the feedback everyone, pushed the series as
2a3263303b51..17dc8ebc0d76. Beyond adding the tags, I implemented the
following last-minute changes, as requested:
- drop extraneous space in "AsciiStrCatS ()" [Jordan]
- introduce CPSR_STRING_SIZE, and replace the open-coded occurrences of
the integer constant 32 with it [Ard]
- remove superfluous return statement [Ard]
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 54+ messages in thread