| Message ID | 1450354795-31608-12-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > --- > 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); > } > -- > 2.4.3
"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: $ 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" <hpoussin@reactos.org> (commit_signer:4/6=67%) Markus Armbruster <armbru@pond.sub.org> (commit_signer:3/6=50%) Leon Alrae <leon.alrae@imgtec.com> (commit_signer:2/6=33%) Paolo Bonzini <pbonzini@redhat.com> (commit_signer:2/6=33%) Dave Airlie <airlied@redhat.com> (commit_signer:1/6=17%) Considering all of the above, do you want me to add the assertions?
On Thu, Dec 17, 2015 at 03:27:36PM +0100, 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" :) 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" <hpoussin@reactos.org> (commit_signer:4/6=67%) > Markus Armbruster <armbru@pond.sub.org> (commit_signer:3/6=50%) > Leon Alrae <leon.alrae@imgtec.com> (commit_signer:2/6=33%) > Paolo Bonzini <pbonzini@redhat.com> (commit_signer:2/6=33%) > Dave Airlie <airlied@redhat.com> (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.
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~
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); }