diff mbox

[for-2.7,v3,17/36] machine: use class base init generated name

Message ID 20160803145541.15355-18-marcandre.lureau@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marc-André Lureau Aug. 3, 2016, 2:55 p.m. UTC 
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(-)

Comments

Markus Armbruster Aug. 4, 2016, 1:56 p.m. UTC | #1 
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 :)
Marc-André Lureau Aug. 4, 2016, 2:31 p.m. UTC | #2 
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.

git grep 'mc->name ='
hw/core/machine.c:        mc->name = g_strndup(cname,

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().
Markus Armbruster Aug. 4, 2016, 3:26 p.m. UTC | #3 
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>
diff mbox

Patch

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 = { \