Re: [Qemu-devel] [PATCH for-2.7 v3 17/36] machine: use class base init generated name

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Thu, Aug 4, 2016 at 5:58 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@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.lureau@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 <armbru@redhat.com>