diff mbox series

[4/4] hw/misc/macio: Return bool from functions taking errp

Message ID bfce0751e82b031f5e6fb3c32cfbce6325434400.1674001242.git.balaton@eik.bme.hu
State New
Headers show
Series Misc macio clean ups | expand

Commit Message

BALATON Zoltan Jan. 18, 2023, 12:32 a.m. UTC 
Use the convention to return bool from functions which take an error
pointer which allows for callers to pass through their error pointer
without needing a local.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/misc/macio/macio.c | 62 +++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 37 deletions(-)

Comments

Philippe Mathieu-Daudé Jan. 18, 2023, 7:19 a.m. UTC | #1 
On 18/1/23 01:32, BALATON Zoltan wrote:
> Use the convention to return bool from functions which take an error
> pointer which allows for callers to pass through their error pointer
> without needing a local.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/misc/macio/macio.c | 62 +++++++++++++++++--------------------------
>   1 file changed, 25 insertions(+), 37 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Mark Cave-Ayland Feb. 1, 2023, 10:44 p.m. UTC | #2 
On 18/01/2023 00:32, BALATON Zoltan wrote:

> Use the convention to return bool from functions which take an error
> pointer which allows for callers to pass through their error pointer
> without needing a local.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/misc/macio/macio.c | 62 +++++++++++++++++--------------------------
>   1 file changed, 25 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index ae2a9a960d..265c0bbd8d 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -90,13 +90,13 @@ static void macio_bar_setup(MacIOState *s)
>       macio_escc_legacy_setup(s);
>   }
>   
> -static void macio_common_realize(PCIDevice *d, Error **errp)
> +static bool macio_common_realize(PCIDevice *d, Error **errp)
>   {
>       MacIOState *s = MACIO(d);
>       SysBusDevice *sbd;
>   
>       if (!qdev_realize(DEVICE(&s->dbdma), BUS(&s->macio_bus), errp)) {
> -        return;
> +        return false;
>       }
>       sbd = SYS_BUS_DEVICE(&s->dbdma);
>       memory_region_add_subregion(&s->bar, 0x08000,
> @@ -108,14 +108,16 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
>       qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
>       qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>       if (!qdev_realize(DEVICE(&s->escc), BUS(&s->macio_bus), errp)) {
> -        return;
> +        return false;
>       }
>   
>       macio_bar_setup(s);
>       pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->bar);
> +
> +    return true;
>   }
>   
> -static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
> +static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
>                                 qemu_irq irq0, qemu_irq irq1, int dmaid,
>                                 Error **errp)
>   {
> @@ -128,7 +130,7 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
>                                &error_abort);
>       macio_ide_register_dma(ide);
>   
> -    qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> +    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
>   }
>   
>   static void macio_oldworld_realize(PCIDevice *d, Error **errp)
> @@ -136,12 +138,9 @@ static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>       MacIOState *s = MACIO(d);
>       OldWorldMacIOState *os = OLDWORLD_MACIO(d);
>       DeviceState *pic_dev = DEVICE(&os->pic);
> -    Error *err = NULL;
>       SysBusDevice *sbd;
>   
> -    macio_common_realize(d, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    if (!macio_common_realize(d, errp)) {
>           return;
>       }
>   
> @@ -176,21 +175,17 @@ static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>       pmac_format_nvram_partition(&os->nvram, os->nvram.size);
>   
>       /* IDE buses */
> -    macio_realize_ide(s, &os->ide[0],
> -                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_IRQ),
> -                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_DMA_IRQ),
> -                      0x16, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    if (!macio_realize_ide(s, &os->ide[0],
> +                           qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_IRQ),
> +                           qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_DMA_IRQ),
> +                           0x16, errp)) {
>           return;
>       }
>   
> -    macio_realize_ide(s, &os->ide[1],
> -                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_IRQ),
> -                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_DMA_IRQ),
> -                      0x1a, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    if (!macio_realize_ide(s, &os->ide[1],
> +                           qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_IRQ),
> +                           qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_DMA_IRQ),
> +                           0x1a, errp)) {
>           return;
>       }
>   }
> @@ -266,13 +261,10 @@ static void macio_newworld_realize(PCIDevice *d, Error **errp)
>       MacIOState *s = MACIO(d);
>       NewWorldMacIOState *ns = NEWWORLD_MACIO(d);
>       DeviceState *pic_dev = DEVICE(&ns->pic);
> -    Error *err = NULL;
>       SysBusDevice *sbd;
>       MemoryRegion *timer_memory = NULL;
>   
> -    macio_common_realize(d, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    if (!macio_common_realize(d, errp)) {
>           return;
>       }
>   
> @@ -288,21 +280,17 @@ static void macio_newworld_realize(PCIDevice *d, Error **errp)
>       sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(pic_dev, NEWWORLD_ESCCA_IRQ));
>   
>       /* IDE buses */
> -    macio_realize_ide(s, &ns->ide[0],
> -                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_IRQ),
> -                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_DMA_IRQ),
> -                      0x16, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    if (!macio_realize_ide(s, &ns->ide[0],
> +                           qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_IRQ),
> +                           qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_DMA_IRQ),
> +                           0x16, errp)) {
>           return;
>       }
>   
> -    macio_realize_ide(s, &ns->ide[1],
> -                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_IRQ),
> -                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_DMA_IRQ),
> -                      0x1a, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    if (!macio_realize_ide(s, &ns->ide[1],
> +                           qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_IRQ),
> +                           qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_DMA_IRQ),
> +                           0x1a, errp)) {
>           return;
>       }

These days you would move macio_common_realize() into TYPE_MACIO, but anyway:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.
BALATON Zoltan Feb. 1, 2023, 11:54 p.m. UTC | #3 
On Wed, 1 Feb 2023, Mark Cave-Ayland wrote:
> On 18/01/2023 00:32, BALATON Zoltan wrote:
>> Use the convention to return bool from functions which take an error
>> pointer which allows for callers to pass through their error pointer
>> without needing a local.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/misc/macio/macio.c | 62 +++++++++++++++++--------------------------
>>   1 file changed, 25 insertions(+), 37 deletions(-)
>> 
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index ae2a9a960d..265c0bbd8d 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -90,13 +90,13 @@ static void macio_bar_setup(MacIOState *s)
>>       macio_escc_legacy_setup(s);
>>   }
>>   -static void macio_common_realize(PCIDevice *d, Error **errp)
>> +static bool macio_common_realize(PCIDevice *d, Error **errp)
>>   {
>>       MacIOState *s = MACIO(d);
>>       SysBusDevice *sbd;
>>         if (!qdev_realize(DEVICE(&s->dbdma), BUS(&s->macio_bus), errp)) {
>> -        return;
>> +        return false;
>>       }
>>       sbd = SYS_BUS_DEVICE(&s->dbdma);
>>       memory_region_add_subregion(&s->bar, 0x08000,
>> @@ -108,14 +108,16 @@ static void macio_common_realize(PCIDevice *d, Error 
>> **errp)
>>       qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
>>       qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>>       if (!qdev_realize(DEVICE(&s->escc), BUS(&s->macio_bus), errp)) {
>> -        return;
>> +        return false;
>>       }
>>         macio_bar_setup(s);
>>       pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->bar);
>> +
>> +    return true;
>>   }
>>   -static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
>> +static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
>>                                 qemu_irq irq0, qemu_irq irq1, int dmaid,
>>                                 Error **errp)
>>   {
>> @@ -128,7 +130,7 @@ static void macio_realize_ide(MacIOState *s, 
>> MACIOIDEState *ide,
>>                                &error_abort);
>>       macio_ide_register_dma(ide);
>>   -    qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
>> +    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
>>   }
>>     static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>> @@ -136,12 +138,9 @@ static void macio_oldworld_realize(PCIDevice *d, Error 
>> **errp)
>>       MacIOState *s = MACIO(d);
>>       OldWorldMacIOState *os = OLDWORLD_MACIO(d);
>>       DeviceState *pic_dev = DEVICE(&os->pic);
>> -    Error *err = NULL;
>>       SysBusDevice *sbd;
>>   -    macio_common_realize(d, &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> +    if (!macio_common_realize(d, errp)) {
>>           return;
>>       }
>>   @@ -176,21 +175,17 @@ static void macio_oldworld_realize(PCIDevice *d, 
>> Error **errp)
>>       pmac_format_nvram_partition(&os->nvram, os->nvram.size);
>>         /* IDE buses */
>> -    macio_realize_ide(s, &os->ide[0],
>> -                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_IRQ),
>> -                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_DMA_IRQ),
>> -                      0x16, &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> +    if (!macio_realize_ide(s, &os->ide[0],
>> +                           qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_IRQ),
>> +                           qdev_get_gpio_in(pic_dev, 
>> OLDWORLD_IDE0_DMA_IRQ),
>> +                           0x16, errp)) {
>>           return;
>>       }
>>   -    macio_realize_ide(s, &os->ide[1],
>> -                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_IRQ),
>> -                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_DMA_IRQ),
>> -                      0x1a, &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> +    if (!macio_realize_ide(s, &os->ide[1],
>> +                           qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_IRQ),
>> +                           qdev_get_gpio_in(pic_dev, 
>> OLDWORLD_IDE1_DMA_IRQ),
>> +                           0x1a, errp)) {
>>           return;
>>       }
>>   }
>> @@ -266,13 +261,10 @@ static void macio_newworld_realize(PCIDevice *d, 
>> Error **errp)
>>       MacIOState *s = MACIO(d);
>>       NewWorldMacIOState *ns = NEWWORLD_MACIO(d);
>>       DeviceState *pic_dev = DEVICE(&ns->pic);
>> -    Error *err = NULL;
>>       SysBusDevice *sbd;
>>       MemoryRegion *timer_memory = NULL;
>>   -    macio_common_realize(d, &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> +    if (!macio_common_realize(d, errp)) {
>>           return;
>>       }
>>   @@ -288,21 +280,17 @@ static void macio_newworld_realize(PCIDevice *d, 
>> Error **errp)
>>       sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(pic_dev, 
>> NEWWORLD_ESCCA_IRQ));
>>         /* IDE buses */
>> -    macio_realize_ide(s, &ns->ide[0],
>> -                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_IRQ),
>> -                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_DMA_IRQ),
>> -                      0x16, &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> +    if (!macio_realize_ide(s, &ns->ide[0],
>> +                           qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_IRQ),
>> +                           qdev_get_gpio_in(pic_dev, 
>> NEWWORLD_IDE0_DMA_IRQ),
>> +                           0x16, errp)) {
>>           return;
>>       }
>>   -    macio_realize_ide(s, &ns->ide[1],
>> -                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_IRQ),
>> -                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_DMA_IRQ),
>> -                      0x1a, &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> +    if (!macio_realize_ide(s, &ns->ide[1],
>> +                           qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_IRQ),
>> +                           qdev_get_gpio_in(pic_dev, 
>> NEWWORLD_IDE1_DMA_IRQ),
>> +                           0x1a, errp)) {
>>           return;
>>       }
>
> These days you would move macio_common_realize() into TYPE_MACIO, but anyway:

Maybe in further patches later as I think we'll need more macio changes in 
the future but for now this is enough to simplify it a bit.

Regards,
BALATON Zoltan

> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
>
> ATB,
>
> Mark.
>
>
diff mbox series

Patch

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index ae2a9a960d..265c0bbd8d 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -90,13 +90,13 @@  static void macio_bar_setup(MacIOState *s)
     macio_escc_legacy_setup(s);
 }
 
-static void macio_common_realize(PCIDevice *d, Error **errp)
+static bool macio_common_realize(PCIDevice *d, Error **errp)
 {
     MacIOState *s = MACIO(d);
     SysBusDevice *sbd;
 
     if (!qdev_realize(DEVICE(&s->dbdma), BUS(&s->macio_bus), errp)) {
-        return;
+        return false;
     }
     sbd = SYS_BUS_DEVICE(&s->dbdma);
     memory_region_add_subregion(&s->bar, 0x08000,
@@ -108,14 +108,16 @@  static void macio_common_realize(PCIDevice *d, Error **errp)
     qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
     qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
     if (!qdev_realize(DEVICE(&s->escc), BUS(&s->macio_bus), errp)) {
-        return;
+        return false;
     }
 
     macio_bar_setup(s);
     pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->bar);
+
+    return true;
 }
 
