[PATCH 0/3] connection: Introduce and Leverage IP Any / Unspecified Address String Constants and Functions

[PATCH 0/3] connection: Introduce and Leverage IP Any / Unspecified Address String Constants and Functions

[Grant Erickson]

The IPv4 and IPv6 any / unspecified address string literals, "0.0.0.0"
and "::" respectively, and comparisons against them appear often
enough throughout the code where they warrant referencing through
file-scoped constants and introducing and leveraging introspection
functions to encapsulate the comparison patterns.

This introduces and uses such constants and 'is_ipv[46]_addr_any_str'
introspection functions to encapsulate these comparison patterns.

Grant Erickson (3):
  connection: Introduce and leverage 'ipv[46]_addr_any_str' constants.
  connection: Introduce and leverage 'is_ipv[46]_addr_any_str'
    functions.
  connection: Document 'is_ipv[46]_addr_any_str'.

 src/connection.c | 84 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 19 deletions(-)

-- 
2.42.0

[PATCH 1/3] connection: Introduce and leverage 'ipv[46]_addr_any_str' constants.

[Grant Erickson]

The IPv4 and IPv6 any / unspecified address string literals, "0.0.0.0"
and "::" respectively, appear often enough throughout the code where
they warrant referencing through file-scoped constants.
---
 src/connection.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index 3749d3a59196..66a3bd656db0 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -57,6 +57,9 @@ struct gateway_data {
 	bool default_checked;
 };
 
+static const char *const ipv4_addr_any_str = "0.0.0.0";
+static const char *const ipv6_addr_any_str = "::";
+
 static GHashTable *gateway_hash = NULL;
 
 /**
@@ -486,7 +489,7 @@ static void set_vpn_routes(struct gateway_data *new_gateway,
 		DBG("active gw %s", active_gateway->ipv4_config->gateway);
 
 		if (g_strcmp0(active_gateway->ipv4_config->gateway,
-							"0.0.0.0") != 0)
+							ipv4_addr_any_str) != 0)
 			dest = active_gateway->ipv4_config->gateway;
 		else
 			dest = NULL;
@@ -506,7 +509,7 @@ static void set_vpn_routes(struct gateway_data *new_gateway,
 		DBG("active gw %s", active_gateway->ipv6_config->gateway);
 
 		if (g_strcmp0(active_gateway->ipv6_config->gateway,
-								"::") != 0)
+					ipv6_addr_any_str) != 0)
 			dest = active_gateway->ipv6_config->gateway;
 		else
 			dest = NULL;
@@ -538,7 +541,7 @@ static int del_routes(struct gateway_data *data,
 						data->ipv4_config->vpn_ip);
 
 		} else if (g_strcmp0(data->ipv4_config->gateway,
-							"0.0.0.0") == 0) {
+						ipv4_addr_any_str) == 0) {
 			status4 = connman_inet_clear_gateway_interface(
 								data->index);
 		} else {
@@ -556,7 +559,8 @@ static int del_routes(struct gateway_data *data,
 						data->index,
 						data->ipv6_config->vpn_ip);
 
-		} else if (g_strcmp0(data->ipv6_config->gateway, "::") == 0) {
+		} else if (g_strcmp0(data->ipv6_config->gateway,
+						ipv6_addr_any_str) == 0) {
 			status6 = connman_inet_clear_ipv6_gateway_interface(
 								data->index);
 		} else {
@@ -716,7 +720,7 @@ static void set_default_gateway(struct gateway_data *data,
 
 	if (do_ipv4 && data->ipv4_config &&
 			g_strcmp0(data->ipv4_config->gateway,
-							"0.0.0.0") == 0) {
+					ipv4_addr_any_str) == 0) {
 		if (connman_inet_set_gateway_interface(index) < 0)
 			return;
 		data->ipv4_config->active = true;
@@ -725,7 +729,7 @@ static void set_default_gateway(struct gateway_data *data,
 
 	if (do_ipv6 && data->ipv6_config &&
 			g_strcmp0(data->ipv6_config->gateway,
-							"::") == 0) {
+					ipv6_addr_any_str) == 0) {
 		if (connman_inet_set_ipv6_gateway_interface(index) < 0)
 			return;
 		data->ipv6_config->active = true;
@@ -798,7 +802,7 @@ static void unset_default_gateway(struct gateway_data *data,
 
 	if (do_ipv4 && data->ipv4_config &&
 			g_strcmp0(data->ipv4_config->gateway,
-							"0.0.0.0") == 0) {
+					ipv4_addr_any_str) == 0) {
 		connman_inet_clear_gateway_interface(index);
 		data->ipv4_config->active = false;
 		return;
@@ -806,7 +810,7 @@ static void unset_default_gateway(struct gateway_data *data,
 
 	if (do_ipv6 && data->ipv6_config &&
 			g_strcmp0(data->ipv6_config->gateway,
-							"::") == 0) {
+					ipv6_addr_any_str) == 0) {
 		connman_inet_clear_ipv6_gateway_interface(index);
 		data->ipv6_config->active = false;
 		return;
@@ -1066,7 +1070,7 @@ static void add_host_route(int family, int index, const char *gateway,
 {
 	switch (family) {
 	case AF_INET:
-		if (g_strcmp0(gateway, "0.0.0.0") != 0) {
+		if (g_strcmp0(gateway, ipv4_addr_any_str) != 0) {
 			/*
 			 * We must not set route to the phy dev gateway in
 			 * VPN link. The packets to VPN link might be routed
@@ -1091,7 +1095,7 @@ static void add_host_route(int family, int index, const char *gateway,
 		break;
 
 	case AF_INET6:
-		if (g_strcmp0(gateway, "::") != 0) {
+		if (g_strcmp0(gateway, ipv6_addr_any_str) != 0) {
 			if (service_type != CONNMAN_SERVICE_TYPE_VPN)
 				connman_inet_add_ipv6_host_route(index,
 								gateway, NULL);
@@ -1176,10 +1180,10 @@ int __connman_connection_gateway_add(struct connman_service *service,
 	 * interface
 	 */
 	if (!gateway && type == CONNMAN_IPCONFIG_TYPE_IPV4)
