[CRIU] [PATCH 2/4] Add support for configuration files
Veronika Kabatova
vkabatov at redhat.com
Tue May 23 10:58:12 PDT 2017
----- Original Message -----
> From: "Dmitry Safonov" <0x7f454c46 at gmail.com>
> To: "Veronika Kabatova" <vkabatov at redhat.com>
> Cc: "crml" <criu at 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 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 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 at openvz.org
> > https://lists.openvz.org/mailman/listinfo/criu
>
>
> --
> Dmitry
>
More information about the CRIU
mailing list