[v2] Fix RPC configuration file handling

Submitted by Adrian Reber on Dec. 18, 2018, 4:43 p.m.

Details

Message ID 1545151436-11764-1-git-send-email-adrian@lisas.de
State New
Series "Fix RPC configuration file handling"
Headers show

Commit Message

Adrian Reber Dec. 18, 2018, 4:43 p.m.
From: Adrian Reber <areber@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.

At the same time this changes/fixes the test. The relevant test case
test_rpc_with_configuration_file_overwriting_rpc() was actually designed
around the broken behaviour. It was only working if a previous
configuration file (set via environment variable in this case) and the
RPC configuration file have the same name. The test case which tests
that RPC configuration file settings are overwriting direct RPC settings
now makes sure that no other configuration file is set via the
environment variable. If it would be set, the test case would still
succeed, even with this patch applied. Which is and which was the
correct behaviour.

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

Signed-off-by: Adrian Reber <areber@redhat.com>
---
 criu/cr-service.c              | 60 ++++++++++++++++++++++++++++++++++--------
 test/others/rpc/config_file.py | 30 ++++++++++++++++-----
 2 files changed, 72 insertions(+), 18 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/cr-service.c b/criu/cr-service.c
index 4a9983a..ca3558d 100644
--- a/criu/cr-service.c
+++ b/criu/cr-service.c
@@ -271,12 +271,21 @@  static int setup_opts_from_req(int sk, CriuOpts *req)
 		char *tmp_work = NULL;
 		char *tmp_imgs = NULL;
 
-		if (opts.output)
+		if (opts.output) {
 			tmp_output = xstrdup(opts.output);
-		if (opts.work_dir)
+			if (!tmp_output)
+				goto err;
+		}
+		if (opts.work_dir) {
 			tmp_work = xstrdup(opts.work_dir);
-		if (opts.imgs_dir)
+			if (!tmp_work)
+				goto err;
+		}
+		if (opts.imgs_dir) {
 			tmp_imgs = xstrdup(opts.imgs_dir);
+			if (!tmp_imgs)
+				goto err;
+		}
 		xfree(opts.output);
 		xfree(opts.work_dir);
 		xfree(opts.imgs_dir);
@@ -292,23 +301,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 +345,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 +365,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 +384,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;
diff --git a/test/others/rpc/config_file.py b/test/others/rpc/config_file.py
index f21d194..4cf204f 100755
--- a/test/others/rpc/config_file.py
+++ b/test/others/rpc/config_file.py
@@ -33,10 +33,24 @@  def setup_config_file(content):
 
 
 def cleanup_config_file(path):
-	del os.environ['CRIU_CONFIG_FILE']
+	try:
+		del os.environ['CRIU_CONFIG_FILE']
+	except:
+		pass
 	os.unlink(path)
 
 
+def cleanup_output(path):
+	try:
+		os.unlink(os.path.join(path, does_not_exist))
+	except OSError:
+		pass
+	try:
+		os.unlink(os.path.join(path, log_file))
+	except OSError:
+		pass
+
+
 def setup_criu_dump_request():
 	# Create criu msg, set it's type to dump request
 	# and set dump options. Checkout more options in protobuf/rpc.proto
@@ -146,6 +160,9 @@  def test_rpc_with_configuration_file_overwriting_rpc():
 	content = 'log-file ' + log + '\n'
 	content += 'no-tcp-established\nno-shell-job'
 	path = setup_config_file(content)
+	# Only set the configuration file via RPC;
+	# not via environment variable
+	del os.environ['CRIU_CONFIG_FILE']
 	req = setup_criu_dump_request()
 	req.opts.config_file = path
 	_, s = setup_swrk()
