I noticed when writing the backend functions for virNetworkUpdate that I was repeating the same sequence of memmove, VIR_REALLOC, nXXX-- (and messed up the args to memmove at least once), and had seen the same sequence in a lot of other places, so I decided to write a few utility functions/macros - see the .h file for full documentation. The intent is to reduce the number of lines of code, but more importantly to eliminate the need to check the element size and element count arithmetic every time we need to do this (I *always* make at least one mistake.) VIR_INSERT_ELEMENT: insert one element at an arbitrary index within an array of objects. The size of each object is determined automatically by the macro using sizeof(*array). If a pointer to a new element is provided, its contents are copied into the inserted space then the original contents are 0'ed out; if no newelem is provided the new space is set to all 0. Compile-time assignment and size compatibility between the array and the new element is guaranteed (see explanation below [*]) VIR_INSERT_ELEMENT_COPY: identical to VIR_INSERT_ELEMENT, except that the original contents of newelem are not cleared to 0 (i.e. a copy is made). VIR_APPEND_ELEMENT: This is just a special case of VIR_INSERT_ELEMENT that "inserts" one past the current last element. VIR_APPEND_ELEMENT_COPY: identical to VIR_APPEND_ELEMENT, except that the original contents of newelem are not cleared to 0 (i.e. a copy is made). VIR_DELETE_ELEMENT: delete one element at an arbitrary index within an array of objects. It's assumed that the element being deleted is already saved elsewhere (or cleared, if that's what is appropriate). All five of these macros have an _INPLACE variant, which skips the memory re-allocation of the array, assuming that the caller has already done it (when inserting) or will do it later (when deleting). Note that VIR_DELETE_ELEMENT* can return a failure, but only if an invalid index is given (index + amount to delete is > current array size), so in most cases you can safely ignore the return (that's why the helper function virDeleteElementsN isn't declared with ATTRIBUTE_RETURN_CHECK). [*] One initial problem with the INSERT and APPEND macros was that, due to both the array pointer and newelem pointer being cast to void* when passing to virInsertElementsN(), any chance of type-checking was lost. If we were going to move in newelem with a memmove anyway, we would be no worse off for this. However, most current open-coded insert/append operations use direct struct assignment to move the new element into place (or just populate the new element directly) - thus use of the new macros would open a possibility for new usage errors that didn't exist before (e.g. accidentally sending &newelemptr rather than newelemptr - I actually did this quite a lot in my test conversions of existing code). But thanks to Eric Blake's clever thinking, I was able to modify the INSERT and APPEND macros so that they *do* check for both assignment and size compatibility of *ptr (an element in the array) and newelem (the element being copied into the new position of the array). This is done via clever use of the C89-guaranteed fact that the sizeof() operator is must have *no* side effects (so an assignment inside sizeof() is checked for validity, but not actually evaluated), and the fact that virInsertElementsN has a "# of new elements" argument that we want to always be 1. What we do is replace the "1" in that argument with a call to VIR_TYPEMATCH(ptr, newelem), which returns 1 on success, and generates a compile error on failure. VIR_TYPEMATCH does three things: * sizeof(*(a) = *(b)) assures that *a and *b are assignment-compatible (they may still have a different size though! e.g. longVar = intVar) (If not, there is a compile-time error. If so, the result of that subexpression is sizeof(*(a)), i.e. one element of the array) * sizeof(*(a) = *(b)) == sizeof(*(b)) checks if *a and *b are also of the same size (so that, e.g. you don't accidentally copy an int plus the random bytes following it into an array of long). It evaluates to 1 if they are the same, and 0 otherwise. * sizeof(char[2 * (result of previous step) - 1]) evaluates to 1 if the previous step was successful (char [(2*1) - 1] ==> char[1]), of generates a compile error if it wasn't successful (char[2*0) -1] ==> char[-1], which isn't legal). So we end up sending "1" to the caller, and in the meantime check that we've actually added the correct &'s and/or *'s to the arguments. (Whew!) The result is that we've been relieved of the burden of getting the math right for the arguments to memmove when expanding/contracting the array, and haven't lost the type checking of ptr and newelem. --- cfg.mk | 2 +- src/libvirt_private.syms | 2 + src/util/memory.c | 106 +++++++++++++++++++++++++++++- src/util/memory.h | 167 ++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 274 insertions(+), 3 deletions(-) diff --git a/cfg.mk b/cfg.mk index c4ae195..f218eb6 100644 --- a/cfg.mk +++ b/cfg.mk @@ -314,7 +314,7 @@ sc_flags_usage: # Avoid functions that should only be called via macro counterparts. sc_prohibit_internal_functions: - @prohibit='vir(Free|AllocN?|ReallocN|File(Close|Fclose|Fdopen)) *\(' \ + @prohibit='vir(Free|AllocN?|ReallocN|(Insert|Delete)ElementsN|File(Close|Fclose|Fdopen)) *\(' \ halt='use VIR_ macros instead of internal functions' \ $(_sc_search_regexp) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 499ab3b..ad7dc17 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -819,8 +819,10 @@ virLogUnlock; virAlloc; virAllocN; virAllocVar; +virDeleteElementsN; virExpandN; virFree; +virInsertElementsN; virReallocN; virResizeN; virShrinkN; diff --git a/src/util/memory.c b/src/util/memory.c index 0f7aca1..40d612d 100644 --- a/src/util/memory.c +++ b/src/util/memory.c @@ -1,7 +1,7 @@ /* * memory.c: safer memory allocation * - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2012 Red Hat, Inc. * Copyright (C) 2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -253,6 +253,110 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) } } +/** + * virInsertElementsN: + * @ptrptr: pointer to hold address of allocated memory + * @size: the size of one element in bytes + * @at: index within array where new elements should be added + * @countptr: variable tracking number of elements currently allocated + * @add: number of elements to add + * @newelems: pointer to array of one or more new elements to move into + * place (the originals will be zeroed out if successful + * and if clearOriginal is true) + * @clearOriginal: false if the new item in the array should be copied + * from the original, and the original left intact. + * true if the original should be 0'd out on success. + * @inPlace: false if we should expand the allocated memory before + * moving, true if we should assume someone else *has + * already* done that. + * + * Re-allocate an array of 'count' elements, each sizeof(*ptr) bytes + * long, to be 'count' + 'add' elements long, then appropriately move + * the elements starting at ptr[at] up by 'count' elements, copy the + * items from 'newelems' into ptr[at], then store the address of + * allocated memory in 'ptr' and the new size in 'count'. If + * 'newelems' is NULL, the new elements at ptr[at] are instead filled + * with zero. + * + * Returns -1 on failure, 0 on success + */ +int +virInsertElementsN(void *ptrptr, size_t size, size_t at, + size_t *countptr, + size_t add, void *newelems, + bool clearOriginal, bool inPlace) +{ + if (at > *countptr) + return -1; + + if (inPlace) { + *countptr += add; + } else if (virExpandN(ptrptr, size, countptr, add) < 0) { + return -1; + } + + /* memory was successfully re-allocated. Move up all elements from + * ptrptr[at] to the end (if we're not "inserting" at the end + * already), memcpy in the new elements, and clear the elements + * from their original location. Remember that *countptr has + * already been updated with new element count! + */ + if (at < *countptr - add) { + memmove(*(char**)ptrptr + (size * (at + add)), + *(char**)ptrptr + (size * at), + size * (*countptr - add - at)); + } + + if (newelems) { + memcpy(*(char**)ptrptr + (size * at), newelems, size * add); + if (clearOriginal) + memset((char*)newelems, 0, size * add); + } else if (inPlace || (at < *countptr - add)) { + /* NB: if inPlace, even memory at the end wasn't initialized */ + memset(*(char**)ptrptr + (size * at), 0, size * add); + } + + return 0; +} + +/** + * virDeleteElementsN: + * @ptr: pointer to hold address of allocated memory + * @size: the size of one element in bytes + * @at: index within array where new elements should be deleted + * @count: variable tracking number of elements currently allocated + * @remove: number of elements to remove + * @inPlace: false if we should shrink the allocated memory when done, + * true if we should assume someone else will do that. + * + * Re-allocate an array of 'count' elements, each sizeof(*ptr) + * bytes long, to be 'count' - 'remove' elements long, then store the + * address of allocated memory in 'ptr' and the new size in 'count'. + * If 'count' <= 'remove', the entire array is freed. + * + * Returns -1 on failure, 0 on success + */ +int +virDeleteElementsN(void *ptrptr, size_t size, size_t at, + size_t *countptr, size_t remove, + bool inPlace) +{ + if (at + remove > *countptr) + return -1; + + /* First move down the elements at the end that won't be deleted, + * then realloc. We assume that the items being deleted have + * already been cleared. + */ + memmove(*(char**)ptrptr + (size * at), + *(char**)ptrptr + (size * (at + remove)), + size * (*countptr - remove - at)); + if (inPlace) + *countptr -= remove; + else + virShrinkN(ptrptr, size, countptr, remove); + return 0; +} /** * Vir_Alloc_Var: diff --git a/src/util/memory.h b/src/util/memory.h index ad8ee64..5a1c4df 100644 --- a/src/util/memory.h +++ b/src/util/memory.h @@ -1,7 +1,7 @@ /* * memory.c: safer memory allocation * - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2012 Red Hat, Inc. * Copyright (C) 2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -59,6 +59,13 @@ int virResizeN(void *ptrptr, size_t size, size_t *alloc, size_t count, ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); void virShrinkN(void *ptrptr, size_t size, size_t *count, size_t toremove) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); +int virInsertElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, + size_t add, void *newelem, + bool clearOriginal, bool inPlace) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); +int virDeleteElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, + size_t remove, bool inPlace) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, @@ -159,6 +166,164 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); virShrinkN(&(ptr), sizeof(*(ptr)), &(count), remove) /* + * VIR_TYPEMATCH: + * + * The following macro seems a bit cryptic, so it needs a thorough + * explanation. Its purpose is to check for assignment compatibility + * and identical size between two values without creating any side + * effects (by doing something silly like actually assigning one to + * the other). Note that it takes advantage of the C89-guaranteed + * property of sizeof() - it cannot have any side effects, so anything + * that happens inside sizeof() will not have any effect at runtime. + * + * VIR_TYPEMATCH evaluates to "1" if the two passed values are both + * assignment-compatible and the same size, and otherwise generates a + * compile-time error. It determines the result by performing the + * following three operations: + * + * * sizeof(*(a) = *(b)) assures that *a and *b are + * assignment-compatible (they may still have a different size + * though! e.g. longVar = intVar) (If not, there is a compile-time + * error. If so, the result of that subexpression is sizeof(*(a)), + * i.e. one element of the array) + * + * * sizeof(*(a) = *(b)) == sizeof(*(b)) checks if *a and *b are also + * of the same size (so that, e.g. you don't accidentally copy an + * int plus the random bytes following it into an array of long). It + * evaluates to 1 if they are the same, and 0 otherwise. + * + * * sizeof(char[2 * (result of previous step) - 1]) evaluates to 1 + * if the previous step was successful (char [(2*1) - 1] == * + * char[1]), of generates a compile error if it wasn't successful + * (char[2*0) -1] == * char[-1], which isn't legal and again + * generates a compile error). + * + * So VIR_TYPECHECK(a, b) will either abort the compile with an error, + * or evaluate to "1", and in the meantime check that we've actually + * added the correct &'s and/or *'s to the arguments. (Whew!) +*/ +# define VIR_TYPEMATCH(a, b) \ + sizeof(char[2 * (sizeof(*(a) = *(b)) == sizeof(*(b))) - 1]) + +/** + * VIR_INSERT_ELEMENT: + * @ptr: pointer to array of objects (*not* ptr to ptr) + * @at: index within array where new elements should be added + * @count: variable tracking number of elements currently allocated + * @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) + * + * Re-allocate an array of 'count' elements, each sizeof(*ptr) bytes + * long, to be 'count' + 1 elements long, then appropriately move + * the elements starting at ptr[at] up by 1 element, copy the + * item 'newelem' into ptr[at], then store the address of + * allocated memory in 'ptr' and the new size in 'count'. + * + * VIR_INSERT_ELEMENT_COPY is identical, but doesn't clear out the + * original element to 0 on success, so there are two copies of the + * element. This is useful if the "element" is actually just a + * pointer to the real data, and you want to maintain a reference to + * it for use after the insert is completed; but if the "element" is + * an object that points to other allocated memory, having multiple + * copies can cause problems (e.g. double free). + * + * VIR_INSERT_ELEMENT_*INPLACE are identical, but assume any necessary + * memory re-allocation has already been done. + * + * VIR_INSERT_ELEMENT_* all need to send "1" as the "add" argument to + * virInsertElementsN (which has the currently-unused capability of + * inserting multiple items at once). We use this to our advantage by + * replacing it with VIR_TYPECHECK(ptr, &newelem) so that we can be + * assured ptr and &newelem are of compatible types. + * + * Returns -1 on failure, 0 on success + * + * + */ +# define VIR_INSERT_ELEMENT(ptr, at, count, newelem) \ + virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ + 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) +# define VIR_INSERT_ELEMENT_INPLACE(ptr, at, count, newelem) \ + virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ + 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) + +/** + * VIR_APPEND_ELEMENT: + * @ptr: pointer to array of objects (*not* ptr to ptr) + * @count: variable tracking number of elements currently allocated + * @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) + * + * Re-allocate an array of 'count' elements, each sizeof(*ptr) bytes + * long, to be 'count' + 1 elements long, then copy the item from + * 'newelem' into ptr[count+1], and store the address of allocated + * memory in 'ptr' and the new size in 'count'. If 'newelem' is NULL, + * the new element at ptr[at] is instead filled with zero. + * + * VIR_APPEND_ELEMENT_COPY is identical, but doesn't clear out the + * original element to 0 on success, so there are two copies of the + * element. This is useful if the "element" is actually just a + * pointer to the real data, and you want to maintain a reference to + * it for use after the append is completed; but if the "element" is + * an object that points to other allocated memory, having multiple + * copies can cause problems (e.g. double free). + * + * VIR_APPEND_ELEMENT_*INPLACE are identical, but assume any + * necessary memory re-allocation has already been done. + * + * VIR_APPEND_ELEMENT_* all need to send "1" as the "add" argument to + * virInsertElementsN (which has the currently-unused capability of + * inserting multiple items at once). We use this to our advantage by + * replacing it with VIR_TYPECHECK(ptr, &newelem) so that we can be + * assured ptr and &newelem are of compatible types. + * + * Returns -1 on failure, 0 on success + * + * + */ +# define VIR_APPEND_ELEMENT(ptr, count, newelem) \ + virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count), \ + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false) +# define VIR_APPEND_ELEMENT_COPY(ptr, count, newelem) \ + virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count), \ + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false) +# define VIR_APPEND_ELEMENT_INPLACE(ptr, count, newelem) \ + virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count), \ + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, true) +# define VIR_APPEND_ELEMENT_COPY_INPLACE(ptr, count, newelem) \ + virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count), \ + VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true) + +/** + * VIR_DELETE_ELEMENT: + * @ptr: pointer to array of objects (*not* ptr to ptr) + * @at: index within array where new elements should be deleted + * @count: variable tracking number of elements currently allocated + * + * Re-allocate an array of 'count' elements, each sizeof(*ptr) + * bytes long, to be 'count' - 1 elements long, then store the + * address of allocated memory in 'ptr' and the new size in 'count'. + * If 'count' <= 1, the entire array is freed. + * + * VIR_DELETE_ELEMENT_INPLACE is identical, but assumes any + * necessary memory re-allocation will be done later. + * + * Returns -1 on failure, 0 on success + */ +# define VIR_DELETE_ELEMENT(ptr, at, count) \ + virDeleteElementsN(&(ptr), sizeof(*(ptr)), at, &(count), 1, false) +# define VIR_DELETE_ELEMENT_INPLACE(ptr, at, count) \ + virDeleteElementsN(&(ptr), sizeof(*(ptr)), at, &(count), 1, true) + +/* * VIR_ALLOC_VAR_OVERSIZED: * @M: size of base structure * @N: number of array elements in trailing array -- 1.7.11.7

The already-written backend functions for virNetworkUpdate that add and delete items into lists within the a network were already debugged to work properly, but future such functions will use VIR_(INSERT|DELETE)_ELEMENT instead, so these are changed for uniformity. --- src/conf/network_conf.c | 91 ++++++++++++++----------------------------------- src/conf/network_conf.h | 4 +-- 2 files changed, 27 insertions(+), 68 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 7220659..d20c7b5 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2495,27 +2495,15 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDefPtr def, goto cleanup; } } + /* add to beginning/end of list */ - if (VIR_REALLOC_N(ipdef->hosts, ipdef->nhosts +1) < 0) { + if (VIR_INSERT_ELEMENT(ipdef->hosts, + command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST + ? 0 : ipdef->nhosts, + ipdef->nhosts, host) < 0) { virReportOOMError(); goto cleanup; } - - if (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) { - - ipdef->hosts[ipdef->nhosts] = host; - ipdef->nhosts++; - memset(&host, 0, sizeof(host)); - - } else { /* implied (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) */ - - memmove(ipdef->hosts + 1, ipdef->hosts, - sizeof(*ipdef->hosts) * ipdef->nhosts); - ipdef->hosts[0] = host; - ipdef->nhosts++; - memset(&host, 0, sizeof(host)); - } - } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { if (virNetworkDHCPHostDefParse(def->name, ctxt->node, &host, false) < 0) @@ -2541,10 +2529,8 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDefPtr def, /* remove it */ virNetworkDHCPHostDefClear(&ipdef->hosts[ii]); - memmove(ipdef->hosts + ii, ipdef->hosts + ii + 1, - sizeof(*ipdef->hosts) * (ipdef->nhosts - ii - 1)); - ipdef->nhosts--; - ignore_value(VIR_REALLOC_N(ipdef->hosts, ipdef->nhosts)); + VIR_DELETE_ELEMENT(ipdef->hosts, ii, ipdef->nhosts); + } else { virNetworkDefUpdateUnknownCommand(command); goto cleanup; @@ -2615,21 +2601,14 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def, } /* add to beginning/end of list */ - if (VIR_REALLOC_N(ipdef->ranges, ipdef->nranges +1) < 0) { + if (VIR_INSERT_ELEMENT(ipdef->ranges, + command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST + ? 0 : ipdef->nranges, + ipdef->nranges, range) < 0) { virReportOOMError(); goto cleanup; } - if (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) { - ipdef->ranges[ipdef->nranges] = range; - } else { /* implied (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) */ - memmove(ipdef->ranges + 1, ipdef->ranges, - sizeof(*ipdef->ranges) * ipdef->nranges); - ipdef->ranges[0] = range; - } - ipdef->nranges++; - memset(&range, 0, sizeof(range)); - } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { if (ii == ipdef->nranges) { @@ -2641,10 +2620,8 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def, /* remove it */ /* NB: nothing to clear from a RangeDef that's being freed */ - memmove(ipdef->ranges + ii, ipdef->ranges + ii + 1, - sizeof(*ipdef->ranges) * (ipdef->nranges - ii - 1)); - ipdef->nranges--; - ignore_value(VIR_REALLOC_N(ipdef->ranges, ipdef->nranges)); + VIR_DELETE_ELEMENT(ipdef->ranges, ii, ipdef->nranges); + } else { virNetworkDefUpdateUnknownCommand(command); goto cleanup; @@ -2719,21 +2696,14 @@ virNetworkDefUpdateForwardInterface(virNetworkDefPtr def, } /* add to beginning/end of list */ - if (VIR_REALLOC_N(def->forwardIfs, def->nForwardIfs + 1) < 0) { + if (VIR_INSERT_ELEMENT(def->forwardIfs, + command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST + ? 0 : def->nForwardIfs, + def->nForwardIfs, iface) < 0) { virReportOOMError(); goto cleanup; } - if (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) { - def->forwardIfs[def->nForwardIfs] = iface; - } else { /* implied (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) */ - memmove(def->forwardIfs + 1, def->forwardIfs, - sizeof(*def->forwardIfs) * def->nForwardIfs); - def->forwardIfs[0] = iface; - } - def->nForwardIfs++; - memset(&iface, 0, sizeof(iface)); - } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { if (ii == def->nForwardIfs) { @@ -2757,10 +2727,8 @@ virNetworkDefUpdateForwardInterface(virNetworkDefPtr def, /* remove it */ virNetworkForwardIfDefClear(&def->forwardIfs[ii]); - memmove(def->forwardIfs + ii, def->forwardIfs + ii + 1, - sizeof(*def->forwardIfs) * (def->nForwardIfs - ii - 1)); - def->nForwardIfs--; - ignore_value(VIR_REALLOC_N(def->forwardIfs, def->nForwardIfs)); + VIR_DELETE_ELEMENT(def->forwardIfs, ii, def->nForwardIfs); + } else { virNetworkDefUpdateUnknownCommand(command); goto cleanup; @@ -2855,29 +2823,20 @@ virNetworkDefUpdatePortGroup(virNetworkDefPtr def, (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) { /* add to beginning/end of list */ - if (VIR_REALLOC_N(def->portGroups, def->nPortGroups +1) < 0) { + if (VIR_INSERT_ELEMENT(def->portGroups, + command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST + ? 0 : def->nPortGroups, + def->nPortGroups, portgroup) < 0) { virReportOOMError(); goto cleanup; } - if (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) { - def->portGroups[def->nPortGroups] = portgroup; - } else { /* implied (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) */ - memmove(def->portGroups + 1, def->portGroups, - sizeof(*def->portGroups) * def->nPortGroups); - def->portGroups[0] = portgroup; - } - def->nPortGroups++; - memset(&portgroup, 0, sizeof(portgroup)); - } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { /* remove it */ virPortGroupDefClear(&def->portGroups[foundName]); - memmove(def->portGroups + foundName, def->portGroups + foundName + 1, - sizeof(*def->portGroups) * (def->nPortGroups - foundName - 1)); - def->nPortGroups--; - ignore_value(VIR_REALLOC_N(def->portGroups, def->nPortGroups)); + VIR_DELETE_ELEMENT(def->portGroups, foundName, def->nPortGroups); + } else { virNetworkDefUpdateUnknownCommand(command); goto cleanup; diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 949b3d2..87d7746 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -129,10 +129,10 @@ struct _virNetworkIpDef { unsigned int prefix; /* ipv6 - only prefix allowed */ virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */ - unsigned int nranges; /* Zero or more dhcp ranges */ + size_t nranges; /* Zero or more dhcp ranges */ virNetworkDHCPRangeDefPtr ranges; - unsigned int nhosts; /* Zero or more dhcp hosts */ + size_t nhosts; /* Zero or more dhcp hosts */ virNetworkDHCPHostDefPtr hosts; char *tftproot; -- 1.7.11.7

On 12/10/2012 02:20 PM, Laine Stump wrote: Thankfully, there were Red Hat patches in 2010 and 2011 for this file (and I assume you did due diligence on the other files where you expanded copyright ranges to include more years than 2012). ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

--- src/conf/domain_event.c | 51 ++++++++----------------------------------------- src/qemu/qemu_domain.c | 6 ++---- 2 files changed, 10 insertions(+), 47 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index cf37308..3bae1c9 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -41,12 +41,12 @@ typedef virDomainMeta *virDomainMetaPtr; struct _virDomainEventCallbackList { unsigned int nextID; - unsigned int count; + size_t count; virDomainEventCallbackPtr *callbacks; }; struct _virDomainEventQueue { - unsigned int count; + size_t count; virDomainEventPtr *events; }; @@ -171,18 +171,7 @@ virDomainEventCallbackListRemove(virConnectPtr conn, (*freecb)(cbList->callbacks[i]->opaque); virObjectUnref(cbList->callbacks[i]->conn); VIR_FREE(cbList->callbacks[i]); - - if (i < (cbList->count - 1)) - memmove(cbList->callbacks + i, - cbList->callbacks + i + 1, - sizeof(*(cbList->callbacks)) * - (cbList->count - (i + 1))); - - if (VIR_REALLOC_N(cbList->callbacks, - cbList->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - cbList->count--; + VIR_DELETE_ELEMENT(cbList->callbacks, i, cbList->count); for (i = 0 ; i < cbList->count ; i++) { if (!cbList->callbacks[i]->deleted) @@ -221,18 +210,7 @@ virDomainEventCallbackListRemoveID(virConnectPtr conn, (*freecb)(cbList->callbacks[i]->opaque); virObjectUnref(cbList->callbacks[i]->conn); VIR_FREE(cbList->callbacks[i]); - - if (i < (cbList->count - 1)) - memmove(cbList->callbacks + i, - cbList->callbacks + i + 1, - sizeof(*(cbList->callbacks)) * - (cbList->count - (i + 1))); - - if (VIR_REALLOC_N(cbList->callbacks, - cbList->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - cbList->count--; + VIR_DELETE_ELEMENT(cbList->callbacks, i, cbList->count); for (i = 0 ; i < cbList->count ; i++) { if (!cbList->callbacks[i]->deleted) @@ -311,13 +289,7 @@ virDomainEventCallbackListPurgeMarked(virDomainEventCallbackListPtr cbList) (*freecb)(cbList->callbacks[i]->opaque); virObjectUnref(cbList->callbacks[i]->conn); VIR_FREE(cbList->callbacks[i]); - - if (i < (cbList->count - 1)) - memmove(cbList->callbacks + i, - cbList->callbacks + i + 1, - sizeof(*(cbList->callbacks)) * - (cbList->count - (i + 1))); - cbList->count--; + VIR_DELETE_ELEMENT_INPLACE(cbList->callbacks, i, cbList->count); i--; } } @@ -391,15 +363,11 @@ virDomainEventCallbackListAddID(virConnectPtr conn, event->dom->id = dom->id; } - /* Make space on list */ - if (VIR_REALLOC_N(cbList->callbacks, cbList->count + 1) < 0) + /* add event to end of list */ + if (VIR_APPEND_ELEMENT_COPY(cbList->callbacks, cbList->count, event) < 0) goto no_memory; virObjectRef(event->conn); - - cbList->callbacks[cbList->count] = event; - cbList->count++; - event->callbackID = cbList->nextID++; for (i = 0 ; i < cbList->count ; i++) { @@ -1192,14 +1160,11 @@ virDomainEventQueuePush(virDomainEventQueuePtr evtQueue, } /* Make space on queue */ - if (VIR_REALLOC_N(evtQueue->events, - evtQueue->count + 1) < 0) { + if (VIR_APPEND_ELEMENT(evtQueue->events, evtQueue->count, event) < 0) { virReportOOMError(); return -1; } - evtQueue->events[evtQueue->count] = event; - evtQueue->count++; return 0; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8d8cf02..189511d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1989,10 +1989,8 @@ qemuDomainCleanupRemove(virDomainObjPtr vm, for (i = 0; i < priv->ncleanupCallbacks; i++) { if (priv->cleanupCallbacks[i] == cb) { - memmove(priv->cleanupCallbacks + i, - priv->cleanupCallbacks + i + 1, - priv->ncleanupCallbacks - i - 1); - priv->ncleanupCallbacks--; + VIR_DELETE_ELEMENT_INPLACE(priv->cleanupCallbacks, + i, priv->ncleanupCallbacks); } } -- 1.7.11.7

The stock remove functions in domain_conf.c already use VIR_DELETE_ELEMENT. This does the same in a xen file. --- src/xen/xm_internal.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 9da2974..aac9ec5 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1477,12 +1477,7 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, dev->data.disk->dst && STREQ(def->disks[i]->dst, dev->data.disk->dst)) { virDomainDiskDefFree(def->disks[i]); - if (i < (def->ndisks - 1)) - memmove(def->disks + i, - def->disks + i + 1, - sizeof(*def->disks) * - (def->ndisks - (i + 1))); - def->ndisks--; + VIR_DELETE_ELEMENT(def->disks, i, def->ndisks); break; } } @@ -1494,12 +1489,7 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, for (i = 0 ; i < def->nnets ; i++) { if (!virMacAddrCmp(&def->nets[i]->mac, &dev->data.net->mac)) { virDomainNetDefFree(def->nets[i]); - if (i < (def->nnets - 1)) - memmove(def->nets + i, - def->nets + i + 1, - sizeof(*def->nets) * - (def->nnets - (i + 1))); - def->nnets--; + VIR_DELETE_ELEMENT(def->nets, i, def->nnets); break; } } -- 1.7.11.7

In both pci and usb cases, the count that held the size of the list was int so it had to be changed to size_t. --- src/util/hostusb.c | 26 +++++++------------------- src/util/pci.c | 18 +++--------------- 2 files changed, 10 insertions(+), 34 deletions(-) diff --git a/src/util/hostusb.c b/src/util/hostusb.c index 81a9f5a..88a7dfc 100644 --- a/src/util/hostusb.c +++ b/src/util/hostusb.c @@ -56,7 +56,7 @@ struct _usbDevice { }; struct _usbDeviceList { - unsigned int count; + size_t count; usbDevice **devs; }; @@ -426,13 +426,11 @@ usbDeviceListAdd(usbDeviceList *list, return -1; } - if (VIR_REALLOC_N(list->devs, list->count+1) < 0) { + if (VIR_APPEND_ELEMENT(list->devs, list->count, dev) < 0) { virReportOOMError(); return -1; } - list->devs[list->count++] = dev; - return 0; } @@ -461,22 +459,12 @@ usbDeviceListSteal(usbDeviceList *list, int i; for (i = 0; i < list->count; i++) { - if (list->devs[i]->bus != dev->bus || - list->devs[i]->dev != dev->dev) - continue; - - ret = list->devs[i]; - - if (i != list->count--) - memmove(&list->devs[i], - &list->devs[i+1], - sizeof(*list->devs) * (list->count - i)); - - if (VIR_REALLOC_N(list->devs, list->count) < 0) { - ; /* not fatal */ + if (list->devs[i]->bus == dev->bus && + list->devs[i]->dev == dev->dev) { + ret = list->devs[i]; + VIR_DELETE_ELEMENT(list->devs, i, list->count); + break; } - - break; } return ret; } diff --git a/src/util/pci.c b/src/util/pci.c index 5971764..ba6fe82 100644 --- a/src/util/pci.c +++ b/src/util/pci.c @@ -72,7 +72,7 @@ struct _pciDevice { }; struct _pciDeviceList { - unsigned count; + size_t count; pciDevice **devs; }; @@ -1561,13 +1561,11 @@ pciDeviceListAdd(pciDeviceList *list, return -1; } - if (VIR_REALLOC_N(list->devs, list->count+1) < 0) { + if (VIR_APPEND_ELEMENT(list->devs, list->count, dev) < 0) { virReportOOMError(); return -1; } - list->devs[list->count++] = dev; - return 0; } @@ -1599,17 +1597,7 @@ pciDeviceListStealIndex(pciDeviceList *list, return NULL; ret = list->devs[idx]; - - if (idx != --list->count) { - memmove(&list->devs[idx], - &list->devs[idx + 1], - sizeof(*list->devs) * (list->count - idx)); - } - - if (VIR_REALLOC_N(list->devs, list->count) < 0) { - ; /* not fatal */ - } - + VIR_DELETE_ELEMENT(list->devs, idx, list->count); return ret; } -- 1.7.11.7

--- src/rpc/virnetclient.c | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index bf2547c..0184998 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -923,25 +923,12 @@ void virNetClientRemoveStream(virNetClientPtr client, virNetClientLock(client); size_t i; for (i = 0 ; i < client->nstreams ; i++) { - if (client->streams[i] == st) + if (client->streams[i] == st) { + VIR_DELETE_ELEMENT(client->streams, i, client->nstreams); + virObjectUnref(st); break; + } } - if (i == client->nstreams) - goto cleanup; - - if (client->nstreams > 1) { - memmove(client->streams + i, - client->streams + i + 1, - sizeof(*client->streams) * - (client->nstreams - (i + 1))); - VIR_SHRINK_N(client->streams, client->nstreams, 1); - } else { - VIR_FREE(client->streams); - client->nstreams = 0; - } - virObjectUnref(st); - -cleanup: virNetClientUnlock(client); } -- 1.7.11.7

--- tests/qemumonitortestutils.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index b55d867..f27fc4b 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -135,15 +135,7 @@ static int qemuMonitorTestProcessCommandJSON(qemuMonitorTestPtr test, ret = qemuMonitorTestAddReponse(test, test->items[0]->response); qemuMonitorTestItemFree(test->items[0]); - if (test->nitems == 1) { - VIR_FREE(test->items); - test->nitems = 0; - } else { - memmove(test->items, - test->items + 1, - sizeof(test->items[0]) * (test->nitems - 1)); - VIR_SHRINK_N(test->items, test->nitems, 1); - } + VIR_DELETE_ELEMENT(test->items, 0, test->nitems); } cleanup: @@ -178,15 +170,7 @@ static int qemuMonitorTestProcessCommandText(qemuMonitorTestPtr test, ret = qemuMonitorTestAddReponse(test, test->items[0]->response); qemuMonitorTestItemFree(test->items[0]); - if (test->nitems == 1) { - VIR_FREE(test->items); - test->nitems = 0; - } else { - memmove(test->items, - test->items + 1, - sizeof(test->items[0]) * (test->nitems - 1)); - VIR_SHRINK_N(test->items, test->nitems, 1); - } + VIR_DELETE_ELEMENT(test->items, 0, test->nitems); } cleanup: @@ -405,11 +389,10 @@ qemuMonitorTestAddItem(qemuMonitorTestPtr test, goto no_memory; virMutexLock(&test->lock); - if (VIR_EXPAND_N(test->items, test->nitems, 1) < 0) { + if (VIR_APPEND_ELEMENT(test->items, test->nitems, item) < 0) { virMutexUnlock(&test->lock); goto no_memory; } - test->items[test->nitems - 1] = item; virMutexUnlock(&test->lock); -- 1.7.11.7

When ebtRulesAppend failed its realloc, it was free'ing the argv it had been send, but not the rules. The caller also didn't free the rules. This is kind of a lost cause, since it's a leak that only occurs after a failure to allocate memory, but the fix also gives the ebtRuleFree function a more accurate name (ebtRuleClear), and I was making a change to this code anyway, so I split out the bugfix for easy review. --- src/util/ebtables.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/util/ebtables.c b/src/util/ebtables.c index f1b2986..d3a8432 100644 --- a/src/util/ebtables.c +++ b/src/util/ebtables.c @@ -94,7 +94,7 @@ enum { }; static void -ebtRuleFree(ebtRule *rule) +ebtRuleClear(ebtRule *rule) { VIR_FREE(rule->rule); @@ -112,20 +112,13 @@ ebtRulesAppend(ebtRules *rules, const char **argv, int command_idx) { + ebtRule tmp = { .rule = rule, .argv = argv, .command_idx = command_idx }; + if (VIR_REALLOC_N(rules->rules, rules->nrules+1) < 0) { - int i = 0; - while (argv[i]) - VIR_FREE(argv[i++]); - VIR_FREE(argv); + ebtRuleClear(&tmp); return ENOMEM; } - - rules->rules[rules->nrules].rule = rule; - rules->rules[rules->nrules].argv = argv; - rules->rules[rules->nrules].command_idx = command_idx; - - rules->nrules++; - + rules->rules[rules->nrules++] = tmp; return 0; } @@ -142,7 +135,7 @@ ebtRulesRemove(ebtRules *rules, if (i >= rules->nrules) return EINVAL; - ebtRuleFree(&rules->rules[i]); + ebtRuleClear(&rules->rules[i]); memmove(&rules->rules[i], &rules->rules[i+1], @@ -163,7 +156,7 @@ ebtRulesFree(ebtRules *rules) if (rules->rules) { for (i = 0; i < rules->nrules; i++) - ebtRuleFree(&rules->rules[i]); + ebtRuleClear(&rules->rules[i]); VIR_FREE(rules->rules); -- 1.7.11.7

--- src/util/virlockspace.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 99b6182..dc156f8 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -223,11 +223,9 @@ virLockSpaceResourceNew(virLockSpacePtr lockspace, } res->lockHeld = true; - if (VIR_EXPAND_N(res->owners, res->nOwners, 1) < 0) + if (VIR_APPEND_ELEMENT(res->owners, res->nOwners, owner) < 0) goto no_memory; - res->owners[res->nOwners-1] = owner; - return res; no_memory: @@ -643,12 +641,10 @@ 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) { + if (VIR_APPEND_ELEMENT(res->owners, res->nOwners, owner) < 0) { virReportOOMError(); goto cleanup; } - res->owners[res->nOwners-1] = owner; - goto done; } virReportError(VIR_ERR_RESOURCE_BUSY, @@ -707,11 +703,7 @@ int virLockSpaceReleaseResource(virLockSpacePtr lockspace, goto cleanup; } - if (i < (res->nOwners - 1)) - memmove(res->owners + i, - res->owners + i + 1, - (res->nOwners - i - 1) * sizeof(res->owners[0])); - VIR_SHRINK_N(res->owners, res->nOwners, 1); + VIR_DELETE_ELEMENT(res->owners, i, res->nOwners); if ((res->nOwners == 0) && virHashRemoveEntry(lockspace->resources, resname) < 0) @@ -753,11 +745,7 @@ virLockSpaceRemoveResourcesForOwner(const void *payload, data->count++; - if (i < (res->nOwners - 1)) - memmove(res->owners + i, - res->owners + i + 1, - (res->nOwners - i - 1) * sizeof(res->owners[0])); - VIR_SHRINK_N(res->owners, res->nOwners, 1); + VIR_DELETE_ELEMENT(res->owners, i, res->nOwners); if (res->nOwners) { VIR_DEBUG("Other shared owners remain"); -- 1.7.11.7