GNOME Bugzilla – Bug 773091
check: Add API to filter g_warning/g_critical etc
Last modified: 2017-01-24 00:14:40 UTC
New API functions to filter log messages before they are processed by GstCheck. This can be used to discard specific messages that are accepted by the test or to add callbacks that test specific messages. Default bevavior when no callback is given to a filter is to discard the message, because it does not makes sense to have a filter with no callback which does not discard; that would be a noop. Discarded messages will in addition to bypass the GstCheck handling also return to GLib that the message is not fatal if it occurs.
Created attachment 337839 [details] [review] check: Add API to filter g_warning/g_critical etc
Review of attachment 337839 [details] [review]: ::: libs/gst/check/gstcheck.c @@ +113,3 @@ + g_free (filter->log_domain); + + if (filter->regex) You allow regex==NULL here, and also log_domain. but the other code wouldn't like that much :) @@ +142,3 @@ + filter = gst_check_log_filter_alloc (log_domain, log_level, regex, + func, user_data); + _gst_check_log_filters = g_list_append (_gst_check_log_filters, filter); g_list_append() is expensive. Use a GQueue. Also probably needs a mutex for thread-safety? ::: libs/gst/check/gstcheck.h @@ +80,3 @@ void gst_check_init (int *argc, char **argv[]); +gpointer gst_check_log_filter_add (const gchar * log_domain, Make this pointer some opaque typedef
Created attachment 343427 [details] [review] Updated patch to address issues in the feedback. - regex is not allowed to be NULL - log_domain may be NULL (because glib allows it) - API uses opaque GstCheckLogFilter * instead of gpointer - Use GQueue - Thread safety - Bonus: Optional destory function for the user data
Comment on attachment 343427 [details] [review] Updated patch to address issues in the feedback. Thanks for the updated patch, looks mostly fine to me, just a few smaller things: >+ gst_check_log_filter_add \ >+ gst_check_log_filter_remove \ >+ gst_check_log_filter_clear \ I think I'd prefer gst_check_add_log_filter() etc. (even if both patterns exist in GStreamer). Maybe it should even be _new() with a separate _add() step? Not sure if that would be nicer for bindings or not. - Please add 'Since: 1.12' markers to all the gtk-doc chunks at the bottom
Created attachment 343640 [details] [review] Updated patch: - Added "Since: 1.12" - Changed function names I did not add a _new() function. Such a change has some side-effects. For instace I would expect a _free(), and then one has to make sure that the filter that's in the queue is alive (not freed) which suggests reference counting, and so on... I'd like to keep this simple as the use case is fairly specific and limited.
Makes sense, thanks for updating the patch: commit 0c36e5766dde198a8015e7f04306838407dc8a86 Author: Stian Selnes <stian@pexip.com> Date: Tue May 24 14:57:54 2016 +0200 check: Add API to filter g_warning/g_critical etc New API functions to filter log messages before they are processed by GstCheck. This can be used to discard specific messages that are accepted by the test or to add callbacks that test specific messages. Default bevavior when no callback is given to a filter is to discard the message, because it does not makes sense to have a filter with no callback which does not discard; that would be a noop. Discarded messages will in addition to bypass the GstCheck handling also return to GLib that the message is not fatal if it occurs. https://bugzilla.gnome.org/show_bug.cgi?id=773091