[libvirt] [PATCH RFC 0/2] Report OOM on VIR_ALLOC failure
Currently, our code is plenty of following scheme: if (VIR_ALLOC(dummyPtr) < 0) { virReportOOMError(); goto cleanup; } or something similar. What if we just move the OOM reporting into VIR_ALLOC? It would have three nice features: 1) sizeof(code base) gets lower. A lot lower. 2) even for callers which don't follow the schema described above, there is no harm reporting so serious error in the logs. No matter that the callee may fall back and return success. 3) Removing virReportOOMError() from the schema does not need to be done at once, but can be split into several patches. In the worst case scenario - the error gets reported twice. But before I start working on other areas of code, I want to make sure there's an agreement if this is even desired. As an example, how much the code base will lose on weight, I've done the conversion under src/util/: 30 files changed, 81 insertions(+), 221 deletions(-) Michal Privoznik (2): viralloc: Report OOM error on failure util: Don't report OOM twice src/util/iohelper.c | 4 +--- src/util/viralloc.c | 23 ++++++++++++++++++----- src/util/viralloc.h | 13 ++++++++----- src/util/virauthconfig.c | 8 ++------ src/util/vircommand.c | 13 +++---------- src/util/virconf.c | 10 ++-------- src/util/virdnsmasq.c | 19 +++++++------------ src/util/virerror.c | 2 +- src/util/vireventpoll.c | 4 +--- src/util/virfile.c | 4 +--- src/util/virhash.c | 10 ++-------- src/util/virkeyfile.c | 4 +--- src/util/virlockspace.c | 11 +++-------- src/util/virnetdev.c | 4 +--- src/util/virnetdevbandwidth.c | 12 +++--------- src/util/virnetdevmacvlan.c | 4 ++-- src/util/virnetdevvlan.c | 4 +--- src/util/virnetdevvportprofile.c | 4 +--- src/util/virnetlink.c | 8 ++------ src/util/virobject.c | 9 ++++----- src/util/virpci.c | 13 +++---------- src/util/virprocess.c | 4 +--- src/util/virsexpr.c | 4 +--- src/util/virstoragefile.c | 24 ++++++------------------ src/util/virstring.c | 5 +++-- src/util/virsysinfo.c | 8 +++----- src/util/virthreadpool.c | 21 +++++---------------- src/util/virtime.c | 8 ++------ src/util/virtypedparam.c | 36 +++++++++--------------------------- src/util/viruri.c | 2 +- src/util/virusb.c | 8 ++------ src/util/virutil.c | 36 ++++++++---------------------------- src/util/virxml.c | 1 - 33 files changed, 108 insertions(+), 232 deletions(-) -- 1.8.1.5
In nearly all cases of calling VIR_ALLOC*, VIR_REALLOC_N, VIR_EXPAND_N, VIR_RESIZE_N, VIR_*_ELEMENT etc. we want to report OOM error, so our source code base is full of: if (VIR_ALLOC(somePtr) < 0) { virReportOOMError(); goto cleanup; } or similar. Moreover, for those few cases where we don't want to report OOM error (e.g. virReportOOMError() itself) a new VIR_ALLOC_NOOOM macro is being introduced. --- src/util/viralloc.c | 23 ++++++++++++++++++----- src/util/viralloc.h | 13 ++++++++----- src/util/virerror.c | 2 +- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/util/viralloc.c b/src/util/viralloc.c index 342b0eb..60c33d2 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -24,8 +24,11 @@ #include <stdlib.h> #include "viralloc.h" +#include "virerror.h" #include "virlog.h" +#define VIR_FROM_THIS VIR_FROM_NONE + #if TEST_OOM static int testMallocNext = 0; static int testMallocFailFirst = 0; @@ -105,6 +108,7 @@ void virAllocTestHook(void (*func)(int, void*) ATTRIBUTE_UNUSED, * virAlloc: * @ptrptr: pointer to pointer for address of allocated memory * @size: number of bytes to allocate + * @report: report OOM error * * Allocate 'size' bytes of memory. Return the address of the * allocated memory in 'ptrptr'. The newly allocated memory is @@ -112,7 +116,7 @@ void virAllocTestHook(void (*func)(int, void*) ATTRIBUTE_UNUSED, * * Returns -1 on failure to allocate, zero on success */ -int virAlloc(void *ptrptr, size_t size) +int virAlloc(void *ptrptr, size_t size, bool report) { #if TEST_OOM if (virAllocTestFail()) { @@ -122,8 +126,11 @@ int virAlloc(void *ptrptr, size_t size) #endif *(void **)ptrptr = calloc(1, size); - if (*(void **)ptrptr == NULL) + if (*(void **)ptrptr == NULL) { + if (report) + virReportOOMError(); return -1; + } return 0; } @@ -150,8 +157,10 @@ int virAllocN(void *ptrptr, size_t size, size_t count) #endif *(void**)ptrptr = calloc(count, size); - if (*(void**)ptrptr == NULL) + if (*(void**)ptrptr == NULL) { + virReportOOMError(); return -1; + } return 0; } @@ -182,8 +191,10 @@ int virReallocN(void *ptrptr, size_t size, size_t count) return -1; } tmp = realloc(*(void**)ptrptr, size * count); - if (!tmp && (size * count)) + if (!tmp && (size * count)) { + virReportOOMError(); return -1; + } *(void**)ptrptr = tmp; return 0; } @@ -422,8 +433,10 @@ int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t co alloc_size = struct_size + (element_size * count); *(void **)ptrptr = calloc(1, alloc_size); - if (*(void **)ptrptr == NULL) + if (*(void **)ptrptr == NULL) { + virReportOOMError(); return -1; + } return 0; } diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 7be7f82..30ffe15 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -46,7 +46,7 @@ /* Don't call these directly - use the macros below */ -int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK +int virAlloc(void *ptrptr, size_t size, bool report) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); @@ -76,13 +76,16 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * VIR_ALLOC: * @ptr: pointer to hold address of allocated memory * - * Allocate sizeof(*ptr) bytes of memory and store - * the address of allocated memory in 'ptr'. Fill the - * newly allocated memory with zeros. + * Allocate sizeof(*ptr) bytes of memory and store the + * address of allocated memory in 'ptr'. Fill the newly + * allocated memory with zeros. If there's a failure, + * OOM error is reported. The VIR_ALLOC_NOOOM macro + * behaves the same except the OOM error reporting. * * Returns -1 on failure, 0 on success */ -# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr))) +# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)), true) +# define VIR_ALLOC_NOOOM(ptr) virAlloc(&(ptr), sizeof(*(ptr)), true) /** * VIR_ALLOC_N: diff --git a/src/util/virerror.c b/src/util/virerror.c index c30642a..c033129 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -204,7 +204,7 @@ virLastErrorObject(void) virErrorPtr err; err = virThreadLocalGet(&virLastErr); if (!err) { - if (VIR_ALLOC(err) < 0) + if (VIR_ALLOC_NOOOM(err) < 0) return NULL; if (virThreadLocalSet(&virLastErr, err) < 0) VIR_FREE(err); -- 1.8.1.5
On 03/22/2013 07:44 AM, Michal Privoznik wrote:
In nearly all cases of calling VIR_ALLOC*, VIR_REALLOC_N, VIR_EXPAND_N, VIR_RESIZE_N, VIR_*_ELEMENT etc. we want to report OOM error, so our source code base is full of:
if (VIR_ALLOC(somePtr) < 0) { virReportOOMError(); goto cleanup; }
or similar. Moreover, for those few cases where we don't want to report OOM error (e.g. virReportOOMError() itself) a new VIR_ALLOC_NOOOM macro is being introduced. --- [...] diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 7be7f82..30ffe15 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -46,7 +46,7 @@
/* Don't call these directly - use the macros below */ -int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK +int virAlloc(void *ptrptr, size_t size, bool report) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); @@ -76,13 +76,16 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * VIR_ALLOC: * @ptr: pointer to hold address of allocated memory * - * Allocate sizeof(*ptr) bytes of memory and store - * the address of allocated memory in 'ptr'. Fill the - * newly allocated memory with zeros. + * Allocate sizeof(*ptr) bytes of memory and store the + * address of allocated memory in 'ptr'. Fill the newly + * allocated memory with zeros. If there's a failure, + * OOM error is reported. The VIR_ALLOC_NOOOM macro + * behaves the same except the OOM error reporting. * * Returns -1 on failure, 0 on success */ -# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr))) +# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)), true) +# define VIR_ALLOC_NOOOM(ptr) virAlloc(&(ptr), sizeof(*(ptr)), true)
should be false in the 2nd case...
/** * VIR_ALLOC_N: diff --git a/src/util/virerror.c b/src/util/virerror.c index c30642a..c033129 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -204,7 +204,7 @@ virLastErrorObject(void) virErrorPtr err; err = virThreadLocalGet(&virLastErr); if (!err) { - if (VIR_ALLOC(err) < 0) + if (VIR_ALLOC_NOOOM(err) < 0) return NULL; if (virThreadLocalSet(&virLastErr, err) < 0) VIR_FREE(err);
ACK with nit fixed.
Adapt code under src/util/ to fact, that VIR_ALLOC* now reports OOM error. There is no need to report it twice now. --- src/util/iohelper.c | 4 +--- src/util/virauthconfig.c | 8 ++------ src/util/vircommand.c | 13 +++---------- src/util/virconf.c | 10 ++-------- src/util/virdnsmasq.c | 19 +++++++------------ src/util/vireventpoll.c | 4 +--- src/util/virfile.c | 4 +--- src/util/virhash.c | 10 ++-------- src/util/virkeyfile.c | 4 +--- src/util/virlockspace.c | 11 +++-------- src/util/virnetdev.c | 4 +--- src/util/virnetdevbandwidth.c | 12 +++--------- src/util/virnetdevmacvlan.c | 4 ++-- src/util/virnetdevvlan.c | 4 +--- src/util/virnetdevvportprofile.c | 4 +--- src/util/virnetlink.c | 8 ++------ src/util/virobject.c | 9 ++++----- src/util/virpci.c | 13 +++---------- src/util/virprocess.c | 4 +--- src/util/virsexpr.c | 4 +--- src/util/virstoragefile.c | 24 ++++++------------------ src/util/virstring.c | 5 +++-- src/util/virsysinfo.c | 8 +++----- src/util/virthreadpool.c | 21 +++++---------------- src/util/virtime.c | 8 ++------ src/util/virtypedparam.c | 36 +++++++++--------------------------- src/util/viruri.c | 2 +- src/util/virusb.c | 8 ++------ src/util/virutil.c | 36 ++++++++---------------------------- src/util/virxml.c | 1 - 30 files changed, 81 insertions(+), 221 deletions(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 2230bcb..d3c966a 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -94,10 +94,8 @@ runIO(const char *path, int fd, int oflags, unsigned long long length) } buf = base; #else - if (VIR_ALLOC_N(buf, buflen + alignMask) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(buf, buflen + alignMask) < 0) goto cleanup; - } base = buf; buf = (char *) (((intptr_t) base + alignMask) & ~alignMask); #endif diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c index 1d1f084..8847d9e 100644 --- a/src/util/virauthconfig.c +++ b/src/util/virauthconfig.c @@ -43,10 +43,8 @@ virAuthConfigPtr virAuthConfigNew(const char *path) { virAuthConfigPtr auth; - if (VIR_ALLOC(auth) < 0) { - virReportOOMError(); + if (VIR_ALLOC(auth) < 0) goto error; - } if (!(auth->path = strdup(path))) { virReportOOMError(); @@ -73,10 +71,8 @@ virAuthConfigPtr virAuthConfigNewData(const char *path, { virAuthConfigPtr auth; - if (VIR_ALLOC(auth) < 0) { - virReportOOMError(); + if (VIR_ALLOC(auth) < 0) goto error; - } if (!(auth->path = strdup(path))) { virReportOOMError(); diff --git a/src/util/vircommand.c b/src/util/vircommand.c index ac56a63..052f1e7 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1804,17 +1804,13 @@ virCommandProcessIO(virCommandPtr cmd) * results accumulated over a prior run of the same command. */ if (cmd->outbuf) { outfd = cmd->outfd; - if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0) { - virReportOOMError(); + if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0) ret = -1; - } } if (cmd->errbuf) { errfd = cmd->errfd; - if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0) { - virReportOOMError(); + if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0) ret = -1; - } } if (ret == -1) goto cleanup; @@ -1888,10 +1884,8 @@ virCommandProcessIO(virCommandPtr cmd) else errfd = -1; } else { - if (VIR_REALLOC_N(*buf, *len + done + 1) < 0) { - virReportOOMError(); + if (VIR_REALLOC_N(*buf, *len + done + 1) < 0) goto cleanup; - } memcpy(*buf + *len, data, done); *len += done; } @@ -2435,7 +2429,6 @@ int virCommandHandshakeWait(virCommandPtr cmd) char *msg; ssize_t len; if (VIR_ALLOC_N(msg, 1024) < 0) { - virReportOOMError(); VIR_FORCE_CLOSE(cmd->handshakeWait[0]); return -1; } diff --git a/src/util/virconf.c b/src/util/virconf.c index 16f074a..1c7075f 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -173,10 +173,8 @@ virConfNew(void) { virConfPtr ret; - if (VIR_ALLOC(ret) < 0) { - virReportOOMError(); + if (VIR_ALLOC(ret) < 0) return NULL; - } ret->filename = NULL; ret->flags = 0; @@ -225,10 +223,8 @@ virConfAddEntry(virConfPtr conf, char *name, virConfValuePtr value, char *comm) if ((comm == NULL) && (name == NULL)) return NULL; - if (VIR_ALLOC(ret) < 0) { - virReportOOMError(); + if (VIR_ALLOC(ret) < 0) return NULL; - } ret->name = name; ret->value = value; @@ -538,7 +534,6 @@ virConfParseValue(virConfParserCtxtPtr ctxt) return NULL; } if (VIR_ALLOC(ret) < 0) { - virReportOOMError(); virConfFreeList(lst); VIR_FREE(str); return NULL; @@ -900,7 +895,6 @@ virConfSetValue(virConfPtr conf, if (!cur) { if (VIR_ALLOC(cur) < 0) { - virReportOOMError(); virConfFreeValue(value); return -1; } diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 2e63d83..2e2ccd1 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -117,8 +117,10 @@ addnhostsAdd(dnsmasqAddnHostsfile *addnhostsfile, if (VIR_ALLOC(addnhostsfile->hosts[idx].hostnames) < 0) goto alloc_error; - if (virAsprintf(&addnhostsfile->hosts[idx].ip, "%s", ipstr) < 0) + if (virAsprintf(&addnhostsfile->hosts[idx].ip, "%s", ipstr) < 0) { + virReportOOMError(); goto alloc_error; + } addnhostsfile->hosts[idx].nhostnames = 0; addnhostsfile->nhosts++; @@ -137,7 +139,6 @@ addnhostsAdd(dnsmasqAddnHostsfile *addnhostsfile, return 0; alloc_error: - virReportOOMError(); VIR_FREE(ipstr); return -1; } @@ -148,10 +149,8 @@ addnhostsNew(const char *name, { dnsmasqAddnHostsfile *addnhostsfile; - if (VIR_ALLOC(addnhostsfile) < 0) { - virReportOOMError(); + if (VIR_ALLOC(addnhostsfile) < 0) return NULL; - } addnhostsfile->hosts = NULL; addnhostsfile->nhosts = 0; @@ -308,7 +307,7 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile, { char *ipstr = NULL; if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0) - goto alloc_error; + return -1; if (!(ipstr = virSocketAddrFormat(ip))) return -1; @@ -359,10 +358,8 @@ hostsfileNew(const char *name, { dnsmasqHostsfile *hostsfile; - if (VIR_ALLOC(hostsfile) < 0) { - virReportOOMError(); + if (VIR_ALLOC(hostsfile) < 0) return NULL; - } hostsfile->hosts = NULL; hostsfile->nhosts = 0; @@ -463,10 +460,8 @@ dnsmasqContextNew(const char *network_name, { dnsmasqContext *ctx; - if (VIR_ALLOC(ctx) < 0) { - virReportOOMError(); + if (VIR_ALLOC(ctx) < 0) return NULL; - } if (!(ctx->config_dir = strdup(config_dir))) { virReportOOMError(); diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c index 5c6d31b..95b60d6 100644 --- a/src/util/vireventpoll.c +++ b/src/util/vireventpoll.c @@ -379,10 +379,8 @@ static struct pollfd *virEventPollMakePollFDs(int *nfds) { } /* Setup the poll file handle data structs */ - if (VIR_ALLOC_N(fds, *nfds) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(fds, *nfds) < 0) return NULL; - } *nfds = 0; for (i = 0 ; i < eventLoop.handlesCount ; i++) { diff --git a/src/util/virfile.c b/src/util/virfile.c index 4a9fa81..8442c7b 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -193,10 +193,8 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) return NULL; } - if (VIR_ALLOC(ret) < 0) { - virReportOOMError(); + if (VIR_ALLOC(ret) < 0) return NULL; - } mode = fcntl(*fd, F_GETFL); diff --git a/src/util/virhash.c b/src/util/virhash.c index 2fe8751..dc4ab4b 100644 --- a/src/util/virhash.c +++ b/src/util/virhash.c @@ -126,10 +126,8 @@ virHashTablePtr virHashCreateFull(ssize_t size, if (size <= 0) size = 256; - if (VIR_ALLOC(table) < 0) { - virReportOOMError(); + if (VIR_ALLOC(table) < 0) return NULL; - } table->seed = virRandomBits(32); table->size = size; @@ -141,7 +139,6 @@ virHashTablePtr virHashCreateFull(ssize_t size, table->keyFree = keyFree; if (VIR_ALLOC_N(table->table, size) < 0) { - virReportOOMError(); VIR_FREE(table); return NULL; } @@ -201,7 +198,6 @@ virHashGrow(virHashTablePtr table, size_t size) return -1; if (VIR_ALLOC_N(table->table, size) < 0) { - virReportOOMError(); table->table = oldtable; return -1; } @@ -670,10 +666,8 @@ virHashKeyValuePairPtr virHashGetItems(virHashTablePtr table, if (numElems < 0) return NULL; - if (VIR_ALLOC_N(iter.sortArray, numElems + 1)) { - virReportOOMError(); + if (VIR_ALLOC_N(iter.sortArray, numElems + 1)) return NULL; - } virHashForEach(table, virHashGetKeysIterator, &iter); diff --git a/src/util/virkeyfile.c b/src/util/virkeyfile.c index d77e95d..fe2f7e5 100644 --- a/src/util/virkeyfile.c +++ b/src/util/virkeyfile.c @@ -286,10 +286,8 @@ virKeyFilePtr virKeyFileNew(void) { virKeyFilePtr conf; - if (VIR_ALLOC(conf) < 0) { - virReportOOMError(); + if (VIR_ALLOC(conf) < 0) goto error; - } if (!(conf->groups = virHashCreate(10, virKeyFileEntryFree))) diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 4ff0f3a..cdccdda 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -224,7 +224,7 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, res->lockHeld = true; if (VIR_EXPAND_N(res->owners, res->nOwners, 1) < 0) - goto no_memory; + goto error; res->owners[res->nOwners-1] = owner; @@ -349,10 +349,8 @@ virLockSpacePtr virLockSpaceNewPostExecRestart(virJSONValuePtr object) size_t j; int m; - if (VIR_ALLOC(res) < 0) { - virReportOOMError(); + if (VIR_ALLOC(res) < 0) goto error; - } res->fd = -1; if (!(tmp = virJSONValueObjectGetString(child, "name"))) { @@ -420,7 +418,6 @@ virLockSpacePtr virLockSpaceNewPostExecRestart(virJSONValuePtr object) res->nOwners = m; if (VIR_ALLOC_N(res->owners, res->nOwners) < 0) { - virReportOOMError(); virLockSpaceResourceFree(res); goto error; } @@ -649,10 +646,8 @@ int virLockSpaceAcquireResource(virLockSpacePtr lockspace, if ((res->flags & VIR_LOCK_SPACE_ACQUIRE_SHARED) && (flags & VIR_LOCK_SPACE_ACQUIRE_SHARED)) { - if (VIR_EXPAND_N(res->owners, res->nOwners, 1) < 0) { - virReportOOMError(); + if (VIR_EXPAND_N(res->owners, res->nOwners, 1) < 0) goto cleanup; - } res->owners[res->nOwners-1] = owner; goto done; diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 00e0f94..652668e 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -995,10 +995,8 @@ virNetDevGetVirtualFunctions(const char *pfname, n_vfname) < 0) goto cleanup; - if (VIR_ALLOC_N(*vfname, *n_vfname) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(*vfname, *n_vfname) < 0) goto cleanup; - } for (i = 0; i < *n_vfname; i++) { diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 2c5b63a..2f4baae 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -296,25 +296,19 @@ virNetDevBandwidthCopy(virNetDevBandwidthPtr *dest, return 0; } - if (VIR_ALLOC(*dest) < 0) { - virReportOOMError(); + if (VIR_ALLOC(*dest) < 0) goto cleanup; - } if (src->in) { - if (VIR_ALLOC((*dest)->in) < 0) { - virReportOOMError(); + if (VIR_ALLOC((*dest)->in) < 0) goto cleanup; - } memcpy((*dest)->in, src->in, sizeof(*src->in)); } if (src->out) { - if (VIR_ALLOC((*dest)->out) < 0) { - virReportOOMError(); + if (VIR_ALLOC((*dest)->out) < 0) VIR_FREE((*dest)->in); goto cleanup; - } memcpy((*dest)->out, src->out, sizeof(*src->out)); } diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index ddea11f..1e351ef 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -775,11 +775,11 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, if (virtPortProfile && virNetlinkEventServiceIsRunning(NETLINK_ROUTE)) { if (VIR_ALLOC(calld) < 0) - goto memory_error; + goto error; if ((calld->cr_ifname = strdup(ifname)) == NULL) goto memory_error; if (VIR_ALLOC(calld->virtPortProfile) < 0) - goto memory_error; + goto error; memcpy(calld->virtPortProfile, virtPortProfile, sizeof(*virtPortProfile)); virMacAddrSet(&calld->macaddress, macaddress); if ((calld->linkdev = strdup(linkdev)) == NULL) diff --git a/src/util/virnetdevvlan.c b/src/util/virnetdevvlan.c index 2fe2017..e05e7e6 100644 --- a/src/util/virnetdevvlan.c +++ b/src/util/virnetdevvlan.c @@ -82,10 +82,8 @@ virNetDevVlanCopy(virNetDevVlanPtr dst, const virNetDevVlanPtr src) if (!src || src->nTags == 0) return 0; - if (VIR_ALLOC_N(dst->tag, src->nTags) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(dst->tag, src->nTags) < 0) return -1; - } dst->trunk = src->trunk; dst->nTags = src->nTags; diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index bb97e3a..c51854a 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -409,10 +409,8 @@ int virNetDevVPortProfileMerge3(virNetDevVPortProfilePtr *result, } /* at least one of the source profiles is non-empty */ - if (VIR_ALLOC(*result) < 0) { - virReportOOMError(); + if (VIR_ALLOC(*result) < 0) return ret; - } /* start with the interface's profile. There are no pointers in a * virtualPortProfile, so a shallow copy is sufficient. diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 0b36fdc..204eef3 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -516,10 +516,8 @@ virNetlinkEventServiceStart(unsigned int protocol, unsigned int groups) VIR_INFO("starting netlink event service with protocol %d", protocol); - if (VIR_ALLOC(srv) < 0) { - virReportOOMError(); + if (VIR_ALLOC(srv) < 0) return -1; - } if (virMutexInit(&srv->lock) < 0) { VIR_FREE(srv); @@ -645,10 +643,8 @@ virNetlinkEventAddClient(virNetlinkEventHandleCallback handleCB, VIR_DEBUG("Used %zu handle slots, adding at least %d more", srv->handlesAlloc, NETLINK_EVENT_ALLOC_EXTENT); if (VIR_RESIZE_N(srv->handles, srv->handlesAlloc, - srv->handlesCount, NETLINK_EVENT_ALLOC_EXTENT) < 0) { - virReportOOMError(); + srv->handlesCount, NETLINK_EVENT_ALLOC_EXTENT) < 0) goto error; - } } r = srv->handlesCount++; diff --git a/src/util/virobject.c b/src/util/virobject.c index 808edc4..d6a8b3a 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -132,7 +132,7 @@ virClassPtr virClassNew(virClassPtr parent, } if (VIR_ALLOC(klass) < 0) - goto no_memory; + goto cleanup; klass->parent = parent; if (!(klass->name = strdup(name))) @@ -144,8 +144,9 @@ virClassPtr virClassNew(virClassPtr parent, return klass; no_memory: - VIR_FREE(klass); virReportOOMError(); +cleanup: + VIR_FREE(klass); return NULL; } @@ -188,10 +189,8 @@ void *virObjectNew(virClassPtr klass) virObjectPtr obj = NULL; char *somebytes; - if (VIR_ALLOC_N(somebytes, klass->objectSize) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(somebytes, klass->objectSize) < 0) return NULL; - } obj = (virObjectPtr)somebytes; obj->magic = klass->magic; diff --git a/src/util/virpci.c b/src/util/virpci.c index 4bb82aa..570dfa1 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1374,10 +1374,8 @@ virPCIDeviceNew(unsigned domain, char *vendor = NULL; char *product = NULL; - if (VIR_ALLOC(dev) < 0) { - virReportOOMError(); + if (VIR_ALLOC(dev) < 0) return NULL; - } dev->domain = domain; dev->bus = bus; @@ -1558,10 +1556,8 @@ virPCIDeviceListAdd(virPCIDeviceListPtr list, return -1; } - if (VIR_REALLOC_N(list->devs, list->count+1) < 0) { - virReportOOMError(); + if (VIR_REALLOC_N(list->devs, list->count+1) < 0) return -1; - } list->devs[list->count++] = dev; @@ -1923,10 +1919,8 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link, } config_address = basename(device_path); - if (VIR_ALLOC(*bdf) != 0) { - virReportOOMError(); + if (VIR_ALLOC(*bdf) != 0) goto out; - } if (virPCIParseDeviceAddress(config_address, *bdf) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2016,7 +2010,6 @@ virPCIGetVirtualFunctions(const char *sysfs_path, *num_virtual_functions); if (VIR_REALLOC_N(*virtual_functions, (*num_virtual_functions) + 1) != 0) { - virReportOOMError(); VIR_FREE(device_link); goto out; } diff --git a/src/util/virprocess.c b/src/util/virprocess.c index a492bd1..4a81503 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -527,10 +527,8 @@ int virProcessGetNamespaces(pid_t pid, if (de->d_name[0] == '.') continue; - if (VIR_EXPAND_N(*fdlist, *nfdlist, 1) < 0) { - virReportOOMError(); + if (VIR_EXPAND_N(*fdlist, *nfdlist, 1) < 0) goto cleanup; - } if (virAsprintf(&nsfile, "%s/%s", nsdir, de->d_name) < 0) { virReportOOMError(); diff --git a/src/util/virsexpr.c b/src/util/virsexpr.c index 81dfbe6..5c19da4 100644 --- a/src/util/virsexpr.c +++ b/src/util/virsexpr.c @@ -48,10 +48,8 @@ sexpr_new(void) { struct sexpr *ret; - if (VIR_ALLOC(ret) < 0) { - virReportOOMError(); + if (VIR_ALLOC(ret) < 0) return NULL; - } ret->kind = SEXPR_NIL; return ret; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index aabb5c8..f12c25e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -318,10 +318,8 @@ qcowXGetBackingStore(char **res, return BACKING_STORE_INVALID; if (size + 1 == 0) return BACKING_STORE_INVALID; - if (VIR_ALLOC_N(*res, size + 1) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(*res, size + 1) < 0) return BACKING_STORE_ERROR; - } memcpy(*res, buf + offset, size); (*res)[size] = '\0'; @@ -396,10 +394,8 @@ vmdk4GetBackingStore(char **res, size_t len; int ret = BACKING_STORE_ERROR; - if (VIR_ALLOC_N(desc, STORAGE_MAX_HEAD + 1) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(desc, STORAGE_MAX_HEAD + 1) < 0) goto cleanup; - } *res = NULL; /* @@ -481,10 +477,8 @@ qedGetBackingStore(char **res, return BACKING_STORE_OK; if (offset + size > buf_size || offset + size < offset) return BACKING_STORE_INVALID; - if (VIR_ALLOC_N(*res, size + 1) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(*res, size + 1) < 0) return BACKING_STORE_ERROR; - } memcpy(*res, buf + offset, size); (*res)[size] = '\0'; @@ -686,10 +680,8 @@ virStorageFileGetMetadataInternal(const char *path, VIR_DEBUG("path=%s, fd=%d, format=%d", path, fd, format); - if (VIR_ALLOC(meta) < 0) { - virReportOOMError(); + if (VIR_ALLOC(meta) < 0) return NULL; - } if (fstat(fd, &sb) < 0) { virReportSystemError(errno, @@ -707,10 +699,8 @@ virStorageFileGetMetadataInternal(const char *path, goto cleanup; } - if (VIR_ALLOC_N(buf, len) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(buf, len) < 0) goto cleanup; - } if ((len = read(fd, buf, len)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), path); @@ -849,10 +839,8 @@ virStorageFileProbeFormatFromFD(const char *path, int fd) return VIR_STORAGE_FILE_DIR; } - if (VIR_ALLOC_N(head, len) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(head, len) < 0) return -1; - } if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { virReportSystemError(errno, _("cannot set to start of '%s'"), path); diff --git a/src/util/virstring.c b/src/util/virstring.c index 0420ca3..35d16be 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -79,7 +79,7 @@ char **virStringSplit(const char *string, size_t len = tmp - remainder; if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0) - goto no_memory; + goto error; if (!(tokens[ntokens] = strndup(remainder, len))) goto no_memory; @@ -90,7 +90,7 @@ char **virStringSplit(const char *string, } if (*string) { if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0) - goto no_memory; + goto error; if (!(tokens[ntokens] = strdup(remainder))) goto no_memory; @@ -105,6 +105,7 @@ char **virStringSplit(const char *string, no_memory: virReportOOMError(); +error: for (i = 0 ; i < ntokens ; i++) VIR_FREE(tokens[i]); VIR_FREE(tokens); diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 83c445d..cde7611 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -331,10 +331,8 @@ virSysinfoParseProcessor(const char *base, virSysinfoDefPtr ret) while ((tmp_base = strstr(tmp_base, "processor ")) && (tmp_base = virSysinfoParseLine(tmp_base, "processor ", &procline))) { - if (VIR_EXPAND_N(ret->processor, ret->nprocessor, 1) < 0) { - virReportOOMError(); + if (VIR_EXPAND_N(ret->processor, ret->nprocessor, 1) < 0) goto cleanup; - } processor = &ret->processor[ret->nprocessor - 1]; processor->processor_manufacturer = strdup(manufacturer); if (!virSysinfoParseDelimited(procline, "version", @@ -778,7 +776,7 @@ virSysinfoRead(void) { goto cleanup; if (VIR_ALLOC(ret) < 0) - goto no_memory; + goto error; ret->type = VIR_SYSINFO_SMBIOS; @@ -806,7 +804,7 @@ cleanup: no_memory: virReportOOMError(); - +error: virSysinfoDefFree(ret); ret = NULL; goto cleanup; diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index e657145..b1e2c0c 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -169,10 +169,8 @@ virThreadPoolPtr virThreadPoolNew(size_t minWorkers, if (minWorkers > maxWorkers) minWorkers = maxWorkers; - if (VIR_ALLOC(pool) < 0) { - virReportOOMError(); + if (VIR_ALLOC(pool) < 0) return NULL; - } pool->jobList.tail = pool->jobList.head = NULL; @@ -193,10 +191,8 @@ virThreadPoolPtr virThreadPoolNew(size_t minWorkers, pool->maxWorkers = maxWorkers; for (i = 0; i < minWorkers; i++) { - if (VIR_ALLOC(data) < 0) { - virReportOOMError(); + if (VIR_ALLOC(data) < 0) goto error; - } data->pool = pool; data->cond = &pool->cond; @@ -216,10 +212,8 @@ virThreadPoolPtr virThreadPoolNew(size_t minWorkers, goto error; for (i = 0; i < prioWorkers; i++) { - if (VIR_ALLOC(data) < 0) { - virReportOOMError(); + if (VIR_ALLOC(data) < 0) goto error; - } data->pool = pool; data->cond = &pool->prioCond; data->priority = true; @@ -313,14 +307,11 @@ int virThreadPoolSendJob(virThreadPoolPtr pool, if (pool->freeWorkers - pool->jobQueueDepth <= 0 && pool->nWorkers < pool->maxWorkers) { - if (VIR_EXPAND_N(pool->workers, pool->nWorkers, 1) < 0) { - virReportOOMError(); + if (VIR_EXPAND_N(pool->workers, pool->nWorkers, 1) < 0) goto error; - } if (VIR_ALLOC(data) < 0) { pool->nWorkers--; - virReportOOMError(); goto error; } @@ -337,10 +328,8 @@ int virThreadPoolSendJob(virThreadPoolPtr pool, } } - if (VIR_ALLOC(job) < 0) { - virReportOOMError(); + if (VIR_ALLOC(job) < 0) goto error; - } job->data = jobData; job->priority = priority; diff --git a/src/util/virtime.c b/src/util/virtime.c index b54a4e8..4684716 100644 --- a/src/util/virtime.c +++ b/src/util/virtime.c @@ -304,10 +304,8 @@ char *virTimeStringNow(void) { char *ret; - if (VIR_ALLOC_N(ret, VIR_TIME_STRING_BUFLEN) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(ret, VIR_TIME_STRING_BUFLEN) < 0) return NULL; - } if (virTimeStringNowRaw(ret) < 0) { virReportSystemError(errno, "%s", @@ -335,10 +333,8 @@ char *virTimeStringThen(unsigned long long when) { char *ret; - if (VIR_ALLOC_N(ret, VIR_TIME_STRING_BUFLEN) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(ret, VIR_TIME_STRING_BUFLEN) < 0) return NULL; - } if (virTimeStringThenRaw(when, ret) < 0) { virReportSystemError(errno, "%s", diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index c196321..aa850f5 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -603,10 +603,8 @@ virTypedParamsAddInt(virTypedParameterPtr *params, virResetLastError(); VIR_TYPED_PARAM_CHECK(); - if (VIR_RESIZE_N(*params, max, n, 1) < 0) { - virReportOOMError(); + if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; - } *maxparams = max; if (virTypedParameterAssign(*params + n, name, @@ -653,10 +651,8 @@ virTypedParamsAddUInt(virTypedParameterPtr *params, virResetLastError(); VIR_TYPED_PARAM_CHECK(); - if (VIR_RESIZE_N(*params, max, n, 1) < 0) { - virReportOOMError(); + if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; - } *maxparams = max; if (virTypedParameterAssign(*params + n, name, @@ -703,10 +699,8 @@ virTypedParamsAddLLong(virTypedParameterPtr *params, virResetLastError(); VIR_TYPED_PARAM_CHECK(); - if (VIR_RESIZE_N(*params, max, n, 1) < 0) { - virReportOOMError(); + if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; - } *maxparams = max; if (virTypedParameterAssign(*params + n, name, @@ -753,10 +747,8 @@ virTypedParamsAddULLong(virTypedParameterPtr *params, virResetLastError(); VIR_TYPED_PARAM_CHECK(); - if (VIR_RESIZE_N(*params, max, n, 1) < 0) { - virReportOOMError(); + if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; - } *maxparams = max; if (virTypedParameterAssign(*params + n, name, @@ -803,10 +795,8 @@ virTypedParamsAddDouble(virTypedParameterPtr *params, virResetLastError(); VIR_TYPED_PARAM_CHECK(); - if (VIR_RESIZE_N(*params, max, n, 1) < 0) { - virReportOOMError(); + if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; - } *maxparams = max; if (virTypedParameterAssign(*params + n, name, @@ -853,10 +843,8 @@ virTypedParamsAddBoolean(virTypedParameterPtr *params, virResetLastError(); VIR_TYPED_PARAM_CHECK(); - if (VIR_RESIZE_N(*params, max, n, 1) < 0) { - virReportOOMError(); + if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; - } *maxparams = max; if (virTypedParameterAssign(*params + n, name, @@ -906,16 +894,12 @@ virTypedParamsAddString(virTypedParameterPtr *params, virResetLastError(); VIR_TYPED_PARAM_CHECK(); - if (VIR_RESIZE_N(*params, max, n, 1) < 0) { - virReportOOMError(); + if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; - } *maxparams = max; - if (value && !(str = strdup(value))) { - virReportOOMError(); + if (value && !(str = strdup(value))) goto error; - } if (virTypedParameterAssign(*params + n, name, VIR_TYPED_PARAM_STRING, str) < 0) { @@ -968,10 +952,8 @@ virTypedParamsAddFromString(virTypedParameterPtr *params, virResetLastError(); VIR_TYPED_PARAM_CHECK(); - if (VIR_RESIZE_N(*params, max, n, 1) < 0) { - virReportOOMError(); + if (VIR_RESIZE_N(*params, max, n, 1) < 0) goto error; - } *maxparams = max; if (virTypedParameterAssignFromStr(*params + n, name, type, value) < 0) diff --git a/src/util/viruri.c b/src/util/viruri.c index 2888cb0..465fdf0 100644 --- a/src/util/viruri.c +++ b/src/util/viruri.c @@ -168,7 +168,7 @@ virURIParse(const char *uri) } if (VIR_ALLOC(ret) < 0) - goto no_memory; + goto error; if (xmluri->scheme && !(ret->scheme = strdup(xmluri->scheme))) diff --git a/src/util/virusb.c b/src/util/virusb.c index 5974602..4ba6de5 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -332,10 +332,8 @@ virUSBDeviceNew(unsigned int bus, { virUSBDevicePtr dev; - if (VIR_ALLOC(dev) < 0) { - virReportOOMError(); + if (VIR_ALLOC(dev) < 0) return NULL; - } dev->bus = bus; dev->dev = devno; @@ -454,10 +452,8 @@ virUSBDeviceListAdd(virUSBDeviceListPtr list, return -1; } - if (VIR_REALLOC_N(list->devs, list->count+1) < 0) { - virReportOOMError(); + if (VIR_REALLOC_N(list->devs, list->count+1) < 0) return -1; - } list->devs[list->count++] = dev; diff --git a/src/util/virutil.c b/src/util/virutil.c index 42b4295..7f375d4 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -368,10 +368,8 @@ virPipeReadUntilEOF(int outfd, int errfd, buf = ((fds[i].fd == outfd) ? outbuf : errbuf); size = (*buf ? strlen(*buf) : 0); - if (VIR_REALLOC_N(*buf, size+got+1) < 0) { - virReportOOMError(); + if (VIR_REALLOC_N(*buf, size+got+1) < 0) goto error; - } memmove(*buf+size, data, got); (*buf)[size+got] = '\0'; } @@ -2233,10 +2231,8 @@ char *virIndexToDiskName(int idx, const char *prefix) offset = strlen(prefix); - if (VIR_ALLOC_N(name, offset + i + 1)) { - virReportOOMError(); + if (VIR_ALLOC_N(name, offset + i + 1)) return NULL; - } strcpy(name, prefix); name[offset + i] = '\0'; @@ -2355,10 +2351,8 @@ static char *virGetUserEnt(uid_t uid, if (val < 0) strbuflen = 1024; - if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) return NULL; - } /* * From the manpage (terrifying but true): @@ -2369,7 +2363,6 @@ static char *virGetUserEnt(uid_t uid, */ while ((rc = getpwuid_r(uid, &pwbuf, strbuf, strbuflen, &pw)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) { - virReportOOMError(); VIR_FREE(strbuf); return NULL; } @@ -2408,10 +2401,8 @@ static char *virGetGroupEnt(gid_t gid) if (val < 0) strbuflen = 1024; - if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) return NULL; - } /* * From the manpage (terrifying but true): @@ -2422,7 +2413,6 @@ static char *virGetGroupEnt(gid_t gid) */ while ((rc = getgrgid_r(gid, &grbuf, strbuf, strbuflen, &gr)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) { - virReportOOMError(); VIR_FREE(strbuf); return NULL; } @@ -2527,16 +2517,12 @@ virGetUserIDByName(const char *name, uid_t *uid) if (val < 0) strbuflen = 1024; - if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) goto cleanup; - } while ((rc = getpwnam_r(name, &pwbuf, strbuf, strbuflen, &pw)) == ERANGE) { - if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) { - virReportOOMError(); + if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) goto cleanup; - } } if (!pw) { @@ -2611,16 +2597,12 @@ virGetGroupIDByName(const char *name, gid_t *gid) if (val < 0) strbuflen = 1024; - if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { - virReportOOMError(); + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) goto cleanup; - } while ((rc = getgrnam_r(name, &grbuf, strbuf, strbuflen, &gr)) == ERANGE) { - if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) { - virReportOOMError(); + if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) goto cleanup; - } } if (!gr) { @@ -2708,14 +2690,12 @@ virSetUIDGID(uid_t uid, gid_t gid) bufsize = 16384; if (VIR_ALLOC_N(buf, bufsize) < 0) { - virReportOOMError(); err = ENOMEM; goto error; } while ((rc = getpwuid_r(uid, &pwd, buf, bufsize, &pwd_result)) == ERANGE) { if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) < 0) { - virReportOOMError(); err = ENOMEM; goto error; } diff --git a/src/util/virxml.c b/src/util/virxml.c index aa55a33..c8ec805 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -609,7 +609,6 @@ virXPathNodeSet(const char *xpath, ret = obj->nodesetval->nodeNr; if (list != NULL && ret) { if (VIR_ALLOC_N(*list, ret) < 0) { - virReportOOMError(); ret = -1; } else { memcpy(*list, obj->nodesetval->nodeTab, -- 1.8.1.5
On 03/22/2013 07:44 AM, Michal Privoznik wrote:
Adapt code under src/util/ to fact, that VIR_ALLOC* now reports OOM error. There is no need to report it twice now. --- src/util/iohelper.c | 4 +--- src/util/virauthconfig.c | 8 ++------ src/util/vircommand.c | 13 +++---------- src/util/virconf.c | 10 ++-------- src/util/virdnsmasq.c | 19 +++++++------------ src/util/vireventpoll.c | 4 +--- src/util/virfile.c | 4 +--- src/util/virhash.c | 10 ++-------- src/util/virkeyfile.c | 4 +--- src/util/virlockspace.c | 11 +++-------- src/util/virnetdev.c | 4 +--- src/util/virnetdevbandwidth.c | 12 +++--------- src/util/virnetdevmacvlan.c | 4 ++-- src/util/virnetdevvlan.c | 4 +--- src/util/virnetdevvportprofile.c | 4 +--- src/util/virnetlink.c | 8 ++------ src/util/virobject.c | 9 ++++----- src/util/virpci.c | 13 +++---------- src/util/virprocess.c | 4 +--- src/util/virsexpr.c | 4 +--- src/util/virstoragefile.c | 24 ++++++------------------ src/util/virstring.c | 5 +++-- src/util/virsysinfo.c | 8 +++----- src/util/virthreadpool.c | 21 +++++---------------- src/util/virtime.c | 8 ++------ src/util/virtypedparam.c | 36 +++++++++--------------------------- src/util/viruri.c | 2 +- src/util/virusb.c | 8 ++------ src/util/virutil.c | 36 ++++++++---------------------------- src/util/virxml.c | 1 - 30 files changed, 81 insertions(+), 221 deletions(-)
diff --git a/src/util/virxml.c b/src/util/virxml.c index aa55a33..c8ec805 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -609,7 +609,6 @@ virXPathNodeSet(const char *xpath, ret = obj->nodesetval->nodeNr; if (list != NULL && ret) { if (VIR_ALLOC_N(*list, ret) < 0) { - virReportOOMError(); ret = -1; } else { memcpy(*list, obj->nodesetval->nodeTab,
Maybe an sed script could help here at least in removing the virReportOOMError, not so easily in removing the '{' and '}'. Now I doubt we will see lots of OOM errors, but there are a lot more that may now report twice ... ACK
On 22.03.2013 18:32, Stefan Berger wrote:
On 03/22/2013 07:44 AM, Michal Privoznik wrote:
Adapt code under src/util/ to fact, that VIR_ALLOC* now reports OOM error. There is no need to report it twice now. ---
Maybe an sed script could help here at least in removing the virReportOOMError, not so easily in removing the '{' and '}'. Now I doubt we will see lots of OOM errors, but there are a lot more that may now report twice ...
ACK
Yep. This is just a patch to show off how much we are going to save. But now that I am thinking this over again - maybe virAsprintf deserves to report OOM error as well (for virlog.c and virerror.c we are gonna need virAsprintf variant without OOM error reporting). And probably a wrapper over strdup as well - again with OOM error reporting. I just don't know if I should do all the adaptation in one patch patch set or in three different series. BTW: I am not that strong in RE, so I am doing this with vim macros. Michal
On Fri, Mar 22, 2013 at 12:44:55PM +0100, Michal Privoznik wrote:
Currently, our code is plenty of following scheme:
if (VIR_ALLOC(dummyPtr) < 0) { virReportOOMError(); goto cleanup; }
or something similar. What if we just move the OOM reporting into VIR_ALLOC? It would have three nice features:
1) sizeof(code base) gets lower. A lot lower.
Yep, makes sense.
2) even for callers which don't follow the schema described above, there is no harm reporting so serious error in the logs. No matter that the callee may fall back and return success.
I think I'd like some analysis of just how many places we have which call VIR_ALLOC, but don't want to report errors. Ought to be possible to grep it, or worst case a quick perl script to analyse thing At very least we need to be careful with virerror.c and virlog.c, since both of those allocate memory, and we don't want them to cause recursion here.
3) Removing virReportOOMError() from the schema does not need to be done at once, but can be split into several patches. In the worst case scenario - the error gets reported twice.
If we do this, we must convert everything before any release. Overwriting errors is something to be avoided, so I don't want us to make this worse. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/22/2013 07:44 AM, Michal Privoznik wrote:
Currently, our code is plenty of following scheme:
if (VIR_ALLOC(dummyPtr) < 0) { virReportOOMError(); goto cleanup; }
or something similar. What if we just move the OOM reporting into VIR_ALLOC?
One thing you might want to consider is the case where the OOM is caused by some bug in the code, for example incorrectly computing the size of a buffer to allocate, or a loop that keeps allocating again and again until failure. Currently we might get some useful information in the form of the function and line number where the error occurred. With your change that information would be lost. Likely the times when this would be helpful would be extremely rare, but just in case, you might want to think about having the VIR_ALLOC* macros send __FILE__, __FUNCTION__, and __LINE__ to virAlloc* to be passed on to a new variation of virReportOOMError() that takes those as args rather than filling them out inline.
participants (4)
-
Daniel P. Berrange -
Laine Stump -
Michal Privoznik -
Stefan Berger