Re: [Qemu-devel] [PATCH v6 06/11] dump: add API to write dump header

[Laszlo Ersek <lersek@redhat.com>]

comments below

On 01/05/14 08:27, Qiao Nuohan 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>
> ---
>  dump.c                |  199 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/dump.h |   96 ++++++++++++++++++++++++
>  2 files changed, 295 insertions(+), 0 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 3b9cf00..e3623b9 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -77,8 +77,16 @@ typedef struct DumpState {
>      int64_t length;
>      Error **errp;
>  
> +    bool flag_flatten;
> +    uint32_t nr_cpus;
> +    size_t page_size;
> +    uint64_t max_mapnr;
> +    size_t len_dump_bitmap;
>      void *note_buf;
>      size_t note_buf_offset;
> +    off_t offset_dump_bitmap;
> +    off_t offset_page;
> +    uint32_t flag_compress;
>  } DumpState;
>  
>  static int dump_cleanup(DumpState *s)
> @@ -773,6 +781,197 @@ static int buf_write_note(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;
> +
> +    /* write common header, the version of kdump-compressed format is 5th */

I think this is a typo (it should say 6th, shouldn't it?), but it's not
critical.

> +    size = sizeof(DiskDumpHeader32);
> +    dh = g_malloc0(size);
> +
> +    strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
> +    dh->header_version = 6;
> +    dh->block_size = s->page_size;
> +    dh->sub_hdr_size = sizeof(struct KdumpSubHeader32) + s->note_size;
> +    dh->sub_hdr_size = DIV_ROUND_UP(dh->sub_hdr_size, dh->block_size);
> +    /* dh->max_mapnr may be truncated, full 64bit is in kh.max_mapnr_64 */
> +    dh->max_mapnr = MIN(s->max_mapnr, UINT_MAX);

You could have simply converted / truncated "s->max_mapnr" to uint32_t
as part of the assignment, but the MIN(..., UINT_MAX) doesn't hurt either.

> +    dh->nr_cpus = s->nr_cpus;
> +    dh->bitmap_blocks = DIV_ROUND_UP(s->len_dump_bitmap, dh->block_size) * 2;
> +    memcpy(&(dh->utsname.machine), "i686", 4);
> +
> +    if (s->flag_compress & DUMP_DH_COMPRESSED_ZLIB) {
> +        dh->status |= DUMP_DH_COMPRESSED_ZLIB;
> +    }
> +#ifdef CONFIG_LZO
> +    if (s->flag_compress & DUMP_DH_COMPRESSED_LZO) {
> +        dh->status |= DUMP_DH_COMPRESSED_LZO;
> +    }
> +#endif
> +#ifdef CONFIG_SNAPPY
> +    if (s->flag_compress & DUMP_DH_COMPRESSED_SNAPPY) {
> +        dh->status |= DUMP_DH_COMPRESSED_SNAPPY;
> +    }
> +#endif
> +
> +    if (write_buffer(s->fd, s->flag_flatten, 0, dh, size) < 0) {
> +        ret = -1;
> +        goto out;
> +    }

The following fields in "dh" are left zero-filled:
- timestamp
- total_ram_blocks
- device_blocks
- written_blocks
- current_cpu

I guess we'll either overwrite them later or it's OK to leave them all
zeroed.

Also... is it OK to write these fields to the file in host native byte
order? What happens if an i686 / x86_64 target is emulated on a BE host?

> +
> +    /* write sub header */
> +    size = sizeof(KdumpSubHeader32);
> +    kh = g_malloc0(size);
> +
> +    /* 64bit max_mapnr_64 */
> +    kh->max_mapnr_64 = s->max_mapnr;
> +    kh->phys_base = PHYS_BASE;
> +    kh->dump_level = DUMP_LEVEL;
> +
> +    kh->offset_note = DISKDUMP_HEADER_BLOCKS * dh->block_size + size;
> +    kh->note_size = s->note_size;
> +
> +    if (write_buffer(s->fd, s->flag_flatten, dh->block_size, kh, size) < 0) {
> +        ret = -1;
> +        goto out;
> +    }

- Same question about endianness as above.

- Again, many fields left zeroed in "kh", but I guess that's OK.

- I would prefer if you repeated the multiplication by
DISKDUMP_HEADER_BLOCKS verbatim in the "offset" write_buffer() argument.

- When this write_buffer() is directed to a regular file in non-flat
mode, then the file might become sparse (you jump over a range of
offsets with lseek() in write_buffer()). If the output has been opened
by qemu itself (ie. "file:....", in qmp_dump_guest_memory()), then due
to the O_TRUNC we can't seek over preexistent data (and keep garbage in
the file). When libvirt pre-opens the file (to send over the fd later),
in doCoreDump(), it also passes O_TRUNC. OK.

> +
> +    /* write note */
> +    s->note_buf = g_malloc(s->note_size);
> +    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, s->flag_flatten, kh->offset_note, s->note_buf,
> +                     s->note_size) < 0) {
> +        ret = -1;
> +        goto out;
> +    }

Right before the write_buffer() call, we know that

  s->note_buf_offset <= s->note_size

because buf_write_note() ensures it.

We know even that

  s->note_buf_offset == s->note_size

there, because write_elf32_notes() produces exactly as many bytes as
we've calculated in advance, in cpu_get_note_size().

