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

["Andreas Färber" <afaerber@suse.de>]

Am 28.05.2013 04:50, schrieb qiaonuohan@cn.fujitsu.com:
> From: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
> 
> Struct dump_bitmap is associated with a tmp file which is used to store bitmap
> in kdump-compressed format temporarily. The following patch will use these
> functions to gather data of bitmap and cache them into tmp files.
> 
> Signed-off-by: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
> Reviewed-by: Zhang Xiaohe <zhangxh@cn.fujitsu.com>
> ---
>  Makefile.target       |    1 +
>  dump_bitmap.c         |  171 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/dump_bitmap.h |   60 +++++++++++++++++
>  3 files changed, 232 insertions(+), 0 deletions(-)
>  create mode 100644 dump_bitmap.c
>  create mode 100644 include/dump_bitmap.h

Wouldn't this fit better in include/sysemu/?

> diff --git a/Makefile.target b/Makefile.target
> index ce4391f..8e557f9 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -110,6 +110,7 @@ obj-y += qtest.o
>  obj-y += hw/
>  obj-$(CONFIG_FDT) += device_tree.o
>  obj-$(CONFIG_KVM) += kvm-all.o
> +obj-y += dump_bitmap.o
>  obj-y += memory.o savevm.o cputlb.o
>  obj-$(CONFIG_HAVE_GET_MEMORY_MAPPING) += memory_mapping.o
>  obj-$(CONFIG_HAVE_CORE_DUMP) += dump.o

My refactoring was merged, so this may conflict, although --3way
resolution should work.

