Code review comment for lp:~jderose/ecryptfs/fix-1597154

Revision history for this message
Tyler Hicks (tyhicks) wrote :

Hi Jason - Thank you for the merge proposal! I see two issues that I think should be addressed:

1) This change is brittle in that if another drive type comes along that has the foo0p0 format, this same bug will be reintroduced. Can you come up with a way to manipulate the drive path so that it handles the situation in a generic way without the need for the `echo $swap | grep -qE "^/dev/nvme*|^/dev/mmcblk*"` conditional?

2) The (existing) logic doesn't handle partition numbers with multiple digits. This can be seen by adding "test_fix /dev/mmcblk0p13" to your test script.

Thanks again!

review: Needs Fixing

« Back to merge proposal