GNOME Bugzilla – Bug 757288
[review] lr/multiple-vpn: Support multiple Libreswan connections
Last modified: 2015-11-16 15:51:43 UTC
https://git.gnome.org/browse/network-manager-openswan/log/?h=lr/multiple-vpn 0735110 service: add --bus-name argument 6ce7421 helper: add --bus-name argument 65707ef service: process the configuration in the service, not the helper The 65707ef contains some heavyweight cleanup, hopefully an improvement. Needs some NetworkManager fixes (it crashes when a tunelled VPN sends in routes and doesn't support route source preference), which are reviewed here: bug #757287
>> service: process the configuration in the service, not the helper + environ = g_listenv (); must free environ. + if (!g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (priv->dbus_skeleton), + connection, + g_dbus_connection_register_object (connection, NM_VPN_DBUS_PLUGIN_PATH, + nmdbus_openswan_helper_interface_info (), whitespace + if (priv->dbus_skeleton) + skeleton = G_DBUS_INTERFACE_SKELETON (priv->dbus_skeleton); the cast macros are always NULL-safe, so the if is unnecessary. >> helper: add --bus-name argument + char *bus_name; const? + g_variant_builder_add_value (&builder, addr4_to_gvariant (lookup_string (env, "PLUTO_NEXT_HOP"))); you must ensure that addr4_to_gvariant() doesn't return NULL. static void _variant_builder_add_value (GVariantBuilder *builder, GVariant *variant) { if (variant) g_variant_builder_add_value (builder, variant); } ... _variant_builder_add_value (&builder, addr4_to_gvariant (lookup_string (env, "PLUTO_NEXT_HOP"))); >> service: add --bus-name argument + g_object_get (self, NM_VPN_SERVICE_PLUGIN_DBUS_SERVICE_NAME, &bus_name, NULL); I'm pretty sure that the getter transfers ownership of the string. You have to free it. Pushed fixups and a commit.
(In reply to Thomas Haller from comment #1) > >> service: process the configuration in the service, not the helper > > > + environ = g_listenv (); > > must free environ. Fixed. > + if (!g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON > (priv->dbus_skeleton), > + connection, > + g_dbus_connection_register_object (connection, NM_VPN_DBUS_PLUGIN_PATH, > + nmdbus_openswan_helper_interface_info (), > > whitespace Turned into blackspace. > + if (priv->dbus_skeleton) > + skeleton = G_DBUS_INTERFACE_SKELETON (priv->dbus_skeleton); > > the cast macros are always NULL-safe, so the if is unnecessary. Removed. > >> helper: add --bus-name argument > > > + char *bus_name; > > const? Yep, added. > + g_variant_builder_add_value (&builder, addr4_to_gvariant > (lookup_string (env, "PLUTO_NEXT_HOP"))); > > you must ensure that addr4_to_gvariant() doesn't return NULL. > > static void > _variant_builder_add_value (GVariantBuilder *builder, GVariant *variant) > { > if (variant) > g_variant_builder_add_value (builder, variant); > } > ... > _variant_builder_add_value (&builder, addr4_to_gvariant (lookup_string (env, > "PLUTO_NEXT_HOP"))); Applied your fixup to deal with that. > >> service: add --bus-name argument > > + g_object_get (self, NM_VPN_SERVICE_PLUGIN_DBUS_SERVICE_NAME, &bus_name, > NULL); > > I'm pretty sure that the getter transfers ownership of the string. You have > to free it. Fixed. > Pushed fixups and a commit. Thank you. Added a fixup for your commit, please take a look. Moved the branch to NetworkManager-libreswan: https://git.gnome.org/browse/network-manager-libreswan/log/?h=lr/multiple-vpn
LGTM
5648085 merge: branch 'lr/multiple-vpn' ab3b309 service: handle route updates and deletions 457b212 service: add --bus-name argument 9694f81 helper: add --bus-name argument acb9eb9 service: process the configuration in the service, not the helper 89b1b49 service: refactor the disconnect handling 22504d2 service: clean the pipe watcher source id 153b75c service: tolerate non-zero from ipsec auto --ready 160953a service: don't assert there's connection for all steps