From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51436) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9Zg8-0005VH-CX for qemu-devel@nongnu.org; Thu, 17 Dec 2015 09:37:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9Zg4-0006ec-JA for qemu-devel@nongnu.org; Thu, 17 Dec 2015 09:37:12 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43246) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9Zg4-0006eX-Dx for qemu-devel@nongnu.org; Thu, 17 Dec 2015 09:37:08 -0500 Date: Thu, 17 Dec 2015 16:37:04 +0200 From: "Michael S. Tsirkin" Message-ID: <20151217163521-mutt-send-email-mst@redhat.com> References: <1450354795-31608-1-git-send-email-armbru@redhat.com> <1450354795-31608-12-git-send-email-armbru@redhat.com> <20151217153756-mutt-send-email-mst@redhat.com> <877fkdjgkn.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <877fkdjgkn.fsf@blackfin.pond.sub.org> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 11/13] isa: Clean up inappropriate hw_error() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: =?iso-8859-1?Q?Herv=E9?= Poussineau , Mark Cave-Ayland , qemu-devel@nongnu.org, Aurelien Jarno , Richard Henderson On Thu, Dec 17, 2015 at 03:27:36PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" writes: >=20 > > On Thu, Dec 17, 2015 at 01:19:53PM +0100, Markus Armbruster wrote: > >> isa_bus_irqs(), isa_create() and isa_try_create() call hw_error() wh= en > >> passed a null bus. Use of hw_error() has always been questionable, > >> because these are used only during machine initialization, and > >> printing CPU registers isn't useful there. > >>=20 > >> Since the previous commit, passing a null bus is a programming error= . > >> Drop the hw_error() and simply let it crash. > >>=20 > >> Cc: Richard Henderson > >> Cc: "Michael S. Tsirkin" > >> Cc: "Herv=E9 Poussineau" > >> Cc: Aurelien Jarno > >> Cc: Mark Cave-Ayland > >> Signed-off-by: Markus Armbruster > >> Reviewed-by: Herv=E9 Poussineau > > > > I'd prefer an assert just in case. >=20 > I understand "prefer", I don't understand "just in case" :) To make debugging a bit less painful in case we missed something. > Adding an assertion here merely converts one kind of crash into another= . >=20 > Doesn't make anything safer, not even just in case something happens we > thought was impossible. >=20 > Does print a message before crashing that some developers may find > useful. >=20 > Might make our belief that null can't happen a bit more explicit. >=20 > My own preference is not to assert the blatantly obvious. However, I'm > certainly willing to defer to a maintainer's or reviewer's preference, > within reason. For what it's worth: >=20 > $ scripts/get_maintainer.pl -f hw/isa/isa-bus.c=20 > get_maintainer.pl: No maintainers found, printing recent contributo= rs. > get_maintainer.pl: Do not blindly cc: them on patches! Use common = sense. >=20 > "Herv=E9 Poussineau" (commit_signer:4/6=3D67= %) > Markus Armbruster (commit_signer:3/6=3D50%) > Leon Alrae (commit_signer:2/6=3D33%) > Paolo Bonzini (commit_signer:2/6=3D33%) > Dave Airlie (commit_signer:1/6=3D17%) >=20 > Considering all of the above, do you want me to add the assertions? Only in case you want my Reviewed-by. --=20 MST