[Qemu-devel] [PATCH] Make removing IOHandlers safe from within an IOHandler

[Qemu-devel] [PATCH] Make removing IOHandlers safe from within an IOHandler

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

I was getting random SEGVs when disconnecting from the VNC server.  I 
tracked it down to the fact that if you remove a IOHandler from another 
IOHandler, all sorts of badness may result as you're removing entries 
from a linked list while transversing it.

My solution is to simply add a deleted flag to each entry and walk the 
list a second time.  During the second transversal, we'll remove nodes 
that need removing.

Haven't seen the SEGV since I started using this patch.

Regards,

Anthony Liguori

[-- Attachment #2: iohandler-remove.diff --]
[-- Type: text/x-patch, Size: 2123 bytes --]

diff -r 5f92961f382b vl.c
--- a/vl.c	Thu Feb 22 01:48:01 2007 +0000
+++ b/vl.c	Sat Feb 24 11:51:00 2007 -0600
@@ -4462,6 +4462,7 @@ typedef struct IOHandlerRecord {
     IOCanRWHandler *fd_read_poll;
     IOHandler *fd_read;
     IOHandler *fd_write;
+    int deleted;
     void *opaque;
     /* temporary data */
     struct pollfd *ufd;
@@ -4487,8 +4488,7 @@ int qemu_set_fd_handler2(int fd,
             if (ioh == NULL)
                 break;
             if (ioh->fd == fd) {
-                *pioh = ioh->next;
-                qemu_free(ioh);
+		ioh->deleted = 1;
                 break;
             }
             pioh = &ioh->next;
@@ -6157,7 +6157,7 @@ void qemu_system_powerdown_request(void)
 
 void main_loop_wait(int timeout)
 {
-    IOHandlerRecord *ioh, *ioh_next;
+    IOHandlerRecord *ioh;
     fd_set rfds, wfds, xfds;
     int ret, nfds;
     struct timeval tv;
@@ -6192,6 +6192,8 @@ void main_loop_wait(int timeout)
     FD_ZERO(&wfds);
     FD_ZERO(&xfds);
     for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
+	if (ioh->deleted)
+	    continue;
         if (ioh->fd_read &&
             (!ioh->fd_read_poll ||
              ioh->fd_read_poll(ioh->opaque) != 0)) {
@@ -6219,9 +6221,11 @@ void main_loop_wait(int timeout)
 #endif
     ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
     if (ret > 0) {
-        /* XXX: better handling of removal */
-        for(ioh = first_io_handler; ioh != NULL; ioh = ioh_next) {
-            ioh_next = ioh->next;
+	IOHandlerRecord **pioh;
+
+        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
+	    if (ioh->deleted)
+		continue;
             if (FD_ISSET(ioh->fd, &rfds)) {
                 ioh->fd_read(ioh->opaque);
             }
@@ -6229,6 +6233,17 @@ void main_loop_wait(int timeout)
                 ioh->fd_write(ioh->opaque);
             }
         }
+
+	/* remove deleted IO handlers */
+	pioh = &first_io_handler;
+	while (*pioh) {
+	    ioh = *pioh;
+	    if (ioh->deleted) {
+		*pioh = ioh->next;
+		qemu_free(ioh);
+	    } else 
+		pioh = &ioh->next;
+	}
     }
 #if defined(CONFIG_SLIRP)
     if (slirp_inited) {

Re: [Qemu-devel] [PATCH] Make removing IOHandlers safe from within an IOHandler

[Daniel P. Berrange]

On Sat, Feb 24, 2007 at 11:54:17AM -0600, Anthony Liguori wrote:
> I was getting random SEGVs when disconnecting from the VNC server.  I 
> tracked it down to the fact that if you remove a IOHandler from another 
> IOHandler, all sorts of badness may result as you're removing entries 
> from a linked list while transversing it.
> 
> My solution is to simply add a deleted flag to each entry and walk the 
> list a second time.  During the second transversal, we'll remove nodes 
> that need removing.
> 
> Haven't seen the SEGV since I started using this patch.

That's pretty much identical solution to the one I just posted along with
the patches for VNC TLS support, so I can also confirm this approach works 
& solves the SEGV issue.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

[Qemu-devel] [PATCH][UPDATE] Make removing IOHandlers safe from within an IOHandler

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

Daniel P. Berrange wrote:
> On Sat, Feb 24, 2007 at 11:54:17AM -0600, Anthony Liguori wrote:
>   
>> I was getting random SEGVs when disconnecting from the VNC server.  I 
>> tracked it down to the fact that if you remove a IOHandler from another 
>> IOHandler, all sorts of badness may result as you're removing entries 
>> from a linked list while transversing it.
>>
>> My solution is to simply add a deleted flag to each entry and walk the 
>> list a second time.  During the second transversal, we'll remove nodes 
>> that need removing.
>>
>> Haven't seen the SEGV since I started using this patch.
>>     
>
> That's pretty much identical solution to the one I just posted along with
> the patches for VNC TLS support, so I can also confirm this approach works 
> & solves the SEGV issue.
>   

Except I was missing one thing that you're patch had.  I didn't reset 
deleted when a new handler was deleted and added again from the same 
callback.  Attached patch addresses that.

Regards,

Anthony Liguori

> Regards,
> Dan.
>   


[-- Attachment #2: iohandler-remove.diff --]
[-- Type: text/x-patch, Size: 2322 bytes --]

diff -r 5f92961f382b vl.c
--- a/vl.c	Thu Feb 22 01:48:01 2007 +0000
+++ b/vl.c	Sat Feb 24 13:05:14 2007 -0600
@@ -4462,6 +4462,7 @@ typedef struct IOHandlerRecord {
     IOCanRWHandler *fd_read_poll;
     IOHandler *fd_read;
     IOHandler *fd_write;
+    int deleted;
     void *opaque;
     /* temporary data */
     struct pollfd *ufd;
@@ -4487,8 +4488,7 @@ int qemu_set_fd_handler2(int fd,
             if (ioh == NULL)
                 break;
             if (ioh->fd == fd) {
-                *pioh = ioh->next;
-                qemu_free(ioh);
+		ioh->deleted = 1;
                 break;
             }
             pioh = &ioh->next;
@@ -4509,6 +4509,7 @@ int qemu_set_fd_handler2(int fd,
         ioh->fd_read = fd_read;
         ioh->fd_write = fd_write;
         ioh->opaque = opaque;
+	ioh->deleted = 0;
     }
     return 0;
 }
@@ -6157,7 +6158,7 @@ void qemu_system_powerdown_request(void)
 
 void main_loop_wait(int timeout)
 {
-    IOHandlerRecord *ioh, *ioh_next;
+    IOHandlerRecord *ioh;
     fd_set rfds, wfds, xfds;
     int ret, nfds;
     struct timeval tv;
@@ -6192,6 +6193,8 @@ void main_loop_wait(int timeout)
     FD_ZERO(&wfds);
     FD_ZERO(&xfds);
     for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
+	if (ioh->deleted)
+	    continue;
         if (ioh->fd_read &&
             (!ioh->fd_read_poll ||
              ioh->fd_read_poll(ioh->opaque) != 0)) {
@@ -6219,9 +6222,11 @@ void main_loop_wait(int timeout)
 #endif
     ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
     if (ret > 0) {
-        /* XXX: better handling of removal */
-        for(ioh = first_io_handler; ioh != NULL; ioh = ioh_next) {
-            ioh_next = ioh->next;
+	IOHandlerRecord **pioh;
+
+        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
+	    if (ioh->deleted)
+		continue;
             if (FD_ISSET(ioh->fd, &rfds)) {
                 ioh->fd_read(ioh->opaque);
             }
@@ -6229,6 +6234,17 @@ void main_loop_wait(int timeout)
                 ioh->fd_write(ioh->opaque);
             }
         }
+
+	/* remove deleted IO handlers */
+	pioh = &first_io_handler;
+	while (*pioh) {
+	    ioh = *pioh;
+	    if (ioh->deleted) {
+		*pioh = ioh->next;
+		qemu_free(ioh);
+	    } else 
+		pioh = &ioh->next;
+	}
     }
 #if defined(CONFIG_SLIRP)
     if (slirp_inited) {