On Tue, Aug 15, 2023 at 11:38:28AM -0300, Fabiano Rosas wrote:
> We don't need to do this in two pieces. One single function makes it
> easier to grasp, specially since it removes the indirection on the
> return value handling.
>
> Signed-off-by: Fabiano Rosas <[email protected]>
> ---
> migration/ram.c | 41 +++++++++++------------------------------
> 1 file changed, 11 insertions(+), 30 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 8ec38f69e8..13935ead1c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1128,32 +1128,6 @@ void ram_release_page(const char *rbname, uint64_t
> offset)
> ram_discard_range(rbname, offset, TARGET_PAGE_SIZE);
> }
>
> -/**
> - * save_zero_page_to_file: send the zero page to the file
> - *
> - * Returns the size of data written to the file, 0 means the page is not
> - * a zero page
> - *
> - * @pss: current PSS channel
> - * @block: block that contains the page we want to send
> - * @offset: offset inside the block for the page
> - */
> -static int save_zero_page_to_file(PageSearchStatus *pss, RAMBlock *block,
> - ram_addr_t offset)
> -{
> - uint8_t *p = block->host + offset;
> - QEMUFile *file = pss->pss_channel;
> - int len = 0;
> -
> - if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> - len += save_page_header(pss, file, block, offset |
> RAM_SAVE_FLAG_ZERO);
> - qemu_put_byte(file, 0);
> - len += 1;
> - ram_release_page(block->idstr, offset);
> - }
> - return len;
> -}
> -
> /**
> * save_zero_page: send the zero page to the stream
> *
> @@ -1167,12 +1141,19 @@ static int save_zero_page_to_file(PageSearchStatus
> *pss, RAMBlock *block,
> static int save_zero_page(RAMState *rs, PageSearchStatus *pss, RAMBlock
> *block,
> ram_addr_t offset)
> {
> - int len = save_zero_page_to_file(pss, block, offset);
> + uint8_t *p = block->host + offset;
> + QEMUFile *file = pss->pss_channel;
> + int len = 0;
>
> - if (!len) {
> - return -1;
> + if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> + return 0;
> }
>
> + len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
> + qemu_put_byte(file, 0);
> + len += 1;
> + ram_release_page(block->idstr, offset);
> +
> stat64_add(&mig_stats.zero_pages, 1);
> ram_transferred_add(len);
>
> @@ -1186,7 +1167,7 @@ static int save_zero_page(RAMState *rs,
> PageSearchStatus *pss, RAMBlock *block,
> XBZRLE_cache_unlock();
> }
>
> - return 1;
> + return len;
I don't think it's correct.. We need to keep the retval definition (how
many pages were sent) rather than returning num of bytes, I think.
I'm curious how did this pass any form of test.. because I think we did
assert that:
/* Be strict to return code; it must be 1, or what else? */
if (migration_ops->ram_save_target_page(rs, pss) != 1) {
error_report_once("%s: ram_save_target_page failed", __func__);
ret = -1;
goto out;
}
Did I miss something?
--
Peter Xu
