| Message ID | 20160728143808.13707-34-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/28/2016 08:38 AM, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Allows to specify a destroy function for the test data. "Allows to" is not idiomatic English. Alternatives that sound better are "Allows $who to specify" (most simply, "Allows one to"), or "Allows specifying" > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > tests/libqtest.c | 15 ++++++++++++++- > tests/libqtest.h | 7 ++++++- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/tests/libqtest.c b/tests/libqtest.c > index eb00f13..6ec56a6 100644 > --- a/tests/libqtest.c > +++ b/tests/libqtest.c > @@ -758,8 +758,21 @@ void qtest_add_func(const char *str, void (*fn)(void)) > g_free(path); > } > > +void qtest_add_data_func_full(const char *str, void *data, > + void (*fn)(const void *), > + GDestroyNotify data_free_func) > +{ > +#if GLIB_CHECK_VERSION(2, 34, 0) > + gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str); > + g_test_add_data_func_full(path, data, fn, data_free_func); > + g_free(path); > +#else > + qtest_add_data_func(str, data, fn); > +#endif The commit message doesn't mention that the code is dependent on glib versions, nor that you are still leaking the data (data_free_func remains uncalled) on older glib. If it is intentional (under the argument that "anyone running on older glib can't care too much about memory leaks encountered only by the testsuite, and the leaks don't affect main qemu"), then stating that in the commit message would let me feel more comfortable giving an R-b. Is there anything we can do even in older glib to unconditionally invoke the cleanup function in the right places? > + > void qtest_add_data_func(const char *str, const void *data, > - void (*fn)(const void *)) > + void (*fn)(const void *)) Why the spurious indentation change?
Hi On Fri, Jul 29, 2016 at 2:16 AM, Eric Blake <eblake@redhat.com> wrote: > On 07/28/2016 08:38 AM, marcandre.lureau@redhat.com wrote: >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> Allows to specify a destroy function for the test data. > > "Allows to" is not idiomatic English. Alternatives that sound better are > "Allows $who to specify" (most simply, "Allows one to"), or "Allows > specifying" > >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> tests/libqtest.c | 15 ++++++++++++++- >> tests/libqtest.h | 7 ++++++- >> 2 files changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/tests/libqtest.c b/tests/libqtest.c >> index eb00f13..6ec56a6 100644 >> --- a/tests/libqtest.c >> +++ b/tests/libqtest.c >> @@ -758,8 +758,21 @@ void qtest_add_func(const char *str, void (*fn)(void)) >> g_free(path); >> } >> >> +void qtest_add_data_func_full(const char *str, void *data, >> + void (*fn)(const void *), >> + GDestroyNotify data_free_func) >> +{ >> +#if GLIB_CHECK_VERSION(2, 34, 0) >> + gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str); >> + g_test_add_data_func_full(path, data, fn, data_free_func); >> + g_free(path); >> +#else >> + qtest_add_data_func(str, data, fn); >> +#endif > > The commit message doesn't mention that the code is dependent on glib > versions, nor that you are still leaking the data (data_free_func > remains uncalled) on older glib. If it is intentional (under the > argument that "anyone running on older glib can't care too much about > memory leaks encountered only by the testsuite, and the leaks don't > affect main qemu"), then stating that in the commit message would let me > feel more comfortable giving an R-b. ok > Is there anything we can do even in older glib to unconditionally invoke > the cleanup function in the right places? Yes, calling the undocumented g_test_add_vtable(), with some casts. Is that acceptable?
On 07/29/2016 02:48 AM, Marc-André Lureau wrote: >>> +#if GLIB_CHECK_VERSION(2, 34, 0) >>> + gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str); >>> + g_test_add_data_func_full(path, data, fn, data_free_func); >>> + g_free(path); >>> +#else >>> + qtest_add_data_func(str, data, fn); >>> +#endif >> >> The commit message doesn't mention that the code is dependent on glib >> versions, nor that you are still leaking the data (data_free_func >> remains uncalled) on older glib. If it is intentional (under the >> argument that "anyone running on older glib can't care too much about >> memory leaks encountered only by the testsuite, and the leaks don't >> affect main qemu"), then stating that in the commit message would let me >> feel more comfortable giving an R-b. > > ok > >> Is there anything we can do even in older glib to unconditionally invoke >> the cleanup function in the right places? > > Yes, calling the undocumented g_test_add_vtable(), with some casts. Is > that acceptable? Since the older versions aren't changing, then yes, I would view back-compat casts along with a note saying "remove this once we can require new-enough glib" as acceptable.
diff --git a/tests/libqtest.c b/tests/libqtest.c index eb00f13..6ec56a6 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -758,8 +758,21 @@ void qtest_add_func(const char *str, void (*fn)(void)) g_free(path); } +void qtest_add_data_func_full(const char *str, void *data, + void (*fn)(const void *), + GDestroyNotify data_free_func) +{ +#if GLIB_CHECK_VERSION(2, 34, 0) + gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str); + g_test_add_data_func_full(path, data, fn, data_free_func); + g_free(path); +#else + qtest_add_data_func(str, data, fn); +#endif +} + void qtest_add_data_func(const char *str, const void *data, - void (*fn)(const void *)) + void (*fn)(const void *)) { gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str); g_test_add_data_func(path, data, fn); diff --git a/tests/libqtest.h b/tests/libqtest.h index 37f37ad..e4c9c39 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -413,15 +413,20 @@ const char *qtest_get_arch(void); void qtest_add_func(const char *str, void (*fn)(void)); /** - * qtest_add_data_func: + * qtest_add_data_func_full: * @str: Test case path. * @data: Test case data * @fn: Test case function + * @data_free_func: GDestroyNotify for data * * Add a GTester testcase with the given name, data and function. * The path is prefixed with the architecture under test, as * returned by qtest_get_arch(). */ +void qtest_add_data_func_full(const char *str, void *data, + void (*fn)(const void *), + GDestroyNotify data_free_func); + void qtest_add_data_func(const char *str, const void *data, void (*fn)(const void *));