From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52270 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PDhNh-0007tr-Kp for qemu-devel@nongnu.org; Wed, 03 Nov 2010 13:44:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PDhNg-0007uw-9E for qemu-devel@nongnu.org; Wed, 03 Nov 2010 13:44:17 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:64386) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PDhNg-0007uo-6l for qemu-devel@nongnu.org; Wed, 03 Nov 2010 13:44:16 -0400 Received: by yxd5 with SMTP id 5so721775yxd.4 for ; Wed, 03 Nov 2010 10:44:15 -0700 (PDT) Message-ID: <4CD19F4C.2060906@codemonkey.ws> Date: Wed, 03 Nov 2010 12:43:40 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1288794584-6099-1-git-send-email-stefanha@linux.vnet.ibm.com> <4CD17861.2060806@codemonkey.ws> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] Delete IOHandlers after potentially running them List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: Stefan Hajnoczi , qemu-devel@nongnu.org On 11/03/2010 10:12 AM, Juan Quintela wrote: > Anthony Liguori 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 >>> --- >>> 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 >> >> >>> } >>> >>> >>>