Libguestfs - Jan 2019 - Re: [PATCH nbdkit v2 1/4] partitioning plugin: Support MBR logical partitions.

This is a revised version of the two series previously posted here:

https://www.redhat.com/archives/libguestfs/2019-January/msg00137.html
https://www.redhat.com/archives/libguestfs/2019-January/msg00139.html

There have been many smaller changes but the highlights are:

 - Using SECTOR_SIZE instead of hard-coding 512 everywhere.

 - Additional safety checks that the EBR chain doesn't
   jump outside the disk.

 - The better test now writes different random data into each
   partition rather than the same string.

 - I pushed the pure refactoring changes.

Rich.

---
 .../nbdkit-partitioning-plugin.pod            |  29 ++--
 plugins/partitioning/virtual-disk.h           |  12 +-
 plugins/partitioning/partition-mbr.c          | 132 +++++++++++++++---
 plugins/partitioning/partitioning.c           |  28 ++--
 plugins/partitioning/virtual-disk.c           |  42 +++++-
 tests/Makefile.am                             |   4 +-
 tests/test-partitioning5.sh                   |  96 +++++++++++++
 7 files changed, 281 insertions(+), 62 deletions(-)

diff --git a/plugins/partitioning/nbdkit-partitioning-plugin.pod
b/plugins/partitioning/nbdkit-partitioning-plugin.pod
index 57a1133..49436d9 100644
--- a/plugins/partitioning/nbdkit-partitioning-plugin.pod
+++ b/plugins/partitioning/nbdkit-partitioning-plugin.pod
@@ -27,23 +27,12 @@ access use the I<-r> flag.
 
 =head2 Partition table type
 
-You can choose either an MBR partition table, which is limited to 4
-partitions, or a GPT partition table.  In theory GPT supports an
-unlimited number of partitions.
-
-The rule for selecting the partition table type is:
+Using the C<partition-type> parameter you can choose either an MBR or
+a GPT partition table.  If this parameter is I<not> present then:
 
 =over 4
 
-=item C<partition-type=mbr> parameter on the command line
-
-⇒ MBR is selected
-
-=item C<partition-type=gpt> parameter on the command line
-
-⇒ GPT is selected
-
-=item else, number of files E<gt> 4
+=item number of files E<gt> 4
 
 ⇒ GPT
 
@@ -131,7 +120,13 @@ C<./> (absolute paths do not need modification).
 =item B<partition-type=mbr>
 
 Add an MBR (DOS-style) partition table.  The MBR format is maximally
