Merge lp:~bladernr/checkbox/1291664-removable-storage-traceback-on-ro-usb into lp:checkbox

Proposed by Jeff Lane 
Status: Merged
Approved by: Zygmunt Krynicki
Approved revision: 2782
Merged at revision: 2783
Proposed branch: lp:~bladernr/checkbox/1291664-removable-storage-traceback-on-ro-usb
Merge into: lp:checkbox
Diff against target: 27 lines (+6/-0)
2 files modified
checkbox-old/CHANGELOG (+2/-0)
checkbox-old/scripts/removable_storage_test (+4/-0)
To merge this branch: bzr merge lp:~bladernr/checkbox/1291664-removable-storage-traceback-on-ro-usb
Reviewer Review Type Date Requested Status
Zygmunt Krynicki (community) Approve
Jeff Lane  Needs Resubmitting
Review via email: mp+210712@code.launchpad.net

Description of the change

Fixes a condition where a write-protected USB device (or probably any WP device) causes the removable_storage_test script to dump a trace and exit prematurely due to the unhandled exception.

Write protected USB devices (and maybe others) cause an IOError in a section where they weren't caught.

Now we catch them

I've tried this on two servers that have embedded USB storage that is write protected, and this resolved the issue on both.

To post a comment you must log in.
Revision history for this message
Zygmunt Krynicki (zyga) wrote :

I think this is a bit eager to jump to conclusions. We can check if the device is read only but we shouldn't treat *all* IOErrors as the device being read only. If you reword the error message to:

+ logging.error("I/O error accessing the device %s", dest)

I will gladly merge it.

review: Needs Fixing
2782. By Jeff Lane 

Fixed the error message for IOError in write_file to be more generic and not assume a read-only mount

Revision history for this message
Jeff Lane  (bladernr) wrote :

Thanks, that makes a lot of sense and I've changed the error message accordingly. You're right, the RO filesystem errors WOULD be I/O errors, but not all I/O errors would be due to a read-only filesystem.

review: Needs Resubmitting
Revision history for this message
Zygmunt Krynicki (zyga) wrote :

Thanks, +1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'checkbox-old/CHANGELOG'
2--- checkbox-old/CHANGELOG 2014-02-27 05:56:02 +0000
3+++ checkbox-old/CHANGELOG 2014-03-13 03:02:58 +0000
4@@ -20,6 +20,8 @@
5 new 14.04 only version for Trusty testing. This is the foundation for 14.04
6 server cert changes.
7 * Added usb3/storage-preinserted to server-selftest-14.04.whitelist
8+ * /scripts/removable_storage_test: fixed an unhandled exception with write
9+ protected devices that caused the script to crash (LP: #1291664)
10
11 [ Po-Hsu Lin]
12 * jobs/power-management.txt.in: add product requirement for laptops into
13
14=== modified file 'checkbox-old/scripts/removable_storage_test'
15--- checkbox-old/scripts/removable_storage_test 2013-05-29 07:50:30 +0000
16+++ checkbox-old/scripts/removable_storage_test 2014-03-13 03:02:58 +0000
17@@ -121,6 +121,10 @@
18 logging.error("Unable to open %s for writing.", dest)
19 logging.error(" %s", exc)
20 return False
21+ except IOError as exc:
22+ logging.error("I/O Error when accessing device %s.", dest)
23+ logging.error(" %s", exc)
24+ return False
25 with outfile:
26 try:
27 outfile.write(self.data)

Subscribers

People subscribed via source and target branches