Re: [Qemu-devel] [RFC][PATCH 04/14 v7] Add API to get memory mapping

[HATAYAMA Daisuke]

From: Wen Congyang <address@hidden>
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 <address@hidden>
>> 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