-compatible with all clients, but only supports up to 4 partitions.
+compatible with all clients.
+
+If there are E<gt> 4 partitions then the first three files are mapped
+to primary partitions, an extended partition
+(L<https://en.wikipedia.org/wiki/Extended_boot_record>) is created as
+partition 4, and the files starting from the 4th will appear as
+partition 5 and upwards.
 
 =item B<partition-type=gpt>
 
@@ -163,10 +158,6 @@ a Linux filesystem.
 
 =head1 LIMITS
 
-This plugin only supports B<primary> MBR partitions, hence the limit
-of 4 partitions with MBR.  This might be increased in future if we
-implement support for logical/extended MBR partitions.
-
 Although this plugin can create GPT partition tables containing more
 than 128 GPT partitions (in fact, unlimited numbers of partitions),
 some clients will not be able to handle this.
diff --git a/plugins/partitioning/virtual-disk.h
b/plugins/partitioning/virtual-disk.h
index 8b798a1..f59df70 100644
--- a/plugins/partitioning/virtual-disk.h
+++ b/plugins/partitioning/virtual-disk.h
@@ -104,7 +104,7 @@ extern struct file *files;
 extern size_t nr_files;
 
 extern struct regions regions;
-extern unsigned char *primary, *secondary;
+extern unsigned char *primary, *secondary, **ebr;
 
 /* Main entry point called after files array has been populated. */
 extern int create_virtual_disk_layout (void);
@@ -115,16 +115,16 @@ extern int create_virtual_disk_layout (void);
 extern int parse_guid (const char *str, char *out)
   __attribute__((__nonnull__ (1, 2)));
 
-/* Internal functions for creating MBR and GPT layouts.  These are
- * published here because the GPT code calls into the MBR code, but
- * are not meant to be called from the main plugin.
+/* Internal function for creating a single MBR PTE.  The GPT code
+ * calls this for creating the protective MBR.
  */
-extern void create_mbr_partition_table (unsigned char *out)
-  __attribute__((__nonnull__ (1)));
 extern void create_mbr_partition_table_entry (const struct region *,
                                               bool bootable, int partition_id,
                                               unsigned char *)
   __attribute__((__nonnull__ (1, 4)));
+
+/* Create MBR or GPT layout. */
+extern void create_mbr_layout (void);
 extern void create_gpt_layout (void);
 
 #endif /* NBDKIT_VIRTUAL_DISK_H */
diff --git a/plugins/partitioning/partition-mbr.c
b/plugins/partitioning/partition-mbr.c
index 0e72cf0..9f4efc8 100644
--- a/plugins/partitioning/partition-mbr.c
+++ b/plugins/partitioning/partition-mbr.c
@@ -50,27 +50,125 @@
 #include "regions.h"
 #include "virtual-disk.h"
 
-/* Create the partition table. */
+static const struct region *find_file_region (size_t i, size_t *j);
+static const struct region *find_ebr_region (size_t i, size_t *j);
+
+/* Create the MBR and optionally EBRs. */
 void
-create_mbr_partition_table (unsigned char *out)
+create_mbr_layout (void)
 {
-  size_t i, j;
-
-  for (j = 0; j < nr_regions (&regions); ++j) {
-    const struct region *region = get_region (&regions, j);
-
-    if (region->type == region_file) {
-      i = region->u.i;
-      assert (i < 4);
-      create_mbr_partition_table_entry (region, i == 0,
-                                        files[i].mbr_id,
-                                        &out[0x1be + 16*i]);
-    }
-  }
+  size_t i, j = 0;
 
   /* Boot signature. */
-  out[0x1fe] = 0x55;
-  out[0x1ff] = 0xaa;
+  primary[0x1fe] = 0x55;
+  primary[0x1ff] = 0xaa;
+
+  if (nr_files <= 4) {
+    /* Basic MBR with no extended partition. */
+    for (i = 0; i < nr_files; ++i) {
+      const struct region *region = find_file_region (i, &j);
+
+      create_mbr_partition_table_entry (region, i == 0, files[i].mbr_id,
+                                        &primary[0x1be + 16*i]);
+    }
+  }
+  else {
+    struct region region;
+    const struct region *rptr, *eptr0, *eptr;
+
+    /* The first three primary partitions correspond to the first
+     * three files.
+     */
+    for (i = 0; i < 3; ++i) {
+      rptr = find_file_region (i, &j);
+      create_mbr_partition_table_entry (rptr, i == 0, files[i].mbr_id,
+                                        &primary[0x1be + 16*i]);
+    }
+
+    /* The fourth partition is an extended PTE and does not correspond
+     * to any file.  This partition starts with the first EBR, so find
+     * it.  The partition extends to the end of the disk.
+     */
+    eptr0 = find_ebr_region (3, &j);
+    region.start = eptr0->start;
+    region.end = virtual_size (&regions) - 1; /* to end of disk */
+    region.len = region.end - region.start + 1;
+    create_mbr_partition_table_entry (&region, false, 0xf,
&primary[0x1ee]);
+
+    /* The remaining files are mapped to logical partitions living in
+     * the fourth extended partition.
+     */
+    for (i = 3; i < nr_files; ++i) {
+      if (i == 3)
+        eptr = eptr0;
+      else
+        eptr = find_ebr_region (i, &j);
+      rptr = find_file_region (i, &j);
+
+      /* Signature. */
+      ebr[i-3][0x1fe] = 0x55;
+      ebr[i-3][0x1ff] = 0xaa;
+
+      /* First entry in EBR contains:
+       * offset from EBR sector to the first sector of the logical partition
+       * total count of sectors in the logical partition
+       */
+      region.start = rptr->start - eptr->start;
+      region.len = rptr->len;
+      create_mbr_partition_table_entry (&region, false, files[i].mbr_id,
+                                        &ebr[i-3][0x1be]);
+
+      if (i < nr_files-1) {
+        size_t j2 = j;
+        const struct region *enext = find_ebr_region (i+1, &j2);
+        const struct region *rnext = find_file_region (i+1, &j2);
+
+        /* Second entry in the EBR contains:
+         * address of next EBR relative to extended partition
+         * total count of sectors in the next logical partition including
+         * next EBR
+         */
+        region.start = enext->start - eptr0->start;
+        region.len = rnext->end - enext->start + 1;
+        create_mbr_partition_table_entry (&region, false, 0xf,
+                                          &ebr[i-3][0x1ce]);
+      }
+    }
+  }
+}
+
+/* Find the region corresponding to file[i].
+ * j is a scratch register ensuring we only do a linear scan.
+ */
+static const struct region *
+find_file_region (size_t i, size_t *j)
+{
+  const struct region *region;
+
+  for (; *j < nr_regions (&regions); ++(*j)) {
+    region = get_region (&regions, *j);
+    if (region->type == region_file && region->u.i == i)
+      return region;
+  }
+  abort ();
+}
+
+/* Find the region corresponding to EBR of file[i] (i >= 3).
+ * j is a scratch register ensuring we only do a linear scan.
+ */
+static const struct region *
+find_ebr_region (size_t i, size_t *j)
+{
+  const struct region *region;
+
+  assert (i >= 3);
+
+  for (; *j < nr_regions (&regions); ++(*j)) {
+    region = get_region (&regions, *j);
+    if (region->type == region_data && region->u.data ==
ebr[i-3])
+      return region;
+  }
+  abort ();
 }
 
 static void
diff --git a/plugins/partitioning/partitioning.c
b/plugins/partitioning/partitioning.c
index 3992bef..689cb24 100644
--- a/plugins/partitioning/partitioning.c
+++ b/plugins/partitioning/partitioning.c
@@ -82,8 +82,11 @@ size_t nr_files = 0;
 /* Virtual disk layout. */
 struct regions regions;
 
-/* Primary and secondary partition tables (secondary is only used for GPT). */
-unsigned char *primary = NULL, *secondary = NULL;
+/* Primary and secondary partition tables and extended boot records.
+ * Secondary PT is only used for GPT.  EBR array of sectors is only
+ * used for MBR with > 4 partitions and has length equal to nr_files-3.
+ */
+unsigned char *primary = NULL, *secondary = NULL, **ebr = NULL;
 
 /* Used to generate random unique partition GUIDs for GPT. */
 static struct random_state random_state;
@@ -106,12 +109,17 @@ partitioning_unload (void)
   free (files);
 
   /* We don't need to free regions.regions[].u.data because it points
-   * to either primary or secondary which we free here.
+   * to primary, secondary or ebr which we free here.
    */
   free_regions (&regions);
 
   free (primary);
   free (secondary);
+  if (ebr) {
+    for (i = 0; i < nr_files-3; ++i)
+      free (ebr[i]);
+    free (ebr);
+  }
 }
 
 static int
