Re: [Qemu-devel] [PATCH v2 11/13] isa: Clean up inappropriate hw_error()

[Michael S. Tsirkin]

On Thu, Dec 17, 2015 at 03:27:36PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> 
> > 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() when
> >> 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.
> >> 
> >> Since the previous commit, passing a null bus is a programming error.
> >> Drop the hw_error() and simply let it crash.
> >> 
> >> Cc: Richard Henderson <address@hidden>
> >> Cc: "Michael S. Tsirkin" <address@hidden>
> >> Cc: "Hervé Poussineau" <address@hidden>
> >> Cc: Aurelien Jarno <address@hidden>
> >> Cc: Mark Cave-Ayland <address@hidden>
> >> Signed-off-by: Markus Armbruster <address@hidden>
> >> Reviewed-by: Hervé Poussineau <address@hidden>
> >
> > I'd prefer an assert just in case.
> 
> 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.
> 
> Doesn't make anything safer, not even just in case something happens we
> thought was impossible.
> 
> Does print a message before crashing that some developers may find
> useful.
> 
> Might make our belief that null can't happen a bit more explicit.
> 
> 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:
> 
>     $ scripts/get_maintainer.pl -f hw/isa/isa-bus.c 
>     get_maintainer.pl: No maintainers found, printing recent contributors.
>     get_maintainer.pl: Do not blindly cc: them on patches!  Use common sense.
> 
>     "Hervé Poussineau" <address@hidden> (commit_signer:4/6=67%)
>     Markus Armbruster <address@hidden> (commit_signer:3/6=50%)
>     Leon Alrae <address@hidden> (commit_signer:2/6=33%)
>     Paolo Bonzini <address@hidden> (commit_signer:2/6=33%)
>     Dave Airlie <address@hidden> (commit_signer:1/6=17%)
> 
> Considering all of the above, do you want me to add the assertions?

Only in case you want my Reviewed-by.

-- 
MST