[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RESEND v2 1/2] qemu-file: Fix qemu_put_compressi
From: |
Li, Liang Z |
Subject: |
Re: [Qemu-devel] [PATCH RESEND v2 1/2] qemu-file: Fix qemu_put_compression_data flaw |
Date: |
Wed, 24 Feb 2016 02:01:46 +0000 |
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