[10/12] isa: Clean up inappropriate hw_error()

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@redhat.com>
---
 hw/isa/isa-bus.c | 11 -----------
 1 file changed, 11 deletions(-)

On 12/10/2015 12:29 PM, 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.

Maybe we can be a little nicer add an assert ? :)

Thanks,
Marcel

>
> 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@redhat.com>
> ---
>   hw/isa/isa-bus.c | 11 -----------
>   1 file changed, 11 deletions(-)
>
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index af6ffd6..630054c 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -63,9 +63,6 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion* address_space,
>
>   void isa_bus_irqs(ISABus *bus, qemu_irq *irqs)
>   {
> -    if (!bus) {
> -        hw_error("Can't set isa irqs with no isa bus present.");
> -    }
>       bus->irqs = irqs;
>   }
>
> @@ -137,10 +134,6 @@ ISADevice *isa_create(ISABus *bus, const char *name)
>   {
>       DeviceState *dev;
>
> -    if (!bus) {
> -        hw_error("Tried to create isa device %s with no isa bus present.",
> -                 name);
> -    }
>       dev = qdev_create(BUS(bus), name);
>       return ISA_DEVICE(dev);
>   }
> @@ -149,10 +142,6 @@ ISADevice *isa_try_create(ISABus *bus, const char *name)
>   {
>       DeviceState *dev;
>
> -    if (!bus) {
> -        hw_error("Tried to create isa device %s with no isa bus present.",
> -                 name);
> -    }
>       dev = qdev_try_create(BUS(bus), name);
>       return ISA_DEVICE(dev);
>   }
>

Marcel Apfelbaum <marcel.apfelbaum@gmail.com> writes:

> On 12/10/2015 12:29 PM, 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.
>
> Maybe we can be a little nicer add an assert ? :)

assert(p) before dereferencing p only converts one kind of crash into
another one.  I tend to do it only when the assert(p) does double-duty
as useful documentation.  Or perhaps when I think there's a real risk of
running into !p in an environment where core dumps are off[*] and
reproducing the failure with a debugger attached could be hard.

To use these three functions, you need an ISABus *.  How could you end
up with a bad one?

* You forget to create the ISA bus, and the compiler is too confused to
  notice.  You'll pass an unitialized ISABus, and asserting it's not
  null is unlikely to help.

* You create multiple ISA buses (that's the only way creating one can
  fail) *and* forget to check for errors.  If you pull that off, I'd
  expect it to explode even in light testing.

* Your pointer gets corrupted between correct initialization and use.
  Asserting it's not null is unlikely to help.


[*] Switching them off on a development machine forfeits your
developer's license, as far as I'm concerned :)

On 12/10/2015 03:09 PM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> writes:
>
>> On 12/10/2015 12:29 PM, 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.
>>
>> Maybe we can be a little nicer add an assert ? :)
>
> assert(p) before dereferencing p only converts one kind of crash into
> another one.  I tend to do it only when the assert(p) does double-duty
> as useful documentation.  Or perhaps when I think there's a real risk of
> running into !p in an environment where core dumps are off[*] and
> reproducing the failure with a debugger attached could be hard.
>
> To use these three functions, you need an ISABus *.  How could you end
> up with a bad one?
>
> * You forget to create the ISA bus, and the compiler is too confused to
>    notice.  You'll pass an unitialized ISABus, and asserting it's not
>    null is unlikely to help.
>
> * You create multiple ISA buses (that's the only way creating one can
>    fail) *and* forget to check for errors.  If you pull that off, I'd
>    expect it to explode even in light testing.

This is the scenario I was referring to.

You are perfectly right that using assert will change a crash into another,
but when I am "succeeding" to crash some code (and I am good at it :)),
if I see a NULL pointer de-reference I am starting to wonder if *is possible*
to have a NULL pointer there and the developer didn't take that into account.
(it couldn't me my bug, right, it got be his :))

However, if I see an assert crash *I know* the developer did not expect a NULL
pointer to be passed at all.

For this specific scenario,  (multiple ISA buses) maybe is such a strange case
that we don't need to bother with an assert.

Thanks for the detailed explanation,
Marcel

>
> * Your pointer gets corrupted between correct initialization and use.
>    Asserting it's not null is unlikely to help.
>
>
> [*] Switching them off on a development machine forfeits your
> developer's license, as far as I'm concerned :)
>

Le 10/12/2015 11:29, Markus Armbruster a écrit :
> 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@redhat.com>
> ---

Creating a second ISA bus raises an error in isa_bus_new, so
Reviewed-by: Hervé Poussineau <hpoussin@reactos.org>