> diff --git a/dump_bitmap.c b/dump_bitmap.c
> new file mode 100644
> index 0000000..f35f39d
> --- /dev/null
> +++ b/dump_bitmap.c
> @@ -0,0 +1,171 @@
> +/*
> + * QEMU dump bitmap
> + *
> + * Copyright (C) 2013 FUJITSU LIMITED
> + *
> + * Authors:
> + *     Qiao Nuohan <qiaonuohan@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.
> + *
> + */
> +
> +#include "qemu-common.h"
> +#include "dump_bitmap.h"
> +
> +int init_dump_bitmap(struct dump_bitmap *db, const char *filename)
> +{
> +    int fd;
> +    char *tmpname;
> +
> +    /* init the tmp file */
> +    tmpname = getenv("TMPDIR");
> +    if (!tmpname) {
> +        tmpname = (char *)P_tmpdir;
> +    }
> +
> +    db->file_name = (char *)g_strdup_printf("%s/%s", tmpname, filename);
> +
> +    fd = mkstemp(db->file_name);
> +    if (fd < 0) {
> +        return -1;

Given that all this is going to be used from a QMP command, I think it
were better to make the function return void and instead supply an Error
**errp argument that can return a textual error that can then be
propagated on.

> +    }
> +
> +    db->fd = fd;
> +    unlink(db->file_name);
> +
> +    /* num_block should be set to -1, for nothing is store in buf now */
> +    db->num_block = -1;
> +    memset(db->buf, 0, BUFSIZE_BITMAP);
> +    /* the tmp file starts to write at the very beginning */
> +    db->offset = 0;
> +
> +    return 0;
> +}
> +
> +int clear_dump_bitmap(struct dump_bitmap *db, size_t len_db)
> +{
> +    int ret;
> +    char buf[BUFSIZE_BITMAP];
> +    off_t offset_bit;
> +
> +    /* use buf to clear the tmp file block by block */
> +    memset(buf, 0, sizeof(buf));
> +
> +    ret = lseek(db->fd, db->offset, SEEK_SET);
> +    if (ret < 0) {
> +        return -1;

Same here.

> +    }
> +
> +    offset_bit = 0;
> +    while (offset_bit < len_db) {
> +        if (write(db->fd, buf, BUFSIZE_BITMAP) != BUFSIZE_BITMAP) {
> +            return -1;
> +        }
> +
> +        offset_bit += BUFSIZE_BITMAP;
> +    }
> +
> +    return 0;
> +}
> +
> +int set_dump_bitmap(struct dump_bitmap *db, unsigned long long pfn, int val)
> +{
> +    int byte, bit;
> +    off_t old_offset, new_offset;
> +    old_offset = db->offset + BUFSIZE_BITMAP * db->num_block;
> +    new_offset = db->offset + BUFSIZE_BITMAP * (pfn / PFN_BUFBITMAP);
> +
> +    /*
> +     * if the block needed to be set is not same as the one cached in buf,
> +     * write the block back to the tmp file, then cache new block to the buf
> +     */
> +    if (0 <= db->num_block && old_offset != new_offset) {
> +        if (lseek(db->fd, old_offset, SEEK_SET) < 0) {
> +            return -1;

Ditto.

> +        }
> +
> +        if (write(db->fd, db->buf, BUFSIZE_BITMAP) != BUFSIZE_BITMAP) {
> +            return -1;
> +        }
> +    }
> +
> +    if (old_offset != new_offset) {
> +        if (lseek(db->fd, new_offset, SEEK_SET) < 0) {
> +            return -1;
> +        }
> +
> +        if (read(db->fd, db->buf, BUFSIZE_BITMAP) != BUFSIZE_BITMAP) {
> +            return -1;
> +        }
> +
> +        db->num_block = pfn / PFN_BUFBITMAP;
> +    }
> +
> +    /* get the exact place of the bit in the buf, set it */
> +    byte = (pfn % PFN_BUFBITMAP) >> 3;
> +    bit  = (pfn % PFN_BUFBITMAP) & 7;
> +    if (val) {
> +        db->buf[byte] |= 1<<bit;
> +    } else {
> +        db->buf[byte] &= ~(1<<bit);
> +    }
> +
> +    return 0;
> +}
> +
> +int sync_dump_bitmap(struct dump_bitmap *db)
> +{
> +    off_t offset;
> +    offset = db->offset + BUFSIZE_BITMAP * db->num_block;
> +
> +    /* db has not been used yet */
> +    if (db->num_block < 0) {
> +        return 0;
> +    }
> +
> +    if (lseek(db->fd, offset, SEEK_SET) < 0) {
> +        return -1;

Ditto.

> +    }
> +
> +    if (write(db->fd, db->buf, BUFSIZE_BITMAP) != BUFSIZE_BITMAP) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * check if the bit is set to 1
> + */
> +static inline int is_on(char *bitmap, int i)

Return bool?

> +{
> +    return bitmap[i>>3] & (1 << (i & 7));
> +}
> +
> +int is_bit_set(struct dump_bitmap *db, unsigned long long pfn)

bool?

> +{
> +    off_t offset;
> +
> +    /* cache the block pfn belongs to, then check the block */
> +    if (pfn == 0 || db->num_block != pfn/PFN_BUFBITMAP) {
> +        offset = db->offset + BUFSIZE_BITMAP * (pfn/PFN_BUFBITMAP);
> +        lseek(db->fd, offset, SEEK_SET);
> +        read(db->fd, db->buf, BUFSIZE_BITMAP);
> +        db->num_block = pfn/PFN_BUFBITMAP;
> +    }
> +
> +    return is_on(db->buf, pfn%PFN_BUFBITMAP);

Coding Style asks for spaces around / and % operators.

> +}
> +
> +void free_dump_bitmap(struct dump_bitmap *db)
> +{
> +    if (db) {
> +        if (db->file_name) {
> +            g_free(db->file_name);
> +        }

The if seems unnecessary, g_free(NULL) should work fine.

> +
> +        g_free(db);
> +    }
> +}
> diff --git a/include/dump_bitmap.h b/include/dump_bitmap.h
> new file mode 100644
> index 0000000..f81106c
> --- /dev/null
> +++ b/include/dump_bitmap.h
> @@ -0,0 +1,60 @@
> +/*
> + * QEMU dump bitmap
> + *
> + * Copyright (C) 2013 FUJITSU LIMITED
> + *
> + * Authors:
> + *     Qiao Nuohan <qiaonuohan@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.
> + *
> + */
> +
> +#ifndef DUMP_BITMAP_H
> +#define DUMP_BITMAP_H
> +
> +#define BUFSIZE_BITMAP              (4096)
> +#define PFN_BUFBITMAP               (CHAR_BIT * BUFSIZE_BITMAP)
> +
> +struct dump_bitmap {
> +    int fd;                     /* fd of the tmp file */
> +    int num_block;              /* number of blocks cached in buf */
> +    char *file_name;            /* name of the tmp file */
> +    char buf[BUFSIZE_BITMAP];   /* used to cache blocks */
> +    off_t offset;               /* offset of the tmp file */
> +};

Please use CamelCase for the struct name, and please supply a typedef
and use that below.

Regards,
Andreas

> +
> +/*
> + * create a tmp file used to store dump bitmap, then init buf of dump_bitmap
> + */
> +int init_dump_bitmap(struct dump_bitmap *db, const char *filename);
> +
> +/*
> + * clear the content of the tmp file with all bits set to 0
> + */
> +int clear_dump_bitmap(struct dump_bitmap *db, size_t len_db);
> +
> +/*
> + * 'pfn' is the number of bit needed to be set, use buf to cache the block which
> + * 'pfn' belongs to, then set 'val' to the bit
> + */
> +int set_dump_bitmap(struct dump_bitmap *db, unsigned long long pfn, int val);
> +
> +/*
> + * when buf is caching a block, sync_dump_bitmap is needed to write the cached
> + * block to the tmp file
> + */
> +int sync_dump_bitmap(struct dump_bitmap *db);
> +
> +/*
> + * check whether 'pfn' is set
> + */
> +int is_bit_set(struct dump_bitmap *db, unsigned long long pfn);
> +
> +/*
> + * free the space used by dump_bitmap
> + */
> +void free_dump_bitmap(struct dump_bitmap *db);
> +
> +#endif
> 


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