[v4,2/9] dump: Add API to manipulate cache_data

From: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>

Struct cache_data is associated with a tmp file which is used to store page
desc and page data in kdump-compressed format temporarily. The following patch
will use these function to gather data of page and cache them in tmp files.

Signed-off-by: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
Reviewed-by: Zhang Xiaohe <zhangxh@cn.fujitsu.com>
---
 Makefile.target      |    2 +-
 cache_data.c         |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/cache_data.h |   56 +++++++++++++++++++++++
 3 files changed, 178 insertions(+), 1 deletions(-)
 create mode 100644 cache_data.c
 create mode 100644 include/cache_data.h

Am 28.05.2013 04:50, schrieb qiaonuohan@cn.fujitsu.com:
> From: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
> 
> Struct cache_data is associated with a tmp file which is used to store page
> desc and page data in kdump-compressed format temporarily.

CacheData please - but I do find the English term "cache data"
irritating as it sounds like data about a cache when instead it is about
cached data. DataCache? CachedData? Maybe a native English speaker can
advise?

> The following patch
> will use these function to gather data of page and cache them in tmp files.
> 
> Signed-off-by: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
> Reviewed-by: Zhang Xiaohe <zhangxh@cn.fujitsu.com>
> ---
>  Makefile.target      |    2 +-
>  cache_data.c         |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/cache_data.h |   56 +++++++++++++++++++++++
>  3 files changed, 178 insertions(+), 1 deletions(-)
>  create mode 100644 cache_data.c
>  create mode 100644 include/cache_data.h
> 
> diff --git a/Makefile.target b/Makefile.target
> index 8e557f9..298ec84 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -110,7 +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 += dump_bitmap.o cache_data.o

Same comment as for dump_bitmap.o: Can we build this only once, please?

>  obj-y += memory.o savevm.o cputlb.o
>  obj-$(CONFIG_HAVE_GET_MEMORY_MAPPING) += memory_mapping.o
>  obj-$(CONFIG_HAVE_CORE_DUMP) += dump.o
> diff --git a/cache_data.c b/cache_data.c
> new file mode 100644
> index 0000000..6e91538
> --- /dev/null
> +++ b/cache_data.c
> @@ -0,0 +1,121 @@
> +/*
> + * QEMU cache data
> + *
> + * 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 "cache_data.h"
> +
> +int init_cache_data(struct cache_data *cd, const char *filename)
> +{
> +    int fd;
> +    char *tmpname;
> +
> +    /* init the tmp file */
> +    tmpname = getenv("TMPDIR");
> +    if (!tmpname) {
> +        tmpname = (char *)P_tmpdir;

P_tmpdir is marked obsolescent in Open Group spec 7. Maybe Erik can
comment some more? Did you verify it builds with mingw32/64?
(I stumbled over it because I found the variable name odd and didn't see
it defined anywhere.)

> +    }
> +
> +    cd->file_name = (char *)g_strdup_printf("%s/%s", tmpname, filename);
> +
> +    fd = mkstemp(cd->file_name);
> +    if (fd < 0) {
> +        return -1;

Error **errp? Same below.

> +    }
> +
> +    cd->fd = fd;
> +    unlink(cd->file_name);
> +
> +    /* init buf */
> +    cd->buf_size = BUFSIZE_CACHE_DATA;
> +    cd->cache_size = 0;
> +    cd->buf = g_malloc0(BUFSIZE_CACHE_DATA);
> +
> +    cd->offset = 0;
> +
> +    return 0;
> +}

I wonder if it makes sense to introduce this interface instead of going
through the block layer, which would offer a number of different
backends at least. CC'ing the experts.

