Re: [Qemu-devel] [PATCH 08/13 v7] dump: add API to write dump header

[Laszlo Ersek <lersek@redhat.com>]

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 <qiaonuohan@cn.fujitsu.com>
> Reviewed-by: Laszlo Ersek <lersek@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