Re: [PATCH 5/8] migration: move calling control_save_page to the common place

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* guangrong.xiao@gmail.com (guangrong.xiao@gmail.com) wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> The function is called by both ram_save_page and ram_save_target_page,
> so move it to the common caller to cleanup the code
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> ---
>  migration/ram.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index e7b8b14c3c..839665d866 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1020,10 +1020,6 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss, bool last_stage)
>      p = block->host + offset;
>      trace_ram_save_page(block->idstr, (uint64_t)offset, p);
>  
> -    if (control_save_page(rs, block, offset, &pages)) {
> -        return pages;
> -    }
> -
>      XBZRLE_cache_lock();
>      pages = save_zero_page(rs, block, offset);
>      if (pages > 0) {
> @@ -1176,10 +1172,6 @@ static int ram_save_compressed_page(RAMState *rs, PageSearchStatus *pss,
>  
>      p = block->host + offset;
>  
> -    if (control_save_page(rs, block, offset, &pages)) {
> -        return pages;
> -    }
> -
>      /* When starting the process of a new block, the first page of
>       * the block should be sent out before other pages in the same
>       * block, and all the pages in last block should have been sent
> @@ -1472,6 +1464,13 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>  
>      /* Check the pages is dirty and if it is send it */
>      if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
> +        RAMBlock *block = pss->block;
> +        ram_addr_t offset = pss->page << TARGET_PAGE_BITS;
> +
> +        if (control_save_page(rs, block, offset, &res)) {
> +            goto page_saved;

OK, but I'd prefer if you avoided this forward goto;  we do use goto but
we tend to keep it just for error cases.

Dave

> +        }
> +
>          /*
>           * If xbzrle is on, stop using the data compression after first
>           * round of migration even if compression is enabled. In theory,
> @@ -1484,6 +1483,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>              res = ram_save_page(rs, pss, last_stage);
>          }
>  
> +page_saved:
>          if (res < 0) {
>              return res;
>          }
> -- 
> 2.14.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK