This patch introduces a local Error **errp = NULL and
replaces error_abort with errp in these paths.
This makes error handling more consistent with QEMU guidelines and avoids
using error_abort in cases where failure should not be treated as a
programming error.
Discussion:
object_property_set_link() can return false only when it fails, and it
sets an error when it fails. Since passing &error_abort causes an abort,
the function never returns false, and the return statement is effectively
dead code. If failure is considered a programming error, using
&error_abort is correct. However, if failure is not a programming error,
passing &error_abort is wrong, and errp should be used instead. This
patch follows the latter approach by replacing error_abort with errp.
https://patchwork.kernel.org/project/qemu-devel/patch/20250717034054.1903991-3-jamin_lin@aspeedtech.com/#26540626
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
hw/arm/aspeed_ast27x0-fc.c | 27 ++++++++++++++++-----------
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
index 7087be4288..b15cb94c39 100644
--- a/hw/arm/aspeed_ast27x0-fc.c
+++ b/hw/arm/aspeed_ast27x0-fc.c
@@ -61,6 +61,7 @@ static void ast2700fc_ca35_init(MachineState *machine)
Ast2700FCState *s = AST2700A1FC(machine);
AspeedSoCState *soc;
AspeedSoCClass *sc;
+ Error **errp = NULL;
object_initialize_child(OBJECT(s), "ca35", &s->ca35, "ast2700-a1");
soc = ASPEED_SOC(&s->ca35);
@@ -71,20 +72,20 @@ static void ast2700fc_ca35_init(MachineState *machine)
memory_region_add_subregion(get_system_memory(), 0, &s->ca35_memory);
if (!memory_region_init_ram(&s->ca35_dram, OBJECT(&s->ca35), "ca35-dram",
- AST2700FC_BMC_RAM_SIZE, &error_abort)) {
+ AST2700FC_BMC_RAM_SIZE, errp)) {
return;
}
if (!object_property_set_link(OBJECT(&s->ca35), "memory",
OBJECT(&s->ca35_memory),
- &error_abort)) {
+ errp)) {
return;
};
if (!object_property_set_link(OBJECT(&s->ca35), "dram",
- OBJECT(&s->ca35_dram), &error_abort)) {
+ OBJECT(&s->ca35_dram), errp)) {
return;
}
if (!object_property_set_int(OBJECT(&s->ca35), "ram-size",
- AST2700FC_BMC_RAM_SIZE, &error_abort)) {
+ AST2700FC_BMC_RAM_SIZE, errp)) {
return;
}
@@ -95,15 +96,15 @@ static void ast2700fc_ca35_init(MachineState *machine)
}
}
if (!object_property_set_int(OBJECT(&s->ca35), "hw-strap1",
- AST2700FC_HW_STRAP1, &error_abort)) {
+ AST2700FC_HW_STRAP1, errp)) {
return;
}
if (!object_property_set_int(OBJECT(&s->ca35), "hw-strap2",
- AST2700FC_HW_STRAP2, &error_abort)) {
+ AST2700FC_HW_STRAP2, errp)) {
return;
}
aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART12, serial_hd(0));
- if (!qdev_realize(DEVICE(&s->ca35), NULL, &error_abort)) {
+ if (!qdev_realize(DEVICE(&s->ca35), NULL, errp)) {
return;
}
@@ -125,6 +126,8 @@ static void ast2700fc_ssp_init(MachineState *machine)
{
AspeedSoCState *soc;
Ast2700FCState *s = AST2700A1FC(machine);
+ Error **errp = NULL;
+
s->ssp_sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
clock_set_hz(s->ssp_sysclk, 200000000ULL);
@@ -134,13 +137,13 @@ static void ast2700fc_ssp_init(MachineState *machine)
qdev_connect_clock_in(DEVICE(&s->ssp), "sysclk", s->ssp_sysclk);
if (!object_property_set_link(OBJECT(&s->ssp), "memory",
- OBJECT(&s->ssp_memory), &error_abort)) {
+ OBJECT(&s->ssp_memory), errp)) {
return;
}
soc = ASPEED_SOC(&s->ssp);
aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART4, serial_hd(1));
- if (!qdev_realize(DEVICE(&s->ssp), NULL, &error_abort)) {
+ if (!qdev_realize(DEVICE(&s->ssp), NULL, errp)) {
return;
}
}
@@ -149,6 +152,8 @@ static void ast2700fc_tsp_init(MachineState *machine)
{
AspeedSoCState *soc;
Ast2700FCState *s = AST2700A1FC(machine);
+ Error **errp = NULL;
+
s->tsp_sysclk = clock_new(OBJECT(s), "TSP_SYSCLK");
clock_set_hz(s->tsp_sysclk, 200000000ULL);
@@ -158,13 +163,13 @@ static void ast2700fc_tsp_init(MachineState *machine)
qdev_connect_clock_in(DEVICE(&s->tsp), "sysclk", s->tsp_sysclk);
if (!object_property_set_link(OBJECT(&s->tsp), "memory",
- OBJECT(&s->tsp_memory), &error_abort)) {
+ OBJECT(&s->tsp_memory), errp)) {
return;
}
soc = ASPEED_SOC(&s->tsp);
aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART7, serial_hd(2));
- if (!qdev_realize(DEVICE(&s->tsp), NULL, &error_abort)) {
+ if (!qdev_realize(DEVICE(&s->tsp), NULL, errp)) {
return;
}
}
--
2.43.0On 9/24/25 07:55, Jamin Lin wrote:
> This patch introduces a local Error **errp = NULL and
> replaces error_abort with errp in these paths.
>
> This makes error handling more consistent with QEMU guidelines and avoids
> using error_abort in cases where failure should not be treated as a
> programming error.
>
> Discussion:
> object_property_set_link() can return false only when it fails, and it
> sets an error when it fails. Since passing &error_abort causes an abort,
> the function never returns false, and the return statement is effectively
> dead code. If failure is considered a programming error, using
> &error_abort is correct. However, if failure is not a programming error,
> passing &error_abort is wrong, and errp should be used instead. This
> patch follows the latter approach by replacing error_abort with errp.
> https://patchwork.kernel.org/project/qemu-devel/patch/20250717034054.1903991-3-jamin_lin@aspeedtech.com/#26540626
>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
> hw/arm/aspeed_ast27x0-fc.c | 27 ++++++++++++++++-----------
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> index 7087be4288..b15cb94c39 100644
> --- a/hw/arm/aspeed_ast27x0-fc.c
> +++ b/hw/arm/aspeed_ast27x0-fc.c
> @@ -61,6 +61,7 @@ static void ast2700fc_ca35_init(MachineState *machine)
ast2700fc_ca35_init(), and others below, should to take an 'Error **errp'
parameter and return a bool, which should be false in case of error. The
caller, ast2700fc_init() would then check the returned value and possibly
report the error.
A good reading on the error topic is in file "include/qapi/error.h".
> Ast2700FCState *s = AST2700A1FC(machine);
> AspeedSoCState *soc;
> AspeedSoCClass *sc;
> + Error **errp = NULL;
>
> object_initialize_child(OBJECT(s), "ca35", &s->ca35, "ast2700-a1");
> soc = ASPEED_SOC(&s->ca35);
> @@ -71,20 +72,20 @@ static void ast2700fc_ca35_init(MachineState *machine)
> memory_region_add_subregion(get_system_memory(), 0, &s->ca35_memory);
>
> if (!memory_region_init_ram(&s->ca35_dram, OBJECT(&s->ca35), "ca35-dram",
> - AST2700FC_BMC_RAM_SIZE, &error_abort)) {
> + AST2700FC_BMC_RAM_SIZE, errp)) {
> return;
> }
> if (!object_property_set_link(OBJECT(&s->ca35), "memory",
> OBJECT(&s->ca35_memory),
> - &error_abort)) {
> + errp)) {
> return;
> };
> if (!object_property_set_link(OBJECT(&s->ca35), "dram",
> - OBJECT(&s->ca35_dram), &error_abort)) {
> + OBJECT(&s->ca35_dram), errp)) {
> return;
> }
> if (!object_property_set_int(OBJECT(&s->ca35), "ram-size",
> - AST2700FC_BMC_RAM_SIZE, &error_abort)) {
> + AST2700FC_BMC_RAM_SIZE, errp)) {
object_property_set_int() is considered as a routine which shouldn't fail.
So the common practice in models is to pass &error_abort and ignore the
returned value.
Thanks,
C.
> return;
> }
>
> @@ -95,15 +96,15 @@ static void ast2700fc_ca35_init(MachineState *machine)
> }
> }
> if (!object_property_set_int(OBJECT(&s->ca35), "hw-strap1",
> - AST2700FC_HW_STRAP1, &error_abort)) {
> + AST2700FC_HW_STRAP1, errp)) {
> return;
> }
> if (!object_property_set_int(OBJECT(&s->ca35), "hw-strap2",
> - AST2700FC_HW_STRAP2, &error_abort)) {
> + AST2700FC_HW_STRAP2, errp)) {
> return;
> }
> aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART12, serial_hd(0));
> - if (!qdev_realize(DEVICE(&s->ca35), NULL, &error_abort)) {
> + if (!qdev_realize(DEVICE(&s->ca35), NULL, errp)) {
> return;
> }
>
> @@ -125,6 +126,8 @@ static void ast2700fc_ssp_init(MachineState *machine)
> {
> AspeedSoCState *soc;
> Ast2700FCState *s = AST2700A1FC(machine);
> + Error **errp = NULL;
> +
> s->ssp_sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
> clock_set_hz(s->ssp_sysclk, 200000000ULL);
>
> @@ -134,13 +137,13 @@ static void ast2700fc_ssp_init(MachineState *machine)
>
> qdev_connect_clock_in(DEVICE(&s->ssp), "sysclk", s->ssp_sysclk);
> if (!object_property_set_link(OBJECT(&s->ssp), "memory",
> - OBJECT(&s->ssp_memory), &error_abort)) {
> + OBJECT(&s->ssp_memory), errp)) {
> return;
> }
>
> soc = ASPEED_SOC(&s->ssp);
> aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART4, serial_hd(1));
> - if (!qdev_realize(DEVICE(&s->ssp), NULL, &error_abort)) {
> + if (!qdev_realize(DEVICE(&s->ssp), NULL, errp)) {
> return;
> }
> }
> @@ -149,6 +152,8 @@ static void ast2700fc_tsp_init(MachineState *machine)
> {
> AspeedSoCState *soc;
> Ast2700FCState *s = AST2700A1FC(machine);
> + Error **errp = NULL;
> +
> s->tsp_sysclk = clock_new(OBJECT(s), "TSP_SYSCLK");
> clock_set_hz(s->tsp_sysclk, 200000000ULL);
>
> @@ -158,13 +163,13 @@ static void ast2700fc_tsp_init(MachineState *machine)
>
> qdev_connect_clock_in(DEVICE(&s->tsp), "sysclk", s->tsp_sysclk);
> if (!object_property_set_link(OBJECT(&s->tsp), "memory",
> - OBJECT(&s->tsp_memory), &error_abort)) {
> + OBJECT(&s->tsp_memory), errp)) {
> return;
> }
>
> soc = ASPEED_SOC(&s->tsp);
> aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART7, serial_hd(2));
> - if (!qdev_realize(DEVICE(&s->tsp), NULL, &error_abort)) {
> + if (!qdev_realize(DEVICE(&s->tsp), NULL, errp)) {
> return;
> }
> }
Hi Cédric
> Subject: Re: [PATCH v2 5/7] hw/arm/aspeed_ast27x0-fc: Replace error_abort
> with local errp
>
> On 9/24/25 07:55, Jamin Lin wrote:
> > This patch introduces a local Error **errp = NULL and replaces
> > error_abort with errp in these paths.
> >
> > This makes error handling more consistent with QEMU guidelines and
> > avoids using error_abort in cases where failure should not be treated
> > as a programming error.
> >
> > Discussion:
> > object_property_set_link() can return false only when it fails, and it
> > sets an error when it fails. Since passing &error_abort causes an
> > abort, the function never returns false, and the return statement is
> > effectively dead code. If failure is considered a programming error,
> > using &error_abort is correct. However, if failure is not a
> > programming error, passing &error_abort is wrong, and errp should be
> > used instead. This patch follows the latter approach by replacing error_abort
> with errp.
> > https://patchwork.kernel.org/project/qemu-devel/patch/20250717034054.1
> > 903991-3-jamin_lin@aspeedtech.com/#26540626
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> > hw/arm/aspeed_ast27x0-fc.c | 27 ++++++++++++++++-----------
> > 1 file changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> > index 7087be4288..b15cb94c39 100644
> > --- a/hw/arm/aspeed_ast27x0-fc.c
> > +++ b/hw/arm/aspeed_ast27x0-fc.c
> > @@ -61,6 +61,7 @@ static void ast2700fc_ca35_init(MachineState
> > *machine)
>
>
> ast2700fc_ca35_init(), and others below, should to take an 'Error **errp'
> parameter and return a bool, which should be false in case of error. The caller,
> ast2700fc_init() would then check the returned value and possibly report the
> error.
Thanks for your review and suggestion.
Will fix it.
>
> A good reading on the error topic is in file "include/qapi/error.h".
>
> > Ast2700FCState *s = AST2700A1FC(machine);
> > AspeedSoCState *soc;
> > AspeedSoCClass *sc;
> > + Error **errp = NULL;
> >
> > object_initialize_child(OBJECT(s), "ca35", &s->ca35, "ast2700-a1");
> > soc = ASPEED_SOC(&s->ca35);
> > @@ -71,20 +72,20 @@ static void ast2700fc_ca35_init(MachineState
> *machine)
> > memory_region_add_subregion(get_system_memory(), 0,
> > &s->ca35_memory);
> >
> > if (!memory_region_init_ram(&s->ca35_dram, OBJECT(&s->ca35),
> "ca35-dram",
> > - AST2700FC_BMC_RAM_SIZE,
> &error_abort)) {
> > + AST2700FC_BMC_RAM_SIZE, errp))
> {
> > return;
> > }
> > if (!object_property_set_link(OBJECT(&s->ca35), "memory",
> > OBJECT(&s->ca35_memory),
> > - &error_abort)) {
> > + errp)) {
> > return;
> > };
> > if (!object_property_set_link(OBJECT(&s->ca35), "dram",
> > - OBJECT(&s->ca35_dram),
> &error_abort)) {
> > + OBJECT(&s->ca35_dram), errp)) {
> > return;
> > }
> > if (!object_property_set_int(OBJECT(&s->ca35), "ram-size",
> > - AST2700FC_BMC_RAM_SIZE,
> &error_abort)) {
> > + AST2700FC_BMC_RAM_SIZE, errp))
> {
>
> object_property_set_int() is considered as a routine which shouldn't fail.
> So the common practice in models is to pass &error_abort and ignore the
> returned value.
>
Will fix it.
Thanks-Jamin
>
> Thanks,
>
> C.
>
>
>
> > return;
> > }
> >
> > @@ -95,15 +96,15 @@ static void ast2700fc_ca35_init(MachineState
> *machine)
> > }
> > }
> > if (!object_property_set_int(OBJECT(&s->ca35), "hw-strap1",
> > - AST2700FC_HW_STRAP1,
> &error_abort)) {
> > + AST2700FC_HW_STRAP1, errp)) {
> > return;
> > }
> > if (!object_property_set_int(OBJECT(&s->ca35), "hw-strap2",
> > - AST2700FC_HW_STRAP2,
> &error_abort)) {
> > + AST2700FC_HW_STRAP2, errp)) {
> > return;
> > }
> > aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART12, serial_hd(0));
> > - if (!qdev_realize(DEVICE(&s->ca35), NULL, &error_abort)) {
> > + if (!qdev_realize(DEVICE(&s->ca35), NULL, errp)) {
> > return;
> > }
> >
> > @@ -125,6 +126,8 @@ static void ast2700fc_ssp_init(MachineState
> *machine)
> > {
> > AspeedSoCState *soc;
> > Ast2700FCState *s = AST2700A1FC(machine);
> > + Error **errp = NULL;
> > +
> > s->ssp_sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
> > clock_set_hz(s->ssp_sysclk, 200000000ULL);
> >
> > @@ -134,13 +137,13 @@ static void ast2700fc_ssp_init(MachineState
> > *machine)
> >
> > qdev_connect_clock_in(DEVICE(&s->ssp), "sysclk", s->ssp_sysclk);
> > if (!object_property_set_link(OBJECT(&s->ssp), "memory",
> > - OBJECT(&s->ssp_memory),
> &error_abort)) {
> > + OBJECT(&s->ssp_memory), errp))
> {
> > return;
> > }
> >
> > soc = ASPEED_SOC(&s->ssp);
> > aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART4, serial_hd(1));
> > - if (!qdev_realize(DEVICE(&s->ssp), NULL, &error_abort)) {
> > + if (!qdev_realize(DEVICE(&s->ssp), NULL, errp)) {
> > return;
> > }
> > }
> > @@ -149,6 +152,8 @@ static void ast2700fc_tsp_init(MachineState
> *machine)
> > {
> > AspeedSoCState *soc;
> > Ast2700FCState *s = AST2700A1FC(machine);
> > + Error **errp = NULL;
> > +
> > s->tsp_sysclk = clock_new(OBJECT(s), "TSP_SYSCLK");
> > clock_set_hz(s->tsp_sysclk, 200000000ULL);
> >
> > @@ -158,13 +163,13 @@ static void ast2700fc_tsp_init(MachineState
> > *machine)
> >
> > qdev_connect_clock_in(DEVICE(&s->tsp), "sysclk", s->tsp_sysclk);
> > if (!object_property_set_link(OBJECT(&s->tsp), "memory",
> > - OBJECT(&s->tsp_memory),
> &error_abort)) {
> > + OBJECT(&s->tsp_memory), errp))
> {
> > return;
> > }
> >
> > soc = ASPEED_SOC(&s->tsp);
> > aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART7, serial_hd(2));
> > - if (!qdev_realize(DEVICE(&s->tsp), NULL, &error_abort)) {
> > + if (!qdev_realize(DEVICE(&s->tsp), NULL, errp)) {
> > return;
> > }
> > }
© 2016 - 2025 Red Hat, Inc.