From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57373) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVK7F-0005Mt-Bs for qemu-devel@nongnu.org; Thu, 04 Aug 2016 10:59:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bVK7C-0003Vr-2K for qemu-devel@nongnu.org; Thu, 04 Aug 2016 10:59:21 -0400 Received: from mx6-phx2.redhat.com ([209.132.183.39]:48725) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVK7B-0003VK-Q5 for qemu-devel@nongnu.org; Thu, 04 Aug 2016 10:59:17 -0400 Date: Thu, 4 Aug 2016 10:59:14 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1223869164.1103430.1470322754832.JavaMail.zimbra@redhat.com> In-Reply-To: <87r3a4d2ea.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> <900945795.1082226.1470318423751.JavaMail.zimbra@redhat.com> <87r3a4d2ea.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: Michael Roth , marcandre lureau , qemu-devel@nongnu.org, pbonzini@redhat.com Hi ----- Original Message ----- > Marc-Andr=C3=A9 Lureau writes: >=20 > > 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 > >> > glib) > >>=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 n= ew > >> 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-arou= nd > >> when we upgrade to 2.28. > > > > ok > > > >>=20 > >> > g_free(config); > >> > } > >> > =20 > >> > @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig > >> > *config) > >> > 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 > > calling 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 helper in > > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06561.html >=20 > Pointlessly dirty: >=20 > g_list_foreach(list, (GFunc)g_free, NULL) >=20 > Trade dirty for cumbersome: >=20 > void free_wrapper(gpointer data, gpointer unused) > { > g_free(data) > } >=20 > g_list_foreach(list, free_wrapper, NULL); >=20 > But this is C. In C, simple things are best done the simple way. Even > when that means we don't get to show off how amazingly well we've been > educated on higher order functions: >=20 > for (node =3D list; node; node =3D next) { > next =3D node->next; > g_free(node); > } >=20 > Simple, stupid, and not dirty. If only we were paid to write more lines of code ;) Anyway, that's fine by = me (it's work after all, I'll write elegant code for fun time ;) > Quote: "Note that it is considered perfectly acceptable to access > list->next directly." Funny that quote is only in GList, but GSList also documents and exposes th= e struct.