[CRIU] [PATCH] Fix RPC configuration file handling

[Andrei Vagin]

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 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.
> > > 
> > > 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);
> > 
> > 
> > 
> > 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
> > >