[Qemu-devel] [PATCH] arch_init: Simplify code for load_xbzrle()

[Qemu-devel] [PATCH] arch_init: Simplify code for load_xbzrle()

[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,


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 arch_init.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 60c975d..98ee5b6 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -908,7 +908,6 @@ static uint64_t ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size)
 
 static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
 {
-    int ret, rc = 0;
     unsigned int xh_len;
     int xh_flags;
 
@@ -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) {
         fprintf(stderr, "Failed to load XBZRLE page - decode error!\n");
-        rc = -1;
-    } else  if (ret > TARGET_PAGE_SIZE) {
-        fprintf(stderr, "Failed to load XBZRLE page - size %d exceeds %d!\n",
-                ret, TARGET_PAGE_SIZE);
-        abort();
+        return -1;
     }
 
-    return rc;
+    return 0;
 }
 
 static inline void *host_from_stream_offset(QEMUFile *f,
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH] arch_init: Simplify code for load_xbzrle()

[Juan Quintela]

Chen Gang <gang.chen.5i5j@gmail.com> 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,
>
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [Qemu-trivial] [PATCH] arch_init: Simplify code for load_xbzrle()

[Michael Tokarev]

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

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH] arch_init: Simplify code for load_xbzrle()

[Chen Gang]

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.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [Qemu-devel] [Qemu-trivial] [PATCH] arch_init: Simplify code for load_xbzrle()

[Michael Tokarev]

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