Re: [Qemu-devel] [PATCH 1/9] Add API to manipulate dump_bitmap

[Eric Blake]

On 05/07/2013 01:16 AM, Qiao Nuohan wrote:
> Struct dump_bitmap is associated with a tmp file, and the tmp file can be used
> to save data of bitmap in kdump-compressed format temporarily.
> The following patch will use these functions to get the data of bitmap and 
> cache
> them into tmp files.
> 
> Signed-off-by: Qiao Nuohan <address@hidden>
> Reviewed-by: Zhang Xiaohe <address@hidden>
> ---

> +    db->file_name = (char *)g_malloc(strlen(filename) + strlen(tmpname) + 1);
> +
> +    strcpy(db->file_name, tmpname);
> +    strcat(db->file_name, "/");
> +    strcat(db->file_name, filename);

Off-by-one buffer overflow, since you forgot space for the NUL byte.  We
use C, not C++, so you don't need to cast the result of g_malloc().

> +++ b/include/dump_bitmap.h
> @@ -0,0 +1,57 @@
> +/*
> + * QEMU dump bitmap
> + *
> + * Copyright Fujitsu, Corp. 2013
> + *
> + * Authors:
> + *     Qiao Nuohan <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +

No double-inclusion guard?

> +#define TMP_DIR                     "/tmp"

Why not reuse P_tmpdir from <stdio.h> instead of reinventing a new name
for this constant?

> +#define BITPERBYTE                  (8)

Why not use CHAR_BIT from <limits.h> instead of reinventing a new name
for this constant?

> +#define BUFSIZE_BITMAP              (4096)
> +#define PFN_BUFBITMAP               (BITPERBYTE * BUFSIZE_BITMAP)
> +
> +struct dump_bitmap {
> +    int fd;                     /* fd of the tmp file used to store dump 
> bitmap */
> +    int no_block;               /* number of block cached in buf */ 

Trailing whitespace.  Run your patch series through scripts/checkpatch.pl.

The name no_block sounds like there aren't any blocks.  You probably
want the name num_block instead.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature