First, please fix your mail user agent -- emails should be properly threaded.
(For the archive: this was in response to <http://thread.gmane.org/gmane.comp.bios.edk2.devel/11175/focus=11177>.) On 04/25/16 20:24, Zenith432 wrote: > On 25/04/2016 08:03 PM, Laszlo Ersek wrote: >> Second, the commit message should explain why the correction is being made. > > There's an explanation with more detail here > http://www.insanelymac.com/forum/topic/304530-clover-change-explanations/?p=2234289 Thanks for the reference, it is interesting. As Marvin pointed out, the patches should come with documentation (non-empty commit messages). I think the argument made in the message linked above is valid and relevant, but the commit messages on the edk2 patches should make the same argument in edk2 terminology. Therefore I recommend the following commit messages: * Patch #1: -------- MdeModulePkg: Variable: add missing VA_END() invocations According to "MdePkg/Include/Base.h", VA_COPY() "initializes Dest as a copy of Start, as if the VA_START macro had been applied to Dest [...]". This implies that each VA_COPY() has to be balanced with VA_END(). Update the CheckRemainingSpaceForConsistencyInternal() function to conform to this requirement. Contributed-under: ... Signed-off-by: ... -------- * Patch #2: -------- MdePkg: UefiDevicePathLib: restart VA_LIST before reuse The VA_*() macros from "MdePkg/Include/Base.h" are modeled after ANSI C, according to the header file -- it says "Support for variable length argument lists using the ANSI standard". ISO C89 says in "7.8 Variable arguments <stdarg.h>", The object *ap* may be passed as an argument to another function; if that function invokes the *va_arg* macro with parameter *ap*, the value of *ap* in the calling function is indeterminate and shall be passed to the *va_end* macro prior to any further reference to *ap*. (The same can be found in ISO C99 7.15.) The UefiDevicePathLibCatPrint() function doesn't conform to this requirement, fix it. Contributed-under: ... Signed-off-by: ... -------- * Patch #3: -------- OvmfPkg: XenBusDxe: prevent reuse of possibly modified VA_LIST The VA_*() macros from "MdePkg/Include/Base.h" are modeled after ANSI C, according to the header file -- it says "Support for variable length argument lists using the ANSI standard". ISO C89 says in "7.8 Variable arguments <stdarg.h>", The object *ap* may be passed as an argument to another function; if that function invokes the *va_arg* macro with parameter *ap*, the value of *ap* in the calling function is indeterminate and shall be passed to the *va_end* macro prior to any further reference to *ap*. (The same can be found in ISO C99 7.15.) The XenStoreVSPrint() function doesn't conform to this requirement. Fix it by creating a separate VA_LIST object with VA_COPY, for the first use. Contributed-under: ... Signed-off-by: ... -------- Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
