From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1MDiF2-0007gj-A9
	for qemu-devel@nongnu.org; Mon, 08 Jun 2009 13:02:36 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1MDiEx-0007Xx-SJ
	for qemu-devel@nongnu.org; Mon, 08 Jun 2009 13:02:36 -0400
Received: from [199.232.76.173] (port=47726 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1MDiEx-0007Xm-L3
	for qemu-devel@nongnu.org; Mon, 08 Jun 2009 13:02:31 -0400
Received: from mx2.redhat.com ([66.187.237.31]:56035)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <uril@redhat.com>) id 1MDiEv-0004Mh-Iv
	for qemu-devel@nongnu.org; Mon, 08 Jun 2009 13:02:31 -0400
Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26])
	by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n58H2Saq030405
	for <qemu-devel@nongnu.org>; Mon, 8 Jun 2009 13:02:28 -0400
Message-ID: <4A2D4421.4020206@redhat.com>
Date: Mon, 08 Jun 2009 20:02:25 +0300
From: Uri Lublin <uril@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH] migrate_fd_close: delete associated
	io-handler before closing the fd
References: <1244460481-13575-1-git-send-email-uril@redhat.com>	
	<1244476182.3632.40.camel@blaa> <4A2D34A7.1080208@redhat.com>
	<1244479094.3632.60.camel@blaa>
In-Reply-To: <1244479094.3632.60.camel@blaa>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
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.gnu.org/pipermail/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: Mark McLoughlin <markmc@redhat.com>
Cc: qemu-devel@nongnu.org

On 06/08/2009 07:38 PM, Mark McLoughlin wrote:
>
> Would the below patch working equally well? But then again, we should
> really remove the I/O handler before closing the fd.

I think it should work too.
My patch does remove the I/O handler before closing the fd.

>
> (The close(s->fd) in migration_fd_cleanup() looks like it can never
> happen - perhaps we should remove it)

Perhaps we should. I think it's like a "plan b" in case qemu_fclose did not 
actually closed the file descriptor (which, as you mentioned, currently, can 
never happen).

> diff --git a/migration.c b/migration.c
> index 401383c..078967f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -154,13 +154,21 @@ void migrate_fd_error(FdMigrationState *s)
>
>   void migrate_fd_cleanup(FdMigrationState *s)
>   {
> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);

Would it be safer to keep the I/O handler deletion before the call to 
qemu_fclose (in addition to adding the one below), or it does not matter.


> +    int fd;
> +
> +    /* qemu_fclose() can cause I/O to be flushed (see buffered_close())
> +     * which, in turn, can cause an I/O handler to be registered. We
> +     * need to delay removing the I/O handler until after qemu_fclose().
> +     */
> +    fd = s->fd;
>
>       if (s->file) {
>           dprintf("closing file\n");
>           qemu_fclose(s->file);
>       }
>
> +    qemu_set_fd_handler2(fd, NULL, NULL, NULL, NULL);
> +
>       if (s->fd != -1)
>           close(s->fd);
>
>

Regards,
     Uri.