| Message ID | 536E20CC.4030401@gmail.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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,
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(-)