Re: [Qemu-devel] [PATCH v15 6/9] Add xbzrle_encode_buffer and xbzrle_decode_buffer functions

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]

On 07/05/2012 06:51 AM, Orit Wasserman wrote:

This commit message is a bit sparse.  I'd document at least the fact
that our nzrun detection code in xbzrle_encode_buffer borrows
long-word-at-a-time NUL-detection tricks from strcmp(), as it is not an
intuitive trick known by all developers.

> Signed-off-by: Benoit Hudzia <benoit.hudzia@sap.com>
> Signed-off-by: Petter Svard <petters@cs.umu.se>
> Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>

I think I touched this one heavily enough that it warrants adding:

Signed-off-by: Eric Blake <eblake@redhat.com>

Other than the commit message, I'm happy with this patch contents now.
Still some nits, though:

> +
> +         /* not aligned to sizeof(long) */
> +        res = (slen - i) % sizeof(long);

Comment indentation is off.

> +        while (res && old_buf[i] == new_buf[i]) {
> +            zrun_len++;
> +            i++;
> +            res--;
> +        }
> +
> +        if (!res) {

A comment here might help:

/* word at a time for speed */

> +            while (i < slen &&
> +                   (*(long *)(old_buf + i)) == (*(long *)(new_buf + i))) {

On re-reading this, I'm worried whether it violates strict C99 aliasing
rules; I'm hoping the compiler doesn't mis-optimize it based on a
loophole of us using questionable semantics.  In practice, I'm confident
that we are only doing aligned reads (otherwise I wouldn't have
suggested this line in the first place), which is why I'm personally
okay with it even if it technically violates C99.  However, I'm open to
suggestions from anyone else on the list on how to better express the
notion of intentionally accessing a long at a time from an aligned
offset into a byte array, if that will help us avoid even theoretical
problems.

> +            nzrun_len++;
> +            res--;
> +        }
> +
> +        if (!res) {
> +            /* truncation to 32-bit long okay */

Again, a longer comment might help:

/* word at a time for speed, use of 32-bit long okay */

> +            long mask = 0x0101010101010101ULL;
> +            while (i < slen) {
> +                xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);

Same potential strict aliasing problems as for zrun detection.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]