From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34578) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVKXX-0000Wt-9z for qemu-devel@nongnu.org; Thu, 04 Aug 2016 11:26:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bVKXS-0000yx-6Q for qemu-devel@nongnu.org; Thu, 04 Aug 2016 11:26:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38568) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVKXR-0000ym-Ue for qemu-devel@nongnu.org; Thu, 04 Aug 2016 11:26:26 -0400 From: Markus Armbruster References: <20160803145541.15355-1-marcandre.lureau@redhat.com> <20160803145541.15355-18-marcandre.lureau@redhat.com> <877fbwfxhy.fsf@dusky.pond.sub.org> Date: Thu, 04 Aug 2016 17:26:21 +0200 In-Reply-To: (=?utf-8?Q?=22Marc-Andr=C3=A9?= Lureau"'s message of "Thu, 04 Aug 2016 14:31:22 +0000") Message-ID: <87r3a4a736.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for-2.7 v3 17/36] machine: use class base init generated name List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: pbonzini@redhat.com, qemu-devel@nongnu.org Marc-Andr=C3=A9 Lureau writes: > Hi > > On Thu, Aug 4, 2016 at 5:58 PM Markus Armbruster wrot= e: > >> marcandre.lureau@redhat.com writes: >> >> > From: Marc-Andr=C3=A9 Lureau >> > >> > Remove machine class name initialization from DEFINE_PC_MACHINE, rely = on >> > class base init name generation instead. Get rid of some leaks that wa= y. >> > >> > Signed-off-by: Marc-Andr=C3=A9 Lureau >> > --- >> > 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 *kl= ass, 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 mac= htype */ >> > - 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 *, uin= t64_t *); >> > { \ >> > MachineClass *mc =3D MACHINE_CLASS(oc); \ >> > optsfn(mc); \ >> > - mc->name =3D namestr; \ >> > mc->init =3D initfn; \ >> > } \ >> > static const TypeInfo pc_machine_type_##suffix =3D { \ >> >> 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 =3D' > hw/core/machine.c: mc->name =3D 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