From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=35318 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PFmRt-0003Vf-P8 for qemu-devel@nongnu.org; Tue, 09 Nov 2010 06:33:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PFmRV-0004ws-2H for qemu-devel@nongnu.org; Tue, 09 Nov 2010 06:33:13 -0500 Received: from mail-ww0-f53.google.com ([74.125.82.53]:35155) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PFmRU-0004wa-UL for qemu-devel@nongnu.org; Tue, 09 Nov 2010 06:32:49 -0500 Received: by wwb29 with SMTP id 29so1257923wwb.10 for ; Tue, 09 Nov 2010 03:32:47 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1288794584-6099-1-git-send-email-stefanha@linux.vnet.ibm.com> <4CD17861.2060806@codemonkey.ws> <4CD19F4C.2060906@codemonkey.ws> Date: Tue, 9 Nov 2010 11:32:47 +0000 Message-ID: Subject: Re: [Qemu-devel] Re: [PATCH] Delete IOHandlers after potentially running them From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Juan Quintela , Stefan Hajnoczi , qemu-devel@nongnu.org On Wed, Nov 3, 2010 at 6:39 PM, Juan Quintela wrote: > Anthony Liguori wrote: >> On 11/03/2010 10:12 AM, Juan Quintela wrote: >>> Anthony Liguori =A0wrote: >>> >>>> 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 neve= r >>>>> called on an IOHandler that is marked for deletion. >>>>> >>>>> Signed-off-by: Stefan Hajnoczi >>>>> --- >>>>> =A0 =A0vl.c | =A0 15 ++++++++------- >>>>> =A0 =A01 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) >>>>> =A0 =A0 =A0 =A0 =A0 =A0IOHandlerRecord *pioh; >>>>> >>>>> =A0 =A0 =A0 =A0 =A0 =A0QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pio= h) { >>>>> - =A0 =A0 =A0 =A0 =A0 =A0if (ioh->deleted) { >>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0QLIST_REMOVE(ioh, next); >>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0qemu_free(ioh); >>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0continue; >>>>> - =A0 =A0 =A0 =A0 =A0 =A0} >>>>> - =A0 =A0 =A0 =A0 =A0 =A0if (ioh->fd_read&& =A0 FD_ISSET(ioh->fd,&rfd= s)) { >>>>> + =A0 =A0 =A0 =A0 =A0 =A0if (!ioh->deleted&& =A0 ioh->fd_read&& =A0 F= D_ISSET(ioh->fd,&rfds)) { >>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ioh->fd_read(ioh->opaque); >>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} >>>>> - =A0 =A0 =A0 =A0 =A0 =A0if (ioh->fd_write&& =A0 FD_ISSET(ioh->fd,&wf= ds)) { >>>>> + =A0 =A0 =A0 =A0 =A0 =A0if (!ioh->deleted&& =A0 ioh->fd_write&& =A0 = FD_ISSET(ioh->fd,&wfds)) { >>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ioh->fd_write(ioh->opaque); >>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} >>>>> + >>>>> + =A0 =A0 =A0 =A0 =A0 =A0/* Do this last in case read/write handlers = marked it for deletion */ >>>>> + =A0 =A0 =A0 =A0 =A0 =A0if (ioh->deleted) { >>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0QLIST_REMOVE(ioh, next); >>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0qemu_free(ioh); >>>>> + =A0 =A0 =A0 =A0 =A0 =A0} >>>>> =A0 =A0 =A0 =A0 =A0 =A0} >>>>> >>>>> >>>> This isn't enough. =A0If 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. =A0you just put an ioh->deleted=3D1 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=3D1. Anthony, are you happy to merge this patch? Stefan