[RESEND,v3,3/7] RPC: Use configuration file also from RPC

Submitted by Adrian Reber on July 10, 2018, 9:39 a.m.

Details

Message ID 1531215570-27770-3-git-send-email-adrian@lisas.de
State New
Series "Series without cover letter"
Headers show

Commit Message

Adrian Reber July 10, 2018, 9:39 a.m.
From: Adrian Reber <areber@redhat.com>

This enables the usage of configuration file values in the RPC code
path. The main change is to verify if the user has actually changed a
configuration option in the configuration file and if it has been
changed the value configured via RPC is ignored.

As it is not possible to detect if a configuration option has the
default value because it has not been changed or set by the user or if
the user actually set it to the default value a copy of the options
structure (opt_changed_by_config) is used to track if a configuration
option has been changed by the user. This structure does not contain
that actual value of the configuration option it only tracks if an
option has been set. The value is still in the original options
structure (opts).

With this it is now possible to override CRIU's behavior if started via
RPC. If the user sets a value in one of the configuration files the
value coming from RPC is ignored. To avoid undesired and undefined
behavior the RPC client has to explicitly enable that configuration
files are overwriting RPC options by setting 'prefer_config_file' to
true.

Signed-off-by: Adrian Reber <areber@redhat.com>
---
 criu/cr-service.c | 154 +++++++++++++++++++++++++++++++++---------------------
 images/rpc.proto  |   2 +
 2 files changed, 97 insertions(+), 59 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/cr-service.c b/criu/cr-service.c
index 3a07d5b..9c612f9 100644
--- a/criu/cr-service.c
+++ b/criu/cr-service.c
@@ -232,6 +232,31 @@  int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
 	return 0;
 }
 
+static bool prefer_config_file = false;
+
+static void set_log_level(int config_log_level, int rpc_log_level)
+{
+	if (opt_changed_by_config.log_level && prefer_config_file) {
+		log_set_loglevel(config_log_level);
+		return;
+	}
+	log_set_loglevel(rpc_log_level);
+}
+
+static void set_opts_char(char **config, char *rpc)
+{
+	/* First check if it is already set by the configuration file. */
+	if (*config && prefer_config_file) {
+		pr_info("Overwriting RPC value %s with configuration file "
+			"value %s\n", rpc, *config);
+		return;
+	}
+
+	/* Check if a value was given via RPC and set it. */
+	if (rpc)
+		*config = rpc;
+}
+
 static char images_dir[PATH_MAX];
 
 static int setup_opts_from_req(int sk, CriuOpts *req)
@@ -257,11 +282,27 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 	BUG_ON(st.st_ino == -1);
 	service_sk_ino = st.st_ino;
 
+	/*
+	 * If the RPC client requests to ignore the default config file
+	 * the structure which has the information about the config file
+	 * changes is just zeroed out.
+	 */
+	if (req->has_no_default_config && req->no_default_config)
+		memset(&opt_changed_by_config, 0, sizeof(opt_changed_by_config));
+	/*
+	 * The client explicitly tells us to use the values from
+	 * the configuration file. This can override options
+	 * specified via RPC. This can lead to undesired behavior
+	 * if the configuration file has settings which are different
+	 * than what the RPC client actually wanted to do.
+	 */
+	if (req->has_prefer_config_file && req->prefer_config_file)
+		prefer_config_file = true;
+
 	/* open images_dir */
 	sprintf(images_dir_path, "/proc/%d/fd/%d", ids.pid, req->images_dir_fd);
 
-	if (req->parent_img)
-		opts.img_parent = req->parent_img;
+	set_opts_char(&opts.img_parent, req->parent_img);
 
 	if (open_image_dir(images_dir_path) < 0) {
 		pr_perror("Can't open images directory");
@@ -291,12 +332,11 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 			pr_perror("No subdirs are allowed in log_file name");
 			goto err;
 		}
+	}
+	set_opts_char(&opts.output, req->log_file);
 
-		opts.output = req->log_file;
-	} else
-		opts.output = DEFAULT_LOG_FILENAME;
+	set_log_level(opts.log_level, req->log_level);
 
-	log_set_loglevel(req->log_level);
 	if (log_init(opts.output) == -1) {
 		pr_perror("Can't initiate log");
 		goto err;
@@ -308,7 +348,8 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 	}
 
 	/* checking flags from client */
-	if (req->has_leave_running && req->leave_running)
+	if (req->has_leave_running && req->leave_running &&
+	    !((opt_changed_by_config.final_state == 1) && prefer_config_file))
 		opts.final_state = TASK_ALIVE;
 
 	if (!req->has_pid) {
@@ -316,7 +357,7 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 		req->pid = ids.pid;
 	}
 