-		gateway = "0.0.0.0";
+		gateway = ipv4_addr_any_str;
 
 	if (!gateway && type == CONNMAN_IPCONFIG_TYPE_IPV6)
-		gateway = "::";
+		gateway = ipv6_addr_any_str;
 
 	DBG("service %p index %d gateway %s vpn ip %s type %d",
 		service, index, gateway, peer, type);
-- 
2.42.0

[PATCH 2/3] connection: Introduce and leverage 'is_ipv[46]_addr_any_str' functions.

[Grant Erickson]

Comparisons against the IPv4 and IPv6 any / unspecified address string
constants, "0.0.0.0" and "::" respectively, appear often enough
throughout the code where they warrant introducing and leveraging
introspection functions to encapsulate the comparison patterns.

This introduces and leverages 'is_ipv[46]_addr_any_str' introspection
functions to encapsulate these comparison patterns.
---
 src/connection.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index 66a3bd656db0..9fb2298dde61 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -192,6 +192,16 @@ static void gateway_data_debug(const char *function,
 	}
 }
 
+static bool is_ipv4_addr_any_str(const char *address)
+{
+	return g_strcmp0(ipv4_addr_any_str, address) == 0;
+}
+
+static bool is_ipv6_addr_any_str(const char *address)
+{
+	return g_strcmp0(ipv6_addr_any_str, address) == 0;
+}
+
 /**
  *  @brief
  *    Find the gateway, or default router, configuration associated
@@ -488,8 +498,7 @@ static void set_vpn_routes(struct gateway_data *new_gateway,
 
 		DBG("active gw %s", active_gateway->ipv4_config->gateway);
 
-		if (g_strcmp0(active_gateway->ipv4_config->gateway,
-							ipv4_addr_any_str) != 0)
+		if (!is_ipv4_addr_any_str(active_gateway->ipv4_config->gateway))
 			dest = active_gateway->ipv4_config->gateway;
 		else
 			dest = NULL;
@@ -508,8 +517,7 @@ static void set_vpn_routes(struct gateway_data *new_gateway,
 
 		DBG("active gw %s", active_gateway->ipv6_config->gateway);
 
-		if (g_strcmp0(active_gateway->ipv6_config->gateway,
-					ipv6_addr_any_str) != 0)
+		if (!is_ipv6_addr_any_str(active_gateway->ipv6_config->gateway))
 			dest = active_gateway->ipv6_config->gateway;
 		else
 			dest = NULL;
@@ -540,8 +548,7 @@ static int del_routes(struct gateway_data *data,
 						data->index,
 						data->ipv4_config->vpn_ip);
 
-		} else if (g_strcmp0(data->ipv4_config->gateway,
-						ipv4_addr_any_str) == 0) {
+		} else if (is_ipv4_addr_any_str(data->ipv4_config->gateway)) {
 			status4 = connman_inet_clear_gateway_interface(
 								data->index);
 		} else {
@@ -559,8 +566,7 @@ static int del_routes(struct gateway_data *data,
 						data->index,
 						data->ipv6_config->vpn_ip);
 
-		} else if (g_strcmp0(data->ipv6_config->gateway,
-						ipv6_addr_any_str) == 0) {
+		} else if (is_ipv6_addr_any_str(data->ipv6_config->gateway)) {
 			status6 = connman_inet_clear_ipv6_gateway_interface(
 								data->index);
 		} else {
@@ -719,8 +725,7 @@ static void set_default_gateway(struct gateway_data *data,
 	index = __connman_service_get_index(data->service);
 
 	if (do_ipv4 && data->ipv4_config &&
-			g_strcmp0(data->ipv4_config->gateway,
-					ipv4_addr_any_str) == 0) {
+			is_ipv4_addr_any_str(data->ipv4_config->gateway)) {
 		if (connman_inet_set_gateway_interface(index) < 0)
 			return;
 		data->ipv4_config->active = true;
@@ -728,8 +733,7 @@ static void set_default_gateway(struct gateway_data *data,
 	}
 
 	if (do_ipv6 && data->ipv6_config &&
-			g_strcmp0(data->ipv6_config->gateway,
-					ipv6_addr_any_str) == 0) {
+			is_ipv6_addr_any_str(data->ipv6_config->gateway)) {
 		if (connman_inet_set_ipv6_gateway_interface(index) < 0)
 			return;
 		data->ipv6_config->active = true;
@@ -801,16 +805,14 @@ static void unset_default_gateway(struct gateway_data *data,
 	index = __connman_service_get_index(data->service);
 
 	if (do_ipv4 && data->ipv4_config &&
-			g_strcmp0(data->ipv4_config->gateway,
-					ipv4_addr_any_str) == 0) {
+			is_ipv4_addr_any_str(data->ipv4_config->gateway)) {
 		connman_inet_clear_gateway_interface(index);
 		data->ipv4_config->active = false;
 		return;
 	}
 
 	if (do_ipv6 && data->ipv6_config &&
-			g_strcmp0(data->ipv6_config->gateway,
-					ipv6_addr_any_str) == 0) {
+			is_ipv6_addr_any_str(data->ipv6_config->gateway)) {
 		connman_inet_clear_ipv6_gateway_interface(index);
 		data->ipv6_config->active = false;
 		return;
@@ -1070,7 +1072,7 @@ static void add_host_route(int family, int index, const char *gateway,
 {
 	switch (family) {
 	case AF_INET:
-		if (g_strcmp0(gateway, ipv4_addr_any_str) != 0) {
+		if (!is_ipv4_addr_any_str(gateway)) {
 			/*
 			 * We must not set route to the phy dev gateway in
 			 * VPN link. The packets to VPN link might be routed
@@ -1095,7 +1097,7 @@ static void add_host_route(int family, int index, const char *gateway,
 		break;
 
 	case AF_INET6:
-		if (g_strcmp0(gateway, ipv6_addr_any_str) != 0) {
+		if (!is_ipv6_addr_any_str(gateway)) {
 			if (service_type != CONNMAN_SERVICE_TYPE_VPN)
 				connman_inet_add_ipv6_host_route(index,
 								gateway, NULL);
-- 
2.42.0

[PATCH 3/3] connection: Document 'is_ipv[46]_addr_any_str'.

[Grant Erickson]

This adds documentation to the 'is_ipv[46]_addr_any_str' functions.
---
 src/connection.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/src/connection.c b/src/connection.c
index 9fb2298dde61..eddfdc721636 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -192,11 +192,51 @@ static void gateway_data_debug(const char *function,
 	}
 }
 
+/**
+ *  @brief
+ *    Determine whether the specified text-formatted IPv4 address is
+ *    the "any" or "unspecified" address.
+ *
+ *  This determines whether the specified text-formatted IPv4 address
+ *  is the "any" or "unspecified" address, that is "0.0.0.0".
+ *
+ *  @param[in]  address  A pointer to an immutable null-terminated C
+ *                       string containing the text-formatted address
+ *                       to determine whether it is the IPv4 "any" or
+ *                       "unspecified address.
+ *
+ *  @returns
+ *    True if @a address is the "any" or "unspecified" IPv4 address;
+ *    otherwise, false.
+ *
+ *  @sa is_ipv6_addr_any_str
+ *
+ */
 static bool is_ipv4_addr_any_str(const char *address)
 {
 	return g_strcmp0(ipv4_addr_any_str, address) == 0;
 }
 
