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

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

[Stefan Hajnoczi]

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);
+            }
         }
     }
 
-- 
1.7.2.3

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

[Juan Quintela]

Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> 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.

Reviewed by: Juan Quintela <quintela@redhat.com>

But once here, what (appart from networking) reads and writes to the
same fd?  And that removes the IOHandler on read while there are write
stuff pending?

Later, Juan.

> 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);
> +            }
>          }
>      }

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

[Stefan Hajnoczi]

On Wed, Nov 3, 2010 at 2:39 PM, Juan Quintela <quintela@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> 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.
>
> Reviewed by: Juan Quintela <quintela@redhat.com>
>
> But once here, what (appart from networking) reads and writes to the
> same fd?  And that removes the IOHandler on read while there are write
> stuff pending?

VNC

Stefan

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

[Anthony Liguori]

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.

The original commit should be reverted.

Regards,

Anthony Liguori

>       }
>
>    

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

[Anthony Liguori]

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);
> +            }
>    

Actually, on second thought, I think this is correct although I still 
think reverting is a better strategy.

Regards,

Anthony Liguori

>           }
>       }
>
>    

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

[Juan Quintela]

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.

> 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
>
>>       }
>>
>>    

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

[Anthony Liguori]

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
>>
>>      
>>>        }
>>>
>>>
>>>        

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

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> 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.

But that is completely wrong.  you just put an ioh->deleted=1 for the
others, and you are right, no?

Later, Juan.

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

[Stefan Hajnoczi]

On Wed, Nov 3, 2010 at 6:39 PM, Juan Quintela <quintela@redhat.com> wrote:
> Anthony Liguori <anthony@codemonkey.ws> wrote:
>> 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.
>
> 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

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

[Anthony Liguori]

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>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   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);
> +            }
>           }
>       }
>
>