[v2,2/4] Track which configuration options have been changed

Submitted by Adrian Reber on May 9, 2018, 5:16 p.m.

Details

Message ID 1525886210-10621-2-git-send-email-adrian@lisas.de
State New
Series "Series without cover letter"
Headers show

Commit Message

Adrian Reber May 9, 2018, 5:16 p.m.
From: Adrian Reber <areber@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.

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.

Signed-off-by: Adrian Reber <areber@redhat.com>
---
 criu/crtools.c            | 102 +++++++++++++++++++++++++++++++++-------------
 criu/include/cr_options.h |  11 +++++
 2 files changed, 84 insertions(+), 29 deletions(-)

Patch hide | download patch | download mbox

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__ */

Comments

Andrey Vagin May 9, 2018, 6:53 p.m.
On Wed, May 09, 2018 at 05:16:48PM +0000, Adrian Reber wrote:
> From: Adrian Reber <areber@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@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
>
Adrian Reber May 10, 2018, 10:25 a.m.
On Wed, May 09, 2018 at 11:53:51AM -0700, Andrei Vagin wrote:
> On Wed, May 09, 2018 at 05:16:48PM +0000, Adrian Reber wrote:
> > From: Adrian Reber <areber@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?

This is the real complicated thing about the configuration file. I
discussed this a lot with Veronika when she implemented it and it was
never really clear what is the right thing to do. The current
implementation is basically what I would expect how a tool handles it
(this must not be correct).

For CLI the configuration files are parsed first, but the user can
overwrite it via command-line. For certain cases this makes sense as the
user might want to change the default behavior of the configuration
files.

Example: The package manager or the system administrator sets
verbosity=4. I as user do not like it and can disable it via CLI by
setting --verbosity=0 (or a configuration file in my home directory).
First configuration files are parsed and then the CLI options are
evaluated and it the end I get --verbosity=0 -> good.


On the other hand for cases like LXC, where LXC builds a
command-line and uses CRIU via CLI it does not make sense as it makes it
impossible to change CRIU's behavior.

Example: LXC sets '-vvvvv' and as user I want to change it to
'verbosity=0'. As CRIU's command-line is part of LXC C code I cannot
change it quickly and therefore I try to set it via configuration file
(verbosity=0). This however does not work as the configuration file is
parsed first and the CLI overrides it. As a user I do not get what I
want -> bad.


So for CLI the current implementation in criu-dev can be correct (real
CLI) or wrong (CLI embedded in C code like LXC). One could argue that
LXC usage of CRIU is not how it is supposed to be and that RPC would be
better, but that is the current situation.


So CLI: first configuration file then CLI

For RPC, I would expect it to be the other way. As user I cannot change
how RPC is used without changing the code. Therefore I would expect that
the configuration file overrides the setting via RPC for me to be able
to change the behavior.

Example: runc also sets verbosity to 4. As a user I do not want big log
files and I change it to 'verbosity=0' via configuration file. Result:
CRIU uses verbosity=0 as the configuration file overrides the RPC
parameters. -> good, as a user I get what I expect.

It is, however, just the other way as CLI. Which is probably bad as it
might confuse the user.

So it really is difficult to do it correctly.

> > 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?

Maybe, not sure. As we would need to run getopt() a second time. I
thought about it, but I was not sure. The problem is that we would need
to change the RPC parameters to a format which getopt() understands and
that also seemed unnecessary complicated.

The current configuration file code is nice that it just transforms the
configuration file to getopt() compatible input, on the other hand that
makes is complicated for the RPC case as we would need to create
getopt() compatible from the RPC options.

		Adrian

> > Signed-off-by: Adrian Reber <areber@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
> >
Andrey Vagin May 14, 2018, 6:40 p.m.
On Wed, May 09, 2018 at 11:53:51AM -0700, Andrei Vagin wrote:
> On Wed, May 09, 2018 at 05:16:48PM +0000, Adrian Reber wrote:
> > From: Adrian Reber <areber@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?

Pavel asked to send this e-mail again, so he can reply to it.
> 
> > 
> > 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@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
> > 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Pavel Emelyanov May 15, 2018, 11:21 a.m.
On 05/10/2018 01:25 PM, Adrian Reber wrote:
> On Wed, May 09, 2018 at 11:53:51AM -0700, Andrei Vagin wrote:
>> On Wed, May 09, 2018 at 05:16:48PM +0000, Adrian Reber wrote:
>>> From: Adrian Reber <areber@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?

