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

[Orit Wasserman]

On 07/05/2012 04:55 PM, Eric Blake wrote:
> 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 <address@hidden>
>> Signed-off-by: Petter Svard <address@hidden>
>> Signed-off-by: Aidan Shribman <address@hidden>
>> Signed-off-by: Orit Wasserman <address@hidden>
> 
> I think I touched this one heavily enough that it warrants adding:
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
Of course
Orit
> 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.
This is a very interesting question ....

Orit
> 
>> +            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.
>