[CRIU] [PATCH 2/4] Add support for configuration files
Dmitry Safonov
0x7f454c46 at gmail.com
Mon May 22 11:23:08 PDT 2017
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 at redhat.com>:
> From: Veronika Kabatova <vkabatov at 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 at 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 at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
--
Dmitry
More information about the CRIU
mailing list