> Nice!!!! Template answer without insntantiate the content O:-) > :-), forgot to remove it. > > > > Liang > > > > > >> -----Original Message----- > >> From: qemu-devel-bounces+liang.z.li=intel....@nongnu.org > >> [mailto:qemu- > >> devel-bounces+liang.z.li=intel....@nongnu.org] On Behalf Of Juan > >> devel-bounces+Quintela > >> Sent: Tuesday, February 23, 2016 5:57 PM > >> To: Li, Liang Z > >> Cc: amit.s...@redhat.com; zhang.zhanghaili...@huawei.com; qemu- > >> de...@nongnu.org; dgilb...@redhat.com > >> Subject: Re: [Qemu-devel] [PATCH RESEND v2 1/2] qemu-file: Fix > >> qemu_put_compression_data flaw > >> > >> "Li, Liang Z" <liang.z...@intel.com> wrote: > >> > Ping ... > >> > > >> > Liang > >> > >> Hi > >> > >> >> We should fix this flaw to make it works with writable QEMUFile. > >> >> > >> >> Signed-off-by: Liang Li <liang.z...@intel.com> > >> >> --- > >> >> migration/qemu-file.c | 23 +++++++++++++++++++++-- > >> >> 1 file changed, 21 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c index > >> >> 0bbd257..b956ab6 100644 > >> >> --- a/migration/qemu-file.c > >> >> +++ b/migration/qemu-file.c > >> >> @@ -606,8 +606,14 @@ uint64_t qemu_get_be64(QEMUFile *f) > >> >> return v; > >> >> } > >> >> > >> >> -/* compress size bytes of data start at p with specific > >> >> compression > >> >> +/* Compress size bytes of data start at p with specific > >> >> +compression > >> >> * level and store the compressed data to the buffer of f. > >> >> + * > >> >> + * When f is not writable, return 0 if f has no space to save the > >> >> + * compressed data. > >> >> + * When f is wirtable and it has no space to save the compressed > >> >> + data, > >> >> + * do fflush first, if f still has no space to save the > >> >> + compressed > >> >> + * data, return 0. > >> >> */ > >> > >> Ok, I still don't understand _why_ f can be writable on compression > >> code, but well. > >> > >> We return r when we are not able to write, right? > >> static int do_compress_ram_page(CompressParam *param) { > >> int bytes_sent, blen; > >> uint8_t *p; > >> RAMBlock *block = param->block; > >> ram_addr_t offset = param->offset; > >> > >> p = block->host + (offset & TARGET_PAGE_MASK); > >> > >> bytes_sent = save_page_header(param->file, block, offset | > >> RAM_SAVE_FLAG_COMPRESS_PAGE); > >> blen = qemu_put_compression_data(param->file, p, > TARGET_PAGE_SIZE, > >> migrate_compress_level()); > >> bytes_sent += blen; > >> > >> return bytes_sent; > >> } > > >> But we don't check for zero when doing the compression, shouldn't > >> this give give an error? > > You arent answering this one. I still think that we sould check for > qemu_put_compression_data() return zero. That is one error. > > >> (yes, caller of do_co_compress_ram_page() don't check for zero either). > >> > > > >> I guess you are trying to do something different with the compression > >> code (otherwise this qemu_fflush() or add_to_iovec() don't make > >> sense), but I don't know what. > >> > >> With current code, the only thing that we miss is the check for errors, > >> right? > >> And if we want to do something else with it, could you explain? Thanks. > >> > >> Later, Juan. > > > > I think these two threads will help to explain why I do that. > > > > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg02675.html > > > > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg02674.html > > They tell that they are generic functions and other code could need the new > functionality. That code don't exist yet. I understand now why you want to > put it as iovec, and it make sense. Can you fix the error handling and we can > add this. > > > I just want to refine the code in the function > > ram_save_compressed_page(), so as to reduce some data copy. > > Ok, that explains why we want to change it to use the iovec, not the writable > part. But I can add both. > > > In the other hand, qemu_put_compression_data() is a common function, > > it maybe used by other code, but it's current implementation has some > > limitation, we should make it robust. > > Please, fix the handling of the error code (change the zero to -1 if it makes > you feel better), resubmit and I will apply, ok? > > Thanks, Juan.
Indeed, change the error code to -1 is better, will do it in the next version. Thanks. Liang
