Re: [Qemu-devel] [PATCH RESEND v2 1/2] qemu-file: Fix qemu_put_compression_data flaw

[Li, Liang Z]

On %D, %SN wrote:
%Q

%C

Liang


> -----Original Message-----
> From: address@hidden [mailto:qemu-
> address@hidden On Behalf Of Juan Quintela
> Sent: Tuesday, February 23, 2016 5:57 PM
> To: Li, Liang Z
> Cc: address@hidden; address@hidden; qemu-
> address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH RESEND v2 1/2] qemu-file: Fix
> qemu_put_compression_data flaw
> 
> "Li, Liang Z" <address@hidden> wrote:
> > Ping ...
> >
> > Liang
> 
> Hi
> 
> >> We should fix this flaw to make it works with writable QEMUFile.
> >>
> >> Signed-off-by: Liang Li <address@hidden>
> >> ---
> >>  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?
> 
> (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

I just want to refine the code in the function ram_save_compressed_page(),
so as to reduce some data copy. 
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.

Liang