Merge lp://staging/~jderose/ecryptfs/fix-1597154 into lp://staging/ecryptfs

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!

Tyler,

Okay, resubmitting with your two issues addressed:

1) I didn't come up with a unified sed expression I liked for building $drive, so I'm still using a conditional, but this time with:

  grep -qE "^/dev/.+[0-9]+p[0-9]+$"

This means the code now correctly handles the generic /dev/foo0p1 pattern. I agree that this is a better approach, although I still feel there is a certain fragility here as we're still making some assumption that we don't know for sure will hold for future types of block devices that merely match this naming pattern.

2) Nice catch! I didn't notice that the original code didn't correctly handle multi-digit partition numbers. In all cases, I'm now building $partno with:

  partno=$(echo $swap | sed "s/.\+[^0-9]\([0-9]\+\)/\1/")

I also updated my (out-of-band) test script, which now correctly handles all the key corner cases for existing types of block devices that I can think of:

"""
#!/bin/sh -e

test_fix() {
    swap=$1
    if echo $swap | grep -qE "^/dev/.+[0-9]+p[0-9]+$"; then
        drive=$(echo $swap | sed "s/\(.\+[0-9]\)p[0-9]\+/\1/")
    else
        drive=$(echo $swap | sed "s/\(.\+[^0-9]\)[0-9]\+/\1/")
    fi
    partno=$(echo $swap | sed "s/.\+[^0-9]\([0-9]\+\)/\1/")
    echo "$swap\t$drive\t$partno"
}

test_fix "/dev/sda3"
test_fix "/dev/sda33"
test_fix "/dev/sdp3"
test_fix "/dev/sdp33"

echo ""
test_fix "/dev/vda3"
test_fix "/dev/vda33"
test_fix "/dev/vdp3"
test_fix "/dev/vdp33"

echo ""
test_fix "/dev/mmcblk0p3"
test_fix "/dev/mmcblk0p33"
test_fix "/dev/mmcblk99p3"
test_fix "/dev/mmcblk99p33"

echo ""
test_fix "/dev/nvme0n1p3"
test_fix "/dev/nvme0n1p33"
test_fix "/dev/nvme99n1p3"
test_fix "/dev/nvme99n1p33"

echo ""
test_fix "/dev/nvme0n11p3"
test_fix "/dev/nvme0n11p33"
test_fix "/dev/nvme99n11p3"
test_fix "/dev/nvme99n11p33"
"""

Please let me know if I missed anything or if you'd like further tweaks!

Hi Jason - the code changes look good. I made a small change to replace the sed regexes special '/' characters with ':' characters to make things a little more readable (the other call to sed in ecryptfs-setup-swap does that, as well).

That said, manual testing is not working as expected. I am testing on a freshly installed amd64 VM that was installed using GPT partitioning on an nvme drive. QEMU is being started like so:

$ qemu-system-x86_64 -enable-kvm -m 1G -vga qxl \
  -drive if=pflash,format=raw,readonly,file=/usr/share/OVMF/OVMF_CODE.fd \
  -drive if=pflash,format=raw,file=OVMF_VARS.fd \
  -drive if=none,format=qcow2,id=nvme0,file=drive.qcow2 \
  -device nvme,drive=nvme0,serial=1234

After running `sudo ecryptfs-setup-swap` and rebooting, I'm seeing /dev/mapper/cryptswap1 being populated but `swapon -s` and `free` shows that no swap partitions are enabled. If I run `sudo swapon -a` then the swap partition is enabled.

I'm trying to debug what's going on here but thought I'd mention it to you to see if you have gotten different results.

I take that back. I've successfully tested this patch in several fresh VMs since my last comment. I think that I may have installed my locally built ecryptfs-utils debs in a way that resulted in cryptsetup (a recommends) not being installed. This looks good to me now.