Re: [Qemu-devel] [RFC 4/5] scripts/dump-guest-memory.py: Cleanup functions

[Laszlo Ersek <lersek@redhat.com>]

On 01/14/16 09:48, Janosch Frank wrote:
> Increase readability by adding newlines and comments, as well as
> removing wrong whitespaces and C style braces around conditionals and
> loops.
> 
> Signed-off-by: Janosch Frank <frankja@linux.vnet.ibm.com>
> ---
>  scripts/dump-guest-memory.py | 71 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 50 insertions(+), 21 deletions(-)
> 
> diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
> index fe93135..2110494 100644
> --- a/scripts/dump-guest-memory.py
> +++ b/scripts/dump-guest-memory.py
> @@ -69,35 +69,60 @@ ELF64_PHDR = ("I"  # p_type
>            )
>  
>  def int128_get64(val):
> -    assert (val["hi"] == 0)
> +    """Returns low 64bit part of Int128 struct."""
> +
> +    assert val["hi"] == 0
>      return val["lo"]
>  
> +
>  def qlist_foreach(head, field_str):
> +    """Generator for qlists."""
> +
>      var_p = head["lh_first"]
> -    while (var_p != 0):
> +    while var_p != 0:
>          var = var_p.dereference()
> -        yield var
>          var_p = var[field_str]["le_next"]
> +        yield var
> +
>  
>  def qemu_get_ram_block(ram_addr):
> +    """Returns the RAMBlock struct to which the given address belongs."""
> +
>      ram_blocks = gdb.parse_and_eval("ram_list.blocks")
> +
>      for block in qlist_foreach(ram_blocks, "next"):
> -        if (ram_addr - block["offset"] < block["used_length"]):
> +        if (ram_addr - block["offset"]) < block["used_length"]:
>              return block
> +
>      raise gdb.GdbError("Bad ram offset %x" % ram_addr)
>  
> +
>  def qemu_get_ram_ptr(ram_addr):
> +    """Returns qemu vaddr for given guest physical address."""
> +
>      block = qemu_get_ram_block(ram_addr)
>      return block["host"] + (ram_addr - block["offset"])
>  
> -def memory_region_get_ram_ptr(mr):
> -    if (mr["alias"] != 0):
> -        return (memory_region_get_ram_ptr(mr["alias"].dereference()) +
> -                mr["alias_offset"])
> -    return qemu_get_ram_ptr(mr["ram_addr"] & TARGET_PAGE_MASK)
> +
> +def memory_region_get_ram_ptr(memory_region):
> +    if memory_region["alias"] != 0:
> +        return (memory_region_get_ram_ptr(memory_region["alias"].dereference())
> +                + memory_region["alias_offset"])
> +
> +    return qemu_get_ram_ptr(memory_region["ram_addr"] & TARGET_PAGE_MASK)
> +
>  
>  def get_guest_phys_blocks():
> +    """Returns a list of ram blocks.
> +
> +    Each block entry contains:
> +    'target_start': guest block phys start address
> +    'target_end':   guest block phys end address
> +    'host_addr':    qemu vaddr of the block's start
> +    """
> +
>      guest_phys_blocks = []
> +
>      print("guest RAM blocks:")
>      print("target_start     target_end       host_addr        message "
>            "count")
> @@ -106,30 +131,34 @@ def get_guest_phys_blocks():
>  
>      current_map_p = gdb.parse_and_eval("address_space_memory.current_map")
>      current_map = current_map_p.dereference()
> +
> +    # Conversion to int is needed for python 3
> +    # compatibility. Otherwise range doesn't cast the value itself and
> +    # breaks.

This comment probably belongs to the previous patch.

>      for cur in range(int(current_map["nr"])):
> -        flat_range   = (current_map["ranges"] + cur).dereference()
> -        mr           = flat_range["mr"].dereference()
> +        flat_range = (current_map["ranges"] + cur).dereference()
> +        memory_region = flat_range["mr"].dereference()
>  
>          # we only care about RAM
> -        if (not mr["ram"]):
> +        if not memory_region["ram"]:
>              continue
>  
>          section_size = int128_get64(flat_range["addr"]["size"])
>          target_start = int128_get64(flat_range["addr"]["start"])
> -        target_end   = target_start + section_size
> -        host_addr    = (memory_region_get_ram_ptr(mr) +
> -                        flat_range["offset_in_region"])
> +        target_end = target_start + section_size
> +        host_addr = (memory_region_get_ram_ptr(memory_region) +
> +                     flat_range["offset_in_region"])

The way you preserved the line wrapping here (i.e., operator at the end
of the line) is inconsistent with the change you employed above, in
memory_region_get_ram_ptr(). There you moved the operator to the start
of the next line.

>          predecessor = None
>  
>          # find continuity in guest physical address space
> -        if (len(guest_phys_blocks) > 0):
> +        if len(guest_phys_blocks) > 0:
>              predecessor = guest_phys_blocks[-1]
>              predecessor_size = (predecessor["target_end"] -
> -                                    predecessor["target_start"])
> +                                predecessor["target_start"])

Ditto.

>  
>              # the memory API guarantees monotonically increasing
>              # traversal
> -            assert (predecessor["target_end"] <= target_start)
> +            assert predecessor["target_end"] <= target_start
>  
>              # we want continuity in both guest-physical and
>              # host-virtual memory
> @@ -137,11 +166,11 @@ def get_guest_phys_blocks():
>                  predecessor["host_addr"] + predecessor_size != host_addr):
>                  predecessor = None
>  
> -        if (predecessor is None):
> +        if predecessor is None:
>              # isolated mapping, add it to the list
>              guest_phys_blocks.append({"target_start": target_start,
> -                                      "target_end"  : target_end,
> -                                      "host_addr"   : host_addr})
> +                                      "target_end":   target_end,
> +                                      "host_addr":    host_addr})
>              message = "added"
>          else:
>              # expand predecessor until @target_end; predecessor's
>