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

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

[Grant Erickson]

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(-)

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;
-- 
2.42.0

Re: [PATCH] main: Use static global rather than 'General' string literal.

[Marcel Holtmann]

Hi Grant,

> 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(-)
> 
> 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";
> +

wouldn’t be

	#define GENERAL_GROUP “General”

or

	#define GROUP_GENERAL “General”

be actually more consistent with the other constants we use here.

I do like the all upper-case to indicate a constant here (and scrapping
the “name” portion).

Otherwise, yes, it is stupid from us to keep using “General” in all
places :(

Regards

Marcel

Re: [PATCH] main: Use static global rather than 'General' string literal.

[Grant Erickson]

On Dec 27, 2023, at 3:48 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> 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(-)
> 
> wouldn’t be
> 
> 	#define GENERAL_GROUP “General”
> 
> or
> 
> 	#define GROUP_GENERAL “General”
> 
> be actually more consistent with the other constants we use here.

There are definitely an appreciable number of existing preprocessor definitions in this file. So, with that in mind, a preprocessor definition would be stylistically more consistent.

> I do like the all upper-case to indicate a constant here (and scrapping
> the “name” portion).
> 
> Otherwise, yes, it is stupid from us to keep using “General” in all
> places :(

At -O2 optimization, all three approaches are neutral from a code-generation perspective. So, the question comes down to one of style:

   text	   data	    bss	    dec	    hex	filename
   9817	    756	    264	  10837	   2a55	connmand-main.o // "General" string literals
   9817	    756	    264	  10837	   2a55	connmand-main.o // "General" static const char * const global
   9817	    756	    264	  10837	   2a55	connmand-main.o // "General" preprocessor macro

I’d be happy to re-submit a v2 patch with the preprocessor definition.

Best,

Grant

-- 
Principal
Nuovations

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

Re: [PATCH] main: Use static global rather than 'General' string literal.

[Marcel Holtmann]

Hi Grant,

>>> 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(-)
>> 
>> wouldn’t be
>> 
>> #define GENERAL_GROUP “General”
>> 
>> or
>> 
>> #define GROUP_GENERAL “General”
>> 
>> be actually more consistent with the other constants we use here.
> 
> There are definitely an appreciable number of existing preprocessor definitions in this file. So, with that in mind, a preprocessor definition would be stylistically more consistent.
> 
>> I do like the all upper-case to indicate a constant here (and scrapping
>> the “name” portion).
>> 
>> Otherwise, yes, it is stupid from us to keep using “General” in all
>> places :(
> 
> At -O2 optimization, all three approaches are neutral from a code-generation perspective. So, the question comes down to one of style:
> 
>   text   data    bss    dec    hex filename
>   9817    756    264  10837   2a55 connmand-main.o // "General" string literals
>   9817    756    264  10837   2a55 connmand-main.o // "General" static const char * const global
>   9817    756    264  10837   2a55 connmand-main.o // "General" preprocessor macro
> 
> I’d be happy to re-submit a v2 patch with the preprocessor definition.

then lets use the preprocessor macro. We defiantly have used that more
than the global string.

Regards

Marcel

Re: [PATCH] main: Use static global rather than 'General' string literal.

[Grant Erickson]

On Dec 28, 2023, at 1:54 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>>>> 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(-)
>>> 
>>> wouldn’t be
>>> 
>>> #define GENERAL_GROUP “General”
>>> 
>>> or
>>> 
>>> #define GROUP_GENERAL “General”
>>> 
>>> be actually more consistent with the other constants we use here.
>> 
>> There are definitely an appreciable number of existing preprocessor definitions in this file. So, with that in mind, a preprocessor definition would be stylistically more consistent.
>> 
>>> I do like the all upper-case to indicate a constant here (and scrapping
>>> the “name” portion).
>>> 
>>> Otherwise, yes, it is stupid from us to keep using “General” in all
>>> places :(
>> 
>> At -O2 optimization, all three approaches are neutral from a code-generation perspective. So, the question comes down to one of style:
>> 
>> [ … ]
>> 
>> I’d be happy to re-submit a v2 patch with the preprocessor definition.
> 
> then lets use the preprocessor macro. We defiantly have used that more
> than the global string.

Marcel,

Submitted as v2.

Best,

Grant

-- 
Principal
Nuovations

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

Re: [PATCH] main: Use static global rather than 'General' string literal.

[Denis Kenzior]

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

Re: [PATCH] main: Use static global rather than 'General' string literal.

[Grant Erickson]

On Dec 22, 2023, at 10:18 AM, Denis Kenzior <denkenz@gmail.com> 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

-- 
Principal
Nuovations

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

Re: [PATCH] main: Use static global rather than 'General' string literal.

[Denis Kenzior]

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

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

[Grant Erickson]

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(-)

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;
-- 
2.42.0