From 7586158ac902878712f6511263ad075abca8c16f Mon Sep 17 00:00:00 2001 From: Anna Gillert Date: Thu, 7 Apr 2022 09:36:38 +0200 Subject: [PATCH 1/4] nixos/manual: Refine doc for `execute` et al - Clarify that shellopts are set in every `execute` call (rather than only `succeed`). - Add documentation for the `timeout` parameter and its default values. --- .../writing-nixos-tests.section.md | 51 ++++++----- .../writing-nixos-tests.section.xml | 84 +++++++++++-------- 2 files changed, 82 insertions(+), 53 deletions(-) diff --git a/nixos/doc/manual/development/writing-nixos-tests.section.md b/nixos/doc/manual/development/writing-nixos-tests.section.md index a9ffffe2277c..4a7fdd413978 100644 --- a/nixos/doc/manual/development/writing-nixos-tests.section.md +++ b/nixos/doc/manual/development/writing-nixos-tests.section.md @@ -159,23 +159,8 @@ The following methods are available on machine objects: `execute` : Execute a shell command, returning a list `(status, stdout)`. - If the command detaches, it must close stdout, as `execute` will wait - for this to consume all output reliably. This can be achieved by - redirecting stdout to stderr `>&2`, to `/dev/console`, `/dev/null` or - a file. Examples of detaching commands are `sleep 365d &`, where the - shell forks a new process that can write to stdout and `xclip -i`, where - the `xclip` command itself forks without closing stdout. - Takes an optional parameter `check_return` that defaults to `True`. - Setting this parameter to `False` will not check for the return code - and return -1 instead. This can be used for commands that shut down - the VM and would therefore break the pipe that would be used for - retrieving the return code. -`succeed` - -: Execute a shell command, raising an exception if the exit status is - not zero, otherwise returning the standard output. Commands are run - with `set -euo pipefail` set: + Commands are run with `set -euo pipefail` set: - If several commands are separated by `;` and one fails, the command as a whole will fail. @@ -183,10 +168,33 @@ The following methods are available on machine objects: - For pipelines, the last non-zero exit status will be returned (if there is one, zero will be returned otherwise). - - Dereferencing unset variables fail the command. + - Dereferencing unset variables fails the command. - - It will wait for stdout to be closed. See `execute` for the - implications. + - It will wait for stdout to be closed. + + If the command detaches, it must close stdout, as `execute` will wait + for this to consume all output reliably. This can be achieved by + redirecting stdout to stderr `>&2`, to `/dev/console`, `/dev/null` or + a file. Examples of detaching commands are `sleep 365d &`, where the + shell forks a new process that can write to stdout and `xclip -i`, where + the `xclip` command itself forks without closing stdout. + + Takes an optional parameter `check_return` that defaults to `True`. + Setting this parameter to `False` will not check for the return code + and return -1 instead. This can be used for commands that shut down + the VM and would therefore break the pipe that would be used for + retrieving the return code. + + A timeout for the command can be specified (in seconds) using the optional + `timeout` parameter, e.g., `execute(cmd, timeout=10)` or + `execute(cmd, timeout=None)`. The default is 900 seconds. + +`succeed` + +: Execute a shell command, raising an exception if the exit status is + not zero, otherwise returning the standard output. Similar to `execute`, + except that the timeout is `None` by default. See `execute` for details on + command execution. `fail` @@ -196,10 +204,13 @@ The following methods are available on machine objects: `wait_until_succeeds` : Repeat a shell command with 1-second intervals until it succeeds. + Has a default timeout of 900 seconds which can be modified, e.g. + `wait_until_succeeds(cmd, timeout=10)`. See `execute` for details on + command execution. `wait_until_fails` -: Repeat a shell command with 1-second intervals until it fails. +: Like `wait_until_succeeds`, but repeating the command until it fails. `wait_for_unit` diff --git a/nixos/doc/manual/from_md/development/writing-nixos-tests.section.xml b/nixos/doc/manual/from_md/development/writing-nixos-tests.section.xml index b194d58e5beb..6a6e2c755edf 100644 --- a/nixos/doc/manual/from_md/development/writing-nixos-tests.section.xml +++ b/nixos/doc/manual/from_md/development/writing-nixos-tests.section.xml @@ -274,35 +274,9 @@ start_all() Execute a shell command, returning a list - (status, stdout). If the command - detaches, it must close stdout, as - execute will wait for this to consume all - output reliably. This can be achieved by redirecting stdout - to stderr >&2, to - /dev/console, - /dev/null or a file. Examples of - detaching commands are sleep 365d &, - where the shell forks a new process that can write to stdout - and xclip -i, where the - xclip command itself forks without - closing stdout. Takes an optional parameter - check_return that defaults to - True. Setting this parameter to - False will not check for the return code - and return -1 instead. This can be used for commands that - shut down the VM and would therefore break the pipe that - would be used for retrieving the return code. + (status, stdout). - - - - - succeed - - - Execute a shell command, raising an exception if the exit - status is not zero, otherwise returning the standard output. Commands are run with set -euo pipefail set: @@ -323,16 +297,57 @@ start_all() - Dereferencing unset variables fail the command. + Dereferencing unset variables fails the command. - It will wait for stdout to be closed. See - execute for the implications. + It will wait for stdout to be closed. + + If the command detaches, it must close stdout, as + execute will wait for this to consume all + output reliably. This can be achieved by redirecting stdout + to stderr >&2, to + /dev/console, + /dev/null or a file. Examples of + detaching commands are sleep 365d &, + where the shell forks a new process that can write to stdout + and xclip -i, where the + xclip command itself forks without + closing stdout. + + + Takes an optional parameter check_return + that defaults to True. Setting this + parameter to False will not check for the + return code and return -1 instead. This can be used for + commands that shut down the VM and would therefore break the + pipe that would be used for retrieving the return code. + + + A timeout for the command can be specified (in seconds) + using the optional timeout parameter, + e.g., execute(cmd, timeout=10) or + execute(cmd, timeout=None). The default + is 900 seconds. + + + + + + succeed + + + + Execute a shell command, raising an exception if the exit + status is not zero, otherwise returning the standard output. + Similar to execute, except that the + timeout is None by default. See + execute for details on command execution. + @@ -353,7 +368,10 @@ start_all() Repeat a shell command with 1-second intervals until it - succeeds. + succeeds. Has a default timeout of 900 seconds which can be + modified, e.g. + wait_until_succeeds(cmd, timeout=10). See + execute for details on command execution. @@ -363,8 +381,8 @@ start_all() - Repeat a shell command with 1-second intervals until it - fails. + Like wait_until_succeeds, but repeating + the command until it fails. From f7e89a59da9f4531c21df0736b5fdfeba19d7c77 Mon Sep 17 00:00:00 2001 From: Anna Gillert Date: Thu, 7 Apr 2022 09:39:33 +0200 Subject: [PATCH 2/4] nixos/test-driver: fix missing shellopts in `execute` Without this fix, setting the shellopts in `machine.execute` is inconsitent. When no timeout is used, shellopts `set -euo pipefail` are applied to the command as expected. When a timeout is specified, the shellopts are not applied to the command itself (which is called inside a `sh -c` that doesn't inherit the shellopts) but rather to the `timeout` command, leading to the following full command: ```bash (set -euo pipefail; timeout 900 sh -c 'cmd') | (base64 --wrap 0; echo)\n ``` With this fix, this is the command we get: ```bash timeout 900 sh -c 'set -euo pipefail; false | true') | (base64 --wrap 0; echo)\n ``` --- nixos/lib/test-driver/test_driver/machine.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/nixos/lib/test-driver/test_driver/machine.py b/nixos/lib/test-driver/test_driver/machine.py index f3e615fe5bf9..ed2aceb96581 100644 --- a/nixos/lib/test-driver/test_driver/machine.py +++ b/nixos/lib/test-driver/test_driver/machine.py @@ -526,10 +526,14 @@ class Machine: self.run_callbacks() self.connect() - if timeout is not None: - command = "timeout {} sh -c {}".format(timeout, shlex.quote(command)) + # Always run command with shell opts + command = f"set -euo pipefail; {command}" + + if timeout is not None: + command = f"timeout {timeout} sh -c {shlex.quote(command)}" + + out_command = f"({command}) | (base64 --wrap 0; echo)\n" - out_command = f"( set -euo pipefail; {command} ) | (base64 --wrap 0; echo)\n" assert self.shell self.shell.send(out_command.encode()) From ed945aeb6e4278a3f1b3f137665ad47b79f525c9 Mon Sep 17 00:00:00 2001 From: tljuniper <48209000+tljuniper@users.noreply.github.com> Date: Mon, 11 Apr 2022 07:53:04 +0200 Subject: [PATCH 3/4] nixos/manual: Clarify execute exit status Co-authored-by: Robert Hensing --- nixos/doc/manual/development/writing-nixos-tests.section.md | 2 +- .../from_md/development/writing-nixos-tests.section.xml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/nixos/doc/manual/development/writing-nixos-tests.section.md b/nixos/doc/manual/development/writing-nixos-tests.section.md index 4a7fdd413978..e5ee1cb01ff1 100644 --- a/nixos/doc/manual/development/writing-nixos-tests.section.md +++ b/nixos/doc/manual/development/writing-nixos-tests.section.md @@ -166,7 +166,7 @@ The following methods are available on machine objects: command as a whole will fail. - For pipelines, the last non-zero exit status will be returned - (if there is one, zero will be returned otherwise). + (if there is one; otherwise zero will be returned). - Dereferencing unset variables fails the command. diff --git a/nixos/doc/manual/from_md/development/writing-nixos-tests.section.xml b/nixos/doc/manual/from_md/development/writing-nixos-tests.section.xml index 6a6e2c755edf..7ce3e4cb2906 100644 --- a/nixos/doc/manual/from_md/development/writing-nixos-tests.section.xml +++ b/nixos/doc/manual/from_md/development/writing-nixos-tests.section.xml @@ -291,8 +291,8 @@ start_all() For pipelines, the last non-zero exit status will be - returned (if there is one, zero will be returned - otherwise). + returned (if there is one; otherwise zero will be + returned). From dbc95f15b8dad5224cbb6a52df979023db6cba98 Mon Sep 17 00:00:00 2001 From: Anna Gillert Date: Mon, 11 Apr 2022 19:16:03 +0200 Subject: [PATCH 4/4] nixos/test-driver: Avoid shell injection in machine.execute() --- nixos/lib/test-driver/test_driver/machine.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/nixos/lib/test-driver/test_driver/machine.py b/nixos/lib/test-driver/test_driver/machine.py index ed2aceb96581..035e3ffe8973 100644 --- a/nixos/lib/test-driver/test_driver/machine.py +++ b/nixos/lib/test-driver/test_driver/machine.py @@ -529,10 +529,13 @@ class Machine: # Always run command with shell opts command = f"set -euo pipefail; {command}" + timeout_str = "" if timeout is not None: - command = f"timeout {timeout} sh -c {shlex.quote(command)}" + timeout_str = f"timeout {timeout}" - out_command = f"({command}) | (base64 --wrap 0; echo)\n" + out_command = ( + f"{timeout_str} sh -c {shlex.quote(command)} | (base64 --wrap 0; echo)\n" + ) assert self.shell self.shell.send(out_command.encode())