From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35883) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpEFp-0000IW-DW for qemu-devel@nongnu.org; Fri, 05 Aug 2011 02:51:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QpEFo-0003Pn-93 for qemu-devel@nongnu.org; Fri, 05 Aug 2011 02:51:33 -0400 Received: from isrv.corpit.ru ([86.62.121.231]:46923) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpEFn-0003Ph-Tj for qemu-devel@nongnu.org; Fri, 05 Aug 2011 02:51:32 -0400 Message-ID: <4E3B92F0.4050801@msgid.tls.msk.ru> Date: Fri, 05 Aug 2011 10:51:28 +0400 From: Michael Tokarev MIME-Version: 1.0 References: <1312383104-9565-1-git-send-email-mjt@msgid.tls.msk.ru> <20110804162441.0045215c@doriath> <4E3AF860.2070005@msgid.tls.msk.ru> <4E3AFA5F.4040708@msgid.tls.msk.ru> <4E3B1B07.2000400@web.de> In-Reply-To: <4E3B1B07.2000400@web.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] do not call monitor_resume() from migrate_fd_put_buffer() error path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: mtosatti@redhat.com, qemu-devel@nongnu.org, Luiz Capitulino 05.08.2011 02:19, Jan Kiszka wrote: [] > Resume on error in migrate_fd_put_buffer raced with migrate_fd_cleanup > triggered via migrate_fd_put_ready called from migrate_fd_connect. > > This migration code is a horrible maze. Patch below tries to move > monitor_resume a bit out of this. Please check if it works for you, then > I'll send it out properly. Yes it's a maze. The patch was mime-damaged, I had to apply it manually, but it wasn't difficult (since it's understandable what it does etc). And now I can't trigger the original problem anymore, with any of my variants. Here we go: (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:cat > /tmp/foo" (qemu) migrate "exec:cat > /tmp/foo" (qemu) migrate "exec:foo" sh: foo: not found (qemu) migrate "exec:foo" sh: foo: not found (qemu) q As you can see, I can hit either of the two cases - with and without extra newline, and in both cases (and in successful case too) it works correctly. There's a difference still between the two - namely that extra newline - but that's something else and merely cosmetic. I also verified that -incoming migration works as expected, in both failure and success cases - and indeed it works. Speaking of the patch: shouldn't migrate_fd_close() be called from migrate_fd_cleanup(), or vise versa, or both be combined into one? In migrate_fd_cleanup(), the new logic is not clear still. New version of it: int migrate_fd_cleanup(FdMigrationState *s) { int ret = 0; qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL); if (s->file) { DPRINTF("closing file\n"); if (qemu_fclose(s->file) != 0) { ret = -1; } s->file = NULL; } else { if (s->mon) { monitor_resume(s->mon); } } if (s->fd != -1) { close(s->fd); s->fd = -1; } return ret; } Why it's EITHER s->file OR s->mon, but not both? Shouldn't the s->mon thing be unconditional? And the whole thing is waiting coroutine conversion badly, since the logic is indeed a maze ;) Thank you! /mjt