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

[Peter Xu]

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" <address@hidden>
> Cc: Juan Quintela <address@hidden>
> Cc: Peter Xu <address@hidden>
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  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.

Thanks,

> +                qemu_get_buffer_in_place(f, (uint8_t **)&postcopy_host_page,
>                                           TARGET_PAGE_SIZE);
>              }
>              break;
> @@ -3265,8 +3263,8 @@ static int ram_load_postcopy(QEMUFile *f)
>                  ret = postcopy_place_page_zero(mis, place_dest,
>                                                 block);
>              } else {
> -                ret = postcopy_place_page(mis, place_dest,
> -                                          place_source, block);
> +                ret = postcopy_place_page(mis, place_dest, 
> postcopy_host_page,
> +                                          block);
>              }
>          }
>      }
> -- 
> 2.24.1
> 

-- 
Peter Xu