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>