[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-trivial] [PATCH] arch_init: Simplify code for loa
From: |
Michael Tokarev |
Subject: |
Re: [Qemu-devel] [Qemu-trivial] [PATCH] arch_init: Simplify code for load_xbzrle() |
Date: |
Mon, 19 May 2014 11:46:10 +0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.5.0 |
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.
>