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

[Richard Henderson <rth@twiddle.net>]

On 12/17/2015 06:27 AM, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> 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 <rth@twiddle.net>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Cc: "Hervé Poussineau" <hpoussin@reactos.org>
>>> Cc: Aurelien Jarno <aurelien@aurel32.net>
>>> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
>>> Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>
>>
>> I'd prefer an assert just in case.
>
> I understand "prefer", I don't understand "just in case" :)
>
> 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:

I'm not a fan of sprinkling obvious assertions everywhere either.  I think the 
patch is fine as-is.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~