Re: [Qemu-devel] [PATCH 4/5] qemu-file: Fix qemu_put_compression_data flaw

[Juan Quintela]

Liang Li <address@hidden> wrote:
> Current qemu_put_compression_data can only work with no writable
> QEMUFile, and can't work with the writable QEMUFile. But it does
> not provide any measure to prevent users from using it with a
> writable QEMUFile.
>
> We should fix this flaw to make it works with writable QEMUFile.
>
> Suggested-by: Juan Quintela <address@hidden>
> Signed-off-by: Liang Li <address@hidden>

Code is not enough.

> ---
>  migration/qemu-file.c | 23 +++++++++++++++++++++--
>  migration/ram.c       |  6 +++++-
>  2 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 6f4a129..b0ef1f3 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -607,8 +607,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 -1 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 -1.
>   */
>  
>  ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
> @@ -617,7 +623,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 -1;
> +        }

I can't yet understand why we can be writting to a qemu_file that is not 
writable.
But well, no harm.


> +        qemu_fflush(f);
> +        blen = IO_BUF_SIZE - sizeof(int32_t);
> +        if (blen < compressBound(size)) {
> +            return -1;
> +        }
>      }
>      if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
>                    (Bytef *)p, size, level) != Z_OK) {
> @@ -625,7 +638,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);
> +    }



> +    }
>      return blen + sizeof(int32_t);
>  }
>  

Ok with the changes in this function.


> diff --git a/migration/ram.c b/migration/ram.c
> index bc34bc5..7e62d8d 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -821,7 +821,11 @@ static int do_compress_ram_page(CompressParam *param)
>                                    RAM_SAVE_FLAG_COMPRESS_PAGE);
>      blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE,
>                                       migrate_compress_level());
> -    bytes_sent += blen;
> +    if (blen < 0) {
> +        error_report("Insufficient buffer for compressed data!");
> +    } else {
> +        bytes_sent += blen;
> +    }
>  
>      return bytes_sent;
>  }


Are you sure that this code is ok?

1st- there are two callers od do_compress_ram_page(), only one of it
     uses the return value
2nd- if we were not able to write the compressed page, we continue
     without a single error.  I think that if blen is < 0, we should
     just abort the migration at this point (we have just sent a header
     and we haven't been able to send the contents under that header.
     Without lot of changes, I can't see how to fix it).

Thanks, Juan.