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

[Stefan Hajnoczi]

On Wed, Nov 3, 2010 at 6:39 PM, Juan Quintela <address@hidden> wrote:
> Anthony Liguori <address@hidden> wrote:
>> On 11/03/2010 10:12 AM, Juan Quintela wrote:
>>> Anthony Liguori<address@hidden>  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<address@hidden>
>>>>> ---
>>>>>    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