Re: [Qemu-devel] [PATCH 11/11 v10] introduce a new monitor command 'dump-guest-memory' to dump guest's memory

[HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>]

From: Wen Congyang <wency@cn.fujitsu.com>
Subject: [PATCH 11/11 v10] introduce a new monitor command 'dump-guest-memory' to dump guest's memory
Date: Tue, 20 Mar 2012 11:57:43 +0800

<cut>

> +typedef struct DumpState {
> +    ArchDumpInfo dump_info;
> +    MemoryMappingList list;
> +    uint16_t phdr_num;
> +    uint32_t sh_info;
> +    bool have_section;
> +    bool resume;
> +    target_phys_addr_t memory_offset;
> +    write_core_dump_function f;

f() is so general. Type information is meaningless enough, but there's
no explicit occurence of the function call of f(). Could you consider
renaming?

> +    void (*cleanup)(void *opaque);
> +    void *opaque;
> +
> +    RAMBlock *block;
> +    ram_addr_t start;
> +    bool has_filter;
> +    int64_t begin;
> +    int64_t length;
> +} DumpState;
> +

<cut>

> +
> +static int write_elf64_load(DumpState *s, MemoryMapping *memory_mapping,
> +                            int phdr_index, target_phys_addr_t offset)
> +{
> +    Elf64_Phdr phdr;
> +    off_t phdr_offset;
> +    int ret;
> +    int endian = s->dump_info.d_endian;
> +
> +    memset(&phdr, 0, sizeof(Elf64_Phdr));
> +    phdr.p_type = cpu_convert_to_target32(PT_LOAD, endian);
> +    phdr.p_offset = cpu_convert_to_target64(offset, endian);
> +    phdr.p_paddr = cpu_convert_to_target64(memory_mapping->phys_addr, endian);
> +    if (offset == -1) {

Question: In what situations does offset become -1?

<cut>

> +static int write_elf_section(DumpState *s, target_phys_addr_t *offset, int type)
> +{
> +    Elf32_Shdr shdr32;
> +    Elf64_Shdr shdr64;
> +    int endian = s->dump_info.d_endian;
> +    int shdr_size;
> +    void *shdr;
> +    int ret;
> +
> +    if (type == 0) {

In other places, you checks s->dump_info.d_class == ELFCLASS64, and
variable ``type'' here has the same meaning. It's better to fit this
to the others.

Also, the ``s->dump_info.d_class == ELFCLASS64'' occurs so many times
in source code. Why not add method to DumpState type to avoid this
check? For example, 

typedef struct DumpState {
  ...
  int write_elf_header(DumpState *s)
  ...
} DumpState;

and then, register appropreate function to the member in
cpu_get_dump_info().

> +     *
> +     * the type of ehdr->e_phnum is uint16_t, so we should avoid overflow
> +     */
> +    s->phdr_num = 1; /* PT_NOTE */
> +    if (s->list.num < UINT16_MAX - 2) {

Is s->list.num < UINT16_MAX - 1 correct? 

> +        s->phdr_num += s->list.num;
> +        s->have_section = false;
> +    } else {
> +        s->have_section = true;
> +        s->phdr_num = PN_XNUM;
> +        s->sh_info = 1; /* PT_NOTE */

It's confusing to use member names, phdr_num and sh_info, from
differnet views. I first think phdr_num is used for an actual number
of program headers, but in reality, this is used as e_phum member in
ELF header. It's better to use e_phnum just as sh_info to avoid
confusion.

Also, this logic is comform to ELF specification, so it's better to
move this to write_elfNN_header() and write_elf_section().

Thanks.
HATAYAMA, Daisuke