Re: [Qemu-devel] [PATCH 2/3] qcow2: refactor data compression

[Kevin Wolf <kwolf@redhat.com>]

Am 08.06.2018 um 21:20 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Make a separate function for compression to be parallelized later.
>  - use .avail_aut field instead of .next_out to calculate size of

s/avail_aut/avail_out/

>    compressed data. It looks more natural and it allows to keep dest to
>    be void pointer
>  - set avail_out to be at least one byte less than input, to be sure
>    avoid inefficient compression earlier
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

>  block/qcow2.c | 74 +++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 47 insertions(+), 27 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 549fee9b69..d4dbe329ab 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -22,11 +22,13 @@
>   * THE SOFTWARE.
>   */
>  
> +#define ZLIB_CONST
> +#include <zlib.h>

The first #include must always be "qemu/osdep.h". If you want to
separate zlib.h from the internal headers, you can move it down instead.

>  #include "qemu/osdep.h"
>  #include "block/block_int.h"
>  #include "sysemu/block-backend.h"
>  #include "qemu/module.h"
> -#include <zlib.h>
>  #include "qcow2.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
> @@ -3674,6 +3676,45 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset,
>      return 0;
>  }
>  
> +/* qcow2_compress()

The first line of the comment should contain only the /*

> + *
> + * @dest - destination buffer, at least of @size-1 bytes
> + * @src - source buffer, @size bytes
> + *
> + * Returns: compressed size on success
> + *          -1 if compression is inefficient
> + *          -2 on any other error
> + */

The logic looks fine.

Initially I intended to request splitting the code motion from the
changes, but I see that this would probably only make things more
complicated, so I'm okay with leaving that as it is.

Kevin