| Message ID | 20231129015253.1254116-2-gerickson@nuovations.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | connection: Introduce and Leverage IP Any / Unspecified Address String Constants and Functions | expand |
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
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
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
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
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
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);