[CRIU] [PATCH v3 1/2] Fix RPC configuration file handling

[Adrian Reber]

From: Adrian Reber <areber at redhat.com>

While writing runc test cases to verify that runc correctly uses RPC
configuration files it became clear that some things were not working as
they are supposed to. Looking closer at the code to set log files
via RPC configuration files I discovered that the code seems wrong (at
least I did not understand it any more (or the intentions behind it)).

This code tries to simplify that logic a bit and add more comments to
make clear what the intentions of the RPC configuration file code is.

v2:
  - fix existing test case to test better (more correct)
  - make changes requested by Andrei
v3:
  - more changes as requested by Andrei

Signed-off-by: Adrian Reber <areber at redhat.com>
---
 criu/cr-service.c | 61 +++++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 20 deletions(-)

diff --git a/criu/cr-service.c b/criu/cr-service.c
index 4a9983ac7..532a87c81 100644
--- a/criu/cr-service.c
+++ b/criu/cr-service.c
@@ -267,22 +267,14 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
 	 * overwrites all options set via RPC.
 	 */
 	if (req->config_file) {
-		char *tmp_output = NULL;
-		char *tmp_work = NULL;
-		char *tmp_imgs = NULL;
+		char *tmp_output = opts.output;
+		char *tmp_work = opts.work_dir;
+		char *tmp_imgs = opts.imgs_dir;
 
-		if (opts.output)
-			tmp_output = xstrdup(opts.output);
-		if (opts.work_dir)
-			tmp_work = xstrdup(opts.work_dir);
-		if (opts.imgs_dir)
-			tmp_imgs = xstrdup(opts.imgs_dir);
-		xfree(opts.output);
-		xfree(opts.work_dir);
-		xfree(opts.imgs_dir);
 		opts.output = NULL;
 		opts.work_dir = NULL;
 		opts.imgs_dir = NULL;
+
 		rpc_cfg_file = req->config_file;
 		i = parse_options(0, NULL, &dummy, &dummy, PARSING_RPC_CONF);
 		pr_warn("parse_options returns %d\n", i);
@@ -292,23 +284,42 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
 			xfree(tmp_imgs);
 			goto err;
 		}
-		if (tmp_output && opts.output && !strncmp(tmp_output, opts.output, PATH_MAX))
+		/* If this is non-NULL, the RPC configuration file had a value, use it.*/
+		if (opts.output)
 			output_changed_by_rpc_conf = true;
-		if (tmp_work && opts.work_dir && !strncmp(tmp_work, opts.work_dir, PATH_MAX))
+		/* If this is NULL, use the old value if it was set. */
+		if (!opts.output && tmp_output) {
+			opts.output = tmp_output;
+			tmp_output = NULL;
+		}
+
+		if (opts.work_dir)
 			work_changed_by_rpc_conf = true;
-		if (tmp_imgs && opts.imgs_dir && !strncmp(tmp_imgs, opts.imgs_dir, PATH_MAX))
+		if (!opts.work_dir && tmp_work) {
+			opts.work_dir = tmp_work;
+			tmp_work = NULL;
+		}
+
+		if (opts.imgs_dir)
 			imgs_changed_by_rpc_conf = true;
+		/*
+		 * As the images directory is a required RPC setting, it is not
+		 * necessary to use the value from other configuration files.
+		 * Either it is set in the RPC configuration file or it is set
+		 * via RPC.
+		 */
 		xfree(tmp_output);
 		xfree(tmp_work);
 		xfree(tmp_imgs);
 	}
 
 	/*
-	 * open images_dir
+	 * open images_dir - images_dir_fd is a required RPC parameter
+	 *
 	 * This assumes that if opts.imgs_dir is set we have a value
 	 * from the configuration file parser. The test to see that
 	 * imgs_changed_by_rpc_conf is true is used to make sure the value
-	 * is not the same as from one of the other configuration files.
+	 * is from the RPC configuration file.
 	 * The idea is that only the RPC configuration file is able to
 	 * overwrite RPC settings:
 	 *  * apply_config(global_conf)
@@ -317,7 +328,7 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
 	 *  * apply_rpc_options()
 	 *  * apply_config(rpc_conf)
 	 */
-	if (opts.imgs_dir && imgs_changed_by_rpc_conf)
+	if (imgs_changed_by_rpc_conf)
 		strncpy(images_dir_path, opts.imgs_dir, PATH_MAX - 1);
 	else
 		sprintf(images_dir_path, "/proc/%d/fd/%d", ids.pid, req->images_dir_fd);
@@ -337,11 +348,17 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
 	}
 
 	/* chdir to work dir */
-	if (opts.work_dir && work_changed_by_rpc_conf)
+	if (work_changed_by_rpc_conf)
+		/* Use the value from the RPC configuration file first. */
 		strncpy(work_dir_path, opts.work_dir, PATH_MAX - 1);
 	else if (req->has_work_dir_fd)
+		/* Use the value set via RPC. */
 		sprintf(work_dir_path, "/proc/%d/fd/%d", ids.pid, req->work_dir_fd);
+	else if (opts.work_dir)
+		/* Use the value from one of the other configuration files. */
+		strncpy(work_dir_path, opts.work_dir, PATH_MAX - 1);
 	else
+		/* Use the images directory a work directory. */
 		strcpy(work_dir_path, images_dir_path);
 
 	if (chdir(work_dir_path)) {
@@ -350,7 +367,11 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
 	}
 
 	/* initiate log file in work dir */
-	if (req->log_file && !(opts.output  && output_changed_by_rpc_conf)) {
+	if (req->log_file && !output_changed_by_rpc_conf) {
+		/*
+		 * If RPC sets a log file and if there nothing from the
+		 * RPC configuration file, use the RPC value.
+		 */
 		if (strchr(req->log_file, '/')) {
 			pr_perror("No subdirs are allowed in log_file name");
 			goto err;
-- 
2.18.0