[CRIU] [PATCH] Fix RPC configuration file handling

[Andrei Vagin]

On Thu, Dec 13, 2018 at 2:46 AM Adrian Reber <adrian at lisas.de> 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.

Do you know what we have to do to be sure that the codes works as expected? ;)

>
> Signed-off-by: Adrian Reber <areber at redhat.com>
> ---
>  criu/cr-service.c | 37 +++++++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/criu/cr-service.c b/criu/cr-service.c
> index 4a9983a..61139d4 100644
> --- a/criu/cr-service.c
> +++ b/criu/cr-service.c
> @@ -292,23 +292,38 @@ 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 = xstrdup(tmp_output);
> +
> +               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 = xstrdup(tmp_work);
> +
> +               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)
> @@ -338,10 +353,16 @@ static int setup_opts_from_req(int sk, CriuOpts *req)
>
>         /* chdir to work dir */
>         if (opts.work_dir && 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 +371,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 && !(opts.output && 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;
> --
> 1.8.3.1
>