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
> > [email protected]/#26540626
> >
> > Signed-off-by: Jamin Lin <[email protected]>
> > ---
> > 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;
> > }
> > }
