Re: [Qemu-devel] [PATCH v2 2/2] postcopy: Check for shared memory

[Halil Pasic]

On 03/09/2017 05:06 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (address@hidden) wrote:
>>
>>
>> On 03/09/2017 02:22 PM, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <address@hidden>
>>>
>>> Postcopy doesn't support migration of RAM shared with another process
>>> yet (we've got a bunch of things to understand).
>>> Check for the case and don't allow postcopy to be enabled.
>>>
>>> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
>>> ---
>>>  migration/postcopy-ram.c | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>>> index effbeb6..dc80dbb 100644
>>> --- a/migration/postcopy-ram.c
>>> +++ b/migration/postcopy-ram.c
>>> @@ -95,6 +95,19 @@ static bool ufd_version_check(int ufd)
>>>      return true;
>>>  }
>>>
>>> +/* Callback from postcopy_ram_supported_by_host block iterator.
>>> + */
>>> +static int test_range_shared(const char *block_name, void *host_addr,
>>> +                             ram_addr_t offset, ram_addr_t length, void 
>>> *opaque)
>>> +{
>>> +    if (qemu_ram_is_shared(qemu_ram_block_by_name(block_name))) {
>>> +        error_report("Postcopy on shared RAM (%s) is not yet supported",
>>> +                     block_name);
>>> +        return 1;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>
>> Hm, this stuff with the iterator seemed a bit strange (too complicated)
>> first, but I'm not familiar with this code. I have no idea why is
>> RAMBlockIterFunc
>>  
>> typedef int (RAMBlockIterFunc)(const char *block_name, void *host_addr,
>>     ram_addr_t offset, ram_addr_t length, void *opaque)
>>
>> and not 
>>
>> typedef int (RAMBlockIterFunc)(RAMBlock *block, void *opaque).
>>
>> The reason does not seem to be abstraction.
> 
> It is, it's because the contents of RAMBlock are private.
> 


That's kind of half-backed abstraction. One can get a pointer to
RAMBlock, just like you did, and then do operations on it, just like you
did. So it would have been perfectly normal to use a pointer to RAMBlock
(incomplete type) and provide accessors (getters) reflecting the public
interface of RAMBlock. Well it may be helpful for making undesired usage
patterns hard, I do not know if this is the reason. (What I mean is if I
wanted to see these just for a single block I have a pointer to, I would
probably have to iterate over all blocks get a pointer by name and
compare it with my pointer, if it matches I have the values. So if such
usage is undesired, it's well reflected in the design).

The whole question is kind of off-topic -- my curious nature is making me
less productive again...

Regards,
Halil