From patchwork Wed Mar 30 19:04:33 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Gleb Natapov X-Patchwork-Id: 88953 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 97078B6F01 for ; Thu, 31 Mar 2011 06:29:42 +1100 (EST) Received: from localhost ([127.0.0.1]:49502 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q50hT-0007lq-IO for incoming@patchwork.ozlabs.org; Wed, 30 Mar 2011 15:05:03 -0400 Received: from [140.186.70.92] (port=56626 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q50h4-0007iN-U0 for qemu-devel@nongnu.org; Wed, 30 Mar 2011 15:04:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q50h3-0006ZN-NR for qemu-devel@nongnu.org; Wed, 30 Mar 2011 15:04:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21200) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q50h3-0006ZG-Dj for qemu-devel@nongnu.org; Wed, 30 Mar 2011 15:04:37 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2UJ4ZxG021182 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 30 Mar 2011 15:04:35 -0400 Received: from dhcp-1-237.tlv.redhat.com (dhcp-1-237.tlv.redhat.com [10.35.1.237]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p2UJ4YTv020571; Wed, 30 Mar 2011 15:04:34 -0400 Received: by dhcp-1-237.tlv.redhat.com (Postfix, from userid 13519) id CD4AD133CEC; Wed, 30 Mar 2011 21:04:33 +0200 (IST) Date: Wed, 30 Mar 2011 21:04:33 +0200 From: Gleb Natapov To: Peter Maydell Subject: Re: [Qemu-devel] [PATCHv3] report that QEMU process was killed by a signal Message-ID: <20110330190433.GE7741@redhat.com> References: <20110315115604.GY10151@redhat.com> <20110330184348.GB7741@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: qemu-devel@nongnu.org X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Wed, Mar 30, 2011 at 07:53:41PM +0100, Peter Maydell wrote: > On 30 March 2011 19:43, Gleb Natapov wrote: > > On Wed, Mar 30, 2011 at 07:39:31PM +0100, Peter Maydell wrote: > >> Unfortunately this patch causes qemu to segfault when killed > >> via ^C (at least on my Ubuntu maverick system). This is because > >> it registers a signal handler with sigaction, but then later > >> the SDL library is initialised and it reinstalls our handler > >> with plain old signal: > >> > >>         ohandler = signal(SIGINT, SDL_HandleSIG); > >>         if ( ohandler != SIG_DFL ) > >>                 signal(SIGINT, ohandler); > >> > > I fixed this in SDL upstream. > > Cool, but we can't rely on the SDL we're linked against being > a bugfixed version. > > >> This is clearly buggy but on the other hand SDL is pretty widely > >> deployed and it's the default QEMU video output method, so I think > >> we need to work around it :-( > >> > >> The most straightforward fix is to get the signal number from > >> argument one and not to bother printing the PID that killed us. > >> > > For debugging purposes pid is useful. We cam register signal handler > > after SDL is initialized though (if waiting for SDL update is not an > > option). > > I'm not convinced about the utility of printing the pid, personally. > Most programs get along fine without printing anything when > they receive a terminal signal. However you're right that it should Well qemu is a bit of special case. It is long running process that takes huge amount of memory and, as suchm it becomes a target of various monitoring script which, when configured incorrectly, start killing perfectly valid guests. In addition killing of the guest looks exactly like guest shutdown to management software because we call shutdow_request in the signal handler. > be straightforward enough to move the signal init. In fact it > looks like this got broken somewhere along the line, the > call to os_setup_signal_handling() is already preceded with a comment: > > /* must be after terminal init, SDL library changes signal handlers */ > os_setup_signal_handling(); > > ...we just aren't actually after the terminal init any more :-( > Exactly. This should do the trick (not tested). Reviewed-by: Peter Maydell --- Gleb. diff --git a/vl.c b/vl.c index 192a240..93aaccf 100644 --- a/vl.c +++ b/vl.c @@ -3148,9 +3148,6 @@ int main(int argc, char **argv, char **envp) cpu_synchronize_all_post_init(); - /* must be after terminal init, SDL library changes signal handlers */ - os_setup_signal_handling(); - set_numa_modes(); current_machine = machine; @@ -3206,6 +3203,9 @@ int main(int argc, char **argv, char **envp) break; } + /* must be after terminal init, SDL library changes signal handlers */ + os_setup_signal_handling(); + #ifdef CONFIG_VNC /* init remote displays */ if (vnc_display) {