From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45910) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WWA8q-0005a7-QR for qemu-devel@nongnu.org; Fri, 04 Apr 2014 15:51:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WWA8i-00072k-5g for qemu-devel@nongnu.org; Fri, 04 Apr 2014 15:51:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9259) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WWA8h-00072f-UK for qemu-devel@nongnu.org; Fri, 04 Apr 2014 15:51:00 -0400 Date: Fri, 4 Apr 2014 20:50:50 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140404195049.GA2492@work-vm> References: <1396605482-8720-1-git-send-email-arei.gonglei@huawei.com> <1396605482-8720-8-git-send-email-arei.gonglei@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1396605482-8720-8-git-send-email-arei.gonglei@huawei.com> Subject: Re: [Qemu-devel] [PATCH v5 07/10] xbzrle: don't check the value in the vm ram repeatedly List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: arei.gonglei@huawei.com Cc: ChenLiang , weidong.huang@huawei.com, quintela@redhat.com, qemu-devel@nongnu.org, dgilbert@redhat.com, pbonzini@redhat.com * arei.gonglei@huawei.com (arei.gonglei@huawei.com) wrote: > From: ChenLiang > > 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 > Signed-off-by: ChenLiang > Signed-off-by: Gonglei > --- > 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 > } > > return d; > -- > 1.7.12.4 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK