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

[qiaonuohan]

> -----Original Message-----
> From: Eric Blake [mailto:address@hidden
> Sent: Wednesday, May 08, 2013 12:14 AM
> To: Qiao Nuohan
> Cc: address@hidden
> Subject: Re: [Qemu-devel] [PATCH 1/9] Add API to manipulate dump_bitmap
> 
> 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().

I will fix it as Daniel suggested.

> 
> > +++ 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.

Got it.

Thanks for your comments.

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