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

[Juan Quintela]

"Li, Liang Z" <address@hidden> wrote:
> On %D, %SN wrote:
> %Q
>
> %C

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

>
> 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?

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.