[PATCH 00/15 RESEND] Improve PAC URL Mutation and PAC URL and WISPr Proxy Failure Logging

[PATCH 00/15 RESEND] Improve PAC URL Mutation and PAC URL and WISPr Proxy Failure Logging

[Grant Erickson]

There appear to be a population of premises ISP equipment (some
possibly correlated with Spectrum Communications in Southern
California, United States) in which the ISP equipment sporadically and
incorrectly emits DHCP Option 252 ("Web Proxy Auto-Discovery (WPAD)")
with an invalid proxy auto-configuration (PAC) URL (such as
'http://wpad.lan/wpad.dat'), even when neither the customer nor the
ISP has such an option actively configured on the equipment.

This URL, per the WPAD specification, leads the Connection Manager to
believe that it must use a proxy to reach HTTP- or HTTPS-based
resources, including those specified for WISPr online reachability
checks. Because neither the PAC file ('wpad.dat' in the example above)
nor the host ('wpad.lan' in the example above) associated with the PAC
URL exist, those online reachability checks unconditionally fail
despite such resources being directly reachable, without an
intervening proxy.

Because:

  * PAC configuration (or lack thereof) has not been historically
  * POSIX error codes are themselves limited (for example, there are
    no -ENOPROXY, -EBADPROXY, or -EPROXYLOOKUP codes)
  * POSIX error code usage for WISPr failures is similarly limited
    (for example, -EINVAL)

insight into the actual nature of an online reachability check failure
is, by extension, limited, particularly when proxy failures are
involved. Consequently, log messages generated from these changes
provide crucial insight into triaging and resolving such failures.

Finally, there have been three (3) call sites in the network service
code where the network service proxy auto-configuration (PAC) URL was
set. However, it was never (but now, with these changes, is) set in
'__connman_service_set_proxy_autoconfig', where it is invoked by the
DHCP client code when DHCP option 252 (Web Proxy Auto-Discovery
(WPAD)) is encountered.

Grant Erickson (15):
  service: Add 'service_log_pac' function.
  service: Add 'service_set_pac' function.
  service: Leverage 'service_set_pac' function.
  service: Ensure the PAC URL is set in
    '__connman_service_set_proxy_autoconfig'.
  service: Document 'service_log_pac'.
  service: Document 'service_set_pac'.
  service: Add 'service_set_proxy_method' function.
  service: Leverage 'service_set_proxy_method'.
  service: Leverage 'service_set_proxy_method'.
  service: Document 'service_set_proxy_method'.
  service: Document 'service_set_proxy_method_auto_handler'.
  service: Document '__connman_service_set_proxy_autoconfig'.
  wispr: Add 'wispr_log_proxy_failure' function.
  wispr: Leverage 'wispr_log_proxy_failure'.
  wispr: Document 'wispr_log_proxy_failure'.

 src/service.c | 273 +++++++++++++++++++++++++++++++++++++++++++-------
 src/wispr.c   |  42 +++++++-
 2 files changed, 276 insertions(+), 39 deletions(-)

-- 
2.45.0

[PATCH 01/15] service: Add 'service_log_pac' function.

[Grant Erickson]

Logs the network service interface name and type at the info level
when the proxy auto-configuration (PAC) URL is set or cleared.

Frequently, a spurious, misconfigured, or otherwise incorrect PAC URL
is the source of connectivity problems, particularly for online
reachability checks.
---
 src/service.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/service.c b/src/service.c
index 805cfca7ad12..0ab299e8ecbf 100644
--- a/src/service.c
+++ b/src/service.c
@@ -555,6 +555,19 @@ int __connman_service_load_modifiable(struct connman_service *service)
 	return 0;
 }
 
