[CRIU] [PATCH v2] Fix RPC configuration file handling
Andrei Vagin
avagin at gmail.com
Wed Dec 19 10:28:42 MSK 2018
On Tue, Dec 18, 2018 at 04:43:56PM +0000, Adrian Reber wrote:
> 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);
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
>
More information about the CRIU
mailing list