@@ -160,14 +177,13 @@  parser.add_argument('dir', type = str, help = "Directory where CRIU images shoul
 
 args = vars(parser.parse_args())
 
-try:
-	# optional cleanup
-	os.unlink(os.path.join(args['dir'], does_not_exist))
-	os.unlink(os.path.join(args['dir'], log_file))
-except OSError:
-	pass
+cleanup_output(args['dir'])
 
 test_broken_configuration_file()
+cleanup_output(args['dir'])
 test_rpc_without_configuration_file()
+cleanup_output(args['dir'])
 test_rpc_with_configuration_file()
+cleanup_output(args['dir'])
 test_rpc_with_configuration_file_overwriting_rpc()
+cleanup_output(args['dir'])

Comments

Andrey Vagin Dec. 19, 2018, 7:28 a.m.
On Tue, Dec 18, 2018 at 04:43:56PM +0000, Adrian Reber wrote:
> From: Adrian Reber <areber@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.
> 
> At the same time this changes/fixes the test. The relevant test case
> test_rpc_with_configuration_file_overwriting_rpc() was actually designed
> around the broken behaviour. It was only working if a previous
> configuration file (set via environment variable in this case) and the
> RPC configuration file have the same name. The test case which tests
> that RPC configuration file settings are overwriting direct RPC settings
> now makes sure that no other configuration file is set via the
> environment variable. If it would be set, the test case would still
> succeed, even with this patch applied. Which is and which was the
> correct behaviour.
> 
> v2:
>   - fix existing test case to test better (more correct)
>   - make changes requested by Andrei
> 
> Signed-off-by: Adrian Reber <areber@redhat.com>
> ---
>  criu/cr-service.c              | 60 ++++++++++++++++++++++++++++++++++--------
>  test/others/rpc/config_file.py | 30 ++++++++++++++++-----
>  2 files changed, 72 insertions(+), 18 deletions(-)
> 
> diff --git a/criu/cr-service.c b/criu/cr-service.c
> index 4a9983a..ca3558d 100644
> --- a/criu/cr-service.c
> +++ b/criu/cr-service.c
> @@ -271,12 +271,21 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>  		char *tmp_work = NULL;
>  		char *tmp_imgs = NULL;
>  
> -		if (opts.output)
> +		if (opts.output) {
>  			tmp_output = xstrdup(opts.output);
> -		if (opts.work_dir)
> +			if (!tmp_output)
> +				goto err;
> +		}
> +		if (opts.work_dir) {
>  			tmp_work = xstrdup(opts.work_dir);
> -		if (opts.imgs_dir)
> +			if (!tmp_work)
> +				goto err;
> +		}
> +		if (opts.imgs_dir) {
>  			tmp_imgs = xstrdup(opts.imgs_dir);
> +			if (!tmp_imgs)
> +				goto err;
> +		}
>  		xfree(opts.output);

This looks weird. Why do we need this xstrdup and xfree? Maybe something
like this:

tmp_output = opts.output;
opts.output = NULL;


>  		xfree(opts.work_dir);
>  		xfree(opts.imgs_dir);
> @@ -292,23 +301,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 +345,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 +365,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 +384,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;
> diff --git a/test/others/rpc/config_file.py b/test/others/rpc/config_file.py

Pls, create a separate patch for test changes.

> index f21d194..4cf204f 100755
> --- a/test/others/rpc/config_file.py
> +++ b/test/others/rpc/config_file.py
> @@ -33,10 +33,24 @@ def setup_config_file(content):
>  
>  
>  def cleanup_config_file(path):
> -	del os.environ['CRIU_CONFIG_FILE']
> +	try:

Pls avoid handling unspecified exceptions. I usually prefer to avoid
handling exceptions where it is possible.

	if os.environ.get('CRIU_CONFIG_FILE', None) is not None:
> +		del os.environ['CRIU_CONFIG_FILE']

> +	except:
> +		pass
>  	os.unlink(path)
>  
>  
> +def cleanup_output(path):
> +	try:

You can do something like this:

	for f in (does_not_exist, log_file):
		f = os.path.join(path, f)
		if os.access(f, os.F_OK):

			os.unlink(f)
> +		os.unlink(os.path.join(path, does_not_exist))
> +	except OSError, e:
or you can check errno:
		if e.errno != ENOENT:
			raise
> +		pass
> +	try:
> +		os.unlink(os.path.join(path, log_file))
> +	except OSError:
> +		pass
> +
> +
>  def setup_criu_dump_request():
>  	# Create criu msg, set it's type to dump request
>  	# and set dump options. Checkout more options in protobuf/rpc.proto
> @@ -146,6 +160,9 @@ def test_rpc_with_configuration_file_overwriting_rpc():
>  	content = 'log-file ' + log + '\n'
>  	content += 'no-tcp-established\nno-shell-job'
>  	path = setup_config_file(content)
> +	# Only set the configuration file via RPC;
> +	# not via environment variable
> +	del os.environ['CRIU_CONFIG_FILE']
>  	req = setup_criu_dump_request()
>  	req.opts.config_file = path
>  	_, s = setup_swrk()
> @@ -160,14 +177,13 @@ parser.add_argument('dir', type = str, help = "Directory where CRIU images shoul
>  
>  args = vars(parser.parse_args())
>  
> -try:
> -	# optional cleanup
> -	os.unlink(os.path.join(args['dir'], does_not_exist))
> -	os.unlink(os.path.join(args['dir'], log_file))
> -except OSError:
> -	pass
> +cleanup_output(args['dir'])
>  
>  test_broken_configuration_file()
> +cleanup_output(args['dir'])
>  test_rpc_without_configuration_file()
> +cleanup_output(args['dir'])
>  test_rpc_with_configuration_file()
> +cleanup_output(args['dir'])
>  test_rpc_with_configuration_file_overwriting_rpc()
> +cleanup_output(args['dir'])
> -- 
> 1.8.3.1
>