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

Andrei Vagin avagin at virtuozzo.com
Thu Jul 12 21:15:54 MSK 2018


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 at 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 at 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 at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu


More information about the CRIU mailing list