comments below
On 01/05/14 08:27, Qiao Nuohan wrote:
DataCache is used to store data temporarily, then the data will be written to
vmcore. These functions will be called later when writing data of page to
vmcore.
Signed-off-by: Qiao Nuohan qiaonuo...@cn.fujitsu.com
---
dump.c| 52
+
include/sysemu/dump.h |9
2 files changed, 61 insertions(+), 0 deletions(-)
diff --git a/dump.c b/dump.c
index 1fae152..b4c40f2 100644
--- a/dump.c
+++ b/dump.c
@@ -1088,6 +1088,58 @@ out:
return ret;
}
+static void prepare_data_cache(DataCache *data_cache, DumpState *s)
+{
+data_cache-fd = s-fd;
+data_cache-data_size = 0;
+data_cache-buf_size = BUFSIZE_DATA_CACHE;
+data_cache-buf = g_malloc0(BUFSIZE_DATA_CACHE);
+}
+
+static int write_cache(DataCache *dc, bool flag_flatten, void *buf, size_t
size)
+{
+/*
+ * check if the space is enough to cache data, if not, write the cached
+ * data to dc-fd and reset the buf
+ */
+if (dc-data_size + size dc-buf_size) {
+if (write_buffer(dc-fd, flag_flatten, dc-offset, dc-buf,
+ dc-data_size) 0) {
+return -1;
+}
+
+dc-offset += dc-data_size;
+dc-data_size = 0;
+}
+
+memcpy(dc-buf + dc-data_size, buf, size);
+dc-data_size += size;
+
+return 0;
+}
I think we should at least add a check (if or assert()) because the
passed-in size could be greater than all of the room we have in the
buffer (ie. size dc-buf_size), and in that case we'd overflow the buffer.
Then,
+
+/* write the remaining data in dc-buf to dc-fd */
+static int sync_data_cache(DataCache *dc, bool flag_flatten)
+{
+if (dc-data_size == 0) {
+return 0;
+}
+
+if (write_buffer(dc-fd, flag_flatten, dc-offset, dc-buf,
+ dc-data_size) 0) {
+return -1;
+}
+
+dc-offset += dc-data_size;
+
+return 0;
+}
Incrementing the offset here, but not resetting dc-data_size, seems
inconsistent. Do both or neither. Ideally, do both, and rebase
write_cache() on top of this (ie. when syncing is necessary, call
sync_data_cache() from write_cache()).
It doesn't cause problems as-is in the current patchset though.
+
+static void free_data_cache(DataCache *data_cache)
+{
+g_free(data_cache-buf);
+}
+
static ram_addr_t get_start_block(DumpState *s)
{
GuestPhysBlock *block;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index b5eaf8d..ab44af8 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -36,6 +36,7 @@
#define BUFSIZE_BITMAP (PAGE_SIZE)
#define PFN_BUFBITMAP (CHAR_BIT * BUFSIZE_BITMAP)
#define ARCH_PFN_OFFSET (0)
+#define BUFSIZE_DATA_CACHE (PAGE_SIZE * 4)
#define paddr_to_pfn(X, page_shift) \
(((unsigned long long)(X) (page_shift)) - ARCH_PFN_OFFSET)
@@ -140,6 +141,14 @@ typedef struct QEMU_PACKED KdumpSubHeader64 {
uint64_t max_mapnr_64; /* header_version 6 and later */
} KdumpSubHeader64;
+typedef struct DataCache {
+int fd; /* fd of the file where to write the cached data */
+char *buf; /* buffer for cached data */
+size_t buf_size;/* size of the buf */
+size_t data_size; /* size of cached data in buf */
+off_t offset; /* offset of the file */
+} DataCache;
+
struct GuestPhysBlockList; /* memory_mapping.h */
int cpu_get_dump_info(ArchDumpInfo *info,
const struct GuestPhysBlockList *guest_phys_blocks);
I feel that stuff that depends on page size should be centralized
somehow. I can't describe it very well now, but I feel that having a
bunch of macros that open-code the page size as 4096, and using struct
members elsewhere (with dynamically set values) for the same purpose, is
a mess.
However that could be refactored in a separate series, *if* you think it
would be worthwhile.
Reviewed-by: Laszlo Ersek ler...@redhat.com
