Re: [Qemu-devel] [PATCH] do not call monitor_resume() from migrate_fd_put_buffer() error path

[Michael Tokarev]

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