Re: [PATCH v2 01/23] hw/ppc/e500: Do not leak struct boot_info

[Bernhard Beschow]

Am 6. Oktober 2024 16:56:52 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 5 Oct 2024, Bernhard Beschow wrote:
>> The struct is allocated once with g_new0() but never free()'d. Fix the 
>> leakage
>> by adding an attribute to struct PPCE500MachineState which avoids the
>> allocation.
>> 
>> While at it remove the obsolete /*< private >*/ markers.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/ppc/e500.h |  9 +++++++--
>> hw/ppc/e500.c | 17 ++++-------------
>> 2 files changed, 11 insertions(+), 15 deletions(-)
>> 
>> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
>> index 8c09ef92e4..5654bb7907 100644
>> --- a/hw/ppc/e500.h
>> +++ b/hw/ppc/e500.h
>> @@ -5,18 +5,23 @@
>> #include "hw/platform-bus.h"
>> #include "qom/object.h"
>> 
>> +typedef struct boot_info {
>> +    uint32_t dt_base;
>> +    uint32_t dt_size;
>> +    uint32_t entry;
>> +} boot_info;
>> +
>
>You either don't need a typedef or don't need a struct name. Since coding 
>style says typedefs should be camel case but other machines don't use a 
>typedef it's probably simplest to drop the typedef and define the machine 
>state field as struct boot_info below for consistency with other similar 
>structs. Otherwise you'd have to come up with some camel case type name here 
>but that would be less consistent with other machines. So I'd go without the 
>typedef.

Indeed, not sure why I added the typedef. I'll omit the typedef then.

This code hasn't changed since v1, so it would have been nice to have caught 
that in the first round to avoid another spin.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> struct PPCE500MachineState {
>> -    /*< private >*/
>>     MachineState parent_obj;
>> 
>>     /* points to instance of TYPE_PLATFORM_BUS_DEVICE if
>>      * board supports dynamic sysbus devices
>>      */
>>     PlatformBusDevice *pbus_dev;
>> +    boot_info boot_info;
>> };
>> 
>> struct PPCE500MachineClass {
>> -    /*< private >*/
>>     MachineClass parent_class;
>> 
>>     /* required -- must at least add toplevel board compatible */
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 3bd12b54ab..75b051009f 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -80,13 +80,6 @@
>> 
>> #define PLATFORM_CLK_FREQ_HZ       (400 * 1000 * 1000)
>> 
>> -struct boot_info
>> -{
>> -    uint32_t dt_base;
>> -    uint32_t dt_size;
>> -    uint32_t entry;
>> -};
>> -
>> static uint32_t *pci_map_create(void *fdt, uint32_t mpic, int first_slot,
>>                                 int nr_slots, int *len)
>> {
>> @@ -919,7 +912,6 @@ void ppce500_init(MachineState *machine)
>>     bool kernel_as_payload;
>>     hwaddr bios_entry = 0;
>>     target_long payload_size;
>> -    struct boot_info *boot_info = NULL;
>>     int dt_size;
>>     int i;
>>     unsigned int smp_cpus = machine->smp.cpus;
>> @@ -974,9 +966,8 @@ void ppce500_init(MachineState *machine)
>>         /* Register reset handler */
>>         if (!i) {
>>             /* Primary CPU */
>> -            boot_info = g_new0(struct boot_info, 1);
>>             qemu_register_reset(ppce500_cpu_reset, cpu);
>> -            env->load_info = boot_info;
>> +            env->load_info = &pms->boot_info;
>>         } else {
>>             /* Secondary CPUs */
>>             qemu_register_reset(ppce500_cpu_reset_sec, cpu);
>> @@ -1274,9 +1265,9 @@ void ppce500_init(MachineState *machine)
>>     }
>>     assert(dt_size < DTB_MAX_SIZE);
>> 
>> -    boot_info->entry = bios_entry;
>> -    boot_info->dt_base = dt_base;
>> -    boot_info->dt_size = dt_size;
>> +    pms->boot_info.entry = bios_entry;
>> +    pms->boot_info.dt_base = dt_base;
>> +    pms->boot_info.dt_size = dt_size;
>> }
>> 
>> static void e500_ccsr_initfn(Object *obj)
>>