[libvirt] [PATCH v1 01/37] viralloc: Report OOM error on failure
Peter Krempa
pkrempa at redhat.com
Thu Jul 4 14:16:12 UTC 2013
On 07/04/13 14:06, Michal Privoznik wrote:
> Similarly to VIR_STRDUP, we want the OOM error to be reported in
> VIR_ALLOC and friends.
> ---
> po/POTFILES.in | 1 +
> python/libvirt-override.c | 102 +++++++++----------
> src/conf/domain_conf.c | 9 +-
> src/conf/network_conf.c | 34 ++-----
> src/conf/node_device_conf.c | 2 +-
> src/esx/esx_vi.c | 2 +-
> src/lxc/lxc_process.c | 4 +-
> src/security/security_manager.c | 4 +-
> src/security/security_selinux.c | 4 +-
> src/util/viralloc.c | 165 ++++++++++++++++++++++++++-----
> src/util/viralloc.h | 213 ++++++++++++++++++++++++++++++++--------
> src/util/virbuffer.c | 8 +-
> src/util/vircgroup.c | 2 +-
> src/util/virerror.c | 4 +-
> src/util/virpci.c | 2 +-
> src/util/virthreadpthread.c | 2 +-
> tests/commandhelper.c | 1 +
> tests/networkxml2conftest.c | 2 +
> tests/test_conf.c | 2 +-
> tests/xmconfigtest.c | 2 +
> 20 files changed, 401 insertions(+), 164 deletions(-)
>
> diff --git a/python/libvirt-override.c b/python/libvirt-override.c
> index 5c5586d..6b6e77b 100644
> --- a/python/libvirt-override.c
> +++ b/python/libvirt-override.c
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 64fd581..d6799d2 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -3995,11 +3978,8 @@ virNetworkDefUpdateDNSTxt(virNetworkDefPtr def,
> /* add to beginning/end of list */
> if (VIR_INSERT_ELEMENT(dns->txts,
> command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST
> - ? 0 : dns->ntxts, dns->ntxts, txt) < 0) {
> - virReportOOMError();
> + ? 0 : dns->ntxts, dns->ntxts, txt, true) < 0)
Here and in multiple similar instances you remove the OOMError and set
the autoreporting to true ...
> goto cleanup;
> - }
> -
> } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) {
>
> if (foundIdx == dns->ntxts) {
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 3209604..7406652 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -1025,7 +1025,7 @@ virNodeDevCapPciDevIommuGroupParseXML(xmlXPathContextPtr ctxt,
> pciAddr->function = addr.function;
> if (VIR_APPEND_ELEMENT(data->pci_dev.iommuGroupDevices,
> data->pci_dev.nIommuGroupDevices,
> - pciAddr) < 0) {
> + pciAddr, false) < 0) {
but here you don't. You probably are going to clean this up later but
you could be consistent.
> virReportOOMError();
> goto cleanup;
> }
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index f7c5c2e..f5ea73a 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -487,10 +487,8 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
> /* The seclabel must be added to @vm prior calling domainGenSecurityLabel
> * which may require seclabel to be presented already */
> if (generated &&
> - VIR_APPEND_ELEMENT(vm->seclabels, vm->nseclabels, seclabel) < 0) {
> - virReportOOMError();
> + VIR_APPEND_ELEMENT(vm->seclabels, vm->nseclabels, seclabel, true) < 0)
and again using auto OOM error reporting
> goto cleanup;
> - }
>
> if (sec_managers[i]->drv->domainGenSecurityLabel(sec_managers[i], vm) < 0) {
> if (VIR_DELETE_ELEMENT(vm->seclabels,
> diff --git a/src/util/viralloc.c b/src/util/viralloc.c
> index 8d6a7e6..16afac7 100644
> --- a/src/util/viralloc.c
> +++ b/src/util/viralloc.c
...
>
> @@ -132,15 +150,28 @@ int virAlloc(void *ptrptr, size_t size)
> * @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.
> + * memory is filled with zeros. In case of OOM error
> + * and @report is true, the error is reported.
I'd write this as:
"If @report is true, OOM errors are reported automatically."
or something similar.
> *
> * Returns -1 on failure to allocate, zero on success
...
> @@ -160,16 +194,29 @@ int virAllocN(void *ptrptr, size_t size, size_t count)
> * @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.
> + * In case of OOM error and @report is true, the error is reported.
here too
> *
> * Returns -1 on failure to allocate, zero on success
> */
...
> @@ -194,24 +246,41 @@ int virReallocN(void *ptrptr, size_t size, size_t count)
> * @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.
> + * In case of OOM error and @report is true, the error is reported.
again
> *
> * Returns -1 on failure to allocate, zero on success
> */
...
> @@ -226,22 +295,38 @@ int virExpandN(void *ptrptr, size_t size, size_t *countptr, size_t add)
> * @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.
> + * allocated memory in 'ptrptr' is zero-filled. In case of OOM error
> + * and @report is true, the error is reported.
...
> @@ -301,7 +393,8 @@ 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.
> + * treated the same as *countptr for convenience. In case of OOM error
> + * and @report is true, the error is reported.
...
> @@ -405,11 +510,20 @@ 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.
> + * objects as structs containing flexible array members. In case of
> + * OOM error and @report is true, the error is reported.
> *
...
> diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> index 35d3a37..f0dba49 100644
> --- a/src/util/viralloc.h
> +++ b/src/util/viralloc.h
> -# define VIR_INSERT_ELEMENT(ptr, at, count, newelem) \
> +# define VIR_INSERT_ELEMENT(ptr, at, count, newelem, report) \
> virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \
> - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false)
> -# define VIR_INSERT_ELEMENT_COPY(ptr, at, count, newelem) \
> + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false, \
> + report, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)
> +# define VIR_INSERT_ELEMENT_COPY(ptr, at, count, newelem, report) \
> virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \
> - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false)
> -# define VIR_INSERT_ELEMENT_INPLACE(ptr, at, count, newelem) \
> + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false, \
> + report, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)
> +# define VIR_INSERT_ELEMENT_INPLACE(ptr, at, count, newelem, report) \
> virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \
> - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, true)
> -# define VIR_INSERT_ELEMENT_COPY_INPLACE(ptr, at, count, newelem) \
> + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, true, \
> + report, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)
> +# define VIR_INSERT_ELEMENT_COPY_INPLACE(ptr, at, count, newelem, report) \
> virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \
> - VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true)
> + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true, \
> + report, VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)
>
> /**
> * VIR_APPEND_ELEMENT:
> @@ -272,6 +379,7 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
> * @newelem: the new element to move into place (*not* a pointer to
> * the element, but the element itself).
> * (the original will be zeroed out if successful)
> + * @report: whether report OOM error on failure
I don't particularly like having a separate argument to enable error
reporting for these. On the other hand, these helpers aren't used widely
so this can be changed without a major refactor.
Also, is there a usage of these helpers, where it isn't desired to
report a error?
> *
> * Re-allocate an array of 'count' elements, each sizeof(*ptr) bytes
> * long, to be 'count' + 1 elements long, then copy the item from
....
> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> index e0b25ed..13fd162 100644
> --- a/src/util/vircgroup.c
> +++ b/src/util/vircgroup.c
> @@ -1171,7 +1171,7 @@ static int virCgroupPartitionEscape(char **path)
> if ((rc = virCgroupPartitionNeedsEscaping(*path)) <= 0)
> return rc;
>
> - if (VIR_INSERT_ELEMENT(*path, 0, len, escape) < 0)
> + if (VIR_INSERT_ELEMENT(*path, 0, len, escape, false) < 0)
Hmmm, right, there is a caller that doesn't want to report errors from
the array element helpers...
> return -ENOMEM;
>
> return 0;
....
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 54f7715..9ed257e 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -2009,7 +2009,7 @@ virPCIGetIOMMUGroupAddressesAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaq
> *copyAddr = *newDevAddr;
>
> if (VIR_APPEND_ELEMENT(*addrList->iommuGroupDevices,
> - *addrList->nIommuGroupDevices, copyAddr) < 0) {
> + *addrList->nIommuGroupDevices, copyAddr, false) < 0) {
> virReportOOMError();
again, this could be cleaned up right away.
> goto cleanup;
> }
Additionally, I think that you need to treat src/util/virlog.c with the
non-error-reporting functions right here to avoid possible infinite
loops that might happen only on OOM errors.
The rest of the patch looks fine and apart from the virlog.c issue I'd
ack this patch.
Peter
More information about the libvir-list
mailing list