On 10/17/2013 04:33 PM, Michal Privoznik wrote:

The last argument of memmove is the amount of bytes to be moved. The amount is in Bytes. We are moving some void pointers around. However, since sizeof(void *) is not Byte on any architecture, we've got the arithmetic wrong.

My personal experience with memmove and memcpy (and for that matter, *any* function that takes void* to an "array of something") is that I almost always get the arithmetic wrong the first time. When I wrote VIR_DELETE_ELEMENT and friends, I had several followup patches which replaced almost all memmove() calls in libvirt with calls to one of the new macros, but didn't ask for an ACK on all of them because I wasn't convinced that automated testing would catch all possible regressions (or that I could properly test them all manually). Maybe I should dig out those patches and see how many of them are still relevant.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d054d64..b8aec2d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2235,12 +2235,9 @@ qemuDomainCleanupRemove(virDomainObjPtr vm, VIR_DEBUG("vm=%s, cb=%p", vm->def->name, cb);

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--; - } + if (priv->cleanupCallbacks[i] == cb) + VIR_DELETE_ELEMENT_INPLACE(priv->cleanupCallbacks, + i, priv->ncleanupCallbacks); }

VIR_SHRINK_N(priv->cleanupCallbacks,

Note that if ncleanupCallbacks_max (and use of VIR_RESIZE) was eliminated for this array, the combination of VIR_DELETE_ELEMENT_INPLACE + VIR_SHRINK_N could be replaced with a single VIR_DELETE_ELEMENT. In my opinion, the compute-time savings of using VIR_RESIZE to reallocate in larger chunks (vs. reallocating each time a single item is added to the array) is insignificant in comparison to the reduced likelyhood of hard-to-find bugs caused by having more complicated code (especially in this case, where the number of cleanup callbacks is going to be very small, and they will be very infrequently added/removed).