Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate

[Juan Quintela <quintela@redhat.com>]

Lei Li <lilei@linux.vnet.ibm.com> wrote:
> qemu_file_rate_limit() never return negative value since the refactor
> by Commit 1964a39, this patch gets rid of the negative check for it,
> adjust bytes_transferred and return value correspondingly in 
> ram_save_iterate().
>
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>
> Change since v1:
>   Return fixes and improvement from Paolo Bonzini.
>   
>  arch_init.c |   15 ++++++++++-----
>  1 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 94d45e1..a26bc89 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -709,15 +709,20 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>       */
>      ram_control_after_iterate(f, RAM_CONTROL_ROUND);
>  
> +    bytes_transferred += total_sent;

Agreed.

> +
> +    /*
> +     * Do not count these 8 bytes into total_sent, so that we can
> +     * return 0 if no page had been dirtied.
> +     */
> +    qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> +    bytes_transferred += 8;
> +
> +    ret = qemu_file_get_error(f);
>      if (ret < 0) {

Not sure this is the right solution.

We are sending anyways RAM_SAVE_FLAG_EOS.

And I think that the right solution is make qemu_get_rate_limit() to
return -1 in case of error (or the error, I don't care).  Looking at the
callers:


migration.c::migration_thread()

we check for error when we qemu_file_rate_limit() returns != 0.  Well.

second call:

        if (qemu_file_rate_limit(s->file)) {
            /* usleep expects microseconds */
            g_usleep((initial_time + BUFFER_DELAY - current_time)*1000);
        }


if should be:

    if (qemu_file_rate_limit(s->file) == 1)

block_migration: not correct, we don't check for the error.

arch_init.c:

   check is correct, but we need to return -ERROR in case of errors.

hw/ppc/spapr.c:
   will work correctly even if changed to -ERROR.


savevm.c: qemu_savevm_state_iterate()


        if (qemu_file_rate_limit(f)) {
            return 0;
        }


check is incorrect again, we should return an error if there is one
error.


I think that returning qemu_rate_limit() to return 0/1/negative makes sense.

Thoughts?

Thanks, Juan.