Please, find my answers inline.

> This is the real complicated thing about the configuration file. I
> discussed this a lot with Veronika when she implemented it and it was
> never really clear what is the right thing to do. The current
> implementation is basically what I would expect how a tool handles it
> (this must not be correct).
> 
> For CLI the configuration files are parsed first, but the user can
> overwrite it via command-line. For certain cases this makes sense as the
> user might want to change the default behavior of the configuration
> files.
> 
> Example: The package manager or the system administrator sets
> verbosity=4. I as user do not like it and can disable it via CLI by
> setting --verbosity=0 (or a configuration file in my home directory).
> First configuration files are parsed and then the CLI options are
> evaluated and it the end I get --verbosity=0 -> good.
> 
> 
> On the other hand for cases like LXC, where LXC builds a
> command-line and uses CRIU via CLI it does not make sense as it makes it
> impossible to change CRIU's behavior.
> 
> Example: LXC sets '-vvvvv' and as user I want to change it to
> 'verbosity=0'. As CRIU's command-line is part of LXC C code I cannot
> change it quickly and therefore I try to set it via configuration file
> (verbosity=0). This however does not work as the configuration file is
> parsed first and the CLI overrides it. As a user I do not get what I
> want -> bad.
> 
> 
> So for CLI the current implementation in criu-dev can be correct (real
> CLI) or wrong (CLI embedded in C code like LXC). One could argue that
> LXC usage of CRIU is not how it is supposed to be and that RPC would be
> better, but that is the current situation.
> 
> 
> So CLI: first configuration file then CLI

Agreed, this is the sane policy for most of the tools out there -- the priority is to
read config file, fix it with environment vairables (if any), then apply CLI options.

> For RPC, I would expect it to be the other way. As user I cannot change
> how RPC is used without changing the code. Therefore I would expect that
> the configuration file overrides the setting via RPC for me to be able
> to change the behavior.
> 
> Example: runc also sets verbosity to 4. As a user I do not want big log
> files and I change it to 'verbosity=0' via configuration file. Result:
> CRIU uses verbosity=0 as the configuration file overrides the RPC
> parameters. -> good, as a user I get what I expect.
> 
> It is, however, just the other way as CLI. Which is probably bad as it
> might confuse the user.

Yup, this is the first thought that I got when saw Andrew's answer above -- RPC is just
another CLI, so it should go last in the priority chain above. However ... (more comments 
below).

> So it really is difficult to do it correctly.
> 
>>> 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?

It will, sure. There are tree (but we can do 4 some day) places where CRIU takes options
from -- config, CLI and RPC. And the sequence of these calls defines the priorities.

> Maybe, not sure. As we would need to run getopt() a second time. I
> thought about it, but I was not sure. The problem is that we would need
> to change the RPC parameters to a format which getopt() understands and
> that also seemed unnecessary complicated.
> 
> The current configuration file code is nice that it just transforms the
> configuration file to getopt() compatible input, on the other hand that
> makes is complicated for the RPC case as we would need to create
> getopt() compatible from the RPC options.

Well, yes. AFAIU the problem, we cannot easily change the way Docker (and others)
"configure" CRIU via RPC and want some way to by-pass the configuration parameter 
through it. Config files seem to suit this, but were designed to set some defaults
that are to be used if no other ... recommendations are given, not to nail down any
option.

Trying to make config file become docker-transparend-cli doesn't just "looks wrong",
it's also problematic since fixing a global config would affect every single CRIU 
invocation that might happen in between. To get a "CLI/RPC overrider" we need
something, that it local to a single CRIU invocation... maybe another "local" or
"one-shot" config file, that CRIU will read after reading the global one and parsing
CLI/RPC. But can we tell CRIU where this one-shot file is through docker/lxd call?

