Message ID | 53789122.7080904@gmail.com |
---|---|
State | New |
Headers | show |
Meanwhile the original version has been merged from juanquintela/tags/migration/20140515 in the original form, so the point is moot already. Juan, can you please indicate that you applied something, instead of just giving a Reviewed-by? Thanks, /mjt 18.05.2014 14:53, Chen Gang wrote: > On 05/17/2014 03:54 PM, Michael Tokarev wrote: >> 10.05.2014 16:51, Chen Gang wrote: >>> For xbzrle_decode_buffer(), when decoding contents will exceed writing >>> buffer, it will return -1, so need not check the return value whether >>> large than writing buffer. >>> >>> And when failure occurs within load_xbzrle(), it always return -1 >>> without any resources which need release. >>> >>> So can remove the related checking statements, and also can remove 'rc' >>> and 'ret' local variables, >> >> Just one comment below. >> >>> @@ -933,18 +932,13 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) >>> qemu_get_buffer(f, xbzrle_decoded_buf, xh_len); >>> >>> /* decode RLE */ >>> - ret = xbzrle_decode_buffer(xbzrle_decoded_buf, xh_len, host, >>> - TARGET_PAGE_SIZE); >>> - if (ret == -1) { >>> + if (xbzrle_decode_buffer(xbzrle_decoded_buf, xh_len, host, >>> + TARGET_PAGE_SIZE) == -1) { >> >> Can we compare like '< 0' here, not like '== -1' ? > > That's fine to me. > >> Are there any other possible return values from xbzrle_decode_buffer() >> which are less than zero but non-error? >> > > Although, at present, when it fails, it will only return -1. > > >> To me, anything less than zero is always error (unless it is one of the >> possible non-error values, like offset for example which can be negative). >> > > That sounds reasonable to me, too. > >> Especially having in mind that in the future, some function may extend >> its error return to include the actual error code (like -errno), in which >> case code which compares with -1 will not work anymore. >> > > Yeah, in the future, it may do. > >> Is it okay to me to apply this with s/== -1/< 0/ ? >> > > At least, it is OK to me. > > > BTW: the related test code for xbzrle_decode_buffer() may also need > improved (although, after read through, I still don't known what it > really want to do). > > diff --git a/tests/test-xbzrle.c b/tests/test-xbzrle.c > index db93b0a..c8b4e58 100644 > --- a/tests/test-xbzrle.c > +++ b/tests/test-xbzrle.c > @@ -162,7 +162,7 @@ static void encode_decode_range(void) > PAGE_SIZE); > > rc = xbzrle_decode_buffer(compressed, dlen, test, PAGE_SIZE); > - g_assert(rc < PAGE_SIZE); > + g_assert(rc < PAGE_SIZE && rc >= 0); > g_assert(memcmp(test, buffer, PAGE_SIZE) == 0); > > g_free(buffer); > > Please help check when you have time. If necessary, I shall send related > patch for it. (this fix may be still incorrect, if so, please help send > related patch for it, and welcome to mark me as Reported-by for it). > > > Thanks. >
diff --git a/tests/test-xbzrle.c b/tests/test-xbzrle.c index db93b0a..c8b4e58 100644 --- a/tests/test-xbzrle.c +++ b/tests/test-xbzrle.c @@ -162,7 +162,7 @@ static void encode_decode_range(void) PAGE_SIZE); rc = xbzrle_decode_buffer(compressed, dlen, test, PAGE_SIZE); - g_assert(rc < PAGE_SIZE); + g_assert(rc < PAGE_SIZE && rc >= 0); g_assert(memcmp(test, buffer, PAGE_SIZE) == 0); g_free(buffer);