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

[Juan Quintela <quintela@redhat.com>]

"Li, Liang Z" <liang.z.li@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.li@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?

(yes, caller of do_co_compress_ram_page() don't check for zero either).


>>  ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t
>> size, @@ -616,7 +622,14 @@ ssize_t
>> qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
>>      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
>> 
>>      if (blen < compressBound(size)) {
>> -        return 0;
>> +        if (!qemu_file_is_writable(f)) {
>> +            return 0;
>> +        }
>> +        qemu_fflush(f);
>> +        blen = IO_BUF_SIZE - sizeof(int32_t);
>> +        if (blen < compressBound(size)) {
>> +            return 0;
>> +        }
>>      }
>>      if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
>>                    (Bytef *)p, size, level) != Z_OK) { @@ -624,7 +637,13 @@ ssize_t
>> qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
>>          return 0;
>>      }
>>      qemu_put_be32(f, blen);
>> +    if (f->ops->writev_buffer) {
>> +        add_to_iovec(f, f->buf + f->buf_index, blen);
>> +    }
>>      f->buf_index += blen;
>> +    if (f->buf_index == IO_BUF_SIZE) {
>> +        qemu_fflush(f);
>> +    }
>>      return blen + sizeof(int32_t);

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.