@@ -237,17 +245,11 @@ partitioning_config_complete (void)
   total_size = 0;
   for (i = 0; i < nr_files; ++i)
     total_size += files[i].statbuf.st_size;
-
-  if (nr_files > 4)
-    needs_gpt = true;
-  else if (total_size > MAX_MBR_DISK_SIZE)
-    needs_gpt = true;
-  else
-    needs_gpt = false;
+  needs_gpt = total_size > MAX_MBR_DISK_SIZE;
 
   /* Choose default parttype if not set. */
   if (parttype == PARTTYPE_UNSET) {
-    if (needs_gpt) {
+    if (needs_gpt || nr_files > 4) {
       parttype = PARTTYPE_GPT;
       nbdkit_debug ("picking partition type GPT");
     }
@@ -257,8 +259,8 @@ partitioning_config_complete (void)
     }
   }
   else if (parttype == PARTTYPE_MBR && needs_gpt) {
-    nbdkit_error ("MBR partition table type supports a maximum of 4
partitions "
-                  "and a maximum virtual disk size of about 2 TB, "
+    nbdkit_error ("MBR partition table type supports "
+                  "a maximum virtual disk size of about 2 TB, "
                   "but you requested %zu partition(s) "
                   "and a total size of %" PRIu64 " bytes (>
%" PRIu64 ").  "
                   "Try using: partition-type=gpt",
diff --git a/plugins/partitioning/virtual-disk.c
b/plugins/partitioning/virtual-disk.c
index e2d72bc..4fe186e 100644
--- a/plugins/partitioning/virtual-disk.c
+++ b/plugins/partitioning/virtual-disk.c
@@ -69,6 +69,25 @@ create_virtual_disk_layout (void)
       nbdkit_error ("malloc: %m");
       return -1;
     }
+
+    if (nr_files > 4) {
+      /* The first 3 primary partitions will be real partitions, the
+       * 4th will be an extended partition, and so we need to store
+       * EBRs for nr_files-3 logical partitions.
+       */
+      ebr = malloc (sizeof (unsigned char *) * (nr_files-3));
+      if (ebr == NULL) {
+        nbdkit_error ("malloc: %m");
+        return -1;
+      }
+      for (i = 0; i < nr_files-3; ++i) {
+        ebr[i] = calloc (1, SECTOR_SIZE);
+        if (ebr[i] == NULL) {
+          nbdkit_error ("malloc: %m");
+          return -1;
+        }
+      }
+    }
   }
   else /* PARTTYPE_GPT */ {
     /* Protective MBR + PT header + PTA = 2 + GPT_PTA_LBAs */
@@ -117,6 +136,20 @@ create_virtual_disk_layout (void)
      */
     assert (IS_ALIGNED (offset, SECTOR_SIZE));
 
+    /* Logical partitions are preceeded by an EBR. */
+    if (parttype == PARTTYPE_MBR && nr_files > 4 && i >=
3) {
+      region.start = offset;
+      region.len = SECTOR_SIZE;
+      region.end = region.start + region.len - 1;
+      region.type = region_data;
+      region.u.data = ebr[i-3];
+      region.description = "EBR";
+      if (append_region (&regions, region) == -1)
+        return -1;
+
+      offset = virtual_size (&regions);
+    }
+
     /* Make sure each partition is aligned for best performance. */
     if (!IS_ALIGNED (offset, files[i].alignment)) {
       region.start = offset;
@@ -207,13 +240,10 @@ create_partition_table (void)
   if (parttype == PARTTYPE_GPT)
     assert (secondary != NULL);
 
-  if (parttype == PARTTYPE_MBR) {
-    assert (nr_files <= 4);
-    create_mbr_partition_table (primary);
-  }
-  else /* parttype == PARTTYPE_GPT */ {
+  if (parttype == PARTTYPE_MBR)
+    create_mbr_layout ();
+  else /* parttype == PARTTYPE_GPT */
     create_gpt_layout ();
-  }
 
   return 0;
 }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 420cb45..27abbaf 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -83,6 +83,7 @@ EXTRA_DIST = \
 	test-partitioning2.sh \
 	test-partitioning3.sh \
 	test-partitioning4.sh \
+	test-partitioning5.sh \
 	test-pattern.sh \
 	test-pattern-largest.sh \
 	test-pattern-largest-for-qemu.sh \
@@ -456,7 +457,8 @@ TESTS += \
 if HAVE_GUESTFISH
 TESTS += \
 	test-partitioning2.sh \
-	test-partitioning3.sh
+	test-partitioning3.sh \
+	test-partitioning5.sh
 endif HAVE_GUESTFISH
 
 # pattern plugin test.
diff --git a/tests/test-partitioning5.sh b/tests/test-partitioning5.sh
new file mode 100755
index 0000000..b4cb6bf
--- /dev/null
+++ b/tests/test-partitioning5.sh
@@ -0,0 +1,96 @@
+#!/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 5: Create a filesystem and embed it in an MBR logical
+# partition.  libguestfs uses virtio-scsi so the practical limit here
+# is about 15 partitions.
+
+source ./functions.sh
+set -e
+set -x
+
+files="partitioning5.pid partitioning5.sock
+       partitioning5.fs
+       partitioning5.p1 partitioning5.p2 partitioning5.p3 partitioning5.p5
partitioning5.p6 partitioning5.p7 partitioning5.p8 partitioning5.p9
partitioning5.p10 partitioning5.p11 partitioning5.p13"
+rm -f $files
+cleanup_fn rm -f $files
+
+# Test that mke2fs works
+if ! mke2fs -V; then
+    echo "$0: missing or broken mke2fs"
+    exit 77
+fi
+
+# Create partitions before and after.
+truncate -s 1 partitioning5.p1
+truncate -s 10M partitioning5.p2
+truncate -s 512 partitioning5.p3
+# partition 4 = extended partition
+truncate -s 1 partitioning5.p5
+truncate -s 512 partitioning5.p6
+truncate -s 1 partitioning5.p7
+truncate -s 1 partitioning5.p8
+truncate -s 10M partitioning5.p9
+truncate -s 512 partitioning5.p10
+truncate -s 1 partitioning5.p11
+# partition 12 = naked filesystem
+truncate -s 10M partitioning5.p13
+
+# Create the naked filesystem.
+truncate -s 20M partitioning5.fs
+mke2fs -F -t ext2 partitioning5.fs
+
+# Run nbdkit.
+start_nbdkit -P partitioning5.pid -U partitioning5.sock \
+             partitioning \
+             partitioning5.p1 partitioning5.p2 \
+             partitioning5.p3 \
+             partitioning5.p5 partitioning5.p6 \
+             partitioning5.p7 partitioning5.p8 \
+             partitioning5.p9 partitioning5.p10 \
+             partitioning5.p11 partitioning5.fs \
+             partitioning5.p13 \
+             partition-type=mbr
+
+# Connect with guestfish and read/write stuff to partition 12.
+guestfish --format=raw -a "nbd://?socket=$PWD/partitioning5.sock"
<<'EOF'
+  run
+  mount /dev/sda12 /
+  touch /hello
+  fill-pattern "abc" 10000 /pattern
+  ll /
+  umount /dev/sda12
+  sync
+EOF
-- 
2.20.1

---
 filters/partition/nbdkit-partition-filter.pod |   5 -
 filters/partition/partition-mbr.c             | 105 +++++++++++++++++-
 tests/test-partitioning1.sh                   |  22 +++-
 3 files changed, 121 insertions(+), 11 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..704410c 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,16 +75,16 @@ 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");
-    return -1;
-  }
-
+  /* Primary partition. */
   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;
