Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> Hi
>
> On Thu, Aug 4, 2016 at 5:58 PM Markus Armbruster <arm...@redhat.com> wrote:
>
>> marcandre.lur...@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
>> >
>> > Remove machine class name initialization from DEFINE_PC_MACHINE, rely on
>> > class base init name generation instead. Get rid of some leaks that way.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> > ---
>> > hw/core/machine.c | 1 +
>> > include/hw/boards.h | 2 +-
>> > include/hw/i386/pc.h | 1 -
>> > 3 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/hw/core/machine.c b/hw/core/machine.c
>> > index e5a456f..00fbe3e 100644
>> > --- a/hw/core/machine.c
>> > +++ b/hw/core/machine.c
>> > @@ -561,6 +561,7 @@ static void machine_class_finalize(ObjectClass *klass,
>> > void *data)
>> > if (mc->compat_props) {
>> > g_array_free(mc->compat_props, true);
>> > }
>> > + g_free(mc->name);
>> > }
>> >
>> > void machine_register_compat_props(MachineState *machine)
>> > diff --git a/include/hw/boards.h b/include/hw/boards.h
>> > index 3e69eca..e46a744 100644
>> > --- a/include/hw/boards.h
>> > +++ b/include/hw/boards.h
>> > @@ -93,7 +93,7 @@ struct MachineClass {
>> > /*< public >*/
>> >
>> > const char *family; /* NULL iff @name identifies a standalone
>> > machtype */
>> > - const char *name;
>> > + char *name;
>> > const char *alias;
>> > const char *desc;
>> >
>> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> > index eb1d414..afd025a 100644
>> > --- a/include/hw/i386/pc.h
>> > +++ b/include/hw/i386/pc.h
>> > @@ -903,7 +903,6 @@ bool e820_get_entry(int, uint32_t, uint64_t *,
>> > uint64_t *);
>> > { \
>> > MachineClass *mc = MACHINE_CLASS(oc); \
>> > optsfn(mc); \
>> > - mc->name = namestr; \
>> > mc->init = initfn; \
>> > } \
>> > static const TypeInfo pc_machine_type_##suffix = { \
>>
>> I guess there are is at least one assignment to mc->name not visible in
>> this patch that assigns an allocated string, which leaks before the
>> patch. The commit message seems to provide a clue "class base init name
>> generation". I could probably find it with some effort, but patches
>> that take that much work to understand make me grumpy. Please provide
>> another clue :)
>>
>
> Sorry, thanks for reminding me to write better commit messages.
Good commit messages are hard.
> git grep 'mc->name ='
> hw/core/machine.c: mc->name = g_strndup(cname,
Aha: the concrete machine type's init function overwrites the strdup()ed
value set by machine_class_base_init(), leaking it. Your fix removes
the overwrites and adds a free.
As far as I can see, you got all such overwrites.
> Is that better:
>
> Remove machine class name initialization from DEFINE_PC_MACHINE, rely on
> name generation from machine_class_base_init() instead, and free the
> corresponding allocation in machine_class_finalize().
Works for me. Alternatively:
machine_class_base_init() member name is allocated by
machine_class_base_init(), but not freed by machine_class_finalize().
Simply freeing there doesn't work, because DEFINE_PC_MACHINE()
overwrites it with a literal string.
Fix DEFINE_PC_MACHINE() not to overwrite it, and add the missing
free to machine_class_finalize().
Use the one you like better, or mix them up to taste.
Reviewed-by: Markus Armbruster <arm...@redhat.com>
