[CRIU] [PATCH v2] Fix RPC configuration file handling
Adrian Reber
adrian at lisas.de
Tue Dec 18 19:43:56 MSK 2018
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.
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 at 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);
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'])
--
1.8.3.1
More information about the CRIU
mailing list