[CRIU] [PATCH v2 2/4] Track which configuration options have been changed
Andrei Vagin
avagin at virtuozzo.com
Wed May 9 21:53:51 MSK 2018
On Wed, May 09, 2018 at 05:16:48PM +0000, Adrian Reber wrote:
> From: Adrian Reber <areber at redhat.com>
>
> For the upcoming RPC configuration file support, the RPC code path needs
> to know which values have been changed from the default.
>
> The reason for this is, that we do not want that values specified via
> RPC are overwriting user specified settings in the configuration file.
>
> For bool options is is not possible to tell if the value is the default
> value as a result of the configuration file or as a result of just being
> the default.
>
> For default values, which are not modified by configuration files we
> want to use the RPC specified value. If a user changes a value to the
> default in the configuration file we do not want to have it changed by
> the RPC code path.
>
> This means that values from the configuration file have a higher
> priority than values configured via RPC.
Should it be the same for command line criu options?
I have some doubts about such behaviour... Maybe we need to add a marker
for a config options, which will say how it should be applied. Pavel,
what do you think?
>
> The reason for this is, that we want to enable the user to change CRIU's
> behavior when embedded in a container runtime and used via RPC (e.g.
> runc).
>
> If an option is specified in the configuration file the values for that
> option are set to 1/true in the corresponding opt_changed_by_config.
>
> Now the RPC code path can check if an option has been changed via
> configuration file and can then ignore the value specified via RPC.
Will it be simpler just to run parsing of config options after handling rpc
options?
>
> Signed-off-by: Adrian Reber <areber at redhat.com>
> ---
> criu/crtools.c | 102 +++++++++++++++++++++++++++++++++-------------
> criu/include/cr_options.h | 11 +++++
> 2 files changed, 84 insertions(+), 29 deletions(-)
>
> diff --git a/criu/crtools.c b/criu/crtools.c
> index 27e54e0..4d91e51 100644
> --- a/criu/crtools.c
> +++ b/criu/crtools.c
> @@ -57,6 +57,7 @@
> #include "img-remote.h"
>
> struct cr_options opts;
> +struct cr_options opt_changed_by_config;
> char **global_conf = NULL;
> char **user_conf = NULL;
>
> @@ -273,8 +274,10 @@ int main(int argc, char *argv[], char *envp[])
> 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}
> + {OPT_NAME, no_argument, &opts.SAVE_TO, true},\
> + {OPT_NAME, no_argument, &opt_changed_by_config.SAVE_TO, false},\
> + {"no-" OPT_NAME, no_argument, &opts.SAVE_TO, false}, \
> + {"no-" OPT_NAME, no_argument, &opt_changed_by_config.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[] = {
> @@ -282,9 +285,9 @@ int main(int argc, char *argv[], char *envp[])
> { "pid", required_argument, 0, 'p' },
> { "leave-stopped", no_argument, 0, 's' },
> { "leave-running", no_argument, 0, 'R' },
> - BOOL_OPT("restore-detached", &opts.restore_detach),
> - BOOL_OPT("restore-sibling", &opts.restore_sibling),
> - BOOL_OPT("daemon", &opts.restore_detach),
> + BOOL_OPT("restore-detached", restore_detach),
> + BOOL_OPT("restore-sibling", restore_sibling),
> + BOOL_OPT("daemon", restore_detach),
> { "contents", no_argument, 0, 'c' },
> { "file", required_argument, 0, 'f' },
> { "fields", required_argument, 0, 'F' },
> @@ -295,27 +298,28 @@ int main(int argc, char *argv[], char *envp[])
> { "root", required_argument, 0, 'r' },
> { USK_EXT_PARAM, optional_argument, 0, 'x' },
> { "help", no_argument, 0, 'h' },
> - BOOL_OPT(SK_EST_PARAM, &opts.tcp_established_ok),
> + BOOL_OPT(SK_EST_PARAM, tcp_established_ok),
> + BOOL_OPT(SK_EST_PARAM, tcp_established_ok),
> { "close", required_argument, 0, 1043 },
> - BOOL_OPT("log-pid", &opts.log_file_per_pid),
> + BOOL_OPT("log-pid", log_file_per_pid),
> { "version", no_argument, 0, 'V' },
> - BOOL_OPT("evasive-devices", &opts.evasive_devices),
> + BOOL_OPT("evasive-devices", evasive_devices),
> { "pidfile", required_argument, 0, 1046 },
> { "veth-pair", required_argument, 0, 1047 },
> { "action-script", required_argument, 0, 1049 },
> - BOOL_OPT(LREMAP_PARAM, &opts.link_remap_ok),
> - BOOL_OPT(OPT_SHELL_JOB, &opts.shell_job),
> - BOOL_OPT(OPT_FILE_LOCKS, &opts.handle_file_locks),
> - BOOL_OPT("page-server", &opts.use_page_server),
> + BOOL_OPT(LREMAP_PARAM, link_remap_ok),
> + BOOL_OPT(OPT_SHELL_JOB, shell_job),
> + BOOL_OPT(OPT_FILE_LOCKS, handle_file_locks),
> + BOOL_OPT("page-server", use_page_server),
> { "address", required_argument, 0, 1051 },
> { "port", required_argument, 0, 1052 },
> { "prev-images-dir", required_argument, 0, 1053 },
> { "ms", no_argument, 0, 1054 },
> - BOOL_OPT("track-mem", &opts.track_mem),
> - BOOL_OPT("auto-dedup", &opts.auto_dedup),
> + BOOL_OPT("track-mem", track_mem),
> + BOOL_OPT("auto-dedup", auto_dedup),
> { "libdir", required_argument, 0, 'L' },
> { "cpu-cap", optional_argument, 0, 1057 },
> - BOOL_OPT("force-irmap", &opts.force_irmap),
> + BOOL_OPT("force-irmap", force_irmap),
> { "ext-mount-map", required_argument, 0, 'M' },
> { "exec-cmd", no_argument, 0, 1059 },
> { "manage-cgroups", optional_argument, 0, 1060 },
> @@ -324,8 +328,8 @@ int main(int argc, char *argv[], char *envp[])
> { "feature", required_argument, 0, 1063 },
> { "skip-mnt", required_argument, 0, 1064 },
> { "enable-fs", required_argument, 0, 1065 },
> - { "enable-external-sharing", no_argument, &opts.enable_external_sharing, true },
> - { "enable-external-masters", no_argument, &opts.enable_external_masters, true },
> + BOOL_OPT("enable-external-sharing", enable_external_sharing),
> + BOOL_OPT("enable-external-masters", enable_external_masters),
> { "freeze-cgroup", required_argument, 0, 1068 },
> { "ghost-limit", required_argument, 0, 1069 },
> { "irmap-scan-path", required_argument, 0, 1070 },
> @@ -334,21 +338,21 @@ int main(int argc, char *argv[], char *envp[])
> { "external", required_argument, 0, 1073 },
> { "empty-ns", required_argument, 0, 1074 },
> { "lazy-pages", no_argument, 0, 1076 },
> - BOOL_OPT("extra", &opts.check_extra_features),
> - BOOL_OPT("experimental", &opts.check_experimental_features),
> + BOOL_OPT("extra", check_extra_features),
> + BOOL_OPT("experimental", check_experimental_features),
> { "all", no_argument, 0, 1079 },
> { "cgroup-props", required_argument, 0, 1080 },
> { "cgroup-props-file", required_argument, 0, 1081 },
> { "cgroup-dump-controller", required_argument, 0, 1082 },
> - BOOL_OPT(SK_INFLIGHT_PARAM, &opts.tcp_skip_in_flight),
> - BOOL_OPT("deprecated", &opts.deprecated_ok),
> - BOOL_OPT("check-only", &opts.check_only),
> - BOOL_OPT("display-stats", &opts.display_stats),
> - BOOL_OPT("weak-sysctls", &opts.weak_sysctls),
> + BOOL_OPT(SK_INFLIGHT_PARAM, tcp_skip_in_flight),
> + BOOL_OPT("deprecated", deprecated_ok),
> + BOOL_OPT("check-only", check_only),
> + BOOL_OPT("display-stats", display_stats),
> + BOOL_OPT("weak-sysctls", weak_sysctls),
> { "status-fd", required_argument, 0, 1088 },
> - BOOL_OPT(SK_CLOSE_PARAM, &opts.tcp_close),
> + BOOL_OPT(SK_CLOSE_PARAM, tcp_close),
> { "verbosity", optional_argument, 0, 'v' },
> - BOOL_OPT("remote", &opts.remote),
> + BOOL_OPT("remote", remote),
> { "config", required_argument, 0, 1089},
> { "no-default-config", no_argument, 0, 1090},
> { },
> @@ -416,20 +420,54 @@ int main(int argc, char *argv[], char *envp[])
> break;
> }
> }
> - if (!opt)
> +
> + /* opt == 0 means getopt is directly writing the value to the destination */
> + if (opt == 0) {
> + /*
> + * Right now all bool options have a negated variant
> + * created automatically:
> + * --tcp-established and --no-tcp-established
> + * For the RPC code path to detect if the default value
> + * comes from the configuration file or because it is
> + * the default value, there is a second structure which
> + * is only used to check if an option has been changed via
> + * the getopt parser: opt_changed_by_config
> + * The following is the code to set those options in that
> + * structure.
> + */
> +
> + /*
> + * The macro BOOL_OPT() always adds the corresponding
> + * opt_changed_by_config at idx + 1. So by writing true
> + * to long_opts[idx + 1].flag we are telling the RPC
> + * code path that the user set a value (it might or might
> + * not be different from the default).
> + */
> +
> + /* Do no write beyond the long_opts array */
> + if (idx + 1 > sizeof(long_opts)/sizeof(long_opts[0]))
> + continue;
> +
> + *long_opts[idx + 1].flag = true;
> +
> + /* No need for further option analysis if opt == 0 */
> continue;
> + }
>
> switch (opt) {
> case 's':
> opts.final_state = TASK_STOPPED;
> + opt_changed_by_config.final_state = 1;
> break;
> case 'R':
> opts.final_state = TASK_ALIVE;
> + opt_changed_by_config.final_state = 1;
> break;
> case 'x':
> if (optarg && unix_sk_ids_parse(optarg) < 0)
> return 1;
> opts.ext_unix_sk = true;
> + opt_changed_by_config.ext_unix_sk = true;
> break;
> case 'p':
> pid = atoi(optarg);
> @@ -458,6 +496,7 @@ int main(int argc, char *argv[], char *envp[])
> break;
> case 'S':
> opts.restore_sibling = true;
> + opt_changed_by_config.restore_sibling = true;
> break;
> case 'D':
> imgs_dir = optarg;
> @@ -481,6 +520,7 @@ int main(int argc, char *argv[], char *envp[])
> opts.log_level = atoi(optarg);
> } else
> opts.log_level++;
> + opt_changed_by_config.log_level = 1;
> break;
> case 1043: {
> int fd;
> @@ -546,6 +586,7 @@ int main(int argc, char *argv[], char *envp[])
> case 1060:
> if (parse_manage_cgroups(&opts, optarg))
> goto usage;
> + opt_changed_by_config.manage_cgroups = 1;
> break;
> case 1061:
> {
> @@ -589,6 +630,7 @@ int main(int argc, char *argv[], char *envp[])
> break;
> case 1069:
> opts.ghost_limit = parse_size(optarg);
> + opt_changed_by_config.ghost_limit = 1;
> break;
> case 1070:
> if (irmap_scan_path_add(optarg))
> @@ -627,9 +669,10 @@ int main(int argc, char *argv[], char *envp[])
> return 1;
> break;
> case 1074:
> - if (!strcmp("net", optarg))
> + if (!strcmp("net", optarg)) {
> opts.empty_ns |= CLONE_NEWNET;
> - else {
> + opt_changed_by_config.empty_ns = 1;
> + } else {
> pr_err("Unsupported empty namespace: %s\n",
> optarg);
> return 1;
> @@ -654,6 +697,7 @@ int main(int argc, char *argv[], char *envp[])
> pr_err("Unable to parse a value of --status-fd\n");
> return 1;
> }
> + opt_changed_by_config.status_fd = 1;
> break;
> case 1089:
> break;
> diff --git a/criu/include/cr_options.h b/criu/include/cr_options.h
> index 1365b2e..72778c7 100644
> --- a/criu/include/cr_options.h
> +++ b/criu/include/cr_options.h
> @@ -128,6 +128,17 @@ struct cr_options {
>
> extern struct cr_options opts;
>
> +/*
> + * This is used to track if options have been changed manually from the
> + * default, or if it was manually set to the default value.
> + *
> + * In this copy of the actual used opts structure options changed by
> + * the configuration file or command-line arguments are set to '1' or 'true'
> + * so that the RPC code knows that this value should not be overwritten
> + * by the RPC caller.
> + */
> +extern struct cr_options opt_changed_by_config;
> +
> extern void init_opts(void);
>
> #endif /* __CR_OPTIONS_H__ */
> --
> 1.8.3.1
>
More information about the CRIU
mailing list