[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] do not call monitor_resume() from migrate_fd_pu
From: |
Michael Tokarev |
Subject: |
Re: [Qemu-devel] [PATCH] do not call monitor_resume() from migrate_fd_put_buffer() error path |
Date: |
Fri, 05 Aug 2011 10:51:28 +0400 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.1.16) Gecko/20110704 Icedove/3.0.11 |
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