Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] use g_free, instead of free

[Andreas Färber]

Am 01.11.2011 07:21, schrieb Dong Xu Wang:
> From: Dong Xu Wang <address@hidden>
> 
> Fix mismatching allocation and deallocation: g_free should be used to pair 
> with g_malloc.
> Also fix coding style.
> 
> Signed-off-by: Dong Xu Wang <address@hidden>

I took the time to go through the changes. Me, I would've preferred this
to be two patches (one cleanup, one fix), since the style changes make
up the majority of this patch... Two style changes are missing for
perfection, cf. inline.

Changelog is missing. Did just the description change since v1? In that
case Ray Wang's Reviewed-by is missing. Otherwise please describe!

Trusting Ray that g_free() was right in the first place,

Reviewed-by: Andreas Färber <address@hidden>

> ---
>  block/cloop.c |  119 
> +++++++++++++++++++++++++++++++--------------------------
>  1 files changed, 65 insertions(+), 54 deletions(-)
> 
> diff --git a/block/cloop.c b/block/cloop.c
> index 775f8a9..1884b8c 100644
> --- a/block/cloop.c
> +++ b/block/cloop.c

> @@ -74,26 +76,28 @@ static int cloop_open(BlockDriverState *bs, int flags)
>      s->offsets = g_malloc(offsets_size);
>      if (bdrv_pread(bs->file, 128 + 4 + 4, s->offsets, offsets_size) <
>              offsets_size) {
> -     goto cloop_close;
> +        goto cloop_close;
>      }
>      for(i=0;i<s->n_blocks;i++) {
> -     s->offsets[i]=be64_to_cpu(s->offsets[i]);
> -     if(i>0) {
> -         uint32_t size=s->offsets[i]-s->offsets[i-1];
> -         if(size>max_compressed_block_size)
> -             max_compressed_block_size=size;
> -     }
> +        s->offsets[i] = be64_to_cpu(s->offsets[i]);
> +        if (i > 0) {
> +            uint32_t size = s->offsets[i] - s->offsets[i-1];

i - 1 theoretically

> +            if (size > max_compressed_block_size) {
> +                max_compressed_block_size = size;
> +            }
> +        }
>      }
>  
>      /* initialize zlib engine */
> -    s->compressed_block = g_malloc(max_compressed_block_size+1);
> +    s->compressed_block = g_malloc(max_compressed_block_size + 1);
>      s->uncompressed_block = g_malloc(s->block_size);
> -    if(inflateInit(&s->zstream) != Z_OK)
> -     goto cloop_close;
> -    s->current_block=s->n_blocks;
> +    if (inflateInit(&s->zstream) != Z_OK) {
> +        goto cloop_close;
> +    }
> +    s->current_block = s->n_blocks;
>  
>      s->sectors_per_block = s->block_size/512;
> -    bs->total_sectors = s->n_blocks*s->sectors_per_block;
> +    bs->total_sectors = s->n_blocks * s->sectors_per_block;
>      qemu_co_mutex_init(&s->lock);
>      return 0;
>  
> @@ -105,27 +109,30 @@ static inline int cloop_read_block(BlockDriverState 
> *bs, int block_num)
>  {
>      BDRVCloopState *s = bs->opaque;
>  
> -    if(s->current_block != block_num) {
> -     int ret;
> -        uint32_t bytes = s->offsets[block_num+1]-s->offsets[block_num];
> +    if (s->current_block != block_num) {
> +        int ret;
> +        uint32_t bytes = s->offsets[block_num + 1]-s->offsets[block_num];

] - s

>  
>          ret = bdrv_pread(bs->file, s->offsets[block_num], 
> s->compressed_block,
>                           bytes);
> -        if (ret != bytes)
> +        if (ret != bytes) {
>              return -1;
> +        }
> +
> +        s->zstream.next_in = s->compressed_block;
> +        s->zstream.avail_in = bytes;
> +        s->zstream.next_out = s->uncompressed_block;
> +        s->zstream.avail_out = s->block_size;
> +        ret = inflateReset(&s->zstream);
> +        if (ret != Z_OK) {
> +            return -1;
> +        }
> +        ret = inflate(&s->zstream, Z_FINISH);
> +        if (ret != Z_STREAM_END || s->zstream.total_out != s->block_size) {
> +            return -1;
> +        }
>  
> -     s->zstream.next_in = s->compressed_block;
> -     s->zstream.avail_in = bytes;
> -     s->zstream.next_out = s->uncompressed_block;
> -     s->zstream.avail_out = s->block_size;
> -     ret = inflateReset(&s->zstream);
> -     if(ret != Z_OK)
> -         return -1;
> -     ret = inflate(&s->zstream, Z_FINISH);
> -     if(ret != Z_STREAM_END || s->zstream.total_out != s->block_size)
> -         return -1;
> -
> -     s->current_block = block_num;
> +        s->current_block = block_num;
>      }
>      return 0;
>  }

> @@ -160,20 +170,21 @@ static coroutine_fn int cloop_co_read(BlockDriverState 
> *bs, int64_t sector_num,
>  static void cloop_close(BlockDriverState *bs)
>  {
>      BDRVCloopState *s = bs->opaque;
> -    if(s->n_blocks>0)
> -     free(s->offsets);
> -    free(s->compressed_block);
> -    free(s->uncompressed_block);
> +    if (s->n_blocks > 0) {
> +        g_free(s->offsets);
> +    }
> +    g_free(s->compressed_block);
> +    g_free(s->uncompressed_block);

Here are the 3 functional changes!

>      inflateEnd(&s->zstream);
>  }
>  
>  static BlockDriver bdrv_cloop = {
> -    .format_name     = "cloop",
> -    .instance_size   = sizeof(BDRVCloopState),
> -    .bdrv_probe              = cloop_probe,
> -    .bdrv_open               = cloop_open,
> -    .bdrv_read          = cloop_co_read,
> -    .bdrv_close              = cloop_close,
> +    .format_name    = "cloop",
> +    .instance_size  = sizeof(BDRVCloopState),
> +    .bdrv_probe     = cloop_probe,
> +    .bdrv_open      = cloop_open,
> +    .bdrv_read      = cloop_co_read,
> +    .bdrv_close     = cloop_close,
>  };
>  
>  static void bdrv_cloop_init(void)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg