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

[David Hildenbrand <david@redhat.com>]

> Am 19.02.2020 um 21:47 schrieb Peter Xu <peterx@redhat.com>:
> 
> 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.

Very right, will drop this patch! Thanks!

> 
> 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.

I test all code I share. This survives „make check“. I assume all tests send small pages where „matches_target_page_size==true“, so the tests did not catch this.

I even spent the last day getting avocado-vt to work and ran multiple (obviously not all) migration tests, including postcopy, so your suggestions have already been considered ...

Could have mentioned that in the cover letter, yes.