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

Submitted by Adrian Reber on Nov. 30, 2018, 11:51 a.m.

Details

Message ID 1543578697-29198-2-git-send-email-adrian@lisas.de
State New
Series "Series without cover letter"
Headers show

Commit Message

Adrian Reber Nov. 30, 2018, 11:51 a.m.
From: Adrian Reber <areber@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@gmail.com>
Signed-off-by: Adrian Reber <areber@redhat.com>
---
 criu/config.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Patch hide | download patch | download mbox

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;
 		}

Comments

Adrian Reber Nov. 30, 2018, 1:36 p.m.
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?

		Adrian

On Fri, Nov 30, 2018 at 11:51:37AM +0000, Adrian Reber wrote:
> From: Adrian Reber <areber@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@gmail.com>
> Signed-off-by: Adrian Reber <areber@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@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu

		Adrian
Radostin Stoyanov Dec. 5, 2018, 9:52 a.m.
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@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@gmail.com>
>> Signed-off-by: Adrian Reber <areber@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@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
> 		Adrian
>
Adrian Reber Dec. 10, 2018, 1:12 p.m.
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@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@gmail.com>
> >> Signed-off-by: Adrian Reber <areber@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@openvz.org
> >> https://lists.openvz.org/mailman/listinfo/criu
> > 		Adrian