From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:59106)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1XaTqo-000305-FO
	for qemu-devel@nongnu.org; Sat, 04 Oct 2014 14:14:39 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1XaTqf-0000Mj-Ea
	for qemu-devel@nongnu.org; Sat, 04 Oct 2014 14:14:38 -0400
Received: from mail-wg0-x233.google.com ([2a00:1450:400c:c00::233]:64951)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1XaTqf-0000Mc-7l
	for qemu-devel@nongnu.org; Sat, 04 Oct 2014 14:14:29 -0400
Received: by mail-wg0-f51.google.com with SMTP id b13so3689133wgh.34
	for <qemu-devel@nongnu.org>; Sat, 04 Oct 2014 11:14:28 -0700 (PDT)
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <543038FD.8030204@redhat.com>
Date: Sat, 04 Oct 2014 20:14:21 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1412358473-31398-1-git-send-email-dgilbert@redhat.com>
	<1412358473-31398-17-git-send-email-dgilbert@redhat.com>
In-Reply-To: <1412358473-31398-17-git-send-email-dgilbert@redhat.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v4 16/47] Return path: Source handling of
	return path
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: "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com>, qemu-devel@nongnu.org
Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, lilei@linux.vnet.ibm.com, quintela@redhat.com, cristian.klein@cs.umu.se, amit.shah@redhat.com, yanghy@cn.fujitsu.com

Il 03/10/2014 19:47, Dr. David Alan Gilbert (git) ha scritto:
> +/* Source side RP state */
> +struct MigrationRetPathState {
> +    uint32_t      latest_ack;
> +    QemuThread    rp_thread;
> +    bool          error;

Should the QemuFile be in here?

> +};
> +

Also please do not abbrev words, and add a typedef that matches the
struct if it is useful.  If it is not, just embed the struct without
giving the type a name (struct { } rp_state).

> +static bool migration_already_active(MigrationState *ms)
> +{
> +    switch (ms->state) {
> +    case MIG_STATE_ACTIVE:
> +    case MIG_STATE_SETUP:
> +        return true;
> +
> +    default:
> +        return false;
> +
> +    }
> +}

Should CANCELLING also be considered active?  It is on the source->dest
path.

> 
> +static void await_outgoing_return_path_close(MigrationState *ms)
> +{
> +    /*
> +     * If this is a normal exit then the destination will send a SHUT and the
> +     * rp_thread will exit, however if there's an error we need to cause
> +     * it to exit, which we can do by a shutdown.
> +     * (canceling must also shutdown to stop us getting stuck here if
> +     * the destination died at just the wrong place)
> +     */
> +    if (qemu_file_get_error(ms->file) && ms->return_path) {
> +        qemu_file_shutdown(ms->return_path);
> +    }

As mentioned early, I think it's simpler to let these function handle
themselves the case where there is no return path, and call them
unconditionally.

Paolo