+/**
+ *  @brief
+ *    Determine whether the specified text-formatted IPv6 address is
+ *    the "any" or "unspecified" address.
+ *
+ *  This determines whether the specified text-formatted IPv6 address
+ *  is the "any" or "unspecified" address, that is "::".
+ *
+ *  @param[in]  address  A pointer to an immutable null-terminated C
+ *                       string containing the text-formatted address
+ *                       to determine whether it is the IPv6 "any" or
+ *                       "unspecified address.
+ *
+ *  @returns
+ *    True if @a address is the "any" or "unspecified" IPv6 address;
+ *    otherwise, false.
+ *
+ *  @sa is_ipv4_addr_any_str
+ *
+ */
 static bool is_ipv6_addr_any_str(const char *address)
 {
 	return g_strcmp0(ipv6_addr_any_str, address) == 0;
-- 
2.42.0

Re: [PATCH 1/3] connection: Introduce and leverage 'ipv[46]_addr_any_str' constants.

[Marcel Holtmann]

Hi Grant,

> The IPv4 and IPv6 any / unspecified address string literals, "0.0.0.0"
> and "::" respectively, appear often enough throughout the code where
> they warrant referencing through file-scoped constants.
> ---
> src/connection.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/src/connection.c b/src/connection.c
> index 3749d3a59196..66a3bd656db0 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -57,6 +57,9 @@ struct gateway_data {
> bool default_checked;
> };
> 
> +static const char *const ipv4_addr_any_str = "0.0.0.0";
> +static const char *const ipv6_addr_any_str = "::";
> +

explain to me the double const. And why a variable and not just a #define.

Regards

Marcel

Re: [PATCH 1/3] connection: Introduce and leverage 'ipv[46]_addr_any_str' constants.

[Grant Erickson]

On Nov 29, 2023, at 5:19 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> The IPv4 and IPv6 any / unspecified address string literals, "0.0.0.0"
>> and "::" respectively, appear often enough throughout the code where
>> they warrant referencing through file-scoped constants.
>> ---
>> src/connection.c | 28 ++++++++++++++++------------
>> 1 file changed, 16 insertions(+), 12 deletions(-)
>> 
>> diff --git a/src/connection.c b/src/connection.c
>> index 3749d3a59196..66a3bd656db0 100644
>> --- a/src/connection.c
>> +++ b/src/connection.c
>> @@ -57,6 +57,9 @@ struct gateway_data {
>> bool default_checked;
>> };
>> 
>> +static const char *const ipv4_addr_any_str = "0.0.0.0";
>> +static const char *const ipv6_addr_any_str = "::";
>> +
> 
> explain to me the double const. And why a variable and not just a #define.

Were we to declare these as:

    static const char *ipv4_addr_any_str = "0.0.0.0";
    static const char *ipv6_addr_any_str = "::";

This says “a mutable pointer to an immutable null-terminated character string”. Consequently, that means somewhere during program run time we can do:

    ipv4_addr_any_str = “ConnMan is the best network management program for Linux”;

and subvert the original intent and behavior of the code.

As a result of the pointer mutability, the compiler is forced to allocate space not only for the initial string value in .text but also, depending on the architecture, 4- or 8-bytes in .data in RAM such that the run time pointer assignment can be accommodated. So, there are a few consequences of this:

    1. This isn’t really what we intended, so we end up with a potential bug in which the constant we thought was constant really isn’t.
    2. We end up wasting space in .data and, as a result, in RAM for behavior we didn’t want or need.

By declaring them as in the patch, it says “an immutable pointer to an immutable null-terminated character string”. Because now both the pointer itself is immutable (and cannot be reassigned) and the contents it points to are immutable and, due to the ’static’ storage/scope qualifier, the compiler can optimize as it sees fit. No space in .data is consumed and no more space in .text is consumed (and potentially less) than had it been a preprocessor definition.

Does that help clarify?

Best,

Grant

-- 
Principal
Nuovations

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

Re: [PATCH 1/3] connection: Introduce and leverage 'ipv[46]_addr_any_str' constants.

[Marcel Holtmann]

Hi Grant,

>>> The IPv4 and IPv6 any / unspecified address string literals, "0.0.0.0"
>>> and "::" respectively, appear often enough throughout the code where
>>> they warrant referencing through file-scoped constants.
>>> ---
>>> src/connection.c | 28 ++++++++++++++++------------
>>> 1 file changed, 16 insertions(+), 12 deletions(-)
>>> 
>>> diff --git a/src/connection.c b/src/connection.c
>>> index 3749d3a59196..66a3bd656db0 100644
>>> --- a/src/connection.c
>>> +++ b/src/connection.c
>>> @@ -57,6 +57,9 @@ struct gateway_data {
>>> bool default_checked;
>>> };
>>> 
>>> +static const char *const ipv4_addr_any_str = "0.0.0.0";
>>> +static const char *const ipv6_addr_any_str = "::";
>>> +
>> 
>> explain to me the double const. And why a variable and not just a #define.
> 
> Were we to declare these as:
> 
>    static const char *ipv4_addr_any_str = "0.0.0.0";
>    static const char *ipv6_addr_any_str = "::";
> 
> This says “a mutable pointer to an immutable null-terminated character string”. Consequently, that means somewhere during program run time we can do:
> 
>    ipv4_addr_any_str = “ConnMan is the best network management program for Linux”;
> 
> and subvert the original intent and behavior of the code.
> 
> As a result of the pointer mutability, the compiler is forced to allocate space not only for the initial string value in .text but also, depending on the architecture, 4- or 8-bytes in .data in RAM such that the run time pointer assignment can be accommodated. So, there are a few consequences of this:
> 
>    1. This isn’t really what we intended, so we end up with a potential bug in which the constant we thought was constant really isn’t.
>    2. We end up wasting space in .data and, as a result, in RAM for behavior we didn’t want or need.
> 
> By declaring them as in the patch, it says “an immutable pointer to an immutable null-terminated character string”. Because now both the pointer itself is immutable (and cannot be reassigned) and the contents it points to are immutable and, due to the ’static’ storage/scope qualifier, the compiler can optimize as it sees fit. No space in .data is consumed and no more space in .text is consumed (and potentially less) than had it been a preprocessor definition.
> 
> Does that help clarify?

fair enough. Can you add a small comment above to remind people that this is done on purpose. Maybe like this:

/* Declare these as an immutable pointer to an immutable
 * null-terminated character string.
 */
static const char *const ipv4_addr_any_str = "0.0.0.0";

Regards

Marcel

Re: [PATCH 1/3] connection: Introduce and leverage 'ipv[46]_addr_any_str' constants.

[Grant Erickson]

On Nov 29, 2023, at 9:58 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>>>> The IPv4 and IPv6 any / unspecified address string literals, "0.0.0.0"
>>>> and "::" respectively, appear often enough throughout the code where
>>>> they warrant referencing through file-scoped constants.
>>>> ---
>>>> src/connection.c | 28 ++++++++++++++++------------
>>>> 1 file changed, 16 insertions(+), 12 deletions(-)
>>>> 
>>>> diff --git a/src/connection.c b/src/connection.c
>>>> index 3749d3a59196..66a3bd656db0 100644
>>>> --- a/src/connection.c
>>>> +++ b/src/connection.c
>>>> @@ -57,6 +57,9 @@ struct gateway_data {
>>>> bool default_checked;
>>>> };
>>>> 
>>>> +static const char *const ipv4_addr_any_str = "0.0.0.0";
>>>> +static const char *const ipv6_addr_any_str = "::";
>>>> +
>>> 
>>> explain to me the double const. And why a variable and not just a #define.
>> 
>> […]
>> 
>> By declaring them as in the patch, it says “an immutable pointer to an immutable null-terminated character string”. Because now both the pointer itself is immutable (and cannot be reassigned) and the contents it points to are immutable and, due to the ’static’ storage/scope qualifier, the compiler can optimize as it sees fit. No space in .data is consumed and no more space in .text is consumed (and potentially less) than had it been a preprocessor definition.
>> 
>> Does that help clarify?
> 
> fair enough. Can you add a small comment above to remind people that this is done on purpose. Maybe like this:
> 
> /* Declare these as an immutable pointer to an immutable
> * null-terminated character string.
> */
> static const char *const ipv4_addr_any_str = "0.0.0.0";

Thanks; will do.

Best,

Grant

-- 
Principal
Nuovations

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

[PATCH v2 0/3] connection: Introduce and Leverage IP Any / Unspecified Address String Constants and Functions

[Grant Erickson]

The IPv4 and IPv6 any / unspecified address string literals, "0.0.0.0"
and "::" respectively, and comparisons against them appear often
enough throughout the code where they warrant referencing through
file-scoped constants and introducing and leveraging introspection
functions to encapsulate the comparison patterns.

This introduces and uses such constants and 'is_ipv[46]_addr_any_str'
introspection functions to encapsulate these comparison patterns.

Grant Erickson (3):
  connection: Introduce and leverage 'ipv[46]_addr_any_str' constants.
  connection: Introduce and leverage 'is_ipv[46]_addr_any_str'
    functions.
  connection: Document 'is_ipv[46]_addr_any_str'.

 src/connection.c | 92 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 73 insertions(+), 19 deletions(-)

-- 
2.42.0

[PATCH v2 1/3] connection: Introduce and leverage 'ipv[46]_addr_any_str' constants.

[Grant Erickson]

The IPv4 and IPv6 any / unspecified address string literals, "0.0.0.0"
and "::" respectively, appear often enough throughout the code where
they warrant referencing through file-scoped constants.
---
 src/connection.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index deab2c034dc7..77ca28934cf2 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -57,6 +57,17 @@ struct gateway_data {
 	bool default_checked;
 };
 
+/*
+ * These are declared as 'const char *const' to effect an immutable
+ * pointer to an immutable null-terminated character string such that
+ * they end up in .text, not .data (which would otherwise be the case
+ * for a 'const char *' declaration), and with the 'static'
+ * storage/scope qualifier, the compiler can optimize their use within
+ * this file as it sees fit.
+ */
+static const char *const ipv4_addr_any_str = "0.0.0.0";
+static const char *const ipv6_addr_any_str = "::";
+
 static GHashTable *gateway_hash = NULL;
 
 /**
@@ -486,7 +497,7 @@ static void set_vpn_routes(struct gateway_data *new_gateway,
 		DBG("active gw %s", active_gateway->ipv4_config->gateway);
 
 		if (g_strcmp0(active_gateway->ipv4_config->gateway,
-							"0.0.0.0") != 0)
+							ipv4_addr_any_str) != 0)
 			dest = active_gateway->ipv4_config->gateway;
 		else
 			dest = NULL;
@@ -506,7 +517,7 @@ static void set_vpn_routes(struct gateway_data *new_gateway,
 		DBG("active gw %s", active_gateway->ipv6_config->gateway);
 
 		if (g_strcmp0(active_gateway->ipv6_config->gateway,
-								"::") != 0)
+					ipv6_addr_any_str) != 0)
 			dest = active_gateway->ipv6_config->gateway;
 		else
 			dest = NULL;
@@ -538,7 +549,7 @@ static int del_routes(struct gateway_data *data,
 						data->ipv4_config->vpn_ip);
 
 		} else if (g_strcmp0(data->ipv4_config->gateway,
-							"0.0.0.0") == 0) {
+						ipv4_addr_any_str) == 0) {
 			status4 = connman_inet_clear_gateway_interface(
 								data->index);
 		} else {
@@ -556,7 +567,8 @@ static int del_routes(struct gateway_data *data,
 						data->index,
 						data->ipv6_config->vpn_ip);
 
-		} else if (g_strcmp0(data->ipv6_config->gateway, "::") == 0) {
+		} else if (g_strcmp0(data->ipv6_config->gateway,
+						ipv6_addr_any_str) == 0) {
 			status6 = connman_inet_clear_ipv6_gateway_interface(
 								data->index);
 		} else {
@@ -716,7 +728,7 @@ static void set_default_gateway(struct gateway_data *data,
 
 	if (do_ipv4 && data->ipv4_config &&
 			g_strcmp0(data->ipv4_config->gateway,
-							"0.0.0.0") == 0) {
+					ipv4_addr_any_str) == 0) {
 		if (connman_inet_set_gateway_interface(index) < 0)
 			return;
 		data->ipv4_config->active = true;
@@ -725,7 +737,7 @@ static void set_default_gateway(struct gateway_data *data,
 
 	if (do_ipv6 && data->ipv6_config &&
 			g_strcmp0(data->ipv6_config->gateway,
-							"::") == 0) {
+					ipv6_addr_any_str) == 0) {
 		if (connman_inet_set_ipv6_gateway_interface(index) < 0)
 			return;
 		data->ipv6_config->active = true;
@@ -798,7 +810,7 @@ static void unset_default_gateway(struct gateway_data *data,
 
 	if (do_ipv4 && data->ipv4_config &&
 			g_strcmp0(data->ipv4_config->gateway,
-							"0.0.0.0") == 0) {
+					ipv4_addr_any_str) == 0) {
 		connman_inet_clear_gateway_interface(index);
 		data->ipv4_config->active = false;
 		return;
@@ -806,7 +818,7 @@ static void unset_default_gateway(struct gateway_data *data,
 
 	if (do_ipv6 && data->ipv6_config &&
 			g_strcmp0(data->ipv6_config->gateway,
-							"::") == 0) {
+					ipv6_addr_any_str) == 0) {
 		connman_inet_clear_ipv6_gateway_interface(index);
 		data->ipv6_config->active = false;
 		return;
@@ -1066,7 +1078,7 @@ static void add_host_route(int family, int index, const char *gateway,
 {
 	switch (family) {
 	case AF_INET:
-		if (g_strcmp0(gateway, "0.0.0.0") != 0) {
+		if (g_strcmp0(gateway, ipv4_addr_any_str) != 0) {
 			/*
 			 * We must not set route to the phy dev gateway in
 			 * VPN link. The packets to VPN link might be routed
@@ -1091,7 +1103,7 @@ static void add_host_route(int family, int index, const char *gateway,
 		break;
 
 	case AF_INET6:
-		if (g_strcmp0(gateway, "::") != 0) {
+		if (g_strcmp0(gateway, ipv6_addr_any_str) != 0) {
 			if (service_type != CONNMAN_SERVICE_TYPE_VPN)
 				connman_inet_add_ipv6_host_route(index,
 								gateway, NULL);
@@ -1176,10 +1188,10 @@ int __connman_connection_gateway_add(struct connman_service *service,
 	 * interface
 	 */
 	if (!gateway && type == CONNMAN_IPCONFIG_TYPE_IPV4)
-		gateway = "0.0.0.0";
+		gateway = ipv4_addr_any_str;
 
 	if (!gateway && type == CONNMAN_IPCONFIG_TYPE_IPV6)
-		gateway = "::";
+		gateway = ipv6_addr_any_str;
 
 	DBG("service %p index %d gateway %s vpn ip %s type %d",
 		service, index, gateway, peer, type);
-- 
2.42.0

[PATCH v2 2/3] connection: Introduce and leverage 'is_ipv[46]_addr_any_str' functions.

[Grant Erickson]

Comparisons against the IPv4 and IPv6 any / unspecified address string
constants, "0.0.0.0" and "::" respectively, appear often enough
throughout the code where they warrant introducing and leveraging
introspection functions to encapsulate the comparison patterns.

This introduces and leverages 'is_ipv[46]_addr_any_str' introspection
functions to encapsulate these comparison patterns.
---
 src/connection.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/src/connection.c b/src/connection.c
index 77ca28934cf2..f7b5ca008d8a 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -200,6 +200,16 @@ static void gateway_data_debug(const char *function,
 	}
 }
 
