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

Submitted by Adrian Reber on Dec. 20, 2018, 4:18 p.m.

Details

Message ID 20181220161813.14555-2-adrian@lisas.de
State Accepted
Series "Series without cover letter"
Headers show

Commit Message

Adrian Reber Dec. 20, 2018, 4:18 p.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 f4fb39b86..3c54fa72a 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

Andrei Vagin Dec. 26, 2018, 9:42 p.m.
On Thu, Dec 20, 2018 at 04:18:12PM +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.

It is a bad explanation, because with your first patch the user will see
errors. I am not sure why we need to ignore unknown options in config
files. It looks dangerous, because the user can do a typo and will not
notice it.

> 
> 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 f4fb39b86..3c54fa72a 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;
>  		}
> -- 
> 2.18.0
>
Radostin Stoyanov Dec. 27, 2018, 2:07 p.m.
On 26/12/2018 21:42, Andrei Vagin wrote:
>> 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.
> It is a bad explanation, because with your first patch the user will see
> errors. I am not sure why we need to ignore unknown options in config
> files. It looks dangerous, because the user can do a typo and will not
> notice it.
>
A user could make a typo and they should be able to notice it in the log
file but
if CRIU exits with an error, this will break the checkpoint/restore
functionality of
container engines.

An example of a software that has such behaviour is sshd. If a user
makes a typo in
the /etc/ssh/sshd_config file, after "systemctl restart sshd", they will
no longer be
able to connect via ssh. Although, for security reasons, the behaviour
of sshd
might be appropriate, I think that in the case of CRIU, showing a
message, instead
of exiting with an error, would be more user-friendly.

Radostin
Andrei Vagin Dec. 28, 2018, 10:41 p.m.
On Thu, Dec 27, 2018 at 02:07:56PM +0000, Radostin Stoyanov wrote:
> On 26/12/2018 21:42, Andrei Vagin wrote:
> >> 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.
> > It is a bad explanation, because with your first patch the user will see
> > errors. I am not sure why we need to ignore unknown options in config
> > files. It looks dangerous, because the user can do a typo and will not
> > notice it.
> >
> A user could make a typo and they should be able to notice it in the log
> file but
> if CRIU exits with an error, this will break the checkpoint/restore
> functionality of
> container engines.
> 
> An example of a software that has such behaviour is sshd. If a user
> makes a typo in
> the /etc/ssh/sshd_config file, after "systemctl restart sshd", they will
> no longer be
> able to connect via ssh. Although, for security reasons, the behaviour
> of sshd
> might be appropriate, I think that in the case of CRIU, showing a
> message, instead
> of exiting with an error, would be more user-friendly.

Yes and no. I understand the sshd reasons, but they can be applied to
criu restore, but not to criu dump, IMHO.

Our goal is to not loose states of processes:
1. criu dump doesn't have to be destructive:
  * in case of any error, processes have to continue running
  * it is better to fail rather than dump wrong or incomplete state

2. sometimes we really may want to restore processes even if there is
some problem

> 
> Radostin
Radostin Stoyanov Dec. 28, 2018, 11:56 p.m.
On 28/12/2018 22:41, Andrei Vagin wrote:
> On Thu, Dec 27, 2018 at 02:07:56PM +0000, Radostin Stoyanov wrote:
>> On 26/12/2018 21:42, Andrei Vagin wrote:
>>>> 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.
>>> It is a bad explanation, because with your first patch the user will see
>>> errors. I am not sure why we need to ignore unknown options in config
>>> files. It looks dangerous, because the user can do a typo and will not
>>> notice it.
>>>
>> A user could make a typo and they should be able to notice it in the log
>> file but
>> if CRIU exits with an error, this will break the checkpoint/restore
>> functionality of
>> container engines.
>>
>> An example of a software that has such behaviour is sshd. If a user
>> makes a typo in
>> the /etc/ssh/sshd_config file, after "systemctl restart sshd", they will
>> no longer be
>> able to connect via ssh. Although, for security reasons, the behaviour
>> of sshd
>> might be appropriate, I think that in the case of CRIU, showing a
>> message, instead
>> of exiting with an error, would be more user-friendly.
> Yes and no. I understand the sshd reasons, but they can be applied to
> criu restore, but not to criu dump, IMHO.
>
> Our goal is to not loose states of processes:
> 1. criu dump doesn't have to be destructive:
>   * in case of any error, processes have to continue running
>   * it is better to fail rather than dump wrong or incomplete state
>
> 2. sometimes we really may want to restore processes even if there is
> some problem
That is a very good point, I agree with you.

