Re: [PATCH v6 06/12] ndctl: add unit test for security ops (minus overwrite)

On Fri, 2018-12-14 at 14:09 -0700, Dave Jiang wrote: Local-only variables don't need to be all uppercase. By convention, only variables you export are uppercase. quote all "$expansions". Generally, consider running the script through shellchek for styling problems. Applies everyehere in the script - prefer "$()" instead of backticks. Also before using keyctl, lets make sure it is installed. test/common provides a function for this - check_prereq() Before the first use, would be a good idea to make sure keypath exists. Do you mean "unlock_dimm()" ? Best to explicitly delineate each variable using ${}: "${path}/${id}/lock_dimm" Also 'unlock' isn't very descriptive, maybe 'unlock_path'? It also contains "nfit_test.0" which should be obtained from the variable "$NFIT_TEST_BUS0". See below Avoid setting global variables, it is easy to lose track of what has been set where. Instead make helper functions like this simply print their result as the output. Then call them like so: sstate="$(get_security_state)" make the '-n' check the responsibility of the caller, and this function could simply be: get_security_state() { $NDCTL list -i -b $NFIT_TEST_BUS0 -d $dev | jq .[].dimms[0].security | tr -d '"' } In most of these cases, since you are checking for sstate to be exactly "some-state", you don't even need a -n check. > +} > + > +enable_passphrase() > +{ > + $NDCTL enable-passphrase -m user:$MASTERKEY $dev > + if [ "$sstate" != "unlocked" ]; then > + echo "Incorrect security state: $sstate expected: unlocked" > + exit 1 > + fi > +} > + > +disable_passphrase() > +{ > + $NDCTL disable-passphrase $dev > + if [ "$sstate" != "disabled" ]; then > + echo "Incorrect security state: $sstate expected: disabled" > + exit 1 > + fi > +} > + > +erase_security() > +{ > + $NDCTL sanitize-dimm -c $dev > + if [ "$sstate" != "disabled" ]; then > + echo "Incorrect security state: $sstate expected: disabled" > + exit 1 > + fi > +} > + > +update_security() > +{ > + $NDCTL update-passphrase -m user:$MASTERKEY $dev > + if [ "$sstate" != "unlocked" ]; then > + echo "Incorrect security state: $sstate expected: unlocked" > + exit 1 > + fi > +} > + > +freeze_security() > +{ > + $NDCTL freeze-security $dev > +} > + > +test_1_security_enable_and_disable() > +{ > + enable_passphrase > + disable_passphrase > +} > + > +test_2_security_enable_and_update() > +{ > + enable_passphrase > + update_security > + disable_passphrase > +} > + > +test_3_security_enable_and_erase() > +{ > + enable_passphrase > + erase_security > +} > + > +test_4_security_unlocking() > +{ > + enable_passphrase > + locking_dimm > + $NDCTL enable-dimm $dev > + if [ "$sstate" != "unlocked" ]; then > + echo "Incorrect security state: $sstate expected: unlocked" > + exit 1 > + fi > + $NDCTL disable-region -b $NFIT_TEST_BUS0 all > + disable_passphrase > +} > + > +# this should always be the last test. with security frozen, nfit_test must > +# be removed and is no longer usable > +test_5_security_freeze() > +{ > + enable_passphrase > + freeze_security > + if [ "$sstate" != "frozen" ]; then > + echo "Incorrect security state: $sstate expected: frozen" > + exit 1 > + fi > + $NDCTL disable-passphrase $dev && { echo "diable succeed after frozen"; exit 1; } What is the 'exit 1' here for? Shouldn't we just fall through and check the state explicitly anyway, even after a successful disable? And if so, no need to echo the success message, the state will just get checked later. > + echo $sstate > + if [ "$sstate" != "frozen" ]; then > + echo "Incorrect security state: $sstate expected: disabled" > + exit 1 > + fi > +} > + > +check_min_kver "4.21" || do_skip "may lack security test handling" "may lack security enabling" - its not just the test infrastructure that was added in 4.21