[CRIU] [PATCH v2 2/4] Add support for configuration files
Andrei Vagin
avagin at virtuozzo.com
Sat Jun 3 04:33:59 MSK 2017
On Wed, May 24, 2017 at 04:18:08PM +0200, vkabatov at redhat.com wrote:
> 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
Will it work for paths with spaces? For example:
work-dir "/home/testuser/criu/work directoy/"
> extra # inline comment
> no-restore-sibling
> tree 111111
>
> Signed-off-by: Veronika Kabatova <vkabatov at redhat.com>
> ---
> criu/Makefile | 7 ++
> criu/crtools.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 223 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/"'
The ".d" postfix usually means that a directoy can have a few config
files and all of them will be used.
[avagin at laptop ~]$ ls -l /etc/cron.d/
total 8
-rw-r--r--. 1 root root 128 Aug 23 2016 0hourly
-rw-r--r--. 1 root root 108 Aug 12 2016 raid-check
Do you parse all of them?
> +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..aee020f 100644
> --- a/criu/crtools.c
> +++ b/criu/crtools.c
> @@ -55,6 +55,8 @@
> #include "../soccr/soccr.h"
>
> struct cr_options opts;
> +char **global_conf = NULL;
> +char **user_conf = NULL;
>
> void init_opts(void)
> {
> @@ -208,21 +210,190 @@ 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) {
I am not sure that this check is correct. Why do you not use argc here?
> + 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[])
can we merge this and the previous functions to not enumirate all
arguments twice
> +{
> + int i = 0;
> + while (args[i]) {
> + if (!strcmp(args[i], "--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)
> +{
> + int i = 0;
> + if (to_free == NULL) return;
> + while (to_free[i] != NULL) {
> + free(to_free[i]);
> + i++;
> + }
> + free(to_free);
> + to_free = NULL;
> +}
> +
> +void free_confs()
> +{
> + free_array(global_conf);
> + free_array(user_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)
> +{
> +#define DEFAULT_CONFIG_SIZE 10
> + FILE* configfile = fopen(filepath, "r");
> + int config_size = DEFAULT_CONFIG_SIZE;
> + int i = 1, len = 0, next;
> + bool was_newline = true;
> + char *tmp_string;
> + char **tmp_conf;
> + char **configuration = xmalloc(config_size * sizeof(char *));
> +
> + if (!configfile) {
> + return NULL;
> + }
it is better to call xmalloc here for configuration
> + if (configuration == NULL) {
> + fclose(configfile);
> + return NULL;
> + }
> + /*
> + * Initialize first element, getopt ignores it. Be consistent with
> + * dynamic memory allocations to be able to keep free_array() function
> + * for freeing char **arrays generic.
> + */
> + configuration[0] = xmalloc(strlen("criu") + 1);
> + if (configuration[0] == NULL) {
> + fclose(configfile);
> + return NULL;
> + }
> + memcpy(configuration[0], "criu\0", strlen("criu") + 1);
> +
> + while (fscanf(configfile, "%ms", &configuration[i]) == 1) {
> + if (configuration[i][0] == '#') {
> + fscanf(configfile, "%*[^\n]");
> + was_newline = true;
> + } else {
> + if (was_newline) {
> + len = strlen(configuration[i]);
> + tmp_string = xrealloc(configuration[i], len + strlen("--") + 1);
> + if (tmp_string == NULL) {
> + free_array(configuration);
> + fclose(configfile);
> + return NULL;
> + }
> + memmove(tmp_string + strlen("--"), tmp_string, len + 1);
> + memmove(tmp_string, "--", strlen("--"));
> + configuration[i] = tmp_string;
> + }
> + i++;
> + fscanf(configfile, "%*[ \t]");
> + was_newline = (next = getc(configfile)) == '\n' ? true : false;
> + if (!was_newline) ungetc(next, configfile);
> + }
> + if (i == config_size) {
> + config_size *= 2;
> + tmp_conf = xrealloc(configuration, config_size * sizeof(char *));
> + if (tmp_conf == NULL) {
> + free_array(configuration);
> + fclose(configfile);
> + return NULL;
> + }
> + configuration = tmp_conf;
> + }
> + }
> + configuration[i] = NULL;
> +
> + fclose(configfile);
> + return configuration;
> +}
> +
> +bool is_help_passed(int argc, char *argv[])
> +{
> + /* 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(argv[i]))) ||
> + (!strncmp(argv[i], "-h", strlen(argv[i])))) {
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +void init_configuration(int argc, char *argv[])
^^^ it has to be static, doesn't it?
> +{
> + char *specific_conf = specific_config_passed(argv);
> + char local_filepath[PATH_MAX + 1];
> + char *home_dir = getenv("HOME");
> +
> + if ((specific_conf == NULL) && (!is_default_config_forbidden(argv))) {
> + global_conf = parse_config(GLOBAL_CONFIG_DIR DEFAULT_CONFIG_FILENAME);
> + 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);
> + user_conf = parse_config(local_filepath);
if local_filepath doesn't exists, it is ok, but if the file exists and
we can't parse it, we have to report an error and exit.
> + }
> + } else if (specific_conf != NULL) {
> + global_conf = parse_config(specific_conf);
> + if (global_conf == NULL) {
> + pr_err("Can't access configuration file %s.\n", specific_conf);
> + exit(1);
> + }
> + }
> +}
> +
> +int main(int argc, char *argv[], char *envp[])
> +{
> +#define PARSING_GLOBAL_CONF 1
> +#define PARSING_USER_CONF 2
> +#define PARSING_ARGV 3
>
> 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, idx;
> + int first_count = 1, second_count = 1;
> + int state = PARSING_GLOBAL_CONF;
> int log_level = DEFAULT_LOGLEVEL;
> char *imgs_dir = ".";
> +
> +#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,11 +466,25 @@ 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},
we need to add descriptions for these options
> { },
> };
>
> #undef BOOL_OPT
>
> + if (is_help_passed(argc, argv)) {
> + usage_error = false;
> + goto usage;
> + }
> + if (atexit(free_confs)) {
> + pr_err("Unable to register cleanup function.\n");
> + return 1;
> + }
I think we can live without this hook ^^^, when a process exits, it
releases memory. In criu, we don't care about releasing memory before
exiting.
> + init_configuration(argc, argv);
> + if (global_conf != NULL) first_count = count_elements(global_conf);
> + if (user_conf != NULL) second_count = count_elements(user_conf);
> +
> BUILD_BUG_ON(PAGE_SIZE != PAGE_IMAGE_SIZE);
> BUILD_BUG_ON(CTL_32 != SYSCTL_TYPE__CTL_32);
> BUILD_BUG_ON(__CTL_STR != SYSCTL_TYPE__CTL_STR);
> @@ -333,9 +518,27 @@ 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_GLOBAL_CONF:
> + opt = getopt_long(first_count, global_conf, short_opts, long_opts, &idx);
> break;
> + case PARSING_USER_CONF:
> + opt = getopt_long(second_count, user_conf, short_opts, long_opts, &idx);
> + break;
> + case PARSING_ARGV:
> + opt = getopt_long(argc, argv, short_opts, long_opts, &idx);
> + break;
> + }
> + if (opt == -1) {
> + if (state < PARSING_ARGV) {
> + state++;
> + optind = 0;
> + continue;
> + } else {
> + break;
> + }
> + }
> if (!opt)
> continue;
>
> @@ -429,7 +632,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 +778,12 @@ int main(int argc, char *argv[], char *envp[])
> return 1;
> }
> break;
> + case 1089:
> + pr_info("Using configuration file %s\n.", optarg);
I don't think that we need to print something here, because logging
isn't set at this moment.
> + 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"))
> --
> 2.7.4
>
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
More information about the CRIU
mailing list