GNOME Bugzilla – Bug 657303
[patches] A few fixes and enhancements for Mobile Broadband and VPN
Last modified: 2011-08-26 12:32:52 UTC
Please find attached 4 patches that fix Mobile Broadband "Options" button and displaying IP info and also fixes displaying VPN info. More details can be found in the original reports: https://bugzilla.redhat.com/show_bug.cgi?id=698054 https://bugzilla.redhat.com/show_bug.cgi?id=723489
Created attachment 194672 [details] [review] network: fix On/Off switch, Options button and info for Mobile Broadband
Created attachment 194673 [details] [review] network: fix displaying IP info to work also for static and PPP configurations This fixes displaying IPv4 stuff for static configurations (no DHCP) and also for Mobile Broadband.
Created attachment 194674 [details] [review] Fix refreshing VPN info
Created attachment 194675 [details] [review] Enhance VPN info Display VPN info for all VPN types (not just vpnc)
Review of attachment 194672 [details] [review]: Please update the commit message to not clip (72-char) ::: panels/network/cc-network-panel.c @@ -438,2 @@ g_free (operator_name); - g_free (operator_name_safe); is that a double-free fix? If so, please make it separately.
Review of attachment 194673 [details] [review]: Looks good. ::: panels/network/cc-network-panel.c @@ +1001,3 @@ + const GSList *list; + struct in_addr addr; + gchar *str = NULL; Use char, not gchar.
Review of attachment 194674 [details] [review]: Same problem with the commit log message.
Review of attachment 194675 [details] [review]: Commit log message again. ::: panels/network/net-vpn.c @@ +96,3 @@ + type = nm_setting_vpn_get_service_type (nm_connection_get_setting_vpn (priv->connection)); + p = strrchr (type, '.'); + priv->service_type = g_strdup (p ? p + 1 : type); Don't like that. @@ +168,3 @@ + if (g_strcmp0 (vpn_type, "pptp") == 0) return ""; + if (g_strcmp0 (vpn_type, "openconnect") == 0) return ""; + if (g_strcmp0 (vpn_type, "vpnc") == 0) return "IPSec ID"; Any reason why you're not returning NULL instead?
Review of attachment 194672 [details] [review]: ::: panels/network/cc-network-panel.c @@ -438,2 @@ g_free (operator_name); - g_free (operator_name_safe); Yeah, operator_name_safe is attached to device object via g_object_set_data_full() and freed when the object is destroyed. I'll separate the change.
Review of attachment 194673 [details] [review]: ::: panels/network/cc-network-panel.c @@ +1001,3 @@ + const GSList *list; + struct in_addr addr; + gchar *str = NULL; Hmm, I prefer char myself. But gchar is used exclusively throughout the file. Are there any guidelines on this?
Review of attachment 194675 [details] [review]: ::: panels/network/net-vpn.c @@ +96,3 @@ + type = nm_setting_vpn_get_service_type (nm_connection_get_setting_vpn (priv->connection)); + p = strrchr (type, '.'); + priv->service_type = g_strdup (p ? p + 1 : type); We need VPN type to display and the string from NM looks like this: "org.freedesktop.NetworkManager.vpnc". So we extract "vpnc" (openvpn or whatever the connection is). What's the problem with the code? @@ +168,3 @@ + if (g_strcmp0 (vpn_type, "pptp") == 0) return ""; + if (g_strcmp0 (vpn_type, "openconnect") == 0) return ""; + if (g_strcmp0 (vpn_type, "openswan") == 0) return ""; Hmm, not really. I've checked the usage again and the string is used as a key in g_hash_table_lookup(), which should be good. So I can change empty strings to NULL if you prefer.
(In reply to comment #10) > Review of attachment 194673 [details] [review]: > > ::: panels/network/cc-network-panel.c > @@ +1001,3 @@ > + const GSList *list; > + struct in_addr addr; > + gchar *str = NULL; > > Hmm, I prefer char myself. But gchar is used exclusively throughout the file. > Are there any guidelines on this? It's fine to use gchar then I guess.
(In reply to comment #11) > Review of attachment 194675 [details] [review]: > > ::: panels/network/net-vpn.c > @@ +96,3 @@ > + type = nm_setting_vpn_get_service_type (nm_connection_get_setting_vpn > (priv->connection)); > + p = strrchr (type, '.'); > + priv->service_type = g_strdup (p ? p + 1 : type); > > We need VPN type to display and the string from NM looks like this: > "org.freedesktop.NetworkManager.vpnc". So we extract "vpnc" (openvpn or > whatever the connection is). What's the problem with the code? Can you please separate it in a function with a meaningful name? Or, seeing as all the vpns start with "org.freedesktop.NetworkManager." remove that from the name. > @@ +168,3 @@ > + if (g_strcmp0 (vpn_type, "pptp") == 0) return ""; > + if (g_strcmp0 (vpn_type, "openconnect") == 0) return ""; > + if (g_strcmp0 (vpn_type, "openswan") == 0) return ""; > > Hmm, not really. I've checked the usage again and the string is used as a key > in g_hash_table_lookup(), which should be good. So I can change empty strings > to NULL if you prefer. OK, fine by me.
Review of attachment 194674 [details] [review]: Committed.