[libvirt] [PATCH] qemuDomainCleanupRemove: s/memmove/VIR_DELETE_ELEMENT_INPLACE/

[Laine Stump]

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 at 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).