However, this is not very easy to see, hence I'd prefer adding *one* of
the following three:
- an assert() before write_buffer() that states the above equality, or
- passing "s->note_buf_offset" to write_buffer() as "size" argument, or
- allocating "s->note_buf" with g_malloc0().

(The 64-bit version below goes with the g_malloc0() choice, which BTW
makes the two versions a bit inconsistent currently.)

> +
> +    /* get offset of dump_bitmap */
> +    s->offset_dump_bitmap = (DISKDUMP_HEADER_BLOCKS + dh->sub_hdr_size) *
> +                             dh->block_size;

So, DISKDUMP_HEADER_BLOCKS covers "dh", and "dh->sub_hdr_size" covers
KdumpSubHeader32 plus the note. Seems OK.

> +
> +    /* get offset of page */
> +    s->offset_page = (DISKDUMP_HEADER_BLOCKS + dh->sub_hdr_size +
> +                      dh->bitmap_blocks) * dh->block_size;

Seems OK too. I guess we'll use these fields later.

Both of these multiplications are done in "unsigned int" (uint32_t)
before converting the product to "off_t".

Are you sure even the second one will fit? "dh->bitmap_blocks" is the
interesting addend. 1 byte in one bitmap covers to 8*4K = 32K bytes
guest RAM. We have two bitmaps. So, if we had close to 4G bytes in the
two bitmaps together (close to 2G bytes in each), we could cover close
to 64 TB guest RAM without overflowing the multiplication. Seems sufficient.


> +
> +out:
> +    g_free(dh);
> +    g_free(kh);
> +    g_free(s->note_buf);
> +
> +    return ret;
> +}
> +
> +/* 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;
> +
> +    /* write common header, the version of kdump-compressed format is 5th */
> +    size = sizeof(DiskDumpHeader64);
> +    dh = g_malloc0(size);
> +
> +    strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
> +    dh->header_version = 6;
> +    dh->block_size = s->page_size;
> +    dh->sub_hdr_size = sizeof(struct KdumpSubHeader64) + s->note_size;
> +    dh->sub_hdr_size = DIV_ROUND_UP(dh->sub_hdr_size, dh->block_size);
> +    /* dh->max_mapnr may be truncated, full 64bit is in kh.max_mapnr_64 */
> +    dh->max_mapnr = MIN(s->max_mapnr, UINT_MAX);
> +    dh->nr_cpus = s->nr_cpus;
> +    dh->bitmap_blocks = DIV_ROUND_UP(s->len_dump_bitmap, dh->block_size) * 2;
> +    memcpy(&(dh->utsname.machine), "x86_64", 6);
> +
> +    if (s->flag_compress & DUMP_DH_COMPRESSED_ZLIB) {
> +        dh->status |= DUMP_DH_COMPRESSED_ZLIB;
> +    }
> +#ifdef CONFIG_LZO
> +    if (s->flag_compress & DUMP_DH_COMPRESSED_LZO) {
> +        dh->status |= DUMP_DH_COMPRESSED_LZO;
> +    }
> +#endif
> +#ifdef CONFIG_SNAPPY
> +    if (s->flag_compress & DUMP_DH_COMPRESSED_SNAPPY) {
> +        dh->status |= DUMP_DH_COMPRESSED_SNAPPY;
> +    }
> +#endif
> +
> +    if (write_buffer(s->fd, s->flag_flatten, 0, dh, size) < 0) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    /* write sub header */
> +    size = sizeof(KdumpSubHeader64);
> +    kh = g_malloc0(size);
> +
> +    /* 64bit max_mapnr_64 */
> +    kh->max_mapnr_64 = s->max_mapnr;
> +    kh->phys_base = PHYS_BASE;
> +    kh->dump_level = DUMP_LEVEL;
> +
> +    kh->offset_note = DISKDUMP_HEADER_BLOCKS * dh->block_size + size;
> +    kh->note_size = s->note_size;
> +
> +    if (write_buffer(s->fd, s->flag_flatten, dh->block_size, kh, size) < 0) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    /* write note */
> +    s->note_buf = g_malloc0(s->note_size);
> +    s->note_buf_offset = 0;
> +
> +    /* use s->note_buf to store notes temporarily */
> +    if (write_elf64_notes(buf_write_note, s) < 0) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    if (write_buffer(s->fd, s->flag_flatten, kh->offset_note, s->note_buf,
> +                     s->note_size) < 0) {
> +        ret = -1;
> +        goto out;
> +    }
> +
> +    /* get offset of dump_bitmap */
> +    s->offset_dump_bitmap = (DISKDUMP_HEADER_BLOCKS + dh->sub_hdr_size) *
> +                             dh->block_size;
> +
> +    /* get offset of page */
> +    s->offset_page = (DISKDUMP_HEADER_BLOCKS + dh->sub_hdr_size +
> +                      dh->bitmap_blocks) * dh->block_size;
> +
> +out:
> +    g_free(dh);
> +    g_free(kh);
> +    g_free(s->note_buf);
> +
> +    return ret;
> +}

I diffed this against the 32-bit version of the function, and the only
"suprising" difference is

-+    s->note_buf = g_malloc(s->note_size);
++    s->note_buf = g_malloc0(s->note_size);

which I already mentioned above.


I couldn't find anything in this patch that I'd call a direct bug. I
think you can address what you want from the above later too.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>