From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=56230 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PsBfs-0003vn-O3
	for qemu-devel@nongnu.org; Wed, 23 Feb 2011 05:10:25 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1PsBfr-0005uR-AU
	for qemu-devel@nongnu.org; Wed, 23 Feb 2011 05:10:24 -0500
Received: from mx1.redhat.com ([209.132.183.28]:13017)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <quintela@redhat.com>) id 1PsBfq-0005uD-V4
	for qemu-devel@nongnu.org; Wed, 23 Feb 2011 05:10:23 -0500
From: Juan Quintela <quintela@redhat.com>
In-Reply-To: <AANLkTik_-8kYfZm7OUkw2_oZ6gYNdT8g=3oG8-nMsf6G@mail.gmail.com>
	(Yoshiaki Tamura's message of "Wed, 23 Feb 2011 18:51:07 +0900")
References: <cover.1298421307.git.quintela@redhat.com>
	<92b9322a569eca234df0b64afb38f5f9cc4f2726.1298421307.git.quintela@redhat.com>
	<AANLkTik_-8kYfZm7OUkw2_oZ6gYNdT8g=3oG8-nMsf6G@mail.gmail.com>
Date: Wed, 23 Feb 2011 11:08:54 +0100
Message-ID: <m3r5az5ah5.fsf@trasno.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: [Qemu-devel] Re: [PATCH 10/22] migration: Refactor and simplify
	error checking in migrate_fd_put_ready
Reply-To: quintela@redhat.com
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/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: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
Cc: qemu-devel@nongnu.org

Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>> =C2=A0migration.c | =C2=A0 20 +++++++++-----------
>> =C2=A01 files changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index f015e02..641df9f 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -361,28 +361,26 @@ static void migrate_fd_put_ready(void *opaque)
>>
>> =C2=A0 =C2=A0 DPRINTF("iterate\n");
>> =C2=A0 =C2=A0 if (qemu_savevm_state_iterate(s->mon, s->file) =3D=3D 1) {
>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0int state;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 int old_vm_running =3D vm_running;
>>
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 DPRINTF("done iterating\n");
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 vm_stop(VMSTOP_MIGRATE);
>>
>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if ((qemu_savevm_state_complete(s->mon, s->=
file)) < 0) {
>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (old_vm_running) {
>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vm_start();
>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0state =3D MIG_STATE_ERROR;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (qemu_savevm_state_complete(s->mon, s->f=
ile) < 0) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0migrate_fd_error(s);
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0state =3D MIG_STATE_COMPLETED;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (migrate_fd_cleanup(s) < 0=
) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0migrate_fd_erro=
r(s);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} else {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0s->state =3D MI=
G_STATE_COMPLETED;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0notifier_list_n=
otify(&migration_state_notifiers);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (migrate_fd_cleanup(s) < 0) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (s->get_status(s) !=3D MIG_STATE_COMPLET=
ED) {
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (old_vm_running) {
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vm_start();
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
> This part, although it's not fair to ask you, but calling
> vm_start when !=3D MIG_STATE_COMPLETED terrifies me because just
> failing migrate_fd_cleanup (mostly calling qemu_fclose) may cause
> split brain between src/dst.  Although I haven't encountered this
> situation, just having stopped VMs on both sides is safer.

I see your pain. I am not happy at all, but this was integrated by
Anthony to fix this bug:

commit 41ef56e61153d7bd27d34a634633bb51b1c5988d
Author: Anthony Liguori <aliguori@us.ibm.com>
Date:   Wed Jun 2 14:55:25 2010 -0500

    migration: respect exit status with exec:

 This fixes https://bugs.launchpad.net/qemu/+bug/391879


I think that it fixes that bug, but it makes me un-easy to restart vm if
there is a failure in migrate_fd_cleanup().  As I didn't wanted to
change behaviour with this series, I left it as it was.

Next on ToDo list is to do something sensible with errors, just now we
are not very good at handling them.

Later, Juan.