remarks below
On 01/17/14 08:46, qiaonuohan wrote:
> the functions are used to write header of kdump-compressed format to vmcore.
> Header of kdump-compressed format includes:
> 1. common header: DiskDumpHeader32 / DiskDumpHeader64
> 2. sub header: KdumpSubHeader32 / KdumpSubHeader64
> 3. extra information: only elf notes here
>
> Signed-off-by: Qiao Nuohan <qiaonuo...@cn.fujitsu.com>
> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
> ---
> dump.c | 223
> +++++++++++++++++++++++++++++++++++++++++++++++++
> include/sysemu/dump.h | 96 +++++++++++++++++++++
> 2 files changed, 319 insertions(+), 0 deletions(-)
>
> diff --git a/dump.c b/dump.c
> index bf7d31d..a7fdc3f 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -778,6 +778,229 @@ static int buf_write_note(const void *buf, size_t size,
> void *opaque)
> return 0;
> }
>
> +/* write common header, sub header and elf note to vmcore */
> +static int create_header32(DumpState *s)
> +{
> + int ret = 0;
> + DiskDumpHeader32 *dh = NULL;
> + KdumpSubHeader32 *kh = NULL;
> + size_t size;
> + int endian = s->dump_info.d_endian;
> + uint32_t block_size;
> + uint32_t sub_hdr_size;
> + uint32_t bitmap_blocks;
> + uint32_t status = 0;
> + uint64_t offset_note;
> +
> + /* write common header, the version of kdump-compressed format is 6th */
thanks for fixing the comment
> + size = sizeof(DiskDumpHeader32);
> + dh = g_malloc0(size);
> +
> + strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
> + dh->header_version = cpu_convert_to_target32(6, endian);
> + block_size = s->page_size;
> + dh->block_size = cpu_convert_to_target32(block_size, endian);
> + sub_hdr_size = sizeof(struct KdumpSubHeader32) + s->note_size;
> + sub_hdr_size = DIV_ROUND_UP(sub_hdr_size, block_size);
> + dh->sub_hdr_size = cpu_convert_to_target32(sub_hdr_size, endian);
> + /* dh->max_mapnr may be truncated, full 64bit is in kh.max_mapnr_64 */
> + dh->max_mapnr = cpu_convert_to_target32(MIN(s->max_mapnr, UINT_MAX),
> + endian);
> + dh->nr_cpus = cpu_convert_to_target32(s->nr_cpus, endian);
> + bitmap_blocks = DIV_ROUND_UP(s->len_dump_bitmap, block_size) * 2;
> + dh->bitmap_blocks = cpu_convert_to_target32(bitmap_blocks, endian);
> + memcpy(&(dh->utsname.machine), "i686", 4);
> +
> + if (s->flag_compress & DUMP_DH_COMPRESSED_ZLIB) {
> + status |= DUMP_DH_COMPRESSED_ZLIB;
> + }
> +#ifdef CONFIG_LZO
> + if (s->flag_compress & DUMP_DH_COMPRESSED_LZO) {
> + status |= DUMP_DH_COMPRESSED_LZO;
> + }
> +#endif
> +#ifdef CONFIG_SNAPPY
> + if (s->flag_compress & DUMP_DH_COMPRESSED_SNAPPY) {
> + status |= DUMP_DH_COMPRESSED_SNAPPY;
> + }
> +#endif
> + dh->status = cpu_convert_to_target32(status, endian);
taking care about endianness looks reasonable to me thus far
> +
> + if (write_buffer(s->fd, 0, dh, size) < 0) {
> + dump_error(s, "dump: failed to write disk dump header.\n");
> + ret = -1;
> + goto out;
> + }
calling dump_error() for cleanup, looks good
(of course I'm not checking completeness, either for the endianness
question or cleanup-on-error coverage, but they do seem sound)
> +
> + /* write sub header */
> + size = sizeof(KdumpSubHeader32);
> + kh = g_malloc0(size);
> +
> + /* 64bit max_mapnr_64 */
> + kh->max_mapnr_64 = cpu_convert_to_target64(s->max_mapnr, endian);
> + kh->phys_base = cpu_convert_to_target32(PHYS_BASE, endian);
> + kh->dump_level = cpu_convert_to_target32(DUMP_LEVEL, endian);
> +
> + offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size;
> + kh->offset_note = cpu_convert_to_target64(offset_note, endian);
> + kh->note_size = cpu_convert_to_target32(s->note_size, endian);
looks good
> +
> + if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
> + block_size, kh, size) < 0) {
OK, flag_flatten is dropped. Plus the multiplication that I asked for is
being done; thanks.
> + dump_error(s, "dump: failed to write kdump sub header.\n");
> + ret = -1;
> + goto out;
> + }
dump_error() looks good
> +
> + /* write note */
> + s->note_buf = g_malloc0(s->note_size);
zeroing it out is now consistent with the 64-bit version
> + s->note_buf_offset = 0;
> +
> + /* use s->note_buf to store notes temporarily */
> + if (write_elf32_notes(buf_write_note, s) < 0) {
> + ret = -1;
> + goto out;
> + }
> +
> + if (write_buffer(s->fd, offset_note, s->note_buf,
> + s->note_size) < 0) {
> + dump_error(s, "dump: failed to write notes");
> + ret = -1;
> + goto out;
> + }
OK, you opted for the third option here, 'allocating "s->note_buf" with
g_malloc0()', thanks.
> +
> + /* get offset of dump_bitmap */
> + s->offset_dump_bitmap = (DISKDUMP_HEADER_BLOCKS + sub_hdr_size) *
> + block_size;
> +
> + /* get offset of page */
> + s->offset_page = (DISKDUMP_HEADER_BLOCKS + sub_hdr_size + bitmap_blocks)
> *
> + block_size;
> +
> +out:
> + g_free(dh);
> + g_free(kh);
> + g_free(s->note_buf);
> +
> + return ret;
> +}
The 32-bit version looks OK to me, the error handling too. Basically the
promise is that this function will have called dump_error() on error
exit. write_elf32_notes() does that internally (and we take it into
account correctly), for any other (ie. write_buffer()) errors we make
the call ourselves.
I also sought to verify that dh->foo and kh->bar fields are only
assigned-to, never read back and used for any calculation.
> +
> +/* write common header, sub header and elf note to vmcore */
> +static int create_header64(DumpState *s)
> +{
> + int ret = 0;
> + DiskDumpHeader64 *dh = NULL;
> + KdumpSubHeader64 *kh = NULL;
> + size_t size;
> + int endian = s->dump_info.d_endian;
> + uint32_t block_size;
> + uint32_t sub_hdr_size;
> + uint32_t bitmap_blocks;
> + uint32_t status = 0;
> + uint64_t offset_note;
> +
> + /* write common header, the version of kdump-compressed format is 6th */
thanks for fixing the comment
> + size = sizeof(DiskDumpHeader64);
> + dh = g_malloc0(size);
> +
> + strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
> + dh->header_version = cpu_convert_to_target32(6, endian);
> + block_size = s->page_size;
> + dh->block_size = cpu_convert_to_target32(block_size, endian);
> + sub_hdr_size = sizeof(struct KdumpSubHeader64) + s->note_size;
> + sub_hdr_size = DIV_ROUND_UP(sub_hdr_size, dh->block_size);
Unfortunately, this is a bug, introduced by the new endian-awareness.
You should divide here by "block_size", not by "dh->block_size", because
that's already converted to target endianness.
I found this problem by extracting the 32-bit and 64-bit versions of the
function (after applying this patch) to separate text files, and diffing
them against each other.
> + dh->sub_hdr_size = cpu_convert_to_target32(sub_hdr_size, endian);
> + /* dh->max_mapnr may be truncated, full 64bit is in kh.max_mapnr_64 */
> + dh->max_mapnr = cpu_convert_to_target32(MIN(s->max_mapnr, UINT_MAX),
> + endian);
> + dh->nr_cpus = cpu_convert_to_target32(s->nr_cpus, endian);
> + bitmap_blocks = DIV_ROUND_UP(s->len_dump_bitmap, dh->block_size) * 2;
Same problem as above: divisor should be host-endian.
> + dh->bitmap_blocks = cpu_convert_to_target32(bitmap_blocks, endian);
> + memcpy(&(dh->utsname.machine), "x86_64", 6);
> +
> + if (s->flag_compress & DUMP_DH_COMPRESSED_ZLIB) {
> + status |= DUMP_DH_COMPRESSED_ZLIB;
> + }
> +#ifdef CONFIG_LZO
> + if (s->flag_compress & DUMP_DH_COMPRESSED_LZO) {
> + status |= DUMP_DH_COMPRESSED_LZO;
> + }
> +#endif
> +#ifdef CONFIG_SNAPPY
> + if (s->flag_compress & DUMP_DH_COMPRESSED_SNAPPY) {
> + status |= DUMP_DH_COMPRESSED_SNAPPY;
> + }
> +#endif
> + dh->status = cpu_convert_to_target32(status, endian);
> +
> + if (write_buffer(s->fd, 0, dh, size) < 0) {
> + dump_error(s, "dump: failed to write disk dump header.\n");
> + ret = -1;
> + goto out;
> + }
> +
> + /* write sub header */
> + size = sizeof(KdumpSubHeader64);
> + kh = g_malloc0(size);
> +
> + /* 64bit max_mapnr_64 */
> + kh->max_mapnr_64 = cpu_convert_to_target64(s->max_mapnr, endian);
> + kh->phys_base = cpu_convert_to_target64(PHYS_BASE, endian);
> + kh->dump_level = cpu_convert_to_target32(DUMP_LEVEL, endian);
> +
> + offset_note = DISKDUMP_HEADER_BLOCKS * dh->block_size + size;
Again, multiplier should be host-endian.
> + kh->offset_note = cpu_convert_to_target64(offset_note, endian);
> + kh->note_size = cpu_convert_to_target64(s->note_size, endian);
> +
> + if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
> + dh->block_size, kh, size) < 0) {
Ditto.
(These bugs are unnoticeable in testing if your host and target
endiannesses match.)
The rest of the patch seems fine. Please replace these four instances of
"dh->block_size" with the host-endian local variable.
Thanks,
Laszlo
