GNOME Bugzilla – Bug 705712
Add network filtering
Last modified: 2013-08-16 11:16:23 UTC
Created attachment 251223 [details] [review] Coding Convention fix Add network filtering in GUPnP Create a new GUPnPWhiteList object that manages a list of entries that will be used to allow or disallow a GUPnPContext. It provides a function that check GUPnPContext objects and signals to indicate changes in the white list. GUPnPConextManager has been updated to use GUPnPWhiteList. To avoid any backward compatibility issue, the white list is disabled by default, and an empty white list that's enabled, behaves as disabled.
Created attachment 251224 [details] [review] Add White List to the build Add White List to the build, but it's not yet used.
Created attachment 251225 [details] [review] GUPnPContetManager implement white list GUPnPContetManager does network filtering by using GUPnPWhiteList
General note: It's either (L)GPL or all rights reserved. Please remove it :)
Review of attachment 251224 [details] [review]: Code style: There are several whitespace and parametere alignments missing. ::: libgupnp/gupnp-white-list.c @@ +149,3 @@ + G_PARAM_CONSTRUCT | + G_PARAM_READWRITE | + G_PARAM_STATIC_NAME | You can use G_PARAM_STATIC_STRINGS alias for the three *_STATIC_* things. @@ +208,3 @@ +gupnp_white_list_new (void) +{ + return g_object_new (GUPNP_TYPE_WHITE_LIST, "enabled", FALSE, NULL); Setting enabled here is unnecessary, it's a construct property and will get assigned the default value (FALSE). @@ +225,3 @@ + white_list->priv->enabled = enable; + g_signal_emit (white_list, signals[ENABLED], + 0, enable, white_list->priv->entries); You should add a property notification here, g_object_notify (white_list, "enabled"); It would be possible to remove the explicit "enabled" signal then and use "notify::enabled" from the generic GObject property notifications. That would probably need a "is_empty" convenience function which you could also use to get rid of the extra argument in the "changed" signal. @@ +273,3 @@ + + s_entry = g_list_find_custom (priv->entries, entry, + gupnp_white_list_compare_entry); You should be able to pass strcmp (or g_ascii_strcasecmp) directly (maybe needs a cast to GCompareFunc) @@ +276,3 @@ + + if (s_entry == NULL) { + priv->entries = g_list_prepend (priv->entries, g_strdup(entry)); missing ws before ( ::: libgupnp/gupnp-white-list.h @@ +106,3 @@ + +gboolean +gupnp_white_list_allow_context (GUPnPWhiteList *white_list, I think this name is misleading. It sounds as if the context would be added to the whitelist somehow
Review of attachment 251225 [details] [review]: Several style issues like missing parameter alignment and whitespaces before "(". Also kind of wondering why you put the whitelist code (which is rather stupid) into a separate class. ::: libgupnp/gupnp-context-manager.c @@ +59,2 @@ GList *objects; /* control points and root devices */ + GList *blacklist; /* Blacklisted Context */ I'd probably rename that to "blacklisted". Makes it clearer. @@ +93,3 @@ + if (wl && + gupnp_white_list_is_enabled(white_list) && + couldn't that just return true if the wl is enabled? @@ +154,3 @@ + black = g_list_find (manager->priv->blacklist, context); + + if (black) { No implicit boolean, use black == NULL (I know there are places where this is used, but let's try to avoid it for new code) @@ +198,3 @@ + if (match) + (void) gssdp_resource_browser_rescan (browser); + Shouldn't this be done only once?
(In reply to comment #5) > Review of attachment 251225 [details] [review]: > > Also kind of wondering why you put the whitelist code (which is rather stupid) > into a separate class. > It's in a separate class because I though we could extend it later to use it also to filter devices. We should use it in Control Point and just add another filtering function. If we plan to only filter networks, I can easily integrate the code in Context Manager and remove this object.
(In reply to comment #5) > Review of attachment 251225 [details] [review]: > .... > > ::: libgupnp/gupnp-context-manager.c > @@ +59,2 @@ > GList *objects; /* control points and root devices */ > + GList *blacklist; /* Blacklisted Context */ > > I'd probably rename that to "blacklisted". Makes it clearer. > Done. > @@ +93,3 @@ > + if (wl && > + gupnp_white_list_is_enabled(white_list) && > + > > couldn't that just return true if the wl is enabled? > Not sure to understand the question here. The wl could be enabled, but empty. So we need to test both conditions. > @@ +154,3 @@ > + black = g_list_find (manager->priv->blacklist, context); > + > + if (black) { > > No implicit boolean, use black == NULL (I know there are places where this is > used, but let's try to avoid it for new code) > Done. > @@ +198,3 @@ > + if (match) > + (void) gssdp_resource_browser_rescan (browser); > + > > Shouldn't this be done only once? We can't use gupnp_context_manager_rescan_control_points that would rescan all control points. So we call it on specific control point, only if it is/become 'active'. But it's called only once by control point.
(In reply to comment #4) > Review of attachment 251224 [details] [review]: > > Code style: There are several whitespace and parametere alignments missing. > > ::: libgupnp/gupnp-white-list.c > @@ +149,3 @@ > + G_PARAM_CONSTRUCT | > + G_PARAM_READWRITE | > + G_PARAM_STATIC_NAME | > > You can use G_PARAM_STATIC_STRINGS alias for the three *_STATIC_* things. > Done. > @@ +208,3 @@ > +gupnp_white_list_new (void) > +{ > + return g_object_new (GUPNP_TYPE_WHITE_LIST, "enabled", FALSE, NULL); > > Setting enabled here is unnecessary, it's a construct property and will get > assigned the default value (FALSE). > Done. > @@ +225,3 @@ > + white_list->priv->enabled = enable; > + g_signal_emit (white_list, signals[ENABLED], > + 0, enable, white_list->priv->entries); > > You should add a property notification here, g_object_notify (white_list, > "enabled"); It would be possible to remove the explicit "enabled" signal then > and use "notify::enabled" from the generic GObject property notifications. > > That would probably need a "is_empty" convenience function which you could also > use to get rid of the extra argument in the "changed" signal. > I will check. > @@ +273,3 @@ > + > + s_entry = g_list_find_custom (priv->entries, entry, > + gupnp_white_list_compare_entry); > > You should be able to pass strcmp (or g_ascii_strcasecmp) directly (maybe needs > a cast to GCompareFunc) > Done. > @@ +276,3 @@ > + > + if (s_entry == NULL) { > + priv->entries = g_list_prepend (priv->entries, > g_strdup(entry)); > > missing ws before ( > Done. > ::: libgupnp/gupnp-white-list.h > @@ +106,3 @@ > + > +gboolean > +gupnp_white_list_allow_context (GUPnPWhiteList *white_list, > > I think this name is misleading. It sounds as if the context would be added to > the whitelist somehow gupnp_white_list_context_is_matching gupnp_white_list_check_context
Created attachment 251504 [details] [review] Add White List to the build
Created attachment 251505 [details] [review] GUPnPContetManager implement white list
Pushed with minor modifications: - Remove the libtool wrapper script for the testcase - Fix a small style issue