> -----Original Message-----
> From: Eric Blake [mailto:ebl...@redhat.com]
> Sent: Wednesday, May 08, 2013 12:14 AM
> To: Qiao Nuohan
> Cc: qemu-devel@nongnu.org
> 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 <qiaonuo...@cn.fujitsu.com>
> > Reviewed-by: Zhang Xiaohe <zhan...@cn.fujitsu.com>
> > ---
>
> > + 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 <qiaonuo...@cn.fujitsu.com>
> > + *
> > + * 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
