[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 11/11 v10] introduce a new monitor command 'dump
From: |
HATAYAMA Daisuke |
Subject: |
Re: [Qemu-devel] [PATCH 11/11 v10] introduce a new monitor command 'dump-guest-memory' to dump guest's memory |
Date: |
Fri, 23 Mar 2012 17:06:22 +0900 ( ) |
From: Wen Congyang <address@hidden>
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