Re: [Qemu-devel] [PATCH for-2.7 v3 32/36] tests: add qtest_add_data_func_full

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <mlureau@redhat.com> writes:

> Hi
>
> ----- Original Message -----
>> marcandre.lureau@redhat.com writes:
>> 
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Allows one to specify a destroy function for the test data.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  tests/libqtest.c | 15 +++++++++++++++
>> >  tests/libqtest.h |  7 ++++++-
>> >  2 files changed, 21 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/tests/libqtest.c b/tests/libqtest.c
>> > index eb00f13..9e2d0cd 100644
>> > --- a/tests/libqtest.c
>> > +++ b/tests/libqtest.c
>> > @@ -758,6 +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)
>> > +{
>> > +    gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
>> > +#if GLIB_CHECK_VERSION(2, 34, 0)
>> > +    g_test_add_data_func_full(path, data, fn, data_free_func);
>> > +#else
>> > +    /* back-compat casts, remove this once we can require new-enough glib
>> > */
>> > +    g_test_add_vtable(path, 0, data, NULL,
>> > +                      (GTestFixtureFunc) fn, (GTestFixtureFunc)
>> > data_free_func);
>> 
>> Umm, are these function casts kosher?
>> 
>> I can't find documentation for g_test_add_vtable().  Got a pointer for
>> me?  Sure GLib 2.22 provides it?
>
> Yes, https://git.gnome.org/browse/glib/tree/glib/gtestutils.h?h=2.22.0#n214
>
> See also: https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg00073.html
>
>
>> 
>> > +#endif
>> > +    g_free(path);
>> > +}
>> > +
>> >  void qtest_add_data_func(const char *str, const void *data,
>> >                           void (*fn)(const void *))
>> >  {
>> > 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().

Recommend to mention that @data is passed to @data_free_func() on test
completion.

>> >   */
>> > +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 *));
>> 
>> Please keep qtest_add_data_func() documented.
>
> Ok (I thought it was quite enough based on the _full description)

You could try something like

    /**
     * qtest_add_data_func and 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,

GTK-Doc extraction wouldn't cope with it, but we're not using it[*].

Or something like

    /**
     * qtest_add_data_func:
     * @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);

    /* Like qtest_add_data_func_full() with a null last argument */
    void qtest_add_data_func(const char *str, const void *data,

Again, no GTK-Doc support, and again, I don't care.


[*] We only pay its price of admission: gratuitous waste of screen space
by repeating the obvious.