> +
> +int write_cache(struct cache_data *cd, void *buf, size_t size)
> +{
> +    /*
> +     * check if the space is enough to cache data, if not write cached
> +     * data to the tmp file
> +     */
> +    if (cd->cache_size + size > cd->buf_size) {
> +        if (lseek(cd->fd, cd->offset, SEEK_SET) < 0) {
> +            return -1;
> +        }
> +
> +        if (write(cd->fd, cd->buf, cd->cache_size) != cd->cache_size) {
> +            return -1;
> +        }
> +
> +        cd->offset += cd->cache_size;
> +        cd->cache_size = 0;
> +    }
> +
> +    memcpy(cd->buf + cd->cache_size, buf, size);
> +    cd->cache_size += size;
> +
> +    return 0;
> +}
> +
> +int sync_cache(struct cache_data *cd)
> +{
> +    /* no data is cached in cache_data */
> +    if (cd->cache_size == 0) {
> +        return 0;
> +    }
> +
> +    if (lseek(cd->fd, cd->offset, SEEK_SET) < 0) {
> +        return -1;
> +    }
> +
> +    if (write(cd->fd, cd->buf, cd->cache_size) != cd->cache_size) {
> +        return -1;
> +    }
> +
> +    cd->offset += cd->cache_size;
> +
> +    return 0;
> +}
> +
> +int read_cache(struct cache_data *cd)
> +{
> +    if (lseek(cd->fd, cd->offset, SEEK_SET) < 0) {
> +        return -1;
> +    }
> +
> +    if (read(cd->fd, cd->buf, cd->cache_size) != cd->cache_size) {
> +        return -1;
> +    }
> +
> +    cd->offset += cd->cache_size;
> +
> +    return 0;
> +}
> +
> +void free_cache_data(struct cache_data *cd)
> +{
> +    if (cd) {
> +        if (cd->file_name) {
> +            g_free(cd->file_name);
> +        }
> +
> +        if (cd->buf) {
> +            g_free(cd->buf);
> +        }
> +
> +        g_free(cd);
> +    }
> +}
> diff --git a/include/cache_data.h b/include/cache_data.h
> new file mode 100644
> index 0000000..18e8680
> --- /dev/null
> +++ b/include/cache_data.h
> @@ -0,0 +1,56 @@
> +/*
> + * QEMU cache data
> + *
> + * 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 CACHE_DATA_H
> +#define CACHE_DATA_H
> +
> +#define BUFSIZE_CACHE_DATA          (4096 * 4)
> +
> +struct cache_data {
> +    int fd;             /* fd of the tmp file used to store cache data */
> +    char *file_name;    /* name of the tmp file */
> +    char *buf;          /* used to cache data */
> +    size_t buf_size;    /* size of the buf */
> +    size_t cache_size;  /* size of cached data in buf */
> +    off_t offset;       /* offset of the tmp file */
> +};

CamelCase and typedef please.

Regards,
Andreas

> +
> +/*
> + * create a tmp file used to store cache data, then init the buf
> + */
> +int init_cache_data(struct cache_data *cd, const char *filename);
> +
> +/*
> + * write data to the tmp file, the data may first be cached in the buf of
> + * cache_data
> + */
> +int write_cache(struct cache_data *cd, void *buf, size_t size);
> +
> +/*
> + * when cache_data is caching data in the buf, sync_cache is needed to write the
> + * data back to tmp file
> + */
> +int sync_cache(struct cache_data *cd);
> +
> +/*  read data from the tmp file to the buf of 'cd', the start place is set by
> + *  cd->offset, and the size is set by cd->cache_size. cd->offset is changed
> + *  automaticlly according to the size of data read this time.
> + */
> +int read_cache(struct cache_data *cd);
> +
> +/*
> + * free the space used by cache_data
> + */
> +void free_cache_data(struct cache_data *cd);
> +
> +#endif
>

On 06/19/2013 01:29 PM, Andreas Färber wrote:

>> +int init_cache_data(struct cache_data *cd, const char *filename)
>> +{
>> +    int fd;
>> +    char *tmpname;
>> +
>> +    /* init the tmp file */
>> +    tmpname = getenv("TMPDIR");
>> +    if (!tmpname) {
>> +        tmpname = (char *)P_tmpdir;
> 
> P_tmpdir is marked obsolescent in Open Group spec 7. Maybe Erik can

s/Erik/Eric/ (but don't worry, you're not the first to make that typo)

Hmm, you are correct that tempnam() is marked as an obsolescent
interface (because it has the same security flaws as mktemp(); the
standard introduced mkstemp() to overcome the security hole but did not
add a replacement for tempnam()).  I guess since nothing else in the
standard refers to P_tmpdir, it was also marked obsolecent.  And since
C99 doesn't require either the constant or the (inherently broken)
tempnam(), it may be safer to guard this line by #ifdef P_tmpdir, rather
than assuming that <stdio.h> blindly provides it.

> comment some more? Did you verify it builds with mingw32/64?
> (I stumbled over it because I found the variable name odd and didn't see
> it defined anywhere.)
> 
>> +    }
>> +
>> +    cd->file_name = (char *)g_strdup_printf("%s/%s", tmpname, filename);
>> +
>> +    fd = mkstemp(cd->file_name);

At least your use of P_tmpdir was to generate a saner template, instead
of trying to use the inherently-broken tempnam().