isa-fdc: assert replaced by proper error exit

[Qemu-devel] [PATCH] isa-fdc: assert replaced by proper error exit

Posted by Eduardo Otubo 8 years, 2 months ago

When not available, isa-fdc falls into assert instead of proper error
exit. This patch fixes this behavior.

Signed-off-by: Eduardo Otubo <otubo@redhat.com>
---
 hw/block/fdc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 401129073b..0b6def4e1d 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2699,11 +2699,15 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
     fdctrl->dma_chann = isa->dma;
     if (fdctrl->dma_chann != -1) {
         fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
-        assert(fdctrl->dma);
+        if (!fdctrl->dma) {
+            error_setg(errp, "isa-fdc not supported");
+            goto error;
+        }
     }
 
     qdev_set_legacy_instance_id(dev, isa->iobase, 2);
     fdctrl_realize_common(dev, fdctrl, &err);
+error:
     if (err != NULL) {
         error_propagate(errp, err);
         return;
-- 
2.13.5

Re: [Qemu-devel] [PATCH] isa-fdc: assert replaced by proper error exit

Posted by Thomas Huth 8 years, 2 months ago

On 01.09.2017 13:07, Eduardo Otubo wrote:
> When not available, isa-fdc falls into assert instead of proper error
> exit. This patch fixes this behavior.
> 
> Signed-off-by: Eduardo Otubo <otubo@redhat.com>
> ---
>  hw/block/fdc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 401129073b..0b6def4e1d 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2699,11 +2699,15 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
>      fdctrl->dma_chann = isa->dma;
>      if (fdctrl->dma_chann != -1) {
>          fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
> -        assert(fdctrl->dma);
> +        if (!fdctrl->dma) {
> +            error_setg(errp, "isa-fdc not supported");
> +            goto error;
> +        }
>      }
>  
>      qdev_set_legacy_instance_id(dev, isa->iobase, 2);
>      fdctrl_realize_common(dev, fdctrl, &err);
> +error:
>      if (err != NULL) {
>          error_propagate(errp, err);
>          return;

Maybe add the reproducer to the commit message:

 qemu-system-ppc64 -S -machine powernv -device isa-fdc

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH] isa-fdc: assert replaced by proper error exit

Posted by Markus Armbruster 8 years, 2 months ago

Eduardo Otubo <otubo@redhat.com> writes:

> When not available, isa-fdc falls into assert instead of proper error
> exit. This patch fixes this behavior.

When what exactly isn't available?

>
> Signed-off-by: Eduardo Otubo <otubo@redhat.com>
> ---
>  hw/block/fdc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 401129073b..0b6def4e1d 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2699,11 +2699,15 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
>      fdctrl->dma_chann = isa->dma;
>      if (fdctrl->dma_chann != -1) {
>          fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
> -        assert(fdctrl->dma);
> +        if (!fdctrl->dma) {
> +            error_setg(errp, "isa-fdc not supported");
> +            goto error;
> +        }
>      }
>  
>      qdev_set_legacy_instance_id(dev, isa->iobase, 2);
>      fdctrl_realize_common(dev, fdctrl, &err);
> +error:
>      if (err != NULL) {
>          error_propagate(errp, err);
>          return;

The actual patch makes me suspect it's "when ISA DMA isn't available".

Re: [Qemu-devel] [PATCH] isa-fdc: assert replaced by proper error exit

Posted by John Snow 8 years, 2 months ago


On 09/01/2017 07:07 AM, Eduardo Otubo wrote:
> When not available, isa-fdc falls into assert instead of proper error
> exit. This patch fixes this behavior.
> 
> Signed-off-by: Eduardo Otubo <otubo@redhat.com>
> ---
>  hw/block/fdc.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 401129073b..0b6def4e1d 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2699,11 +2699,15 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
>      fdctrl->dma_chann = isa->dma;
>      if (fdctrl->dma_chann != -1) {
>          fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
> -        assert(fdctrl->dma);
> +        if (!fdctrl->dma) {
> +            error_setg(errp, "isa-fdc not supported");
> +            goto error;
> +        }
>      }
>  
>      qdev_set_legacy_instance_id(dev, isa->iobase, 2);
>      fdctrl_realize_common(dev, fdctrl, &err);
> +error:
>      if (err != NULL) {
>          error_propagate(errp, err);
>          return;
> 

I was going to ask if we needn't take care of undoing the portio or IRQ
registration, but it turns out that there is not one single caller of
portio_list_destroy in all of QEMU...

Paolo admitted as much in e3fb0ade83420a86464ee50c71f2daf5641cab10 when
he updated the destroy function :)

It looks like the way we assign IRQs to ISA devices is kind of simple
and not trivial to undo correctly, so instead of barking up that tree,
why don't we hoist the isa_get_dma call up and fail before we try to
initialize portio or IRQs?