[2/4] Add support for configuration files

Submitted by Veronika Kabatova on May 22, 2017, 3 p.m.

Details

Message ID 1495465217-25647-3-git-send-email-vkabatov@redhat.com
State New
Series "Implementation of configuration files"
Headers show

Commit Message

Veronika Kabatova May 22, 2017, 3 p.m.
From: Veronika Kabatova <vkabatov@redhat.com>

Implementation changes for usage of simple configuration files. Before
parsing the command line options, either default configuration files
(/etc/criu.d/default.conf, $HOME/.criu.d/default.conf; in this order) are
parsed, or a specific config file passed by the user. Two new options are
introduced: "--config FILEPATH" option allows users to specify a single
configuration file they want to use; and "--no-default-config" option to
forbid the parsing of default configuration files. Both options are to be
passed only via the command line.

Usage of configuration files is not mandatory to keep backwards
compatibility. The implementation of this feature tries to be compatible
with command line usage -- the user should get the same results whether
he passes the options (in the right order of parsing) on command line or
writes them in config files. This allows the user to:

1) Override boolean options if needed
2) Specify partial configuration for options that are possible to pass
   several times (e.g. "--external"), and pass the rest of the options
   based on process runtime by command line

Configuration file syntax allows comments marked with '#' sign, the rest
of the line after '#' is ignored. The user can use one option per line
(with argument supplied on the same line if needed, divided with whitespace
characters), the options are the same as long options (without the "--"
prefix used on command line).