@@ -86,6 +92,95 @@ find_mbr_partition (struct nbdkit_next_ops *next_ops, void
*nxdata,
     }
   }
 
+  /* Logical partition. */
+
+  /* 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;
+
+ 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.
+   */
+  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. */
+    if (next_ops->pread (nxdata, sector, sizeof sector, ebr, 0,
+                         &errno) == -1)
+      return -1;
+
+    if (i == partnum) {
+      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;
+  }
+
   nbdkit_error ("MBR partition %d not found", partnum);
   return -1;
 }
diff --git a/tests/test-partitioning1.sh b/tests/test-partitioning1.sh
index 76ab43b..8aa45b9 100755
--- a/tests/test-partitioning1.sh
+++ b/tests/test-partitioning1.sh
@@ -77,7 +77,27 @@ 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 \
+       type-guid=A2A0D0EB-E5B9-3344-87C0-68B6B72699C7 \
+       file-data \
+       type-guid=AF3DC60F-8384-7247-8E79-3D69D8477DE4 \
+       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-partition.sh | 148 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 152 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 27abbaf..007203f 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-partition.sh \
 	test-partitioning1.sh \
 	test-partitioning2.sh \
 	test-partitioning3.sh \
@@ -792,11 +793,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-partition.sh
 
 # truncate filter tests.
 TESTS += \
