Re: [PATCH uq/master 2/9] event_notifier: remove event_notifier_test

From: Avi Kivity <avi@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org, mtosatti@redhat.com, kvm@vger.kernel.org,
	anthony.perard@citrix.com, jan.kiszka@siemens.com,
	mst@redhat.com, stefano.stabellini@eu.citrix.com
Subject: Re: [PATCH uq/master 2/9] event_notifier: remove event_notifier_test
Date: Thu, 12 Jul 2012 14:04:35 +0300	[thread overview]
Message-ID: <4FFEAF43.3070907@redhat.com> (raw)
In-Reply-To: <4FFEA738.6040807@redhat.com>

On 07/12/2012 01:30 PM, Paolo Bonzini wrote:
> Il 12/07/2012 11:10, Avi Kivity ha scritto:
>> On 07/05/2012 06:16 PM, Paolo Bonzini wrote:
>>> This is broken; since the eventfd is used in nonblocking mode there
>>> is a race between reading and writing.
>>>
>> 
>>> diff --git a/event_notifier.c b/event_notifier.c
>>> index 2b210f4..c339bfe 100644
>>> --- a/event_notifier.c
>>> +++ b/event_notifier.c
>>> @@ -51,18 +51,3 @@ int event_notifier_test_and_clear(EventNotifier *e)
>>>      int r = read(e->fd, &value, sizeof(value));
>>>      return r == sizeof(value);
>>>  }
>>> -
>>> -int event_notifier_test(EventNotifier *e)
>>> -{
>>> -    uint64_t value;
>>> -    int r = read(e->fd, &value, sizeof(value));
>>> -    if (r == sizeof(value)) {
>>> -        /* restore previous value. */
>>> -        int s = write(e->fd, &value, sizeof(value));
>>> -        /* never blocks because we use EFD_SEMAPHORE.
>>> -         * If we didn't we'd get EAGAIN on overflow
>>> -         * and we'd have to write code to ignore it. */
>>> -        assert(s == sizeof(value));
>>> -    }
>>> -    return r == sizeof(value);
>>> -}
>> 
>> I don't see the race.  Mind explaining?
> 
> The assertion can actually fire, there's nothing that prevents this from
> happening:
> 
>     event_notifier_test()
>         read(fd, &value, 8)
>                                       write(fd, <large value>, 8)
>         write(fd, &value, 8)
> 
> event_notifier_set will always write a 1 and it will take a large amount
> of writes to reach overflow :) but that may not be true of other writers
> using the same file descriptor.

The first write would have overflowed without event_notifier_test(), and
there's no reasonable way to deal with it; nor is there any reason to,
since the limit is so large.

> Then, the comment is wrong in two ways.  First, we do not use
> EFD_SEMAPHORE (though even if we did the only difference is that value
> will be always one).  Second, we cannot write code to ignore EAGAIN,
> because then we've lost the value.
> 
> With blocking I/O things would not be much better, because then
> event_notifier_test() might block on the write.  That would be quite
> surprising.
> 
> If we cared, we could implement the function more easily and corectly
> with poll(), checking for POLLIN in the revents.  But I don't see a
> sensible use case for it anyway.

Right, it's useless.  I'll adjust the comment (and the whitespace fix)
and apply.

-- 
error compiling committee.c: too many arguments to function