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

[Li, Liang Z]

> Nice!!!! Template answer without insntantiate the content O:-)
> 

:-),  forgot to remove it.
> >
> > Liang
> >
> >
> >> -----Original Message-----
> >> From: address@hidden
> >> [mailto:qemu-
> >> address@hidden On Behalf Of Juan
> >> devel-bounces+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?
> 
> 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