-	if (req->has_ext_unix_sk) {
+	if (req->has_ext_unix_sk && !(opt_changed_by_config.ext_unix_sk && prefer_config_file)) {
 		opts.ext_unix_sk = req->ext_unix_sk;
 		for (i = 0; i < req->n_unix_sk_ino; i++) {
 			if (unix_sk_id_add((unsigned int)req->unix_sk_ino[i]->inode) < 0)
@@ -324,10 +365,9 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 		}
 	}
 
-	if (req->root)
-		opts.root = req->root;
+	set_opts_char(&opts.root, req->root);
 
-	if (req->has_rst_sibling) {
+	if (req->has_rst_sibling && !(opt_changed_by_config.restore_sibling && prefer_config_file)) {
 		if (!opts.swrk_restore) {
 			pr_err("rst_sibling is not allowed in standalone service\n");
 			goto err;
@@ -336,35 +376,35 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 		opts.restore_sibling = req->rst_sibling;
 	}
 
-	if (req->has_tcp_established)
-		opts.tcp_established_ok = req->tcp_established;
+#define _REALLY_USE_RPC_VALUE(option, rpc) do { \
+	if (req->has_##rpc && !(opt_changed_by_config.option && prefer_config_file)) { \
+		opts.option = req->rpc; \
+	} \
+	if (opt_changed_by_config.option && prefer_config_file) \
+		pr_info("Using %s from configuration file\n", #option); \
+} while (0)
 
-	if (req->has_tcp_skip_in_flight)
-		opts.tcp_skip_in_flight = req->tcp_skip_in_flight;
+#define REALLY_USE_RPC_VALUE(option) _REALLY_USE_RPC_VALUE(option, option)
 
-	if (req->has_weak_sysctls)
-		opts.weak_sysctls = req->weak_sysctls;
+	_REALLY_USE_RPC_VALUE(tcp_established_ok, tcp_established);
 
-	if (req->has_evasive_devices)
-		opts.evasive_devices = req->evasive_devices;
+	REALLY_USE_RPC_VALUE(tcp_skip_in_flight);
 
-	if (req->has_shell_job)
-		opts.shell_job = req->shell_job;
+	REALLY_USE_RPC_VALUE(weak_sysctls);
 
-	if (req->has_file_locks)
-		opts.handle_file_locks = req->file_locks;
+	REALLY_USE_RPC_VALUE(evasive_devices);
 
-	if (req->has_track_mem)
-		opts.track_mem = req->track_mem;
+	REALLY_USE_RPC_VALUE(shell_job);
 
-	if (req->has_link_remap)
-		opts.link_remap_ok = req->link_remap;
+	_REALLY_USE_RPC_VALUE(handle_file_locks, file_locks);
 
-	if (req->has_auto_dedup)
-		opts.auto_dedup = req->auto_dedup;
+	REALLY_USE_RPC_VALUE(track_mem);
 
-	if (req->has_force_irmap)
-		opts.force_irmap = req->force_irmap;
+	_REALLY_USE_RPC_VALUE(link_remap_ok, link_remap);
+
+	REALLY_USE_RPC_VALUE(auto_dedup);
+
+	REALLY_USE_RPC_VALUE(force_irmap);
 
 	if (req->n_exec_cmd > 0) {
 		opts.exec_cmd = xmalloc((req->n_exec_cmd + 1) * sizeof(char *));
@@ -372,16 +412,16 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 		opts.exec_cmd[req->n_exec_cmd] = NULL;
 	}
 
-	if (req->has_lazy_pages) {
-		opts.lazy_pages = req->lazy_pages;
-	}
+	REALLY_USE_RPC_VALUE(lazy_pages);
 
-	if (req->ps) {
-		opts.port = (short)req->ps->port;
+	if (req->ps || opts.use_page_server) {
+		if (!(opt_changed_by_config.port && prefer_config_file))
+			opts.port = (short)req->ps->port;
 
 		if (!opts.lazy_pages) {
 			opts.use_page_server = true;
-			opts.addr = req->ps->address;
+			if (!(opt_changed_by_config.addr && prefer_config_file))
+				opts.addr = req->ps->address;
 
 			if (req->ps->has_fd) {
 				if (!opts.swrk_restore)
@@ -439,8 +479,7 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 			goto err;
 	}
 
-	if (req->has_cpu_cap)
-		opts.cpu_cap = req->cpu_cap;
+	REALLY_USE_RPC_VALUE(cpu_cap);
 
 	/*
 	 * FIXME: For backward compatibility we setup
@@ -448,11 +487,11 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 	 * other modes as well via separate option
 	 * probably.
 	 */
-	if (req->has_manage_cgroups)
+	if (req->has_manage_cgroups && !(opt_changed_by_config.manage_cgroups && prefer_config_file))
 		opts.manage_cgroups = req->manage_cgroups ? CG_MODE_SOFT : CG_MODE_IGNORE;
 
 	/* Override the manage_cgroup if mode is set explicitly */
-	if (req->has_manage_cgroups_mode) {
+	if (req->has_manage_cgroups_mode && !(opt_changed_by_config.manage_cgroups && prefer_config_file)) {
 		unsigned int mode;
 
 		switch (req->manage_cgroups_mode) {
@@ -484,36 +523,33 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 		opts.manage_cgroups = mode;
 	}
 
-	if (req->freeze_cgroup)
-		opts.freeze_cgroup = req->freeze_cgroup;
+	set_opts_char(&opts.freeze_cgroup, req->freeze_cgroup);
 
-	if (req->has_timeout)
-		opts.timeout = req->timeout;
+	REALLY_USE_RPC_VALUE(timeout);
 
-	if (req->cgroup_props)
-		opts.cgroup_props = req->cgroup_props;
+	set_opts_char(&opts.cgroup_props, req->cgroup_props);
 
-	if (req->cgroup_props_file)
-		opts.cgroup_props_file = req->cgroup_props_file;
+	set_opts_char(&opts.cgroup_props_file, req->cgroup_props_file);
 
 	for (i = 0; i < req->n_cgroup_dump_controller; i++) {
 		if (!cgp_add_dump_controller(req->cgroup_dump_controller[i]))
 			goto err;
 	}
 
-	if (req->has_auto_ext_mnt)
-		opts.autodetect_ext_mounts = req->auto_ext_mnt;
+	_REALLY_USE_RPC_VALUE(autodetect_ext_mounts, auto_ext_mnt);
+
+	_REALLY_USE_RPC_VALUE(enable_external_sharing, ext_sharing);
+
+	_REALLY_USE_RPC_VALUE(enable_external_masters, ext_masters);
 
-	if (req->has_ext_sharing)
-		opts.enable_external_sharing = req->ext_sharing;
+	_REALLY_USE_RPC_VALUE(enable_external_masters, ext_masters);
 
-	if (req->has_ext_masters)
-		opts.enable_external_masters = req->ext_masters;
+	REALLY_USE_RPC_VALUE(ghost_limit);
 
-	if (req->has_ghost_limit)
-		opts.ghost_limit = req->ghost_limit;
+#undef REALLY_USE_RPC_VALUE
+#undef _REALLY_USE_RPC_VALUE
 
-	if (req->has_empty_ns) {
+	if (req->has_empty_ns && !(opt_changed_by_config.empty_ns && prefer_config_file)) {
 		opts.empty_ns = req->empty_ns;
 		if (req->empty_ns & ~(CLONE_NEWNET))
 			goto err;
@@ -526,7 +562,7 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 		}
 	}
 
-	if (req->has_status_fd) {
+	if (req->has_status_fd && !(opt_changed_by_config.status_fd && prefer_config_file)) {
 		sprintf(status_fd, "/proc/%d/fd/%d", ids.pid, req->status_fd);
 		opts.status_fd = open(status_fd, O_WRONLY);
 		if (opts.status_fd < 0)
diff --git a/images/rpc.proto b/images/rpc.proto
index 71f47d5..33ef330 100644
--- a/images/rpc.proto
+++ b/images/rpc.proto
@@ -111,6 +111,8 @@  message criu_opts {
 	optional bool			lazy_pages		= 48;
 	optional int32			status_fd		= 49;
 	optional bool			orphan_pts_master	= 50;
+	optional bool			no_default_config	= 51;
+	optional bool			prefer_config_file	= 52;
 }
 
 message criu_dump_resp {

Comments

Andrei Vagin July 11, 2018, 6:45 p.m.
On Tue, Jul 10, 2018 at 09:39:26AM +0000, Adrian Reber wrote:
> From: Adrian Reber <areber@redhat.com>
> 
> This enables the usage of configuration file values in the RPC code
> path. The main change is to verify if the user has actually changed a
> configuration option in the configuration file and if it has been
> changed the value configured via RPC is ignored.
> 
> As it is not possible to detect if a configuration option has the
> default value because it has not been changed or set by the user or if
> the user actually set it to the default value a copy of the options
> structure (opt_changed_by_config) is used to track if a configuration
> option has been changed by the user. This structure does not contain
> that actual value of the configuration option it only tracks if an
> option has been set. The value is still in the original options
> structure (opts).
> 
> With this it is now possible to override CRIU's behavior if started via
> RPC. If the user sets a value in one of the configuration files the
> value coming from RPC is ignored. To avoid undesired and undefined
> behavior the RPC client has to explicitly enable that configuration
> files are overwriting RPC options by setting 'prefer_config_file' to
> true.

I think all this logic with determining whether a value is a default one
or not are overcomplicated.

prefer_config_file should not be global. We have configs in /etc and a
user home directory which change default values.

Then we should have an option or options to specify configs which has to
be applied before rpc or command line options and after them.

struct options opts = {};

apply_config(global_conf)
apply_config(user_conf)
apply_config(pre_conf)
parse_command_line()
apply_rpc_options()
apply_config(post_conf)

Here on each step, we override options of a previous step.


> 
> Signed-off-by: Adrian Reber <areber@redhat.com>
> ---
>  criu/cr-service.c | 154 +++++++++++++++++++++++++++++++++---------------------
>  images/rpc.proto  |   2 +
>  2 files changed, 97 insertions(+), 59 deletions(-)
> 
> diff --git a/criu/cr-service.c b/criu/cr-service.c
> index 3a07d5b..9c612f9 100644
> --- a/criu/cr-service.c
> +++ b/criu/cr-service.c
> @@ -232,6 +232,31 @@ int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
>  	return 0;
>  }
>  
> +static bool prefer_config_file = false;
> +
> +static void set_log_level(int config_log_level, int rpc_log_level)
> +{
> +	if (opt_changed_by_config.log_level && prefer_config_file) {
> +		log_set_loglevel(config_log_level);
> +		return;
> +	}
> +	log_set_loglevel(rpc_log_level);
> +}
> +
> +static void set_opts_char(char **config, char *rpc)
> +{
> +	/* First check if it is already set by the configuration file. */
> +	if (*config && prefer_config_file) {
> +		pr_info("Overwriting RPC value %s with configuration file "
> +			"value %s\n", rpc, *config);
> +		return;
> +	}
> +
> +	/* Check if a value was given via RPC and set it. */
> +	if (rpc)
> +		*config = rpc;
> +}
> +
>  static char images_dir[PATH_MAX];
>  
>  static int setup_opts_from_req(int sk, CriuOpts *req)
> @@ -257,11 +282,27 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  	BUG_ON(st.st_ino == -1);
>  	service_sk_ino = st.st_ino;
>  
> +	/*
> +	 * If the RPC client requests to ignore the default config file
> +	 * the structure which has the information about the config file
> +	 * changes is just zeroed out.
> +	 */
> +	if (req->has_no_default_config && req->no_default_config)
> +		memset(&opt_changed_by_config, 0, sizeof(opt_changed_by_config));
> +	/*
> +	 * The client explicitly tells us to use the values from
> +	 * the configuration file. This can override options
> +	 * specified via RPC. This can lead to undesired behavior
> +	 * if the configuration file has settings which are different
> +	 * than what the RPC client actually wanted to do.
> +	 */
> +	if (req->has_prefer_config_file && req->prefer_config_file)
> +		prefer_config_file = true;
> +
>  	/* open images_dir */
>  	sprintf(images_dir_path, "/proc/%d/fd/%d", ids.pid, req->images_dir_fd);
>  
> -	if (req->parent_img)
> -		opts.img_parent = req->parent_img;
> +	set_opts_char(&opts.img_parent, req->parent_img);
>  
>  	if (open_image_dir(images_dir_path) < 0) {
>  		pr_perror("Can't open images directory");
> @@ -291,12 +332,11 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  			pr_perror("No subdirs are allowed in log_file name");
>  			goto err;
>  		}
> +	}
> +	set_opts_char(&opts.output, req->log_file);
>  
> -		opts.output = req->log_file;
> -	} else
> -		opts.output = DEFAULT_LOG_FILENAME;
> +	set_log_level(opts.log_level, req->log_level);
>  
> -	log_set_loglevel(req->log_level);
>  	if (log_init(opts.output) == -1) {
>  		pr_perror("Can't initiate log");
>  		goto err;
> @@ -308,7 +348,8 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  	}
>  
>  	/* checking flags from client */
> -	if (req->has_leave_running && req->leave_running)
> +	if (req->has_leave_running && req->leave_running &&
> +	    !((opt_changed_by_config.final_state == 1) && prefer_config_file))
>  		opts.final_state = TASK_ALIVE;
>  
>  	if (!req->has_pid) {
> @@ -316,7 +357,7 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  		req->pid = ids.pid;
>  	}
>  
> -	if (req->has_ext_unix_sk) {
> +	if (req->has_ext_unix_sk && !(opt_changed_by_config.ext_unix_sk && prefer_config_file)) {
>  		opts.ext_unix_sk = req->ext_unix_sk;
>  		for (i = 0; i < req->n_unix_sk_ino; i++) {
>  			if (unix_sk_id_add((unsigned int)req->unix_sk_ino[i]->inode) < 0)
> @@ -324,10 +365,9 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  		}
>  	}
>  
> -	if (req->root)
> -		opts.root = req->root;
> +	set_opts_char(&opts.root, req->root);
>  
> -	if (req->has_rst_sibling) {
> +	if (req->has_rst_sibling && !(opt_changed_by_config.restore_sibling && prefer_config_file)) {
>  		if (!opts.swrk_restore) {
>  			pr_err("rst_sibling is not allowed in standalone service\n");
>  			goto err;
> @@ -336,35 +376,35 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  		opts.restore_sibling = req->rst_sibling;
>  	}
>  
> -	if (req->has_tcp_established)
> -		opts.tcp_established_ok = req->tcp_established;
> +#define _REALLY_USE_RPC_VALUE(option, rpc) do { \
> +	if (req->has_##rpc && !(opt_changed_by_config.option && prefer_config_file)) { \
> +		opts.option = req->rpc; \
> +	} \
> +	if (opt_changed_by_config.option && prefer_config_file) \
> +		pr_info("Using %s from configuration file\n", #option); \
> +} while (0)
>  
> -	if (req->has_tcp_skip_in_flight)
> -		opts.tcp_skip_in_flight = req->tcp_skip_in_flight;
> +#define REALLY_USE_RPC_VALUE(option) _REALLY_USE_RPC_VALUE(option, option)
>  
> -	if (req->has_weak_sysctls)
> -		opts.weak_sysctls = req->weak_sysctls;
> +	_REALLY_USE_RPC_VALUE(tcp_established_ok, tcp_established);
>  
> -	if (req->has_evasive_devices)
> -		opts.evasive_devices = req->evasive_devices;
> +	REALLY_USE_RPC_VALUE(tcp_skip_in_flight);
>  
> -	if (req->has_shell_job)
> -		opts.shell_job = req->shell_job;
> +	REALLY_USE_RPC_VALUE(weak_sysctls);
>  
> -	if (req->has_file_locks)
> -		opts.handle_file_locks = req->file_locks;
> +	REALLY_USE_RPC_VALUE(evasive_devices);
>  
> -	if (req->has_track_mem)
> -		opts.track_mem = req->track_mem;
> +	REALLY_USE_RPC_VALUE(shell_job);
>  
> -	if (req->has_link_remap)
> -		opts.link_remap_ok = req->link_remap;
> +	_REALLY_USE_RPC_VALUE(handle_file_locks, file_locks);
>  
> -	if (req->has_auto_dedup)
> -		opts.auto_dedup = req->auto_dedup;
> +	REALLY_USE_RPC_VALUE(track_mem);
>  
> -	if (req->has_force_irmap)
> -		opts.force_irmap = req->force_irmap;
> +	_REALLY_USE_RPC_VALUE(link_remap_ok, link_remap);
> +
> +	REALLY_USE_RPC_VALUE(auto_dedup);
> +
> +	REALLY_USE_RPC_VALUE(force_irmap);
>  
>  	if (req->n_exec_cmd > 0) {
>  		opts.exec_cmd = xmalloc((req->n_exec_cmd + 1) * sizeof(char *));
> @@ -372,16 +412,16 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  		opts.exec_cmd[req->n_exec_cmd] = NULL;
>  	}
>  
> -	if (req->has_lazy_pages) {
> -		opts.lazy_pages = req->lazy_pages;
> -	}
> +	REALLY_USE_RPC_VALUE(lazy_pages);
>  
> -	if (req->ps) {
> -		opts.port = (short)req->ps->port;
> +	if (req->ps || opts.use_page_server) {
> +		if (!(opt_changed_by_config.port && prefer_config_file))
> +			opts.port = (short)req->ps->port;
>  
>  		if (!opts.lazy_pages) {
>  			opts.use_page_server = true;
> -			opts.addr = req->ps->address;
> +			if (!(opt_changed_by_config.addr && prefer_config_file))
> +				opts.addr = req->ps->address;
>  
>  			if (req->ps->has_fd) {
>  				if (!opts.swrk_restore)
> @@ -439,8 +479,7 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  			goto err;
>  	}
>  
> -	if (req->has_cpu_cap)
> -		opts.cpu_cap = req->cpu_cap;
> +	REALLY_USE_RPC_VALUE(cpu_cap);
>  
>  	/*
>  	 * FIXME: For backward compatibility we setup
> @@ -448,11 +487,11 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  	 * other modes as well via separate option
>  	 * probably.
>  	 */
> -	if (req->has_manage_cgroups)
> +	if (req->has_manage_cgroups && !(opt_changed_by_config.manage_cgroups && prefer_config_file))
>  		opts.manage_cgroups = req->manage_cgroups ? CG_MODE_SOFT : CG_MODE_IGNORE;
>  
>  	/* Override the manage_cgroup if mode is set explicitly */
> -	if (req->has_manage_cgroups_mode) {
> +	if (req->has_manage_cgroups_mode && !(opt_changed_by_config.manage_cgroups && prefer_config_file)) {
>  		unsigned int mode;
>  
>  		switch (req->manage_cgroups_mode) {
> @@ -484,36 +523,33 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  		opts.manage_cgroups = mode;
>  	}
>  
> -	if (req->freeze_cgroup)
> -		opts.freeze_cgroup = req->freeze_cgroup;
> +	set_opts_char(&opts.freeze_cgroup, req->freeze_cgroup);
>  
> -	if (req->has_timeout)
> -		opts.timeout = req->timeout;
> +	REALLY_USE_RPC_VALUE(timeout);
>  
> -	if (req->cgroup_props)
> -		opts.cgroup_props = req->cgroup_props;
> +	set_opts_char(&opts.cgroup_props, req->cgroup_props);
>  
> -	if (req->cgroup_props_file)
> -		opts.cgroup_props_file = req->cgroup_props_file;
> +	set_opts_char(&opts.cgroup_props_file, req->cgroup_props_file);
>  
>  	for (i = 0; i < req->n_cgroup_dump_controller; i++) {
>  		if (!cgp_add_dump_controller(req->cgroup_dump_controller[i]))
>  			goto err;
>  	}
>  
> -	if (req->has_auto_ext_mnt)
> -		opts.autodetect_ext_mounts = req->auto_ext_mnt;
> +	_REALLY_USE_RPC_VALUE(autodetect_ext_mounts, auto_ext_mnt);
> +
> +	_REALLY_USE_RPC_VALUE(enable_external_sharing, ext_sharing);
> +
> +	_REALLY_USE_RPC_VALUE(enable_external_masters, ext_masters);
>  
> -	if (req->has_ext_sharing)
> -		opts.enable_external_sharing = req->ext_sharing;
> +	_REALLY_USE_RPC_VALUE(enable_external_masters, ext_masters);
>  
> -	if (req->has_ext_masters)
> -		opts.enable_external_masters = req->ext_masters;
> +	REALLY_USE_RPC_VALUE(ghost_limit);
>  
> -	if (req->has_ghost_limit)
> -		opts.ghost_limit = req->ghost_limit;
> +#undef REALLY_USE_RPC_VALUE
> +#undef _REALLY_USE_RPC_VALUE
>  
> -	if (req->has_empty_ns) {
> +	if (req->has_empty_ns && !(opt_changed_by_config.empty_ns && prefer_config_file)) {
>  		opts.empty_ns = req->empty_ns;
>  		if (req->empty_ns & ~(CLONE_NEWNET))
>  			goto err;
> @@ -526,7 +562,7 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  		}
>  	}
>  
> -	if (req->has_status_fd) {
> +	if (req->has_status_fd && !(opt_changed_by_config.status_fd && prefer_config_file)) {
>  		sprintf(status_fd, "/proc/%d/fd/%d", ids.pid, req->status_fd);
>  		opts.status_fd = open(status_fd, O_WRONLY);
>  		if (opts.status_fd < 0)
> diff --git a/images/rpc.proto b/images/rpc.proto
> index 71f47d5..33ef330 100644
> --- a/images/rpc.proto
> +++ b/images/rpc.proto
> @@ -111,6 +111,8 @@ message criu_opts {
>  	optional bool			lazy_pages		= 48;
>  	optional int32			status_fd		= 49;
>  	optional bool			orphan_pts_master	= 50;
> +	optional bool			no_default_config	= 51;
> +	optional bool			prefer_config_file	= 52;
>  }
>  
>  message criu_dump_resp {
> -- 
> 1.8.3.1
>
Andrei Vagin July 12, 2018, 6:15 p.m.
On Wed, Jul 11, 2018 at 11:45:33AM -0700, Andrei Vagin wrote:
> On Tue, Jul 10, 2018 at 09:39:26AM +0000, Adrian Reber wrote:
> > From: Adrian Reber <areber@redhat.com>
> > 
> > This enables the usage of configuration file values in the RPC code
> > path. The main change is to verify if the user has actually changed a
> > configuration option in the configuration file and if it has been
> > changed the value configured via RPC is ignored.
> > 
> > As it is not possible to detect if a configuration option has the
> > default value because it has not been changed or set by the user or if
> > the user actually set it to the default value a copy of the options
> > structure (opt_changed_by_config) is used to track if a configuration
> > option has been changed by the user. This structure does not contain
> > that actual value of the configuration option it only tracks if an
> > option has been set. The value is still in the original options
> > structure (opts).
> > 
> > With this it is now possible to override CRIU's behavior if started via
> > RPC. If the user sets a value in one of the configuration files the
> > value coming from RPC is ignored. To avoid undesired and undefined
> > behavior the RPC client has to explicitly enable that configuration
> > files are overwriting RPC options by setting 'prefer_config_file' to
> > true.
> 
> I think all this logic with determining whether a value is a default one
> or not are overcomplicated.
> 
> prefer_config_file should not be global. We have configs in /etc and a
> user home directory which change default values.
> 
> Then we should have an option or options to specify configs which has to
> be applied before rpc or command line options and after them.
> 
> struct options opts = {};
> 
> apply_config(global_conf)
> apply_config(user_conf)
> apply_config(pre_conf)
> parse_command_line()
> apply_rpc_options()
> apply_config(post_conf)
> 
> Here on each step, we override options of a previous step.

What I mean is that we need to apply configs one by one and we will not
need to know what value was a default one. Adrian, do I miss something?
I feel that we have some miscommunication here. If you think so too, we
can schedule a meeting to discuss this in a real time.

> 
> 
> > 
> > Signed-off-by: Adrian Reber <areber@redhat.com>
> > ---
> >  criu/cr-service.c | 154 +++++++++++++++++++++++++++++++++---------------------
> >  images/rpc.proto  |   2 +
> >  2 files changed, 97 insertions(+), 59 deletions(-)
> > 
> > diff --git a/criu/cr-service.c b/criu/cr-service.c
> > index 3a07d5b..9c612f9 100644
> > --- a/criu/cr-service.c
> > +++ b/criu/cr-service.c
> > @@ -232,6 +232,31 @@ int send_criu_rpc_script(enum script_actions act, char *name, int sk, int fd)
> >  	return 0;
> >  }
> >  
> > +static bool prefer_config_file = false;
> > +
> > +static void set_log_level(int config_log_level, int rpc_log_level)
> > +{
> > +	if (opt_changed_by_config.log_level && prefer_config_file) {
> > +		log_set_loglevel(config_log_level);
> > +		return;
> > +	}
> > +	log_set_loglevel(rpc_log_level);
> > +}
> > +
> > +static void set_opts_char(char **config, char *rpc)
> > +{
> > +	/* First check if it is already set by the configuration file. */
> > +	if (*config && prefer_config_file) {
> > +		pr_info("Overwriting RPC value %s with configuration file "
> > +			"value %s\n", rpc, *config);
> > +		return;
> > +	}
> > +
> > +	/* Check if a value was given via RPC and set it. */
> > +	if (rpc)
> > +		*config = rpc;
> > +}
> > +
> >  static char images_dir[PATH_MAX];
> >  
> >  static int setup_opts_from_req(int sk, CriuOpts *req)
> > @@ -257,11 +282,27 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  	BUG_ON(st.st_ino == -1);
> >  	service_sk_ino = st.st_ino;
> >  
> > +	/*
> > +	 * If the RPC client requests to ignore the default config file
> > +	 * the structure which has the information about the config file
> > +	 * changes is just zeroed out.
> > +	 */
> > +	if (req->has_no_default_config && req->no_default_config)
> > +		memset(&opt_changed_by_config, 0, sizeof(opt_changed_by_config));
> > +	/*
> > +	 * The client explicitly tells us to use the values from
> > +	 * the configuration file. This can override options
> > +	 * specified via RPC. This can lead to undesired behavior
> > +	 * if the configuration file has settings which are different
> > +	 * than what the RPC client actually wanted to do.
> > +	 */
> > +	if (req->has_prefer_config_file && req->prefer_config_file)
> > +		prefer_config_file = true;
> > +
> >  	/* open images_dir */
> >  	sprintf(images_dir_path, "/proc/%d/fd/%d", ids.pid, req->images_dir_fd);
> >  
> > -	if (req->parent_img)
> > -		opts.img_parent = req->parent_img;
> > +	set_opts_char(&opts.img_parent, req->parent_img);
> >  
> >  	if (open_image_dir(images_dir_path) < 0) {
> >  		pr_perror("Can't open images directory");
> > @@ -291,12 +332,11 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  			pr_perror("No subdirs are allowed in log_file name");
> >  			goto err;
> >  		}
> > +	}
> > +	set_opts_char(&opts.output, req->log_file);
> >  
> > -		opts.output = req->log_file;
> > -	} else
> > -		opts.output = DEFAULT_LOG_FILENAME;
> > +	set_log_level(opts.log_level, req->log_level);
> >  
> > -	log_set_loglevel(req->log_level);
> >  	if (log_init(opts.output) == -1) {
> >  		pr_perror("Can't initiate log");
> >  		goto err;
> > @@ -308,7 +348,8 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  	}
> >  
> >  	/* checking flags from client */
> > -	if (req->has_leave_running && req->leave_running)
> > +	if (req->has_leave_running && req->leave_running &&
> > +	    !((opt_changed_by_config.final_state == 1) && prefer_config_file))
> >  		opts.final_state = TASK_ALIVE;
> >  
> >  	if (!req->has_pid) {
> > @@ -316,7 +357,7 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  		req->pid = ids.pid;
> >  	}
> >  
> > -	if (req->has_ext_unix_sk) {
> > +	if (req->has_ext_unix_sk && !(opt_changed_by_config.ext_unix_sk && prefer_config_file)) {
> >  		opts.ext_unix_sk = req->ext_unix_sk;
> >  		for (i = 0; i < req->n_unix_sk_ino; i++) {
> >  			if (unix_sk_id_add((unsigned int)req->unix_sk_ino[i]->inode) < 0)
> > @@ -324,10 +365,9 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  		}
> >  	}
> >  
> > -	if (req->root)
> > -		opts.root = req->root;
> > +	set_opts_char(&opts.root, req->root);
> >  
> > -	if (req->has_rst_sibling) {
> > +	if (req->has_rst_sibling && !(opt_changed_by_config.restore_sibling && prefer_config_file)) {
> >  		if (!opts.swrk_restore) {
> >  			pr_err("rst_sibling is not allowed in standalone service\n");
> >  			goto err;
> > @@ -336,35 +376,35 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  		opts.restore_sibling = req->rst_sibling;
> >  	}
> >  
> > -	if (req->has_tcp_established)
> > -		opts.tcp_established_ok = req->tcp_established;
> > +#define _REALLY_USE_RPC_VALUE(option, rpc) do { \
> > +	if (req->has_##rpc && !(opt_changed_by_config.option && prefer_config_file)) { \
> > +		opts.option = req->rpc; \
> > +	} \
> > +	if (opt_changed_by_config.option && prefer_config_file) \
> > +		pr_info("Using %s from configuration file\n", #option); \
> > +} while (0)
> >  
> > -	if (req->has_tcp_skip_in_flight)
> > -		opts.tcp_skip_in_flight = req->tcp_skip_in_flight;
> > +#define REALLY_USE_RPC_VALUE(option) _REALLY_USE_RPC_VALUE(option, option)
> >  
> > -	if (req->has_weak_sysctls)
> > -		opts.weak_sysctls = req->weak_sysctls;
> > +	_REALLY_USE_RPC_VALUE(tcp_established_ok, tcp_established);
> >  
> > -	if (req->has_evasive_devices)
> > -		opts.evasive_devices = req->evasive_devices;
> > +	REALLY_USE_RPC_VALUE(tcp_skip_in_flight);
> >  
> > -	if (req->has_shell_job)
> > -		opts.shell_job = req->shell_job;
> > +	REALLY_USE_RPC_VALUE(weak_sysctls);
> >  
> > -	if (req->has_file_locks)
> > -		opts.handle_file_locks = req->file_locks;
> > +	REALLY_USE_RPC_VALUE(evasive_devices);
> >  
> > -	if (req->has_track_mem)
> > -		opts.track_mem = req->track_mem;
> > +	REALLY_USE_RPC_VALUE(shell_job);
> >  
> > -	if (req->has_link_remap)
> > -		opts.link_remap_ok = req->link_remap;
> > +	_REALLY_USE_RPC_VALUE(handle_file_locks, file_locks);
> >  
> > -	if (req->has_auto_dedup)
> > -		opts.auto_dedup = req->auto_dedup;
> > +	REALLY_USE_RPC_VALUE(track_mem);
> >  
> > -	if (req->has_force_irmap)
> > -		opts.force_irmap = req->force_irmap;
> > +	_REALLY_USE_RPC_VALUE(link_remap_ok, link_remap);
> > +
> > +	REALLY_USE_RPC_VALUE(auto_dedup);
> > +
> > +	REALLY_USE_RPC_VALUE(force_irmap);
> >  
> >  	if (req->n_exec_cmd > 0) {
> >  		opts.exec_cmd = xmalloc((req->n_exec_cmd + 1) * sizeof(char *));
> > @@ -372,16 +412,16 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  		opts.exec_cmd[req->n_exec_cmd] = NULL;
> >  	}
> >  
> > -	if (req->has_lazy_pages) {
> > -		opts.lazy_pages = req->lazy_pages;
> > -	}
> > +	REALLY_USE_RPC_VALUE(lazy_pages);
> >  
> > -	if (req->ps) {
> > -		opts.port = (short)req->ps->port;
> > +	if (req->ps || opts.use_page_server) {
> > +		if (!(opt_changed_by_config.port && prefer_config_file))
> > +			opts.port = (short)req->ps->port;
> >  
> >  		if (!opts.lazy_pages) {
> >  			opts.use_page_server = true;
> > -			opts.addr = req->ps->address;
> > +			if (!(opt_changed_by_config.addr && prefer_config_file))
> > +				opts.addr = req->ps->address;
> >  
> >  			if (req->ps->has_fd) {
> >  				if (!opts.swrk_restore)
> > @@ -439,8 +479,7 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  			goto err;
> >  	}
> >  
> > -	if (req->has_cpu_cap)
> > -		opts.cpu_cap = req->cpu_cap;
> > +	REALLY_USE_RPC_VALUE(cpu_cap);
> >  
> >  	/*
> >  	 * FIXME: For backward compatibility we setup
> > @@ -448,11 +487,11 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  	 * other modes as well via separate option
> >  	 * probably.
> >  	 */
> > -	if (req->has_manage_cgroups)
> > +	if (req->has_manage_cgroups && !(opt_changed_by_config.manage_cgroups && prefer_config_file))
> >  		opts.manage_cgroups = req->manage_cgroups ? CG_MODE_SOFT : CG_MODE_IGNORE;
> >  
> >  	/* Override the manage_cgroup if mode is set explicitly */
> > -	if (req->has_manage_cgroups_mode) {
> > +	if (req->has_manage_cgroups_mode && !(opt_changed_by_config.manage_cgroups && prefer_config_file)) {
> >  		unsigned int mode;
> >  
> >  		switch (req->manage_cgroups_mode) {
> > @@ -484,36 +523,33 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  		opts.manage_cgroups = mode;
> >  	}
> >  
> > -	if (req->freeze_cgroup)
> > -		opts.freeze_cgroup = req->freeze_cgroup;
> > +	set_opts_char(&opts.freeze_cgroup, req->freeze_cgroup);
> >  
> > -	if (req->has_timeout)
> > -		opts.timeout = req->timeout;
> > +	REALLY_USE_RPC_VALUE(timeout);
> >  
> > -	if (req->cgroup_props)
> > -		opts.cgroup_props = req->cgroup_props;
> > +	set_opts_char(&opts.cgroup_props, req->cgroup_props);
> >  
> > -	if (req->cgroup_props_file)
> > -		opts.cgroup_props_file = req->cgroup_props_file;
> > +	set_opts_char(&opts.cgroup_props_file, req->cgroup_props_file);
> >  
> >  	for (i = 0; i < req->n_cgroup_dump_controller; i++) {
> >  		if (!cgp_add_dump_controller(req->cgroup_dump_controller[i]))
> >  			goto err;
> >  	}
> >  
> > -	if (req->has_auto_ext_mnt)
> > -		opts.autodetect_ext_mounts = req->auto_ext_mnt;
> > +	_REALLY_USE_RPC_VALUE(autodetect_ext_mounts, auto_ext_mnt);
> > +
> > +	_REALLY_USE_RPC_VALUE(enable_external_sharing, ext_sharing);
> > +
> > +	_REALLY_USE_RPC_VALUE(enable_external_masters, ext_masters);
> >  
> > -	if (req->has_ext_sharing)
> > -		opts.enable_external_sharing = req->ext_sharing;
> > +	_REALLY_USE_RPC_VALUE(enable_external_masters, ext_masters);
> >  
> > -	if (req->has_ext_masters)
> > -		opts.enable_external_masters = req->ext_masters;
> > +	REALLY_USE_RPC_VALUE(ghost_limit);
> >  
> > -	if (req->has_ghost_limit)
> > -		opts.ghost_limit = req->ghost_limit;
> > +#undef REALLY_USE_RPC_VALUE
> > +#undef _REALLY_USE_RPC_VALUE
> >  
> > -	if (req->has_empty_ns) {
> > +	if (req->has_empty_ns && !(opt_changed_by_config.empty_ns && prefer_config_file)) {
> >  		opts.empty_ns = req->empty_ns;
> >  		if (req->empty_ns & ~(CLONE_NEWNET))
> >  			goto err;
> > @@ -526,7 +562,7 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
> >  		}
> >  	}
> >  
> > -	if (req->has_status_fd) {
> > +	if (req->has_status_fd && !(opt_changed_by_config.status_fd && prefer_config_file)) {
> >  		sprintf(status_fd, "/proc/%d/fd/%d", ids.pid, req->status_fd);
> >  		opts.status_fd = open(status_fd, O_WRONLY);
> >  		if (opts.status_fd < 0)
> > diff --git a/images/rpc.proto b/images/rpc.proto
> > index 71f47d5..33ef330 100644
> > --- a/images/rpc.proto
> > +++ b/images/rpc.proto
> > @@ -111,6 +111,8 @@ message criu_opts {
> >  	optional bool			lazy_pages		= 48;
> >  	optional int32			status_fd		= 49;
> >  	optional bool			orphan_pts_master	= 50;
> > +	optional bool			no_default_config	= 51;
> > +	optional bool			prefer_config_file	= 52;
> >  }
> >  
> >  message criu_dump_resp {
> > -- 
> > 1.8.3.1
> > 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu