[CRIU] [PATCH 2/2] Do not error out in RPC mode with wrong config file entries

[Adrian Reber]

On Wed, Dec 05, 2018 at 09:52:34AM +0000, Radostin Stoyanov wrote:
> On 30/11/2018 13:36, Adrian Reber wrote:
> > So this fails in CI because we have a test to make sure RPC correctly
> > detects wrong configuration file entries:
> >
> > $ ./config_file.py /tmp
> > Connecting to CRIU in swrk mode.
> > FAIL: CRIU should have returned 1 instead of -9
> >
> > So, not sure if we actually want to skip wrong entries in the
> > configuration file. Any preferences? How should we handle wrong entries
> > in configuration files in RPC mode?
> One example where invalid configuration file entries are not ignored is
> sshd.
> For instance, if we set an invalid option in /etc/ssh/sshd_config,
> followed by
> "systemctl restart sshd" will cause sshd to fail, and therefore users
> will no longer
> be able to access the server via ssh. I find this behaviour very annoying.
> 
> I can imagine a similar example with CRIU. If a user is trying to live
> migrate a
> container but an invalid option has been set in CRIU's config file, the
> container
> migration is going to fail.
> 
> IMHO a more "user friendly" behaviour would be to show a warning in the log
> file rather than fail.

Sounds good. I will remove the failing test and resend the patch series.

		Adrian

> > On Fri, Nov 30, 2018 at 11:51:37AM +0000, Adrian Reber wrote:
> >> From: Adrian Reber <areber at redhat.com>
> >>
> >> Relates: https://github.com/checkpoint-restore/criu/issues/578
> >>
> >> If the config parser finds a unknown option in the configuration file,
> >> the wrong option is printed out and CRIU exits.
> >>
> >> In RPC mode this is not the best thing to do, as CRIU might not be able
> >> to print the message to the user.
> >>
> >> This changes CRIU's behaviour in RPC mode to write a wrong configuration
> >> option to the log file. In CLI mode nothing changes:
> >>
> >> $ echo test >> /etc/criu/default.conf
> >> $ criu check
> >> criu: unrecognized option '--test'
> >> $ runc checkpoint <container>
> >> $ grep Unknown dump.log
> >> Warn  (criu/config.c:812): Unknown option encountered: --test
> >> $ echo test-runc >> /etc/criu/runc.conf
> >> $ runc restore -d <container>
> >> $ grep Unknown restore.log
> >> Warn  (criu/config.c:812): Unknown option encountered: --test
> >> Warn  (criu/config.c:812): Unknown option encountered: --test-runc
> >>
> >> This way unknown configuration file entries do not break RPC mode, but
> >> they are reported.
> >>
> >> Reported-by: Radostin Stoyanov <rstoyanov1 at gmail.com>
> >> Signed-off-by: Adrian Reber <areber at redhat.com>
> >> ---
> >>  criu/config.c | 38 ++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 38 insertions(+)
> >>
> >> diff --git a/criu/config.c b/criu/config.c
> >> index f4fb39b..3c54fa7 100644
> >> --- a/criu/config.c
> >> +++ b/criu/config.c
> >> @@ -31,6 +31,8 @@
> >>  
> >>  struct cr_options opts;
> >>  
> >> +static bool rpc_mode = false;
> >> +
> >>  static int count_elements(char **to_count)
> >>  {
> >>  	int count = 0;
> >> @@ -235,6 +237,20 @@ static int pre_parse(int argc, char **argv, bool *usage_error, bool *no_default_
> >>  		} else if (strstr(argv[i], "--config=") != NULL) {
> >>  			*cfg_file = argv[i] + strlen("--config=");
> >>  			*no_default_config = true;
> >> +		} else if (!strcmp(argv[i], "swrk")) {
> >> +			/*
> >> +			 * In RPC mode we do not want to error out if we
> >> +			 * encounter unknown options. The options can only
> >> +			 * be from a configuration file. To not error out
> >> +			 * because of wrong lines in the configuration file
> >> +			 * this just prints the wrong option into the log.
> >> +			 */
> >> +			rpc_mode = true;
> >> +			/*
> >> +			 * This is only needed so that getopt() does not
> >> +			 * print invalid options to stderr.
> >> +			 */
> >> +			opterr = 0;
> >>  		}
> >>  	}
> >>  
> >> @@ -787,6 +803,28 @@ int parse_options(int argc, char **argv, bool *usage_error,
> >>  		case 'h':
> >>  			*usage_error = false;
> >>  			return 2;
> >> +		case '?':
> >> +			/*
> >> +			 * In RPC mode we do not want to
> >> +			 * error out if an unknown option is found.
> >> +			 * This writes it to the log file and continues.
> >> +			 */
> >> +			if (rpc_mode) {
> >> +				pr_warn("Unknown option encountered: %s\n", _argv[optind - 1]);
> >> +				break;
> >> +			} else {
> >> +				/*
> >> +				 * Only an unknown option that starts with '-' needs to be
> >> +				 * reported to the user. getopt() knows nothing about our
> >> +				 * commands (dump, check, swrk, ...). Those should be
> >> +				 * ignored.
> >> +				 */
> >> +				if (_argv[optind - 1][0] == '-') {
> >> +					*usage_error = true;
> >> +					return 2;
> >> +				}
> >> +			}
> >> +			break;
> >>  		default:
> >>  			return 2;
> >>  		}
> >> -- 
> >> 1.8.3.1
> >>
> >> _______________________________________________
> >> CRIU mailing list
> >> CRIU at openvz.org
> >> https://lists.openvz.org/mailman/listinfo/criu
> > 		Adrian