main: Use static global rather than 'General' string literal.

Commit Message

Grant Erickson Dec. 21, 2023, 7:05 a.m. UTC

This converts a potential run time error due to a mis-spelling of the
"General" configuration group name repeatedly used a string literal
into a potential compile time error due to a mis-spelling of the
static global that now references it once and only once.
---
 src/main.c | 85 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 36 deletions(-)

Comments

Denis Kenzior Dec. 22, 2023, 6:18 p.m. UTC | #1

Hi Grant,

>   
> +/*
> + * This is declared as 'const char *const' to effect an immutable
> + * pointer to an immutable null-terminated character string such that
> + * it ends 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 its use within
> + * this file as it sees fit.
> + */

I think you already had a similar comment in src/gateway.c.  Since this is 
something that makes sense to make into the accepted (and encouraged) pattern, 
would it make sense to document this in doc/coding-style.txt and avoid 
replicating this verbiage across the project?

> +static const char *const general_group_name = "General";

Given doc/coding-style.txt, item M3, should this be 'static const char * const'?

Also, since you're using this somewhat like a #defined constant, should this be 
capitalized for clarity?

Regards,
-Denis

Grant Erickson Dec. 22, 2023, 6:48 p.m. UTC | #2

On Dec 22, 2023, at 10:18 AM, Denis Kenzior <[e-mail]> wrote:
>>  +/*
>> + * This is declared as 'const char *const' to effect an immutable
>> + * pointer to an immutable null-terminated character string such that
>> + * it ends 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 its use within
>> + * this file as it sees fit.
>> + */
> 
> I think you already had a similar comment in src/gateway.c.

Confirmed; I had added that per Marcel’s feedback.

>  Since this is something that makes sense to make into the accepted (and encouraged) pattern, would it make sense to document this in doc/coding-style.txt and avoid replicating this verbiage across the project?

