On Wed, Nov 3, 2010 at 6:39 PM, Juan Quintela <quint...@redhat.com> wrote:
> Anthony Liguori <anth...@codemonkey.ws> wrote:
>> On 11/03/2010 10:12 AM, Juan Quintela wrote:
>>> Anthony Liguori<anth...@codemonkey.ws> wrote:
>>>
>>>> On 11/03/2010 09:29 AM, Stefan Hajnoczi wrote:
>>>>
>>>>> Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read()
>>>>> handler that deletes its IOHandler is exposed to .fd_write() being
>>>>> called on the deleted IOHandler.
>>>>>
>>>>> This patch fixes deletion so that .fd_read() and .fd_write() are never
>>>>> called on an IOHandler that is marked for deletion.
>>>>>
>>>>> Signed-off-by: Stefan Hajnoczi<stefa...@linux.vnet.ibm.com>
>>>>> ---
>>>>> vl.c | 15 ++++++++-------
>>>>> 1 files changed, 8 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/vl.c b/vl.c
>>>>> index 7038952..6f56123 100644
>>>>> --- a/vl.c
>>>>> +++ b/vl.c
>>>>> @@ -1252,17 +1252,18 @@ void main_loop_wait(int nonblocking)
>>>>> IOHandlerRecord *pioh;
>>>>>
>>>>> QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) {
>>>>> - if (ioh->deleted) {
>>>>> - QLIST_REMOVE(ioh, next);
>>>>> - qemu_free(ioh);
>>>>> - continue;
>>>>> - }
>>>>> - if (ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) {
>>>>> + if (!ioh->deleted&& ioh->fd_read&&
>>>>> FD_ISSET(ioh->fd,&rfds)) {
>>>>> ioh->fd_read(ioh->opaque);
>>>>> }
>>>>> - if (ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) {
>>>>> + if (!ioh->deleted&& ioh->fd_write&&
>>>>> FD_ISSET(ioh->fd,&wfds)) {
>>>>> ioh->fd_write(ioh->opaque);
>>>>> }
>>>>> +
>>>>> + /* Do this last in case read/write handlers marked it for
>>>>> deletion */
>>>>> + if (ioh->deleted) {
>>>>> + QLIST_REMOVE(ioh, next);
>>>>> + qemu_free(ioh);
>>>>> + }
>>>>> }
>>>>>
>>>>>
>>>> This isn't enough. If you end up with a handler deleting the next
>>>> pointer and the current pointer, you'll end up running off the end of
>>>> the list.
>>>>
>>> What is the point of that?
>>>
>>> That a handler can remove itself is ok.
>>> But that a handler can remove also the next in a list that is used for
>>> other things looks pretty insane to me.
>>>
>>
>> If you have multiple file descriptors registered for something and you
>> get an EOF on one of the file descriptors, your clean-up action that
>> happens as a result of closing the session may involve deleting more
>> than one file descriptor callback.
>
> But that is completely wrong. you just put an ioh->deleted=1 for the
> others, and you are right, no?
Yes, other IOHandlerRecords will not be removed from the list yet,
they will only be marked as ioh->deleted=1.
Anthony, are you happy to merge this patch?
Stefan