+static bool is_ipv4_addr_any_str(const char *address)
+{
+	return g_strcmp0(ipv4_addr_any_str, address) == 0;
+}
+
+static bool is_ipv6_addr_any_str(const char *address)
+{
+	return g_strcmp0(ipv6_addr_any_str, address) == 0;
+}
+
 /**
  *  @brief
  *    Find the gateway, or default router, configuration associated
@@ -496,8 +506,7 @@ static void set_vpn_routes(struct gateway_data *new_gateway,
 
 		DBG("active gw %s", active_gateway->ipv4_config->gateway);
 
-		if (g_strcmp0(active_gateway->ipv4_config->gateway,
-							ipv4_addr_any_str) != 0)
+		if (!is_ipv4_addr_any_str(active_gateway->ipv4_config->gateway))
 			dest = active_gateway->ipv4_config->gateway;
 		else
 			dest = NULL;
@@ -516,8 +525,7 @@ static void set_vpn_routes(struct gateway_data *new_gateway,
 
 		DBG("active gw %s", active_gateway->ipv6_config->gateway);
 
-		if (g_strcmp0(active_gateway->ipv6_config->gateway,
-					ipv6_addr_any_str) != 0)
+		if (!is_ipv6_addr_any_str(active_gateway->ipv6_config->gateway))
 			dest = active_gateway->ipv6_config->gateway;
 		else
 			dest = NULL;
@@ -548,8 +556,7 @@ static int del_routes(struct gateway_data *data,
 						data->index,
 						data->ipv4_config->vpn_ip);
 
-		} else if (g_strcmp0(data->ipv4_config->gateway,
-						ipv4_addr_any_str) == 0) {
+		} else if (is_ipv4_addr_any_str(data->ipv4_config->gateway)) {
 			status4 = connman_inet_clear_gateway_interface(
 								data->index);
 		} else {
@@ -567,8 +574,7 @@ static int del_routes(struct gateway_data *data,
 						data->index,
 						data->ipv6_config->vpn_ip);
 
-		} else if (g_strcmp0(data->ipv6_config->gateway,
-						ipv6_addr_any_str) == 0) {
+		} else if (is_ipv6_addr_any_str(data->ipv6_config->gateway)) {
 			status6 = connman_inet_clear_ipv6_gateway_interface(
 								data->index);
 		} else {
@@ -727,8 +733,7 @@ static void set_default_gateway(struct gateway_data *data,
 	index = __connman_service_get_index(data->service);
 
 	if (do_ipv4 && data->ipv4_config &&
-			g_strcmp0(data->ipv4_config->gateway,
-					ipv4_addr_any_str) == 0) {
+			is_ipv4_addr_any_str(data->ipv4_config->gateway)) {
 		if (connman_inet_set_gateway_interface(index) < 0)
 			return;
 		data->ipv4_config->active = true;
@@ -736,8 +741,7 @@ static void set_default_gateway(struct gateway_data *data,
 	}
 
 	if (do_ipv6 && data->ipv6_config &&
-			g_strcmp0(data->ipv6_config->gateway,
-					ipv6_addr_any_str) == 0) {
+			is_ipv6_addr_any_str(data->ipv6_config->gateway)) {
 		if (connman_inet_set_ipv6_gateway_interface(index) < 0)
 			return;
 		data->ipv6_config->active = true;
@@ -809,16 +813,14 @@ static void unset_default_gateway(struct gateway_data *data,
 	index = __connman_service_get_index(data->service);
 
 	if (do_ipv4 && data->ipv4_config &&
-			g_strcmp0(data->ipv4_config->gateway,
-					ipv4_addr_any_str) == 0) {
+			is_ipv4_addr_any_str(data->ipv4_config->gateway)) {
 		connman_inet_clear_gateway_interface(index);
 		data->ipv4_config->active = false;
 		return;
 	}
 
 	if (do_ipv6 && data->ipv6_config &&
-			g_strcmp0(data->ipv6_config->gateway,
-					ipv6_addr_any_str) == 0) {
+			is_ipv6_addr_any_str(data->ipv6_config->gateway)) {
 		connman_inet_clear_ipv6_gateway_interface(index);
 		data->ipv6_config->active = false;
 		return;
@@ -1078,7 +1080,7 @@ static void add_host_route(int family, int index, const char *gateway,
 {
 	switch (family) {
 	case AF_INET:
-		if (g_strcmp0(gateway, ipv4_addr_any_str) != 0) {
+		if (!is_ipv4_addr_any_str(gateway)) {
 			/*
 			 * We must not set route to the phy dev gateway in
 			 * VPN link. The packets to VPN link might be routed
@@ -1103,7 +1105,7 @@ static void add_host_route(int family, int index, const char *gateway,
 		break;
 
 	case AF_INET6:
-		if (g_strcmp0(gateway, ipv6_addr_any_str) != 0) {
+		if (!is_ipv6_addr_any_str(gateway)) {
 			if (service_type != CONNMAN_SERVICE_TYPE_VPN)
 				connman_inet_add_ipv6_host_route(index,
 								gateway, NULL);
-- 
2.42.0

[PATCH v2 3/3] connection: Document 'is_ipv[46]_addr_any_str'.

[Grant Erickson]

This adds documentation to the 'is_ipv[46]_addr_any_str' functions.
---
 src/connection.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/src/connection.c b/src/connection.c
index f7b5ca008d8a..a1a962578e9f 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -200,11 +200,51 @@ static void gateway_data_debug(const char *function,
 	}
 }
 
+/**
+ *  @brief
+ *    Determine whether the specified text-formatted IPv4 address is
+ *    the "any" or "unspecified" address.
+ *
+ *  This determines whether the specified text-formatted IPv4 address
+ *  is the "any" or "unspecified" address, that is "0.0.0.0".
+ *
+ *  @param[in]  address  A pointer to an immutable null-terminated C
+ *                       string containing the text-formatted address
+ *                       to determine whether it is the IPv4 "any" or
+ *                       "unspecified address.
+ *
+ *  @returns
+ *    True if @a address is the "any" or "unspecified" IPv4 address;
+ *    otherwise, false.
+ *
+ *  @sa is_ipv6_addr_any_str
+ *
+ */
 static bool is_ipv4_addr_any_str(const char *address)
 {
 	return g_strcmp0(ipv4_addr_any_str, address) == 0;
 }
 