-- Pavel
Adrian Reber May 15, 2018, 5:38 p.m.
On Tue, May 15, 2018 at 02:21:49PM +0300, Pavel Emelyanov wrote:
> On 05/10/2018 01:25 PM, Adrian Reber wrote:
> > On Wed, May 09, 2018 at 11:53:51AM -0700, Andrei Vagin wrote:
> >> On Wed, May 09, 2018 at 05:16:48PM +0000, Adrian Reber wrote:
> >>> From: Adrian Reber <areber@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?
> 
> Please, find my answers inline.
> 
> > This is the real complicated thing about the configuration file. I
> > discussed this a lot with Veronika when she implemented it and it was
> > never really clear what is the right thing to do. The current
> > implementation is basically what I would expect how a tool handles it
> > (this must not be correct).
> > 
> > For CLI the configuration files are parsed first, but the user can
> > overwrite it via command-line. For certain cases this makes sense as the
> > user might want to change the default behavior of the configuration
> > files.
> > 
> > Example: The package manager or the system administrator sets
> > verbosity=4. I as user do not like it and can disable it via CLI by
> > setting --verbosity=0 (or a configuration file in my home directory).
> > First configuration files are parsed and then the CLI options are
> > evaluated and it the end I get --verbosity=0 -> good.
> > 
> > 
> > On the other hand for cases like LXC, where LXC builds a
> > command-line and uses CRIU via CLI it does not make sense as it makes it
> > impossible to change CRIU's behavior.
> > 
> > Example: LXC sets '-vvvvv' and as user I want to change it to
> > 'verbosity=0'. As CRIU's command-line is part of LXC C code I cannot
> > change it quickly and therefore I try to set it via configuration file
> > (verbosity=0). This however does not work as the configuration file is
> > parsed first and the CLI overrides it. As a user I do not get what I
> > want -> bad.
> > 
> > 
> > So for CLI the current implementation in criu-dev can be correct (real
> > CLI) or wrong (CLI embedded in C code like LXC). One could argue that
> > LXC usage of CRIU is not how it is supposed to be and that RPC would be
> > better, but that is the current situation.
> > 
> > 
> > So CLI: first configuration file then CLI
> 
> Agreed, this is the sane policy for most of the tools out there -- the priority is to
> read config file, fix it with environment vairables (if any), then apply CLI options.
> 
> > For RPC, I would expect it to be the other way. As user I cannot change
> > how RPC is used without changing the code. Therefore I would expect that
> > the configuration file overrides the setting via RPC for me to be able
> > to change the behavior.
> > 
> > Example: runc also sets verbosity to 4. As a user I do not want big log
> > files and I change it to 'verbosity=0' via configuration file. Result:
> > CRIU uses verbosity=0 as the configuration file overrides the RPC
> > parameters. -> good, as a user I get what I expect.
> > 
> > It is, however, just the other way as CLI. Which is probably bad as it
> > might confuse the user.
> 
> Yup, this is the first thought that I got when saw Andrew's answer above -- RPC is just
> another CLI, so it should go last in the priority chain above. However ... (more comments 
> below).
> 
> > So it really is difficult to do it correctly.
> > 
> >>> 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?
> 
> It will, sure. There are tree (but we can do 4 some day) places where CRIU takes options
> from -- config, CLI and RPC. And the sequence of these calls defines the priorities.
> 
> > Maybe, not sure. As we would need to run getopt() a second time. I
> > thought about it, but I was not sure. The problem is that we would need
> > to change the RPC parameters to a format which getopt() understands and
> > that also seemed unnecessary complicated.
> > 
> > The current configuration file code is nice that it just transforms the
> > configuration file to getopt() compatible input, on the other hand that
> > makes is complicated for the RPC case as we would need to create
> > getopt() compatible from the RPC options.
> 
> Well, yes. AFAIU the problem, we cannot easily change the way Docker (and others)
> "configure" CRIU via RPC and want some way to by-pass the configuration parameter 
> through it. Config files seem to suit this, but were designed to set some defaults
> that are to be used if no other ... recommendations are given, not to nail down any
> option.
> 
> Trying to make config file become docker-transparend-cli doesn't just "looks wrong",
> it's also problematic since fixing a global config would affect every single CRIU 
> invocation that might happen in between. To get a "CLI/RPC overrider" we need
> something, that it local to a single CRIU invocation... maybe another "local" or
> "one-shot" config file, that CRIU will read after reading the global one and parsing
> CLI/RPC. But can we tell CRIU where this one-shot file is through docker/lxd call?