Is it OK to close https://github.com/checkpoint-restore/criu/issues/578
with the explanation above?

Thanks,
Radostin
Andrei Vagin Jan. 2, 2019, 5:32 p.m.
On Fri, Dec 28, 2018 at 11:56:16PM +0000, Radostin Stoyanov wrote:
> On 28/12/2018 22:41, Andrei Vagin wrote:
> > On Thu, Dec 27, 2018 at 02:07:56PM +0000, Radostin Stoyanov wrote:
> >> On 26/12/2018 21:42, Andrei Vagin wrote:
> >>>> 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.
> >>> It is a bad explanation, because with your first patch the user will see
> >>> errors. I am not sure why we need to ignore unknown options in config
> >>> files. It looks dangerous, because the user can do a typo and will not
> >>> notice it.
> >>>
> >> A user could make a typo and they should be able to notice it in the log
> >> file but
> >> if CRIU exits with an error, this will break the checkpoint/restore
> >> functionality of
> >> container engines.
> >>
> >> An example of a software that has such behaviour is sshd. If a user
> >> makes a typo in
> >> the /etc/ssh/sshd_config file, after "systemctl restart sshd", they will
> >> no longer be
> >> able to connect via ssh. Although, for security reasons, the behaviour
> >> of sshd
> >> might be appropriate, I think that in the case of CRIU, showing a
> >> message, instead
> >> of exiting with an error, would be more user-friendly.
> > Yes and no. I understand the sshd reasons, but they can be applied to
> > criu restore, but not to criu dump, IMHO.
> >
> > Our goal is to not loose states of processes:
> > 1. criu dump doesn't have to be destructive:
> >   * in case of any error, processes have to continue running
> >   * it is better to fail rather than dump wrong or incomplete state
> >
> > 2. sometimes we really may want to restore processes even if there is
> > some problem
> That is a very good point, I agree with you.
> 
> Is it OK to close https://github.com/checkpoint-restore/criu/issues/578
> with the explanation above?

I would like to know what Adrian is thinking about this.
> 
> Thanks,
> Radostin
Adrian Reber Jan. 2, 2019, 5:36 p.m.
On Wed, Jan 02, 2019 at 09:32:09AM -0800, Andrei Vagin wrote:
> On Fri, Dec 28, 2018 at 11:56:16PM +0000, Radostin Stoyanov wrote:
> > On 28/12/2018 22:41, Andrei Vagin wrote:
> > > On Thu, Dec 27, 2018 at 02:07:56PM +0000, Radostin Stoyanov wrote:
> > >> On 26/12/2018 21:42, Andrei Vagin wrote:
> > >>>> 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.
> > >>> It is a bad explanation, because with your first patch the user will see
> > >>> errors. I am not sure why we need to ignore unknown options in config
> > >>> files. It looks dangerous, because the user can do a typo and will not
> > >>> notice it.
> > >>>
> > >> A user could make a typo and they should be able to notice it in the log
> > >> file but
> > >> if CRIU exits with an error, this will break the checkpoint/restore
> > >> functionality of
> > >> container engines.
> > >>
> > >> An example of a software that has such behaviour is sshd. If a user
> > >> makes a typo in
> > >> the /etc/ssh/sshd_config file, after "systemctl restart sshd", they will
> > >> no longer be
> > >> able to connect via ssh. Although, for security reasons, the behaviour
> > >> of sshd
> > >> might be appropriate, I think that in the case of CRIU, showing a
> > >> message, instead
> > >> of exiting with an error, would be more user-friendly.
> > > Yes and no. I understand the sshd reasons, but they can be applied to
> > > criu restore, but not to criu dump, IMHO.
> > >
> > > Our goal is to not loose states of processes:
> > > 1. criu dump doesn't have to be destructive:
> > >   * in case of any error, processes have to continue running
> > >   * it is better to fail rather than dump wrong or incomplete state
> > >
> > > 2. sometimes we really may want to restore processes even if there is
> > > some problem
> > That is a very good point, I agree with you.
> > 
> > Is it OK to close https://github.com/checkpoint-restore/criu/issues/578
> > with the explanation above?
> 
> I would like to know what Adrian is thinking about this.