diff --git a/tests/test-partition.sh b/tests/test-partition.sh
new file mode 100755
index 0000000..a9b2bdf
--- /dev/null
+++ b/tests/test-partition.sh
@@ -0,0 +1,148 @@
+#!/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="partition.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
+
+    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 [ $nrparts -le 4 ] || [ $label != "dos" ] || [ $part -ne 4
]; 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 6 <<'EOF'
+2048 2047 L -
+4096 4095 L -
+8192 8191 L -
+16384 16383 E -
+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

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 | 92 +++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 007203f..dfc065e 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 \
@@ -454,7 +455,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 4177640..f80c7ae 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..ea84854
--- /dev/null
+++ b/tests/test-partitioning6.sh
@@ -0,0 +1,92 @@
+#!/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
+
+# 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"
+    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/21/19 12:15 PM, Richard W.M. Jones wrote:
> ---
>  .../nbdkit-partitioning-plugin.pod            |  29 ++--
>  plugins/partitioning/virtual-disk.h           |  12 +-
>  plugins/partitioning/partition-mbr.c          | 132 +++++++++++++++---
>  plugins/partitioning/partitioning.c           |  28 ++--
>  plugins/partitioning/virtual-disk.c           |  42 +++++-
>  tests/Makefile.am                             |   4 +-
>  tests/test-partitioning5.sh                   |  96 +++++++++++++
>  7 files changed, 281 insertions(+), 62 deletions(-)
LGTM

