Re: [PATCH v1 07/13] migrate/ram: Get rid of "place_source" in ram_load_postcopy()

[David Hildenbrand <david@redhat.com>]

On 19.02.20 21:55, Peter Xu wrote:
> On Wed, Feb 19, 2020 at 03:47:30PM -0500, Peter Xu wrote:
>> On Wed, Feb 19, 2020 at 05:17:19PM +0100, David Hildenbrand wrote:
>>> It's always the same value.
>>
>> I guess not, because...
>>
>>>
>>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>> Cc: Juan Quintela <quintela@redhat.com>
>>> Cc: Peter Xu <peterx@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  migration/ram.c | 8 +++-----
>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index cbd54947fb..75014717f6 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -3119,7 +3119,6 @@ static int ram_load_postcopy(QEMUFile *f)
>>>          ram_addr_t addr;
>>>          void *host = NULL;
>>>          void *page_buffer = NULL;
>>> -        void *place_source = NULL;
>>>          RAMBlock *block = NULL;
>>>          uint8_t ch;
>>>          int len;
>>> @@ -3188,7 +3187,6 @@ static int ram_load_postcopy(QEMUFile *f)
>>>                  place_needed = true;
>>>                  target_pages = 0;
>>>              }
>>> -            place_source = postcopy_host_page;
>>>          }
>>>  
>>>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>>> @@ -3220,7 +3218,7 @@ static int ram_load_postcopy(QEMUFile *f)
>>>                   * buffer to make sure the buffer is valid when
>>>                   * placing the page.
>>>                   */
>>> -                qemu_get_buffer_in_place(f, (uint8_t **)&place_source,
>>
>> ... it can be modified inside the call.
>>
>> I feel like this patch could even fail the QEMU unit test.  It would
>> be good to mention what tests have been carried out in the cover
>> letter or with RFC tag if no test is done yet.
>>
>> For a series like this, I'll try at least the unit tests and smoke on
>> both precopy and postcopy.  The resizing test would be even better but
>> seems untrivial, so maybe optional.
> 
> For resizing test, an easy way (I can think of) is to temporarily
> remove the size check below in your test branch:

Yeah, it's especially hard to have a reliable test one can materialize.
I played with manual tests like this myself.

I'm thinking about testing this with a device that can trigger resizes
on demand, e.g., virtio-mem, for now on my private branch. But I'd
really like to have some test one can automate at one point ... however,
there seems to be no easy way to achieve that right now.

Thanks!

-- 
Thanks,

David / dhildenb