From: Wen Congyang <we...@cn.fujitsu.com>
Subject: Re: [RFC][PATCH 04/14 v7] Add API to get memory mapping
Date: Thu, 01 Mar 2012 14:17:53 +0800
> At 03/01/2012 02:01 PM, HATAYAMA Daisuke Wrote:
>> From: Wen Congyang <we...@cn.fujitsu.com>
>> Subject: [RFC][PATCH 04/14 v7] Add API to get memory mapping
>> Date: Thu, 01 Mar 2012 10:43:13 +0800
>>
>>> +int qemu_get_guest_memory_mapping(MemoryMappingList *list)
>>> +{
>>> + CPUState *env;
>>> + MemoryMapping *memory_mapping;
>>> + RAMBlock *block;
>>> + ram_addr_t offset, length;
>>> + int ret;
>>> +
>>> +#if defined(CONFIG_HAVE_GET_MEMORY_MAPPING)
>>> + for (env = first_cpu; env != NULL; env = env->next_cpu) {
>>> + ret = cpu_get_memory_mapping(list, env);
>>> + if (ret < 0) {
>>> + return -1;
>>> + }
>>> + }
>>> +#else
>>> + return -2;
>>> +#endif
>>> +
>>> + /* some memory may be not mapped, add them into memory mapping's list
>>> */
>>
>> The part from here is logic fully for 2nd kernel? If so, I think it
>> better to describe why this addtional mapping is needed; we should
>> assume most people doesn't know kdump mechanism.
>
> Not only for 2nd kernel. If the guest has 1 vcpu, and is in the 2nd kernel,
> we need this logic for 1st kernel.
>
So you should describe two cases explicitly. I don't understand them
from ``some memory''.
and please consider cleanup below too. Conditionals over two lines are
hard to read.
>>
>> I think it more readable if shortening memory_mapping->phys_addr and
>> memmory_maping->length at the berinning of the innermost foreach loop.
>>
>> m_phys_addr = memory_mapping->phys_addr;
>> m_length = memory_mapping->length;
>>
>> Then, each conditionals becomes compact.
Thanks.
HATAYAMA, Daisuke