Configuration file example (syntax purposes only, doesn't make sense):

$ cat ~/.criu.d/default.conf
tcp-established
work-dir /home/<USERNAME>/criu/work_directory
extra # inline comment
no-restore-sibling
tree	111111

Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
---
 criu/Makefile             |   7 ++
 criu/crtools.c            | 203 ++++++++++++++++++++++++++++++++++++++++++++--
 criu/include/cr_options.h |   7 ++
 3 files changed, 209 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/Makefile b/criu/Makefile
index b2c6633..8e1a803 100644
--- a/criu/Makefile
+++ b/criu/Makefile
@@ -15,6 +15,12 @@  ifeq ($(filter clean mrproper,$(MAKECMDGOALS)),)
 endif
 
 #
+# Configuration file paths
+CONFIG-DEFINES		+= -DGLOBAL_CONFIG_DIR='"/etc/criu.d/"'
+CONFIG-DEFINES		+= -DDEFAULT_CONFIG_FILENAME='"default.conf"'
+CONFIG-DEFINES		+= -DUSER_CONFIG_DIR='".criu.d/"'
+
+#
 # General flags.
 ccflags-y		+= -fno-strict-aliasing
 ccflags-y		+= -iquote criu/include
@@ -24,6 +30,7 @@  ccflags-y		+= -iquote $(ARCH_DIR)/include
 ccflags-y		+= -iquote .
 ccflags-y		+= -I/usr/include/libnl3
 ccflags-y		+= $(COMPEL_UAPI_INCLUDES)
+ccflags-y		+= $(CONFIG-DEFINES)
 
 export ccflags-y
 
diff --git a/criu/crtools.c b/criu/crtools.c
index ab8e1b5..c4c5ebd 100644
--- a/criu/crtools.c
+++ b/criu/crtools.c
@@ -55,6 +55,8 @@ 
 #include "../soccr/soccr.h"
 
 struct cr_options opts;
+char **first_conf = NULL;
+char **second_conf = NULL;
 
 void init_opts(void)
 {
@@ -208,21 +210,178 @@  bool deprecated_ok(char *what)
 	return false;
 }
 
-int main(int argc, char *argv[], char *envp[])
+bool is_default_config_forbidden(char *args[])
 {
+	int i = 0;
+	while (args[i] != NULL) {
+		if (!strncmp(args[i], "--no-default-config", strlen("--no-default-config")))
+			return true;
+		i++;
+	}
+	return false;
+}
 
-#define BOOL_OPT(OPT_NAME, SAVE_TO) \
-		{OPT_NAME, no_argument, SAVE_TO, true},\
-		{"no-" OPT_NAME, no_argument, SAVE_TO, false}
+char * specific_config_passed(char *args[])
+{
+	int i = 0;
+	while (args[i]) {
+		if (!strncmp(args[i], "--config", strlen("--config"))) {
+			/* getopt takes next string as required argument automatically */
+			return args[i + 1];
+		} else if (strstr(args[i], "--config=") != NULL) {
+			return args[i] + strlen("--config=");
+		}
+		i++;
+	}
+	return NULL;
+}
+
+void free_array(char **to_free)
+{
+	if (to_free == NULL) return;
+	int i = 0;
+	while (to_free[i] != NULL) {
+		free(to_free[i]);
+		i++;
+	}
+	free(to_free);
+	to_free = NULL;
+}
+
+void free_confs()
+{
+	free_array(first_conf);
+	free_array(second_conf);
+}
 
+static int count_elements(char **to_count)
+{
+		int count = 0;
+		if (to_count != NULL)
+			while (to_count[count] != NULL)
+				count++;
+		return count;
+}
+
+char ** parse_config(char *filepath, bool is_default)
+{
+	FILE* configfile = fopen(filepath, "r");
+	if (!configfile) {
+		/* Nonexistent default config file is *NOT* an error */
+		if (!is_default)
+			pr_err("Can't access configuration file %s.\n", filepath);
+		return NULL;
+	}
+
+	int size = 10;
+	int elements_read = 0, len = 0;
+	bool was_newline = true;
+	char **configuration = xmalloc(size * sizeof(char *));
+	if (configuration == NULL) {
+		fclose(configfile);
+		return NULL;
+	}
+	/* Initialize first element, getopt ignores it */
+	configuration[0] = xmalloc((strlen("criu") + 1) * sizeof(char));
+	if (configuration[0] == NULL) {
+		fclose(configfile);
+		return NULL;
+	}
+	memcpy(configuration[0], "criu\0", strlen("criu") + 1);
+
+	while (fscanf(configfile, "%ms", &configuration[elements_read + 1]) == 1) {
+		if (configuration[elements_read + 1][0] == '#') {
+			fscanf(configfile, "%*[^\n]");
+			was_newline = true;
+		} else {
+			if (was_newline) {
+				len = strlen(configuration[elements_read + 1]);
+				char *tmp = xrealloc(configuration[elements_read + 1], len + strlen("--") + 1);
+				if (tmp == NULL) {
+					free_array(configuration);
+					fclose(configfile);
+					return NULL;
+				}
+				memmove(tmp + strlen("--"), tmp, len + 1);
+				memmove(tmp, "--", strlen("--"));
+				configuration[elements_read + 1] = tmp;
+			}
+			elements_read++;
+			was_newline = fgetc(configfile) == '\n' ? true : false;
+		}
+		if (elements_read == size) {
+			size *= 2;
+			char **tmp_conf = xrealloc(configuration, size * sizeof(char *));
+			if (tmp_conf == NULL) {
+				free_array(configuration);
+				fclose(configfile);
+				return NULL;
+			}
+			configuration = tmp_conf;
+		}
+	}
+	configuration[elements_read + 1] = NULL;
+
+	fclose(configfile);
+	return configuration;
+}
+
+int main(int argc, char *argv[], char *envp[])
+{
 	pid_t pid = 0, tree_id = 0;
 	int ret = -1;
 	bool usage_error = true;
 	bool has_exec_cmd = false;
 	bool has_sub_command;
-	int opt, idx;
+	int opt = 0;
+	int idx;
+	int state = PARSING_FIRST;
 	int log_level = DEFAULT_LOGLEVEL;
 	char *imgs_dir = ".";
+
+	/* Check for --help / -h on commandline before parsing, otherwise
+	 * the help message won't be displayed if there is an error in
+	 * configuration file syntax. Checks are kept in parser in case of
+	 * option being put in the configuration file itself.
+	 */
+	int i;
+	for (i = 0; i < argc; i++) {
+		if ((!strncmp(argv[i], "--help", strlen("--help"))) ||
+				(!strncmp(argv[i], "-h", strlen("-h")))) {
+			usage_error = false;
+			goto usage;
+		}
+	}
+
+	if (atexit(free_confs)) {
+		pr_err("Unable to register cleanup function.\n");
+		return 1;
+	}
+	char *specific_conf = specific_config_passed(argv);
+	if ((specific_conf == NULL) && (!is_default_config_forbidden(argv))) {
+		first_conf = parse_config(GLOBAL_CONFIG_DIR DEFAULT_CONFIG_FILENAME, true);
+		char local_filepath[PATH_MAX + 1];
+		char *home_dir = getenv("HOME");
+		if (!home_dir) {
+			pr_info("Unable to get $HOME directory, local configuration file will not be used.");
+		} else {
+			snprintf(local_filepath, PATH_MAX, "%s/%s%s",
+					home_dir, USER_CONFIG_DIR, DEFAULT_CONFIG_FILENAME);
+			second_conf = parse_config(local_filepath, true);
+		}
+	} else if (specific_conf != NULL) {
+		first_conf = parse_config(specific_conf, false);
+		if (first_conf == NULL) {
+			return 1;
+		}
+	}
+	int first_count = count_elements(first_conf);
+	int second_count = count_elements(second_conf);
+
+#define BOOL_OPT(OPT_NAME, SAVE_TO) \
+		{OPT_NAME, no_argument, SAVE_TO, true},\
+		{"no-" OPT_NAME, no_argument, SAVE_TO, false}
+
 	static const char short_opts[] = "dSsRf:F:t:p:hcD:o:v::x::Vr:jJ:lW:L:M:";
 	static struct option long_opts[] = {
 		{ "tree",			required_argument,	0, 't'	},
@@ -295,6 +454,8 @@  int main(int argc, char *argv[], char *envp[])
 		{ "status-fd",			required_argument,	0, 1088 },
 		BOOL_OPT("remote", &opts.remote),
 		{ "verbosity",			optional_argument,	0, 'v'	},
+		{ "config",			required_argument,	0, 1089},
+		{ "no-default-config",		no_argument,		0, 1090},
 		{ },
 	};
 
@@ -333,9 +494,30 @@  int main(int argc, char *argv[], char *envp[])
 
 	while (1) {
 		idx = -1;
-		opt = getopt_long(argc, argv, short_opts, long_opts, &idx);
-		if (opt == -1)
+
+		switch (state) {
+		case PARSING_FIRST:
+			opt = getopt_long(first_count, first_conf, short_opts, long_opts, &idx);
+			break;
+		case PARSING_SECOND:
+			opt = getopt_long(second_count, second_conf, short_opts, long_opts, &idx);
+			break;
+		case PARSING_ARGV:
+			opt = getopt_long(argc, argv, short_opts, long_opts, &idx);
+			break;
+		default:
 			break;
+		}
+
+		if (opt == -1) {
+			if (state < PARSING_ARGV) {
+				state++;
+				optind = 0;
+				continue;
+			} else {
+				break;
+			}
+		}
 		if (!opt)
 			continue;
 
@@ -429,7 +611,6 @@  int main(int argc, char *argv[], char *envp[])
 		case 1049:
 			if (add_script(optarg))
 				return 1;
-
 			break;
 		case 1051:
 			opts.addr = optarg;
@@ -576,6 +757,12 @@  int main(int argc, char *argv[], char *envp[])
 				return 1;
 			}
 			break;
+		case 1089:
+			pr_info("Using configuration file %s\n.", optarg);
+			break;
+		case 1090:
+			pr_info("Default configuration files disabled.\n");
+			break;
 		case 'V':
 			pr_msg("Version: %s\n", CRIU_VERSION);
 			if (strcmp(CRIU_GITID, "0"))
diff --git a/criu/include/cr_options.h b/criu/include/cr_options.h
index 94fc717..f56c064 100644
--- a/criu/include/cr_options.h
+++ b/criu/include/cr_options.h
@@ -128,4 +128,11 @@  extern struct cr_options opts;
 
 extern void init_opts(void);
 
+/*
+ * Option parsing state constants (marking max. 2 configuration files and argv)
+ */
+#define PARSING_FIRST	1
+#define PARSING_SECOND	2
+#define PARSING_ARGV	3
+
 #endif /* __CR_OPTIONS_H__ */

Comments

Dmitry Safonov May 22, 2017, 6:23 p.m.
Hi Veronika,

I like this stuff, but this looks like it wants some cleanups.
Please, check comments below.

2017-05-22 18:00 GMT+03:00  <vkabatov@redhat.com>:
> From: Veronika Kabatova <vkabatov@redhat.com>
>
> Implementation changes for usage of simple configuration files. Before
> parsing the command line options, either default configuration files
> (/etc/criu.d/default.conf, $HOME/.criu.d/default.conf; in this order) are
> parsed, or a specific config file passed by the user. Two new options are
> introduced: "--config FILEPATH" option allows users to specify a single
> configuration file they want to use; and "--no-default-config" option to
> forbid the parsing of default configuration files. Both options are to be
> passed only via the command line.
>
> Usage of configuration files is not mandatory to keep backwards
> compatibility. The implementation of this feature tries to be compatible
> with command line usage -- the user should get the same results whether
> he passes the options (in the right order of parsing) on command line or
> writes them in config files. This allows the user to:
>
> 1) Override boolean options if needed
> 2) Specify partial configuration for options that are possible to pass
>    several times (e.g. "--external"), and pass the rest of the options
>    based on process runtime by command line
>
> Configuration file syntax allows comments marked with '#' sign, the rest
> of the line after '#' is ignored. The user can use one option per line
> (with argument supplied on the same line if needed, divided with whitespace
> characters), the options are the same as long options (without the "--"
> prefix used on command line).
>
> Configuration file example (syntax purposes only, doesn't make sense):
>
> $ cat ~/.criu.d/default.conf
> tcp-established
> work-dir /home/<USERNAME>/criu/work_directory
> extra # inline comment
> no-restore-sibling
> tree    111111
>
> Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
> ---
>  criu/Makefile             |   7 ++
>  criu/crtools.c            | 203 ++++++++++++++++++++++++++++++++++++++++++++--
>  criu/include/cr_options.h |   7 ++
>  3 files changed, 209 insertions(+), 8 deletions(-)
>
> diff --git a/criu/Makefile b/criu/Makefile
> index b2c6633..8e1a803 100644
> --- a/criu/Makefile
> +++ b/criu/Makefile
> @@ -15,6 +15,12 @@ ifeq ($(filter clean mrproper,$(MAKECMDGOALS)),)
>  endif
>
>  #
> +# Configuration file paths
> +CONFIG-DEFINES         += -DGLOBAL_CONFIG_DIR='"/etc/criu.d/"'
> +CONFIG-DEFINES         += -DDEFAULT_CONFIG_FILENAME='"default.conf"'
> +CONFIG-DEFINES         += -DUSER_CONFIG_DIR='".criu.d/"'
> +
> +#
>  # General flags.
>  ccflags-y              += -fno-strict-aliasing
>  ccflags-y              += -iquote criu/include
> @@ -24,6 +30,7 @@ ccflags-y             += -iquote $(ARCH_DIR)/include
>  ccflags-y              += -iquote .
>  ccflags-y              += -I/usr/include/libnl3
>  ccflags-y              += $(COMPEL_UAPI_INCLUDES)
> +ccflags-y              += $(CONFIG-DEFINES)
>
>  export ccflags-y
>
> diff --git a/criu/crtools.c b/criu/crtools.c
> index ab8e1b5..c4c5ebd 100644
> --- a/criu/crtools.c
> +++ b/criu/crtools.c
> @@ -55,6 +55,8 @@
>  #include "../soccr/soccr.h"
>
>  struct cr_options opts;
> +char **first_conf = NULL;
> +char **second_conf = NULL;

This better be something more descriptive like
global_conf/user_conf or something.

>
>  void init_opts(void)
>  {
> @@ -208,21 +210,178 @@ bool deprecated_ok(char *what)
>         return false;
>  }
>
> -int main(int argc, char *argv[], char *envp[])
> +bool is_default_config_forbidden(char *args[])
>  {
> +       int i = 0;
> +       while (args[i] != NULL) {
> +               if (!strncmp(args[i], "--no-default-config", strlen("--no-default-config")))
> +                       return true;

I don't like those argv[] helpers: before the patch we've parsed
argv string only once and four times after this patch.
But well, it looks to be the simplest solution for overwriting
opts with cmdline parameters, so I don't argue.

> +               i++;
> +       }
> +       return false;
> +}
>
> -#define BOOL_OPT(OPT_NAME, SAVE_TO) \
> -               {OPT_NAME, no_argument, SAVE_TO, true},\
> -               {"no-" OPT_NAME, no_argument, SAVE_TO, false}
> +char * specific_config_passed(char *args[])
> +{
> +       int i = 0;
> +       while (args[i]) {
> +               if (!strncmp(args[i], "--config", strlen("--config"))) {
> +                       /* getopt takes next string as required argument automatically */
> +                       return args[i + 1];
> +               } else if (strstr(args[i], "--config=") != NULL) {

When we will hit this condition?
AFAICS, `--config=path` will go to the first condition
as you compare strlen("--config") bytes.

> +                       return args[i] + strlen("--config=");
> +               }
> +               i++;
> +       }
> +       return NULL;
> +}
> +
> +void free_array(char **to_free)
> +{
> +       if (to_free == NULL) return;
> +       int i = 0;
> +       while (to_free[i] != NULL) {
> +               free(to_free[i]);
> +               i++;
> +       }
> +       free(to_free);
> +       to_free = NULL;
> +}
> +
> +void free_confs()
> +{
> +       free_array(first_conf);
> +       free_array(second_conf);
> +}
>
> +static int count_elements(char **to_count)
> +{
> +               int count = 0;
> +               if (to_count != NULL)
> +                       while (to_count[count] != NULL)
> +                               count++;
> +               return count;
> +}
> +
> +char ** parse_config(char *filepath, bool is_default)
> +{
> +       FILE* configfile = fopen(filepath, "r");
> +       if (!configfile) {
> +               /* Nonexistent default config file is *NOT* an error */
> +               if (!is_default)
> +                       pr_err("Can't access configuration file %s.\n", filepath);
> +               return NULL;
> +       }
> +
> +       int size = 10;

Please, use some define for this like
#define NR_CONIFIG_OPTIONS 10

> +       int elements_read = 0, len = 0;
> +       bool was_newline = true;
> +       char **configuration = xmalloc(size * sizeof(char *));

Variable declarations should be at the function beginning,
that's C style, gcc by default may bark on you with warnings
for such stuff.

While at it - `elements_read' is too gross for loop counter,
could it be just `i'?
While in contrast `size' may be more descriptive..

> +       if (configuration == NULL) {
> +               fclose(configfile);
> +               return NULL;
> +       }
> +       /* Initialize first element, getopt ignores it */
> +       configuration[0] = xmalloc((strlen("criu") + 1) * sizeof(char));

sizeof(char) = 1, I assure you!

> +       if (configuration[0] == NULL) {
> +               fclose(configfile);
> +               return NULL;
> +       }
> +       memcpy(configuration[0], "criu\0", strlen("criu") + 1);

Why is this not just
configuration[0] = "criu";
huh?

> +
> +       while (fscanf(configfile, "%ms", &configuration[elements_read + 1]) == 1) {

Can we have i/elements_read initialized to 1, to suppress this +1 noise?

> +               if (configuration[elements_read + 1][0] == '#') {
> +                       fscanf(configfile, "%*[^\n]");
> +                       was_newline = true;
> +               } else {
> +                       if (was_newline) {
> +                               len = strlen(configuration[elements_read + 1]);
> +                               char *tmp = xrealloc(configuration[elements_read + 1], len + strlen("--") + 1);
> +                               if (tmp == NULL) {
> +                                       free_array(configuration);
> +                                       fclose(configfile);
> +                                       return NULL;
> +                               }
> +                               memmove(tmp + strlen("--"), tmp, len + 1);
> +                               memmove(tmp, "--", strlen("--"));
> +                               configuration[elements_read + 1] = tmp;
> +                       }
> +                       elements_read++;
> +                       was_newline = fgetc(configfile) == '\n' ? true : false;
> +               }
> +               if (elements_read == size) {
> +                       size *= 2;
> +                       char **tmp_conf = xrealloc(configuration, size * sizeof(char *));
> +                       if (tmp_conf == NULL) {
> +                               free_array(configuration);
> +                               fclose(configfile);
> +                               return NULL;
> +                       }
> +                       configuration = tmp_conf;
> +               }
> +       }
> +       configuration[elements_read + 1] = NULL;
> +
> +       fclose(configfile);
> +       return configuration;
> +}
> +
> +int main(int argc, char *argv[], char *envp[])
> +{
>         pid_t pid = 0, tree_id = 0;
>         int ret = -1;
>         bool usage_error = true;
>         bool has_exec_cmd = false;
>         bool has_sub_command;
> -       int opt, idx;
> +       int opt = 0;
> +       int idx;
> +       int state = PARSING_FIRST;
>         int log_level = DEFAULT_LOGLEVEL;
>         char *imgs_dir = ".";
> +
> +       /* Check for --help / -h on commandline before parsing, otherwise
> +        * the help message won't be displayed if there is an error in
> +        * configuration file syntax. Checks are kept in parser in case of
> +        * option being put in the configuration file itself.
> +        */
> +       int i;
> +       for (i = 0; i < argc; i++) {
> +               if ((!strncmp(argv[i], "--help", strlen("--help"))) ||
> +                               (!strncmp(argv[i], "-h", strlen("-h")))) {
> +                       usage_error = false;
> +                       goto usage;
> +               }
> +       }

Can it go into a separate function?
Look, main() is at this moment 760 SLOC, why do you try
to add some more?

> +
> +       if (atexit(free_confs)) {
> +               pr_err("Unable to register cleanup function.\n");
> +               return 1;
> +       }
> +       char *specific_conf = specific_config_passed(argv);

Variables should be at function/block beginning.

> +       if ((specific_conf == NULL) && (!is_default_config_forbidden(argv))) {
> +               first_conf = parse_config(GLOBAL_CONFIG_DIR DEFAULT_CONFIG_FILENAME, true);
> +               char local_filepath[PATH_MAX + 1];
> +               char *home_dir = getenv("HOME");
> +               if (!home_dir) {
> +                       pr_info("Unable to get $HOME directory, local configuration file will not be used.");
> +               } else {
> +                       snprintf(local_filepath, PATH_MAX, "%s/%s%s",
> +                                       home_dir, USER_CONFIG_DIR, DEFAULT_CONFIG_FILENAME);
> +                       second_conf = parse_config(local_filepath, true);
> +               }
> +       } else if (specific_conf != NULL) {
> +               first_conf = parse_config(specific_conf, false);
> +               if (first_conf == NULL) {

Maybe remove then parse_config() argument and move
error messaging here?

> +                       return 1;
> +               }
> +       }
> +       int first_count = count_elements(first_conf);
> +       int second_count = count_elements(second_conf);

In the funciton's begginning.
Not main's, but some new function.

> +
> +#define BOOL_OPT(OPT_NAME, SAVE_TO) \
> +               {OPT_NAME, no_argument, SAVE_TO, true},\
> +               {"no-" OPT_NAME, no_argument, SAVE_TO, false}
> +
>         static const char short_opts[] = "dSsRf:F:t:p:hcD:o:v::x::Vr:jJ:lW:L:M:";
>         static struct option long_opts[] = {
>                 { "tree",                       required_argument,      0, 't'  },
> @@ -295,6 +454,8 @@ int main(int argc, char *argv[], char *envp[])
>                 { "status-fd",                  required_argument,      0, 1088 },
>                 BOOL_OPT("remote", &opts.remote),
>                 { "verbosity",                  optional_argument,      0, 'v'  },
> +               { "config",                     required_argument,      0, 1089},
> +               { "no-default-config",          no_argument,            0, 1090},
>                 { },
>         };
>
> @@ -333,9 +494,30 @@ int main(int argc, char *argv[], char *envp[])
>
>         while (1) {
>                 idx = -1;
> -               opt = getopt_long(argc, argv, short_opts, long_opts, &idx);
> -               if (opt == -1)
> +
> +               switch (state) {
> +               case PARSING_FIRST:
> +                       opt = getopt_long(first_count, first_conf, short_opts, long_opts, &idx);
> +                       break;
> +               case PARSING_SECOND:
> +                       opt = getopt_long(second_count, second_conf, short_opts, long_opts, &idx);
> +                       break;
> +               case PARSING_ARGV:
> +                       opt = getopt_long(argc, argv, short_opts, long_opts, &idx);
> +                       break;
> +               default:
>                         break;
> +               }
> +
> +               if (opt == -1) {
> +                       if (state < PARSING_ARGV) {

So, as you have default switch break, you
can just omit this comparisson.

> +                               state++;
> +                               optind = 0;
> +                               continue;
> +                       } else {
> +                               break;
> +                       }
> +               }
>                 if (!opt)
>                         continue;
>
> @@ -429,7 +611,6 @@ int main(int argc, char *argv[], char *envp[])
>                 case 1049:
>                         if (add_script(optarg))
>                                 return 1;
> -
>                         break;
>                 case 1051:
>                         opts.addr = optarg;
> @@ -576,6 +757,12 @@ int main(int argc, char *argv[], char *envp[])
>                                 return 1;
>                         }
>                         break;
> +               case 1089:
> +                       pr_info("Using configuration file %s\n.", optarg);
> +                       break;
> +               case 1090:
> +                       pr_info("Default configuration files disabled.\n");
> +                       break;
>                 case 'V':
>                         pr_msg("Version: %s\n", CRIU_VERSION);
>                         if (strcmp(CRIU_GITID, "0"))
> diff --git a/criu/include/cr_options.h b/criu/include/cr_options.h
> index 94fc717..f56c064 100644
> --- a/criu/include/cr_options.h
> +++ b/criu/include/cr_options.h
> @@ -128,4 +128,11 @@ extern struct cr_options opts;
>
>  extern void init_opts(void);
>
> +/*
> + * Option parsing state constants (marking max. 2 configuration files and argv)

This comment doesn't tell me anything, actually.

> + */
> +#define PARSING_FIRST  1
> +#define PARSING_SECOND 2
> +#define PARSING_ARGV   3

Why all this stuff in header?
Do you use it in the following patches from another c-files?

> +
>  #endif /* __CR_OPTIONS_H__ */
> --
> 2.7.4
>
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Veronika Kabatova May 23, 2017, 5:58 p.m.
----- Original Message -----
> From: "Dmitry Safonov" <0x7f454c46@gmail.com>
> To: "Veronika Kabatova" <vkabatov@redhat.com>
> Cc: "crml" <criu@openvz.org>
> Sent: Monday, May 22, 2017 8:23:08 PM
> Subject: Re: [CRIU] [PATCH 2/4] Add support for configuration files
> 
> Hi Veronika,
> 
> I like this stuff, but this looks like it wants some cleanups.
> Please, check comments below.
> 

Hi, thanks for the thorough review! I agree with most of what you said.
Comments below as well. If you won't have additional advice about these
things I can submit the fixed patchset.

> 2017-05-22 18:00 GMT+03:00  <vkabatov@redhat.com>:
> > From: Veronika Kabatova <vkabatov@redhat.com>
> >
> > Implementation changes for usage of simple configuration files. Before
> > parsing the command line options, either default configuration files
> > (/etc/criu.d/default.conf, $HOME/.criu.d/default.conf; in this order) are
> > parsed, or a specific config file passed by the user. Two new options are
> > introduced: "--config FILEPATH" option allows users to specify a single
> > configuration file they want to use; and "--no-default-config" option to
> > forbid the parsing of default configuration files. Both options are to be
> > passed only via the command line.
> >
> > Usage of configuration files is not mandatory to keep backwards
> > compatibility. The implementation of this feature tries to be compatible
> > with command line usage -- the user should get the same results whether
> > he passes the options (in the right order of parsing) on command line or
> > writes them in config files. This allows the user to:
> >
> > 1) Override boolean options if needed
> > 2) Specify partial configuration for options that are possible to pass
> >    several times (e.g. "--external"), and pass the rest of the options
> >    based on process runtime by command line
> >
> > Configuration file syntax allows comments marked with '#' sign, the rest
> > of the line after '#' is ignored. The user can use one option per line
> > (with argument supplied on the same line if needed, divided with whitespace
> > characters), the options are the same as long options (without the "--"
> > prefix used on command line).
> >
> > Configuration file example (syntax purposes only, doesn't make sense):
> >
> > $ cat ~/.criu.d/default.conf
> > tcp-established
> > work-dir /home/<USERNAME>/criu/work_directory
> > extra # inline comment
> > no-restore-sibling
> > tree    111111
> >
> > Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
> > ---
> >  criu/Makefile             |   7 ++
> >  criu/crtools.c            | 203
> >  ++++++++++++++++++++++++++++++++++++++++++++--
> >  criu/include/cr_options.h |   7 ++
> >  3 files changed, 209 insertions(+), 8 deletions(-)
> >
> > diff --git a/criu/Makefile b/criu/Makefile
> > index b2c6633..8e1a803 100644
> > --- a/criu/Makefile
> > +++ b/criu/Makefile
> > @@ -15,6 +15,12 @@ ifeq ($(filter clean mrproper,$(MAKECMDGOALS)),)
> >  endif
> >
> >  #
> > +# Configuration file paths
> > +CONFIG-DEFINES         += -DGLOBAL_CONFIG_DIR='"/etc/criu.d/"'
> > +CONFIG-DEFINES         += -DDEFAULT_CONFIG_FILENAME='"default.conf"'
> > +CONFIG-DEFINES         += -DUSER_CONFIG_DIR='".criu.d/"'
> > +
> > +#
> >  # General flags.
> >  ccflags-y              += -fno-strict-aliasing
> >  ccflags-y              += -iquote criu/include
> > @@ -24,6 +30,7 @@ ccflags-y             += -iquote $(ARCH_DIR)/include
> >  ccflags-y              += -iquote .
> >  ccflags-y              += -I/usr/include/libnl3
> >  ccflags-y              += $(COMPEL_UAPI_INCLUDES)
> > +ccflags-y              += $(CONFIG-DEFINES)
> >
> >  export ccflags-y
> >
> > diff --git a/criu/crtools.c b/criu/crtools.c
> > index ab8e1b5..c4c5ebd 100644
> > --- a/criu/crtools.c
> > +++ b/criu/crtools.c
> > @@ -55,6 +55,8 @@
> >  #include "../soccr/soccr.h"
> >
> >  struct cr_options opts;
> > +char **first_conf = NULL;
> > +char **second_conf = NULL;
> 
> This better be something more descriptive like
> global_conf/user_conf or something.
> 
> >
> >  void init_opts(void)
> >  {
> > @@ -208,21 +210,178 @@ bool deprecated_ok(char *what)
> >         return false;
> >  }
> >
> > -int main(int argc, char *argv[], char *envp[])
> > +bool is_default_config_forbidden(char *args[])
> >  {
> > +       int i = 0;
> > +       while (args[i] != NULL) {
> > +               if (!strncmp(args[i], "--no-default-config",
> > strlen("--no-default-config")))
> > +                       return true;
> 
> I don't like those argv[] helpers: before the patch we've parsed
> argv string only once and four times after this patch.
> But well, it looks to be the simplest solution for overwriting
> opts with cmdline parameters, so I don't argue.
> 

I agree with you, it's not the nicest solution, but so far the
easiest one that occurred to me. Of course if someone has a better
idea I can implement that instead.

> > +               i++;
> > +       }
> > +       return false;
> > +}
> >
> > -#define BOOL_OPT(OPT_NAME, SAVE_TO) \
> > -               {OPT_NAME, no_argument, SAVE_TO, true},\
> > -               {"no-" OPT_NAME, no_argument, SAVE_TO, false}
> > +char * specific_config_passed(char *args[])
> > +{
> > +       int i = 0;
> > +       while (args[i]) {
> > +               if (!strncmp(args[i], "--config", strlen("--config"))) {
> > +                       /* getopt takes next string as required argument
> > automatically */
> > +                       return args[i + 1];
> > +               } else if (strstr(args[i], "--config=") != NULL) {
> 
> When we will hit this condition?
> AFAICS, `--config=path` will go to the first condition
> as you compare strlen("--config") bytes.
> 

Good catch of my stupid mistake, thanks!

> > +                       return args[i] + strlen("--config=");
> > +               }
> > +               i++;
> > +       }
> > +       return NULL;
> > +}
> > +
> > +void free_array(char **to_free)
> > +{
> > +       if (to_free == NULL) return;
> > +       int i = 0;
> > +       while (to_free[i] != NULL) {
> > +               free(to_free[i]);
> > +               i++;
> > +       }
> > +       free(to_free);
> > +       to_free = NULL;
> > +}
> > +
> > +void free_confs()
> > +{
> > +       free_array(first_conf);
> > +       free_array(second_conf);
> > +}
> >
> > +static int count_elements(char **to_count)
> > +{
> > +               int count = 0;
> > +               if (to_count != NULL)
> > +                       while (to_count[count] != NULL)
> > +                               count++;
> > +               return count;
> > +}
> > +
> > +char ** parse_config(char *filepath, bool is_default)
> > +{
> > +       FILE* configfile = fopen(filepath, "r");
> > +       if (!configfile) {
> > +               /* Nonexistent default config file is *NOT* an error */
> > +               if (!is_default)
> > +                       pr_err("Can't access configuration file %s.\n",
> > filepath);
> > +               return NULL;
> > +       }
> > +
> > +       int size = 10;
> 
> Please, use some define for this like
> #define NR_CONIFIG_OPTIONS 10
> 
> > +       int elements_read = 0, len = 0;
> > +       bool was_newline = true;
> > +       char **configuration = xmalloc(size * sizeof(char *));
> 
> Variable declarations should be at the function beginning,
> that's C style, gcc by default may bark on you with warnings
> for such stuff.
>

GCC has never been angry at me for not doing that, but I changed
it to match your style.
 
> While at it - `elements_read' is too gross for loop counter,
> could it be just `i'?
> While in contrast `size' may be more descriptive..
> 
> > +       if (configuration == NULL) {
> > +               fclose(configfile);
> > +               return NULL;
> > +       }
> > +       /* Initialize first element, getopt ignores it */
> > +       configuration[0] = xmalloc((strlen("criu") + 1) * sizeof(char));
> 
> sizeof(char) = 1, I assure you!
> 
> > +       if (configuration[0] == NULL) {
> > +               fclose(configfile);
> > +               return NULL;
> > +       }
> > +       memcpy(configuration[0], "criu\0", strlen("criu") + 1);
> 
> Why is this not just
> configuration[0] = "criu";
> huh?
> 

Getting a nice core dump on my machine if I use this. I suspect it's because
of freeing the configuration arrays, and valgrind kinda confirms it saying
the address is in r-x mapped file segment, mentioned on free() call. I guess
I can change the free_array function, but because of potential reuse of it on
any other char **array I prefer not to.

> > +
> > +       while (fscanf(configfile, "%ms", &configuration[elements_read + 1])
> > == 1) {
> 
> Can we have i/elements_read initialized to 1, to suppress this +1 noise?
> 
> > +               if (configuration[elements_read + 1][0] == '#') {
> > +                       fscanf(configfile, "%*[^\n]");
> > +                       was_newline = true;
> > +               } else {
> > +                       if (was_newline) {
> > +                               len = strlen(configuration[elements_read +
> > 1]);
> > +                               char *tmp =
> > xrealloc(configuration[elements_read + 1], len + strlen("--") + 1);
> > +                               if (tmp == NULL) {
> > +                                       free_array(configuration);
> > +                                       fclose(configfile);
> > +                                       return NULL;
> > +                               }
> > +                               memmove(tmp + strlen("--"), tmp, len + 1);
> > +                               memmove(tmp, "--", strlen("--"));
> > +                               configuration[elements_read + 1] = tmp;
> > +                       }
> > +                       elements_read++;
> > +                       was_newline = fgetc(configfile) == '\n' ? true :
> > false;
> > +               }
> > +               if (elements_read == size) {
> > +                       size *= 2;
> > +                       char **tmp_conf = xrealloc(configuration, size *
> > sizeof(char *));
> > +                       if (tmp_conf == NULL) {
> > +                               free_array(configuration);
> > +                               fclose(configfile);
> > +                               return NULL;
> > +                       }
> > +                       configuration = tmp_conf;
> > +               }
> > +       }
> > +       configuration[elements_read + 1] = NULL;
> > +
> > +       fclose(configfile);
> > +       return configuration;
> > +}
> > +
> > +int main(int argc, char *argv[], char *envp[])
> > +{
> >         pid_t pid = 0, tree_id = 0;
> >         int ret = -1;
> >         bool usage_error = true;
> >         bool has_exec_cmd = false;
> >         bool has_sub_command;
> > -       int opt, idx;
> > +       int opt = 0;
> > +       int idx;
> > +       int state = PARSING_FIRST;
> >         int log_level = DEFAULT_LOGLEVEL;
> >         char *imgs_dir = ".";
> > +
> > +       /* Check for --help / -h on commandline before parsing, otherwise
> > +        * the help message won't be displayed if there is an error in
> > +        * configuration file syntax. Checks are kept in parser in case of
> > +        * option being put in the configuration file itself.
> > +        */
> > +       int i;
> > +       for (i = 0; i < argc; i++) {
> > +               if ((!strncmp(argv[i], "--help", strlen("--help"))) ||
> > +                               (!strncmp(argv[i], "-h", strlen("-h")))) {
> > +                       usage_error = false;
> > +                       goto usage;
> > +               }
> > +       }
> 
> Can it go into a separate function?
> Look, main() is at this moment 760 SLOC, why do you try
> to add some more?
> 
> > +
> > +       if (atexit(free_confs)) {
> > +               pr_err("Unable to register cleanup function.\n");
> > +               return 1;
> > +       }
> > +       char *specific_conf = specific_config_passed(argv);
> 
> Variables should be at function/block beginning.
> 
> > +       if ((specific_conf == NULL) &&
> > (!is_default_config_forbidden(argv))) {
> > +               first_conf = parse_config(GLOBAL_CONFIG_DIR
> > DEFAULT_CONFIG_FILENAME, true);
> > +               char local_filepath[PATH_MAX + 1];
> > +               char *home_dir = getenv("HOME");
> > +               if (!home_dir) {
> > +                       pr_info("Unable to get $HOME directory, local
> > configuration file will not be used.");
> > +               } else {
> > +                       snprintf(local_filepath, PATH_MAX, "%s/%s%s",
> > +                                       home_dir, USER_CONFIG_DIR,
> > DEFAULT_CONFIG_FILENAME);
> > +                       second_conf = parse_config(local_filepath, true);
> > +               }
> > +       } else if (specific_conf != NULL) {
> > +               first_conf = parse_config(specific_conf, false);
> > +               if (first_conf == NULL) {
> 
> Maybe remove then parse_config() argument and move
> error messaging here?
> 
> > +                       return 1;
> > +               }
> > +       }
> > +       int first_count = count_elements(first_conf);
> > +       int second_count = count_elements(second_conf);
> 
> In the funciton's begginning.
> Not main's, but some new function.
> 
> > +
> > +#define BOOL_OPT(OPT_NAME, SAVE_TO) \
> > +               {OPT_NAME, no_argument, SAVE_TO, true},\
> > +               {"no-" OPT_NAME, no_argument, SAVE_TO, false}
> > +
> >         static const char short_opts[] =
> >         "dSsRf:F:t:p:hcD:o:v::x::Vr:jJ:lW:L:M:";
> >         static struct option long_opts[] = {
> >                 { "tree",                       required_argument,      0,
> >                 't'  },
> > @@ -295,6 +454,8 @@ int main(int argc, char *argv[], char *envp[])
> >                 { "status-fd",                  required_argument,      0,
> >                 1088 },
> >                 BOOL_OPT("remote", &opts.remote),
> >                 { "verbosity",                  optional_argument,      0,
> >                 'v'  },
> > +               { "config",                     required_argument,      0,
> > 1089},
> > +               { "no-default-config",          no_argument,            0,
> > 1090},
> >                 { },
> >         };
> >
> > @@ -333,9 +494,30 @@ int main(int argc, char *argv[], char *envp[])
> >
> >         while (1) {
> >                 idx = -1;
> > -               opt = getopt_long(argc, argv, short_opts, long_opts, &idx);
> > -               if (opt == -1)
> > +
> > +               switch (state) {
> > +               case PARSING_FIRST:
> > +                       opt = getopt_long(first_count, first_conf,
> > short_opts, long_opts, &idx);
> > +                       break;
> > +               case PARSING_SECOND:
> > +                       opt = getopt_long(second_count, second_conf,
> > short_opts, long_opts, &idx);
> > +                       break;
> > +               case PARSING_ARGV:
> > +                       opt = getopt_long(argc, argv, short_opts,
> > long_opts, &idx);
> > +                       break;
> > +               default:
> >                         break;
> > +               }
> > +
> > +               if (opt == -1) {
> > +                       if (state < PARSING_ARGV) {
> 
> So, as you have default switch break, you
> can just omit this comparisson.
> 

The default branch in switch breaks only from the switch. This one is
for ending the while loop. I guess I can remove the default branch if
it's confusing since no other values are expected, I'm just used to
have it there.

> > +                               state++;
> > +                               optind = 0;
> > +                               continue;
> > +                       } else {
> > +                               break;
> > +                       }
> > +               }
> >                 if (!opt)
> >                         continue;
> >
> > @@ -429,7 +611,6 @@ int main(int argc, char *argv[], char *envp[])
> >                 case 1049:
> >                         if (add_script(optarg))
> >                                 return 1;
> > -
> >                         break;
> >                 case 1051:
> >                         opts.addr = optarg;
> > @@ -576,6 +757,12 @@ int main(int argc, char *argv[], char *envp[])
> >                                 return 1;
> >                         }
> >                         break;
> > +               case 1089:
> > +                       pr_info("Using configuration file %s\n.", optarg);
> > +                       break;
> > +               case 1090:
> > +                       pr_info("Default configuration files disabled.\n");
> > +                       break;
> >                 case 'V':
> >                         pr_msg("Version: %s\n", CRIU_VERSION);
> >                         if (strcmp(CRIU_GITID, "0"))
> > diff --git a/criu/include/cr_options.h b/criu/include/cr_options.h
> > index 94fc717..f56c064 100644
> > --- a/criu/include/cr_options.h
> > +++ b/criu/include/cr_options.h
> > @@ -128,4 +128,11 @@ extern struct cr_options opts;
> >
> >  extern void init_opts(void);
> >
> > +/*
> > + * Option parsing state constants (marking max. 2 configuration files and
> > argv)
> 
> This comment doesn't tell me anything, actually.
> 
> > + */
> > +#define PARSING_FIRST  1
> > +#define PARSING_SECOND 2
> > +#define PARSING_ARGV   3
> 
> Why all this stuff in header?
> Do you use it in the following patches from another c-files?
> 
> > +
> >  #endif /* __CR_OPTIONS_H__ */
> > --
> > 2.7.4
> >
> > _______________________________________________
> > CRIU mailing list
> > CRIU@openvz.org
> > https://lists.openvz.org/mailman/listinfo/criu
> 
> 
> --
>              Dmitry
>
Dmitry Safonov May 24, 2017, 9:41 a.m.
2017-05-23 20:58 GMT+03:00 Veronika Kabatova <vkabatov@redhat.com>:
>
>
> ----- Original Message -----
>> From: "Dmitry Safonov" <0x7f454c46@gmail.com>
>> To: "Veronika Kabatova" <vkabatov@redhat.com>
>> Cc: "crml" <criu@openvz.org>
>> Sent: Monday, May 22, 2017 8:23:08 PM
>> Subject: Re: [CRIU] [PATCH 2/4] Add support for configuration files
>>
>> Hi Veronika,
>>
>> I like this stuff, but this looks like it wants some cleanups.
>> Please, check comments below.
>>
>
> Hi, thanks for the thorough review! I agree with most of what you said.
> Comments below as well. If you won't have additional advice about these
> things I can submit the fixed patchset.
>
>> 2017-05-22 18:00 GMT+03:00  <vkabatov@redhat.com>:
>> > From: Veronika Kabatova <vkabatov@redhat.com>
>> >
>> > Implementation changes for usage of simple configuration files. Before
>> > parsing the command line options, either default configuration files
>> > (/etc/criu.d/default.conf, $HOME/.criu.d/default.conf; in this order) are
>> > parsed, or a specific config file passed by the user. Two new options are
>> > introduced: "--config FILEPATH" option allows users to specify a single
>> > configuration file they want to use; and "--no-default-config" option to
>> > forbid the parsing of default configuration files. Both options are to be
>> > passed only via the command line.
>> >
>> > Usage of configuration files is not mandatory to keep backwards
>> > compatibility. The implementation of this feature tries to be compatible
>> > with command line usage -- the user should get the same results whether
>> > he passes the options (in the right order of parsing) on command line or
>> > writes them in config files. This allows the user to:
>> >
>> > 1) Override boolean options if needed
>> > 2) Specify partial configuration for options that are possible to pass
>> >    several times (e.g. "--external"), and pass the rest of the options
>> >    based on process runtime by command line
>> >
>> > Configuration file syntax allows comments marked with '#' sign, the rest
>> > of the line after '#' is ignored. The user can use one option per line
>> > (with argument supplied on the same line if needed, divided with whitespace
>> > characters), the options are the same as long options (without the "--"
>> > prefix used on command line).
>> >
>> > Configuration file example (syntax purposes only, doesn't make sense):
>> >
>> > $ cat ~/.criu.d/default.conf
>> > tcp-established
>> > work-dir /home/<USERNAME>/criu/work_directory
>> > extra # inline comment
>> > no-restore-sibling
>> > tree    111111
>> >
>> > Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
[..]
>>
>> > +       int elements_read = 0, len = 0;
>> > +       bool was_newline = true;
>> > +       char **configuration = xmalloc(size * sizeof(char *));
>>
>> Variable declarations should be at the function beginning,
>> that's C style, gcc by default may bark on you with warnings
>> for such stuff.
>>
>
> GCC has never been angry at me for not doing that, but I changed
> it to match your style.

Well, it's -Wdeclaration-after-statement GCC's option.
It's used by default in Linux kernel and we try to follow kernel's
code style in CRIU.
You can view it if you want here:
http://elixir.free-electrons.com/linux/latest/source/Documentation/process/coding-style.rst

>
>> While at it - `elements_read' is too gross for loop counter,
>> could it be just `i'?
>> While in contrast `size' may be more descriptive..
>>
>> > +       if (configuration == NULL) {
>> > +               fclose(configfile);
>> > +               return NULL;
>> > +       }
>> > +       /* Initialize first element, getopt ignores it */
>> > +       configuration[0] = xmalloc((strlen("criu") + 1) * sizeof(char));
>>
>> sizeof(char) = 1, I assure you!
>>
>> > +       if (configuration[0] == NULL) {
>> > +               fclose(configfile);
>> > +               return NULL;
>> > +       }
>> > +       memcpy(configuration[0], "criu\0", strlen("criu") + 1);
>>
>> Why is this not just
>> configuration[0] = "criu";
>> huh?
>>
>
> Getting a nice core dump on my machine if I use this. I suspect it's because
> of freeing the configuration arrays, and valgrind kinda confirms it saying
> the address is in r-x mapped file segment, mentioned on free() call. I guess
> I can change the free_array function, but because of potential reuse of it on
> any other char **array I prefer not to.

Oh, I see - then change free_array *or* add a comment here, please.

>> > @@ -333,9 +494,30 @@ int main(int argc, char *argv[], char *envp[])
>> >
>> >         while (1) {
>> >                 idx = -1;
>> > -               opt = getopt_long(argc, argv, short_opts, long_opts, &idx);
>> > -               if (opt == -1)
>> > +
>> > +               switch (state) {
>> > +               case PARSING_FIRST:
>> > +                       opt = getopt_long(first_count, first_conf,
>> > short_opts, long_opts, &idx);
>> > +                       break;
>> > +               case PARSING_SECOND:
>> > +                       opt = getopt_long(second_count, second_conf,
>> > short_opts, long_opts, &idx);
>> > +                       break;
>> > +               case PARSING_ARGV:
>> > +                       opt = getopt_long(argc, argv, short_opts,
>> > long_opts, &idx);
>> > +                       break;
>> > +               default:
>> >                         break;
>> > +               }
>> > +
>> > +               if (opt == -1) {
>> > +                       if (state < PARSING_ARGV) {
>>
>> So, as you have default switch break, you
>> can just omit this comparisson.
>>
>
> The default branch in switch breaks only from the switch. This one is
> for ending the while loop. I guess I can remove the default branch if
> it's confusing since no other values are expected, I'm just used to
> have it there.

Yeah, please, remove the default as it's not used and makes
no difference with/without it anyway.
Dmitry Safonov May 24, 2017, 9:50 a.m.
2017-05-24 12:41 GMT+03:00 Dmitry Safonov <0x7f454c46@gmail.com>:
> 2017-05-23 20:58 GMT+03:00 Veronika Kabatova <vkabatov@redhat.com>:
>>
>>
>> ----- Original Message -----
>>> From: "Dmitry Safonov" <0x7f454c46@gmail.com>
>>> To: "Veronika Kabatova" <vkabatov@redhat.com>
>>> Cc: "crml" <criu@openvz.org>
>>> Sent: Monday, May 22, 2017 8:23:08 PM
>>> Subject: Re: [CRIU] [PATCH 2/4] Add support for configuration files
>>>
>>> Hi Veronika,
>>>
>>> I like this stuff, but this looks like it wants some cleanups.
>>> Please, check comments below.
>>>
>>
>> Hi, thanks for the thorough review! I agree with most of what you said.
>> Comments below as well. If you won't have additional advice about these
>> things I can submit the fixed patchset.
>>
>>> 2017-05-22 18:00 GMT+03:00  <vkabatov@redhat.com>:
>>> > From: Veronika Kabatova <vkabatov@redhat.com>
>>> >
>>> > Implementation changes for usage of simple configuration files. Before
>>> > parsing the command line options, either default configuration files
>>> > (/etc/criu.d/default.conf, $HOME/.criu.d/default.conf; in this order) are
>>> > parsed, or a specific config file passed by the user. Two new options are
>>> > introduced: "--config FILEPATH" option allows users to specify a single
>>> > configuration file they want to use; and "--no-default-config" option to
>>> > forbid the parsing of default configuration files. Both options are to be
>>> > passed only via the command line.
>>> >
>>> > Usage of configuration files is not mandatory to keep backwards
>>> > compatibility. The implementation of this feature tries to be compatible
>>> > with command line usage -- the user should get the same results whether
>>> > he passes the options (in the right order of parsing) on command line or
>>> > writes them in config files. This allows the user to:
>>> >
>>> > 1) Override boolean options if needed
>>> > 2) Specify partial configuration for options that are possible to pass
>>> >    several times (e.g. "--external"), and pass the rest of the options
>>> >    based on process runtime by command line
>>> >
>>> > Configuration file syntax allows comments marked with '#' sign, the rest
>>> > of the line after '#' is ignored. The user can use one option per line
>>> > (with argument supplied on the same line if needed, divided with whitespace
>>> > characters), the options are the same as long options (without the "--"
>>> > prefix used on command line).
>>> >
>>> > Configuration file example (syntax purposes only, doesn't make sense):
>>> >
>>> > $ cat ~/.criu.d/default.conf
>>> > tcp-established
>>> > work-dir /home/<USERNAME>/criu/work_directory
>>> > extra # inline comment
>>> > no-restore-sibling
>>> > tree    111111
>>> >
>>> > Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
> [..]
>>>
>>> > +       int elements_read = 0, len = 0;
>>> > +       bool was_newline = true;
>>> > +       char **configuration = xmalloc(size * sizeof(char *));
>>>
>>> Variable declarations should be at the function beginning,
>>> that's C style, gcc by default may bark on you with warnings
>>> for such stuff.
>>>
>>
>> GCC has never been angry at me for not doing that, but I changed
>> it to match your style.
>
> Well, it's -Wdeclaration-after-statement GCC's option.
> It's used by default in Linux kernel and we try to follow kernel's
> code style in CRIU.
> You can view it if you want here:
> http://elixir.free-electrons.com/linux/latest/source/Documentation/process/coding-style.rst

Ok, actually, it's not described there (it's been some time since I last looked
there, so was sure it's written), but kernel just follows it with
enabled gcc option.

>
>>
>>> While at it - `elements_read' is too gross for loop counter,
>>> could it be just `i'?
>>> While in contrast `size' may be more descriptive..
>>>
>>> > +       if (configuration == NULL) {
>>> > +               fclose(configfile);
>>> > +               return NULL;
>>> > +       }
>>> > +       /* Initialize first element, getopt ignores it */
>>> > +       configuration[0] = xmalloc((strlen("criu") + 1) * sizeof(char));
>>>
>>> sizeof(char) = 1, I assure you!
>>>
>>> > +       if (configuration[0] == NULL) {
>>> > +               fclose(configfile);
>>> > +               return NULL;
>>> > +       }
>>> > +       memcpy(configuration[0], "criu\0", strlen("criu") + 1);
>>>
>>> Why is this not just
>>> configuration[0] = "criu";
>>> huh?
>>>
>>
>> Getting a nice core dump on my machine if I use this. I suspect it's because
>> of freeing the configuration arrays, and valgrind kinda confirms it saying
>> the address is in r-x mapped file segment, mentioned on free() call. I guess
>> I can change the free_array function, but because of potential reuse of it on
>> any other char **array I prefer not to.
>
> Oh, I see - then change free_array *or* add a comment here, please.
>
>>> > @@ -333,9 +494,30 @@ int main(int argc, char *argv[], char *envp[])
>>> >
>>> >         while (1) {
>>> >                 idx = -1;
>>> > -               opt = getopt_long(argc, argv, short_opts, long_opts, &idx);
>>> > -               if (opt == -1)
>>> > +
>>> > +               switch (state) {
>>> > +               case PARSING_FIRST:
>>> > +                       opt = getopt_long(first_count, first_conf,
>>> > short_opts, long_opts, &idx);
>>> > +                       break;
>>> > +               case PARSING_SECOND:
>>> > +                       opt = getopt_long(second_count, second_conf,
>>> > short_opts, long_opts, &idx);
>>> > +                       break;
>>> > +               case PARSING_ARGV:
>>> > +                       opt = getopt_long(argc, argv, short_opts,
>>> > long_opts, &idx);
>>> > +                       break;
>>> > +               default:
>>> >                         break;
>>> > +               }
>>> > +
>>> > +               if (opt == -1) {
>>> > +                       if (state < PARSING_ARGV) {
>>>
>>> So, as you have default switch break, you
>>> can just omit this comparisson.
>>>
>>
>> The default branch in switch breaks only from the switch. This one is
>> for ending the while loop. I guess I can remove the default branch if
>> it's confusing since no other values are expected, I'm just used to
>> have it there.
>
> Yeah, please, remove the default as it's not used and makes
> no difference with/without it anyway.
>
> --
>              Dmitry