Merge ~zongminl/plainbox-provider-checkbox:fix-1953294-missing-exception-handling into plainbox-provider-checkbox:master

Proposed by Vic Liu
Status: Merged
Approved by: Vic Liu
Approved revision: b1754cb94eaf1e495039ca9bddedff8527b9d240
Merged at revision: 3faee4c5045f54d055828a62e642f7ea2e032298
Proposed branch: ~zongminl/plainbox-provider-checkbox:fix-1953294-missing-exception-handling
Merge into: plainbox-provider-checkbox:master
Diff against target: 40 lines (+11/-3)
1 file modified
bin/boot_mode_test_snappy.py (+11/-3)
Reviewer Review Type Date Requested Status
Devices Certification Bot Needs Fixing
Pierre Equoy Approve
Review via email: mp+412834@code.launchpad.net

Commit message

Fix: lp1953294 - miscellanea/secure_boot_mode_* fails with a CalledProcessError exception when Secure Boot not activated

- Added exception handler so a SystemExit can be raised when CalledProcessError occurs
- Skipped the volume in gadget.yaml when there is no 'bootloader' defined in it to avoid KeyError

To post a comment you must log in.
Revision history for this message
Pierre Equoy (pieq) wrote :

Thanks for your help!

It now outputs this:

-----
Bootloader is grub

+ mokutil --sb-state
This system doesn't support Secure Boot
/tmp/nest-bvvsju26.648efd11d9fcfc5a24433b6c16a3b54cc18049637438ab18211e9597782aa0e1/boot_mode_test_snappy.py:77: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details.
  data = yaml.load(f)
Error: Command 'mokutil --sb-state' returned non-zero exit status 255.
-----

I think there is a fix to be made to the `data = yaml.load(f)` (line 19 below) to avoid showing the DeprecationWarning, and as for the command itself, maybe we should just run the command, since `mokutil --sb-state` already outputs "This system doesn't support Secure Boot" by default:

>>> import subprocess
>>> p = subprocess.run("mokutil --sb-state".split())
This system doesn't support Secure Boot
>>> p.stderr
>>> p.stdout
>>> p.args
['mokutil', '--sb-state']
>>> p.returncode
255

review: Needs Fixing
Revision history for this message
Vic Liu (zongminl) wrote :

Fixed according to @pieq's suggestion:
1. The commands using here already print error messages on their own, removing exception message when raising SystemExit, now this exits with code 1 silently to prevent redundant messages
2. Using SafeLoader to prevent DeprecationWarning printed when calling yaml.load()

Revision history for this message
Pierre Equoy (pieq) wrote :

+1

Thanks!

review: Approve
Revision history for this message
Devices Certification Bot (ce-certification-qa) wrote :

The merge was fine but running tests failed.

"10.38.105.54"
"10.38.105.108"
"10.38.105.197"
[xenial] [07:01:09] starting container
Device project added to xenial-testing
"10.38.105.54"
[xenial] [07:01:32] provisioning container
[bionic] [07:02:01] starting container
Device project added to bionic-testing
"10.38.105.120"
[bionic] [07:02:27] provisioning container
[focal] [07:02:58] starting container
[xenial] [07:03:06] Starting tests...
[xenial] Found a test script: ./requirements/container-tests-provider-checkbox
Device project added to focal-testing
"10.38.105.222"
[focal] [07:03:19] provisioning container
[bionic] [07:03:32] Starting tests...
[bionic] Found a test script: ./requirements/container-tests-provider-checkbox
[focal] [07:04:58] Starting tests...
[focal] Found a test script: ./requirements/container-tests-provider-checkbox
[bionic] [07:05:21] container-tests-provider-checkbox: PASS
[bionic] [07:05:21] Fixing file permissions in source directory
[bionic] [07:05:22] Destroying container
[xenial] [07:05:46] container-tests-provider-checkbox: FAIL
[xenial] output: https://paste.ubuntu.com/p/8G6662Vs4x/
[xenial] [07:05:49] Fixing file permissions in source directory
[xenial] [07:05:49] Destroying container
[focal] [07:07:52] container-tests-provider-checkbox: FAIL
[focal] output: https://paste.ubuntu.com/p/TXhKPmvvjM/
[focal] [07:07:54] Fixing file permissions in source directory
[focal] [07:07:54] Destroying container

review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/bin/boot_mode_test_snappy.py b/bin/boot_mode_test_snappy.py
index 23b30e4..0e0957a 100755
--- a/bin/boot_mode_test_snappy.py
+++ b/bin/boot_mode_test_snappy.py
@@ -19,7 +19,10 @@ from checkbox_support.snap_utils.system import get_lk_bootimg_path
1919
20def fitdumpimage(filename):20def fitdumpimage(filename):
21 cmd = 'dumpimage -l {}'.format(filename)21 cmd = 'dumpimage -l {}'.format(filename)
22 out = sp.check_output(cmd, shell=True).decode(sys.stdout.encoding)22 try:
23 out = sp.check_output(cmd, shell=True).decode(sys.stdout.encoding)
24 except Exception:
25 raise SystemExit(1)
23 buf = io.StringIO(out)26 buf = io.StringIO(out)
2427
25 # first line should identify FIT file28 # first line should identify FIT file
@@ -71,8 +74,10 @@ def main():
71 'ERROR: failed to find gadget.yaml at {}'.format(gadget_yaml))74 'ERROR: failed to find gadget.yaml at {}'.format(gadget_yaml))
7275
73 with open(gadget_yaml) as f:76 with open(gadget_yaml) as f:
74 data = yaml.load(f)77 data = yaml.load(f, Loader=yaml.SafeLoader)
75 for k in data['volumes'].keys():78 for k in data['volumes'].keys():
79 if 'bootloader' not in data['volumes'][k]:
80 continue
76 bootloader = data['volumes'][k]['bootloader']81 bootloader = data['volumes'][k]['bootloader']
77 if not bootloader:82 if not bootloader:
78 raise SystemExit('ERROR: could not find name of bootloader')83 raise SystemExit('ERROR: could not find name of bootloader')
@@ -162,7 +167,10 @@ def main():
162 if bootloader == 'grub':167 if bootloader == 'grub':
163 cmd = 'mokutil --sb-state'168 cmd = 'mokutil --sb-state'
164 print('+', cmd, flush=True)169 print('+', cmd, flush=True)
165 out = sp.check_output(cmd, shell=True).decode(sys.stdout.encoding)170 try:
171 out = sp.check_output(cmd, shell=True).decode(sys.stdout.encoding)
172 except Exception:
173 raise SystemExit(1)
166 print(out, flush=True)174 print(out, flush=True)
167 if out != 'SecureBoot enabled\n':175 if out != 'SecureBoot enabled\n':
168 raise SystemExit('ERROR: mokutil reports Secure Boot not in use')176 raise SystemExit('ERROR: mokutil reports Secure Boot not in use')

Subscribers

People subscribed via source and target branches