Peter Xu <pet...@redhat.com> writes:
> 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 <faro...@suse.de>
>> ---
>> 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?
Kind of, this code is correct. It's just that I made save_zero_page()
return bytes like save_zero_page_to_file() used to do and made
ram_save_target_page() return 1 instead of passing the value from
save_zero_page() along.
But there's a bug in patch 3 because what I described above should only
happen in this patch 5.
