On 04/25/16 18:23, Zenith432 wrote:
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zenith432 <[email protected]>
> ---
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 2 ++
> MdePkg/Library/UefiDevicePathLib/DevicePathToText.c | 6 ++++--
> OvmfPkg/XenBusDxe/XenStore.c | 5 ++++-
> 3 files changed, 10 insertions(+), 3 deletions(-)
First, this patch should be split into three separate patches (one per
top-level pkg). The subject lines should say something like:
MdeModulePkg: Variable: add missing VA_END() invocations
MdePkg: UefiDevicePathLib: add missing VA_END() invocations
OvmfPkg: XenBusDxe: add missing VA_END() invocations
Second, the commit message should explain why the correction is being made.
Third, I have specific comments too:
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 3f0240b..e1c3631 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -1771,6 +1771,7 @@ CheckRemainingSpaceForConsistencyInternal (
> TotalNeededSize += VariableEntry->VariableSize;
> VariableEntry = VA_ARG (Args, VARIABLE_ENTRY_CONSISTENCY *);
> }
> + VA_END (Args);
I agree.
>
> if (RemainingVariableStorageSize >= TotalNeededSize) {
> //
> @@ -1823,6 +1824,7 @@ CheckRemainingSpaceForConsistencyInternal (
> RemainingVariableStorageSize -= VariableEntry->VariableSize;
> VariableEntry = VA_ARG (Args, VARIABLE_ENTRY_CONSISTENCY *);
> }
> + VA_END (Args);
>
> return TRUE;
> }
I agree. These two VA_END() macro invocations are paired with the
preexistent (currently unbalanced) VA_COPY() invocations that are not
shown in the context.
> diff --git a/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> b/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> index 92db3b1..b936f85 100644
> --- a/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> +++ b/MdePkg/Library/UefiDevicePathLib/DevicePathToText.c
> @@ -38,10 +38,12 @@ UefiDevicePathLibCatPrint (
> )
> {
> UINTN Count;
> - VA_LIST Args;
> + VA_LIST Args, Args2;
>
> VA_START (Args, Fmt);
> - Count = SPrintLength (Fmt, Args);
> + VA_COPY (Args2, Args);
> + Count = SPrintLength (Fmt, Args2);
> + VA_END(Args2);
>
> if ((Str->Count + (Count + 1)) * sizeof (CHAR16) > Str->Capacity) {
> Str->Capacity = (Str->Count + (Count + 1) * 2) * sizeof (CHAR16);
I agree that the current code is not correct, but instead of creating a
copy, I propose to simply call VA_END() and VA_START() again, right
after SPrintLength().
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index 61976f9..aea887b 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -1319,8 +1319,11 @@ XenStoreVSPrint (
> CHAR8 *Buf;
> XENSTORE_STATUS Status;
> UINTN BufSize;
> + VA_LIST Marker2;
>
> - BufSize = SPrintLengthAsciiFormat (FormatString, Marker) + 1;
> + VA_COPY(Marker2, Marker);
> + BufSize = SPrintLengthAsciiFormat (FormatString, Marker2) + 1;
> + VA_END(Marker2);
> Buf = AllocateZeroPool (BufSize);
> AsciiVSPrint (Buf, BufSize, FormatString, Marker);
> Status = XenStoreWrite (Transaction, DirectoryPath, Node, Buf);
>
I agree.
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