Pavel, thanks for your comments, although I am not sure what you
propose. So for RPC you basically say we do not want to have
configuration files, right? So should we merge the CLI patches?
Everything concerning the CLI patches seems to be as everyone expects.

I guess I understand the 'one-shot' idea but I am also not sure how it
should work. Maybe you are also not sure ;) We are looking for mechanism
to influence LXD (which uses CLI) and docker/runc (which uses RPC). But
it should be different than configuration files, right?

		Adrian
Pavel Emelyanov May 17, 2018, 12:02 p.m.
>> Trying to make config file become docker-transparend-cli doesn't just "looks wrong",
>> it's also problematic since fixing a global config would affect every single CRIU 
>> invocation that might happen in between. To get a "CLI/RPC overrider" we need
>> something, that it local to a single CRIU invocation... maybe another "local" or
>> "one-shot" config file, that CRIU will read after reading the global one and parsing
>> CLI/RPC. But can we tell CRIU where this one-shot file is through docker/lxd call?
> 
> Pavel, thanks for your comments, although I am not sure what you
> propose. So for RPC you basically say we do not want to have
> configuration files, right? 

Not exactly. We may still use them, but in the "low prio" mode, i.e. the options
passed via RPC shoudl take priority over those read from config.

> So should we merge the CLI patches?

Yes, since ...

> Everything concerning the CLI patches seems to be as everyone expects.

... this :)

> I guess I understand the 'one-shot' idea but I am also not sure how it
> should work. Maybe you are also not sure ;) 

