On Sun, Jun 14, 2009 at 08:25:43AM -0400, Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
> > On Thu, Jun 11, 2009 at 04:16:47PM +0300, Michael S. Tsirkin wrote:
> >
> >>> +
> >>> + ret = file->f_op->poll(file, &irqfd->pt);
> >>> + if (ret < 0)
> >>> + goto fail;
> >>>
> >
> > Looking at it some more, we have:
> > struct file_operations {
> > ....
> > unsigned int (*poll) (struct file *, struct poll_table_struct *);
> >
> > So the comparison above does not seem to make sense:
> > it seems that the return value from poll can not be negative.
> >
>
> Indeed. Will fix.
> > Will the callback be executed if someone did a write to eventfd
> > before we attached it? If no, maybe we should call it here
> > if ret != 0.
> >
>
> I do the cleanup in case the callback has been called, but poll() fails
> somewhere internally afterwards. Perhaps this is not a realistic
> scenario, but it was my motivation for adding the wqh cleanup.
Could it be that poll returns the event mask and you mistake it for
error?
> >
> >>> +
> >>> + irqfd->file = file;
> >>> +
> >>> + mutex_lock(&kvm->lock);
> >>> + list_add_tail(&irqfd->list, &kvm->irqfds);
> >>> + mutex_unlock(&kvm->lock);
> >>> +
> >>> + return 0;
> >>> +
> >>> +fail:
> >>> + if (irqfd->wqh)
> >>> + remove_wait_queue(irqfd->wqh, &irqfd->wait);
> >>>
> >> Why are these 2 lines here? Either we might get a callback even though
> >> poll failed - and then this test without lock is probably racy -
> >> or we can't, and then we can replace the above with BUG_ON(irqfd->wqh).
> >>
> >> Which is it? I think the later ...
> >>
> >>
> >>
> >>> +
> >>> + if (file && !IS_ERR(file))
> >>> + fput(file);
> >>> +
> >>> + kfree(irqfd);
> >>> + return ret;
> >>> +}
> >>> +
> >>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
