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;
 }
 
 /*
-- 
2.35.3

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

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.