Libguestfs - Jan 2019 - [PATCH nbdkit v3 0/5] partition filter: Support MBR logical partitions.

I think this addresses everything in Eric's v2 review.

Note that the first patch is best viewed using ‘-w’ to ignore
whitespaces changes.

Rich.

---
 filters/partition/nbdkit-partition-filter.pod |   5 -
 filters/partition/partition-mbr.c             | 127 ++++++++++++++++--
 tests/test-partitioning1.sh                   |  20 ++-
 3 files changed, 134 insertions(+), 18 deletions(-)

diff --git a/filters/partition/nbdkit-partition-filter.pod
b/filters/partition/nbdkit-partition-filter.pod
index 4a615b6..ccd1b52 100644
--- a/filters/partition/nbdkit-partition-filter.pod
+++ b/filters/partition/nbdkit-partition-filter.pod
@@ -19,11 +19,6 @@ This works like the C<qemu-nbd -P> option.
 The opposite of this filter is L<nbdkit-partitioning-plugin(1)> which
 adds a virtual partition table to a file or files.
 
-=head1 NOTE
-
-Only MBR primary partitions and GPT partition tables are supported.
-MBR logical partitions are B<not> supported.
-
 =head1 PARAMETERS
 
 =over 4
diff --git a/filters/partition/partition-mbr.c
b/filters/partition/partition-mbr.c
index 41ee350..18725c8 100644
--- a/filters/partition/partition-mbr.c
+++ b/filters/partition/partition-mbr.c
@@ -36,14 +36,20 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
+#include <inttypes.h>
 #include <string.h>
+#include <errno.h>
 
 #include <nbdkit-filter.h>
 
 #include "byte-swapping.h"
+#include "isaligned.h"
 
 #include "partition.h"
 