Should we, at some point, allow the plugin user to specify the
advertised filesystem type and/or the boot flag for various partitions
(and not just that all file names presented are exposed as non-bootable
partitions of the same hard-coded type)?  But that's a separate addition.

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

On 1/21/19 12:15 PM, Richard W.M. Jones wrote:
> ---
>  filters/partition/nbdkit-partition-filter.pod |   5 -
>  filters/partition/partition-mbr.c             | 105 +++++++++++++++++-
>  tests/test-partitioning1.sh                   |  22 +++-
>  3 files changed, 121 insertions(+), 11 deletions(-)
> 
> @@ -69,16 +75,16 @@ 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");
> -    return -1;
> -  }
> -
> +  /* Primary partition. */
>    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) {
Given a disk with only 2 partitions, but requesting access to partition
3, fails this loop, and falls into:
>        *offset_r = partition.start_sector * SECTOR_SIZE;
>        *range_r = partition.nr_sectors * SECTOR_SIZE;
> @@ -86,6 +92,95 @@ find_mbr_partition (struct nbdkit_next_ops *next_ops,
void *nxdata,
>      }
>    }
>  
> +  /* Logical partition. */
> +
> +  /* 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;
...this code, which returns the wrong error message (partition 3 is not
logical, but the real problem is that there is no partition 3 in the
master table, not that there is no extended partition).

For example:
term1$ ./nbdkit --unix nbd.sock -r -fv partitioning README README
term2$ ./nbdkit -p 10810 -r -fv --filter=partition nbd socket=nbd.sock
partition=3
term3$ ./qemu-nbd --list -p 10810
term2> ...
nbdkit: nbd[1]: error: MBR logical partition selected, but there is no
extended partition in the partition table
nbdkit: nbd[1]: debug: partition: close
nbdkit: nbd[1]: debug: close
nbdkit: nbd[1]: debug: sending request type 2 (NBD_CMD_DISC), flags 0,
offset 0, count 0, cookie 0
nbdkit: debug: permanent failure while talking to server
/home/eblake/nbdkit/nbd.sock: Bad message
term3>
qemu-nbd: Failed to read initial magic: Unexpected end-of-file before
all bytes were read

It's also a bit awkward that we don't detect the invalid partition
number until a client first connects - I don't know if that can be
improved to detect it earlier before even allowing clients, especially
if we are going to hand up on the client before even giving them the
magic number.  (I also don't know if the "Bad message" claim from
the
nbd plugin is worth improving, but that's unrelated to this series).

Similarly, if requesting partition 4, when partition 4 happens to be the
extended partition container for logical partitions, the code goes to
found_extended...
> +
> + 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.
The statement is a fair limitation on what we are willing to read, and
matches what the partitioning plugin generates, but I think there are
disks in the wild where you can have logical partitions out of order.
If we DO decide to support such disks, we'll need to make sure a bad
disk can't make us loop forever when chasing backwards.
> +   */
> +  for (i = 5; ; ++i) {
...performs the loop but never finds partition 4 (because it started
looking for partition 5)...
> +    /* 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. */
> +    if (next_ops->pread (nxdata, sector, sizeof sector, ebr, 0,
> +                         &errno) == -1)
> +      return -1;
> +
> +    if (i == partnum) {
> +      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);
In other words, this safety check matches the comment above that you
insist on making progress, but I think actual hardware can boot logical
partitions that are listed in non-ascending order.
> +      return -1;
> +    }
> +    ebr = next_ebr;
> +  }
> +
>    nbdkit_error ("MBR partition %d not found", partnum);
...and eventually gives the misleading message that the partition was
not found (when it was found, but was not usable because it is extended
rather than primary).
>    return -1;
>  }
> diff --git a/tests/test-partitioning1.sh b/tests/test-partitioning1.sh
> index 76ab43b..8aa45b9 100755
> --- a/tests/test-partitioning1.sh
> +++ b/tests/test-partitioning1.sh
> @@ -77,7 +77,27 @@ 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 \
> +       type-guid=A2A0D0EB-E5B9-3344-87C0-68B6B72699C7 \
Does type-guid even work with mbr?
> +       file-data \
> +       type-guid=AF3DC60F-8384-7247-8E79-3D69D8477DE4 \
> +       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 \
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 1/21/19 12:15 PM, Richard W.M. Jones wrote:
> 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-partition.sh | 148 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 152 insertions(+), 106 deletions(-)
Looks good (any new testing is better than the lack of testing
beforehand).  That said,
> +test dos 6 <<'EOF'
> +2048 2047 L -
> +4096 4095 L -
> +8192 8191 L -
> +16384 16383 E -
> +17000 999 L -
> +18000 999 L -
> +EOF
> +
Can sfdisk be convinced to write logical partitions out-of-order, which
would catch our current refusal to read non-ascending partitions (and/or
prove that any loop-detection code we add to allow non-ascending
partitions works), as a future enhancement to the test?

And I don't see any test of an extended partition in anything other than
primary partition 4, even though sfdisk handles that.

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

On 1/21/19 12:15 PM, Richard W.M. Jones wrote:
> 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 | 92 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 96 insertions(+), 2 deletions(-)
ACK

> @@ -0,0 +1,92 @@
> +#!/usr/bin/env bash
> +
> +# 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
Pointless check - we KNOW we are running bash, so we KNOW printf is
available as the bash builtin.

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