Re: [Qemu-devel] [PATCH v5 07/10] xbzrle: don't check the value in the vm ram repeatedly

[Dr. David Alan Gilbert]

* address@hidden (address@hidden) wrote:
> From: ChenLiang <address@hidden>
> 
> xbzrle_encode_buffer checks the value in the vm ram repeatedly.
> It is risk if runs xbzrle_encode_buffer on changing data.
> And it is not necessary.
> 
> Reported-by: Dr. David Alan Gilbert <address@hidden>
> Signed-off-by: ChenLiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  xbzrle.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/xbzrle.c b/xbzrle.c
> index fbcb35d..92cccd7 100644
> --- a/xbzrle.c
> +++ b/xbzrle.c
> @@ -27,9 +27,10 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t 
> *new_buf, int slen,
>                           uint8_t *dst, int dlen)
>  {
>      uint32_t zrun_len = 0, nzrun_len = 0;
> -    int d = 0, i = 0;
> +    int d = 0, i = 0, j;
>      long res, xor;
>      uint8_t *nzrun_start = NULL;
> +    uint8_t *xor_ptr = (uint8_t *)(&xor);
>  
>      g_assert(!(((uintptr_t)old_buf | (uintptr_t)new_buf | slen) %
>                 sizeof(long)));
> @@ -82,6 +83,8 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t 
> *new_buf, int slen,
>          if (d + 2 > dlen) {
>              return -1;
>          }
> +        i++;
> +        nzrun_len++;

Yes, I think that's safe - I was checking for if an overflow was possible, but
my reading is that before this 'i++' i can be a maximum of slen-1, so here
it's a maximum of slen and the next loop won't happen in that case.

>          /* not aligned to sizeof(long) */
>          res = (slen - i) % sizeof(long);
>          while (res && old_buf[i] != new_buf[i]) {
> @@ -98,11 +101,16 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t 
> *new_buf, int slen,
>                  xor = *(long *)(old_buf + i) ^ *(long *)(new_buf + i);
>                  if ((xor - mask) & ~xor & (mask << 7)) {
>                      /* found the end of an nzrun within the current long */
> -                    while (old_buf[i] != new_buf[i]) {
> -                        nzrun_len++;
> -                        i++;
> +                    for (j = 0; j < sizeof(long); j++) {
> +                        if (0 == xor_ptr[j]) {
> +                            break;
> +                        }
> +                    }
> +                    i += j;
> +                    nzrun_len += j;

> +                    if (j != sizeof(long)) {
> +                        break;
>                      }
> -                    break;
>                  } else {
>                      i += sizeof(long);
>                      nzrun_len += sizeof(long);
> @@ -118,6 +126,8 @@ int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t 
> *new_buf, int slen,
>          memcpy(dst + d, nzrun_start, nzrun_len);
>          d += nzrun_len;
>          nzrun_len = 0;
> +        i++;
> +        zrun_len++;

I think that's also safe, because if i was now 'slen' the mainloop would exit, 
that would
mean the last zero run wasn't encoded, but there seems to already be a check 
that causes
the last zero run not to be encoded.

Reviewed-by: Dr. David Alan Gilbert <address@hidden>

>      }
>  
>      return d;
> -- 
> 1.7.12.4
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK