"Li, Liang Z" <liang.z...@intel.com> wrote: > On %D, %SN wrote: > %Q > > %C
Nice!!!! Template answer without insntantiate the content O:-) > > 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 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.