The whole series was just trying to help with Radostin's 'bug'. But if
nobody sees this as a problem we can drop the whole thing.

		Adrian
Radostin Stoyanov Jan. 2, 2019, 6:14 p.m.
On 02/01/2019 17:36, Adrian Reber wrote:
>>>> Yes and no. I understand the sshd reasons, but they can be applied to
>>>> criu restore, but not to criu dump, IMHO.
>>>>
>>>> Our goal is to not loose states of processes:
>>>> 1. criu dump doesn't have to be destructive:
>>>>   * in case of any error, processes have to continue running
>>>>   * it is better to fail rather than dump wrong or incomplete state
>>>>
>>>> 2. sometimes we really may want to restore processes even if there is
>>>> some problem
>>> That is a very good point, I agree with you.
>>>
>>> Is it OK to close https://github.com/checkpoint-restore/criu/issues/578
>>> with the explanation above?
>> I would like to know what Adrian is thinking about this.
> The whole series was just trying to help with Radostin's 'bug'. But if
> nobody sees this as a problem we can drop the whole thing.
>
I understand that we want criu to exit with an error when we have
unknown options in config files, thus we can drop PATCH v5 2/3 and 3/3.

However, I think that PATCH v5 1/3 (Printout early log messages) is useful.
Is there a reason why we don't want this patch?

Radostin
Andrei Vagin Jan. 8, 2019, 8:15 p.m.
On Wed, Jan 02, 2019 at 06:14:23PM +0000, Radostin Stoyanov wrote:
> On 02/01/2019 17:36, Adrian Reber wrote:
> >>>> Yes and no. I understand the sshd reasons, but they can be applied to
> >>>> criu restore, but not to criu dump, IMHO.
> >>>>
> >>>> Our goal is to not loose states of processes:
> >>>> 1. criu dump doesn't have to be destructive:
> >>>>   * in case of any error, processes have to continue running
> >>>>   * it is better to fail rather than dump wrong or incomplete state
> >>>>
> >>>> 2. sometimes we really may want to restore processes even if there is
> >>>> some problem
> >>> That is a very good point, I agree with you.
> >>>
> >>> Is it OK to close https://github.com/checkpoint-restore/criu/issues/578
> >>> with the explanation above?
> >> I would like to know what Adrian is thinking about this.
> > The whole series was just trying to help with Radostin's 'bug'. But if
> > nobody sees this as a problem we can drop the whole thing.
> >
> I understand that we want criu to exit with an error when we have
> unknown options in config files, thus we can drop PATCH v5 2/3 and 3/3.
> 
> However, I think that PATCH v5 1/3 (Printout early log messages) is useful.
> Is there a reason why we don't want this patch?

We want it!

> 
> Radostin
Adrian Reber Jan. 8, 2019, 9:12 p.m.
On Tue, Jan 08, 2019 at 12:15:57PM -0800, Andrei Vagin wrote:
> On Wed, Jan 02, 2019 at 06:14:23PM +0000, Radostin Stoyanov wrote:
> > On 02/01/2019 17:36, Adrian Reber wrote:
> > >>>> Yes and no. I understand the sshd reasons, but they can be applied to
> > >>>> criu restore, but not to criu dump, IMHO.
> > >>>>
> > >>>> Our goal is to not loose states of processes:
> > >>>> 1. criu dump doesn't have to be destructive:
> > >>>>   * in case of any error, processes have to continue running
> > >>>>   * it is better to fail rather than dump wrong or incomplete state
> > >>>>
> > >>>> 2. sometimes we really may want to restore processes even if there is
> > >>>> some problem
> > >>> That is a very good point, I agree with you.
> > >>>
> > >>> Is it OK to close https://github.com/checkpoint-restore/criu/issues/578
> > >>> with the explanation above?
> > >> I would like to know what Adrian is thinking about this.
> > > The whole series was just trying to help with Radostin's 'bug'. But if
> > > nobody sees this as a problem we can drop the whole thing.
> > >
> > I understand that we want criu to exit with an error when we have
> > unknown options in config files, thus we can drop PATCH v5 2/3 and 3/3.
> > 
> > However, I think that PATCH v5 1/3 (Printout early log messages) is useful.
> > Is there a reason why we don't want this patch?
> 
> We want it!

Okay, let me check the status of this and include the changes you sent
some time ago and resent.

		Adrian