I think so. I’d probably phrase something to the effect of:

    If there are no preprocessor requirements (such as concatenation), string constants should be declared at the narrowest
    possible scope as declared as 'const char *const' to effect an immutable pointer to an immutable null-terminated character
    string such that it ends up in .text, not .data (which would otherwise be the case for a 'const char *’ declaration).

    Unless the scope is cross-module, they should also be declared with the ‘static' storage/scope qualifier such that the
    compiler can locally optimize its use within the scope as it sees fit.

    If there are preprocessor requirements (such as concatenation), string constants should be declared as preprocessor
    definitions.

>> +static const char *const general_group_name = "General";
> 
> Given doc/coding-style.txt, item M3, should this be 'static const char * const'?

Unfortunately, checkpatch.pl would be in conflict with M3 and insists they be declared as I did.

I run checkpatch.pl as:

    checkpatch.pl --max-line-length=80 --no-signoff --no-tree --ignore SPLIT_STRING,INITIALISED_STATIC

since that seems to embody most of the style deviations I see in connman relative to what checkpatch.pl expects by default for the kernel. Ideally, there’d be a local wrapper in the project for checkpatch that specifies the desired options that reflect the project’s preferred style. As I’ve mentioned previously, in the future, I’d like to see a script in the project that uses clang-format to both verify and enforce project syntactic style.

> Also, since you're using this somewhat like a #defined constant, should this be capitalized for clarity?

Personally, I like to reserve all capitalized symbol names for the preprocessor as a convention.

Best,

Grant

Denis Kenzior Dec. 22, 2023, 8:21 p.m. UTC | #3

Hi Grant,

>>   Since this is something that makes sense to make into the accepted (and encouraged) pattern, would it make sense to document this in doc/coding-style.txt and avoid replicating this verbiage across the project?
> 
> I think so. I’d probably phrase something to the effect of:
> 
>      If there are no preprocessor requirements (such as concatenation), string constants should be declared at the narrowest
>      possible scope as declared as 'const char *const' to effect an immutable pointer to an immutable null-terminated character
>      string such that it ends up in .text, not .data (which would otherwise be the case for a 'const char *’ declaration).
> 
>      Unless the scope is cross-module, they should also be declared with the ‘static' storage/scope qualifier such that the
>      compiler can locally optimize its use within the scope as it sees fit.
> 
>      If there are preprocessor requirements (such as concatenation), string constants should be declared as preprocessor
>      definitions.
> 

Yeah, that sounds reasonable to me.  I think this change should be added to 
connman/ell/iwd/ofono

>>> +static const char *const general_group_name = "General";
>>
>> Given doc/coding-style.txt, item M3, should this be 'static const char * const'?
> 
> Unfortunately, checkpatch.pl would be in conflict with M3 and insists they be declared as I did.
> 

Hmm, that seems counter to what the linux kernel seems to prefer by a 8x margin:

[denkenz@archdev linux]$ grep -R 'const char \*const' * | wc -l
1595
[denkenz@archdev linux]$ grep -R 'const char \* const' * | wc -l
13650

> I run checkpatch.pl as:
> 
>      checkpatch.pl --max-line-length=80 --no-signoff --no-tree --ignore SPLIT_STRING,INITIALISED_STATIC

Which version of checkpatch are you running?  I see no warning when I run latest 
linux git version against such a diff:

diff --git a/src/main.c b/src/main.c
index 3ce8340fc90b..c41668a6de35 100644
--- a/src/main.c
+++ b/src/main.c
@@ -54,6 +54,8 @@

  #include "src/backtrace.h"

+static const char * const foobar = "sdf2434";
+

> 
> since that seems to embody most of the style deviations I see in connman relative to what checkpatch.pl expects by default for the kernel. Ideally, there’d be a local wrapper in the project for checkpatch that specifies the desired options that reflect the project’s preferred style. As I’ve mentioned previously, in the future, I’d like to see a script in the project that uses clang-format to both verify and enforce project syntactic style.

Yes, agreed.  I was hoping to play with CI over the holidays.

> 
>> Also, since you're using this somewhat like a #defined constant, should this be capitalized for clarity?
> 
> Personally, I like to reserve all capitalized symbol names for the preprocessor as a convention.

Okay, no strong feeling here.  Linux coding style doesn't really say much except:
"Names of macros defining constants and labels in enums are capitalized."

Having constants capitalized is a pretty general pattern though, and in ConnMan 
as well.  For example, gdchp/common.h:

static const uint8_t MAC_BCAST_ADDR[ETH_ALEN] __attribute__((aligned(2))) = {

static const uint8_t MAC_ANY_ADDR[ETH_ALEN] __attribute__((aligned(2))) = {

iwd did something pretty similar to what you're doing in this patch and 
approached it this way:
https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/knownnetworks.c#n69

Regards,
-Denis

Patch

diff --git a/src/main.c b/src/main.c
index 241c713d7980..c01a9e59d11c 100644
--- a/src/main.c
+++ b/src/main.c
@@ -69,6 +69,16 @@ 
 #define MAINFILE "main.conf"
 #define CONFIGMAINFILE CONFIGDIR "/" MAINFILE
 
+/*
+ * This is declared as 'const char *const' to effect an immutable
+ * pointer to an immutable null-terminated character string such that
+ * it ends 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 its use within
+ * this file as it sees fit.
+ */
+static const char *const general_group_name = "General";
+
 static char *default_auto_connect[] = {
 	"wifi",
 	"ethernet",
@@ -321,14 +331,14 @@  static void check_config(GKeyFile *config)
 	keys = g_key_file_get_groups(config, NULL);
 
 	for (j = 0; keys && keys[j]; j++) {
-		if (g_strcmp0(keys[j], "General") != 0)
+		if (g_strcmp0(keys[j], general_group_name) != 0)
 			connman_warn("Unknown group %s in %s",
 						keys[j], MAINFILE);
 	}
 
 	g_strfreev(keys);
 
-	keys = g_key_file_get_keys(config, "General", NULL, NULL);
+	keys = g_key_file_get_keys(config, general_group_name, NULL, NULL);
 
 	for (j = 0; keys && keys[j]; j++) {
 		bool found;
@@ -450,21 +460,22 @@  static void parse_config(GKeyFile *config)
 
 	DBG("parsing %s", MAINFILE);
 
-	boolean = g_key_file_get_boolean(config, "General",
+	boolean = g_key_file_get_boolean(config, general_group_name,
 						CONF_BG_SCAN, &error);
 	if (!error)
 		connman_settings.bg_scan = boolean;
 
 	g_clear_error(&error);
 
-	timeservers = __connman_config_get_string_list(config, "General",
+	timeservers = __connman_config_get_string_list(config,
+					general_group_name,
 					CONF_PREF_TIMESERVERS, NULL, &error);
 	if (!error)
 		connman_settings.pref_timeservers = timeservers;
 
 	g_clear_error(&error);
 
-	str_list = __connman_config_get_string_list(config, "General",
+	str_list = __connman_config_get_string_list(config, general_group_name,
 			CONF_AUTO_CONNECT_TECHS, &len, &error);
 
 	if (!error)
@@ -478,7 +489,7 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	str_list = __connman_config_get_string_list(config, "General",
+	str_list = __connman_config_get_string_list(config, general_group_name,
 			CONF_FAVORITE_TECHS, &len, &error);
 
 	if (!error)
@@ -492,7 +503,7 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	str_list = __connman_config_get_string_list(config, "General",
+	str_list = __connman_config_get_string_list(config, general_group_name,
 			CONF_PREFERRED_TECHS, &len, &error);
 
 	if (!error)
@@ -503,7 +514,7 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	str_list = __connman_config_get_string_list(config, "General",
+	str_list = __connman_config_get_string_list(config, general_group_name,
 			CONF_ALWAYS_CONNECTED_TECHS, &len, &error);
 
 	if (!error)
@@ -514,7 +525,7 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	str_list = __connman_config_get_string_list(config, "General",
+	str_list = __connman_config_get_string_list(config, general_group_name,
 			CONF_FALLBACK_NAMESERVERS, &len, &error);
 
 	if (!error)
@@ -525,21 +536,22 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	integer = g_key_file_get_integer(config, "General",
+	integer = g_key_file_get_integer(config, general_group_name,
 			CONF_TIMEOUT_INPUTREQ, &error);
 	if (!error && integer >= 0)
 		connman_settings.timeout_inputreq = integer * 1000;
 
 	g_clear_error(&error);
 
-	integer = g_key_file_get_integer(config, "General",
+	integer = g_key_file_get_integer(config, general_group_name,
 			CONF_TIMEOUT_BROWSERLAUNCH, &error);
 	if (!error && integer >= 0)
 		connman_settings.timeout_browserlaunch = integer * 1000;
 
 	g_clear_error(&error);
 
-	interfaces = __connman_config_get_string_list(config, "General",
+	interfaces = __connman_config_get_string_list(config,
+			general_group_name,
 			CONF_BLACKLISTED_INTERFACES, &len, &error);
 
 	if (!error)
@@ -550,7 +562,7 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	boolean = __connman_config_get_bool(config, "General",
+	boolean = __connman_config_get_bool(config, general_group_name,
 					CONF_ALLOW_HOSTNAME_UPDATES,
 					&error);
 	if (!error)
@@ -558,7 +570,7 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	boolean = __connman_config_get_bool(config, "General",
+	boolean = __connman_config_get_bool(config, general_group_name,
 					CONF_ALLOW_DOMAINNAME_UPDATES,
 					&error);
 	if (!error)
@@ -566,14 +578,14 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	boolean = __connman_config_get_bool(config, "General",
+	boolean = __connman_config_get_bool(config, general_group_name,
 			CONF_SINGLE_TECH, &error);
 	if (!error)
 		connman_settings.single_tech = boolean;
 
 	g_clear_error(&error);
 
-	tethering = __connman_config_get_string_list(config, "General",
+	tethering = __connman_config_get_string_list(config, general_group_name,
 			CONF_TETHERING_TECHNOLOGIES, &len, &error);
 
 	if (!error)
@@ -581,7 +593,7 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	boolean = __connman_config_get_bool(config, "General",
+	boolean = __connman_config_get_bool(config, general_group_name,
 					CONF_PERSISTENT_TETHERING_MODE,
 					&error);
 	if (!error)
@@ -589,14 +601,14 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	boolean = __connman_config_get_bool(config, "General",
+	boolean = __connman_config_get_bool(config, general_group_name,
 					CONF_ENABLE_6TO4, &error);
 	if (!error)
 		connman_settings.enable_6to4 = boolean;
 
 	g_clear_error(&error);
 
-	string = __connman_config_get_string(config, "General",
+	string = __connman_config_get_string(config, general_group_name,
 					CONF_VENDOR_CLASS_ID, &error);
 	if (!error)
 		connman_settings.vendor_class_id = string;
@@ -605,7 +617,7 @@  static void parse_config(GKeyFile *config)
 
 	/* EnableOnlineCheck */
 
-	boolean = __connman_config_get_bool(config, "General",
+	boolean = __connman_config_get_bool(config, general_group_name,
 					CONF_ENABLE_ONLINE_CHECK, &error);
 	if (!error) {
 		connman_warn("\"%s\" is deprecated; use \"%s\" instead.",
@@ -619,7 +631,7 @@  static void parse_config(GKeyFile *config)
 
 	/* EnableOnlineToReadyTransition */
 
-	boolean = __connman_config_get_bool(config, "General",
+	boolean = __connman_config_get_bool(config, general_group_name,
 			CONF_ENABLE_ONLINE_TO_READY_TRANSITION, &error);
 	if (!error) {
 		connman_warn("\"%s\" is deprecated; use \"%s\" instead.",
@@ -633,7 +645,7 @@  static void parse_config(GKeyFile *config)
 
 	/* OnlineCheckMode */
 
-	string = __connman_config_get_string(config, "General",
+	string = __connman_config_get_string(config, general_group_name,
 				CONF_ONLINE_CHECK_MODE, &error);
 	if (!error) {
 		connman_settings.online_check_mode =
@@ -653,7 +665,7 @@  static void parse_config(GKeyFile *config)
 
 	/* OnlineCheckConnectTimeout */
 
-	real = g_key_file_get_double(config, "General",
+	real = g_key_file_get_double(config, general_group_name,
 			CONF_ONLINE_CHECK_CONNECT_TIMEOUT, &error);
 	if (!error) {
 		if (real < 0) {
@@ -670,7 +682,7 @@  static void parse_config(GKeyFile *config)
 
 	/* OnlineCheckIPv4URL */
 
-	string = __connman_config_get_string(config, "General",
+	string = __connman_config_get_string(config, general_group_name,
 					CONF_ONLINE_CHECK_IPV4_URL, &error);
 	if (!error)
 		connman_settings.online_check_ipv4_url = string;
@@ -682,7 +694,7 @@  static void parse_config(GKeyFile *config)
 
 	/* OnlineCheckIPv6URL */
 
-	string = __connman_config_get_string(config, "General",
+	string = __connman_config_get_string(config, general_group_name,
 					CONF_ONLINE_CHECK_IPV6_URL, &error);
 	if (!error)
 		connman_settings.online_check_ipv6_url = string;
@@ -694,14 +706,14 @@  static void parse_config(GKeyFile *config)
 
 	/* OnlineCheck{Initial,Max}Interval */
 
-	integer = g_key_file_get_integer(config, "General",
+	integer = g_key_file_get_integer(config, general_group_name,
 			CONF_ONLINE_CHECK_INITIAL_INTERVAL, &error);
 	if (!error && integer >= 0)
 		connman_settings.online_check_initial_interval = integer;
 
 	g_clear_error(&error);
 
-	integer = g_key_file_get_integer(config, "General",
+	integer = g_key_file_get_integer(config, general_group_name,
 			CONF_ONLINE_CHECK_MAX_INTERVAL, &error);
 	if (!error && integer >= 0)
 		connman_settings.online_check_max_interval = integer;
@@ -722,7 +734,7 @@  static void parse_config(GKeyFile *config)
 
 	/* OnlineCheckFailuresThreshold */
 
-	integer = g_key_file_get_integer(config, "General",
+	integer = g_key_file_get_integer(config, general_group_name,
 			CONF_ONLINE_CHECK_FAILURES_THRESHOLD, &error);
 	if (!error && integer >= 0)
 		connman_settings.online_check_failures_threshold = integer;
@@ -738,7 +750,7 @@  static void parse_config(GKeyFile *config)
 
 	/* OnlineCheckSuccessesThreshold */
 
-	integer = g_key_file_get_integer(config, "General",
+	integer = g_key_file_get_integer(config, general_group_name,
 			CONF_ONLINE_CHECK_SUCCESSES_THRESHOLD, &error);
 	if (!error && integer >= 0)
 		connman_settings.online_check_successes_threshold = integer;
@@ -754,7 +766,7 @@  static void parse_config(GKeyFile *config)
 
 	/* OnlineCheckIntervalStyle */
 
-	string = __connman_config_get_string(config, "General",
+	string = __connman_config_get_string(config, general_group_name,
 					CONF_ONLINE_CHECK_INTERVAL_STYLE, &error);
 	if (!error) {
 		if ((g_strcmp0(string, ONLINE_CHECK_INTERVAL_STYLE_FIBONACCI) == 0) ||
@@ -772,27 +784,28 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	boolean = __connman_config_get_bool(config, "General",
+	boolean = __connman_config_get_bool(config, general_group_name,
 				CONF_AUTO_CONNECT_ROAMING_SERVICES, &error);
 	if (!error)
 		connman_settings.auto_connect_roaming_services = boolean;
 
 	g_clear_error(&error);
 
-	boolean = __connman_config_get_bool(config, "General", CONF_ACD, &error);
+	boolean = __connman_config_get_bool(config, general_group_name,
+				CONF_ACD, &error);
 	if (!error)
 		connman_settings.acd = boolean;
 
 	g_clear_error(&error);
 
-	boolean = __connman_config_get_bool(config, "General",
+	boolean = __connman_config_get_bool(config, general_group_name,
 				CONF_USE_GATEWAYS_AS_TIMESERVERS, &error);
 	if (!error)
 		connman_settings.use_gateways_as_timeservers = boolean;
 
 	g_clear_error(&error);
 
-	string = __connman_config_get_string(config, "General",
+	string = __connman_config_get_string(config, general_group_name,
 				CONF_LOCALTIME, &error);
 	if (!error)
 		connman_settings.localtime = string;
@@ -801,14 +814,14 @@  static void parse_config(GKeyFile *config)
 
 	g_clear_error(&error);
 
-	boolean = __connman_config_get_bool(config, "General",
+	boolean = __connman_config_get_bool(config, general_group_name,
 				CONF_REGDOM_FOLLOWS_TIMEZONE, &error);
 	if (!error)
 		connman_settings.regdom_follows_timezone = boolean;
 
 	g_clear_error(&error);
 
-	string = __connman_config_get_string(config, "General",
+	string = __connman_config_get_string(config, general_group_name,
 				CONF_RESOLV_CONF, &error);
 	if (!error)
 		connman_settings.resolv_conf = string;