Fix RPC configuration file handling

Submitted by Adrian Reber on Dec. 13, 2018, 10:46 a.m.

Details

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

Commit Message

Adrian Reber Dec. 13, 2018, 10:46 a.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.

Signed-off-by: Adrian Reber <areber@redhat.com>
---
 criu/cr-service.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

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;

Comments

Andrei Vagin Dec. 13, 2018, 6:40 p.m.
On Thu, Dec 13, 2018 at 2:46 AM Adrian Reber <adrian@lisas.de> 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.

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

>
> Signed-off-by: Adrian Reber <areber@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
>
Adrian Reber Dec. 14, 2018, 8:38 a.m.
On Thu, Dec 13, 2018 at 10:40:52AM -0800, Andrei Vagin wrote:
> On Thu, Dec 13, 2018 at 2:46 AM Adrian Reber <adrian@lisas.de> 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.
> 
> Do you know what we have to do to be sure that the codes works as expected? ;)

Good question ;) I will add a test to test/other/rpc to make sure it
works as expected right now and that it will not break in the future. As
the whole RPC configuration file is new it is probably good that we are
now working on runc to figure out how it should actually work.

Do you want the test case being part of this patch or is it okay to send
a new test case independent of this patch (once it is merged).

		Adrian
Andrei Vagin Dec. 14, 2018, 5:39 p.m.
On Thu, Dec 13, 2018 at 10:46:33AM +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.
> 
> Signed-off-by: Adrian Reber <areber@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);



tmp_output is a copy of opts.output:
tmp_output = xstrdup(opts.output);

Why do we need xstrdup() here? Maybe something like this:

opts.output = tmp_output;
tmp_output = NULL;


BTW: xstrdup can fail, so we have to check its return value.


> +
> +		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)) {

output_changed_by_rpc_conf is true only if opts.output isn't NULL

> +		/*
> +		 * 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
>
Adrian Reber Dec. 17, 2018, 1:18 p.m.
On Fri, Dec 14, 2018 at 09:39:19AM -0800, Andrei Vagin wrote:
> On Thu, Dec 13, 2018 at 10:46:33AM +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.
> > 
> > Signed-off-by: Adrian Reber <areber@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);
> 
> 
> 
> tmp_output is a copy of opts.output:
> tmp_output = xstrdup(opts.output);
> 
> Why do we need xstrdup() here? Maybe something like this:
> 
> opts.output = tmp_output;
> tmp_output = NULL;

Correct, that is enough.

> BTW: xstrdup can fail, so we have to check its return value.

Really? I thought that is why we have the macro:

#define __xalloc(op, size, ...)                                         \
        ({                                                              \
                void *___p = op( __VA_ARGS__ );                         \
                if (!___p)                                              \
                        pr_err("%s: Can't allocate %li bytes\n",        \
                               __func__, (long)(size));                 \
                ___p;                                                   \
        })

#define xstrdup(str)            __xalloc(strdup, strlen(str) + 1, str)


> > +
> > +		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)) {
> 
> output_changed_by_rpc_conf is true only if opts.output isn't NULL

Also, correct.

> > +		/*
> > +		 * 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
> >
Andrei Vagin Dec. 17, 2018, 5:20 p.m.
On Mon, Dec 17, 2018 at 02:18:28PM +0100, Adrian Reber wrote:
> On Fri, Dec 14, 2018 at 09:39:19AM -0800, Andrei Vagin wrote:
> > On Thu, Dec 13, 2018 at 10:46:33AM +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.
> > > 
> > > Signed-off-by: Adrian Reber <areber@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);
> > 
> > 
> > 
> > tmp_output is a copy of opts.output:
> > tmp_output = xstrdup(opts.output);
> > 
> > Why do we need xstrdup() here? Maybe something like this:
> > 
> > opts.output = tmp_output;
> > tmp_output = NULL;
> 
> Correct, that is enough.
> 
> > BTW: xstrdup can fail, so we have to check its return value.
> 
> Really? I thought that is why we have the macro:
> 
> #define __xalloc(op, size, ...)                                         \
>         ({                                                              \
>                 void *___p = op( __VA_ARGS__ );                         \
>                 if (!___p)                                              \
>                         pr_err("%s: Can't allocate %li bytes\n",        \
>                                __func__, (long)(size));                 \
>                 ___p;                                                   \
>         })
> 
> #define xstrdup(str)            __xalloc(strdup, strlen(str) + 1, str)

You don't need to print an error, but you still need to handle it...

> 
> 
> > > +
> > > +		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)) {
> > 
> > output_changed_by_rpc_conf is true only if opts.output isn't NULL
> 
> Also, correct.
> 
> > > +		/*
> > > +		 * 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
> > >