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

[Radostin Stoyanov]

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.

Radostin
> 		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
>