Re: [Qemu-devel] [PATCH 2/8] migration: stop allocating and freeingmemory frequently

[jiang.biao2]

Hi, guangrong
> 
> +/* return the size after compression, or negative value on error */
> +static int qemu_compress_data(z_stream *stream, uint8_t *dest, size_t 
> dest_len,
> +                              const uint8_t *source, size_t source_len)
> +{
> +    int err;
> +
> +    err = deflateReset(stream);
> +    if (err != Z_OK) {
> +        return -1;
> +    }
> +
> +    stream->avail_in = source_len;
> +    stream->next_in = (uint8_t *)source;
> +    stream->avail_out = dest_len;
> +    stream->next_out = dest;
>+
duplicated code with qemu_uncompress(), would initializing stream outside 
of qemu_compress_data() be better? In that case, we could pass much less
parameters down, and avoid the duplicated code. Or could we encapsulate 
some struct to ease the case?
> +    err = deflate(stream, Z_FINISH);
> +    if (err != Z_STREAM_END) {
> +        return -1;
> +    }
> +
> +    return stream->next_out - dest;
> +}
> +
> 
> @@ -683,8 +707,10 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const 
> uint8_t *p, size_t size,
> return -1;
> }
> }
> -    if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
> -                  (Bytef *)p, size, level) != Z_OK) {
> +
> +    blen = qemu_compress_data(stream, f->buf + f->buf_index + 
> sizeof(int32_t),
> +                              blen, p, size);
The "level" parameter is never used after the patch, could we just removed it? 
On the other hand, deflate() of zlib supports compression level too(by 
deflateInit(stream, level)), should we just reuse the level properly?  If not, 
the 
*migrate parameter compress_level* will be useless. 
> +    if (blen < 0) {
> error_report("Compress Failed!");
> return 0;
> }
>
> +/* return the size after decompression, or negative value on error */
> +static int qemu_uncompress(z_stream *stream, uint8_t *dest, size_t dest_len,
> +                           uint8_t *source, size_t source_len)
The name of *qemu_uncompress* does not quite match *qemu_compress_data*,
would *qemu_uncompress_data* be better?
Besides, the prototype is not consistent with  *qemu_compress_data* either, 
should the -*source- be -const- also here?
> +{
> +    int err;
> +
> +    err = inflateReset(stream);
> +    if (err != Z_OK) {
> +        return -1;
> +    }
> +
> +    stream->avail_in = source_len;
> +    stream->next_in = source;
> +    stream->avail_out = dest_len;
> +    stream->next_out = dest;
> +
> +    err = inflate(stream, Z_NO_FLUSH);
> +    if (err != Z_STREAM_END) {
> +        return -1;
> +    }
> +
> +    return stream->total_out;
> +}
> +

Jiang
Regards,