[libvirt] [PATCH 4/4] test: Fix possible ap leak for VIR_TEST_PRELOAD

[John Ferlan]

On 06/28/2017 07:45 AM, Peter Krempa wrote:
> On Wed, Jun 28, 2017 at 07:37:38 -0400, John Ferlan wrote:
>> Commit id 'f0469c61e' added the ability to load multiple mock libraries
>> by using the va_start() varable args marker; however, if something within
>> the VIR_TEST_PRELOAD fails, then control is returned to caller without
>> calling va_end. So add a couple more markers to indicat the need to call
>> va_end w/ the ap before returning.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  tests/qemucapsprobe.c | 2 +-
>>  tests/testutils.c     | 4 ++--
>>  tests/testutils.h     | 6 +++++-
>>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> [...]
> 
>> diff --git a/tests/testutils.h b/tests/testutils.h
>> index c7f02e4..e4c602b 100644
>> --- a/tests/testutils.h
>> +++ b/tests/testutils.h
>> @@ -115,12 +115,14 @@ int virTestMain(int argc,
>>          return virTestMain(argc, argv, func, NULL);     \
>>      }
>>  
>> -# define VIR_TEST_PRELOAD(lib)                                          \
>> +# define VIR_TEST_PRELOAD(lib, haveap, _ap)                             \
>>      do {                                                                \
>>          const char *preload = getenv("LD_PRELOAD");                     \
>>          if (preload == NULL || strstr(preload, lib) == NULL) {          \
>>              char *newenv;                                               \
>>              if (!virFileIsExecutable(lib)) {                            \
>> +                if (haveap)                                             \
>> +                    va_end(_ap);                                        \
>>                  perror(lib);                                            \
> 
> NACK, this is gross. Please make this macro jump to a label passed as
> argument so that the caller can do cleanup.
> 

yuck - this was the cheaper/simpler approach. Not a fan of labels in the
macro... It would mean that virTestMain gets two labels (e.g. "error:"
and "va_error:") with a lot of code in between that returns EXIT_FAILURE
that could conceivably also "goto error;", but won't... I dunno, I just
find that worse.  Similar for main in qemucapsprobe.c which would get an
error: label to return EXIT_FAILURE even though there's 5 other such
returns prior to that which don't goto error;

I'll just drop this and keep my local copy as is. If someone wants to be
more elegant, then have at it ;-)

Thanks for the quick reviews on these.

John