[libvirt] [PATCH 0/7] util: abort when out of memory
See this previous thread: https://www.redhat.com/archives/libvir-list/2019-May/msg00273.html The executive summary is that catching & reporting ENOMEM is not worth the maint cost because: - this code will almost never run on Linux hosts - if it does run it will likely have bad behaviour silently dropping data or crashing the process - apps using libvirt often do so via a non-C language that aborts/exits the app on OOM regardless, or use other C libraries that abort - we can build a system that is more reliable when OOM happens by not catching OOM, instead simply letting apps exit, restart and carry on where they left off The first part of the series does the bare minimum to cull OOM handling. With this done, the main reason why we never adopted glib is now removed. Thus the second part of this series introduces use of glib into libvirt and converts our our allocation APIs to use the glib allocation APIs internally. In future I'd especially like to use glib to replace virObject code, which I knowingly wrote as a somewhat pathetic clone of GObject. Eliminating all our DBus code is also another thing I'd like, since Glib's DBus client code is better IMHO. Note that none of the callers are updated at this point, so they are all still attempting to handle errors from VIR_ALLOC, VIR_STRDUP, etc. If we convert VIR_ALLOC calls to remove return value checks, and then later convert to glib's g_new0 API, we go through two lots of churn which I think is undesirable. Thus I think it is highly desirable to introduce glib straight away and do a single conversion step. It also means we can introduce other data structures from glib to replace ours and avoid wasting time converting those too. Overall the code in this series is all quite simple and a nice cleanup. 50% of the lines culled come from the first patch removing OOM testing, the other 50% come from viralloc.c impl simplification The interesting question is the impact is has on downstreams who have to backport patches to older versions. If we start converting callers to g_new0, etc, then downstream have to either * Change g_new0 back into VIR_ALLOC and likely re-add many goto calls we purged Or * Backport all the patches in this series that drop OOM handling and introduce glib usage If we start adopting other glib features beyond g_new0, then downstreams are pretty much forced into option 2. Given the benefit I think we'll see from this series in our codebase, I'm inclined to say we should prioritize the future, over prioritizing the past (backports), and thus freely adopt use of glib APIs rightaway. IOW, I think we should expect vendors to backport this series as a starting point, before any other patches. I've not tested but I'm hopeful that such a backport of this series is fairly easy. The viralloc.{c,h} file hasn't seen much change in recent times so ought to be a clean cherry-pick. The glib additions might hit some conflicts in makefiles, but not too terrible I hope. Probably worth doing a test to see just how easy backports are over the past 1 year of releases. Daniel P. Berrangé (7): 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 build: probe for glib-2 library in configure build: link to glib library util: use glib allocation functions configure.ac | 19 +- docs/docs.html.in | 3 - docs/internals/oomtesting.html.in | 213 -------------------- libvirt.spec.in | 1 + m4/virt-glib.m4 | 30 +++ mingw-libvirt.spec.in | 2 + src/Makefile.am | 2 + src/internal.h | 1 + src/libvirt_private.syms | 4 - src/lxc/Makefile.inc.am | 2 + src/remote/Makefile.inc.am | 1 + src/util/Makefile.inc.am | 1 + src/util/viralloc.c | 313 ++++-------------------------- src/util/viralloc.h | 204 ++++--------------- src/util/virstring.c | 93 +++------ src/util/virstring.h | 79 +++----- tests/Makefile.am | 4 +- tests/oomtrace.pl | 41 ---- tests/qemuxml2argvtest.c | 12 +- tests/testutils.c | 189 +----------------- tests/testutils.h | 2 - tests/virfirewalltest.c | 12 -- tools/Makefile.am | 1 + 23 files changed, 179 insertions(+), 1050 deletions(-) delete mode 100644 docs/internals/oomtesting.html.in create mode 100644 m4/virt-glib.m4 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. 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 7c76a9c9ec..36e75ac3c0 100644 --- a/configure.ac +++ b/configure.ac @@ -766,22 +766,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) ;; @@ -1052,7 +1036,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 a34d92f5ef..cd9e29cac1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1483,10 +1483,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 f92710db43..a319e386ec 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 9395cc19a2..ac54f088e0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -552,14 +552,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 8b2c51044e..b4009f0a48 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 Thu, Aug 29, 2019 at 07:02:44PM +0100, 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.
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
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.c | 201 +++++++++++--------------------------------- src/util/viralloc.h | 145 +++++++++++--------------------- 2 files changed, 97 insertions(+), 249 deletions(-) diff --git a/src/util/viralloc.c b/src/util/viralloc.c index 5a0adcc706..b74f657733 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,27 @@ 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; + virReallocN(ptrptr, size, *countptr + add); + memset(*(char **)ptrptr + (size * *countptr), 0, size * add); + *countptr += add; + return 0; } /** @@ -204,50 +143,35 @@ 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); + virExpandN(ptrptr, size, allocptr, delta); + return 0; } /** @@ -266,8 +190,7 @@ 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)); + virReallocN(ptrptr, size, *countptr -= toremove); } else { virFree(ptrptr); *countptr = 0; @@ -290,11 +213,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 +221,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 +229,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 +242,8 @@ 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 { + virExpandN(ptrptr, size, countptr, add); } /* memory was successfully re-allocated. Move up all elements from @@ -407,11 +318,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 +326,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..1437ad4b29 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -44,36 +44,27 @@ /* 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) - 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) - 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) - 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) - 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) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); +int virAlloc(void *ptrptr, size_t size) + ATTRIBUTE_NONNULL(1); +int virAllocN(void *ptrptr, size_t size, size_t count) + ATTRIBUTE_NONNULL(1); +int virReallocN(void *ptrptr, size_t size, size_t count) + ATTRIBUTE_NONNULL(1); +int virExpandN(void *ptrptr, size_t size, size_t *count, size_t add) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); +int virResizeN(void *ptrptr, size_t size, size_t *alloc, size_t count, size_t desired) + 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) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); +int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count) + ATTRIBUTE_NONNULL(1); void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); void virDispose(void *ptrptr, size_t count, size_t element_size, size_t *countptr) @@ -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 Thu, Aug 29, 2019 at 07:02:45PM +0100, 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.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.c | 201 +++++++++++--------------------------------- src/util/viralloc.h | 145 +++++++++++--------------------- 2 files changed, 97 insertions(+), 249 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 8/29/19 2:02 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.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.c | 201 +++++++++++--------------------------------- src/util/viralloc.h | 145 +++++++++++--------------------- 2 files changed, 97 insertions(+), 249 deletions(-)
FWIW: I applied the series to my Coverity branch to see what (if anything) gets shaken there. Good news is - not much. There were a couple of things I already reported on that were fixed for 5.7.0 (commit ed7e342b0 and be9d259eb). There's one more false positive in qemuDomainGetNumaParameters that probably would be fixed by AUTOFREE stuff (nodeset gets set to NULL after virTypedParameterAssign). Beyond that the only thing is two CHECKED_RETURN's (e.g. N callers check the return value, but these 2 don't). I consider both false positives; however, it could also be argued that unless any of the functions where either 0 or abort occurs, then make them just void, but that's a sh*load more work... [...]
/** @@ -204,50 +143,35 @@ 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); + virExpandN(ptrptr, size, allocptr, delta); + return 0;
Could just return virExpandN here...
}
[...]
@@ -312,12 +229,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 +242,8 @@ 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 { + virExpandN(ptrptr, size, countptr, add); }
This one's more painful, with using ignore_value() wrapper to just pacify Coverity. [...] I'm not saying anything has to be done, but I figured it could be a useful data point for you - John
On Thu, Sep 05, 2019 at 06:03:57PM -0400, John Ferlan wrote:
On 8/29/19 2:02 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.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.c | 201 +++++++++++--------------------------------- src/util/viralloc.h | 145 +++++++++++--------------------- 2 files changed, 97 insertions(+), 249 deletions(-)
FWIW: I applied the series to my Coverity branch to see what (if anything) gets shaken there. Good news is - not much. There were a couple of things I already reported on that were fixed for 5.7.0 (commit ed7e342b0 and be9d259eb).
There's one more false positive in qemuDomainGetNumaParameters that probably would be fixed by AUTOFREE stuff (nodeset gets set to NULL after virTypedParameterAssign).
Beyond that the only thing is two CHECKED_RETURN's (e.g. N callers check the return value, but these 2 don't). I consider both false positives; however, it could also be argued that unless any of the functions where either 0 or abort occurs, then make them just void, but that's a sh*load more work...
[...]
/** @@ -204,50 +143,35 @@ 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); + virExpandN(ptrptr, size, allocptr, delta); + return 0;
Could just return virExpandN here...
}
[...]
@@ -312,12 +229,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 +242,8 @@ 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 { + virExpandN(ptrptr, size, countptr, add); }
This one's more painful, with using ignore_value() wrapper to just pacify Coverity.
[...]
I'm not saying anything has to be done, but I figured it could be a useful data point for you -
In this patch I removed the ATTRIBUTE_RETURN_CHECK annotation, but I'm going to revert that bit. We don't really want churn from if (VIR_ALLOC(foo) < 0) return -1; to VIR_ALLOC(foo); to foo = g_new0(struct Foo, 1) By keeping ATTRIBUTE_RETURN_CHECK we forcably avoid the intermediate conversion step, which will keep things saner I think. That will avoid creating 100's of new coverity warnings for the intermediate step. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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. 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 1437ad4b29..1d1cc0ba42 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 Thu, Aug 29, 2019 at 07:02:46PM +0100, 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.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.h | 74 --------------------------------------------- 1 file changed, 74 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The functions are left returning an "int" to avoid an immediate big-bang cleanup. They'll simply never return anything other than 0. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstring.c | 93 +++++++++++--------------------------------- src/util/virstring.h | 79 +++++++++++++------------------------ 2 files changed, 49 insertions(+), 123 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..7d1ae1306b 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -128,23 +128,15 @@ 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) - 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) - 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) - 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) - ATTRIBUTE_RETURN_CHECK; +int virStrdup(char **dest, const char *src) + ATTRIBUTE_NONNULL(1); + +int virStrndup(char **dest, const char *src, ssize_t n) + ATTRIBUTE_NONNULL(1); +int virAsprintfInternal(char **strp, const char *fmt, ...) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); +int virVasprintfInternal(char **strp, const char *fmt, va_list list) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 0); /** * VIR_STRDUP: @@ -155,11 +147,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 +160,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 +177,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 +195,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: @@ -264,9 +241,7 @@ size_t virStringListLength(const char * const *strings); * Returns -1 on failure, number of bytes printed on success. */ -#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 Thu, Aug 29, 2019 at 07:02:47PM +0100, 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.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstring.c | 93 +++++++++++--------------------------------- src/util/virstring.h | 79 +++++++++++++------------------------ 2 files changed, 49 insertions(+), 123 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 8/29/19 12:02 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.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virstring.c | 93 +++++++++++--------------------------------- src/util/virstring.h | 79 +++++++++++++------------------------ 2 files changed, 49 insertions(+), 123 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..7d1ae1306b 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -128,23 +128,15 @@ 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) - 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) - 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) - 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) - ATTRIBUTE_RETURN_CHECK; +int virStrdup(char **dest, const char *src) + ATTRIBUTE_NONNULL(1); + +int virStrndup(char **dest, const char *src, ssize_t n) + ATTRIBUTE_NONNULL(1); +int virAsprintfInternal(char **strp, const char *fmt, ...) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); +int virVasprintfInternal(char **strp, const char *fmt, va_list list) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 0);
/** * VIR_STRDUP: @@ -155,11 +147,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 +160,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 +177,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 +195,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: @@ -264,9 +241,7 @@ size_t virStringListLength(const char * const *strings); * Returns -1 on failure, number of bytes printed on success.
You missed changing the comments for virAsprintfQuiet. Regards, Jim
*/
-#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);
Prepare for linking with glib by probing for it at configure time. Per supported platforms target, the min glib versions on relevant distros are: RHEL-8: 2.56.1 RHEL-7: 2.50.3 Debian (Buster): 2.58.3 Debian (Stretch): 2.50.3 OpenBSD (Ports): 2.58.3 FreeBSD (Ports): 2.56.3 OpenSUSE Leap 15: 2.54.3 SLE12-SP2: 2.48.2 Ubuntu (Xenial): 2.48.0 macOS (Homebrew): 2.56.0 This suggests that a minimum glib of 2.48 is a reasonable target. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- configure.ac | 2 ++ libvirt.spec.in | 1 + m4/virt-glib.m4 | 30 ++++++++++++++++++++++++++++++ mingw-libvirt.spec.in | 2 ++ 4 files changed, 35 insertions(+) create mode 100644 m4/virt-glib.m4 diff --git a/configure.ac b/configure.ac index 36e75ac3c0..5036957865 100644 --- a/configure.ac +++ b/configure.ac @@ -317,6 +317,7 @@ LIBVIRT_CHECK_DLOPEN LIBVIRT_CHECK_FIREWALLD LIBVIRT_CHECK_FIREWALLD_ZONE LIBVIRT_CHECK_FUSE +LIBVIRT_CHECK_GLIB LIBVIRT_CHECK_GLUSTER LIBVIRT_CHECK_GNUTLS LIBVIRT_CHECK_HAL @@ -998,6 +999,7 @@ LIBVIRT_RESULT_DLOPEN LIBVIRT_RESULT_FIREWALLD LIBVIRT_RESULT_FIREWALLD_ZONE LIBVIRT_RESULT_FUSE +LIBVIRT_RESULT_GLIB LIBVIRT_RESULT_GLUSTER LIBVIRT_RESULT_GNUTLS LIBVIRT_RESULT_HAL diff --git a/libvirt.spec.in b/libvirt.spec.in index 41c4a142d6..cbd36577a4 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -274,6 +274,7 @@ BuildRequires: systemd-units %if %{with_libxl} BuildRequires: xen-devel %endif +BuildRequires: glib2-devel >= 2.48 BuildRequires: libxml2-devel BuildRequires: libxslt BuildRequires: readline-devel diff --git a/m4/virt-glib.m4 b/m4/virt-glib.m4 new file mode 100644 index 0000000000..9c7acb7889 --- /dev/null +++ b/m4/virt-glib.m4 @@ -0,0 +1,30 @@ +dnl The glib.so library +dnl +dnl Copyright (C) 2016 Red Hat, Inc. +dnl +dnl This library is free software; you can redistribute it and/or +dnl modify it under the terms of the GNU Lesser General Public +dnl License as published by the Free Software Foundation; either +dnl version 2.1 of the License, or (at your option) any later version. +dnl +dnl This library is distributed in the hope that it will be useful, +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +dnl Lesser General Public License for more details. +dnl +dnl You should have received a copy of the GNU Lesser General Public +dnl License along with this library. If not, see +dnl <http://www.gnu.org/licenses/>. +dnl + +AC_DEFUN([LIBVIRT_ARG_GLIB], [ + LIBVIRT_ARG_WITH([GLIB], [glib-2.0 location], [check]) +]) + +AC_DEFUN([LIBVIRT_CHECK_GLIB],[ + LIBVIRT_CHECK_PKG([GLIB], [gthread-2.0], [2.48.0]) +]) + +AC_DEFUN([LIBVIRT_RESULT_GLIB], [ + LIBVIRT_RESULT_LIB([GLIB]) +]) diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in index a20c4b7d74..c29f3eeed2 100644 --- a/mingw-libvirt.spec.in +++ b/mingw-libvirt.spec.in @@ -52,6 +52,8 @@ BuildRequires: mingw32-gcc BuildRequires: mingw64-gcc BuildRequires: mingw32-binutils BuildRequires: mingw64-binutils +BuildRequires: mingw32-glib2 >= 2.48 +BuildRequires: mingw64-glib2 >= 2.48 BuildRequires: mingw32-libgpg-error BuildRequires: mingw64-libgpg-error BuildRequires: mingw32-libgcrypt -- 2.21.0
On Thu, Aug 29, 2019 at 07:02:48PM +0100, Daniel P. Berrangé wrote:
Prepare for linking with glib by probing for it at configure time. Per supported platforms target, the min glib versions on relevant distros are:
RHEL-8: 2.56.1 RHEL-7: 2.50.3 Debian (Buster): 2.58.3 Debian (Stretch): 2.50.3 OpenBSD (Ports): 2.58.3 FreeBSD (Ports): 2.56.3 OpenSUSE Leap 15: 2.54.3 SLE12-SP2: 2.48.2 Ubuntu (Xenial): 2.48.0 macOS (Homebrew): 2.56.0
This suggests that a minimum glib of 2.48 is a reasonable target.
Note that CentOS 6 has 2.28.8
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- configure.ac | 2 ++ libvirt.spec.in | 1 + m4/virt-glib.m4 | 30 ++++++++++++++++++++++++++++++ mingw-libvirt.spec.in | 2 ++ 4 files changed, 35 insertions(+) create mode 100644 m4/virt-glib.m4
diff --git a/m4/virt-glib.m4 b/m4/virt-glib.m4 new file mode 100644 index 0000000000..9c7acb7889 --- /dev/null +++ b/m4/virt-glib.m4 @@ -0,0 +1,30 @@ +dnl The glib.so library +dnl +dnl Copyright (C) 2016 Red Hat, Inc. +dnl +dnl This library is free software; you can redistribute it and/or +dnl modify it under the terms of the GNU Lesser General Public +dnl License as published by the Free Software Foundation; either +dnl version 2.1 of the License, or (at your option) any later version. +dnl +dnl This library is distributed in the hope that it will be useful, +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +dnl Lesser General Public License for more details. +dnl +dnl You should have received a copy of the GNU Lesser General Public +dnl License along with this library. If not, see +dnl <http://www.gnu.org/licenses/>. +dnl + +AC_DEFUN([LIBVIRT_ARG_GLIB], [ + LIBVIRT_ARG_WITH([GLIB], [glib-2.0 location], [check]) +]) + +AC_DEFUN([LIBVIRT_CHECK_GLIB],[ + LIBVIRT_CHECK_PKG([GLIB], [gthread-2.0], [2.48.0])
Given that pretty much everything requires us to allocate memory, failing to find it should be fatal. (Which OTOH would block even docs generation, which should not need C code to be run) Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Mon, Sep 02, 2019 at 04:05:12PM +0200, Ján Tomko wrote:
On Thu, Aug 29, 2019 at 07:02:48PM +0100, Daniel P. Berrangé wrote:
Prepare for linking with glib by probing for it at configure time. Per supported platforms target, the min glib versions on relevant distros are:
RHEL-8: 2.56.1 RHEL-7: 2.50.3 Debian (Buster): 2.58.3 Debian (Stretch): 2.50.3 OpenBSD (Ports): 2.58.3 FreeBSD (Ports): 2.56.3 OpenSUSE Leap 15: 2.54.3 SLE12-SP2: 2.48.2 Ubuntu (Xenial): 2.48.0 macOS (Homebrew): 2.56.0
This suggests that a minimum glib of 2.48 is a reasonable target.
Note that CentOS 6 has 2.28.8
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- configure.ac | 2 ++ libvirt.spec.in | 1 + m4/virt-glib.m4 | 30 ++++++++++++++++++++++++++++++ mingw-libvirt.spec.in | 2 ++ 4 files changed, 35 insertions(+) create mode 100644 m4/virt-glib.m4
diff --git a/m4/virt-glib.m4 b/m4/virt-glib.m4 new file mode 100644 index 0000000000..9c7acb7889 --- /dev/null +++ b/m4/virt-glib.m4 @@ -0,0 +1,30 @@ +dnl The glib.so library +dnl +dnl Copyright (C) 2016 Red Hat, Inc. +dnl +dnl This library is free software; you can redistribute it and/or +dnl modify it under the terms of the GNU Lesser General Public +dnl License as published by the Free Software Foundation; either +dnl version 2.1 of the License, or (at your option) any later version. +dnl +dnl This library is distributed in the hope that it will be useful, +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +dnl Lesser General Public License for more details. +dnl +dnl You should have received a copy of the GNU Lesser General Public +dnl License along with this library. If not, see +dnl <http://www.gnu.org/licenses/>. +dnl + +AC_DEFUN([LIBVIRT_ARG_GLIB], [ + LIBVIRT_ARG_WITH([GLIB], [glib-2.0 location], [check]) +]) + +AC_DEFUN([LIBVIRT_CHECK_GLIB],[ + LIBVIRT_CHECK_PKG([GLIB], [gthread-2.0], [2.48.0])
Given that pretty much everything requires us to allocate memory, failing to find it should be fatal.
Opps, yes, forgot this macro isn't fatal.
(Which OTOH would block even docs generation, which should not need C code to be run)
libvirt.org docs generation is already doomed due to the recent libxml2 min version update, so I'm already working to fix that. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Add the main glib.h to internal.h so that all common code can use it. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/Makefile.am | 2 ++ src/internal.h | 1 + src/lxc/Makefile.inc.am | 2 ++ src/remote/Makefile.inc.am | 1 + src/util/Makefile.inc.am | 1 + tests/Makefile.am | 3 ++- tools/Makefile.am | 1 + 7 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index f5093b9c90..85993309d9 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -34,6 +34,7 @@ AM_CPPFLAGS = -I../gnulib/lib \ WARN_CFLAGS += $(STRICT_FRAME_LIMIT_CFLAGS) AM_CFLAGS = $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(WARN_CFLAGS) \ $(LOCK_CHECKING_CFLAGS) \ $(WIN32_EXTRA_CFLAGS) \ @@ -560,6 +561,7 @@ libvirt_admin_la_LIBADD += \ $(YAJL_LIBS) \ $(DEVMAPPER_LIBS) \ $(LIBXML_LIBS) \ + $(GLIB_LIBS) \ $(SSH2_LIBS) \ $(SASL_LIBS) \ $(GNUTLS_LIBS) \ diff --git a/src/internal.h b/src/internal.h index adc1e3f496..55aaf937cf 100644 --- a/src/internal.h +++ b/src/internal.h @@ -27,6 +27,7 @@ #include <stdint.h> #include <stdio.h> #include <string.h> +#include <glib.h> #if STATIC_ANALYSIS # undef NDEBUG /* Don't let a prior NDEBUG definition cause trouble. */ diff --git a/src/lxc/Makefile.inc.am b/src/lxc/Makefile.inc.am index b4d560702c..0c9618e185 100644 --- a/src/lxc/Makefile.inc.am +++ b/src/lxc/Makefile.inc.am @@ -184,6 +184,7 @@ libvirt_lxc_LDFLAGS = \ $(PIE_LDFLAGS) \ $(CAPNG_LIBS) \ $(LIBXML_LIBS) \ + $(GLIB_LIBS) \ $(NULL) libvirt_lxc_LDADD = \ libvirt.la \ @@ -200,6 +201,7 @@ libvirt_lxc_CFLAGS = \ $(PIE_CFLAGS) \ $(CAPNG_CFLAGS) \ $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(LIBNL_CFLAGS) \ $(FUSE_CFLAGS) \ $(DBUS_CFLAGS) \ diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am index abf04d998a..e05ae64169 100644 --- a/src/remote/Makefile.inc.am +++ b/src/remote/Makefile.inc.am @@ -38,6 +38,7 @@ REMOTE_DAEMON_SOURCES = \ REMOTE_DAEMON_CFLAGS = \ $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(GNUTLS_CFLAGS) \ $(SASL_CFLAGS) \ $(XDR_CFLAGS) \ diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index 46866cf213..73ada1d6a3 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -288,6 +288,7 @@ libvirt_util_la_LIBADD = \ $(DBUS_LIBS) \ $(WIN32_EXTRA_LIBS) \ $(LIBXML_LIBS) \ + $(GLIB_LIBS) \ $(SECDRIVER_LIBS) \ $(NUMACTL_LIBS) \ $(ACL_LIBS) \ diff --git a/tests/Makefile.am b/tests/Makefile.am index a319e386ec..fdc991a97a 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -35,6 +35,7 @@ AM_CFLAGS = \ -Dabs_srcdir="\"$(abs_srcdir)\"" \ -Dabs_top_srcdir="\"$(abs_top_srcdir)\"" \ $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(LIBNL_CFLAGS) \ $(GNUTLS_CFLAGS) \ $(SASL_CFLAGS) \ @@ -522,7 +523,7 @@ libxlxml2domconfigtest_LDADD = $(libxl_LDADDS) $(LIBXML_LIBS) libxlmock_la_SOURCES = \ libxlmock.c -libxlmock_la_CFLAGS = $(LIBXL_CFLAGS) $(LIBXML_CFLAGS) +libxlmock_la_CFLAGS = $(LIBXL_CFLAGS) $(LIBXML_CFLAGS) $(GLIB_CFLAGS) libxlmock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS) libxlmock_la_LIBADD = $(MOCKLIBS_LIBS) diff --git a/tools/Makefile.am b/tools/Makefile.am index 29fdbfe846..9489cb35db 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -36,6 +36,7 @@ AM_CFLAGS = \ $(COVERAGE_CFLAGS) \ $(PIE_CFLAGS) \ $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ $(NULL) AM_LDFLAGS = \ -- 2.21.0
On Thu, Aug 29, 2019 at 07:02:49PM +0100, Daniel P. Berrangé wrote:
Add the main glib.h to internal.h so that all common code can use it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/Makefile.am | 2 ++ src/internal.h | 1 + src/lxc/Makefile.inc.am | 2 ++ src/remote/Makefile.inc.am | 1 + src/util/Makefile.inc.am | 1 + tests/Makefile.am | 3 ++- tools/Makefile.am | 1 + 7 files changed, 10 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
GLib requires that any memory allocated with g_new/g_malloc/etc is free'd by g_free. This means mixing virAlloc with g_free, or g_new with virFree will violate API rules. The easy way to dea with this is to simply make our allocation functions wrappers to the glib functions. Use of our allocation functions can be phased out gradually over time. When doing such conversions, the return values checks can be dropped at the same time. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/src/util/viralloc.c b/src/util/viralloc.c index b74f657733..b50d989415 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -45,10 +45,7 @@ VIR_LOG_INIT("util.alloc"); int virAlloc(void *ptrptr, size_t size) { - *(void **)ptrptr = calloc(1, size); - if (*(void **)ptrptr == NULL) - abort(); - + *(void **)ptrptr = g_malloc0(size); return 0; } @@ -69,10 +66,7 @@ int virAllocN(void *ptrptr, size_t size, size_t count) { - *(void**)ptrptr = calloc(count, size); - if (*(void**)ptrptr == NULL) - abort(); - + *(void**)ptrptr = g_malloc0_n(count, size); return 0; } @@ -94,16 +88,7 @@ int virReallocN(void *ptrptr, size_t size, size_t count) { - void *tmp; - - if (xalloc_oversized(count, size)) - abort(); - - tmp = realloc(*(void**)ptrptr, size * count); - if (!tmp && ((size * count) != 0)) - abort(); - - *(void**)ptrptr = tmp; + *(void **)ptrptr = g_realloc_n(*(void**)ptrptr, size, count); return 0; } @@ -341,9 +326,7 @@ int virAllocVar(void *ptrptr, abort(); alloc_size = struct_size + (element_size * count); - *(void **)ptrptr = calloc(1, alloc_size); - if (*(void **)ptrptr == NULL) - abort(); + *(void **)ptrptr = g_malloc0(alloc_size); return 0; } @@ -360,7 +343,7 @@ void virFree(void *ptrptr) { int save_errno = errno; - free(*(void**)ptrptr); + g_free(*(void**)ptrptr); *(void**)ptrptr = NULL; errno = save_errno; } @@ -393,7 +376,7 @@ void virDispose(void *ptrptr, if (*(void**)ptrptr && count > 0) memset(*(void **)ptrptr, 0, count * element_size); - free(*(void**)ptrptr); + g_free(*(void**)ptrptr); *(void**)ptrptr = NULL; if (countptr) -- 2.21.0
On Thu, Aug 29, 2019 at 07:02:50PM +0100, Daniel P. Berrangé wrote:
GLib requires that any memory allocated with g_new/g_malloc/etc is free'd by g_free. This means mixing virAlloc with g_free, or g_new with virFree will violate API rules. The easy way to dea with this is to simply make our allocation functions wrappers
s/dea/deal/
to the glib functions.
Use of our allocation functions can be phased out gradually over time. When doing such conversions, the return values checks can be dropped at the same time.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-)
I'm afraid this is incomplete. The obvious mismatch being VIR_STRDUP calling strdup while VIR_FREE would call g_free now. Same with (v)asprintf. We also use VIR_FREE to free memory allocated by external libraries (e.g. virXMLPropString; while there is a xmlFree function which is just an alias for free, we never use it in libvirt code) Maybe the safer approach would be to convert the code gradually and explicitly use g_alloc and g_free/g_autoptr without the wrappers? Jano
On Mon, Sep 02, 2019 at 04:52:47PM +0200, Ján Tomko wrote:
On Thu, Aug 29, 2019 at 07:02:50PM +0100, Daniel P. Berrangé wrote:
GLib requires that any memory allocated with g_new/g_malloc/etc is free'd by g_free. This means mixing virAlloc with g_free, or g_new with virFree will violate API rules. The easy way to dea with this is to simply make our allocation functions wrappers
s/dea/deal/
to the glib functions.
Use of our allocation functions can be phased out gradually over time. When doing such conversions, the return values checks can be dropped at the same time.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-)
I'm afraid this is incomplete.
The obvious mismatch being VIR_STRDUP calling strdup while VIR_FREE would call g_free now.
Opps, yes.
Same with (v)asprintf. We also use VIR_FREE to free memory allocated by external libraries (e.g. virXMLPropString; while there is a xmlFree function which is just an alias for free, we never use it in libvirt code)
Hmm, not using xmlFree is arguably a bug in libvirt code. Most other libs quite probably mandate plain 'free'.
Maybe the safer approach would be to convert the code gradually and explicitly use g_alloc and g_free/g_autoptr without the wrappers?
I'm not convince that would be especially safe - the caller of any API which returns an allocated string now checks to look at the impl to see how it was allocated - with deep call chains this gets quite error prone I think. What's interesting is that the glib docs says * It's important to match g_malloc() (and wrappers such as g_new()) with * g_free(), g_slice_alloc() (and wrappers such as g_slice_new()) with * g_slice_free(), plain malloc() with free(), and (if you're using C++) * new with delete and new[] with delete[]. Otherwise bad things can happen, * since these allocators may use different memory pools (and new/delete call * constructors and destructors). but the actual code just does gpointer g_malloc (gsize n_bytes) { if (G_LIKELY (n_bytes)) { gpointer mem; mem = malloc (n_bytes); TRACE (GLIB_MEM_ALLOC((void*) mem, (unsigned int) n_bytes, 0, 0)); if (mem) return mem; g_error ("%s: failed to allocate %"G_GSIZE_FORMAT" bytes", G_STRLOC, n_bytes); } TRACE(GLIB_MEM_ALLOC((void*) NULL, (int) n_bytes, 0, 0)); return NULL; } void g_free (gpointer mem) { free (mem); TRACE(GLIB_MEM_FREE((void*) mem)); } IOW, it is entirely safe to mix free + g_free today - looks like the warning is just talking about a hypothetical risk that doesn't actually exist right now, at least for free vs g_free. The g_slice warning is definitely relevant though since that uses a pool allocator So I'm inclined to fix the mistakes in this patch, and then just deal with using plain 'free' for any cases we deal with external libraries on best effort, in the belief that it doesnt' actually matter for glib anyway. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Sep 02, 2019 at 04:00:36PM +0100, Daniel P. Berrangé wrote:
On Mon, Sep 02, 2019 at 04:52:47PM +0200, Ján Tomko wrote:
On Thu, Aug 29, 2019 at 07:02:50PM +0100, Daniel P. Berrangé wrote:
GLib requires that any memory allocated with g_new/g_malloc/etc is free'd by g_free. This means mixing virAlloc with g_free, or g_new with virFree will violate API rules. The easy way to dea with this is to simply make our allocation functions wrappers
s/dea/deal/
to the glib functions.
Use of our allocation functions can be phased out gradually over time. When doing such conversions, the return values checks can be dropped at the same time.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/viralloc.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-)
I'm afraid this is incomplete.
The obvious mismatch being VIR_STRDUP calling strdup while VIR_FREE would call g_free now.
Opps, yes.
Same with (v)asprintf. We also use VIR_FREE to free memory allocated by external libraries (e.g. virXMLPropString; while there is a xmlFree function which is just an alias for free, we never use it in libvirt code)
Hmm, not using xmlFree is arguably a bug in libvirt code.
Most other libs quite probably mandate plain 'free'.
The other really big thing is that some of our public APIs return an allocated 'char *' that we document the application must call 'free()' on. We cannot change our public API contract in this respect.
Maybe the safer approach would be to convert the code gradually and explicitly use g_alloc and g_free/g_autoptr without the wrappers?
I'm not convince that would be especially safe - the caller of any API which returns an allocated string now checks to look at the impl to see how it was allocated - with deep call chains this gets quite error prone I think.
What's interesting is that the glib docs says
* It's important to match g_malloc() (and wrappers such as g_new()) with * g_free(), g_slice_alloc() (and wrappers such as g_slice_new()) with * g_slice_free(), plain malloc() with free(), and (if you're using C++) * new with delete and new[] with delete[]. Otherwise bad things can happen, * since these allocators may use different memory pools (and new/delete call * constructors and destructors).
but the actual code just does
gpointer g_malloc (gsize n_bytes) { if (G_LIKELY (n_bytes)) { gpointer mem;
mem = malloc (n_bytes); TRACE (GLIB_MEM_ALLOC((void*) mem, (unsigned int) n_bytes, 0, 0)); if (mem) return mem;
g_error ("%s: failed to allocate %"G_GSIZE_FORMAT" bytes", G_STRLOC, n_bytes); }
TRACE(GLIB_MEM_ALLOC((void*) NULL, (int) n_bytes, 0, 0));
return NULL; }
void g_free (gpointer mem) { free (mem); TRACE(GLIB_MEM_FREE((void*) mem)); }
IOW, it is entirely safe to mix free + g_free today - looks like the warning is just talking about a hypothetical risk that doesn't actually exist right now, at least for free vs g_free.
It appears this was a change in GLib 2.46. Before this time they allowed for a custom malloc impl to be registered. In 2.46 this feature was dropped and the system malloc hardcoded: https://gitlab.gnome.org/GNOME/glib/commit/3be6ed6 The docs were not fully updated though. So the only thing that 'g_free' does differently is to emit systemtap trace events.
The g_slice warning is definitely relevant though since that uses a pool allocator
So I'm inclined to fix the mistakes in this patch, and then just deal with using plain 'free' for any cases we deal with external libraries on best effort, in the belief that it doesnt' actually matter for glib anyway.
I think I'm going to propose a docs update to glib to make it explicit that you are permitted to use g_new + free() and see what their maintainers say about that - whether they'll guarantee this forever more. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 8/29/19 12:02 PM, Daniel P. Berrangé wrote:
See this previous thread:
https://www.redhat.com/archives/libvir-list/2019-May/msg00273.html
The executive summary is that catching & reporting ENOMEM is not worth the maint cost because:
- this code will almost never run on Linux hosts
- if it does run it will likely have bad behaviour silently dropping data or crashing the process
- apps using libvirt often do so via a non-C language that aborts/exits the app on OOM regardless, or use other C libraries that abort
- we can build a system that is more reliable when OOM happens by not catching OOM, instead simply letting apps exit, restart and carry on where they left off
The first part of the series does the bare minimum to cull OOM handling.
With this done, the main reason why we never adopted glib is now removed. Thus the second part of this series introduces use of glib into libvirt and converts our our allocation APIs to use the glib allocation APIs internally.
In future I'd especially like to use glib to replace virObject code, which I knowingly wrote as a somewhat pathetic clone of GObject. Eliminating all our DBus code is also another thing I'd like, since Glib's DBus client code is better IMHO.
Note that none of the callers are updated at this point, so they are all still attempting to handle errors from VIR_ALLOC, VIR_STRDUP, etc. If we convert VIR_ALLOC calls to remove return value checks, and then later convert to glib's g_new0 API, we go through two lots of churn which I think is undesirable.
Thus I think it is highly desirable to introduce glib straight away and do a single conversion step. It also means we can introduce other data structures from glib to replace ours and avoid wasting time converting those too.
Overall the code in this series is all quite simple and a nice cleanup. 50% of the lines culled come from the first patch removing OOM testing, the other 50% come from viralloc.c impl simplification
The interesting question is the impact is has on downstreams who have to backport patches to older versions.
If we start converting callers to g_new0, etc, then downstream have to either
* Change g_new0 back into VIR_ALLOC and likely re-add many goto calls we purged
Or
* Backport all the patches in this series that drop OOM handling and introduce glib usage
If we start adopting other glib features beyond g_new0, then downstreams are pretty much forced into option 2.
Given the benefit I think we'll see from this series in our codebase, I'm inclined to say we should prioritize the future, over prioritizing the past (backports), and thus freely adopt use of glib APIs rightaway.
IOW, I think we should expect vendors to backport this series as a starting point, before any other patches. I've not tested but I'm hopeful that such a backport of this series is fairly easy. The viralloc.{c,h} file hasn't seen much change in recent times so ought to be a clean cherry-pick. The glib additions might hit some conflicts in makefiles, but not too terrible I hope. Probably worth doing a test to see just how easy backports are over the past 1 year of releases.
Given the primary maintenance burden I'll be seeing over the next years is on versions 5.1.0 and 4.0.0, I took at stab at backporting this series to those releases. Here are some notes I scribbled while backporting to 5.1.0: Patch1 has conflicts due to moving of virtauto out of viralloc.h by commit a4bfc252. Patch2 has conflicts, mostly due to whitespace changes from switching to '#pragma once'. E.g. commit a6d438a9 in the case of viralloc.h Patch3 has similar conflicts to 2. Too bad you have to patch functions (that have conflicts) in patch 2 that are removed in this patch. Patch4 has conflicts due to '#pragma once' Patch5 applies cleanly! Patch6 has conflicts due to '#pragma once' Patch7 applies cleanly! I then took the refreshed patches and tried applying to 4.0.0: Patch1 has conflicts due to no autoptr stuff in 4.0.0 and due to some changes in testutils.c (hunk 5). Patch2 and patch3 apply cleanly! Patch4 has conflicts in hunk 5 of virstring.h, but I couldn't spot cause of conflict. Patch5 has some fuzz due to introduction of *_FIREWALLD_ZONE Patch6 has conflicts since there are no */Makefile.inc.am in 4.0.0 Regards, Jim
participants (4)
-
Daniel P. Berrangé -
Jim Fehlig -
John Ferlan -
Ján Tomko