From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:38862)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1VJgYK-0002mf-BV
	for qemu-devel@nongnu.org; Wed, 11 Sep 2013 05:17:42 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1VJgYE-0005FY-BX
	for qemu-devel@nongnu.org; Wed, 11 Sep 2013 05:17:36 -0400
Received: from mx1.redhat.com ([209.132.183.28]:43423)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1VJgYE-0005DY-1p
	for qemu-devel@nongnu.org; Wed, 11 Sep 2013 05:17:30 -0400
From: Juan Quintela <quintela@redhat.com>
In-Reply-To: <1378285356-5308-4-git-send-email-lilei@linux.vnet.ibm.com> (Lei
	Li's message of "Wed, 4 Sep 2013 17:02:36 +0800")
References: <1378285356-5308-1-git-send-email-lilei@linux.vnet.ibm.com>
	<1378285356-5308-4-git-send-email-lilei@linux.vnet.ibm.com>
Date: Wed, 11 Sep 2013 11:17:22 +0200
Message-ID: <877gen3efx.fsf@elfo.elfo>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for
	ram_save_iterate
Reply-To: quintela@redhat.com
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Lei Li <lilei@linux.vnet.ibm.com>
Cc: anthony@codemonkey.ws, pbonzini@redhat.com, qemu-devel@nongnu.org, mrhines@linux.vnet.ibm.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.