Yup :( I remember the problem, but still have no good answer for it.

> We are looking for mechanism> to influence LXD (which uses CLI) and docker/runc (which uses RPC). But
> it should be different than configuration files, right?

My idea was to introduce two config files for criu. The first one is exsting one, that
criu reads from known global place (/etc/...). Then criu parses CLI/RPC options and 
overrides values read from config. Then should go the 2nd config, which is formatted
the same way as the 1st one, but it sits not in some other place path to which is
somehow (I don't know how to pass this knowledge "through" docker/lxd) told to criu
by the caller. This 2nd config file will override options set via CLI/RPC. Also, since
the path to this 2nd config file is specified by the caller, if some other criu invocation
happens in parallel it will not erroneously pick this file's options.

-- Pavel
Adrian Reber May 18, 2018, 5:46 a.m.
On Thu, May 17, 2018 at 03:02:52PM +0300, Pavel Emelyanov wrote:
> 
> >> Trying to make config file become docker-transparend-cli doesn't just "looks wrong",
> >> it's also problematic since fixing a global config would affect every single CRIU 
> >> invocation that might happen in between. To get a "CLI/RPC overrider" we need
> >> something, that it local to a single CRIU invocation... maybe another "local" or
> >> "one-shot" config file, that CRIU will read after reading the global one and parsing
> >> CLI/RPC. But can we tell CRIU where this one-shot file is through docker/lxd call?
> > 
> > Pavel, thanks for your comments, although I am not sure what you
> > propose. So for RPC you basically say we do not want to have
> > configuration files, right? 
> 
> Not exactly. We may still use them, but in the "low prio" mode, i.e. the options
> passed via RPC shoudl take priority over those read from config.
> 
> > So should we merge the CLI patches?
> 
> Yes, since ...
> 
> > Everything concerning the CLI patches seems to be as everyone expects.
> 
> ... this :)
> 
> > I guess I understand the 'one-shot' idea but I am also not sure how it
> > should work. Maybe you are also not sure ;) 
> 
> Yup :( I remember the problem, but still have no good answer for it.
> 
> > We are looking for mechanism> to influence LXD (which uses CLI) and docker/runc (which uses RPC). But
> > it should be different than configuration files, right?
> 
> My idea was to introduce two config files for criu. The first one is exsting one, that
> criu reads from known global place (/etc/...). Then criu parses CLI/RPC options and 
> overrides values read from config. Then should go the 2nd config, which is formatted
> the same way as the 1st one, but it sits not in some other place path to which is
> somehow (I don't know how to pass this knowledge "through" docker/lxd) told to criu
> by the caller. This 2nd config file will override options set via CLI/RPC. Also, since
> the path to this 2nd config file is specified by the caller, if some other criu invocation
> happens in parallel it will not erroneously pick this file's options.

Sounds good. I would say we have no problem changing the latest LXC and
runc to adapt to this functionality. We already have "--config FILEPATH"
so we can tell CRIU to load a specific configuration file.

So should CRIU, in the RPC and CLI case, just behave differently if a
non-default location for the config file is specified? As soon as
CLI/RPC sees '--config FILEPATH' this config file is evaluated after the
CLI/RPC arguments, or do we want an additional parameter which switches
CRIU explicitly into config-file-overrides CLI/RPC parameters?

The good thing about being explicit is that it will not be a unexpected
behavior as the user has to explicitly request it. As we probably have
to change LXC and runc anyway it would be no problem to specify both
'--config FILEPATH' and '--override-rpc/cli'. We could also combine it
with something like '--override-rpc/cli-with FILEPATH'.

Is that something which makes sense?

		Adrian
Pavel Emelyanov May 18, 2018, 2:21 p.m.
On 05/18/2018 08:46 AM, Adrian Reber wrote:
> On Thu, May 17, 2018 at 03:02:52PM +0300, Pavel Emelyanov wrote:
>>
>>>> Trying to make config file become docker-transparend-cli doesn't just "looks wrong",
>>>> it's also problematic since fixing a global config would affect every single CRIU 
>>>> invocation that might happen in between. To get a "CLI/RPC overrider" we need
>>>> something, that it local to a single CRIU invocation... maybe another "local" or
>>>> "one-shot" config file, that CRIU will read after reading the global one and parsing
>>>> CLI/RPC. But can we tell CRIU where this one-shot file is through docker/lxd call?
>>>
>>> Pavel, thanks for your comments, although I am not sure what you
>>> propose. So for RPC you basically say we do not want to have
>>> configuration files, right? 
>>
>> Not exactly. We may still use them, but in the "low prio" mode, i.e. the options
>> passed via RPC shoudl take priority over those read from config.
>>
>>> So should we merge the CLI patches?
>>
>> Yes, since ...
>>
>>> Everything concerning the CLI patches seems to be as everyone expects.
>>
>> ... this :)
>>
>>> I guess I understand the 'one-shot' idea but I am also not sure how it
>>> should work. Maybe you are also not sure ;) 
>>
>> Yup :( I remember the problem, but still have no good answer for it.
>>
>>> We are looking for mechanism> to influence LXD (which uses CLI) and docker/runc (which uses RPC). But
>>> it should be different than configuration files, right?
>>
>> My idea was to introduce two config files for criu. The first one is exsting one, that
>> criu reads from known global place (/etc/...). Then criu parses CLI/RPC options and 
>> overrides values read from config. Then should go the 2nd config, which is formatted
>> the same way as the 1st one, but it sits not in some other place path to which is
>> somehow (I don't know how to pass this knowledge "through" docker/lxd) told to criu
>> by the caller. This 2nd config file will override options set via CLI/RPC. Also, since
>> the path to this 2nd config file is specified by the caller, if some other criu invocation
>> happens in parallel it will not erroneously pick this file's options.
> 
> Sounds good. I would say we have no problem changing the latest LXC and
> runc to adapt to this functionality. We already have "--config FILEPATH"
> so we can tell CRIU to load a specific configuration file.

That's good news :)

> So should CRIU, in the RPC and CLI case, just behave differently if a
> non-default location for the config file is specified? As soon as
> CLI/RPC sees '--config FILEPATH' this config file is evaluated after the
> CLI/RPC arguments, or do we want an additional parameter which switches
> CRIU explicitly into config-file-overrides CLI/RPC parameters?

I would say we need an additional parameter. Explicit is better than implicit.

> The good thing about being explicit is that it will not be a unexpected
> behavior as the user has to explicitly request it. As we probably have
> to change LXC and runc anyway it would be no problem to specify both
> '--config FILEPATH' and '--override-rpc/cli'. We could also combine it
> with something like '--override-rpc/cli-with FILEPATH'.
> 
> Is that something which makes sense?

Yup.

BTW, another option might be to create an RPC tunnel %) Like in criu_opts
we define an optional 'string more_opts' variable and teach ldx and runc
pass this string as-is from caller to criu. Criu then decodes this value
into criu_opts structure and overrides the options that has been just set
from config and RPC...

-- Pavel