| Message ID | 20160803145541.15355-4-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
Copying the QGA maintainer. marcandre.lureau@redhat.com writes: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Free the list, not just the elements. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > 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 > > +/* > + * A GFunc function helper freeing the first argument (not part of glib) 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. 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? 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 :) > + */ > +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); Modern GLib code doesn't need silly wrappers: g_list_free_full(config->blacklist, g_free); Unfortunately, this requires 2.28, and we're stull stuck at 2.22. Please explain this in the commit message. 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. > g_free(config); > } > > @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig *config) > return EXIT_SUCCESS; > } > > -static void free_blacklist_entry(gpointer entry, gpointer unused) > -{ > - g_free(entry); > -} > - > int main(int argc, char **argv) > { > int ret = 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); If you at least explain why not g_list_free_full() in the commit message, you can add Reviewed-by: Markus Armbruster <armbru@redhat.com> But I'd like to encourage you to explore providing a replacement for g_list_free_full().
Hi ----- Original Message ----- > Copying the QGA maintainer. > > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Free the list, not just the elements. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > 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 > > > > +/* > > + * A GFunc function helper freeing the first argument (not part of glib) > > 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. > > 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? > > 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 :) > > > + */ > > +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); > > Modern GLib code doesn't need silly wrappers: > > g_list_free_full(config->blacklist, g_free); > > Unfortunately, this requires 2.28, and we're stull stuck at 2.22. > Please explain this in the commit message. > > 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 > > > g_free(config); > > } > > > > @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig *config) > > return EXIT_SUCCESS; > > } > > > > -static void free_blacklist_entry(gpointer entry, gpointer unused) > > -{ > > - g_free(entry); > > -} > > - > > int main(int argc, char **argv) > > { > > int ret = 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); > > If you at least explain why not g_list_free_full() in the commit > message, you can add > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > 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
Marc-André Lureau <mlureau@redhat.com> writes: > Hi > > ----- Original Message ----- >> Copying the QGA maintainer. >> >> marcandre.lureau@redhat.com writes: >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> >> > >> > Free the list, not just the elements. >> > >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> > --- >> > 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 >> > >> > +/* >> > + * A GFunc function helper freeing the first argument (not part of glib) >> >> 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. >> >> 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? >> >> 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 :) >> >> > + */ >> > +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); >> >> Modern GLib code doesn't need silly wrappers: >> >> g_list_free_full(config->blacklist, g_free); >> >> Unfortunately, this requires 2.28, and we're stull stuck at 2.22. >> Please explain this in the commit message. >> >> 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 > >> >> > g_free(config); >> > } >> > >> > @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig *config) >> > return EXIT_SUCCESS; >> > } >> > >> > -static void free_blacklist_entry(gpointer entry, gpointer unused) >> > -{ >> > - g_free(entry); >> > -} >> > - >> > int main(int argc, char **argv) >> > { >> > int ret = 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); >> >> If you at least explain why not g_list_free_full() in the commit >> message, you can add >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> >> 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 Pointlessly dirty: g_list_foreach(list, (GFunc)g_free, NULL) Trade dirty for cumbersome: void free_wrapper(gpointer data, gpointer unused) { g_free(data) } g_list_foreach(list, free_wrapper, NULL); 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: for (node = list; node; node = next) { next = node->next; g_free(node); } Simple, stupid, and not dirty. Quote: "Note that it is considered perfectly acceptable to access list->next directly."
Hi ----- Original Message ----- > Marc-André Lureau <mlureau@redhat.com> writes: > > > Hi > > > > ----- Original Message ----- > >> Copying the QGA maintainer. > >> > >> marcandre.lureau@redhat.com writes: > >> > >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > > >> > Free the list, not just the elements. > >> > > >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > --- > >> > 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 > >> > > >> > +/* > >> > + * A GFunc function helper freeing the first argument (not part of > >> > glib) > >> > >> 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. > >> > >> 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? > >> > >> 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 :) > >> > >> > + */ > >> > +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); > >> > >> Modern GLib code doesn't need silly wrappers: > >> > >> g_list_free_full(config->blacklist, g_free); > >> > >> Unfortunately, this requires 2.28, and we're stull stuck at 2.22. > >> Please explain this in the commit message. > >> > >> 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 > > > >> > >> > g_free(config); > >> > } > >> > > >> > @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig > >> > *config) > >> > return EXIT_SUCCESS; > >> > } > >> > > >> > -static void free_blacklist_entry(gpointer entry, gpointer unused) > >> > -{ > >> > - g_free(entry); > >> > -} > >> > - > >> > int main(int argc, char **argv) > >> > { > >> > int ret = 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); > >> > >> If you at least explain why not g_list_free_full() in the commit > >> message, you can add > >> Reviewed-by: Markus Armbruster <armbru@redhat.com> > >> > >> 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 > > Pointlessly dirty: > > g_list_foreach(list, (GFunc)g_free, NULL) > > Trade dirty for cumbersome: > > void free_wrapper(gpointer data, gpointer unused) > { > g_free(data) > } > > g_list_foreach(list, free_wrapper, NULL); > > 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: > > for (node = list; node; node = next) { > next = node->next; > g_free(node); > } > > 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 the struct.
Marc-André Lureau <mlureau@redhat.com> writes: > Hi > > ----- Original Message ----- >> Marc-André Lureau <mlureau@redhat.com> writes: >> >> > Hi >> > >> > ----- Original Message ----- >> >> Copying the QGA maintainer. >> >> >> >> marcandre.lureau@redhat.com writes: >> >> >> >> > From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> > >> >> > Free the list, not just the elements. >> >> > >> >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> > --- >> >> > 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 >> >> > >> >> > +/* >> >> > + * A GFunc function helper freeing the first argument (not part of >> >> > glib) >> >> >> >> 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. >> >> >> >> 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? >> >> >> >> 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 :) >> >> >> >> > + */ >> >> > +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); >> >> >> >> Modern GLib code doesn't need silly wrappers: >> >> >> >> g_list_free_full(config->blacklist, g_free); >> >> >> >> Unfortunately, this requires 2.28, and we're stull stuck at 2.22. >> >> Please explain this in the commit message. >> >> >> >> 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 >> > >> >> >> >> > g_free(config); >> >> > } >> >> > >> >> > @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig >> >> > *config) >> >> > return EXIT_SUCCESS; >> >> > } >> >> > >> >> > -static void free_blacklist_entry(gpointer entry, gpointer unused) >> >> > -{ >> >> > - g_free(entry); >> >> > -} >> >> > - >> >> > int main(int argc, char **argv) >> >> > { >> >> > int ret = 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); >> >> >> >> If you at least explain why not g_list_free_full() in the commit >> >> message, you can add >> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> >> >> >> 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 >> >> Pointlessly dirty: >> >> g_list_foreach(list, (GFunc)g_free, NULL) >> >> Trade dirty for cumbersome: >> >> void free_wrapper(gpointer data, gpointer unused) >> { >> g_free(data) >> } >> >> g_list_foreach(list, free_wrapper, NULL); >> >> 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: >> >> for (node = list; node; node = next) { >> next = node->next; >> g_free(node); >> } >> >> 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 ;) Lisp is ---> that way. At least the stupid loop is hidden away in glib-compat.h. The actual uses become neater, e.g. g_list_free_full(config->blacklist, g_free); instead of g_list_foreach(config->blacklist, qemu_g_func_free, NULL); g_list_free(config->blacklist); I think that's worth a bit of extra work in glib-compat.h. >> 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 the struct.
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 +/* + * A GFunc function helper freeing the first argument (not part of glib) + */ +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); g_free(config); } @@ -1310,11 +1312,6 @@ static int run_agent(GAState *s, GAConfig *config) return EXIT_SUCCESS; } -static void free_blacklist_entry(gpointer entry, gpointer unused) -{ - g_free(entry); -} - int main(int argc, char **argv) { int ret = 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);