arch_init: Simplify code for load_xbzrle()

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,

Signed-off-by Chen Gang <[email protected]>

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,

Quoted 9 lines · Chen Gang

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.

Quoted 3 lines

(quoted diff) +0 -0

@@ -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);
 

Review static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) · line 936

Quoted 6 lines

(quoted diff) +2 -3

     /* 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' ?

Are there any other possible return values from xbzrle_decode_buffer() which are less than zero but non-error?

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).

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.

Is it okay to me to apply this with s/== -1/< 0/ ?

Quoted 16 lines · Michael Tokarev

Quoted 9 lines · Chen Gang

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.

Quoted 3 lines

(quoted diff) +0 -0

@@ -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);
 

Quoted 8 lines

Review static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host) · line 936

Quoted 6 lines

(quoted diff) +2 -3

     /* 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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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).

tests/test-xbzrle.c +1 -1

index db93b0a..c8b4e58 100644
@@ -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.