Merge lp:~larsks/cloud-utils/bug-1807171 into lp:cloud-utils

[Lars Kellogg-Stedman]

growpart: fix bug occurring if start sector and size were the same.

The existing sed expression would erroneously change the start sector
of a partition, rather than the size, if the start sector and size
were identical. This commit modifies the sed expression so that it
will only operate on the final match in the line.

[Lars Kellogg-Stedman (larsks)]

Ugh, so, the first stab a test is invalid because it only checks the output of growpart, and growpart lies. I'll submit an update in a moment.

[Scott Moser (smoser)]

2 problems with the test case:
a.) it has some tab/spaces indentation issues. for better or worse (probably worse), current expectation in those files is tabs.
b.) running it with current growpart does not catch the failure.

heres some modifications that catch the failure.
http://paste.ubuntu.com/p/BX5BkX7XFB/

integrate those , commit and i'll be happy.

thanks!

[lp:~larsks/cloud-utils/bug-1807171]

340. By Lars Kellogg-Stedman on 2018-12-06

Fix test for bug #1807171

The test in the previous commit did not actually verify that the
final partition table matched the desired configuration.

[Lars Kellogg-Stedman (larsks)]

I fixed the test, but in a completely different fashion. I've tested the test case against both the fix and the previous commit to verify that it breaks when expected.

[Scott Moser (smoser)]

larsks,
this looks good. thank you.

review: Approve

[Scott Moser (smoser)]

Lars,

Looking at it a bit more I'mk going to ake a small change in how we read back
the content from the sfdisk dump. My initial suggestion was trying to be
more forgiving in sfdisk output. sfdisk --dump probably doesn't guarantee
that its output is identical across different versions. For the simplist
case imagine that they add a new field to the header. That would cause
the test to fail.

So what I've done here is add a 'read_pt_info' that tries to read
a specific key for a specific partition in --dump output in a relatively
forgiving way.

Basically... I just don't want to babysit test failures that are not
test failures in the event of sfdisk changing its output.

[Lars Kellogg-Stedman (larsks)]

It seems that we need to trust the output of sfdisk --dump because we rely on it in growpart. The simplest fix would be to just discard the headers and only look at the partition lines. In any case, I'm happy as long as you're happy :).

=== modified file 'bin/growpart'
--- bin/growpart	2018-10-19 16:46:40 +0000
+++ bin/growpart	2018-12-06 22:40:58 +0000
@@ -323,7 +323,7 @@
 	# now, change the size for this partition in ${dump_out} to be the
 	# new size
 	new_size=$((${max_end}-${pt_start}))
-	sed "\|^\s*${dpart} |s/${pt_size},/${new_size},/" "${dump_out}" \
+	sed "\|^\s*${dpart} |s/\(.*\)${pt_size},/\1${new_size},/" "${dump_out}" \
 		>"${new_out}" ||
 		fail "failed to change size in output"
 

=== added file 'test/test-growpart-start-matches-size'
--- test/test-growpart-start-matches-size	1970-01-01 00:00:00 +0000
+++ test/test-growpart-start-matches-size	2018-12-06 22:40:58 +0000
@@ -0,0 +1,95 @@
+#!/bin/bash
+#
+# Create a disk image where there exists a partition whose sizes matches the
+# start sector.
+# brought up under bug 1807171, which describes an error in the sed expression
+# used to generate the replacement partition map
+
+set -e
+
+TEMP_D=""
+
+rq() {
+    local out="${TEMP_D}/out"
+    "$@" > "$out" 2>&1 || { echo "FAILED:" "$@"; cat "$out"; return 1; }
+}
+fail() { echo "FAILED:" "$@" 1>&2; exit 1; }
+
+setup_image() {
+    sfdisk disk.img <<EOF
+label: gpt
+label-id: db24000c-6ef3-4a17-b71c-1064baa29514
+device: disk.img
+unit: sectors
+first-lba: 2048
+last-lba: 4194270
+
+disk.img1 : start=        2048, size=     1024000, type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B, uuid=5bc16165-bfc0-4e13-94eb-b898dc0bca41
+disk.img2 : start=     1026048, size=     1026048, type=4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709, uuid=a0e1636e-b759-4e7a-bd14-6f3d6c04745d
+EOF
+}
+
+cleanup() {
+    rm -rf "${TEMP_D}"
+}
+
+TEMP_D=$(mktemp -d ${TMPDIR:-/tmp}/${0##*/}.XXXXXX)
+trap cleanup EXIT
+
+# the sfdisk and sgdisk resizers result in slightly different output, because
+# of course they do.
+
+expected_sfdisk='label: gpt
+label-id: DB24000C-6EF3-4A17-B71C-1064BAA29514
+device: disk.img
+unit: sectors
+first-lba: 2048
+last-lba: 4194270
+
+disk.img1 : start=        2048, size=     1024000, type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B, uuid=5BC16165-BFC0-4E13-94EB-B898DC0BCA41
+disk.img2 : start=     1026048, size=     3168223, type=4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709, uuid=A0E1636E-B759-4E7A-BD14-6F3D6C04745D'
+
+expected_sgdisk='label: gpt
+label-id: DB24000C-6EF3-4A17-B71C-1064BAA29514
+device: disk.img
+unit: sectors
+first-lba: 2048
+last-lba: 4192256
+
+disk.img1 : start=        2048, size=     1024000, type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B, uuid=5BC16165-BFC0-4E13-94EB-B898DC0BCA41
+disk.img2 : start=     1026048, size=     3166208, type=4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709, uuid=A0E1636E-B759-4E7A-BD14-6F3D6C04745D'
+
+
+CR='
+'
+
+test_resize () {
+    local resizer=$1
+    local expected=$2
+	echo "====== Testing with resizer=$resizer ====="
+
+    (
+        cd ${TEMP_D}
+        echo "$expected" > partitions.expected
+
+        rq truncate "--size=2G" disk.img
+        rq setup_image || fail "setup image $img"
+
+        sfdisk --dump disk.img > partitions.before
+
+        if ! GROWPART_RESIZER=$resizer \
+                growpart -v -v disk.img 2 2>"stderr" > "stdout"; then
+            cat "stderr" "stdout"
+            fail "[resizer=$resizer] growpart failed"
+        fi
+        
+        sfdisk --dump disk.img > partitions.after
+        if ! diff -U0 partitions.expected partitions.after; then
+            fail "[resizer=$resizer] partition table is incorrect"
+        fi 
+    )
+}
+
+set -e
+test_resize sfdisk "$expected_sfdisk"
+test_resize sgdisk "$expected_sgdisk"