On 04/25/16 18:23, Zenith432 wrote: > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Zenith432 <zenith...@users.sourceforge.net> > --- > 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel