Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Anthony Liguori]

On 09/04/2011 09:03 AM, Avi Kivity wrote:
> On 08/22/2011 04:12 PM, Anthony Liguori wrote:
> > This patch changes qemu_set_fd_handler to be implemented in terms of
> > g_io_add_watch(). The semantics are a bit different so some glue is
> > required.
> >
> > qemu_set_fd_handler2 is much harder to convert because of its use of
> > polling.
> >
> > The glib main loop has the major of advantage of having a proven
> > thread safe
> > architecture. By using the glib main loop instead of our own, it will
> > allow us
> > to eventually introduce multiple I/O threads.
> >
> > I'm pretty sure that this will work on Win32, but I would appreciate
> > some help
> > testing. I think the semantics of g_io_channel_unix_new() are really
> > just tied
> > to the notion of a "unix fd" and not necessarily unix itself.
>
> 'git bisect' fingered this as responsible for breaking
> qcow2+cache=unsafe. I think there's an off-by-one here and the guilty
> patch is the one that switches the main loop, but that's just a guess.
>
> The symptoms are that a guest that is restarted (new qemu process) after
> install doesn't make it through grub - some image data didn't make it do
> disk. With qcow2 and cache=unsafe that can easily happen through exit
> notifiers not being run and the entire qcow2 metadata being thrown out
> the window. Running with raw+cache=unsafe works.

Can you share your full command line?

Nothing that would be in the obvious path actually uses 
qemu_set_fd_handler...

Regards,

Anthony Liguori

> > diff --git a/iohandler.c b/iohandler.c
> > index 4deae1e..5ef66fb 100644
> > --- a/iohandler.c
> > +++ b/iohandler.c
> > @@ -80,12 +80,67 @@ int qemu_set_fd_handler2(int fd,
> > return 0;
> > }
> >
> > +typedef struct IOTrampoline
> > +{
> > + GIOChannel *chan;
> > + IOHandler *fd_read;
> > + IOHandler *fd_write;
> > + void *opaque;
> > + guint tag;
> > +} IOTrampoline;
> > +
> > +static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond,
> > gpointer opaque)
> > +{
> > + IOTrampoline *tramp = opaque;
> > +
> > + if (tramp->opaque == NULL) {
> > + return FALSE;
> > + }
> > +
> > + if ((cond& G_IO_IN)&& tramp->fd_read) {
> > + tramp->fd_read(tramp->opaque);
> > + }
> > +
> > + if ((cond& G_IO_OUT)&& tramp->fd_write) {
> > + tramp->fd_write(tramp->opaque);
> > + }
> > +
> > + return TRUE;
> > +}
> > +
> > int qemu_set_fd_handler(int fd,
> > IOHandler *fd_read,
> > IOHandler *fd_write,
> > void *opaque)
> > {
> > - return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> > + static IOTrampoline fd_trampolines[FD_SETSIZE];
> > + IOTrampoline *tramp =&fd_trampolines[fd];
> > +
> > + if (tramp->tag != 0) {
> > + g_io_channel_unref(tramp->chan);
> > + g_source_remove(tramp->tag);
> > + }
> > +
> > + if (opaque) {
> > + GIOCondition cond = 0;
> > +
> > + tramp->fd_read = fd_read;
> > + tramp->fd_write = fd_write;
> > + tramp->opaque = opaque;
> > +
> > + if (fd_read) {
> > + cond |= G_IO_IN | G_IO_ERR;
> > + }
> > +
> > + if (fd_write) {
> > + cond |= G_IO_OUT | G_IO_ERR;
> > + }
> > +
> > + tramp->chan = g_io_channel_unix_new(fd);
> > + tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp);
> > + }
> > +
> > + return 0;
> > }
> >
> > void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set
> > *writefds, fd_set *xfds)