Re: [Qemu-devel] [PATCH for-2.6] virtio: Register host notifier handler as external

[Fam Zheng <famz@redhat.com>]

On Thu, 04/21 13:42, Kevin Wolf wrote:
> Am 21.04.2016 um 10:57 hat Fam Zheng geschrieben:
> > On Thu, 04/21 10:07, Kevin Wolf wrote:
> > > Am 21.04.2016 um 07:05 hat Fam Zheng geschrieben:
> > > > On Thu, 04/21 10:04, Fam Zheng wrote:
> > > > > This ensures the bdrv_drained_begin() in block layer is effective and
> > > > > fixes launchpad bug #1570134.
> > > > 
> > > > Forgot to add to the subject, but this patch is for 2.6.
> > > 
> > > Also CCing qemu-stable (this affects 2.5) and qemu-block.
> > > 
> > > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > > > ---
> > > > >  hw/virtio/virtio.c | 7 ++++---
> > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > > index f745c4a..002c2c6 100644
> > > > > --- a/hw/virtio/virtio.c
> > > > > +++ b/hw/virtio/virtio.c
> > > > > @@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
> > > > >                                                 bool set_handler)
> > > > >  {
> > > > >      if (assign && set_handler) {
> > > > > -        event_notifier_set_handler(&vq->host_notifier,
> > > > > -                                   virtio_queue_host_notifier_read);
> > > > > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > > > > +                               true, virtio_queue_host_notifier_read);
> > > > >      } else {
> > > > > -        event_notifier_set_handler(&vq->host_notifier, NULL);
> > > > > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > > > > +                               true, NULL);
> > > > >      }
> > > 
> > > This feels like fixing one special case of a more general problem. Are
> > > we sure that event_notifier_set_handler() should ever result in an
> > > internal handler? The default should probably be external, and block
> > > drivers need to take care to use functions that have the is_external
> > > parameter so they can mark themselves internal.
> > > 
> > 
> > Looks like this is the only event_notifier_set_handler caller that
> > should be fixed.  If we see problem in other devices they can be
> > addressed case by case, I'm not sure marking event_notifier_set_handler
> > as external is a better idea for 2.6. (The other two callers are
> > hyperv.c, which seems never set the handler, and ccid-card-emulated.c,
> > which device I have no idea about.)
> 
> As we don't have a lot of time for 2.6 any more, this local fix is
> probably the right thing to do. It fixes what we know is broken and
> has a very low risk of breaking other things.
> 
> But we should probably review all fd handlers during the 2.7 development
> cycle. I didn't see anything obvious that could possibly call into the
> block layer, but even unrelated and therefore harmless users should
> probably be marked as external to avoid unnecessary traps for future
> development.

Completely agreed. I'm adding this to my todo list for 2.7.

Fam