From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1HL2Gm-00075q-JC for qemu-devel@nongnu.org; Sat, 24 Feb 2007 14:09:20 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1HL2Gk-00074e-0V for qemu-devel@nongnu.org; Sat, 24 Feb 2007 14:09:20 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1HL2Gj-00074Z-MI for qemu-devel@nongnu.org; Sat, 24 Feb 2007 14:09:17 -0500 Received: from nz-out-0506.google.com ([64.233.162.234]) by monty-python.gnu.org with esmtp (Exim 4.52) id 1HL2Gj-0002wb-AD for qemu-devel@nongnu.org; Sat, 24 Feb 2007 14:09:17 -0500 Received: by nz-out-0506.google.com with SMTP id i11so1454466nzi for ; Sat, 24 Feb 2007 11:09:16 -0800 (PST) Message-ID: <45E08D59.7080903@codemonkey.ws> Date: Sat, 24 Feb 2007 13:09:13 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <45E07BC9.1080209@codemonkey.ws> <20070224183914.GA15380@redhat.com> In-Reply-To: <20070224183914.GA15380@redhat.com> Content-Type: multipart/mixed; boundary="------------040908050103000500040006" Subject: [Qemu-devel] [PATCH][UPDATE] Make removing IOHandlers safe from within an IOHandler Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org This is a multi-part message in MIME format. --------------040908050103000500040006 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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. > --------------040908050103000500040006 Content-Type: text/x-patch; name="iohandler-remove.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="iohandler-remove.diff" 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) { --------------040908050103000500040006--