[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Chen Gang
Subject: Re: [Qemu-trivial] [PATCH] arch_init: Simplify code for load_xbzrle()
Date: Sun, 18 May 2014 18:53:22 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7

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

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

Chen Gang

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]