+/**
+ *  @brief
+ *    Determine whether the specified text-formatted IPv6 address is
+ *    the "any" or "unspecified" address.
+ *
+ *  This determines whether the specified text-formatted IPv6 address
+ *  is the "any" or "unspecified" address, that is "::".
+ *
+ *  @param[in]  address  A pointer to an immutable null-terminated C
+ *                       string containing the text-formatted address
+ *                       to determine whether it is the IPv6 "any" or
+ *                       "unspecified address.
+ *
+ *  @returns
+ *    True if @a address is the "any" or "unspecified" IPv6 address;
+ *    otherwise, false.
+ *
+ *  @sa is_ipv4_addr_any_str
+ *
+ */
 static bool is_ipv6_addr_any_str(const char *address)
 {
 	return g_strcmp0(ipv6_addr_any_str, address) == 0;
-- 
2.42.0

Re: [PATCH 1/3] connection: Introduce and leverage 'ipv[46]_addr_any_str' constants.

[Grant Erickson]

On Nov 29, 2023, at 9:58 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>>>> The IPv4 and IPv6 any / unspecified address string literals, "0.0.0.0"
>>>> and "::" respectively, appear often enough throughout the code where
>>>> they warrant referencing through file-scoped constants.
>>>> ---
>>> 
>>> [ … ]
>>> 
>>> explain to me the double const. And why a variable and not just a #define.
>> 
>> Does that help clarify?
> 
> fair enough. Can you add a small comment above to remind people that this is done on purpose. Maybe like this:
> 
> /* Declare these as an immutable pointer to an immutable
> * null-terminated character string.
> */
> static const char *const ipv4_addr_any_str = "0.0.0.0";

Resubmitted as v2.

Best,

Grant

-- 
Principal
Nuovations

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

Re: [PATCH v2 0/3] connection: Introduce and Leverage IP Any / Unspecified Address String Constants and Functions

[Marcel Holtmann]

Hi Grant,

> The IPv4 and IPv6 any / unspecified address string literals, "0.0.0.0"
> and "::" respectively, and comparisons against them appear often
> enough throughout the code where they warrant referencing through
> file-scoped constants and introducing and leveraging introspection
> functions to encapsulate the comparison patterns.
> 
> This introduces and uses such constants and 'is_ipv[46]_addr_any_str'
> introspection functions to encapsulate these comparison patterns.
> 
> Grant Erickson (3):
>  connection: Introduce and leverage 'ipv[46]_addr_any_str' constants.
>  connection: Introduce and leverage 'is_ipv[46]_addr_any_str'
>    functions.
>  connection: Document 'is_ipv[46]_addr_any_str'.
> 
> src/connection.c | 92 ++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 73 insertions(+), 19 deletions(-)

all 3 patches have been applied.

Regards

Marcel