-static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
+static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
                               qemu_irq irq0, qemu_irq irq1, int dmaid,
                               Error **errp)
 {
@@ -128,7 +130,7 @@  static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
                              &error_abort);
     macio_ide_register_dma(ide);
 
-    qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
+    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
 }
 
 static void macio_oldworld_realize(PCIDevice *d, Error **errp)
@@ -136,12 +138,9 @@  static void macio_oldworld_realize(PCIDevice *d, Error **errp)
     MacIOState *s = MACIO(d);
     OldWorldMacIOState *os = OLDWORLD_MACIO(d);
     DeviceState *pic_dev = DEVICE(&os->pic);
-    Error *err = NULL;
     SysBusDevice *sbd;
 
-    macio_common_realize(d, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!macio_common_realize(d, errp)) {
         return;
     }
 
@@ -176,21 +175,17 @@  static void macio_oldworld_realize(PCIDevice *d, Error **errp)
     pmac_format_nvram_partition(&os->nvram, os->nvram.size);
 
     /* IDE buses */
-    macio_realize_ide(s, &os->ide[0],
-                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_IRQ),
-                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_DMA_IRQ),
-                      0x16, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!macio_realize_ide(s, &os->ide[0],
+                           qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_IRQ),
+                           qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_DMA_IRQ),
+                           0x16, errp)) {
         return;
     }
 
-    macio_realize_ide(s, &os->ide[1],
-                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_IRQ),
-                      qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_DMA_IRQ),
-                      0x1a, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!macio_realize_ide(s, &os->ide[1],
+                           qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_IRQ),
+                           qdev_get_gpio_in(pic_dev, OLDWORLD_IDE1_DMA_IRQ),
+                           0x1a, errp)) {
         return;
     }
 }
@@ -266,13 +261,10 @@  static void macio_newworld_realize(PCIDevice *d, Error **errp)
     MacIOState *s = MACIO(d);
     NewWorldMacIOState *ns = NEWWORLD_MACIO(d);
     DeviceState *pic_dev = DEVICE(&ns->pic);
-    Error *err = NULL;
     SysBusDevice *sbd;
     MemoryRegion *timer_memory = NULL;
 
-    macio_common_realize(d, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!macio_common_realize(d, errp)) {
         return;
     }
 
@@ -288,21 +280,17 @@  static void macio_newworld_realize(PCIDevice *d, Error **errp)
     sysbus_connect_irq(sbd, 1, qdev_get_gpio_in(pic_dev, NEWWORLD_ESCCA_IRQ));
 
     /* IDE buses */
-    macio_realize_ide(s, &ns->ide[0],
-                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_IRQ),
-                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_DMA_IRQ),
-                      0x16, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!macio_realize_ide(s, &ns->ide[0],
+                           qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_IRQ),
+                           qdev_get_gpio_in(pic_dev, NEWWORLD_IDE0_DMA_IRQ),
+                           0x16, errp)) {
         return;
     }
 
-    macio_realize_ide(s, &ns->ide[1],
-                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_IRQ),
-                      qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_DMA_IRQ),
-                      0x1a, &err);
-    if (err) {
-        error_propagate(errp, err);
+    if (!macio_realize_ide(s, &ns->ide[1],
+                           qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_IRQ),
+                           qdev_get_gpio_in(pic_dev, NEWWORLD_IDE1_DMA_IRQ),
+                           0x1a, errp)) {
         return;
     }