diff mbox series

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

Message ID 20231223185801.2389526-1-gerickson@nuovations.com (mailing list archive)
State Not Applicable, archived
Headers show
Series main: Use static global rather than 'General' string literal. | expand

Commit Message

Grant Erickson Dec. 23, 2023, 6:58 p.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

Marcel Holtmann Dec. 27, 2023, 11:48 a.m. UTC | #1 
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
Grant Erickson Dec. 27, 2023, 10:28 p.m. UTC | #2 
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
Marcel Holtmann Dec. 28, 2023, 9:54 a.m. UTC | #3 
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
Grant Erickson Dec. 28, 2023, 4:39 p.m. UTC | #4 
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
diff mbox series

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;