On 12/10/2012 02:20 PM, 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.
>
> 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.
This is slightly out of date, now that we reworked things to always add
a new element. (Our IRC conversation was that the underlying function
should still support NULL instead of an array, but that we won't worry
about passing NULL via the macros until we have a need for a
VIR_INSERT_N_ELEMENTS macro later on).
> 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).
We probably ought to VIR_WARN about coding errors if we hit the invalid
index case.
>
> [*] One initial problem with the INSERT and APPEND macros was that,
> due to both the array pointer and newelem pointer being cast to void*
> when passing to virInsertElementsN(), any chance of type-checking was
> lost. If we were going to move in newelem with a memmove anyway, we
> would be no worse off for this. However, most current open-coded
> insert/append operations use direct struct assignment to move the new
> element into place (or just populate the new element directly) - thus
> use of the new macros would open a possibility for new usage errors
> that didn't exist before (e.g. accidentally sending &newelemptr rather
> than newelemptr - I actually did this quite a lot in my test
> conversions of existing code).
>
> But thanks to Eric Blake's clever thinking, I was able to modify the
> INSERT and APPEND macros so that they *do* check for both assignment
> and size compatibility of *ptr (an element in the array) and newelem
> (the element being copied into the new position of the array). This is
> done via clever use of the C89-guaranteed fact that the sizeof()
> operator is must have *no* side effects (so an assignment inside
s/is //
> 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:
Now that you have a similar comment in the code, you could get away with
trimming this explanation out of the commit message. Then again, long
commit messages hurt no one, so I don't care either way.
>
> * sizeof(*(a) = *(b)) assures that *a and *b are
> assignment-compatible (they may still have a different size
> though! e.g. longVar = intVar) (If not, there is a compile-time
> error. If so, the result of that subexpression is sizeof(*(a)),
> i.e. one element of the array)
>
> * sizeof(*(a) = *(b)) == sizeof(*(b)) checks if *a and *b are also
> of the same size (so that, e.g. you don't accidentally copy an
> int plus the random bytes following it into an array of long). It
> evaluates to 1 if they are the same, and 0 otherwise.
>
> * sizeof(char[2 * (result of previous step) - 1]) evaluates to 1 if
> the previous step was successful (char [(2*1) - 1] ==> char[1]),
> of generates a compile error if it wasn't successful
> (char[2*0) -1] ==> char[-1], which isn't legal).
>
> So we end up sending "1" to the caller, and in the meantime check that we've
> actually added the correct &'s and/or *'s to the arguments. (Whew!)
>
> The result is that we've been relieved of the burden of getting the
> math right for the arguments to memmove when expanding/contracting the
> array, and haven't lost the type checking of ptr and newelem.
> ---
> cfg.mk | 2 +-
> src/libvirt_private.syms | 2 +
> src/util/memory.c | 106 +++++++++++++++++++++++++++++-
> src/util/memory.h | 167 ++++++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 274 insertions(+), 3 deletions(-)
ACK once you fix the following nits:
> +int
> +virInsertElementsN(void *ptrptr, size_t size, size_t at,
> + size_t *countptr,
> + size_t add, void *newelems,
> + bool clearOriginal, bool inPlace)
> +{
> + if (at > *countptr)
> + return -1;
debug message about a coding error if we ever get here
> + if (newelems) {
> + memcpy(*(char**)ptrptr + (size * at), newelems, size * add);
> + if (clearOriginal)
> + memset((char*)newelems, 0, size * add);
> + } else if (inPlace || (at < *countptr - add)) {
> + /* NB: if inPlace, even memory at the end wasn't initialized */
It took me two reads to get this. Maybe:
s/inPlace, even/inPlace, assume/
> +virDeleteElementsN(void *ptrptr, size_t size, size_t at,
> + size_t *countptr, size_t remove,
> + bool inPlace)
> +{
> + if (at + remove > *countptr)
> + return -1;
Again, debug message if we hit here.
> /*
> + * VIR_TYPEMATCH:
> + *
> + * The following macro seems a bit cryptic, so it needs a thorough
> + * explanation. Its purpose is to check for assignment compatibility
> + * and identical size between two values without creating any side
> + * effects (by doing something silly like actually assigning one to
> + * the other). Note that it takes advantage of the C89-guaranteed
> + * property of sizeof() - it cannot have any side effects, so anything
> + * that happens inside sizeof() will not have any effect at runtime.
> + *
> + * VIR_TYPEMATCH evaluates to "1" if the two passed values are both
> + * assignment-compatible and the same size, and otherwise generates a
> + * compile-time error. It determines the result by performing the
> + * following three operations:
> + *
> + * * sizeof(*(a) = *(b)) assures that *a and *b are
> + * assignment-compatible (they may still have a different size
> + * though! e.g. longVar = intVar) (If not, there is a compile-time
> + * error. If so, the result of that subexpression is sizeof(*(a)),
> + * i.e. one element of the array)
> + *
> + * * sizeof(*(a) = *(b)) == sizeof(*(b)) checks if *a and *b are also
> + * of the same size (so that, e.g. you don't accidentally copy an
> + * int plus the random bytes following it into an array of long). It
> + * evaluates to 1 if they are the same, and 0 otherwise.
> + *
> + * * sizeof(char[2 * (result of previous step) - 1]) evaluates to 1
> + * if the previous step was successful (char [(2*1) - 1] == *
s/*$//
> + * char[1]), of generates a compile error if it wasn't successful
s/of/or/
> + * (char[2*0) -1] == * char[-1], which isn't legal and again
s/== */==/
> + * generates a compile error).
Who writes the law that says what C code is legal? I prefer the term
'valid', to avoid confusion with legal issues. :)
Also, you mentioned 'generate a compile error' twice. I would just end
with 'which isn't valid).'
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list