+/* See also linux.git/block/partitions/msdos.c:is_extended_partition */
+#define is_extended(byte) ((byte) == 0x5 || (byte) == 0xf || (byte) == 0x85)
+
 struct mbr_partition {
   uint8_t part_type_byte;
   uint32_t start_sector;
@@ -69,20 +75,117 @@ find_mbr_partition (struct nbdkit_next_ops *next_ops, void
*nxdata,
 {
   int i;
   struct mbr_partition partition;
+  uint32_t ep_start_sector, ep_nr_sectors;
+  uint64_t ebr, next_ebr;
+  uint8_t sector[SECTOR_SIZE];
 
-  if (partnum > 4) {
-    nbdkit_error ("MBR logical partitions are not supported");
+  /* Primary partition. */
+  if (partnum <= 4) {
+    for (i = 0; i < 4; ++i) {
+      get_mbr_partition (mbr, i, &partition);
+      if (partition.nr_sectors > 0 &&
+          partition.part_type_byte != 0 &&
+          !is_extended (partition.part_type_byte) &&
+          partnum == i+1) {
+        *offset_r = partition.start_sector * SECTOR_SIZE;
+        *range_r = partition.nr_sectors * SECTOR_SIZE;
+        return 0;
+      }
+    }
+
+    /* Not found falls through to error case at the end of the function. */
+  }
+
+  /* Logical partition. */
+  else {
+    /* Find the extended partition in the primary partition table. */
+    for (i = 0; i < 4; ++i) {
+      get_mbr_partition (mbr, i, &partition);
+      if (partition.nr_sectors > 0 &&
+          is_extended (partition.part_type_byte)) {
+        goto found_extended;
+      }
+    }
+    nbdkit_error ("MBR logical partition selected, "
+                  "but there is no extended partition in the partition
table");
     return -1;
-  }
-
-  for (i = 0; i < 4; ++i) {
-    get_mbr_partition (mbr, i, &partition);
-    if (partition.nr_sectors > 0 &&
-        partition.part_type_byte != 0 &&
-        partnum == i+1) {
-      *offset_r = partition.start_sector * SECTOR_SIZE;
-      *range_r = partition.nr_sectors * SECTOR_SIZE;
-      return 0;
+
+  found_extended:
+    ep_start_sector = partition.start_sector;
+    ep_nr_sectors = partition.nr_sectors;
+    ebr = ep_start_sector * (uint64_t)SECTOR_SIZE;
+
+    /* This loop will terminate eventually because we only accept
+     * links which strictly increase the EBR pointer.  There are valid
+     * partition tables which do odd things like arranging the
+     * partitions in reverse order, but we will not accept them here.
+     */
+    for (i = 5; ; ++i) {
+      /* Check that the ebr is aligned and pointing inside the disk
+       * and doesn't point to the MBR.
+       */
+      if (!IS_ALIGNED (ebr, SECTOR_SIZE) ||
+          ebr < SECTOR_SIZE || ebr >= size-SECTOR_SIZE) {
+        nbdkit_error ("invalid EBR chain: "
+                      "next EBR boot sector is located outside disk
boundary");
+        return -1;
+      }
+
+      /* Read the EBR sector. */
+      nbdkit_debug ("partition: reading EBR at %" PRIi64, ebr);
+      if (next_ops->pread (nxdata, sector, sizeof sector, ebr, 0,
+                           &errno) == -1)
+        return -1;
+
+      if (partnum == i) {
+        uint64_t offset, range;
+
+        /* First entry in EBR points to the logical partition. */
+        get_mbr_partition (sector, 0, &partition);
+
+        /* The first entry start sector is relative to the EBR. */
+        offset = ebr + partition.start_sector * (uint64_t)SECTOR_SIZE;
+        range = partition.nr_sectors * (uint64_t)SECTOR_SIZE;
+
+        /* Logical partition cannot be before the corresponding EBR,
+         * and it cannot extend beyond the enclosing extended
+         * partition.
+         */
+        if (offset <= ebr ||
+            offset + range >
+            ((uint64_t)ep_start_sector + ep_nr_sectors) * SECTOR_SIZE) {
+          nbdkit_error ("logical partition start or size out of range
"
+                        "(offset=%" PRIu64 ", range=%"
PRIu64 ", "
+                        "ep:startsect=%" PRIu32 ",
ep:nrsects=%" PRIu32 ")",
+                        offset, range, ep_start_sector, ep_nr_sectors);
+          return -1;
+        }
+        *offset_r = offset;
+        *range_r = range;
+        return 0;
+      }
+
+      /* Second entry in EBR links to the next EBR. */
+      get_mbr_partition (sector, 1, &partition);
+
+      /* All zeroes means the end of the chain. */
+      if (partition.start_sector == 0 && partition.nr_sectors == 0)
+        break;
+
+      /* The second entry start sector is relative to the start to the
+       * extended partition.
+       */
+      next_ebr +        ((uint64_t)ep_start_sector + partition.start_sector) *
SECTOR_SIZE;
+
+      /* Make sure the next EBR > current EBR. */
+      if (next_ebr <= ebr) {
+        nbdkit_error ("invalid EBR chain: "
+                      "next EBR %" PRIu64 " <= current EBR
%" PRIu64,
+                      next_ebr, ebr);
+        return -1;
+      }
+      ebr = next_ebr;
     }
   }
 
diff --git a/tests/test-partitioning1.sh b/tests/test-partitioning1.sh
index 76ab43b..f705dc8 100755
--- a/tests/test-partitioning1.sh
+++ b/tests/test-partitioning1.sh
@@ -77,7 +77,25 @@ nbdkit -f -v -D partitioning.regions=1 -U - \
 # Contents of partitioning1.out should be identical to file-data.
 cmp file-data partitioning1.out
 
-# Same test with GPT and more partitions.
+# Same test with > 4 MBR partitions.
+# Note we select partition 6 because partition 4 is the extended partition.
+nbdkit -f -v -D partitioning.regions=1 -U - \
+       --filter=partition \
+       partitioning \
+       partitioning1-p1 \
+       partitioning1-p2 \
+       partitioning1-p3 \
+       partitioning1-p4 \
+       file-data \
+       partitioning1-p5 \
+       partitioning1-p6 \
+       partition-type=mbr \
+       partition=6 \
+       --run 'qemu-img convert $nbd partitioning1.out'
+
+cmp file-data partitioning1.out
+
+# Same test with GPT.
 nbdkit -f -v -D partitioning.regions=1 -U - \
        --filter=partition \
        partitioning \
-- 
2.20.1

Test the partition filter against real life partition tables created
by sfdisk.
---
 tests/test-partition.c   | 101 ------------------------
 README                   |   2 +
 tests/Makefile.am        |   7 +-
 tests/test-partition1.sh | 165 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 169 insertions(+), 106 deletions(-)

diff --git a/tests/test-partition.c b/tests/test-partition.c
deleted file mode 100644
index 3de60d8..0000000
--- a/tests/test-partition.c
+++ /dev/null
@@ -1,101 +0,0 @@
-/* nbdkit
- * Copyright (C) 2018 Red Hat Inc.
- * All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are
- * met:
- *
- * * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- *
- * * Redistributions in binary form must reproduce the above copyright
- * notice, this list of conditions and the following disclaimer in the
- * documentation and/or other materials provided with the distribution.
- *
- * * Neither the name of Red Hat nor the names of its contributors may be
- * used to endorse or promote products derived from this software without
- * specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
- * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
- * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
- * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
- * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
- * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
- * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
- * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
- * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
- */
-
-#include <config.h>
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <stdint.h>
-#include <inttypes.h>
-#include <string.h>
-#include <unistd.h>
-
-#include <guestfs.h>
-
-#include "test.h"
-
-int
-main (int argc, char *argv[])
-{
-  guestfs_h *g;
-  int r;
-  char *data;
-
-  if (test_start_nbdkit ("-r",
-                         "--filter", "partition",
-                         "file", "disk",
-                         "partition=1",
-                         NULL) == -1)
-    exit (EXIT_FAILURE);
-
-  g = guestfs_create ();
-  if (g == NULL) {
-    perror ("guestfs_create");
-    exit (EXIT_FAILURE);
-  }
-
-  r = guestfs_add_drive_opts (g, "",
-                              GUESTFS_ADD_DRIVE_OPTS_READONLY, 1,
-                              GUESTFS_ADD_DRIVE_OPTS_FORMAT, "raw",
-                              GUESTFS_ADD_DRIVE_OPTS_PROTOCOL, "nbd",
-                              GUESTFS_ADD_DRIVE_OPTS_SERVER, server,
-                              -1);
-  if (r == -1)
-    exit (EXIT_FAILURE);
-
-  if (guestfs_launch (g) == -1)
-    exit (EXIT_FAILURE);
-
-  /* Because we're using the partition filter, the device should
-   * appear to be a filesystem directly on a whole disk.
-   */
-  if (guestfs_mount_ro (g, "/dev/sda", "/") == -1)
-    exit (EXIT_FAILURE);
-
-  data = guestfs_cat (g, "/hello.txt");
-  if (!data)
-    exit (EXIT_FAILURE);
-
-  if (strcmp (data, "hello,world") != 0) {
-    fprintf (stderr,
-             "%s FAILED: unexpected content of /hello.txt file "
-             "(actual: %s, expected: \"hello,world\")\n",
-             program_name, data);
-    exit (EXIT_FAILURE);
-  }
-
-  free (data);
-
-  guestfs_close (g);
-  exit (EXIT_SUCCESS);
-}
diff --git a/README b/README
index 5824d38..63e1bea 100644
--- a/README
+++ b/README
@@ -149,6 +149,8 @@ For non-essential enhancements to the test suite:
 
  - qemu-io (usually shipped with qemu)
 
+ - sfdisk (from util-linux)
+
  - socat
 
  - ss (from iproute package)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 033c1a1..18936e4 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -79,6 +79,7 @@ EXTRA_DIST = \
 	test-offset2.sh \
 	test-parallel-file.sh \
 	test-parallel-nbd.sh \
+	test-partition1.sh \
 	test-partitioning1.sh \
 	test-partitioning2.sh \
 	test-partitioning3.sh \
@@ -794,11 +795,7 @@ test_offset_LDADD = libtest.la $(LIBGUESTFS_LIBS)
 TESTS += test-offset2.sh
 
 # partition filter test.
-LIBGUESTFS_TESTS += test-partition
-
-test_partition_SOURCES = test-partition.c test.h
-test_partition_CFLAGS = $(WARNINGS_CFLAGS) $(LIBGUESTFS_CFLAGS)
-test_partition_LDADD = libtest.la $(LIBGUESTFS_LIBS)
+TESTS += test-partition1.sh
 
 # truncate filter tests.
 TESTS += \
diff --git a/tests/test-partition1.sh b/tests/test-partition1.sh
new file mode 100755
index 0000000..3884446
--- /dev/null
+++ b/tests/test-partition1.sh
@@ -0,0 +1,165 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2019 Red Hat Inc.
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -e
+set -x
+
+d="partition1.d"
+rm -rf $d
+cleanup_fn rm -rf $d
+mkdir $d
+
+# Test that sfdisk is available and working.
+if ! sfdisk --help >/dev/null; then
+    echo "$0: missing or broken sfdisk"
+    exit 77
+fi
+
+# Test that /dev/urandom exists and can be read.
+if ! test -r /dev/urandom; then
+    echo "$0: mising or unreadable /dev/urandom"
+    exit 77
+fi
+
+# Test that qemu-img is available and working.
+if ! qemu-img --help >/dev/null; then
+    echo "$0: missing or broken qemu-img"
+    exit 77
+fi
+
+test ()
+{
+    label=$1
+    nrparts=$2
+    skip_extended=$3
+
+    rm -f $d/disk
+    truncate -s 1G $d/disk
+    sfdisk -X $label $d/disk
+
+    # Run nbdkit on each partition, copying data in and out.
+    for ((part=1; part <= $nrparts; ++part)); do
+        # The smallest partition in any test is 1023 sectors.  However
+        # to make things quicker only write a sector of random data.
+        dd if=/dev/urandom of=$d/rand bs=512 count=1
+
+        if [ "$part" != "$skip_extended" ]; then
+            nbdkit -f -v --filter=partition file $d/disk partition=$part \
+                   --run "qemu-img convert -n $d/rand \$nbd"
+            nbdkit -f -v --filter=partition file $d/disk partition=$part \
+                   --run "qemu-img convert \$nbd $d/out"
+            truncate -s 512 $d/out
+            cmp $d/rand $d/out
+        fi
+    done
+}
+
+test dos 1 <<'EOF'
+2048 1023 L -
+EOF
+
+test dos 2 <<'EOF'
+2048 1023 L -
+4096 4095 L -
+EOF
+
+test dos 3 <<'EOF'
+2048 1023 L -
+4096 4095 L -
+8192 8191 L -
+EOF
+
+test dos 4 <<'EOF'
+2048 1023 L -
+4096 4095 L -
+8192 8191 L -
+16384 16383 L -
+EOF
+
+test dos 6 4 <<'EOF'
+2048 2047 L -
+4096 4095 L -
+8192 8191 L -
+16384 16383 E -
+17000 999 L -
+18000 999 L -
+EOF
+
+test dos 6 1 <<'EOF'
+16384 16383 E -
+2048 2047 L -
+4096 4095 L -
+8192 8191 L -
+17000 999 L -
+18000 999 L -
+EOF
+
+test gpt 1 <<'EOF'
+2048 1023 L -
+EOF
+
+test gpt 2 <<'EOF'
+2048 1023 L -
+4096 4095 L -
+EOF
+
+test gpt 3 <<'EOF'
+2048 1023 L -
+4096 4095 L -
+8192 8191 L -
+EOF
+
+test gpt 4 <<'EOF'
+2048 1023 L -
+4096 4095 L -
+8192 8191 L -
+16384 16383 L -
+EOF
+
+test gpt 5 <<'EOF'
+2048 2047 L -
+4096 4095 L -
+8192 8191 L -
+16384 16383 L -
+32768 32767 L -
+EOF
+
+test gpt 6 <<'EOF'
+2048 2047 L -
+4096 4095 L -
+8192 8191 L -
+16384 16383 L -
+32768 32767 L -
+65536 65535 L -
+EOF
-- 
2.20.1

Thanks: Eric Blake for suggesting the test.
---
 tests/Makefile.am        |  4 +-
 tests/test-partition2.sh | 87 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 18936e4..0db5045 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -795,7 +795,9 @@ test_offset_LDADD = libtest.la $(LIBGUESTFS_LIBS)
 TESTS += test-offset2.sh
 
 # partition filter test.
-TESTS += test-partition1.sh
+TESTS += \
+	test-partition1.sh \
+	test-partition2.sh
 
 # truncate filter tests.
 TESTS += \
diff --git a/tests/test-partition2.sh b/tests/test-partition2.sh
new file mode 100755
index 0000000..71bac55
--- /dev/null
+++ b/tests/test-partition2.sh
@@ -0,0 +1,87 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2019 Red Hat Inc.
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -e
+set -x
+
+# Test that qemu-img is available and working.
+if ! qemu-img --help >/dev/null; then
+    echo "$0: missing or broken qemu-img"
+    exit 77
+fi
+
+files="partition2.log"
+rm -f $files
+cleanup_fn rm -f $files
+
+# Select partition 3 from a 2 partition disk.
+nbdkit -U - -r -f -v --filter=partition \
+         partitioning disk disk partition-type=mbr partition=3 \
+         --run 'qemu-img info $nbd' > partition2.log 2>&1 ||:
+
+cat partition2.log
+
+grep "MBR partition 3 not found" partition2.log
+
+# Selection partition 11 from a 9 partition disk.  NB: partition 4 is
+# the extended partition so it is skipped.  This test is slightly
+# different from above as it invokes the code supporting logical
+# partitions.
+nbdkit -U - -r -f -v --filter=partition \
+         partitioning disk disk disk disk disk disk disk disk disk \
+         partition-type=mbr partition=11 \
+         --run 'qemu-img info $nbd' > partition2.log 2>&1 ||:
+
+cat partition2.log
+
+grep "MBR partition 11 not found" partition2.log
+
+# It should be impossible to select an extended partition.
+nbdkit -U - -r -f -v --filter=partition \
+         partitioning disk disk disk disk disk partition-type=mbr partition=4 \
+         --run 'qemu-img info $nbd' > partition2.log 2>&1 ||:
+
+cat partition2.log
+
+grep "MBR partition 4 not found" partition2.log
+
+# Selecting a logical partition on a disk without an extended
+# partition gives a different error.
+nbdkit -U - -r -f -v --filter=partition \
+         partitioning disk disk partition-type=mbr partition=5 \
+         --run 'qemu-img info $nbd' > partition2.log 2>&1 ||:
+
+cat partition2.log
+
+grep "there is no extended partition" partition2.log
-- 
2.20.1

It's a bash builtin, it always exists.

Thanks: Eric Blake.
---
 tests/test-partitioning4.sh | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/tests/test-partitioning4.sh b/tests/test-partitioning4.sh
index 4177640..eb76b3c 100755
--- a/tests/test-partitioning4.sh
+++ b/tests/test-partitioning4.sh
@@ -43,15 +43,6 @@ source ./functions.sh
 set -e
 set -x
 
-# Check if the printf utility is available.  This is probably using
-# the bash builtin (not the one from coreutils) which does not
-# understand --flags, so we have to test it using a dummy format
-# string.
-if ! printf ""; then
-    echo "$0: missing or broken printf"
-    exit 77
-fi
-
 # Test that qemu-img works
 if ! qemu-img --help >/dev/null; then
     echo "$0: missing or broken qemu-img"
-- 
2.20.1

Since both the partitioning plugin and partition filter now support
MBR logical partitions, the original test for GPT
(test-partitioning4.sh) can be duplicated and modified to test MBR.
---
 tests/Makefile.am           |  4 +-
 tests/test-partitioning4.sh |  2 +-
 tests/test-partitioning6.sh | 83 +++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0db5045..b950847 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -85,6 +85,7 @@ EXTRA_DIST = \
 	test-partitioning3.sh \
 	test-partitioning4.sh \
 	test-partitioning5.sh \
+	test-partitioning6.sh \
 	test-pattern.sh \
 	test-pattern-largest.sh \
 	test-pattern-largest-for-qemu.sh \
@@ -456,7 +457,8 @@ test_memory_LDADD = libtest.la $(LIBGUESTFS_LIBS)
 # partitioning plugin test.
 TESTS += \
 	test-partitioning1.sh \
-	test-partitioning4.sh
+	test-partitioning4.sh \
+	test-partitioning6.sh
 if HAVE_GUESTFISH
 TESTS += \
 	test-partitioning2.sh \
diff --git a/tests/test-partitioning4.sh b/tests/test-partitioning4.sh
index eb76b3c..8988a5c 100755
--- a/tests/test-partitioning4.sh
+++ b/tests/test-partitioning4.sh
@@ -33,7 +33,7 @@
 
 # Test the partitioning plugin.
 #
-# Test 4: Test > 128 partitions.
+# Test 4: Test > 128 partitions using GPT.
 #
 # virtio-scsi (used by libguestfs) doesn't support more than 15
 # partitions.  In fact the only client which supports this is our own
diff --git a/tests/test-partitioning6.sh b/tests/test-partitioning6.sh
new file mode 100755
index 0000000..6dbb215
--- /dev/null
+++ b/tests/test-partitioning6.sh
@@ -0,0 +1,83 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2019 Red Hat Inc.
+# All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Test the partitioning plugin.
+#
+# Test 4: Test > 128 partitions using MBR.
+#
+# virtio-scsi (used by libguestfs) doesn't support more than 15
+# partitions.  In fact the only client which supports this is our own
+# partition filter so we use that for the test.
+
+source ./functions.sh
+set -e
+set -x
+
+# Test that qemu-img works
+if ! qemu-img --help >/dev/null; then
+    echo "$0: missing or broken qemu-img"
+    exit 77
+fi
+
+d=partitioning6.d
+rm -rf $d
+mkdir $d
+cleanup_fn rm -rf $d
+
+# Create the partitions.
+for i in {1..768}; do
+    truncate -s 1 $(printf '%s/part.%04d' $d $i)
+done
+
+# Create partition 250 containing data and truncate it to a whole
+# number of sectors.
+rm $d/part.0250
+for i in {0..1000}; do
+    echo -n "hello " >> $d/part.0250
+done
+truncate -s 6144 $d/part.0250
+
+# Run nbdkit.
+#
+# Note we select partition 251 (not 250) because partition 4 is the
+# extended partition and everything partition following moves up by 1.
+nbdkit -f -v -D partitioning.regions=1 -U - \
+             --filter=partition \
+             partitioning \
+             $d/part.* \
+             partition-type=mbr \
+             partition=251 \
+             --run "qemu-img convert \$nbd $d/out"
+
+# The output should be identical to partition 250.
+cmp $d/part.0250 $d/out
-- 
2.20.1

On 1/22/19 4:20 PM, Richard W.M. Jones wrote:
> I think this addresses everything in Eric's v2 review.
Yep, I'm not spotting anything else; I think you're ready to go.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 1/22/19 4:20 PM, Richard W.M. Jones wrote:
> Test the partition filter against real life partition tables created
> by sfdisk.
> ---
> +
> +test dos 1 <<'EOF'
> +2048 1023 L -
> +EOF
> +
> +test dos 2 <<'EOF'
> +2048 1023 L -
> +4096 4095 L -
> +EOF
I don't know how to make sfdisk populate ONLY partition 2 using
non-label form (short of populating both 1 and 2, then deleting
partition 1), but as there are disks in the wild like that, it might
make for an interesting test.  But I _do_ know how to do it using
labeled form, by doing it manually and then letting --dump tell me how
to repeat it:

$ truncate --size 1G disk
$ sfdisk -X dos disk <<\EOF
, 100M
, 100M
EOF
$ sfdisk --delete disk 1
$ sfdisk --dump disk
label: dos
label-id: 0x1b18e388
device: disk
unit: sectors

disk2 : start=      206848, size=      204800, type=83


In fact, labeled form is how you convince sfdisk to create partitions in
non-ascending order, to the point that 'sfdisk --reorder' then shuffles
the tables (but not the partitions themselves) to be back in ascending
order (of course, reordering can invalidate /etc/fstab and other
references in the guest OS that were keying off of partition number
instead of label)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org