[libvirt] [PATCHv3 00/17] new VIR_(APPEND|INSERT|DELETE)_ELEMENT macros
DO NOT BE SCARED OFF BY THE PATCH COUNT!! Most of these are very short, and they all follow the same pattern; beyond that, 01/17 and 02/17 are the only ones that are really time-sensitive. The first patch in this series adds new macros to insert/append/delete elements from the arrays of variously-sized structs that are commonly used in libvirt. The intent is to eliminate the arithmetic mistakes associated with calling memmove() directly, and shorten application code. A full description of the added macros and their functions is included in 01/17. Patches 02..17 convert various bits of hand-coded array manipulation in libvirt to use the new macros. They can be taken now, later, or never (although I'd like to have at least 02/17 taken now, since other similar functions will be added in an upcoming patch series, and I want them to all use the same logic). Mostly I went through all of those conversions to see just how useful/usable the new macros were (and they led to a couple of rewrites). I will be posting another series momentarily that has a dependency on 01/17.
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 passin 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 POSIX-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 | 102 +++++++++++++++++++++++++++++++++++++++- src/util/memory.h | 118 ++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 221 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..d6d86ae 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,106 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) } } +/** + * virInsertElementsN: + * @ptrptr: pointer to hold address of allocated memory + * @at: index within array where new elements should be added + * @countptr: variable tracking number of elements currently allocated + * @add: number of elements to add + * @newelem: 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 'newelem' into ptr[at], then store the address of + * allocated memory in 'ptr' and the new size in 'count'. If + * 'newelem' 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 *newelem, + bool clearOriginal, bool inPlace) +{ + if (at > *countptr) + return -1; + + if (inPlace) { + (*countptr)++; + } 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 (newelem) { + memmove(*(char**)ptrptr + (size * at), newelem, size * add); + if (clearOriginal) + memset((char*)newelem, 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 + * @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)--; + 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..8388de5 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, @@ -158,6 +165,115 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); # define VIR_SHRINK_N(ptr, count, remove) \ virShrinkN(&(ptr), sizeof(*(ptr)), &(count), remove) +/** + * 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: ptr to new element to move into place (*not* 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 from 'newelem' into ptr[at], then 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_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. + * + * Returns -1 on failure, 0 on success + * + * + */ +# define VIR_TYPEMATCH(a, b) \ + sizeof(char[2 * (sizeof(*(a) = *(b)) == sizeof(*(b))) - 1]) + +# 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: ptr to new element to move into place (*not* 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. + * + * 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 -- 1.7.11.7
On 12/07/2012 11:16 AM, Laine Stump wrote:
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.
[*] 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 passin to virInsertElementsN(), any chance of type-checking was
s/passin/passing/
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 POSIX-guaranteed fact that the sizeof()
Actually guaranteed by C89, which is even more fundamental than POSIX (Microsoft obeys the C standard but not POSIX).
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.
It looks like you also set things up for a future patch to add VIR_INSERT_N_ELEMENTS (or some such name) where you can splice in an array rather than just one element at a time; there, we could use the identity property that '1 * N' is still N for the user request of how many elements to insert at once.
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:
It might be nice to (additionally?) mention these three things as a comment in the source code, so that someone reading it later knows why the macro is even present, without having to look at git history.
* 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!)
Yep, I take full responsibility for that black magic :)
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.
@@ -253,6 +253,106 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) } }
+/** + * virInsertElementsN: + * @ptrptr: pointer to hold address of allocated memory
Missing: @size: size of one element of @ptrptr or @newelem
+ * @at: index within array where new elements should be added + * @countptr: variable tracking number of elements currently allocated + * @add: number of elements to add
All of your existing macros pass '1' to this, but I think it is nice to leave this in so that we can add future macros that pass in multiple elements at once.
+ * @newelem: 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)
Indentation is not consistent - here you are flush...
+ * @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.
...but here, you offset second lines. Either one works, but it looks nicer to be consistent (I think the offset looks slightly nicer, to quickly pick out the @name: entries)
+ * @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 'newelem' into ptr[at], then store the address of + * allocated memory in 'ptr' and the new size in 'count'. If + * 'newelem' 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 *newelem, + bool clearOriginal, bool inPlace) +{ + if (at > *countptr) + return -1; + + if (inPlace) { + (*countptr)++;
Oops. Should be: *countptr += add; (it worked only because you always pass add==1 for now)
+ } else if (virExpandN(ptrptr, size, countptr, add) < 0) { + return -1; + }
Hmm, this can fail both for invalid usage (at > *countptr) and for OOM (virExpandN failed), but we don't issue the error message. On the other hand, callers should debug things to avoid invalid usage, so it should be okay if they blindly call virReportOOMError() on failure.
+ + /* 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 (newelem) { + memmove(*(char**)ptrptr + (size * at), newelem, size * add);
This could technically be memcpy, as ptrptr and newlem should not overlap, for a potential minor speedup.
+ if (clearOriginal) + memset((char*)newelem, 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
Again, document @size.
+ * @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)--;
Oops, same bug again where you got lucky that remove==1; should be: *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..8388de5 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, @@ -158,6 +165,115 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); # define VIR_SHRINK_N(ptr, count, remove) \ virShrinkN(&(ptr), sizeof(*(ptr)), &(count), remove)
spot [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: ptr to new element to move into place (*not* 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 from 'newelem' into ptr[at], then 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_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. + * + * Returns -1 on failure, 0 on success + * + * + */ +# define VIR_TYPEMATCH(a, b) \ + sizeof(char[2 * (sizeof(*(a) = *(b)) == sizeof(*(b))) - 1])
I would move this helper array up to spot [1] above, along with documenting that it is either the compile-time constant '1' or a compile error due to improper code (that is, the summary of how the three sizeof() work, as mentioned in the commit message, might be worth listing here). Ouch - we have a problem with your stated goal of allowing newelem to be passed as NULL instead of &var when it is just desirable to leave space but not actually populate it. sizeof(*(NULL)), aka sizeof(*((void*)0)), is a compilation error. That implies that none of the rest of your series ever directly passed NULL, but always passed &orig. I'm not sure whether the goal of allowing us to pass NULL makes sense if we don't have a client for it; if it does make sense, then we need to enhance VIR_TYPEMATCH (perhaps by using gcc-specific magic in order to ensure we don't ever evaluate *NULL); if it doesn't make sense, it might be simpler to pass 'orig' instead of '&orig' (but that means touching all callers in your series).
+ +# 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)
These look okay, unless my concern about NULL newelem forces us to rewrite slightly.
+ +/** + * VIR_APPEND_ELEMENT: ... Looks okay, as nice shorthand for the common insert-at-end case.
+/** + * 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)
Looks good. And here, we don't have to worry about type-matching. I think I found enough logic flaws, plus my concern about NULL newelem, to need a v4. But I'll still need some time to figure out how to rework the type-checking macro to work with a NULL newelem. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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..777b89f 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
The volume object list is also directly manipulated in the storage driver, test storage driver, and parallels storage driver (this should probably be consolidated). --- src/conf/interface_conf.c | 13 ++----------- src/conf/interface_conf.h | 4 ++-- src/conf/network_conf.c | 11 +---------- src/conf/network_conf.h | 2 +- src/conf/node_device_conf.c | 11 +---------- src/conf/node_device_conf.h | 4 ++-- src/conf/nwfilter_conf.c | 11 +---------- src/conf/nwfilter_conf.h | 2 +- src/conf/storage_conf.c | 11 +---------- src/conf/storage_conf.h | 4 ++-- src/parallels/parallels_storage.c | 14 +------------- src/storage/storage_backend_rbd.c | 2 +- src/storage/storage_driver.c | 12 +----------- src/test/test_driver.c | 14 +------------- 14 files changed, 18 insertions(+), 97 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index fc3602a..078f2ca 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -1,7 +1,7 @@ /* * interface_conf.c: interfaces XML handling * - * Copyright (C) 2006-2010 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -1313,16 +1313,7 @@ void virInterfaceRemove(virInterfaceObjListPtr interfaces, if (interfaces->objs[i] == iface) { virInterfaceObjUnlock(interfaces->objs[i]); virInterfaceObjFree(interfaces->objs[i]); - - if (i < (interfaces->count - 1)) - memmove(interfaces->objs + i, interfaces->objs + i + 1, - sizeof(*(interfaces->objs)) * (interfaces->count - (i + 1))); - - if (VIR_REALLOC_N(interfaces->objs, interfaces->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - interfaces->count--; - + VIR_DELETE_ELEMENT(interfaces->objs, i, interfaces->count); break; } virInterfaceObjUnlock(interfaces->objs[i]); diff --git a/src/conf/interface_conf.h b/src/conf/interface_conf.h index 1749629..bcfca62 100644 --- a/src/conf/interface_conf.h +++ b/src/conf/interface_conf.h @@ -1,7 +1,7 @@ /* * interface_conf.h: interface XML handling entry points * - * Copyright (C) 2006-2009 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -171,7 +171,7 @@ struct _virInterfaceObj { typedef struct _virInterfaceObjList virInterfaceObjList; typedef virInterfaceObjList *virInterfaceObjListPtr; struct _virInterfaceObjList { - unsigned int count; + size_t count; virInterfaceObjPtr *objs; }; diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 777b89f..c36bdd3 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -495,16 +495,7 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, if (nets->objs[i] == net) { virNetworkObjUnlock(nets->objs[i]); virNetworkObjFree(nets->objs[i]); - - if (i < (nets->count - 1)) - memmove(nets->objs + i, nets->objs + i + 1, - sizeof(*(nets->objs)) * (nets->count - (i + 1))); - - if (VIR_REALLOC_N(nets->objs, nets->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - nets->count--; - + VIR_DELETE_ELEMENT(nets->objs, i, nets->count); break; } virNetworkObjUnlock(nets->objs[i]); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 87d7746..d4dc214 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -231,7 +231,7 @@ struct _virNetworkObj { typedef struct _virNetworkObjList virNetworkObjList; typedef virNetworkObjList *virNetworkObjListPtr; struct _virNetworkObjList { - unsigned int count; + size_t count; virNetworkObjPtr *objs; }; diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 41fa8e4..66a446c 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -218,16 +218,7 @@ void virNodeDeviceObjRemove(virNodeDeviceObjListPtr devs, if (devs->objs[i] == dev) { virNodeDeviceObjUnlock(dev); virNodeDeviceObjFree(devs->objs[i]); - - if (i < (devs->count - 1)) - memmove(devs->objs + i, devs->objs + i + 1, - sizeof(*(devs->objs)) * (devs->count - (i + 1))); - - if (VIR_REALLOC_N(devs->objs, devs->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - devs->count--; - + VIR_DELETE_ELEMENT(devs->objs, i, devs->count); break; } virNodeDeviceObjUnlock(dev); diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index e394042..ee02991 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -1,7 +1,7 @@ /* * node_device_conf.h: config handling for node devices * - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2012 Red Hat, Inc. * Copyright (C) 2008 Virtual Iron Software, Inc. * Copyright (C) 2008 David F. Lively * @@ -199,7 +199,7 @@ struct _virNodeDeviceObj { typedef struct _virNodeDeviceObjList virNodeDeviceObjList; typedef virNodeDeviceObjList *virNodeDeviceObjListPtr; struct _virNodeDeviceObjList { - unsigned int count; + size_t count; virNodeDeviceObjPtr *objs; }; diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 46580e9..2d32bd3 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -426,16 +426,7 @@ virNWFilterObjRemove(virNWFilterObjListPtr nwfilters, if (nwfilters->objs[i] == nwfilter) { virNWFilterObjUnlock(nwfilters->objs[i]); virNWFilterObjFree(nwfilters->objs[i]); - - if (i < (nwfilters->count - 1)) - memmove(nwfilters->objs + i, nwfilters->objs + i + 1, - sizeof(*(nwfilters->objs)) * (nwfilters->count - (i + 1))); - - if (VIR_REALLOC_N(nwfilters->objs, nwfilters->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - nwfilters->count--; - + VIR_DELETE_ELEMENT(nwfilters->objs, i, nwfilters->count); break; } virNWFilterObjUnlock(nwfilters->objs[i]); diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 805fbe7..4a2a9f3 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -547,7 +547,7 @@ struct _virNWFilterObj { typedef struct _virNWFilterObjList virNWFilterObjList; typedef virNWFilterObjList *virNWFilterObjListPtr; struct _virNWFilterObjList { - unsigned int count; + size_t count; virNWFilterObjPtr *objs; }; diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ad0f0c1..164eea0 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -402,16 +402,7 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, if (pools->objs[i] == pool) { virStoragePoolObjUnlock(pools->objs[i]); virStoragePoolObjFree(pools->objs[i]); - - if (i < (pools->count - 1)) - memmove(pools->objs + i, pools->objs + i + 1, - sizeof(*(pools->objs)) * (pools->count - (i + 1))); - - if (VIR_REALLOC_N(pools->objs, pools->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - pools->count--; - + VIR_DELETE_ELEMENT(pools->objs, i, pools->count); break; } virStoragePoolObjUnlock(pools->objs[i]); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 743b768..e42e4c3 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -116,7 +116,7 @@ struct _virStorageVolDef { typedef struct _virStorageVolDefList virStorageVolDefList; typedef virStorageVolDefList *virStorageVolDefListPtr; struct _virStorageVolDefList { - unsigned int count; + size_t count; virStorageVolDefPtr *objs; }; @@ -319,7 +319,7 @@ struct _virStoragePoolObj { typedef struct _virStoragePoolObjList virStoragePoolObjList; typedef virStoragePoolObjList *virStoragePoolObjListPtr; struct _virStoragePoolObjList { - unsigned int count; + size_t count; virStoragePoolObjPtr *objs; }; diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index 9075dfd..2b5b9ba 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -1187,19 +1187,7 @@ parallelsStorageVolumeDelete(virStorageVolPtr vol, unsigned int flags) } virStorageVolDefFree(privvol); - - if (i < (privpool->volumes.count - 1)) - memmove(privpool->volumes.objs + i, - privpool->volumes.objs + i + 1, - sizeof(*(privpool->volumes.objs)) * - (privpool->volumes.count - (i + 1))); - - if (VIR_REALLOC_N(privpool->volumes.objs, - privpool->volumes.count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - privpool->volumes.count--; - + VIR_DELETE_ELEMENT(privpool->volumes.objs, i, privpool->volumes.count); break; } } diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index bc61cf7..b9212fe 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -343,7 +343,7 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, pool->volumes.objs[pool->volumes.count++] = vol; } - VIR_DEBUG("Found %d images in RBD pool %s", + VIR_DEBUG("Found %zu images in RBD pool %s", pool->volumes.count, pool->def->source.name); ret = 0; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c567fff..d7a71e8 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2177,17 +2177,7 @@ storageVolumeDelete(virStorageVolPtr obj, VIR_INFO("Deleting volume '%s' from storage pool '%s'", vol->name, pool->def->name); virStorageVolDefFree(vol); - vol = NULL; - - if (i < (pool->volumes.count - 1)) - memmove(pool->volumes.objs + i, pool->volumes.objs + i + 1, - sizeof(*(pool->volumes.objs)) * (pool->volumes.count - (i + 1))); - - if (VIR_REALLOC_N(pool->volumes.objs, pool->volumes.count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - pool->volumes.count--; - + VIR_DELETE_ELEMENT(pool->volumes.objs, i, pool->volumes.count); break; } } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6ca59e2..a33841f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5035,19 +5035,7 @@ testStorageVolumeDelete(virStorageVolPtr vol, for (i = 0 ; i < privpool->volumes.count ; i++) { if (privpool->volumes.objs[i] == privvol) { virStorageVolDefFree(privvol); - - if (i < (privpool->volumes.count - 1)) - memmove(privpool->volumes.objs + i, - privpool->volumes.objs + i + 1, - sizeof(*(privpool->volumes.objs)) * - (privpool->volumes.count - (i + 1))); - - if (VIR_REALLOC_N(privpool->volumes.objs, - privpool->volumes.count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - privpool->volumes.count--; - + VIR_DELETE_ELEMENT(privpool->volumes.objs, i, privpool->volumes.count); break; } } -- 1.7.11.7
The convenience macros replace manual alloc/movement except in cases where the list has been pre-allocated to the necessary size, *and* a new item is just being appended - in that case it's just as concise (and arguably easier to interpret) to leave it as is - a simple direct assignment ("list[count++] = item"). Note that when the pointer being appended to the array will continue to be used by its original name after the append is done, we must use VIR_APPEND_ELEMENT_COPY, so that the original pointer isn't cleared. This was only necessary in one case (virDomainNetInsert(), since the same pointer may also be sent to virDomainHostdevInsert()) --- src/conf/domain_conf.c | 105 +++++++------------------------------------------ 1 file changed, 14 insertions(+), 91 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6aa5f79..5940a04 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7840,9 +7840,8 @@ virDomainChrTargetTypeToString(int deviceType, int virDomainHostdevInsert(virDomainDefPtr def, virDomainHostdevDefPtr hostdev) { - if (VIR_REALLOC_N(def->hostdevs, def->nhostdevs + 1) < 0) + if (VIR_APPEND_ELEMENT(def->hostdevs, def->nhostdevs, &hostdev) < 0) return -1; - def->hostdevs[def->nhostdevs++] = hostdev; return 0; } @@ -7851,19 +7850,7 @@ virDomainHostdevRemove(virDomainDefPtr def, size_t i) { virDomainHostdevDefPtr hostdev = def->hostdevs[i]; - if (def->nhostdevs > 1) { - memmove(def->hostdevs + i, - def->hostdevs + i + 1, - sizeof(*def->hostdevs) * - (def->nhostdevs - (i + 1))); - def->nhostdevs--; - if (VIR_REALLOC_N(def->hostdevs, def->nhostdevs) < 0) { - /* ignore, harmless */ - } - } else { - VIR_FREE(def->hostdevs); - def->nhostdevs = 0; - } + VIR_DELETE_ELEMENT(def->hostdevs, i, def->nhostdevs); return hostdev; } @@ -8018,13 +8005,8 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def, if (insertAt == -1) insertAt = def->ndisks; - if (insertAt < def->ndisks) - memmove(def->disks + insertAt + 1, - def->disks + insertAt, - (sizeof(def->disks[0]) * (def->ndisks-insertAt))); - - def->disks[insertAt] = disk; - def->ndisks++; + ignore_value(VIR_INSERT_ELEMENT_INPLACE(def->disks, insertAt, + def->ndisks, &disk)); } @@ -8033,19 +8015,7 @@ virDomainDiskRemove(virDomainDefPtr def, size_t i) { virDomainDiskDefPtr disk = def->disks[i]; - if (def->ndisks > 1) { - memmove(def->disks + i, - def->disks + i + 1, - sizeof(*def->disks) * - (def->ndisks - (i + 1))); - def->ndisks--; - if (VIR_REALLOC_N(def->disks, def->ndisks) < 0) { - /* ignore, harmless */ - } - } else { - VIR_FREE(def->disks); - def->ndisks = 0; - } + VIR_DELETE_ELEMENT(def->disks, i, def->ndisks); return disk; } @@ -8072,10 +8042,8 @@ virDomainHasDiskMirror(virDomainObjPtr vm) int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net) { - if (VIR_REALLOC_N(def->nets, def->nnets + 1) < 0) + if (VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, &net) < 0) return -1; - def->nets[def->nnets] = net; - def->nnets++; if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV) { /* hostdev net devices must also exist in the hostdevs array */ return virDomainHostdevInsert(def, &net->data.hostdev.def); @@ -8148,18 +8116,7 @@ virDomainNetRemove(virDomainDefPtr def, size_t i) } } } - if (def->nnets > 1) { - memmove(def->nets + i, - def->nets + i + 1, - sizeof(*def->nets) * (def->nnets - (i + 1))); - def->nnets--; - if (VIR_REALLOC_N(def->nets, def->nnets) < 0) { - /* ignore harmless */ - } - } else { - VIR_FREE(def->nets); - def->nnets = 0; - } + VIR_DELETE_ELEMENT(def->nets, i, def->nnets); return net; } @@ -8206,13 +8163,8 @@ void virDomainControllerInsertPreAlloced(virDomainDefPtr def, if (insertAt == -1) insertAt = def->ncontrollers; - if (insertAt < def->ncontrollers) - memmove(def->controllers + insertAt + 1, - def->controllers + insertAt, - (sizeof(def->controllers[0]) * (def->ncontrollers-insertAt))); - - def->controllers[insertAt] = controller; - def->ncontrollers++; + ignore_value(VIR_INSERT_ELEMENT_INPLACE(def->controllers, insertAt, + def->ncontrollers, &controller)); } int @@ -8236,20 +8188,7 @@ virDomainControllerRemove(virDomainDefPtr def, size_t i) { virDomainControllerDefPtr controller = def->controllers[i]; - if (def->ncontrollers > 1) { - memmove(def->controllers + i, - def->controllers + i + 1, - sizeof(*def->controllers) * - (def->ncontrollers - (i + 1))); - def->ncontrollers--; - if (VIR_REALLOC_N(def->controllers, def->ncontrollers) < 0) { - /* ignore, harmless */ - } - } else { - VIR_FREE(def->controllers); - def->ncontrollers = 0; - } - + VIR_DELETE_ELEMENT(def->controllers, i, def->ncontrollers); return controller; } @@ -8311,16 +8250,7 @@ virDomainLeaseRemoveAt(virDomainDefPtr def, size_t i) virDomainLeaseDefPtr lease = def->leases[i]; - if (def->nleases > 1) { - memmove(def->leases + i, - def->leases + i + 1, - sizeof(*def->leases) * - (def->nleases - (i + 1))); - VIR_SHRINK_N(def->leases, def->nleases, 1); - } else { - VIR_FREE(def->leases); - def->nleases = 0; - } + VIR_DELETE_ELEMENT(def->leases, i, def->nleases); return lease; } @@ -8607,15 +8537,11 @@ static int virDomainDefMaybeAddController(virDomainDefPtr def, cont->opts.vioserial.vectors = -1; } - - if (VIR_REALLOC_N(def->controllers, def->ncontrollers+1) < 0) { + if (VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, &cont) < 0) { VIR_FREE(cont); virReportOOMError(); return -1; } - def->controllers[def->ncontrollers] = cont; - def->ncontrollers++; - return 0; } @@ -9923,12 +9849,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, input->bus = VIR_DOMAIN_INPUT_BUS_XEN; } - if (VIR_REALLOC_N(def->inputs, def->ninputs + 1) < 0) { + if (VIR_APPEND_ELEMENT(def->inputs, def->ninputs, &input) < 0) { virDomainInputDefFree(input); goto no_memory; } - def->inputs[def->ninputs] = input; - def->ninputs++; } @@ -9980,11 +9904,10 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, } video->vram = virDomainVideoDefaultRAM(def, video->type); video->heads = 1; - if (VIR_ALLOC_N(def->videos, 1) < 0) { + if (VIR_APPEND_ELEMENT(def->videos, def->nvideos, &video) < 0) { virDomainVideoDefFree(video); goto no_memory; } - def->videos[def->nvideos++] = video; } /* analysis of the host devices */ -- 1.7.11.7
--- 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..5e18fa5 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
nvcpupin was declared as int, as were the nvcpupin args in several functions. These were all changed to size_t. virDomainVcpuPinAdd() was leaking a cpumask if VIR_REALLOC_N failed. This is completely inconsequential (since the only reason for failure was OOM, so we would already be dead at this time anyway), but I fixed the omission just to be tidy. --- src/conf/domain_conf.c | 47 ++++++++++++++--------------------------------- src/conf/domain_conf.h | 12 ++++++------ src/qemu/qemu_cgroup.c | 2 +- src/qemu/qemu_cgroup.h | 2 +- src/qemu/qemu_driver.c | 4 ++-- 5 files changed, 24 insertions(+), 43 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5940a04..31b35f9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1553,7 +1553,7 @@ virDomainClockDefClear(virDomainClockDefPtr def) } virDomainVcpuPinDefPtr * -virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, int nvcpupin) +virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, size_t nvcpupin) { int i = 0; virDomainVcpuPinDefPtr *ret = NULL; @@ -1598,9 +1598,9 @@ virDomainVcpuPinDefFree(virDomainVcpuPinDefPtr def) void virDomainVcpuPinDefArrayFree(virDomainVcpuPinDefPtr *def, - int nvcpupin) + size_t nvcpupin) { - int i; + size_t i; if (!def || !nvcpupin) return; @@ -11532,7 +11532,7 @@ int virDomainDefAddImplicitControllers(virDomainDefPtr def) * Return 1 if exists, 0 if not. */ int virDomainVcpuPinIsDuplicate(virDomainVcpuPinDefPtr *def, - int nvcpupin, + size_t nvcpupin, int vcpu) { int i; @@ -11550,7 +11550,7 @@ virDomainVcpuPinIsDuplicate(virDomainVcpuPinDefPtr *def, virDomainVcpuPinDefPtr virDomainVcpuPinFindByVcpu(virDomainVcpuPinDefPtr *def, - int nvcpupin, + size_t nvcpupin, int vcpu) { int i; @@ -11567,7 +11567,7 @@ virDomainVcpuPinFindByVcpu(virDomainVcpuPinDefPtr *def, } int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, - int *nvcpupin, + size_t *nvcpupin, unsigned char *cpumap, int maplen, int vcpu) @@ -11605,14 +11605,13 @@ int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, return -1; } - if (VIR_REALLOC_N(*vcpupin_list, *nvcpupin + 1) < 0) { + if (VIR_APPEND_ELEMENT(*vcpupin_list, *nvcpupin, &vcpupin) < 0) { virReportOOMError(); + VIR_FREE(vcpupin->cpumask); VIR_FREE(vcpupin); return -1; } - (*vcpupin_list)[(*nvcpupin)++] = vcpupin; - return 0; } @@ -11620,38 +11619,20 @@ int virDomainVcpuPinDel(virDomainDefPtr def, int vcpu) { int n; - bool deleted = false; - virDomainVcpuPinDefPtr *vcpupin_list = def->cputune.vcpupin; /* No vcpupin exists yet */ - if (!def->cputune.nvcpupin) { + if (!def->cputune.nvcpupin) return 0; - } for (n = 0; n < def->cputune.nvcpupin; n++) { - if (vcpupin_list[n]->vcpuid == vcpu) { - VIR_FREE(vcpupin_list[n]->cpumask); - VIR_FREE(vcpupin_list[n]); - memmove(&vcpupin_list[n], - &vcpupin_list[n+1], - (def->cputune.nvcpupin - n - 1) * sizeof(virDomainVcpuPinDef *)); - deleted = true; + if (def->cputune.vcpupin[n]->vcpuid == vcpu) { + VIR_FREE(def->cputune.vcpupin[n]->cpumask); + VIR_FREE(def->cputune.vcpupin[n]); + VIR_DELETE_ELEMENT(def->cputune.vcpupin, n, + def->cputune.nvcpupin); break; } } - - if (!deleted) - return 0; - - if (--def->cputune.nvcpupin == 0) { - VIR_FREE(def->cputune.vcpupin); - } else { - if (VIR_REALLOC_N(def->cputune.vcpupin, def->cputune.nvcpupin) < 0) { - virReportOOMError(); - return -1; - } - } - return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7ad5377..d37b454 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1627,17 +1627,17 @@ struct _virDomainVcpuPinDef { }; void virDomainVcpuPinDefFree(virDomainVcpuPinDefPtr def); -void virDomainVcpuPinDefArrayFree(virDomainVcpuPinDefPtr *def, int nvcpupin); +void virDomainVcpuPinDefArrayFree(virDomainVcpuPinDefPtr *def, size_t nvcpupin); virDomainVcpuPinDefPtr *virDomainVcpuPinDefCopy(virDomainVcpuPinDefPtr *src, - int nvcpupin); + size_t nvcpupin); int virDomainVcpuPinIsDuplicate(virDomainVcpuPinDefPtr *def, - int nvcpupin, + size_t nvcpupin, int vcpu); virDomainVcpuPinDefPtr virDomainVcpuPinFindByVcpu(virDomainVcpuPinDefPtr *def, - int nvcpupin, + size_t nvcpupin, int vcpu); typedef struct _virDomainNumatuneDef virDomainNumatuneDef; @@ -1707,7 +1707,7 @@ struct _virDomainDef { long long quota; unsigned long long emulator_period; long long emulator_quota; - int nvcpupin; + size_t nvcpupin; virDomainVcpuPinDefPtr *vcpupin; virDomainVcpuPinDefPtr emulatorpin; } cputune; @@ -1997,7 +1997,7 @@ int virDomainDefCompatibleDevice(virDomainDefPtr def, virDomainDeviceDefPtr dev); int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, - int *nvcpupin, + size_t *nvcpupin, unsigned char *cpumap, int maplen, int vcpu); diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 30cd1d6..0494f93 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -491,7 +491,7 @@ cleanup: int qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, virDomainVcpuPinDefPtr *vcpupin, - int nvcpupin, + size_t nvcpupin, int vcpuid) { int i; diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index e212581..34d7fde 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -53,7 +53,7 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup, long long quota); int qemuSetupCgroupVcpuPin(virCgroupPtr cgroup, virDomainVcpuPinDefPtr *vcpupin, - int nvcpupin, + size_t nvcpupin, int vcpuid); int qemuSetupCgroupEmulatorPin(virCgroupPtr cgroup, virBitmapPtr cpumask); int qemuSetupCgroupForVcpu(virQEMUDriverPtr driver, virDomainObjPtr vm); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e099c5c..11bebe4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4066,7 +4066,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, int ret = -1; qemuDomainObjPrivatePtr priv; bool doReset = false; - int newVcpuPinNum = 0; + size_t newVcpuPinNum = 0; virDomainVcpuPinDefPtr *newVcpuPin = NULL; virBitmapPtr pcpumap = NULL; @@ -4339,7 +4339,7 @@ qemuDomainPinEmulator(virDomainPtr dom, int ret = -1; qemuDomainObjPrivatePtr priv; bool doReset = false; - int newVcpuPinNum = 0; + size_t newVcpuPinNum = 0; virDomainVcpuPinDefPtr *newVcpuPin = NULL; virBitmapPtr pcpumap = NULL; -- 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
Replaces memmove/VIR_REALLOC_N for xenUnifiedDomainList and XenStoreWatchList. --- src/xen/xen_driver.c | 14 +------------- src/xen/xen_driver.h | 4 ++-- src/xen/xen_inotify.c | 16 +++------------- src/xen/xs_internal.c | 13 +------------ src/xen/xs_internal.h | 2 +- 5 files changed, 8 insertions(+), 41 deletions(-) diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index d2de141..b7159c6 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2495,19 +2495,7 @@ xenUnifiedRemoveDomainInfo(xenUnifiedDomainInfoListPtr list, VIR_FREE(list->doms[i]->name); VIR_FREE(list->doms[i]); - - if (i < (list->count - 1)) - memmove(list->doms + i, - list->doms + i + 1, - sizeof(*(list->doms)) * - (list->count - (i + 1))); - - if (VIR_REALLOC_N(list->doms, - list->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - list->count--; - + VIR_DELETE_ELEMENT(list->doms, i, list->count); return 0; } } diff --git a/src/xen/xen_driver.h b/src/xen/xen_driver.h index 078980e..b0aae9e 100644 --- a/src/xen/xen_driver.h +++ b/src/xen/xen_driver.h @@ -1,7 +1,7 @@ /* * xen_unified.c: Unified Xen driver. * - * Copyright (C) 2007, 2010-2011 Red Hat, Inc. + * Copyright (C) 2007, 2010-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -141,7 +141,7 @@ typedef struct _xenUnifiedDomainInfo xenUnifiedDomainInfo; typedef xenUnifiedDomainInfo *xenUnifiedDomainInfoPtr; struct _xenUnifiedDomainInfoList { - unsigned int count; + size_t count; xenUnifiedDomainInfoPtr *doms; }; typedef struct _xenUnifiedDomainInfoList xenUnifiedDomainInfoList; diff --git a/src/xen/xen_inotify.c b/src/xen/xen_inotify.c index 2e40015..fe9a813 100644 --- a/src/xen/xen_inotify.c +++ b/src/xen/xen_inotify.c @@ -4,7 +4,7 @@ * /etc/xen * /var/lib/xend/domains * - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2012 Red Hat, Inc. * Copyright (C) 2008 VirtualIron * * This library is free software; you can redistribute it and/or @@ -174,18 +174,8 @@ xenInotifyXendDomainsDirRemoveEntry(virConnectPtr conn, if (!memcmp(uuid, priv->configInfoList->doms[i]->uuid, VIR_UUID_BUFLEN)) { VIR_FREE(priv->configInfoList->doms[i]->name); VIR_FREE(priv->configInfoList->doms[i]); - - if (i < (priv->configInfoList->count - 1)) - memmove(priv->configInfoList->doms + i, - priv->configInfoList->doms + i + 1, - sizeof(*(priv->configInfoList->doms)) * - (priv->configInfoList->count - (i + 1))); - - if (VIR_REALLOC_N(priv->configInfoList->doms, - priv->configInfoList->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - priv->configInfoList->count--; + VIR_DELETE_ELEMENT(priv->configInfoList->doms, i, + priv->configInfoList->count); return 0; } } diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c index 8a0af62..1b8d3c7 100644 --- a/src/xen/xs_internal.c +++ b/src/xen/xs_internal.c @@ -1246,18 +1246,7 @@ int xenStoreRemoveWatch(virConnectPtr conn, VIR_FREE(list->watches[i]->path); VIR_FREE(list->watches[i]->token); VIR_FREE(list->watches[i]); - - if (i < (list->count - 1)) - memmove(list->watches + i, - list->watches + i + 1, - sizeof(*(list->watches)) * - (list->count - (i + 1))); - - if (VIR_REALLOC_N(list->watches, - list->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - list->count--; + VIR_DELETE_ELEMENT(list->watches, i, list->count); return 0; } } diff --git a/src/xen/xs_internal.h b/src/xen/xs_internal.h index 84d0d29..e3799b9 100644 --- a/src/xen/xs_internal.h +++ b/src/xen/xs_internal.h @@ -89,7 +89,7 @@ typedef struct _xenStoreWatch xenStoreWatch; typedef xenStoreWatch *xenStoreWatchPtr; struct _xenStoreWatchList { - unsigned int count; + size_t count; xenStoreWatchPtr *watches; }; typedef struct _xenStoreWatchList xenStoreWatchList; -- 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..2eaf7a7 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..921826f 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/util/threads-win32.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/util/threads-win32.c b/src/util/threads-win32.c index 142aa4f..b44d562 100644 --- a/src/util/threads-win32.c +++ b/src/util/threads-win32.c @@ -1,7 +1,7 @@ /* * threads-win32.c: basic thread synchronization primitives * - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -163,12 +163,10 @@ int virCondWait(virCondPtr c, virMutexPtr m) virMutexLock(&c->lock); - if (VIR_REALLOC_N(c->waiters, c->nwaiters + 1) < 0) { + if (VIR_APPEND_ELEMENT_COPY(c->waiters, c->nwaiters, &event) < 0) { virMutexUnlock(&c->lock); return -1; } - c->waiters[c->nwaiters] = event; - c->nwaiters++; virMutexUnlock(&c->lock); @@ -200,14 +198,7 @@ void virCondSignal(virCondPtr c) if (c->nwaiters) { HANDLE event = c->waiters[0]; - if (c->nwaiters > 1) - memmove(c->waiters, - c->waiters + 1, - sizeof(c->waiters[0]) * (c->nwaiters-1)); - if (VIR_REALLOC_N(c->waiters, c->nwaiters - 1) < 0) { - ; - } - c->nwaiters--; + VIR_DELETE_ELEMENT(c->waiters, 0, c->nwaiters); SetEvent(event); } -- 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
The pointer in client continues to be used after the APPEND is done, so we must use the _COPY variant of the append macro. --- src/rpc/virnetserver.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 6a5a53a..ecf1c01 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -277,11 +277,10 @@ static int virNetServerAddClient(virNetServerPtr srv, if (virNetServerClientInit(client) < 0) goto error; - if (VIR_EXPAND_N(srv->clients, srv->nclients, 1) < 0) { + if (VIR_APPEND_ELEMENT_COPY(srv->clients, srv->nclients, &client) < 0) { virReportOOMError(); goto error; } - srv->clients[srv->nclients-1] = client; virObjectRef(client); virNetServerClientSetDispatcher(client, @@ -1134,16 +1133,7 @@ void virNetServerRun(virNetServerPtr srv) virNetServerClientClose(srv->clients[i]); if (virNetServerClientIsClosed(srv->clients[i])) { virObjectUnref(srv->clients[i]); - if (srv->nclients > 1) { - memmove(srv->clients + i, - srv->clients + i + 1, - sizeof(*srv->clients) * (srv->nclients - (i + 1))); - VIR_SHRINK_N(srv->clients, srv->nclients, 1); - } else { - VIR_FREE(srv->clients); - srv->nclients = 0; - } - + VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients); goto reprocess; } } -- 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..8dc62e4 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
There are a few lists in the nwfilter data structures that lend themselves to using the new convenience macros. Note that this patch incidentally fixes what appears to be a bug in virNWFilterUnRegisterCallbackDriver - it was finding an entry to delete, adjusting the array to get rid of it, then writing a 0 over the item that had just been moved into the place previously occupied by the now-deleted item (I think the intent was to 0 out the end of the list, which also doesn't make sense, since the memory allocated to the list had just been decreased in size). --- src/conf/nwfilter_conf.c | 35 ++++++++++++++-------------------- src/conf/nwfilter_conf.h | 6 +++--- src/conf/nwfilter_params.c | 8 +------- src/nwfilter/nwfilter_gentech_driver.c | 15 ++++++--------- 4 files changed, 24 insertions(+), 40 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 2d32bd3..3b15984 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -395,20 +395,19 @@ virNWFilterRuleDefAddString(virNWFilterRuleDefPtr nwf, const char *string, size_t maxstrlen) { - if (VIR_REALLOC_N(nwf->strings, nwf->nstrings+1) < 0) { + char *copy = strndup(string, maxstrlen); + + if (!copy) { virReportOOMError(); return NULL; } - nwf->strings[nwf->nstrings] = strndup(string, maxstrlen); - - if (!nwf->strings[nwf->nstrings]) { + if (VIR_APPEND_ELEMENT(nwf->strings, nwf->nstrings, ©) < 0) { virReportOOMError(); + VIR_FREE(copy); return NULL; } - nwf->nstrings++; - return nwf->strings[nwf->nstrings-1]; } @@ -2585,14 +2584,14 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt) { entry->include = virNWFilterIncludeParse(curr); if (entry->rule || entry->include) { - if (VIR_REALLOC_N(ret->filterEntries, ret->nentries+1) < 0) { - VIR_FREE(entry); + if (VIR_APPEND_ELEMENT(ret->filterEntries, ret->nentries, &entry) < 0) { + virNWFilterEntryFree(entry); virReportOOMError(); goto cleanup; } - ret->filterEntries[ret->nentries++] = entry; - } else + } else { VIR_FREE(entry); + } } curr = curr->next; } @@ -2811,7 +2810,7 @@ virNWFilterDefLoopDetect(virConnectPtr conn, return _virNWFilterDefLoopDetect(conn, nwfilters, def, def->name); } -int nCallbackDriver; +size_t nCallbackDriver; #define MAX_CALLBACK_DRIVER 10 static virNWFilterCallbackDriverPtr callbackDrvArray[MAX_CALLBACK_DRIVER]; @@ -2831,12 +2830,8 @@ virNWFilterUnRegisterCallbackDriver(virNWFilterCallbackDriverPtr cbd) while (i < nCallbackDriver && callbackDrvArray[i] != cbd) i++; - if (i < nCallbackDriver) { - memmove(&callbackDrvArray[i], &callbackDrvArray[i+1], - (nCallbackDriver - i - 1) * sizeof(callbackDrvArray[i])); - callbackDrvArray[i] = 0; - nCallbackDriver--; - } + if (i < nCallbackDriver) + VIR_DELETE_ELEMENT_INPLACE(callbackDrvArray, i, nCallbackDriver); } void @@ -3047,16 +3042,14 @@ virNWFilterObjAssignDef(virConnectPtr conn, nwfilter->active = 0; nwfilter->def = def; - if (VIR_REALLOC_N(nwfilters->objs, nwfilters->count + 1) < 0) { + if (VIR_APPEND_ELEMENT(nwfilters->objs, nwfilters->count, &nwfilter) < 0) { nwfilter->def = NULL; virNWFilterObjUnlock(nwfilter); virNWFilterObjFree(nwfilter); virReportOOMError(); return NULL; } - nwfilters->objs[nwfilters->count++] = nwfilter; - - return nwfilter; + return nwfilters->objs[nwfilters->count - 1]; } diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 4a2a9f3..893b345 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -476,7 +476,7 @@ struct _virNWFilterRuleDef { size_t nVarAccess; virNWFilterVarAccessPtr *varAccess; - int nstrings; + size_t nstrings; char **strings; }; @@ -524,7 +524,7 @@ struct _virNWFilterDef { char *chainsuffix; virNWFilterChainPriority chainPriority; - int nentries; + size_t nentries; virNWFilterEntryPtr *filterEntries; }; @@ -571,7 +571,7 @@ typedef virNWFilterTechDriver *virNWFilterTechDriverPtr; typedef struct _virNWFilterRuleInst virNWFilterRuleInst; typedef virNWFilterRuleInst *virNWFilterRuleInstPtr; struct _virNWFilterRuleInst { - int ndata; + size_t ndata; void **data; virNWFilterTechDriverPtr techdriver; }; diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index 7254519..0bd6ae4 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -269,13 +269,7 @@ virNWFilterVarValueDelNthValue(virNWFilterVarValuePtr val, unsigned int pos) case NWFILTER_VALUE_TYPE_ARRAY: if (pos < val->u.array.nValues) { VIR_FREE(val->u.array.values[pos]); - val->u.array.nValues--; - - if (pos < val->u.array.nValues) - memmove(&val->u.array.values[pos], - &val->u.array.values[pos + 1], - sizeof(val->u.array.values[0]) * - (val->u.array.nValues - pos)); + VIR_DELETE_ELEMENT_INPLACE(val->u.array.values, pos, val->u.array.nValues); return 0; } break; diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 572acf4..15d41cd 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -1,7 +1,7 @@ /* * nwfilter_gentech_driver.c: generic technology driver * - * Copyright (C) 2011 Red Hat, Inc. + * Copyright (C) 2011, 2012 Red Hat, Inc. * Copyright (C) 2010 IBM Corp. * Copyright (C) 2010 Stefan Berger * @@ -109,11 +109,10 @@ int virNWFilterRuleInstAddData(virNWFilterRuleInstPtr res, void *data) { - if (VIR_REALLOC_N(res->data, res->ndata+1) < 0) { + if (VIR_APPEND_ELEMENT(res->data, res->ndata, &data) < 0) { virReportOOMError(); return -1; } - res->data[res->ndata++] = data; return 0; } @@ -387,7 +386,7 @@ _virNWFilterInstantiateRec(virNWFilterTechDriverPtr techdriver, virNWFilterDefPtr filter, const char *ifname, virNWFilterHashTablePtr vars, - int *nEntries, + size_t *nEntries, virNWFilterRuleInstPtr **insts, enum instCase useNewFilter, bool *foundNewFilter, virNWFilterDriverStatePtr driver) @@ -413,14 +412,12 @@ _virNWFilterInstantiateRec(virNWFilterTechDriverPtr techdriver, break; } - if (VIR_REALLOC_N(*insts, (*nEntries)+1) < 0) { + if (VIR_APPEND_ELEMENT(*insts, *nEntries, &inst) < 0) { virReportOOMError(); rc = -1; break; } - (*insts)[(*nEntries)++] = inst; - } else if (inc) { VIR_DEBUG("Instantiating filter %s", inc->filterref); obj = virNWFilterObjFindByName(&driver->nwfilters, inc->filterref); @@ -595,7 +592,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, static int -virNWFilterRuleInstancesToArray(int nEntries, +virNWFilterRuleInstancesToArray(size_t nEntries, virNWFilterRuleInstPtr *insts, void ***ptrs, int *nptrs) @@ -662,7 +659,7 @@ virNWFilterInstantiate(const unsigned char *vmuuid ATTRIBUTE_UNUSED, { int rc; int j, nptrs; - int nEntries = 0; + size_t nEntries = 0; virNWFilterRuleInstPtr *insts = NULL; void **ptrs = NULL; int instantiate = 1; -- 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/ebtables.c | 10 ++-------- src/util/ebtables.h | 4 ++-- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/src/util/ebtables.c b/src/util/ebtables.c index d3a8432..6c21d39 100644 --- a/src/util/ebtables.c +++ b/src/util/ebtables.c @@ -114,11 +114,10 @@ ebtRulesAppend(ebtRules *rules, { ebtRule tmp = { .rule = rule, .argv = argv, .command_idx = command_idx }; - if (VIR_REALLOC_N(rules->rules, rules->nrules+1) < 0) { + if (VIR_APPEND_ELEMENT(rules->rules, rules->nrules, &tmp) < 0) { ebtRuleClear(&tmp); return ENOMEM; } - rules->rules[rules->nrules++] = tmp; return 0; } @@ -137,12 +136,7 @@ ebtRulesRemove(ebtRules *rules, ebtRuleClear(&rules->rules[i]); - memmove(&rules->rules[i], - &rules->rules[i+1], - (rules->nrules - i - 1) * sizeof(ebtRule)); - - rules->nrules--; - + VIR_DELETE_ELEMENT_INPLACE(rules->rules, i, rules->nrules); return 0; } diff --git a/src/util/ebtables.h b/src/util/ebtables.h index 49dc8a0..0381c25 100644 --- a/src/util/ebtables.h +++ b/src/util/ebtables.h @@ -1,6 +1,6 @@ /* * Copyright (C) 2009 IBM Corp. - * Copyright (C) 2007, 2008 Red Hat, Inc. + * Copyright (C) 2007, 2008, 2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -38,7 +38,7 @@ typedef struct char *table; char *chain; - int nrules; + size_t nrules; ebtRule *rules; } ebtRules; -- 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..5166bd8 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
On 12/07/2012 11:16 AM, Laine Stump wrote:
DO NOT BE SCARED OFF BY THE PATCH COUNT!! Most of these are very short, and they all follow the same pattern; beyond that, 01/17 and 02/17 are the only ones that are really time-sensitive.
The first patch in this series adds new macros to insert/append/delete elements from the arrays of variously-sized structs that are commonly used in libvirt. The intent is to eliminate the arithmetic mistakes associated with calling memmove() directly, and shorten application code. A full description of the added macros and their functions is included in 01/17.
Patches 02..17 convert various bits of hand-coded array manipulation in libvirt to use the new macros. They can be taken now, later, or never (although I'd like to have at least 02/17 taken now, since other similar functions will be added in an upcoming patch series, and I want them to all use the same logic). Mostly I went through all of those conversions to see just how useful/usable the new macros were (and they led to a couple of rewrites).
I will be posting another series momentarily that has a dependency on 01/17.
Dunno why you don't have a diffstat, to make it a bit more impressive (the --cover-letter option to get send-email or git format will create a diffstat into your 0/17 mail): cfg.mk | 2 +- src/conf/domain_conf.c | 152 ++++++--------------------------- src/conf/domain_conf.h | 12 +-- src/conf/domain_event.c | 51 ++--------- src/conf/interface_conf.c | 13 +-- src/conf/interface_conf.h | 4 +- src/conf/network_conf.c | 102 ++++++---------------- src/conf/network_conf.h | 6 +- src/conf/node_device_conf.c | 11 +-- src/conf/node_device_conf.h | 4 +- src/conf/nwfilter_conf.c | 46 ++++------ src/conf/nwfilter_conf.h | 8 +- src/conf/nwfilter_params.c | 8 +- src/conf/storage_conf.c | 11 +-- src/conf/storage_conf.h | 4 +- src/libvirt_private.syms | 2 + src/nwfilter/nwfilter_gentech_driver.c | 15 ++-- src/parallels/parallels_storage.c | 14 +-- src/qemu/qemu_cgroup.c | 2 +- src/qemu/qemu_cgroup.h | 2 +- src/qemu/qemu_domain.c | 6 +- src/qemu/qemu_driver.c | 4 +- src/rpc/virnetclient.c | 21 +---- src/rpc/virnetserver.c | 14 +-- src/storage/storage_backend_rbd.c | 2 +- src/storage/storage_driver.c | 12 +-- src/test/test_driver.c | 14 +-- src/util/ebtables.c | 29 ++----- src/util/ebtables.h | 4 +- src/util/hostusb.c | 26 ++---- src/util/memory.c | 102 +++++++++++++++++++++- src/util/memory.h | 118 ++++++++++++++++++++++++- src/util/pci.c | 18 +--- src/util/threads-win32.c | 15 +--- src/util/virlockspace.c | 20 +---- src/xen/xen_driver.c | 14 +-- src/xen/xen_driver.h | 4 +- src/xen/xen_inotify.c | 16 +--- src/xen/xm_internal.c | 14 +-- src/xen/xs_internal.c | 13 +-- src/xen/xs_internal.h | 2 +- tests/qemumonitortestutils.c | 23 +---- 42 files changed, 384 insertions(+), 576 deletions(-) That is, in spite of 200+ lines added to memory.[ch], you still managed to let the series have a net reduction of nearly 200 lines to the overall code base by shaving redundant code elsewhere, all while making the usage pattern safer :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/07/2012 01:53 PM, Eric Blake wrote:
On 12/07/2012 11:16 AM, Laine Stump wrote:
DO NOT BE SCARED OFF BY THE PATCH COUNT!! Most of these are very short, and they all follow the same pattern; beyond that, 01/17 and 02/17 are the only ones that are really time-sensitive.
The first patch in this series adds new macros to insert/append/delete elements from the arrays of variously-sized structs that are commonly used in libvirt. The intent is to eliminate the arithmetic mistakes associated with calling memmove() directly, and shorten application code. A full description of the added macros and their functions is included in 01/17.
Patches 02..17 convert various bits of hand-coded array manipulation in libvirt to use the new macros. They can be taken now, later, or never (although I'd like to have at least 02/17 taken now, since other similar functions will be added in an upcoming patch series, and I want them to all use the same logic). Mostly I went through all of those conversions to see just how useful/usable the new macros were (and they led to a couple of rewrites).
I will be posting another series momentarily that has a dependency on 01/17. Dunno why you don't have a diffstat, to make it a bit more impressive (the --cover-letter option to get send-email or git format will create a diffstat into your 0/17 mail):
Ah, I always use --compose, and sometimes wondered why other people had a diffstat and I didn't :-)
That is, in spite of 200+ lines added to memory.[ch], you still managed to let the series have a net reduction of nearly 200 lines to the overall code base by shaving redundant code elsewhere, all while making the usage pattern safer :)
There's still a couple of disappointments: 1) I have to typecast a NULL newelem 2) I had to add the silly "_COPY" versions of the macros because sometimes the element being inserted to an array is just a pointer, and the caller wants to continue to use that same reference to it after return.
On 12/07/2012 02:24 PM, Laine Stump wrote:
There's still a couple of disappointments:
1) I have to typecast a NULL newelem
Ah, so you DID hit the compile error on *(NULL), and figured that *((type*)NULL)) works around it. Yeah, it would be slick if we could cleanly avoid the need for the typecast, and only check the compile-time safety on the &orig form. I'll see if I can come up with anything, but I'm not holding my breath (making decisions at preprocessor time can be a lot harder than it seems).
2) I had to add the silly "_COPY" versions of the macros because sometimes the element being inserted to an array is just a pointer, and the caller wants to continue to use that same reference to it after return.
Yeah - we ended up inventing lots of variants, even without adding some _N arbitrary-size variants into the mix. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump