From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34355) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVIzO-0005Zb-A7 for qemu-devel@nongnu.org; Thu, 04 Aug 2016 09:47:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bVIzL-0000tb-5q for qemu-devel@nongnu.org; Thu, 04 Aug 2016 09:47:10 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:49470) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVIzK-0000tV-Tc for qemu-devel@nongnu.org; Thu, 04 Aug 2016 09:47:07 -0400 Date: Thu, 4 Aug 2016 09:47:03 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <900945795.1082226.1470318423751.JavaMail.zimbra@redhat.com> In-Reply-To: <87fuqkfyz6.fsf@dusky.pond.sub.org> References: <20160803145541.15355-1-marcandre.lureau@redhat.com> <20160803145541.15355-4-marcandre.lureau@redhat.com> <87fuqkfyz6.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 03/36] qga: free the whole blacklist List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre lureau , qemu-devel@nongnu.org, pbonzini@redhat.com, Michael Roth Hi ----- Original Message ----- > Copying the QGA maintainer. >=20 > marcandre.lureau@redhat.com writes: >=20 > > From: Marc-Andr=C3=A9 Lureau > > > > Free the list, not just the elements. > > > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > --- > > include/glib-compat.h | 9 +++++++++ > > qga/main.c | 8 ++------ > > 2 files changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/include/glib-compat.h b/include/glib-compat.h > > index 01aa7b3..6d643b2 100644 > > --- a/include/glib-compat.h > > +++ b/include/glib-compat.h > > @@ -260,4 +260,13 @@ static inline void g_hash_table_add(GHashTable > > *hash_table, gpointer key) > > } while (0) > > #endif > > =20 > > +/* > > + * A GFunc function helper freeing the first argument (not part of gli= b) >=20 > Well, it's not part of GLib, because non-obsolete GLib has no use for > it! You'd simply pass g_free directly to a _free_full() function > instead of passing a silly wrapper to a _foreach() function. >=20 > The commit does a bit more than just plug a leak, it also provides a new > helper for general use. Mention in the commit message? >=20 > I wonder how many more of these silly g_free() wrappers we have. A > quick grep reports hits in string-input-visitor.c and > qobject/json-streamer.c. If you add one to a header, you get to hunt > them down :) >=20 > > + */ > > +static inline void qemu_g_func_free(gpointer data, > > + gpointer user_data) > > +{ > > + g_free(data); > > +} > > + > > #endif > > diff --git a/qga/main.c b/qga/main.c > > index 4c3b2c7..868508b 100644 > > --- a/qga/main.c > > +++ b/qga/main.c > > @@ -1175,6 +1175,8 @@ static void config_free(GAConfig *config) > > #ifdef CONFIG_FSFREEZE > > g_free(config->fsfreeze_hook); > > #endif > > + g_list_foreach(config->blacklist, qemu_g_func_free, NULL); > > + g_list_free(config->blacklist); >=20 > Modern GLib code doesn't need silly wrappers: >=20 > g_list_free_full(config->blacklist, g_free); >=20 > Unfortunately, this requires 2.28, and we're stull stuck at 2.22. > Please explain this in the commit message. >=20 > Even better, provide a replacement in glib-compat.h #if > !GLIB_CHECK_VERSION(2, 28, 0). Will facilitate ditching the work-around > when we upgrade to 2.28. ok >=20 > > g_free(config); > > } > > =20 > > @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig *confi= g) > > return EXIT_SUCCESS; > > } > > =20 > > -static void free_blacklist_entry(gpointer entry, gpointer unused) > > -{ > > - g_free(entry); > > -} > > - > > int main(int argc, char **argv) > > { > > int ret =3D EXIT_SUCCESS; > > @@ -1379,7 +1376,6 @@ end: > > if (s->channel) { > > ga_channel_free(s->channel); > > } > > - g_list_foreach(config->blacklist, free_blacklist_entry, NULL); > > g_free(s->pstate_filepath); > > g_free(s->state_filepath_isfrozen); >=20 > If you at least explain why not g_list_free_full() in the commit > message, you can add > Reviewed-by: Markus Armbruster >=20 > But I'd like to encourage you to explore providing a replacement for > g_list_free_full(). Such replacement would make use of a (GFunc) cast, glib implementation is c= alling g_list_foreach (list, (GFunc) free_func, NULL), but Eric didn't want= to have such cast in qemu code. He suggested to have the static inline hel= per in https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06561.html