[Qemu-devel] Re: [PATCH] Delete IOHandlers after potentially running them

[Anthony Liguori <anthony@codemonkey.ws>]

On 11/03/2010 10:12 AM, Juan Quintela wrote:
> Anthony Liguori<anthony@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<stefanha@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.

Regards,

Anthony Liguori

>> The original commit should be reverted.
>>      
> If that behaviour is expected, then I agree that we should revert it.
> But I would consider that behaviour wrong.
>
> Later, Juan.
>
>    
>> Regards,
>>
>> Anthony Liguori
>>
>>      
>>>        }
>>>
>>>
>>>