From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:45575)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <d.hatayama@jp.fujitsu.com>) id 1SAzWL-0000Mf-Kn
	for qemu-devel@nongnu.org; Fri, 23 Mar 2012 04:06:54 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <d.hatayama@jp.fujitsu.com>) id 1SAzWE-0002X4-MM
	for qemu-devel@nongnu.org; Fri, 23 Mar 2012 04:06:49 -0400
Received: from fgwmail6.fujitsu.co.jp ([192.51.44.36]:57393)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <d.hatayama@jp.fujitsu.com>) id 1SAzWE-0002WQ-69
	for qemu-devel@nongnu.org; Fri, 23 Mar 2012 04:06:42 -0400
Received: from m4.gw.fujitsu.co.jp (unknown [10.0.50.74])
	by fgwmail6.fujitsu.co.jp (Postfix) with ESMTP id 99DD33EE0BC
	for <qemu-devel@nongnu.org>; Fri, 23 Mar 2012 17:06:37 +0900 (JST)
Received: from smail (m4 [127.0.0.1])
	by outgoing.m4.gw.fujitsu.co.jp (Postfix) with ESMTP id 8328E45DE52
	for <qemu-devel@nongnu.org>; Fri, 23 Mar 2012 17:06:37 +0900 (JST)
Received: from s4.gw.fujitsu.co.jp (s4.gw.fujitsu.co.jp [10.0.50.94])
	by m4.gw.fujitsu.co.jp (Postfix) with ESMTP id 5DB6745DE56
	for <qemu-devel@nongnu.org>; Fri, 23 Mar 2012 17:06:37 +0900 (JST)
Received: from s4.gw.fujitsu.co.jp (localhost.localdomain [127.0.0.1])
	by s4.gw.fujitsu.co.jp (Postfix) with ESMTP id 4FE3F1DB802F
	for <qemu-devel@nongnu.org>; Fri, 23 Mar 2012 17:06:37 +0900 (JST)
Received: from m107.s.css.fujitsu.com (m107.s.css.fujitsu.com [10.240.81.147])
	by s4.gw.fujitsu.co.jp (Postfix) with ESMTP id E50A01DB8040
	for <qemu-devel@nongnu.org>; Fri, 23 Mar 2012 17:06:36 +0900 (JST)
Date: Fri, 23 Mar 2012 17:06:22 +0900 (   )
Message-Id: <20120323.170622.115912902.d.hatayama@jp.fujitsu.com>
From: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
In-Reply-To: <4F680037.9090101@cn.fujitsu.com>
References: <4F680037.9090101@cn.fujitsu.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 11/11 v10] introduce a new monitor command
 'dump-guest-memory' to dump guest's memory
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: wency@cn.fujitsu.com
Cc: jan.kiszka@siemens.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, anderson@redhat.com, anthony@codemonkey.ws, eblake@redhat.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