+static void service_log_pac(const struct connman_service *service,
+				const char *url)
+{
+	g_autofree char *interface = NULL;
+
+	interface = connman_service_get_interface(service);
+
+	connman_info("Interface %s [ %s ] proxy auto-configuration (PAC) URL %s.",
+				 interface,
+				 __connman_service_type2string(service->type),
+				 url ? url : "is not set");
+}
+
 static int service_load(struct connman_service *service)
 {
 	GKeyFile *keyfile;
-- 
2.45.0

[PATCH 02/15] service: Add 'service_set_pac' function.

[Grant Erickson]

Allows for logging, setting, and dispatching property changed
notifications from a single place.
---
 src/service.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/service.c b/src/service.c
index 0ab299e8ecbf..c48959a7940f 100644
--- a/src/service.c
+++ b/src/service.c
@@ -233,6 +233,7 @@ static struct connman_ipconfig *create_ip4config(struct connman_service *service
 static struct connman_ipconfig *create_ip6config(struct connman_service *service,
 		int index);
 static void dns_changed(struct connman_service *service);
+static void proxy_changed(struct connman_service *service);
 static void vpn_auto_connect(void);
 static void trigger_autoconnect(struct connman_service *service);
 static void service_list_sort(const char *function);
@@ -568,6 +569,32 @@ static void service_log_pac(const struct connman_service *service,
 				 url ? url : "is not set");
 }
 
+static void service_set_pac(struct connman_service *service,
+				const char *pac,
+				bool dochanged)
+{
+	DBG("service %p (%s) pac %p (%s) dochanged %u",
+		service, connman_service_get_identifier(service),
+		pac,
+		pac ? pac : "<null>",
+		dochanged);
+
+	if (service->hidden)
+		return;
+
+	service_log_pac(service, pac);
+
+	g_free(service->pac);
+
+	if (pac && strlen(pac) > 0)
+		service->pac = g_strstrip(g_strdup(pac));
+	else
+		service->pac = NULL;
+
+	if (dochanged)
+		proxy_changed(service);
+}
+
 static int service_load(struct connman_service *service)
 {
 	GKeyFile *keyfile;
-- 
2.45.0

[PATCH 03/15] service: Leverage 'service_set_pac' function.

[Grant Erickson]

Leverage 'service_set_pac' at the three call sites the proxy
auto-configuration (PAC) URL is currently set or cleared for a network
service.

Frequently, a spurious, misconfigured, or otherwise incorrect PAC URL
is the source of connectivity problems, particularly for online
reachability checks. Consequently, being able to log all such
mutations can be helpful in triaging such problems.
---
 src/service.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/src/service.c b/src/service.c
index c48959a7940f..da851d681bc7 100644
--- a/src/service.c
+++ b/src/service.c
@@ -770,8 +770,7 @@ static int service_load(struct connman_service *service)
 	str = g_key_file_get_string(keyfile,
 				service->identifier, "Proxy.URL", NULL);
 	if (str) {
-		g_free(service->pac);
-		service->pac = str;
+		service_set_pac(service, str, false);
 	}
 
 	service->mdns_config = g_key_file_get_boolean(keyfile,
@@ -5757,12 +5756,9 @@ void __connman_service_timeserver_changed(struct connman_service *service,
 void __connman_service_set_pac(struct connman_service *service,
 					const char *pac)
 {
-	if (service->hidden)
-		return;
-	g_free(service->pac);
-	service->pac = g_strdup(pac);
+	const bool dochanged = true;
 
-	proxy_changed(service);
+	service_set_pac(service, pac, dochanged);
 }
 
 void __connman_service_set_agent_identity(struct connman_service *service,
@@ -6049,12 +6045,7 @@ static int update_proxy_configuration(struct connman_service *service,
 
 		break;
 	case CONNMAN_SERVICE_PROXY_METHOD_AUTO:
-		g_free(service->pac);
-
-		if (url && strlen(url) > 0)
-			service->pac = g_strstrip(g_strdup(url));
-		else
-			service->pac = NULL;
+		service_set_pac(service, url, false);
 
 		/* if we are connected:
 		   - if service->pac == NULL
-- 
2.45.0

[PATCH 04/15] service: Ensure the PAC URL is set in '__connman_service_set_proxy_autoconfig'.

[Grant Erickson]

Historically, there have been three (3) call sites in the network
service code where the network service proxy auto-configuration (PAC)
URL is set. However, it was never (but now, with this change, is) set
in '__connman_service_set_proxy_autoconfig', where it is invoked by
the DHCP client code when DHCP option 252 (Web Proxy Auto-Discovery
(WPAD)) is encountered.
---
 src/service.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/service.c b/src/service.c
index da851d681bc7..73cf8bca71a2 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5600,9 +5600,13 @@ const char *connman_service_get_proxy_url(const struct connman_service *service)
 void __connman_service_set_proxy_autoconfig(struct connman_service *service,
 							const char *url)
 {
+	const bool dochanged = true;
+
 	if (!service || service->hidden)
 		return;
 
+	service_set_pac(service, url, !dochanged);
+
 	service->proxy = CONNMAN_SERVICE_PROXY_METHOD_AUTO;
 
 	if (service->ipconfig_ipv4) {
-- 
2.45.0

[PATCH 05/15] service: Document 'service_log_pac'.

[Grant Erickson]

Add documentation to the 'service_log_pac' function.
---
 src/service.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/service.c b/src/service.c
index 73cf8bca71a2..a4b13be2fd47 100644
--- a/src/service.c
+++ b/src/service.c
@@ -556,6 +556,21 @@ int __connman_service_load_modifiable(struct connman_service *service)
 	return 0;
 }
 
+/**
+ *  @brief
+ *    Log the proxy auto-configuration (PAC) URL associated with the
+ *    specified service.
+ *
+ *  @param[in]  service  A pointer to the immutable network
+ *                       service for which to log the proxy
+ *                       auto-configuration (PAC) URL.
+ *  @param[in]  url      An optional pointer to the immutable null-
+ *                       terminated C string containing the proxy
+ *                       auto-configuration (PAC) URL.
+ *
+ *  @private
+ *
+ */
 static void service_log_pac(const struct connman_service *service,
 				const char *url)
 {
-- 
2.45.0

[PATCH 06/15] service: Document 'service_set_pac'.

[Grant Erickson]

Add documentation to the 'service_set_pac' function.
---
 src/service.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/service.c b/src/service.c
index a4b13be2fd47..0713520d88a1 100644
--- a/src/service.c
+++ b/src/service.c
@@ -584,6 +584,30 @@ static void service_log_pac(const struct connman_service *service,
 				 url ? url : "is not set");
 }
 
+/**
+ *  @brief
+ *    Set and log the proxy auto-configuration (PAC) URL for the
+ *    specified service.
+ *
+ *  If the specified service is a hidden service, no set or log
+ *  actions are taken.
+ *
+ *  @param[in,out]  service    A pointer to the mutable network
+ *                             service for which to set the proxy
+ *                             auto-configuration (PAC) URL.
+ *  @param[in]      url        An pointer to the immutable null-
+ *                             terminated C string containing the proxy
+ *                             auto-configuration (PAC) URL to set.
+ *  @param[in]      dochanged  A Boolean indicating whether or a D-Bus
+ *                             change notification should be sent for
+ *                             the service "Proxy" property.
+ *
+ *  @sa proxy_changed
+ *  @sa service_log_pac
+ *
+ *  @private
+ *
+ */
 static void service_set_pac(struct connman_service *service,
 				const char *pac,
 				bool dochanged)
-- 
2.45.0

[PATCH 07/15] service: Add 'service_set_proxy_method' function.

[Grant Erickson]

Allows for logging, setting, and dispatching property changed
notifications for the network service proxy method and associated URL,
if any, from a single place.
---
 src/service.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/src/service.c b/src/service.c
index 0713520d88a1..e58958635992 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5568,6 +5568,37 @@ const char * const *connman_service_get_timeservers(const struct connman_service
 	return (const char * const *)service->timeservers;
 }
 
+static void service_set_proxy_method(struct connman_service *service,
+			enum connman_service_proxy_method method,
+			bool donotifier,
+			bool (*handler)(struct connman_service *service,
+				enum connman_service_proxy_method method,
+				const void *context),
+			const void *context)
+{
+	DBG("service %p (%s) method %d (%s) donotifier %u "
+		"handler %p, context %p",
+		service, connman_service_get_identifier(service),
+		method, proxymethod2string(method),
+		donotifier,
+		handler,
+		context);
+
+	if (!service || service->hidden)
+		return;
+
+	service->proxy = method;
+
+	if (handler != NULL)
+		if (handler(service, method, context) != true)
+			return;
+
+	proxy_changed(service);
+
+	if (donotifier)
+		__connman_notifier_proxy_changed(service);
+}
+
 /**
  *  @brief
  *    Set the web proxy method of the specified service.
-- 
2.45.0

[PATCH 08/15] service: Leverage 'service_set_proxy_method'.

[Grant Erickson]

Leverage 'service_set_proxy_method' for
'connman_service_set_proxy_method', the first of two call sites.
---
 src/service.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/src/service.c b/src/service.c
index e58958635992..5c94c2962de8 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5617,19 +5617,14 @@ static void service_set_proxy_method(struct connman_service *service,
 void connman_service_set_proxy_method(struct connman_service *service,
 					enum connman_service_proxy_method method)
 {
-	DBG("service %p (%s) method %d (%s)",
-		service, connman_service_get_identifier(service),
-		method, proxymethod2string(method));
-
-	if (!service || service->hidden)
-		return;
-
-	service->proxy = method;
+	const bool donotifier = method != CONNMAN_SERVICE_PROXY_METHOD_AUTO;
+	void * const context = NULL;
 
-	proxy_changed(service);
-
-	if (method != CONNMAN_SERVICE_PROXY_METHOD_AUTO)
-		__connman_notifier_proxy_changed(service);
+	service_set_proxy_method(service,
+		method,
+		donotifier,
+		NULL,
+		context);
 }
 
 enum connman_service_proxy_method connman_service_get_proxy_method(
-- 
2.45.0

[PATCH 09/15] service: Leverage 'service_set_proxy_method'.

[Grant Erickson]

Leverage 'service_set_proxy_method' for
'__connman_service_set_proxy_autoconfig', the second of two call
sites.
---
 src/service.c | 49 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/src/service.c b/src/service.c
index 5c94c2962de8..d2d8e7cea065 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5662,32 +5662,47 @@ const char *connman_service_get_proxy_url(const struct connman_service *service)
 	return service->pac;
 }
 
-void __connman_service_set_proxy_autoconfig(struct connman_service *service,
-							const char *url)
+static bool service_set_proxy_method_auto_handler(
+				struct connman_service *service,
+				enum connman_service_proxy_method method,
+				const void *context)
 {
-	const bool dochanged = true;
-
-	if (!service || service->hidden)
-		return;
-
-	service_set_pac(service, url, !dochanged);
-
-	service->proxy = CONNMAN_SERVICE_PROXY_METHOD_AUTO;
+	const char * const url = context;
 
 	if (service->ipconfig_ipv4) {
 		if (__connman_ipconfig_set_proxy_autoconfig(
-			    service->ipconfig_ipv4, url) < 0)
-			return;
+				service->ipconfig_ipv4, url) < 0)
+			return false;
 	} else if (service->ipconfig_ipv6) {
 		if (__connman_ipconfig_set_proxy_autoconfig(
-			    service->ipconfig_ipv6, url) < 0)
-			return;
+				service->ipconfig_ipv6, url) < 0)
+			return false;
 	} else
-		return;
+		return false;
 
-	proxy_changed(service);
+	return true;
+}
+
+void __connman_service_set_proxy_autoconfig(struct connman_service *service,
+							const char *url)
+{
+	const bool dochanged = true;
+	const bool donotifier = true;
+	const void *context = url;
 
-	__connman_notifier_proxy_changed(service);
+	DBG("service %p (%s) url %p (%s)",
+		service,
+		connman_service_get_identifier(service),
+		url,
+		url ? url : "<null>");
+
+	service_set_pac(service, url, !dochanged);
+
+	service_set_proxy_method(service,
+		CONNMAN_SERVICE_PROXY_METHOD_AUTO,
+		donotifier,
+		service_set_proxy_method_auto_handler,
+		context);
 }
 
 const char *connman_service_get_proxy_autoconfig(struct connman_service *service)
-- 
2.45.0

[PATCH 10/15] service: Document 'service_set_proxy_method'.

[Grant Erickson]

Add documentation to the 'service_set_proxy_method' function.
---
 src/service.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/src/service.c b/src/service.c
index d2d8e7cea065..3fd56188093d 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5568,6 +5568,41 @@ const char * const *connman_service_get_timeservers(const struct connman_service
 	return (const char * const *)service->timeservers;
 }
 
+/**
+ *  @brief
+ *    Set the proxy method for the specified service.
+ *
+ *  This attempts to set the proxy method for the specified
+ *  service. If the service is null or if the service is hidden, no
+ *  action is taken. If specified, a handler will be invoked, with the
+ *  specified context, after setting the proxy method.
+ *
+ *  If the handler is specified and it returns true, following, a
+ *  D-Bus change notification should be sent for the service "Proxy"
+ *  property and, if requested by @a donotifier, the proxy changed
+ *  notifier chain will be run.
+ *
+ *  @param[in,out]  service     A pointer to the mutable network
+ *                              service for which to set the proxy
+ *                              method.
+ *  @param[in]      method      The network service proxy method to set
+ *                              on @a service.
+ *  @param[in]      donotifier  A Boolean indicating whether the proxy
+ *                              changed notifier chain should be run
+ *                              after @a method is set on @a service.
+ *  @param[in]      handler     An optional pointer to a handler that,
+ *                              if specified, will be invoked after @a
+ *                              method is set on @a service.
+ *  @param[in]      context     An optional pointer to immutable context
+ *                              that will be passed to @a handler,
+ *                              along with @a service and @a method.
+ *
+ *  @sa __connman_notifier_proxy_changed
+ *  @sa proxy_changed
+ *
+ *  @private
+ *
+ */
 static void service_set_proxy_method(struct connman_service *service,
 			enum connman_service_proxy_method method,
 			bool donotifier,
-- 
2.45.0

[PATCH 11/15] service: Document 'service_set_proxy_method_auto_handler'.

[Grant Erickson]

Add documentation to the 'service_set_proxy_method_auto_handler' function.
---
 src/service.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/src/service.c b/src/service.c
index 3fd56188093d..b27828d98a42 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5697,6 +5697,36 @@ const char *connman_service_get_proxy_url(const struct connman_service *service)
 	return service->pac;
 }
 
+/**
+ *  @brief
+ *    A post-mutation handler for the service proxy method when the
+ *    method is #CONNMAN_SERVICE_PROXY_METHOD_AUTO.
+ *
+ *  @param[in,out]  service     A pointer to the mutable network
+ *                              service for which the proxy method was
+ *                              set.
+ *  @param[in]      method      The network service proxy method @a
+ *                              service was set to.
+ *  @param[in]      context     An pointer to immutable context
+ *                              that was passed, along with @a
+ *                              service, @a method, and this handler,
+ *                              to #service_set_proxy_method. For this
+ *                              handler, @a context is an optional
+ *                              null-terminated C string containing
+ *                              the service proxy auto-configuration
+ *                              (PAC) URL.
+ *
+ *  @returns
+ *    True if #service_set_proxy_method should continue with state
+ *    change notifications and processing proxy notifier changes after
+ *    the handler returns; otherwise, false.
+ *
+ *  @sa __connman_ipconfig_set_proxy_autoconfig
+ *  @sa service_set_proxy_method
+ *
+ *  @private
+ *
+ */
 static bool service_set_proxy_method_auto_handler(
 				struct connman_service *service,
 				enum connman_service_proxy_method method,
-- 
2.45.0

[PATCH 12/15] service: Document '__connman_service_set_proxy_autoconfig'.

[Grant Erickson]

Add documentation to the '__connman_service_set_proxy_autoconfig' function.
---
 src/service.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/src/service.c b/src/service.c
index b27828d98a42..0cfb516016b5 100644
--- a/src/service.c
+++ b/src/service.c
@@ -5748,6 +5748,23 @@ static bool service_set_proxy_method_auto_handler(
 	return true;
 }
 
+/**
+ *  @brief
+ *    Set the network service proxy method to # and proxy
+ *    auto-configuation (PAC) URL.
+ *
+ *  @param[in,out]  service  A pointer to the mutable network
+ *                           service for which to set the proxy
+ *                           method.
+ *  @param[in]  url          An optional pointer to the immutable
+ *                           null-terminated C string containing the
+ *                           proxy auto-configuration (PAC) URL.
+ *
+ *  @sa service_set_pac
+ *  @sa service_set_proxy_method
+ *  @sa service_set_proxy_method_auto_handler
+ *
+ */
 void __connman_service_set_proxy_autoconfig(struct connman_service *service,
 							const char *url)
 {
-- 
2.45.0

[PATCH 13/15] wispr: Add 'wispr_log_proxy_failure' function.

[Grant Erickson]

Logs the network service interface name, type, proxy
auto-configuration (PAC) URL, online check URL, and proxy failure
reason at the error level when a Wireless Internet Service Provider
roaming (WISPr) online reachability check proxy failure occurs.

Frequently, a spurious, misconfigured, or otherwise incorrect PAC URL
is the source of WISPr online reachability check failures. Because
POSIX error codes are themselves limited (for example, there is no
-EBADPROXY or -EPROXYLKUP codes) and their usage for WISPr failures is
similarly limited (for example, -EINVAL), insight into the actual
nature of an online reachability check failure is, by extension,
limited. Consequently, log messages generated from this function
provide crucial insight into triaging and resolving such failures.
---
 src/wispr.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/wispr.c b/src/wispr.c
index b1b0f337d3f8..60a3a011da58 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -1118,6 +1118,24 @@ static char *parse_proxy(const char *proxy)
 	return proxy_server;
 }
 
+static void wispr_log_proxy_failure(
+		struct connman_wispr_portal_context const *wp_context,
+		const char *reason)
+{
+	g_autofree char *interface = NULL;
+
+	interface = connman_service_get_interface(wp_context->service);
+
+	connman_error("%s with proxy auto-configuration (PAC) URL %s "
+			"for %s [ %s ] online check URL %s",
+			reason,
+			connman_service_get_proxy_url(wp_context->service),
+			interface,
+			__connman_service_type2string(
+				connman_service_get_type(wp_context->service)),
+			wp_context->status_url);
+}
+
 static void proxy_callback(const char *proxy, void *user_data)
 {
 	struct connman_wispr_portal_context *wp_context = user_data;
-- 
2.45.0

[PATCH 14/15] wispr: Leverage 'wispr_log_proxy_failure'.

[Grant Erickson]

Leverage 'wispr_log_proxy_failure' from 'proxy_callback' with the "No
valid proxy" reason and from 'wispr_portal_detect' with the "Failed to
lookup" reason on Wireless Internet Service Provider roaming (WISPr)
online reachability check proxy failures.
---
 src/wispr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/wispr.c b/src/wispr.c
index 60a3a011da58..10990c725986 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -1147,7 +1147,7 @@ static void proxy_callback(const char *proxy, void *user_data)
 		return;
 
 	if (!proxy) {
-		DBG("no valid proxy");
+		wispr_log_proxy_failure(wp_context, "No valid proxy");
 
 		wp_context->cb(wp_context->service,
 				wp_context->type,
@@ -1320,6 +1320,8 @@ static int wispr_portal_detect(struct connman_wispr_portal_context *wp_context,
 						proxy_callback, wp_context);
 
 		if (wp_context->proxy_token == 0) {
+			wispr_log_proxy_failure(wp_context, "Failed to lookup");
+
 			err = -EINVAL;
 			wispr_portal_context_unref(wp_context);
 		}
-- 
2.45.0

[PATCH 15/15] wispr: Document 'wispr_log_proxy_failure'.

[Grant Erickson]

Add documentation to the 'wispr_log_proxy_failure' function.
---
 src/wispr.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/wispr.c b/src/wispr.c
index 10990c725986..9b0e4b24a3db 100644
--- a/src/wispr.c
+++ b/src/wispr.c
@@ -1118,6 +1118,26 @@ static char *parse_proxy(const char *proxy)
 	return proxy_server;
 }
 
+/**
+ *  @brief
+ *    Log a Wireless Internet Service Provider roaming (WISPr) proxy
+ *    auto-configuration (PAC)-related online reachability check
+ *    failure.
+ *
+ *  @param[in]  wp_context  A pointer to the immutable WISPr/portal
+ *                          context for which to log the proxy
+ *                          auto-configuration (PAC)-related online
+ *                          reachability check failure.
+ *  @param[in]  reason      A pointer to an immutable, null-terminated
+ *                          C string containing the reason for the
+ *                          proxy auto-configuration (PAC)-related
+ *                          online reachability check failure which
+ *                          will be included at the start of the log
+ *                          message.
+ *
+ *  @private
+ *
+ */
 static void wispr_log_proxy_failure(
 		struct connman_wispr_portal_context const *wp_context,
 		const char *reason)
-- 
2.45.0

Re: [PATCH 03/15] service: Leverage 'service_set_pac' function.

[Denis Kenzior]

Hi Grant,

On 2/21/25 9:56 AM, Grant Erickson wrote:
> Leverage 'service_set_pac' at the three call sites the proxy
> auto-configuration (PAC) URL is currently set or cleared for a network
> service.
> 
> Frequently, a spurious, misconfigured, or otherwise incorrect PAC URL
> is the source of connectivity problems, particularly for online
> reachability checks. Consequently, being able to log all such
> mutations can be helpful in triaging such problems.
> ---
>   src/service.c | 17 ++++-------------
>   1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/src/service.c b/src/service.c
> index c48959a7940f..da851d681bc7 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -770,8 +770,7 @@ static int service_load(struct connman_service *service)
>   	str = g_key_file_get_string(keyfile,
>   				service->identifier, "Proxy.URL", NULL);
>   	if (str) {
> -		g_free(service->pac);
> -		service->pac = str;
> +		service_set_pac(service, str, false);

Does this leak str?

Also, Kernel Coding Style prefers that braces are not used for single statement 
blocks.  Quoting:
"Do not unnecessarily use braces where a single statement will do.

if (condition)
         action();
"

See https://www.kernel.org/doc/html/latest/process/coding-style.html

>   	}
>   
>   	service->mdns_config = g_key_file_get_boolean(keyfile,

Regards,
-Denis

Re: [PATCH 08/15] service: Leverage 'service_set_proxy_method'.

[Denis Kenzior]

Hi Grant,

On 2/21/25 9:56 AM, Grant Erickson wrote:
> Leverage 'service_set_proxy_method' for
> 'connman_service_set_proxy_method', the first of two call sites.

Could this commit description be clearer?  E.g. mention 
connman_service_set_proxy_method() instead of 'first of two call sites'?

> ---
>   src/service.c | 19 +++++++------------
>   1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/src/service.c b/src/service.c
> index e58958635992..5c94c2962de8 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -5617,19 +5617,14 @@ static void service_set_proxy_method(struct connman_service *service,
>   void connman_service_set_proxy_method(struct connman_service *service,
>   					enum connman_service_proxy_method method)
>   {
> -	DBG("service %p (%s) method %d (%s)",
> -		service, connman_service_get_identifier(service),
> -		method, proxymethod2string(method));
> -
> -	if (!service || service->hidden)
> -		return;
> -
> -	service->proxy = method;
> +	const bool donotifier = method != CONNMAN_SERVICE_PROXY_METHOD_AUTO;
> +	void * const context = NULL;

What's the point to this variable?

>   
> -	proxy_changed(service);
> -
> -	if (method != CONNMAN_SERVICE_PROXY_METHOD_AUTO)
> -		__connman_notifier_proxy_changed(service);
> +	service_set_proxy_method(service,
> +		method,
> +		donotifier,
> +		NULL,
> +		context);

Just use NULL here which is clearer?

>   }
>   
>   enum connman_service_proxy_method connman_service_get_proxy_method(

Regards,
-Denis

Re: [PATCH 00/15 RESEND] Improve PAC URL Mutation and PAC URL and WISPr Proxy Failure Logging

[patchwork-bot+connman]

Hello:

This series was applied to connman.git (master)
by Denis Kenzior <denkenz@gmail.com>:

On Fri, 21 Feb 2025 07:56:38 -0800 you wrote:
> There appear to be a population of premises ISP equipment (some
> possibly correlated with Spectrum Communications in Southern
> California, United States) in which the ISP equipment sporadically and
> incorrectly emits DHCP Option 252 ("Web Proxy Auto-Discovery (WPAD)")
> with an invalid proxy auto-configuration (PAC) URL (such as
> 'http://wpad.lan/wpad.dat'), even when neither the customer nor the
> ISP has such an option actively configured on the equipment.
> 
> [...]

Here is the summary with links:
  - [01/15] service: Add 'service_log_pac' function.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=b0ea957e1973
  - [02/15] service: Add 'service_set_pac' function.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=298a1a8c2806
  - [03/15] service: Leverage 'service_set_pac' function.
    (no matching commit)
  - [04/15] service: Ensure the PAC URL is set in '__connman_service_set_proxy_autoconfig'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=a73f053de112
  - [05/15] service: Document 'service_log_pac'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=8f281ed058b0
  - [06/15] service: Document 'service_set_pac'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=34855234cc6d
  - [07/15] service: Add 'service_set_proxy_method' function.
    (no matching commit)
  - [08/15] service: Leverage 'service_set_proxy_method'.
    (no matching commit)
  - [09/15] service: Leverage 'service_set_proxy_method'.
    (no matching commit)
  - [10/15] service: Document 'service_set_proxy_method'.
    (no matching commit)
  - [11/15] service: Document 'service_set_proxy_method_auto_handler'.
    (no matching commit)
  - [12/15] service: Document '__connman_service_set_proxy_autoconfig'.
    (no matching commit)
  - [13/15] wispr: Add 'wispr_log_proxy_failure' function.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=58c9eb5ab1f9
  - [14/15] wispr: Leverage 'wispr_log_proxy_failure'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=0f26b82712c1
  - [15/15] wispr: Document 'wispr_log_proxy_failure'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=81d651b269f9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 09/15] service: Leverage 'service_set_proxy_method'.

[Denis Kenzior]

Hi Grant,

On 2/21/25 9:56 AM, Grant Erickson wrote:
> Leverage 'service_set_proxy_method' for
> '__connman_service_set_proxy_autoconfig', the second of two call
> sites.
> ---
>   src/service.c | 49 ++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/src/service.c b/src/service.c
> index 5c94c2962de8..d2d8e7cea065 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -5662,32 +5662,47 @@ const char *connman_service_get_proxy_url(const struct connman_service *service)
>   	return service->pac;
>   }
>   
> -void __connman_service_set_proxy_autoconfig(struct connman_service *service,
> -							const char *url)
> +static bool service_set_proxy_method_auto_handler(
> +				struct connman_service *service,
> +				enum connman_service_proxy_method method,
> +				const void *context)
>   {
> -	const bool dochanged = true;
> -
> -	if (!service || service->hidden)
> -		return;
> -
> -	service_set_pac(service, url, !dochanged);
> -
> -	service->proxy = CONNMAN_SERVICE_PROXY_METHOD_AUTO;
> +	const char * const url = context;
>   
>   	if (service->ipconfig_ipv4) {
>   		if (__connman_ipconfig_set_proxy_autoconfig(
> -			    service->ipconfig_ipv4, url) < 0)
> -			return;
> +				service->ipconfig_ipv4, url) < 0)
> +			return false;
>   	} else if (service->ipconfig_ipv6) {
>   		if (__connman_ipconfig_set_proxy_autoconfig(
> -			    service->ipconfig_ipv6, url) < 0)
> -			return;
> +				service->ipconfig_ipv6, url) < 0)
> +			return false;
>   	} else
> -		return;
> +		return false;
>   
> -	proxy_changed(service);
> +	return true;
> +}
> +
> +void __connman_service_set_proxy_autoconfig(struct connman_service *service,
> +							const char *url)
> +{
> +	const bool dochanged = true;
> +	const bool donotifier = true;
> +	const void *context = url;

Why are these needed?

>   
> -	__connman_notifier_proxy_changed(service);
> +	DBG("service %p (%s) url %p (%s)",
> +		service,
> +		connman_service_get_identifier(service),
> +		url,
> +		url ? url : "<null>");
> +
> +	service_set_pac(service, url, !dochanged);
> +
> +	service_set_proxy_method(service,
> +		CONNMAN_SERVICE_PROXY_METHOD_AUTO,
> +		donotifier,
> +		service_set_proxy_method_auto_handler,
> +		context);

Conversion from any pointer type to void * is automatic in C; using 'url' would 
be just fine here?

>   }
>   
>   const char *connman_service_get_proxy_autoconfig(struct connman_service *service)

Regards,
-Denis

Re: [PATCH 09/15] service: Leverage 'service_set_proxy_method'.

[Grant Erickson]

On Mar 4, 2025, at 9:12 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> On 2/21/25 9:56 AM, Grant Erickson wrote:
>> +void __connman_service_set_proxy_autoconfig(struct connman_service *service,
>> + const char *url)
>> +{
>> + const bool dochanged = true;
>> + const bool donotifier = true;
>> + const void *context = url;
> 
> Why are these needed?

As always, I have a preference / habit of making code self-documenting wherever possible. So, rather than seeing “true, true” in the parameter list and then having to run through the code to figure out what those are / mean, it’s self-documented.

The ‘url’ one was not strictly necessary but it was “parallel construction” with the other two.

>>  - __connman_notifier_proxy_changed(service);
>> + DBG("service %p (%s) url %p (%s)",
>> + service,
>> + connman_service_get_identifier(service),
>> + url,
>> + url ? url : "<null>");
>> +
>> + service_set_pac(service, url, !dochanged);
>> +
>> + service_set_proxy_method(service,
>> + CONNMAN_SERVICE_PROXY_METHOD_AUTO,
>> + donotifier,
>> + service_set_proxy_method_auto_handler,
>> + context);
> 
> Conversion from any pointer type to void * is automatic in C; using 'url' would be just fine here?

Confirmed; see above.

Best,

Grant

-- 
Principal
Nuovations

gerickson@nuovations.com
https://www.nuovations.com/

Re: [PATCH 08/15] service: Leverage 'service_set_proxy_method'.

[Grant Erickson]

On Mar 4, 2025, at 9:09 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> On 2/21/25 9:56 AM, Grant Erickson wrote:
>> Leverage 'service_set_proxy_method' for
>> 'connman_service_set_proxy_method', the first of two call sites.
> 
> Could this commit description be clearer?  E.g. mention connman_service_set_proxy_method() instead of 'first of two call sites'?

Will revise.

>> ---
>>  src/service.c | 19 +++++++------------
>>  1 file changed, 7 insertions(+), 12 deletions(-)
>> diff --git a/src/service.c b/src/service.c
>> index e58958635992..5c94c2962de8 100644
>> --- a/src/service.c
>> +++ b/src/service.c
>> @@ -5617,19 +5617,14 @@ static void service_set_proxy_method(struct connman_service *service,
>>  void connman_service_set_proxy_method(struct connman_service *service,
>>   enum connman_service_proxy_method method)
>>  {
>> - DBG("service %p (%s) method %d (%s)",
>> - service, connman_service_get_identifier(service),
>> - method, proxymethod2string(method));
>> -
>> - if (!service || service->hidden)
>> - return;
>> -
>> - service->proxy = method;
>> + const bool donotifier = method != CONNMAN_SERVICE_PROXY_METHOD_AUTO;
>> + void * const context = NULL;
> 
> What's the point to this variable?

As always, I have a preference / habit of making code self-documenting wherever possible. So, rather than seeing “true, NULL” in the parameter list and then having to run through the code to figure out what those are / mean, it’s self-documented.

>>  - proxy_changed(service);
>> -
>> - if (method != CONNMAN_SERVICE_PROXY_METHOD_AUTO)
>> - __connman_notifier_proxy_changed(service);
>> + service_set_proxy_method(service,
>> + method,
>> + donotifier,
>> + NULL,
>> + context);
> 
> Just use NULL here which is clearer?

Will do.

Best,

Grant

-- 
Principal
Nuovations

gerickson@nuovations.com
https://www.nuovations.com/

Re: [PATCH 03/15] service: Leverage 'service_set_pac' function.

[Grant Erickson]

On Mar 4, 2025, at 9:06 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> On 2/21/25 9:56 AM, Grant Erickson wrote:
>> Leverage 'service_set_pac' at the three call sites the proxy
>> auto-configuration (PAC) URL is currently set or cleared for a network
>> service.
>> Frequently, a spurious, misconfigured, or otherwise incorrect PAC URL
>> is the source of connectivity problems, particularly for online
>> reachability checks. Consequently, being able to log all such
>> mutations can be helpful in triaging such problems.
>> ---
>>  src/service.c | 17 ++++-------------
>>  1 file changed, 4 insertions(+), 13 deletions(-)
>> diff --git a/src/service.c b/src/service.c
>> index c48959a7940f..da851d681bc7 100644
>> --- a/src/service.c
>> +++ b/src/service.c
>> @@ -770,8 +770,7 @@ static int service_load(struct connman_service *service)
>>   str = g_key_file_get_string(keyfile,
>>   service->identifier, "Proxy.URL", NULL);
>>   if (str) {
>> - g_free(service->pac);
>> - service->pac = str;
>> + service_set_pac(service, str, false);
> 
> Does this leak str?

I do not believe so, no. service_set_pac now handles the same:

    g_free(service->pac);

    if (pac && strlen(pac) > 0)
        service->pac = g_strstrip(g_strdup(pac));
    else
        service->pac = NULL;

except now instead of being individually duplicated and scattered around to several call sites, it is all handled in one with service_set_pac.

> Also, Kernel Coding Style prefers that braces are not used for single statement blocks.  Quoting:
> "Do not unnecessarily use braces where a single statement will do.
> 
> if (condition)
>        action();
> "
> 
> See https://www.kernel.org/doc/html/latest/process/coding-style.html

I will address this.

Thanks,

Grant

-- 
Principal
Nuovations

gerickson@nuovations.com
https://www.nuovations.com/

Re: [PATCH 03/15] service: Leverage 'service_set_pac' function.

[Denis Kenzior]

Hi Grant,

<snip>

 >>> @@ -770,8 +770,7 @@ static int service_load(struct connman_service *service)
>>>    str = g_key_file_get_string(keyfile,
>>>    service->identifier, "Proxy.URL", NULL);
>>>    if (str) {
>>> - g_free(service->pac);
>>> - service->pac = str;
>>> + service_set_pac(service, str, false);
>>
>> Does this leak str?
> 
> I do not believe so, no. service_set_pac now handles the same:
> 
>      g_free(service->pac);
> 
>      if (pac && strlen(pac) > 0)
>          service->pac = g_strstrip(g_strdup(pac));
>      else
>          service->pac = NULL;
> 
> except now instead of being individually duplicated and scattered around to several call sites, it is all handled in one with service_set_pac.

service_set_pac receives a const char *.  But g_key_file_get_string() returns a 
newly allocated char *, which is assigned to 'str'.  What frees it?

[1] 
https://www.manpagez.com/html/glib/glib-2.42.0/glib-Key-value-file-parser.php#g-key-file-get-string

Regards,
-Denis

Re: [PATCH 00/15 RESEND] Improve PAC URL Mutation and PAC URL and WISPr Proxy Failure Logging

[patchwork-bot+connman]

Hello:

This series was applied to connman.git (master)
by Denis Kenzior <denkenz@gmail.com>:

On Fri, 21 Feb 2025 07:56:38 -0800 you wrote:
> There appear to be a population of premises ISP equipment (some
> possibly correlated with Spectrum Communications in Southern
> California, United States) in which the ISP equipment sporadically and
> incorrectly emits DHCP Option 252 ("Web Proxy Auto-Discovery (WPAD)")
> with an invalid proxy auto-configuration (PAC) URL (such as
> 'http://wpad.lan/wpad.dat'), even when neither the customer nor the
> ISP has such an option actively configured on the equipment.
> 
> [...]

Here is the summary with links:
  - [01/15] service: Add 'service_log_pac' function.
    (no matching commit)
  - [02/15] service: Add 'service_set_pac' function.
    (no matching commit)
  - [03/15] service: Leverage 'service_set_pac' function.
    (no matching commit)
  - [04/15] service: Ensure the PAC URL is set in '__connman_service_set_proxy_autoconfig'.
    (no matching commit)
  - [05/15] service: Document 'service_log_pac'.
    (no matching commit)
  - [06/15] service: Document 'service_set_pac'.
    (no matching commit)
  - [07/15] service: Add 'service_set_proxy_method' function.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=d8fcf6cc0c54
  - [08/15] service: Leverage 'service_set_proxy_method'.
    (no matching commit)
  - [09/15] service: Leverage 'service_set_proxy_method'.
    (no matching commit)
  - [10/15] service: Document 'service_set_proxy_method'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=25e6914c6e46
  - [11/15] service: Document 'service_set_proxy_method_auto_handler'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=1123f398de9f
  - [12/15] service: Document '__connman_service_set_proxy_autoconfig'.
    https://git.kernel.org/pub/scm/network/connman/connman.git/?id=288b28bd2c57
  - [13/15] wispr: Add 'wispr_log_proxy_failure' function.
    (no matching commit)
  - [14/15] wispr: Leverage 'wispr_log_proxy_failure'.
    (no matching commit)
  - [15/15] wispr: Document 'wispr_log_proxy_failure'.
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html