[5/5] migration/ram: Merge save_zero_page functions

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 <farosas@suse.de>
---
 migration/ram.c | 41 +++++++++++------------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

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 <farosas@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?

Peter Xu <peterx@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 <farosas@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.