[libvirt] [PATCH v2 0/4] util: abort when out of memory
This is the first part of a previously posted series: https://www.redhat.com/archives/libvir-list/2019-August/msg01374.html I've temporarily split the addition of glib into a separate branch until we get clarity on guarantees around g_malloc() and mallc() using the same allocator. I published a docs update https://gitlab.gnome.org/GNOME/glib/merge_requests/1099/ to attempt to get confirmation from glib maintainers on this matter. Changed in v2: - Keep ATTRIBUTE_RETURN_CHECK annotations. We're aborting on OOM, but don't want to update all callers to drop return value checks yet - Update docs for virAsprintfQuiet too Daniel P. Berrangé (4): util: purge all code for testing OOM handling util: make allocation functions abort on OOM util: remove several unused _QUIET allocation macro variants util: make string functions abort on OOM configure.ac | 17 -- docs/docs.html.in | 3 - docs/internals/oomtesting.html.in | 213 -------------------- src/libvirt_private.syms | 4 - src/util/viralloc.c | 314 +++++------------------------- src/util/viralloc.h | 192 ++++-------------- src/util/virstring.c | 93 +++------ src/util/virstring.h | 73 +++---- tests/Makefile.am | 1 - tests/oomtrace.pl | 41 ---- tests/qemuxml2argvtest.c | 12 +- tests/testutils.c | 189 +----------------- tests/testutils.h | 2 - tests/virfirewalltest.c | 12 -- 14 files changed, 136 insertions(+), 1030 deletions(-) delete mode 100644 docs/internals/oomtesting.html.in delete mode 100755 tests/oomtrace.pl -- 2.21.0
The OOM handling requires special build time options which we never enable in our CI. Even once enabled the tests are incredibly slow and typically require manual inspection of the results to weed out false positives. Since there was previous agreement to switch to abort on OOM in libvirt code, there's no point continuing to keep the unused OOM testing code. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- configure.ac | 17 --- docs/docs.html.in | 3 - docs/internals/oomtesting.html.in | 213 ------------------------------ src/libvirt_private.syms | 4 - src/util/viralloc.c | 111 ---------------- src/util/viralloc.h | 5 - tests/Makefile.am | 1 - tests/oomtrace.pl | 41 ------ tests/qemuxml2argvtest.c | 12 +- tests/testutils.c | 189 +------------------------- tests/testutils.h | 2 - tests/virfirewalltest.c | 12 -- 12 files changed, 6 insertions(+), 604 deletions(-) delete mode 100644 docs/internals/oomtesting.html.in delete mode 100755 tests/oomtrace.pl diff --git a/configure.ac b/configure.ac index bf9e7681bc..8cb7de9c19 100644 --- a/configure.ac +++ b/configure.ac @@ -764,22 +764,6 @@ if test "$enable_test_coverage" = yes; then WARN_CFLAGS=$save_WARN_CFLAGS fi -LIBVIRT_ARG_ENABLE([TEST_OOM], [memory allocation failure checking], [no]) -case "$enable_test_oom" in - yes|no) ;; - *) AC_MSG_ERROR([bad value ${enable_test_oom} for test-oom option]) ;; -esac - -if test "$enable_test_oom" = yes; then - have_trace=yes - AC_CHECK_HEADER([execinfo.h],[],[have_trace=no]) - AC_CHECK_FUNC([backtrace],[],[have_trace=no]) - if test "$have_trace" = "yes"; then - AC_DEFINE([TEST_OOM_TRACE], 1, [Whether backtrace() is available]) - fi - AC_DEFINE([TEST_OOM], 1, [Whether malloc OOM checking is enabled]) -fi - LIBVIRT_ARG_ENABLE([TEST_LOCKING], [thread locking tests using CIL], [no]) case "$enable_test_locking" in yes|no) ;; @@ -1048,7 +1032,6 @@ AC_MSG_NOTICE([]) AC_MSG_NOTICE([Test suite]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([ Coverage: $enable_test_coverage]) -AC_MSG_NOTICE([ Alloc OOM: $enable_test_oom]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Miscellaneous]) AC_MSG_NOTICE([]) diff --git a/docs/docs.html.in b/docs/docs.html.in index ba9514279a..c1741c89b4 100644 --- a/docs/docs.html.in +++ b/docs/docs.html.in @@ -155,9 +155,6 @@ <dt><a href="internals/locking.html">Lock managers</a></dt> <dd>Use lock managers to protect disk content</dd> - <dt><a href="internals/oomtesting.html">Out of memory testing</a></dt> - <dd>Simulating OOM conditions in the test suite</dd> - <dt><a href="testsuites.html">Functional testing</a></dt> <dd>Testing libvirt with <a href="testtck.html">TCK test suite</a> and <a href="testapi.html">Libvirt-test-API</a></dd> diff --git a/docs/internals/oomtesting.html.in b/docs/internals/oomtesting.html.in deleted file mode 100644 index 72d0f2c6ff..0000000000 --- a/docs/internals/oomtesting.html.in +++ /dev/null @@ -1,213 +0,0 @@ -<?xml version="1.0" encoding="UTF-8"?> -<!DOCTYPE html> -<html xmlns="http://www.w3.org/1999/xhtml"> - <body> - <h1>Out of memory testing</h1> - - <ul id="toc"></ul> - - - <p> - This page describes how to use the test suite todo out of memory - testing. - </p> - - <h2>Building with OOM testing</h2> - - <p> - Since OOM testing requires hooking into the malloc APIs, it is - not enabled by default. The flag <code>--enable-test-oom</code> - must be given to <code>configure</code>. When this is done the - libvirt allocation APIs will have some hooks enabled. - </p> - - <pre> -$ ./configure --enable-test-oom -</pre> - - - <h2><a id="basicoom">Basic OOM testing support</a></h2> - - <p> - The first step in validating OOM usage is to run a test suite - with full OOM testing enabled. This is done by setting the - <code>VIR_TEST_OOM=1</code> environment variable. The way this - works is that it runs the test once normally to "prime" any - static memory allocations. Then it runs it once more counting - the total number of memory allocations. Then it runs it in a - loop failing a different memory allocation each time. For every - memory allocation failure triggered, it expects the test case - to return an error. OOM testing is quite slow requiring each - test case to be executed O(n) times, where 'n' is the total - number of memory allocations. This results in a total number - of memory allocations of '(n * (n + 1) ) / 2' - </p> - - <pre> -$ VIR_TEST_OOM=1 ./qemuxml2argvtest - 1) QEMU XML-2-ARGV minimal ... OK - Test OOM for nalloc=42 .......................................... OK - 2) QEMU XML-2-ARGV minimal-s390 ... OK - Test OOM for nalloc=28 ............................ OK - 3) QEMU XML-2-ARGV machine-aliases1 ... OK - Test OOM for nalloc=38 ...................................... OK - 4) QEMU XML-2-ARGV machine-aliases2 ... OK - Test OOM for nalloc=38 ...................................... OK - 5) QEMU XML-2-ARGV machine-core-on ... OK - Test OOM for nalloc=37 ..................................... OK -...snip... -</pre> - - <p> - In this output, the first line shows the normal execution and - the test number, and the second line shows the total number - of memory allocations from that test case. - </p> - - <h3><a id="valgrind">Tracking failures with valgrind</a></h3> - - <p> - The test suite should obviously *not* crash during OOM testing. - If it does crash, then to assist in tracking down the problem - it is worth using valgrind and only running a single test case. - For example, supposing test case 5 crashed. Then re-run the - test with - </p> - - <pre> -$ VIR_TEST_OOM=1 VIR_TEST_RANGE=5 ../run valgrind ./qemuxml2argvtest -...snip... - 5) QEMU XML-2-ARGV machine-core-on ... OK - Test OOM for nalloc=37 ..................................... OK -...snip... - </pre> - - <p> - Valgrind should report the cause of the crash - for example a - double free or use of uninitialized memory or NULL pointer - access. - </p> - - <h3><a id="stacktraces">Tracking failures with stack traces</a></h3> - - <p> - With some really difficult bugs valgrind is not sufficient to - identify the cause. In this case, it is useful to identify the - precise allocation which was failed, to allow the code path - to the error to be traced. The <code>VIR_TEST_OOM</code> - env variable can be given a range of memory allocations to - test. So if a test case has 150 allocations, it can be told - to only test allocation numbers 7-10. The <code>VIR_TEST_OOM_TRACE</code> - variable can be used to print out stack traces. - </p> - - <pre> -$ VIR_TEST_OOM_TRACE=2 VIR_TEST_OOM=1:7-10 VIR_TEST_RANGE=5 \ - ../run valgrind ./qemuxml2argvtest - 5) QEMU XML-2-ARGV machine-core-on ... OK - Test OOM for nalloc=37 !virAllocN -/home/berrange/src/virt/libvirt/src/util/viralloc.c:180 -virDomainDefParseXML -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:11786 (discriminator 1) -virDomainDefParseNode -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:12677 -virDomainDefParse -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:12621 -testCompareXMLToArgvFiles -/home/berrange/src/virt/libvirt/tests/qemuxml2argvtest.c:107 -virtTestRun -/home/berrange/src/virt/libvirt/tests/testutils.c:266 -mymain -/home/berrange/src/virt/libvirt/tests/qemuxml2argvtest.c:388 (discriminator 2) -virtTestMain -/home/berrange/src/virt/libvirt/tests/testutils.c:791 -__libc_start_main -??:? -_start -??:? -!virAlloc -/home/berrange/src/virt/libvirt/src/util/viralloc.c:133 -virDomainDiskDefParseXML -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:4790 -virDomainDefParseXML -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:11797 -virDomainDefParseNode -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:12677 -virDomainDefParse -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:12621 -testCompareXMLToArgvFiles -/home/berrange/src/virt/libvirt/tests/qemuxml2argvtest.c:107 -virtTestRun -/home/berrange/src/virt/libvirt/tests/testutils.c:266 -mymain -/home/berrange/src/virt/libvirt/tests/qemuxml2argvtest.c:388 (discriminator 2) -virtTestMain -/home/berrange/src/virt/libvirt/tests/testutils.c:791 -__libc_start_main -??:? -_start -??:? -!virAllocN -/home/berrange/src/virt/libvirt/src/util/viralloc.c:180 -virXPathNodeSet -/home/berrange/src/virt/libvirt/src/util/virxml.c:609 -virDomainDefParseXML -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:11805 -virDomainDefParseNode -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:12677 -virDomainDefParse -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:12621 -testCompareXMLToArgvFiles -/home/berrange/src/virt/libvirt/tests/qemuxml2argvtest.c:107 -virtTestRun -/home/berrange/src/virt/libvirt/tests/testutils.c:266 -mymain -/home/berrange/src/virt/libvirt/tests/qemuxml2argvtest.c:388 (discriminator 2) -virtTestMain -/home/berrange/src/virt/libvirt/tests/testutils.c:791 -__libc_start_main -??:? -_start -??:? -!virAllocN -/home/berrange/src/virt/libvirt/src/util/viralloc.c:180 -virDomainDefParseXML -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:11808 (discriminator 1) -virDomainDefParseNode -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:12677 -virDomainDefParse -/home/berrange/src/virt/libvirt/src/conf/domain_conf.c:12621 -testCompareXMLToArgvFiles -/home/berrange/src/virt/libvirt/tests/qemuxml2argvtest.c:107 -virtTestRun -/home/berrange/src/virt/libvirt/tests/testutils.c:266 -mymain -/home/berrange/src/virt/libvirt/tests/qemuxml2argvtest.c:388 (discriminator 2) -virtTestMain -/home/berrange/src/virt/libvirt/tests/testutils.c:791 -__libc_start_main -??:? -_start -??:? - </pre> - - <h3><a id="noncrash">Non-crash related problems</a></h3> - - <p> - Not all memory allocation bugs result in code crashing. Sometimes - the code will be silently ignoring the allocation failure, resulting - in incorrect data being produced. For example the XML parser may - mistakenly treat an allocation failure as indicating that an XML - attribute was not set in the input document. It is hard to identify - these problems from the test suite automatically. For this, the - test suites should be run with <code>VIR_TEST_DEBUG=1</code> set - and then stderr analysed for any unexpected data. For example, - the XML conversion may show an embedded "(null)" literal, or the - test suite might complain about missing elements / attributes - in the actual vs expected data. These are all signs of bugs in - OOM handling. In the future the OOM tests will be enhanced to - validate that an error VIR_ERR_NO_MEMORY is returned for each - allocation failed, rather than some other error. - </p> - </body> -</html> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 918f81470b..9dde419d6e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1484,10 +1484,6 @@ virSecurityManagerVerify; # util/viralloc.h virAlloc; virAllocN; -virAllocTestCount; -virAllocTestHook; -virAllocTestInit; -virAllocTestOOM; virAllocVar; virDeleteElementsN; virDispose; diff --git a/src/util/viralloc.c b/src/util/viralloc.c index e82bfa0acd..5a0adcc706 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -30,80 +30,6 @@ VIR_LOG_INIT("util.alloc"); -#if TEST_OOM -static int testMallocNext; -static int testMallocFailFirst; -static int testMallocFailLast; -static void (*testMallocHook)(int, void*); -static void *testMallocHookData; - -void virAllocTestInit(void) -{ - testMallocNext = 1; - testMallocFailFirst = 0; - testMallocFailLast = 0; -} - -int virAllocTestCount(void) -{ - return testMallocNext - 1; -} - -void virAllocTestHook(void (*func)(int, void*), void *data) -{ - testMallocHook = func; - testMallocHookData = data; -} - -void virAllocTestOOM(int n, int m) -{ - testMallocNext = 1; - testMallocFailFirst = n; - testMallocFailLast = n + m - 1; -} - -static int virAllocTestFail(void) -{ - int fail = 0; - if (testMallocNext == 0) - return 0; - - fail = - testMallocNext >= testMallocFailFirst && - testMallocNext <= testMallocFailLast; - - if (fail && testMallocHook) - (testMallocHook)(testMallocNext, testMallocHookData); - - testMallocNext++; - return fail; -} - -#else - -void virAllocTestOOM(int n ATTRIBUTE_UNUSED, - int m ATTRIBUTE_UNUSED) -{ - /* nada */ -} - -int virAllocTestCount(void) -{ - return 0; -} - -void virAllocTestInit(void) -{ - /* nada */ -} - -void virAllocTestHook(void (*func)(int, void*) ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED) -{ - /* nada */ -} -#endif - /** * virAlloc: @@ -130,16 +56,6 @@ int virAlloc(void *ptrptr, const char *funcname, size_t linenr) { -#if TEST_OOM - if (virAllocTestFail()) { - *(void **)ptrptr = NULL; - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - errno = ENOMEM; - return -1; - } -#endif - *(void **)ptrptr = calloc(1, size); if (*(void **)ptrptr == NULL) { if (report) @@ -177,16 +93,6 @@ int virAllocN(void *ptrptr, const char *funcname, size_t linenr) { -#if TEST_OOM - if (virAllocTestFail()) { - *(void **)ptrptr = NULL; - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - errno = ENOMEM; - return -1; - } -#endif - *(void**)ptrptr = calloc(count, size); if (*(void**)ptrptr == NULL) { if (report) @@ -226,14 +132,6 @@ int virReallocN(void *ptrptr, size_t linenr) { void *tmp; -#if TEST_OOM - if (virAllocTestFail()) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - errno = ENOMEM; - return -1; - } -#endif if (xalloc_oversized(count, size)) { if (report) @@ -539,15 +437,6 @@ int virAllocVar(void *ptrptr, { size_t alloc_size = 0; -#if TEST_OOM - if (virAllocTestFail()) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - errno = ENOMEM; - return -1; - } -#endif - if (VIR_ALLOC_VAR_OVERSIZED(struct_size, count, element_size)) { if (report) virReportOOMErrorFull(domcode, filename, funcname, linenr); diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 2b82096fde..3e169e272c 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -600,11 +600,6 @@ void virDisposeString(char **strptr) sizeof(*(ptr)), NULL) -void virAllocTestInit(void); -int virAllocTestCount(void); -void virAllocTestOOM(int n, int m); -void virAllocTestHook(void (*func)(int, void*), void *data); - /** * VIR_AUTOFREE: * @type: type of the variable to be freed automatically diff --git a/tests/Makefile.am b/tests/Makefile.am index 8c343857a5..e1bcd10c7c 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -109,7 +109,6 @@ EXTRA_DIST = \ nwfilterxml2firewalldata \ nwfilterxml2xmlin \ nwfilterxml2xmlout \ - oomtrace.pl \ qemuagentdata \ qemublocktestdata \ qemucapabilitiesdata \ diff --git a/tests/oomtrace.pl b/tests/oomtrace.pl deleted file mode 100755 index f799262f2c..0000000000 --- a/tests/oomtrace.pl +++ /dev/null @@ -1,41 +0,0 @@ -#!/usr/bin/env perl - -use strict; -use warnings; - -(my $ME = $0) =~ s|.*/||; -# use File::Coda; # http://meyering.net/code/Coda/ -END { - defined fileno STDOUT or return; - close STDOUT and return; - warn "$ME: failed to close standard output: $!\n"; - $? ||= 1; -} - - -my @data = <>; - - -my %trace; -my %lines; - -foreach (@data) { - if (/^\s*TRACE:\s+(\S+?)(?:\(.*\))?\s+\[0x(.*)\]\s*$/ ) { - $trace{$2} = $1; - } -} - -foreach my $key (keys %trace) { - my $val = $trace{$key}; - my $info = $val =~ /\?\?/ ? $val : `addr2line -e $val $key`; - $lines{$key} = $info; -} - - -foreach (@data) { - if (/^\s*TRACE:\s+(\S+?)(?:\(.*\))?\s+\[0x(.*)\]\s*$/ ) { - print $lines{$2}; - } else { - print; - } -} diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1f2ae5958a..abff3d1c61 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -567,14 +567,12 @@ testCompareXMLToArgv(const void *data) VIR_TEST_DEBUG("Error expected but there wasn't any."); goto cleanup; } - if (!virTestOOMActive()) { - if (flags & FLAG_EXPECT_FAILURE) { - if ((log = virTestLogContentAndReset())) - VIR_TEST_DEBUG("Got expected error: \n%s", log); - } - virResetLastError(); - ret = 0; + if (flags & FLAG_EXPECT_FAILURE) { + if ((log = virTestLogContentAndReset())) + VIR_TEST_DEBUG("Got expected error: \n%s", log); } + virResetLastError(); + ret = 0; cleanup: VIR_FREE(log); diff --git a/tests/testutils.c b/tests/testutils.c index 003893dc44..064460b4dc 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -42,13 +42,6 @@ #include "virprocess.h" #include "virstring.h" -#ifdef TEST_OOM -# ifdef TEST_OOM_TRACE -# include <dlfcn.h> -# include <execinfo.h> -# endif -#endif - #define VIR_FROM_THIS VIR_FROM_NONE VIR_LOG_INIT("tests.testutils"); @@ -61,17 +54,6 @@ static unsigned int testVerbose = -1; static unsigned int testExpensive = -1; static unsigned int testRegenerate = -1; -#ifdef TEST_OOM -static unsigned int testOOM; -static unsigned int testOOMStart = -1; -static unsigned int testOOMEnd = -1; -static unsigned int testOOMTrace; -# ifdef TEST_OOM_TRACE -void *testAllocStack[30]; -int ntestAllocStack; -# endif -#endif -static bool testOOMActive; static size_t testCounter; static virBitmapPtr testBitmap; @@ -79,11 +61,6 @@ static virBitmapPtr testBitmap; char *progname; static char *perl; -bool virTestOOMActive(void) -{ - return testOOMActive; -} - static int virTestUseTerminalColors(void) { return isatty(STDOUT_FILENO); @@ -104,42 +81,6 @@ virTestGetFlag(const char *name) return flag; } -#ifdef TEST_OOM_TRACE -static void virTestAllocHook(int nalloc ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) -{ - ntestAllocStack = backtrace(testAllocStack, ARRAY_CARDINALITY(testAllocStack)); -} -#endif - -#ifdef TEST_OOM_TRACE -static void -virTestShowTrace(void) -{ - size_t j; - for (j = 2; j < ntestAllocStack; j++) { - Dl_info info; - char *cmd; - - dladdr(testAllocStack[j], &info); - if (info.dli_fname && - strstr(info.dli_fname, ".so")) { - if (virAsprintf(&cmd, ADDR2LINE " -f -e %s %p", - info.dli_fname, - ((void*)((unsigned long long)testAllocStack[j] - - (unsigned long long)info.dli_fbase))) < 0) - continue; - } else { - if (virAsprintf(&cmd, ADDR2LINE " -f -e %s %p", - (char*)(info.dli_fname ? info.dli_fname : "<unknown>"), - testAllocStack[j]) < 0) - continue; - } - ignore_value(system(cmd)); - VIR_FREE(cmd); - } -} -#endif /* * Runs test @@ -207,76 +148,6 @@ virTestRun(const char *title, fprintf(stderr, "!"); } -#ifdef TEST_OOM - if (testOOM && ret != EXIT_AM_SKIP) { - int nalloc; - int oomret; - int start, end; - size_t i; - virResetLastError(); - virAllocTestInit(); -# ifdef TEST_OOM_TRACE - virAllocTestHook(virTestAllocHook, NULL); -# endif - oomret = body(data); - nalloc = virAllocTestCount(); - fprintf(stderr, " Test OOM for nalloc=%d ", nalloc); - if (testOOMStart == -1 || - testOOMEnd == -1) { - start = 0; - end = nalloc; - } else { - start = testOOMStart; - end = testOOMEnd + 1; - } - testOOMActive = true; - for (i = start; i < end; i++) { - bool missingFail = false; -# ifdef TEST_OOM_TRACE - memset(testAllocStack, 0, sizeof(testAllocStack)); - ntestAllocStack = 0; -# endif - virAllocTestOOM(i + 1, 1); - oomret = body(data); - - /* fprintf() disabled because XML parsing APIs don't allow - * distinguish between element / attribute not present - * in the XML (which is non-fatal), vs OOM / malformed - * which should be fatal. Thus error reporting for - * optionally present XML is mostly broken. - */ - if (oomret == 0) { - missingFail = true; -# if 0 - fprintf(stderr, " alloc %zu failed but no err status\n", i + 1); -# endif - } else { - if (virGetLastErrorCode() == VIR_ERR_OK) { -# if 0 - fprintf(stderr, " alloc %zu failed but no error report\n", i + 1); -# endif - missingFail = true; - } - } - if ((missingFail && testOOMTrace) || (testOOMTrace > 1)) { - fprintf(stderr, "%s", "!"); -# ifdef TEST_OOM_TRACE - virTestShowTrace(); -# endif - ret = -1; - } else { - fprintf(stderr, "%s", "."); - } - } - testOOMActive = false; - if (ret == 0) - fprintf(stderr, " OK\n"); - else - fprintf(stderr, " FAILED\n"); - virAllocTestInit(); - } -#endif /* TEST_OOM */ - unsetenv("VIR_TEST_MOCK_TESTNAME"); return ret; } @@ -876,8 +747,7 @@ virtTestLogOutput(virLogSourcePtr source ATTRIBUTE_UNUSED, { struct virtTestLogData *log = data; virCheckFlags(VIR_LOG_STACK_TRACE,); - if (!testOOMActive) - virBufferAsprintf(&log->buf, "%s: %s", timestamp, str); + virBufferAsprintf(&log->buf, "%s: %s", timestamp, str); } static void @@ -971,9 +841,6 @@ int virTestMain(int argc, va_list ap; int ret; char *testRange = NULL; -#ifdef TEST_OOM - char *oomstr; -#endif size_t noutputs = 0; virLogOutputPtr output = NULL; virLogOutputPtr *outputs = NULL; @@ -1034,60 +901,6 @@ int virTestMain(int argc, } } -#ifdef TEST_OOM - if ((oomstr = getenv("VIR_TEST_OOM")) != NULL) { - char *next; - if (testDebug == -1) - testDebug = 1; - testOOM = 1; - if (oomstr[0] != '\0' && - oomstr[1] == ':') { - if (virStrToLong_ui(oomstr + 2, &next, 10, &testOOMStart) < 0) { - fprintf(stderr, "Cannot parse range %s\n", oomstr); - return EXIT_FAILURE; - } - if (*next == '\0') { - testOOMEnd = testOOMStart; - } else { - if (*next != '-') { - fprintf(stderr, "Cannot parse range %s\n", oomstr); - return EXIT_FAILURE; - } - if (virStrToLong_ui(next+1, NULL, 10, &testOOMEnd) < 0) { - fprintf(stderr, "Cannot parse range %s\n", oomstr); - return EXIT_FAILURE; - } - } - } else { - testOOMStart = -1; - testOOMEnd = -1; - } - } - -# ifdef TEST_OOM_TRACE - if ((oomstr = getenv("VIR_TEST_OOM_TRACE")) != NULL) { - if (virStrToLong_ui(oomstr, NULL, 10, &testOOMTrace) < 0) { - fprintf(stderr, "Cannot parse oom trace %s\n", oomstr); - return EXIT_FAILURE; - } - } -# else - if (getenv("VIR_TEST_OOM_TRACE")) { - fprintf(stderr, "%s", "OOM test tracing not enabled in this build\n"); - return EXIT_FAILURE; - } -# endif -#else /* TEST_OOM */ - if (getenv("VIR_TEST_OOM")) { - fprintf(stderr, "%s", "OOM testing not enabled in this build\n"); - return EXIT_FAILURE; - } - if (getenv("VIR_TEST_OOM_TRACE")) { - fprintf(stderr, "%s", "OOM test tracing not enabled in this build\n"); - return EXIT_FAILURE; - } -#endif /* TEST_OOM */ - /* Find perl early because some tests override PATH */ perl = virFindFileInPath("perl"); diff --git a/tests/testutils.h b/tests/testutils.h index f273ca4240..85ba9fbc0b 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -43,8 +43,6 @@ extern char *progname; # error Fix Makefile.am #endif -bool virTestOOMActive(void); - int virTestRun(const char *title, int (*body)(const void *data), const void *data); diff --git a/tests/virfirewalltest.c b/tests/virfirewalltest.c index 78685a3bf4..7e4e80e09b 100644 --- a/tests/virfirewalltest.c +++ b/tests/virfirewalltest.c @@ -611,9 +611,6 @@ testFirewallNoRollback(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virTestOOMActive()) - goto cleanup; - if (virBufferError(&cmdbuf)) goto cleanup; @@ -701,9 +698,6 @@ testFirewallSingleRollback(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virTestOOMActive()) - goto cleanup; - if (virBufferError(&cmdbuf)) goto cleanup; @@ -794,9 +788,6 @@ testFirewallManyRollback(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virTestOOMActive()) - goto cleanup; - if (virBufferError(&cmdbuf)) goto cleanup; @@ -917,9 +908,6 @@ testFirewallChainedRollback(const void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - if (virTestOOMActive()) - goto cleanup; - if (virBufferError(&cmdbuf)) goto cleanup; -- 2.21.0
On 9/12/19 1:31 PM, Daniel P. Berrangé wrote:
The OOM handling requires special build time options which we never enable in our CI. Even once enabled the tests are incredibly slow and typically require manual inspection of the results to weed out false positives.
Since there was previous agreement to switch to abort on OOM in libvirt code, there's no point continuing to keep the unused OOM testing code.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- configure.ac | 17 --- docs/docs.html.in | 3 - docs/internals/oomtesting.html.in | 213 ------------------------------ src/libvirt_private.syms | 4 - src/util/viralloc.c | 111 ---------------- src/util/viralloc.h | 5 - tests/Makefile.am | 1 - tests/oomtrace.pl | 41 ------ tests/qemuxml2argvtest.c | 12 +- tests/testutils.c | 189 +------------------------- tests/testutils.h | 2 - tests/virfirewalltest.c | 12 -- 12 files changed, 6 insertions(+), 604 deletions(-) delete mode 100644 docs/internals/oomtesting.html.in delete mode 100755 tests/oomtrace.pl
diff --git a/configure.ac b/configure.ac index bf9e7681bc..8cb7de9c19 100644 --- a/configure.ac +++ b/configure.ac @@ -764,22 +764,6 @@ if test "$enable_test_coverage" = yes; then WARN_CFLAGS=$save_WARN_CFLAGS fi
-LIBVIRT_ARG_ENABLE([TEST_OOM], [memory allocation failure checking], [no]) -case "$enable_test_oom" in - yes|no) ;; - *) AC_MSG_ERROR([bad value ${enable_test_oom} for test-oom option]) ;; -esac - -if test "$enable_test_oom" = yes; then - have_trace=yes - AC_CHECK_HEADER([execinfo.h],[],[have_trace=no]) - AC_CHECK_FUNC([backtrace],[],[have_trace=no]) - if test "$have_trace" = "yes"; then - AC_DEFINE([TEST_OOM_TRACE], 1, [Whether backtrace() is available]) - fi - AC_DEFINE([TEST_OOM], 1, [Whether malloc OOM checking is enabled]) -fi - LIBVIRT_ARG_ENABLE([TEST_LOCKING], [thread locking tests using CIL], [no]) case "$enable_test_locking" in yes|no) ;; @@ -1048,7 +1032,6 @@ AC_MSG_NOTICE([]) AC_MSG_NOTICE([Test suite]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([ Coverage: $enable_test_coverage]) -AC_MSG_NOTICE([ Alloc OOM: $enable_test_oom])
I've just created a merge conflict here, sorry. But it's trivial to resolve. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
The functions are left returning an "int" to avoid an immediate big-bang cleanup. They'll simply never return anything other than 0, except for virInsertN which can still return an error if the requested insertion index is out of range. Interestingly in that case, the _QUIET function would none the less report an error. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.c | 203 +++++++++++--------------------------------- src/util/viralloc.h | 133 ++++++++++------------------- 2 files changed, 93 insertions(+), 243 deletions(-) diff --git a/src/util/viralloc.c b/src/util/viralloc.c index 5a0adcc706..10a8d0fb73 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -35,33 +35,20 @@ VIR_LOG_INIT("util.alloc"); * virAlloc: * @ptrptr: pointer to pointer for address of allocated memory * @size: number of bytes to allocate - * @report: whether to report OOM error, if there is one - * @domcode: error domain code - * @filename: caller's filename - * @funcname: caller's funcname - * @linenr: caller's line number * * Allocate 'size' bytes of memory. Return the address of the * allocated memory in 'ptrptr'. The newly allocated memory is - * filled with zeros. If @report is true, OOM errors are - * reported automatically. + * filled with zeros. * - * Returns -1 on failure to allocate, zero on success + * Returns zero on success, aborts on OOM */ int virAlloc(void *ptrptr, - size_t size, - bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr) + size_t size) { *(void **)ptrptr = calloc(1, size); - if (*(void **)ptrptr == NULL) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - return -1; - } + if (*(void **)ptrptr == NULL) + abort(); + return 0; } @@ -70,35 +57,22 @@ int virAlloc(void *ptrptr, * @ptrptr: pointer to pointer for address of allocated memory * @size: number of bytes to allocate * @count: number of elements to allocate - * @report: whether to report OOM error, if there is one - * @domcode: error domain code - * @filename: caller's filename - * @funcname: caller's funcname - * @linenr: caller's line number * * Allocate an array of memory 'count' elements long, * each with 'size' bytes. Return the address of the * allocated memory in 'ptrptr'. The newly allocated - * memory is filled with zeros. If @report is true, - * OOM errors are reported automatically. + * memory is filled with zeros. * - * Returns -1 on failure to allocate, zero on success + * Returns zero on success, aborts on OOM */ int virAllocN(void *ptrptr, size_t size, - size_t count, - bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr) + size_t count) { *(void**)ptrptr = calloc(count, size); - if (*(void**)ptrptr == NULL) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - return -1; - } + if (*(void**)ptrptr == NULL) + abort(); + return 0; } @@ -107,44 +81,28 @@ int virAllocN(void *ptrptr, * @ptrptr: pointer to pointer for address of allocated memory * @size: number of bytes to allocate * @count: number of elements in array - * @report: whether to report OOM error, if there is one - * @domcode: error domain code - * @filename: caller's filename - * @funcname: caller's funcname - * @linenr: caller's line number * * Resize the block of memory in 'ptrptr' to be an array of * 'count' elements, each 'size' bytes in length. Update 'ptrptr' * with the address of the newly allocated memory. On failure, * 'ptrptr' is not changed and still points to the original memory * block. Any newly allocated memory in 'ptrptr' is uninitialized. - * If @report is true, OOM errors are reported automatically. * - * Returns -1 on failure to allocate, zero on success + * Returns zero on success, aborts on OOM */ int virReallocN(void *ptrptr, size_t size, - size_t count, - bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr) + size_t count) { void *tmp; - if (xalloc_oversized(count, size)) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - errno = ENOMEM; - return -1; - } + if (xalloc_oversized(count, size)) + abort(); + tmp = realloc(*(void**)ptrptr, size * count); - if (!tmp && ((size * count) != 0)) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - return -1; - } + if (!tmp && ((size * count) != 0)) + abort(); + *(void**)ptrptr = tmp; return 0; } @@ -155,46 +113,28 @@ int virReallocN(void *ptrptr, * @size: number of bytes per element * @countptr: pointer to number of elements in array * @add: number of elements to add - * @report: whether to report OOM error, if there is one - * @domcode: error domain code - * @filename: caller's filename - * @funcname: caller's funcname - * @linenr: caller's line number * * Resize the block of memory in 'ptrptr' to be an array of * '*countptr' + 'add' elements, each 'size' bytes in length. * Update 'ptrptr' and 'countptr' with the details of the newly * allocated memory. On failure, 'ptrptr' and 'countptr' are not * changed. Any newly allocated memory in 'ptrptr' is zero-filled. - * If @report is true, OOM errors are reported automatically. * - * Returns -1 on failure to allocate, zero on success + * Returns zero on success, aborts on OOM */ int virExpandN(void *ptrptr, size_t size, size_t *countptr, - size_t add, - bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr) + size_t add) { - int ret; + if (*countptr + add < *countptr) + abort(); - if (*countptr + add < *countptr) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - errno = ENOMEM; - return -1; - } - ret = virReallocN(ptrptr, size, *countptr + add, report, - domcode, filename, funcname, linenr); - if (ret == 0) { - memset(*(char **)ptrptr + (size * *countptr), 0, size * add); - *countptr += add; - } - return ret; + if (virReallocN(ptrptr, size, *countptr + add) < 0) + abort(); + memset(*(char **)ptrptr + (size * *countptr), 0, size * add); + *countptr += add; + return 0; } /** @@ -204,50 +144,34 @@ int virExpandN(void *ptrptr, * @allocptr: pointer to number of elements allocated in array * @count: number of elements currently used in array * @add: minimum number of additional elements to support in array - * @report: whether to report OOM error, if there is one - * @domcode: error domain code - * @filename: caller's filename - * @funcname: caller's funcname - * @linenr: caller's line number * * If 'count' + 'add' is larger than '*allocptr', then resize the * block of memory in 'ptrptr' to be an array of at least 'count' + * 'add' elements, each 'size' bytes in length. Update 'ptrptr' and * 'allocptr' with the details of the newly allocated memory. On * failure, 'ptrptr' and 'allocptr' are not changed. Any newly - * allocated memory in 'ptrptr' is zero-filled. If @report is true, - * OOM errors are reported automatically. - * + * allocated memory in 'ptrptr' is zero-filled. * - * Returns -1 on failure to allocate, zero on success + * Returns zero on success, aborts on OOM */ int virResizeN(void *ptrptr, size_t size, size_t *allocptr, size_t count, - size_t add, - bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr) + size_t add) { size_t delta; - if (count + add < count) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - errno = ENOMEM; - return -1; - } + if (count + add < count) + abort(); + if (count + add <= *allocptr) return 0; delta = count + add - *allocptr; if (delta < *allocptr / 2) delta = *allocptr / 2; - return virExpandN(ptrptr, size, allocptr, delta, report, - domcode, filename, funcname, linenr); + return virExpandN(ptrptr, size, allocptr, delta); } /** @@ -266,8 +190,8 @@ int virResizeN(void *ptrptr, void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) { if (toremove < *countptr) { - ignore_value(virReallocN(ptrptr, size, *countptr -= toremove, - false, 0, NULL, NULL, 0)); + if (virReallocN(ptrptr, size, *countptr -= toremove) < 0) + abort(); } else { virFree(ptrptr); *countptr = 0; @@ -290,11 +214,6 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) * @inPlace: false if we should expand the allocated memory before * moving, true if we should assume someone else *has * already* done that. - * @report: whether to report OOM error, if there is one - * @domcode: error domain code - * @filename: caller's filename - * @funcname: caller's funcname - * @linenr: caller's line number * * Re-allocate an array of *countptr elements, each sizeof(*ptrptr) bytes * long, to be *countptr+add elements long, then appropriately move @@ -303,8 +222,7 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) * allocated memory in *ptrptr and the new size in *countptr. If * newelems is NULL, the new elements at ptrptr[at] are instead filled * with zero. at must be between [0,*countptr], except that -1 is - * treated the same as *countptr for convenience. If @report is true, - * OOM errors are reported automatically. + * treated the same as *countptr for convenience. * * Returns -1 on failure, 0 on success */ @@ -312,12 +230,7 @@ int virInsertElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, size_t add, void *newelems, - bool clearOriginal, bool inPlace, - bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr) + bool clearOriginal, bool inPlace) { if (at == -1) { at = *countptr; @@ -330,9 +243,9 @@ virInsertElementsN(void *ptrptr, size_t size, size_t at, if (inPlace) { *countptr += add; - } else if (virExpandN(ptrptr, size, countptr, add, report, - domcode, filename, funcname, linenr) < 0) { - return -1; + } else { + if (virExpandN(ptrptr, size, countptr, add) < 0) + abort(); } /* memory was successfully re-allocated. Move up all elements from @@ -407,11 +320,6 @@ virDeleteElementsN(void *ptrptr, size_t size, size_t at, * @struct_size: size of initial struct * @element_size: size of array elements * @count: number of array elements to allocate - * @report: whether to report OOM error, if there is one - * @domcode: error domain code - * @filename: caller's filename - * @funcname: caller's funcname - * @linenr: caller's line number * * Allocate struct_size bytes plus an array of 'count' elements, each * of size element_size. This sort of allocation is useful for @@ -420,37 +328,24 @@ virDeleteElementsN(void *ptrptr, size_t size, size_t at, * The caller of this type of API is expected to know the length of * the array that will be returned and allocate a suitable buffer to * contain the returned data. C99 refers to these variable length - * objects as structs containing flexible array members. If @report - * is true, OOM errors are reported automatically. + * objects as structs containing flexible array members. * * Returns -1 on failure, 0 on success */ int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, - size_t count, - bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr) + size_t count) { size_t alloc_size = 0; - if (VIR_ALLOC_VAR_OVERSIZED(struct_size, count, element_size)) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - errno = ENOMEM; - return -1; - } + if (VIR_ALLOC_VAR_OVERSIZED(struct_size, count, element_size)) + abort(); alloc_size = struct_size + (element_size * count); *(void **)ptrptr = calloc(1, alloc_size); - if (*(void **)ptrptr == NULL) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - return -1; - } + if (*(void **)ptrptr == NULL) + abort(); return 0; } diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 3e169e272c..78f72a6c6a 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -44,35 +44,26 @@ /* Don't call these directly - use the macros below */ -int virAlloc(void *ptrptr, size_t size, bool report, int domcode, - const char *filename, const char *funcname, size_t linenr) +int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); -int virAllocN(void *ptrptr, size_t size, size_t count, bool report, int domcode, - const char *filename, const char *funcname, size_t linenr) +int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); -int virReallocN(void *ptrptr, size_t size, size_t count, bool report, int domcode, - const char *filename, const char *funcname, size_t linenr) +int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); -int virExpandN(void *ptrptr, size_t size, size_t *count, size_t add, bool report, - int domcode, const char *filename, const char *funcname, size_t linenr) +int virExpandN(void *ptrptr, size_t size, size_t *count, size_t add) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); -int virResizeN(void *ptrptr, size_t size, size_t *alloc, size_t count, size_t desired, - bool report, int domcode, const char *filename, - const char *funcname, size_t linenr) +int virResizeN(void *ptrptr, size_t size, size_t *alloc, size_t count, size_t desired) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); void virShrinkN(void *ptrptr, size_t size, size_t *count, size_t toremove) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); int virInsertElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, size_t add, void *newelem, - bool clearOriginal, bool inPlace, bool report, int domcode, - const char *filename, const char *funcname, size_t linenr) + bool clearOriginal, bool inPlace) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); int virDeleteElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, size_t toremove, bool inPlace) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); -int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count, - bool report, int domcode, const char *filename, - const char *funcname, size_t linenr) +int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); @@ -91,10 +82,9 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure (with OOM error reported), 0 on success + * Returns 0 on success, aborts on OOM */ -#define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)), true, VIR_FROM_THIS, \ - __FILE__, __FUNCTION__, __LINE__) +#define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr))) /** * VIR_ALLOC_QUIET: @@ -106,9 +96,9 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure, 0 on success + * Returns 0 on success, aborts on OOM */ -#define VIR_ALLOC_QUIET(ptr) virAlloc(&(ptr), sizeof(*(ptr)), false, 0, NULL, NULL, 0) +#define VIR_ALLOC_QUIET(ptr) VIR_ALLOC(ptr) /** * VIR_ALLOC_N: @@ -121,10 +111,9 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure (with OOM error reported), 0 on success + * Returns 0 on success, aborts on OOM */ -#define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count), true, \ - VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) +#define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count)) /** * VIR_ALLOC_N_QUIET: @@ -137,10 +126,9 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure, 0 on success + * Returns 0 on success, aborts on OOM */ -#define VIR_ALLOC_N_QUIET(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count), \ - false, 0, NULL, NULL, 0) +#define VIR_ALLOC_N_QUIET(ptr, count) VIR_ALLOC_N(ptr, count) /** * VIR_REALLOC_N: @@ -153,11 +141,9 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure (with OOM error reported), 0 on success + * Returns 0 on success, aborts on OOM */ -#define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count), \ - true, VIR_FROM_THIS, __FILE__, \ - __FUNCTION__, __LINE__) +#define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count)) /** * VIR_REALLOC_N_QUIET: @@ -170,10 +156,9 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure, 0 on success + * Returns 0 on success, aborts on OOM */ -#define VIR_REALLOC_N_QUIET(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count), \ - false, 0, NULL, NULL, 0) +#define VIR_REALLOC_N_QUIET(ptr, count) VIR_REALLOC_N(ptr, count) /** * VIR_EXPAND_N: @@ -188,11 +173,9 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure (with OOM error reported), 0 on success + * Returns 0 on success, aborts on OOM */ -#define VIR_EXPAND_N(ptr, count, add) \ - virExpandN(&(ptr), sizeof(*(ptr)), &(count), add, true, VIR_FROM_THIS, \ - __FILE__, __FUNCTION__, __LINE__) +#define VIR_EXPAND_N(ptr, count, add) virExpandN(&(ptr), sizeof(*(ptr)), &(count), add) /** * VIR_EXPAND_N_QUIET: @@ -207,10 +190,9 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure, 0 on success + * Returns 0 on success, aborts on OOM */ -#define VIR_EXPAND_N_QUIET(ptr, count, add) \ - virExpandN(&(ptr), sizeof(*(ptr)), &(count), add, false, 0, NULL, NULL, 0) +#define VIR_EXPAND_N_QUIET(ptr, count, add) VIR_EXPAND_N(ptr, count, add) /** * VIR_RESIZE_N: @@ -232,11 +214,10 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure (with OOM error reported), 0 on success + * Returns 0 on success, aborts on OOM */ #define VIR_RESIZE_N(ptr, alloc, count, add) \ - virResizeN(&(ptr), sizeof(*(ptr)), &(alloc), count, add, true, \ - VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) + virResizeN(&(ptr), sizeof(*(ptr)), &(alloc), count, add) /** * VIR_RESIZE_N_QUIET: @@ -258,11 +239,9 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure, 0 on success + * Returns 0 on success, aborts on OOM */ -#define VIR_RESIZE_N_QUIET(ptr, alloc, count, add) \ - virResizeN(&(ptr), sizeof(*(ptr)), &(alloc), count, add, \ - false, 0, NULL, NULL, 0) +#define VIR_RESIZE_N_QUIET(ptr, alloc, count, add) VIR_RESIZE_N(ptr, alloc, count, add) /** * VIR_SHRINK_N: @@ -359,38 +338,26 @@ void virDisposeString(char **strptr) */ #define VIR_INSERT_ELEMENT(ptr, at, count, newelem) \ virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false, \ - true, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false) #define VIR_INSERT_ELEMENT_COPY(ptr, at, count, newelem) \ virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false, \ - true, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false) #define VIR_INSERT_ELEMENT_INPLACE(ptr, at, count, newelem) \ virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, true, \ - true, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, true) #define VIR_INSERT_ELEMENT_COPY_INPLACE(ptr, at, count, newelem) \ virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true, \ - true, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true) /* Quiet version of macros above */ #define VIR_INSERT_ELEMENT_QUIET(ptr, at, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false, \ - false, 0, NULL, NULL, 0) + VIR_INSERT_ELEMENT(ptr, at, count, newelem) #define VIR_INSERT_ELEMENT_COPY_QUIET(ptr, at, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false, \ - false, 0, NULL, NULL, 0) + VIR_INSERT_ELEMENT_COPY(ptr, at, count, newelem) #define VIR_INSERT_ELEMENT_INPLACE_QUIET(ptr, at, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, true, \ - false, 0, NULL, NULL, 0) + VIR_INSERT_ELEMENT_INPLACE(ptr, at, count, newelem) #define VIR_INSERT_ELEMENT_COPY_INPLACE_QUIET(ptr, at, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true, \ - false, 0, NULL, NULL, 0) + VIR_INSERT_ELEMENT_COPY_INPLACE(ptr, at, count, newelem) /** * VIR_APPEND_ELEMENT: @@ -429,34 +396,24 @@ void virDisposeString(char **strptr) */ #define VIR_APPEND_ELEMENT(ptr, count, newelem) \ virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false, \ - true, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false) #define VIR_APPEND_ELEMENT_COPY(ptr, count, newelem) \ virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false, \ - true, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false) #define VIR_APPEND_ELEMENT_INPLACE(ptr, count, newelem) \ ignore_value(virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count), \ VIR_TYPEMATCH(ptr, &(newelem)), \ - &(newelem), true, true, false, \ - VIR_FROM_THIS, __FILE__, \ - __FUNCTION__, __LINE__)) + &(newelem), true, true)) #define VIR_APPEND_ELEMENT_COPY_INPLACE(ptr, count, newelem) \ ignore_value(virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count), \ VIR_TYPEMATCH(ptr, &(newelem)), \ - &(newelem), false, true, false, \ - VIR_FROM_THIS, __FILE__, \ - __FUNCTION__, __LINE__)) + &(newelem), false, true)) /* Quiet version of macros above */ #define VIR_APPEND_ELEMENT_QUIET(ptr, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false, \ - false, 0, NULL, NULL, 0) + VIR_APPEND_ELEMENT(ptr, count, newelem) #define VIR_APPEND_ELEMENT_COPY_QUIET(ptr, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count), \ - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false, \ - false, 0, NULL, NULL, 0) + VIR_APPEND_ELEMENT_COPY(ptr, count, newelem) /** * VIR_DELETE_ELEMENT: @@ -510,11 +467,10 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure (with OOM error reported), 0 on success + * Returns 0 on success, aborts on OOM */ #define VIR_ALLOC_VAR(ptr, type, count) \ - virAllocVar(&(ptr), sizeof(*(ptr)), sizeof(type), (count), true, \ - VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__) + virAllocVar(&(ptr), sizeof(*(ptr)), sizeof(type), (count)) /** * VIR_ALLOC_VAR_QUIET: @@ -535,8 +491,7 @@ void virDisposeString(char **strptr) * * Returns -1 on failure, 0 on success */ -#define VIR_ALLOC_VAR_QUIET(ptr, type, count) \ - virAllocVar(&(ptr), sizeof(*(ptr)), sizeof(type), (count), false, 0, NULL, NULL, 0) +#define VIR_ALLOC_VAR_QUIET(ptr, type, count) VIR_ALLOC_VAR(ptr, type, count) /** * VIR_FREE: -- 2.21.0
On 9/12/19 1:31 PM, Daniel P. Berrangé wrote:
The functions are left returning an "int" to avoid an immediate big-bang cleanup. They'll simply never return anything other than 0, except for virInsertN which can still return an error if the requested insertion index is out of range. Interestingly in that case, the _QUIET function would none the less report an error.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.c | 203 +++++++++++--------------------------------- src/util/viralloc.h | 133 ++++++++++------------------- 2 files changed, 93 insertions(+), 243 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
Only a few of the _QUIET allocation macros are used. Since we're no longer reporting OOM as errors, we want to eliminate all the _QUIET variants. This starts with the easy, unused, cases. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.h | 74 --------------------------------------------- 1 file changed, 74 deletions(-) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 78f72a6c6a..3e72e40bc9 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -177,23 +177,6 @@ void virDisposeString(char **strptr) */ #define VIR_EXPAND_N(ptr, count, add) virExpandN(&(ptr), sizeof(*(ptr)), &(count), add) -/** - * VIR_EXPAND_N_QUIET: - * @ptr: pointer to hold address of allocated memory - * @count: variable tracking number of elements currently allocated - * @add: number of elements to add - * - * Re-allocate an array of 'count' elements, each sizeof(*ptr) - * bytes long, to be 'count' + 'add' elements long, then store the - * address of allocated memory in 'ptr' and the new size in 'count'. - * The new elements are filled with zero. - * - * This macro is safe to use on arguments with side effects. - * - * Returns 0 on success, aborts on OOM - */ -#define VIR_EXPAND_N_QUIET(ptr, count, add) VIR_EXPAND_N(ptr, count, add) - /** * VIR_RESIZE_N: * @ptr: pointer to hold address of allocated memory @@ -219,30 +202,6 @@ void virDisposeString(char **strptr) #define VIR_RESIZE_N(ptr, alloc, count, add) \ virResizeN(&(ptr), sizeof(*(ptr)), &(alloc), count, add) -/** - * VIR_RESIZE_N_QUIET: - * @ptr: pointer to hold address of allocated memory - * @alloc: variable tracking number of elements currently allocated - * @count: number of elements currently in use - * @add: minimum number of elements to additionally support - * - * Blindly using VIR_EXPAND_N(array, alloc, 1) in a loop scales - * quadratically, because every iteration must copy contents from - * all prior iterations. But amortized linear scaling can be achieved - * by tracking allocation size separately from the number of used - * elements, and growing geometrically only as needed. - * - * If 'count' + 'add' is larger than 'alloc', then geometrically reallocate - * the array of 'alloc' elements, each sizeof(*ptr) bytes long, and store - * the address of allocated memory in 'ptr' and the new size in 'alloc'. - * The new elements are filled with zero. - * - * This macro is safe to use on arguments with side effects. - * - * Returns 0 on success, aborts on OOM - */ -#define VIR_RESIZE_N_QUIET(ptr, alloc, count, add) VIR_RESIZE_N(ptr, alloc, count, add) - /** * VIR_SHRINK_N: * @ptr: pointer to hold address of allocated memory @@ -349,16 +308,6 @@ void virDisposeString(char **strptr) virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true) -/* Quiet version of macros above */ -#define VIR_INSERT_ELEMENT_QUIET(ptr, at, count, newelem) \ - VIR_INSERT_ELEMENT(ptr, at, count, newelem) -#define VIR_INSERT_ELEMENT_COPY_QUIET(ptr, at, count, newelem) \ - VIR_INSERT_ELEMENT_COPY(ptr, at, count, newelem) -#define VIR_INSERT_ELEMENT_INPLACE_QUIET(ptr, at, count, newelem) \ - VIR_INSERT_ELEMENT_INPLACE(ptr, at, count, newelem) -#define VIR_INSERT_ELEMENT_COPY_INPLACE_QUIET(ptr, at, count, newelem) \ - VIR_INSERT_ELEMENT_COPY_INPLACE(ptr, at, count, newelem) - /** * VIR_APPEND_ELEMENT: * @ptr: pointer to array of objects (*not* ptr to ptr) @@ -412,8 +361,6 @@ void virDisposeString(char **strptr) /* Quiet version of macros above */ #define VIR_APPEND_ELEMENT_QUIET(ptr, count, newelem) \ VIR_APPEND_ELEMENT(ptr, count, newelem) -#define VIR_APPEND_ELEMENT_COPY_QUIET(ptr, count, newelem) \ - VIR_APPEND_ELEMENT_COPY(ptr, count, newelem) /** * VIR_DELETE_ELEMENT: @@ -472,27 +419,6 @@ void virDisposeString(char **strptr) #define VIR_ALLOC_VAR(ptr, type, count) \ virAllocVar(&(ptr), sizeof(*(ptr)), sizeof(type), (count)) -/** - * VIR_ALLOC_VAR_QUIET: - * @ptr: pointer to hold address of allocated memory - * @type: element type of trailing array - * @count: number of array elements to allocate - * - * Allocate sizeof(*ptr) bytes plus an array of 'count' elements, each - * sizeof('type'). This sort of allocation is useful for receiving - * the data of certain ioctls and other APIs which return a struct in - * which the last element is an array of undefined length. The caller - * of this type of API is expected to know the length of the array - * that will be returned and allocate a suitable buffer to contain the - * returned data. C99 refers to these variable length objects as - * structs containing flexible array members. - * - * This macro is safe to use on arguments with side effects. - * - * Returns -1 on failure, 0 on success - */ -#define VIR_ALLOC_VAR_QUIET(ptr, type, count) VIR_ALLOC_VAR(ptr, type, count) - /** * VIR_FREE: * @ptr: pointer holding address to be freed -- 2.21.0
On 9/12/19 1:31 PM, Daniel P. Berrangé wrote:
Only a few of the _QUIET allocation macros are used. Since we're no longer reporting OOM as errors, we want to eliminate all the _QUIET variants. This starts with the easy, unused, cases.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.h | 74 --------------------------------------------- 1 file changed, 74 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
The functions are left returning an "int" to avoid an immediate big-bang cleanup. They'll simply never return anything other than 0. Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstring.c | 93 +++++++++++--------------------------------- src/util/virstring.h | 73 ++++++++++++---------------------- 2 files changed, 47 insertions(+), 119 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index bd269e98fe..2064944b0b 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -726,40 +726,27 @@ virDoubleToStr(char **strp, double number) int -virVasprintfInternal(bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr, - char **strp, +virVasprintfInternal(char **strp, const char *fmt, va_list list) { int ret; - if ((ret = vasprintf(strp, fmt, list)) == -1) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - *strp = NULL; - } + if ((ret = vasprintf(strp, fmt, list)) == -1) + abort(); + return ret; } int -virAsprintfInternal(bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr, - char **strp, +virAsprintfInternal(char **strp, const char *fmt, ...) { va_list ap; int ret; va_start(ap, fmt); - ret = virVasprintfInternal(report, domcode, filename, - funcname, linenr, strp, fmt, ap); + ret = virVasprintfInternal(strp, fmt, ap); va_end(ap); return ret; } @@ -937,37 +924,20 @@ virStringIsEmpty(const char *str) * virStrdup: * @dest: where to store duplicated string * @src: the source string to duplicate - * @report: whether to report OOM error, if there is one - * @domcode: error domain code - * @filename: caller's filename - * @funcname: caller's funcname - * @linenr: caller's line number - * - * Wrapper over strdup, which reports OOM error if told so, - * in which case callers wants to pass @domcode, @filename, - * @funcname and @linenr which should represent location in - * caller's body where virStrdup is called from. Consider - * using VIR_STRDUP which sets these automatically. - * - * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise. + * + * Wrapper over strdup, which aborts on OOM error. + * + * Returns: 0 for NULL src, 1 on successful copy, aborts on OOM */ int virStrdup(char **dest, - const char *src, - bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr) + const char *src) { *dest = NULL; if (!src) return 0; - if (!(*dest = strdup(src))) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - return -1; - } + if (!(*dest = strdup(src))) + abort(); return 1; } @@ -977,45 +947,28 @@ virStrdup(char **dest, * @dest: where to store duplicated string * @src: the source string to duplicate * @n: how many bytes to copy - * @report: whether to report OOM error, if there is one - * @domcode: error domain code - * @filename: caller's filename - * @funcname: caller's funcname - * @linenr: caller's line number - * - * Wrapper over strndup, which reports OOM error if told so, - * in which case callers wants to pass @domcode, @filename, - * @funcname and @linenr which should represent location in - * caller's body where virStrndup is called from. Consider - * using VIR_STRNDUP which sets these automatically. + * + * Wrapper over strndup, which aborts on OOM error. * * In case @n is smaller than zero, the whole @src string is * copied. * - * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise. + * Returns: 0 for NULL src, 1 on successful copy, aborts on OOM */ int virStrndup(char **dest, const char *src, - ssize_t n, - bool report, - int domcode, - const char *filename, - const char *funcname, - size_t linenr) + ssize_t n) { *dest = NULL; if (!src) return 0; if (n < 0) n = strlen(src); - if (!(*dest = strndup(src, n))) { - if (report) - virReportOOMErrorFull(domcode, filename, funcname, linenr); - return -1; - } + if (!(*dest = strndup(src, n))) + abort(); - return 1; + return 1; } @@ -1483,10 +1436,8 @@ virStringEncodeBase64(const uint8_t *buf, size_t buflen) char *ret; base64_encode_alloc((const char *) buf, buflen, &ret); - if (!ret) { - virReportOOMError(); - return NULL; - } + if (!ret) + abort(); return ret; } diff --git a/src/util/virstring.h b/src/util/virstring.h index 5e64ad1bb9..f537f3472e 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -128,22 +128,16 @@ int virStrcpy(char *dest, const char *src, size_t destbytes) #define virStrcpyStatic(dest, src) virStrcpy((dest), (src), sizeof(dest)) /* Don't call these directly - use the macros below */ -int virStrdup(char **dest, const char *src, bool report, int domcode, - const char *filename, const char *funcname, size_t linenr) +int virStrdup(char **dest, const char *src) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); -int virStrndup(char **dest, const char *src, ssize_t n, bool report, int domcode, - const char *filename, const char *funcname, size_t linenr) +int virStrndup(char **dest, const char *src, ssize_t n) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); -int virAsprintfInternal(bool report, int domcode, const char *filename, - const char *funcname, size_t linenr, char **strp, - const char *fmt, ...) - ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7) ATTRIBUTE_FMT_PRINTF(7, 8) +int virAsprintfInternal(char **strp, const char *fmt, ...) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3) ATTRIBUTE_RETURN_CHECK; -int virVasprintfInternal(bool report, int domcode, const char *filename, - const char *funcname, size_t linenr, char **strp, - const char *fmt, va_list list) - ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7) ATTRIBUTE_FMT_PRINTF(7, 0) +int virVasprintfInternal(char **strp, const char *fmt, va_list list) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 0) ATTRIBUTE_RETURN_CHECK; /** @@ -155,11 +149,9 @@ int virVasprintfInternal(bool report, int domcode, const char *filename, * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure (with OOM error reported), 0 if @src was NULL, - * 1 if @src was copied + * Returns 0 if @src was NULL, 1 if @src was copied, aborts on OOM */ -#define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ - __FILE__, __FUNCTION__, __LINE__) +#define VIR_STRDUP(dst, src) virStrdup(&(dst), src) /** * VIR_STRDUP_QUIET: @@ -170,9 +162,9 @@ int virVasprintfInternal(bool report, int domcode, const char *filename, * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure, 0 if @src was NULL, 1 if @src was copied + * Returns 0 if @src was NULL, 1 if @src was copied, aborts on OOM */ -#define VIR_STRDUP_QUIET(dst, src) virStrdup(&(dst), src, false, 0, NULL, NULL, 0) +#define VIR_STRDUP_QUIET(dst, src) VIR_STRDUP(dst, src) /** * VIR_STRNDUP: @@ -187,12 +179,9 @@ int virVasprintfInternal(bool report, int domcode, const char *filename, * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure (with OOM error reported), 0 if @src was NULL, - * 1 if @src was copied + * Returns 0 if @src was NULL, 1 if @src was copied, aborts on OOM */ -#define VIR_STRNDUP(dst, src, n) virStrndup(&(dst), src, n, true, \ - VIR_FROM_THIS, __FILE__, \ - __FUNCTION__, __LINE__) +#define VIR_STRNDUP(dst, src, n) virStrndup(&(dst), src, n) /** * VIR_STRNDUP_QUIET: @@ -208,51 +197,41 @@ int virVasprintfInternal(bool report, int domcode, const char *filename, * * This macro is safe to use on arguments with side effects. * - * Returns -1 on failure, 0 if @src was NULL, 1 if @src was copied + * Returns 0 if @src was NULL, 1 if @src was copied, aborts on OOM */ -#define VIR_STRNDUP_QUIET(dst, src, n) virStrndup(&(dst), src, n, false, \ - 0, NULL, NULL, 0) +#define VIR_STRNDUP_QUIET(dst, src, n) virStrndup(&(dst), src, n) size_t virStringListLength(const char * const *strings); /** * virVasprintf * - * Like glibc's vasprintf but makes sure *strp == NULL on failure, in which - * case the OOM error is reported too. + * Like glibc's vasprintf but aborts on OOM * - * Returns -1 on failure (with OOM error reported), number of bytes printed - * on success. + * Returns number of bytes printed on success, aborts on OOM */ -#define virVasprintf(strp, fmt, list) \ - virVasprintfInternal(true, VIR_FROM_THIS, __FILE__, __FUNCTION__, \ - __LINE__, strp, fmt, list) +#define virVasprintf(strp, fmt, list) virVasprintfInternal(strp, fmt, list) /** * virVasprintfQuiet * - * Like glibc's vasprintf but makes sure *strp == NULL on failure. + * Like glibc's vasprintf but aborts on OOM. * - * Returns -1 on failure, number of bytes printed on success. + * Returns number of bytes printed on success, aborts on OOM */ -#define virVasprintfQuiet(strp, fmt, list) \ - virVasprintfInternal(false, 0, NULL, NULL, 0, strp, fmt, list) +#define virVasprintfQuiet(strp, fmt, list) virVasprintf(strp, fmt, list) /** * virAsprintf: * @strp: variable to hold result (char **) * @fmt: printf format * - * Like glibc's asprintf but makes sure *strp == NULL on failure, in which case - * the OOM error is reported too. + * Like glibc's asprintf but aborts on OOM. * - * Returns -1 on failure (with OOM error reported), number of bytes printed - * on success. + * Returns number of bytes printed on success, aborts on OOM */ -#define virAsprintf(strp, ...) \ - virAsprintfInternal(true, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__, \ - strp, __VA_ARGS__) +#define virAsprintf(strp, ...) virAsprintfInternal(strp, __VA_ARGS__) /** * virAsprintfQuiet: @@ -261,12 +240,10 @@ size_t virStringListLength(const char * const *strings); * * Like glibc's asprintf but makes sure *strp == NULL on failure. * - * Returns -1 on failure, number of bytes printed on success. + * Returns number of bytes printed on success, aborts on OOM */ -#define virAsprintfQuiet(strp, ...) \ - virAsprintfInternal(false, 0, NULL, NULL, 0, \ - strp, __VA_ARGS__) +#define virAsprintfQuiet(strp, ...) virAsprintf(strp, __VA_ARGS__) int virStringSortCompare(const void *a, const void *b); int virStringSortRevCompare(const void *a, const void *b); -- 2.21.0
On 9/12/19 1:31 PM, Daniel P. Berrangé wrote:
The functions are left returning an "int" to avoid an immediate big-bang cleanup. They'll simply never return anything other than 0.
Reviewed-by: Ján Tomko <jtomko@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstring.c | 93 +++++++++++--------------------------------- src/util/virstring.h | 73 ++++++++++++---------------------- 2 files changed, 47 insertions(+), 119 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik