From c54a2ed82f5efc7cfe1e91d1580980d634cfcd10 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 09:35:15 +0100 Subject: [PATCH 1/4] shared: add nm_strv_ptrarray_*() helpers These are simple macros/functions that append a heap-allocated string to a GPtrArray. It is intended for a GPtrArray which takes ownership of the strings (meaning, it has a g_free() GDestroyNotify). --- shared/nm-utils/nm-shared-utils.h | 71 +++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index 940a193f7d..e82b53f7cb 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -296,6 +296,37 @@ nm_memdup (gconstpointer data, gsize size) /*****************************************************************************/ +/* generic macro to convert an int to a (heap allocated) string. + * + * Usually, an inline function nm_strdup_int64() would be enough. However, + * that cannot be used for guint64. So, we would also need nm_strdup_uint64(). + * This causes suble error potential, because the caller needs to ensure to + * use the right one (and compiler isn't going to help as it silently casts). + * + * Instead, this generic macro is supposed to handle all integers correctly. */ +#if _NM_CC_SUPPORT_GENERIC +#define nm_strdup_int(val) \ + _Generic ((val), \ + gint8: g_strdup_printf ("%d", (int) (val)), \ + gint16: g_strdup_printf ("%d", (int) (val)), \ + gint32: g_strdup_printf ("%d", (int) (val)), \ + gint64: g_strdup_printf ("%"G_GINT64_FORMAT, (gint64) (val)), \ + \ + guint8: g_strdup_printf ("%u", (guint) (val)), \ + guint16: g_strdup_printf ("%u", (guint) (val)), \ + guint32: g_strdup_printf ("%u", (guint) (val)), \ + guint64: g_strdup_printf ("%"G_GUINT64_FORMAT, (guint64) (val)) \ + ) +#else +#define nm_strdup_int(val) \ + ( ( sizeof (val) == sizeof (guint64) \ + && ((typeof (val)) -1) > 0) \ + ? g_strdup_printf ("%"G_GUINT64_FORMAT, (guint64) (val)) \ + : g_strdup_printf ("%"G_GINT64_FORMAT, (gint64) (val))) +#endif + +/*****************************************************************************/ + extern const void *const _NM_PTRARRAY_EMPTY[1]; #define NM_PTRARRAY_EMPTY(type) ((type const*) _NM_PTRARRAY_EMPTY) @@ -959,4 +990,44 @@ void nm_utils_invoke_on_idle (NMUtilsInvokeOnIdleCallback callback, gpointer callback_user_data, GCancellable *cancellable); +/*****************************************************************************/ + +static inline void +nm_strv_ptrarray_add_string_take (GPtrArray *cmd, + char *str) +{ + nm_assert (cmd); + nm_assert (str); + + g_ptr_array_add (cmd, str); +} + +static inline void +nm_strv_ptrarray_add_string_dup (GPtrArray *cmd, + const char *str) +{ + nm_strv_ptrarray_add_string_take (cmd, + g_strdup (str)); +} + +#define nm_strv_ptrarray_add_string_concat(cmd, ...) \ + nm_strv_ptrarray_add_string_take ((cmd), g_strconcat (__VA_ARGS__, NULL)) + +#define nm_strv_ptrarray_add_string_printf(cmd, ...) \ + nm_strv_ptrarray_add_string_take ((cmd), g_strdup_printf (__VA_ARGS__)) + +#define nm_strv_ptrarray_add_int(cmd, val) \ + nm_strv_ptrarray_add_string_take ((cmd), nm_strdup_int (val)) + +static inline void +nm_strv_ptrarray_take_gstring (GPtrArray *cmd, + GString **gstr) +{ + nm_assert (gstr && *gstr); + + nm_strv_ptrarray_add_string_take (cmd, + g_string_free (g_steal_pointer (gstr), + FALSE)); +} + #endif /* __NM_SHARED_UTILS_H__ */ From 4419dbed13ffce7557f4bc3053c9717c6b37516d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 08:55:55 +0100 Subject: [PATCH 2/4] dnsmasq: refactor construction of command line options in create_dm_cmd_line() Having a NMCmdLine implementation here is wrong. For one, it local to nm-dnsmasq-manager.c and not reusable. If there is anything of value in such an implementation, then it should possibly also be useful at other places that create command line arguments. Note that in the end, command line arguments are just strv arrays. There are different ways how to construct that strv array. For example, do we need to clone the strings that we add? How to do that most elegantly and efficiently? The previous implementation for example used a GStringChunk for that (quite creative!). The point is, there are pros and cons about how to create strv arrays. But constructing command line options shouldn't be abstracted in a NMCmdLine API. It should use a suitable API for creating an strv array. Otherwise, it's too much abstraction. Drop NMCmdLine and use GPtrArray directly. Together with a few helper functions nm_strv_ptrarray_*() that is our preferred way to create such strv arrays. Is it perfect? No, we still g_strdup() static strings. That could be optimized. But then we would want an optimized API for constructing strv arrays, not NMCmdLine. --- src/dnsmasq/nm-dnsmasq-manager.c | 157 ++++++++++--------------------- 1 file changed, 52 insertions(+), 105 deletions(-) diff --git a/src/dnsmasq/nm-dnsmasq-manager.c b/src/dnsmasq/nm-dnsmasq-manager.c index 3fe2f48926..42917bfdd7 100644 --- a/src/dnsmasq/nm-dnsmasq-manager.c +++ b/src/dnsmasq/nm-dnsmasq-manager.c @@ -73,51 +73,6 @@ G_DEFINE_TYPE (NMDnsMasqManager, nm_dnsmasq_manager, G_TYPE_OBJECT) /*****************************************************************************/ -typedef struct { - GPtrArray *array; - GStringChunk *chunk; -} NMCmdLine; - -static NMCmdLine * -nm_cmd_line_new (void) -{ - NMCmdLine *cmd; - - cmd = g_slice_new (NMCmdLine); - cmd->array = g_ptr_array_new (); - cmd->chunk = g_string_chunk_new (1024); - - return cmd; -} - -static void -nm_cmd_line_destroy (NMCmdLine *cmd) -{ - g_ptr_array_free (cmd->array, TRUE); - g_string_chunk_free (cmd->chunk); - g_slice_free (NMCmdLine, cmd); -} - -static char * -nm_cmd_line_to_str (NMCmdLine *cmd) -{ - char *str; - - g_ptr_array_add (cmd->array, NULL); - str = g_strjoinv (" ", (char **) cmd->array->pdata); - g_ptr_array_remove_index (cmd->array, cmd->array->len - 1); - - return str; -} - -static void -nm_cmd_line_add_string (NMCmdLine *cmd, const char *str) -{ - g_ptr_array_add (cmd->array, g_string_chunk_insert (cmd->chunk, str)); -} - -/*****************************************************************************/ - static void dm_watch_cb (GPid pid, int status, gpointer user_data) { @@ -145,40 +100,39 @@ dm_watch_cb (GPid pid, int status, gpointer user_data) g_signal_emit (manager, signals[STATE_CHANGED], 0, NM_DNSMASQ_STATUS_DEAD); } -static NMCmdLine * +static GPtrArray * create_dm_cmd_line (const char *iface, const NMIP4Config *ip4_config, const char *pidfile, GError **error) { - NMCmdLine *cmd; + gs_unref_ptrarray GPtrArray *cmd = NULL; nm_auto_free_gstring GString *s = NULL; char first[INET_ADDRSTRLEN]; char last[INET_ADDRSTRLEN]; - char localaddr[INET_ADDRSTRLEN]; + char listen_address_s[INET_ADDRSTRLEN]; char tmpaddr[INET_ADDRSTRLEN]; - char *error_desc = NULL; + gs_free char *error_desc = NULL; const char *dm_binary; const NMPlatformIP4Address *listen_address; guint i, n; listen_address = nm_ip4_config_get_first_address (ip4_config); + g_return_val_if_fail (listen_address, NULL); dm_binary = nm_utils_find_helper ("dnsmasq", DNSMASQ_PATH, error); if (!dm_binary) return NULL; - s = g_string_sized_new (100); + cmd = g_ptr_array_new_with_free_func (g_free); - /* Create dnsmasq command line */ - cmd = nm_cmd_line_new (); - nm_cmd_line_add_string (cmd, dm_binary); + nm_strv_ptrarray_add_string_dup (cmd, dm_binary); if ( nm_logging_enabled (LOGL_TRACE, LOGD_SHARING) || getenv ("NM_DNSMASQ_DEBUG")) { - nm_cmd_line_add_string (cmd, "--log-dhcp"); - nm_cmd_line_add_string (cmd, "--log-queries"); + nm_strv_ptrarray_add_string_dup (cmd, "--log-dhcp"); + nm_strv_ptrarray_add_string_dup (cmd, "--log-queries"); } /* dnsmasq may read from its default config file location, which if that @@ -187,25 +141,23 @@ create_dm_cmd_line (const char *iface, * as the gateway or whatever. So tell dnsmasq not to use any config file * at all. */ - nm_cmd_line_add_string (cmd, "--conf-file=/dev/null"); + nm_strv_ptrarray_add_string_dup (cmd, "--conf-file=/dev/null"); - nm_cmd_line_add_string (cmd, "--no-hosts"); - nm_cmd_line_add_string (cmd, "--keep-in-foreground"); - nm_cmd_line_add_string (cmd, "--bind-interfaces"); - nm_cmd_line_add_string (cmd, "--except-interface=lo"); - nm_cmd_line_add_string (cmd, "--clear-on-reload"); + nm_strv_ptrarray_add_string_dup (cmd, "--no-hosts"); + nm_strv_ptrarray_add_string_dup (cmd, "--keep-in-foreground"); + nm_strv_ptrarray_add_string_dup (cmd, "--bind-interfaces"); + nm_strv_ptrarray_add_string_dup (cmd, "--except-interface=lo"); + nm_strv_ptrarray_add_string_dup (cmd, "--clear-on-reload"); /* Use strict order since in the case of VPN connections, the VPN's * nameservers will be first in resolv.conf, and those need to be tried * first by dnsmasq to successfully resolve names from the VPN. */ - nm_cmd_line_add_string (cmd, "--strict-order"); + nm_strv_ptrarray_add_string_dup (cmd, "--strict-order"); - nm_utils_inet4_ntop (listen_address->address, localaddr); - g_string_append (s, "--listen-address="); - g_string_append (s, localaddr); - nm_cmd_line_add_string (cmd, s->str); - g_string_truncate (s, 0); + nm_utils_inet4_ntop (listen_address->address, listen_address_s); + + nm_strv_ptrarray_add_string_concat (cmd, "--listen-address=", listen_address_s); if (!nm_dnsmasq_utils_get_range (listen_address, first, last, &error_desc)) { g_set_error_literal (error, @@ -213,59 +165,55 @@ create_dm_cmd_line (const char *iface, NM_MANAGER_ERROR_FAILED, error_desc); _LOGW ("failed to find DHCP address ranges: %s", error_desc); - g_free (error_desc); - nm_cmd_line_destroy (cmd); return NULL; } - g_string_append_printf (s, "--dhcp-range=%s,%s,60m", first, last); - nm_cmd_line_add_string (cmd, s->str); - g_string_truncate (s, 0); + nm_strv_ptrarray_add_string_printf (cmd, + "--dhcp-range=%s,%s,60m", + first, + last); if (nm_ip4_config_best_default_route_get (ip4_config)) { - g_string_append (s, "--dhcp-option=option:router,"); - g_string_append (s, localaddr); - nm_cmd_line_add_string (cmd, s->str); - g_string_truncate (s, 0); + nm_strv_ptrarray_add_string_concat (cmd, + "--dhcp-option=option:router,", + listen_address_s); } if ((n = nm_ip4_config_get_num_nameservers (ip4_config))) { + nm_gstring_prepare (&s); g_string_append (s, "--dhcp-option=option:dns-server"); for (i = 0; i < n; i++) { g_string_append_c (s, ','); g_string_append (s, nm_utils_inet4_ntop (nm_ip4_config_get_nameserver (ip4_config, i), tmpaddr)); } - nm_cmd_line_add_string (cmd, s->str); - g_string_truncate (s, 0); + nm_strv_ptrarray_take_gstring (cmd, &s); } if ((n = nm_ip4_config_get_num_searches (ip4_config))) { + nm_gstring_prepare (&s); g_string_append (s, "--dhcp-option=option:domain-search"); for (i = 0; i < n; i++) { g_string_append_c (s, ','); g_string_append (s, nm_ip4_config_get_search (ip4_config, i)); } - nm_cmd_line_add_string (cmd, s->str); - g_string_truncate (s, 0); + nm_strv_ptrarray_take_gstring (cmd, &s); } - nm_cmd_line_add_string (cmd, "--dhcp-lease-max=50"); + nm_strv_ptrarray_add_string_dup (cmd, "--dhcp-lease-max=50"); - g_string_append (s, "--dhcp-leasefile=" NMSTATEDIR); - g_string_append_printf (s, "/dnsmasq-%s.leases", iface); - nm_cmd_line_add_string (cmd, s->str); - g_string_truncate (s, 0); + nm_strv_ptrarray_add_string_printf (cmd, + "--dhcp-leasefile=%s/dnsmasq-%s.leases", + NMSTATEDIR, + iface); - g_string_append (s, "--pid-file="); - g_string_append (s, pidfile); - nm_cmd_line_add_string (cmd, s->str); - g_string_truncate (s, 0); + nm_strv_ptrarray_add_string_concat (cmd, "--pid-file=", pidfile); /* dnsmasq exits if the conf dir is not present */ if (g_file_test (CONFDIR, G_FILE_TEST_IS_DIR)) - nm_cmd_line_add_string (cmd, "--conf-dir=" CONFDIR); + nm_strv_ptrarray_add_string_dup (cmd, "--conf-dir=" CONFDIR); - return cmd; + g_ptr_array_add (cmd, NULL); + return g_steal_pointer (&cmd); } static void @@ -313,7 +261,7 @@ nm_dnsmasq_manager_start (NMDnsMasqManager *manager, GError **error) { NMDnsMasqManagerPrivate *priv; - NMCmdLine *dm_cmd; + gs_unref_ptrarray GPtrArray *dm_cmd = NULL; gs_free char *cmd_str = NULL; g_return_val_if_fail (NM_IS_DNSMASQ_MANAGER (manager), FALSE); @@ -328,28 +276,27 @@ nm_dnsmasq_manager_start (NMDnsMasqManager *manager, if (!dm_cmd) return FALSE; - g_ptr_array_add (dm_cmd->array, NULL); - _LOGI ("starting dnsmasq..."); - _LOGD ("command line: %s", (cmd_str = nm_cmd_line_to_str (dm_cmd))); + _LOGD ("command line: %s", (cmd_str = g_strjoinv (" ", (char **) dm_cmd->pdata))); priv->pid = 0; - if (!g_spawn_async (NULL, (char **) dm_cmd->array->pdata, NULL, + if (!g_spawn_async (NULL, + (char **) dm_cmd->pdata, + NULL, G_SPAWN_DO_NOT_REAP_CHILD, - nm_utils_setpgid, NULL, - &priv->pid, error)) { - goto out; - } + nm_utils_setpgid, + NULL, + &priv->pid, + error)) + return FALSE; + + nm_assert (priv->pid > 0); _LOGD ("dnsmasq started with pid %d", priv->pid); priv->dm_watch_id = g_child_watch_add (priv->pid, (GChildWatchFunc) dm_watch_cb, manager); - out: - if (dm_cmd) - nm_cmd_line_destroy (dm_cmd); - - return priv->pid > 0; + return TRUE; } void From 35d9169c3c4a0361c363eca8a34ed6c575a620f1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 10:30:12 +0100 Subject: [PATCH 3/4] ppp: replace NMCmdLine API with plain GPtrArray in create_pppd_cmd_line() --- src/ppp/nm-ppp-manager.c | 231 ++++++++++++++------------------------- 1 file changed, 85 insertions(+), 146 deletions(-) diff --git a/src/ppp/nm-ppp-manager.c b/src/ppp/nm-ppp-manager.c index 1595961f45..d012fcb710 100644 --- a/src/ppp/nm-ppp-manager.c +++ b/src/ppp/nm-ppp-manager.c @@ -675,61 +675,6 @@ out: /*****************************************************************************/ -typedef struct { - GPtrArray *array; - GStringChunk *chunk; -} NMCmdLine; - -static NMCmdLine * -nm_cmd_line_new (void) -{ - NMCmdLine *cmd; - - cmd = g_slice_new (NMCmdLine); - cmd->array = g_ptr_array_new (); - cmd->chunk = g_string_chunk_new (1024); - - return cmd; -} - -static void -nm_cmd_line_destroy (NMCmdLine *cmd) -{ - g_ptr_array_free (cmd->array, TRUE); - g_string_chunk_free (cmd->chunk); - g_slice_free (NMCmdLine, cmd); -} - -static char * -nm_cmd_line_to_str (NMCmdLine *cmd) -{ - char *str; - - g_ptr_array_add (cmd->array, NULL); - str = g_strjoinv (" ", (char **) cmd->array->pdata); - g_ptr_array_remove_index (cmd->array, cmd->array->len - 1); - - return str; -} - -static void -nm_cmd_line_add_string (NMCmdLine *cmd, const char *str) -{ - g_ptr_array_add (cmd->array, g_string_chunk_insert (cmd->chunk, str)); -} - -static void -nm_cmd_line_add_int (NMCmdLine *cmd, int i) -{ - char *str; - - str = g_strdup_printf ("%d", i); - nm_cmd_line_add_string (cmd, str); - g_free (str); -} - -/*****************************************************************************/ - NM_UTILS_LOOKUP_STR_DEFINE_STATIC (pppd_exit_code_to_str, int, NM_UTILS_LOOKUP_DEFAULT ("Unknown error"), NM_UTILS_LOOKUP_STR_ITEM ( 1, "Fatal pppd error"); @@ -801,7 +746,7 @@ pppd_timed_out (gpointer data) return FALSE; } -static NMCmdLine * +static GPtrArray * create_pppd_cmd_line (NMPPPManager *self, NMSettingPpp *setting, NMSettingPppoe *pppoe, @@ -814,9 +759,8 @@ create_pppd_cmd_line (NMPPPManager *self, { NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); const char *pppd_binary = NULL; - NMCmdLine *cmd; + gs_unref_ptrarray GPtrArray *cmd = NULL; gboolean ppp_debug; - static int unit; g_return_val_if_fail (setting != NULL, NULL); @@ -836,53 +780,50 @@ create_pppd_cmd_line (NMPPPManager *self, return NULL; } - /* Create pppd command line */ - cmd = nm_cmd_line_new (); - nm_cmd_line_add_string (cmd, pppd_binary); + cmd = g_ptr_array_new_with_free_func (g_free); - nm_cmd_line_add_string (cmd, "nodetach"); - nm_cmd_line_add_string (cmd, "lock"); + nm_strv_ptrarray_add_string_dup (cmd, pppd_binary); + + nm_strv_ptrarray_add_string_dup (cmd, "nodetach"); + nm_strv_ptrarray_add_string_dup (cmd, "lock"); /* NM handles setting the default route */ - nm_cmd_line_add_string (cmd, "nodefaultroute"); + nm_strv_ptrarray_add_string_dup (cmd, "nodefaultroute"); if (!ip4_enabled) - nm_cmd_line_add_string (cmd, "noip"); + nm_strv_ptrarray_add_string_dup (cmd, "noip"); if (ip6_enabled) { /* Allow IPv6 to be configured by IPV6CP */ - nm_cmd_line_add_string (cmd, "ipv6"); - nm_cmd_line_add_string (cmd, ","); + nm_strv_ptrarray_add_string_dup (cmd, "ipv6"); + nm_strv_ptrarray_add_string_dup (cmd, ","); } else - nm_cmd_line_add_string (cmd, "noipv6"); + nm_strv_ptrarray_add_string_dup (cmd, "noipv6"); ppp_debug = !!getenv ("NM_PPP_DEBUG"); if (nm_logging_enabled (LOGL_DEBUG, LOGD_PPP)) ppp_debug = TRUE; if (ppp_debug) - nm_cmd_line_add_string (cmd, "debug"); + nm_strv_ptrarray_add_string_dup (cmd, "debug"); if (ppp_name) { - nm_cmd_line_add_string (cmd, "user"); - nm_cmd_line_add_string (cmd, ppp_name); + nm_strv_ptrarray_add_string_dup (cmd, "user"); + nm_strv_ptrarray_add_string_dup (cmd, ppp_name); } if (pppoe) { - char *dev_str; const char *pppoe_service; - nm_cmd_line_add_string (cmd, "plugin"); - nm_cmd_line_add_string (cmd, "rp-pppoe.so"); + nm_strv_ptrarray_add_string_dup (cmd, "plugin"); + nm_strv_ptrarray_add_string_dup (cmd, "rp-pppoe.so"); - dev_str = g_strdup_printf ("nic-%s", priv->parent_iface); - nm_cmd_line_add_string (cmd, dev_str); - g_free (dev_str); + nm_strv_ptrarray_add_string_concat (cmd, "nic-", priv->parent_iface); pppoe_service = nm_setting_pppoe_get_service (pppoe); if (pppoe_service) { - nm_cmd_line_add_string (cmd, "rp_pppoe_service"); - nm_cmd_line_add_string (cmd, pppoe_service); + nm_strv_ptrarray_add_string_dup (cmd, "rp_pppoe_service"); + nm_strv_ptrarray_add_string_dup (cmd, pppoe_service); } } else if (adsl) { const char *protocol = nm_setting_adsl_get_protocol (adsl); @@ -891,110 +832,110 @@ create_pppd_cmd_line (NMPPPManager *self, guint32 vpi = nm_setting_adsl_get_vpi (adsl); guint32 vci = nm_setting_adsl_get_vci (adsl); const char *encaps = nm_setting_adsl_get_encapsulation (adsl); - char *vpivci; - nm_cmd_line_add_string (cmd, "plugin"); - nm_cmd_line_add_string (cmd, "pppoatm.so"); + nm_strv_ptrarray_add_string_dup (cmd, "plugin"); + nm_strv_ptrarray_add_string_dup (cmd, "pppoatm.so"); - vpivci = g_strdup_printf("%d.%d", vpi, vci); - nm_cmd_line_add_string (cmd, vpivci); - g_free (vpivci); + nm_strv_ptrarray_add_string_printf (cmd, "%d.%d", vpi, vci); if (g_strcmp0 (encaps, NM_SETTING_ADSL_ENCAPSULATION_LLC) == 0) - nm_cmd_line_add_string (cmd, "llc-encaps"); + nm_strv_ptrarray_add_string_dup (cmd, "llc-encaps"); else /*if (g_strcmp0 (encaps, NM_SETTING_ADSL_ENCAPSULATION_VCMUX) == 0)*/ - nm_cmd_line_add_string (cmd, "vc-encaps"); + nm_strv_ptrarray_add_string_dup (cmd, "vc-encaps"); } else if (!strcmp (protocol, NM_SETTING_ADSL_PROTOCOL_PPPOE)) { - nm_cmd_line_add_string (cmd, "plugin"); - nm_cmd_line_add_string (cmd, "rp-pppoe.so"); - nm_cmd_line_add_string (cmd, priv->parent_iface); + nm_strv_ptrarray_add_string_dup (cmd, "plugin"); + nm_strv_ptrarray_add_string_dup (cmd, "rp-pppoe.so"); + nm_strv_ptrarray_add_string_dup (cmd, priv->parent_iface); } - nm_cmd_line_add_string (cmd, "noipdefault"); + nm_strv_ptrarray_add_string_dup (cmd, "noipdefault"); } else { - nm_cmd_line_add_string (cmd, priv->parent_iface); + nm_strv_ptrarray_add_string_dup (cmd, priv->parent_iface); /* Don't send some random address as the local address */ - nm_cmd_line_add_string (cmd, "noipdefault"); + nm_strv_ptrarray_add_string_dup (cmd, "noipdefault"); } if (nm_setting_ppp_get_baud (setting)) - nm_cmd_line_add_int (cmd, nm_setting_ppp_get_baud (setting)); + nm_strv_ptrarray_add_int (cmd, nm_setting_ppp_get_baud (setting)); else if (baud_override) - nm_cmd_line_add_int (cmd, (int) baud_override); + nm_strv_ptrarray_add_int (cmd, baud_override); /* noauth by default, because we certainly don't have any information * with which to verify anything the peer gives us if we ask it to * authenticate itself, which is what 'auth' really means. */ - nm_cmd_line_add_string (cmd, "noauth"); + nm_strv_ptrarray_add_string_dup (cmd, "noauth"); if (nm_setting_ppp_get_refuse_eap (setting)) - nm_cmd_line_add_string (cmd, "refuse-eap"); + nm_strv_ptrarray_add_string_dup (cmd, "refuse-eap"); if (nm_setting_ppp_get_refuse_pap (setting)) - nm_cmd_line_add_string (cmd, "refuse-pap"); + nm_strv_ptrarray_add_string_dup (cmd, "refuse-pap"); if (nm_setting_ppp_get_refuse_chap (setting)) - nm_cmd_line_add_string (cmd, "refuse-chap"); + nm_strv_ptrarray_add_string_dup (cmd, "refuse-chap"); if (nm_setting_ppp_get_refuse_mschap (setting)) - nm_cmd_line_add_string (cmd, "refuse-mschap"); + nm_strv_ptrarray_add_string_dup (cmd, "refuse-mschap"); if (nm_setting_ppp_get_refuse_mschapv2 (setting)) - nm_cmd_line_add_string (cmd, "refuse-mschap-v2"); + nm_strv_ptrarray_add_string_dup (cmd, "refuse-mschap-v2"); if (nm_setting_ppp_get_nobsdcomp (setting)) - nm_cmd_line_add_string (cmd, "nobsdcomp"); + nm_strv_ptrarray_add_string_dup (cmd, "nobsdcomp"); if (nm_setting_ppp_get_no_vj_comp (setting)) - nm_cmd_line_add_string (cmd, "novj"); + nm_strv_ptrarray_add_string_dup (cmd, "novj"); if (nm_setting_ppp_get_nodeflate (setting)) - nm_cmd_line_add_string (cmd, "nodeflate"); + nm_strv_ptrarray_add_string_dup (cmd, "nodeflate"); if (nm_setting_ppp_get_require_mppe (setting)) - nm_cmd_line_add_string (cmd, "require-mppe"); + nm_strv_ptrarray_add_string_dup (cmd, "require-mppe"); if (nm_setting_ppp_get_require_mppe_128 (setting)) - nm_cmd_line_add_string (cmd, "require-mppe-128"); + nm_strv_ptrarray_add_string_dup (cmd, "require-mppe-128"); if (nm_setting_ppp_get_mppe_stateful (setting)) - nm_cmd_line_add_string (cmd, "mppe-stateful"); + nm_strv_ptrarray_add_string_dup (cmd, "mppe-stateful"); if (nm_setting_ppp_get_crtscts (setting)) - nm_cmd_line_add_string (cmd, "crtscts"); + nm_strv_ptrarray_add_string_dup (cmd, "crtscts"); /* Always ask for DNS, we don't have to use them if the connection * overrides the returned servers. */ - nm_cmd_line_add_string (cmd, "usepeerdns"); + nm_strv_ptrarray_add_string_dup (cmd, "usepeerdns"); if (nm_setting_ppp_get_mru (setting)) { - nm_cmd_line_add_string (cmd, "mru"); - nm_cmd_line_add_int (cmd, nm_setting_ppp_get_mru (setting)); + nm_strv_ptrarray_add_string_dup (cmd, "mru"); + nm_strv_ptrarray_add_int (cmd, nm_setting_ppp_get_mru (setting)); } if (nm_setting_ppp_get_mtu (setting)) { - nm_cmd_line_add_string (cmd, "mtu"); - nm_cmd_line_add_int (cmd, nm_setting_ppp_get_mtu (setting)); + nm_strv_ptrarray_add_string_dup (cmd, "mtu"); + nm_strv_ptrarray_add_int (cmd, nm_setting_ppp_get_mtu (setting)); } - nm_cmd_line_add_string (cmd, "lcp-echo-failure"); - nm_cmd_line_add_int (cmd, nm_setting_ppp_get_lcp_echo_failure (setting)); + nm_strv_ptrarray_add_string_dup (cmd, "lcp-echo-failure"); + nm_strv_ptrarray_add_int (cmd, nm_setting_ppp_get_lcp_echo_failure (setting)); - nm_cmd_line_add_string (cmd, "lcp-echo-interval"); - nm_cmd_line_add_int (cmd, nm_setting_ppp_get_lcp_echo_interval (setting)); + nm_strv_ptrarray_add_string_dup (cmd, "lcp-echo-interval"); + nm_strv_ptrarray_add_int (cmd, nm_setting_ppp_get_lcp_echo_interval (setting)); /* Avoid pppd to exit if no traffic going through */ - nm_cmd_line_add_string (cmd, "idle"); - nm_cmd_line_add_int (cmd, 0); + nm_strv_ptrarray_add_string_dup (cmd, "idle"); + nm_strv_ptrarray_add_string_dup (cmd, "0"); - nm_cmd_line_add_string (cmd, "ipparam"); - nm_cmd_line_add_string (cmd, nm_dbus_object_get_path (NM_DBUS_OBJECT (self))); + nm_strv_ptrarray_add_string_dup (cmd, "ipparam"); + nm_strv_ptrarray_add_string_dup (cmd, nm_dbus_object_get_path (NM_DBUS_OBJECT (self))); - nm_cmd_line_add_string (cmd, "plugin"); - nm_cmd_line_add_string (cmd, NM_PPPD_PLUGIN); + nm_strv_ptrarray_add_string_dup (cmd, "plugin"); + nm_strv_ptrarray_add_string_dup (cmd, NM_PPPD_PLUGIN); if (pppoe && nm_setting_pppoe_get_parent (pppoe)) { + static int unit; + /* The PPP interface is going to be renamed, so pass a * different unit each time so that activations don't * race with each others. */ - nm_cmd_line_add_string (cmd, "unit"); - nm_cmd_line_add_int (cmd, unit); + nm_strv_ptrarray_add_string_dup (cmd, "unit"); + nm_strv_ptrarray_add_int (cmd, unit); unit = unit < G_MAXINT ? unit + 1 : 0; } - return cmd; + g_ptr_array_add (cmd, NULL); + return g_steal_pointer (&cmd); } static void @@ -1038,8 +979,8 @@ _ppp_manager_start (NMPPPManager *self, gs_unref_object NMSettingPpp *s_ppp_free = NULL; NMSettingPppoe *pppoe_setting; NMSettingAdsl *adsl_setting; - NMCmdLine *ppp_cmd; - char *cmd_str; + gs_unref_ptrarray GPtrArray *ppp_cmd = NULL; + gs_free char *cmd_str = NULL; struct stat st; const char *ip6_method, *ip4_method; gboolean ip6_enabled = FALSE; @@ -1104,23 +1045,25 @@ _ppp_manager_start (NMPPPManager *self, ip6_enabled, err); if (!ppp_cmd) - goto out; - - g_ptr_array_add (ppp_cmd->array, NULL); + goto fail; _LOGI ("starting PPP connection"); - cmd_str = nm_cmd_line_to_str (ppp_cmd); _LOGD ("command line: %s", cmd_str); - g_free (cmd_str); + (cmd_str = g_strjoinv (" ", (char **) ppp_cmd->pdata)); priv->pid = 0; - if (!g_spawn_async (NULL, (char **) ppp_cmd->array->pdata, NULL, + if (!g_spawn_async (NULL, + (char **) ppp_cmd->pdata, + NULL, G_SPAWN_DO_NOT_REAP_CHILD, - nm_utils_setpgid, NULL, - &priv->pid, err)) { - goto out; - } + nm_utils_setpgid, + NULL, + &priv->pid, + err)) + goto fail; + + nm_assert (priv->pid > 0); _LOGI ("pppd started with pid %lld", (long long) priv->pid); @@ -1128,14 +1071,10 @@ _ppp_manager_start (NMPPPManager *self, priv->ppp_timeout_handler = g_timeout_add_seconds (timeout_secs, pppd_timed_out, self); priv->act_req = g_object_ref (req); -out: - if (ppp_cmd) - nm_cmd_line_destroy (ppp_cmd); - - if (priv->pid <= 0) - nm_dbus_object_unexport (NM_DBUS_OBJECT (self)); - - return priv->pid > 0; + return TRUE; +fail: + nm_dbus_object_unexport (NM_DBUS_OBJECT (self)); + return FALSE; } static void From 1a2e767f1fd2d8b550ab61cdfabc721280cf923e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 10:58:59 +0100 Subject: [PATCH 4/4] device/shared: set ANDROID_METERED option 43 for shared connections The problem is that updating the metered value of a shared connection is not implemented. The user needs to fully reactivate the profile for changes to take effect. That is unfortunate, especially because reapplying the route metric works in other other cases. --- src/devices/nm-device.c | 35 ++++++++++++++++++++++++++++++-- src/dnsmasq/nm-dnsmasq-manager.c | 14 ++++++++++++- src/dnsmasq/nm-dnsmasq-manager.h | 1 + src/nm-manager.c | 8 ++++++++ src/nm-manager.h | 2 ++ 5 files changed, 57 insertions(+), 3 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 57021e4b79..c2223a9850 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -10175,6 +10175,9 @@ start_sharing (NMDevice *self, NMIP4Config *config, GError **error) const NMPlatformIP4Address *ip4_addr = NULL; const char *ip_iface; GError *local = NULL; + NMConnection *conn; + NMSettingConnection *s_con; + gboolean announce_android_metered; g_return_val_if_fail (config, FALSE); @@ -10196,7 +10199,7 @@ start_sharing (NMDevice *self, NMIP4Config *config, GError **error) return FALSE; req = nm_device_get_act_request (self); - g_assert (req); + g_return_val_if_fail (req, FALSE); netmask = _nm_utils_ip4_prefix_to_netmask (ip4_addr->plen); nm_utils_inet4_ntop (netmask, str_mask); @@ -10217,7 +10220,35 @@ start_sharing (NMDevice *self, NMIP4Config *config, GError **error) nm_act_request_set_shared (req, TRUE); - if (!nm_dnsmasq_manager_start (priv->dnsmasq_manager, config, &local)) { + conn = nm_act_request_get_applied_connection (req); + s_con = nm_connection_get_setting_connection (conn); + + switch (nm_setting_connection_get_metered (s_con)) { + case NM_METERED_YES: + /* honor the metered flag. Note that reapply on the device does not affect + * the metered setting. This is different from other profiles, where the + * metered flag of an activated profile can be changed (reapplied). */ + announce_android_metered = TRUE; + break; + case NM_METERED_UNKNOWN: + /* we pick up the current value and announce it. But again, we cannot update + * the announced setting without restarting dnsmasq. That means, if the default + * route changes w.r.t. being metered, then the shared connection does not get + * updated before reactivating. */ + announce_android_metered = NM_IN_SET (nm_manager_get_metered (nm_manager_get ()), + NM_METERED_YES, + NM_METERED_GUESS_YES); + break; + default: + announce_android_metered = FALSE; + break; + } + + + if (!nm_dnsmasq_manager_start (priv->dnsmasq_manager, + config, + announce_android_metered, + &local)) { g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, "could not start dnsmasq due to %s", local->message); g_error_free (local); diff --git a/src/dnsmasq/nm-dnsmasq-manager.c b/src/dnsmasq/nm-dnsmasq-manager.c index 42917bfdd7..1afc6e0d71 100644 --- a/src/dnsmasq/nm-dnsmasq-manager.c +++ b/src/dnsmasq/nm-dnsmasq-manager.c @@ -104,6 +104,7 @@ static GPtrArray * create_dm_cmd_line (const char *iface, const NMIP4Config *ip4_config, const char *pidfile, + gboolean announce_android_metered, GError **error) { gs_unref_ptrarray GPtrArray *cmd = NULL; @@ -199,6 +200,12 @@ create_dm_cmd_line (const char *iface, nm_strv_ptrarray_take_gstring (cmd, &s); } + if (announce_android_metered) { + /* force option 43 to announce ANDROID_METERED. Do this, even if the client + * did not ask for this option. See https://www.lorier.net/docs/android-metered.html */ + nm_strv_ptrarray_add_string_dup (cmd, "--dhcp-option-force=43,ANDROID_METERED"); + } + nm_strv_ptrarray_add_string_dup (cmd, "--dhcp-lease-max=50"); nm_strv_ptrarray_add_string_printf (cmd, @@ -258,6 +265,7 @@ out: gboolean nm_dnsmasq_manager_start (NMDnsMasqManager *manager, NMIP4Config *ip4_config, + gboolean announce_android_metered, GError **error) { NMDnsMasqManagerPrivate *priv; @@ -272,7 +280,11 @@ nm_dnsmasq_manager_start (NMDnsMasqManager *manager, kill_existing_by_pidfile (priv->pidfile); - dm_cmd = create_dm_cmd_line (priv->iface, ip4_config, priv->pidfile, error); + dm_cmd = create_dm_cmd_line (priv->iface, + ip4_config, + priv->pidfile, + announce_android_metered, + error); if (!dm_cmd) return FALSE; diff --git a/src/dnsmasq/nm-dnsmasq-manager.h b/src/dnsmasq/nm-dnsmasq-manager.h index a0ad295cd4..dd4a9069c0 100644 --- a/src/dnsmasq/nm-dnsmasq-manager.h +++ b/src/dnsmasq/nm-dnsmasq-manager.h @@ -48,6 +48,7 @@ NMDnsMasqManager *nm_dnsmasq_manager_new (const char *iface); gboolean nm_dnsmasq_manager_start (NMDnsMasqManager *manager, NMIP4Config *ip4_config, + gboolean announce_android_metered, GError **error); void nm_dnsmasq_manager_stop (NMDnsMasqManager *manager); diff --git a/src/nm-manager.c b/src/nm-manager.c index 0710483d1a..7914d16d1b 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -1445,6 +1445,14 @@ nm_manager_update_metered (NMManager *self) } } +NMMetered +nm_manager_get_metered (NMManager *self) +{ + g_return_val_if_fail (NM_IS_MANAGER (self), NM_METERED_UNKNOWN); + + return NM_MANAGER_GET_PRIVATE (self)->metered; +} + static void nm_manager_update_state (NMManager *self) { diff --git a/src/nm-manager.h b/src/nm-manager.h index 11cba1a5ce..06ca633de8 100644 --- a/src/nm-manager.h +++ b/src/nm-manager.h @@ -174,4 +174,6 @@ void nm_manager_dbus_set_property_handle (NMDBusObject *obj, GVariant *value, gpointer user_data); +NMMetered nm_manager_get_metered (NMManager *self); + #endif /* __NETWORKMANAGER_MANAGER_H__ */