[PATCH 0/7] sfdisk improvements

[PATCH 0/7] sfdisk improvements

[Petr Uzel]

Hi all,


First two patches are just minor fixes unrelated to the rest.


Third one makes sfdisk not to issue a fatal warning (overridable, though),
if the partition does not start/end on the cylinder boundary and
the unit specified is other than cylinders. These days,
we shouldn't care about the CHS that much IMO...


The last 4 sfdisk patches are followup to
http://www.spinics.net/lists/util-linux-ng/msg04760.html
http://www.spinics.net/lists/util-linux-ng/msg04761.html

The goal these patches is to use 64 bit integer types in sfdisk to
represent the sector numbers. This makes it easy to check if the numbers
overflew the DOS pt limit, which is 2^32. I also tried to detect the overflow
right at the moment when it happens (like Karel suggested), but this turned out
to be ugly.

I decided to use unsigned long long to represent the 64-bit integers,
which might not be the best choice (maybe we should use off_t or uint64_t
instead??). Comments are welcomed.

Thanks,


Petr Uzel (7):
  sfdisk: file descriptor is of int type
  sfdisk: fix weird indentation in msdos_partition()
  sfdisk: make the cylinder boundary check less fatal
  sfdisk: get_disksize() returns long long
  sfdisk: introduce get_ull() function
  sfdisk: use unsigned long long to internally represent sector number
  sfdisk: warn if partition exceeds partition table limits

 fdisk/sfdisk.c |  165 +++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 121 insertions(+), 44 deletions(-)

-- 
1.7.3.4

[PATCH 1/7] sfdisk: file descriptor is of int type

[Petr Uzel]

Signed-off-by: Petr Uzel <petr.uzel@suse.cz>
---
 fdisk/sfdisk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fdisk/sfdisk.c b/fdisk/sfdisk.c
index 46245d3..81a50ee 100644
--- a/fdisk/sfdisk.c
+++ b/fdisk/sfdisk.c
@@ -156,7 +156,7 @@ fatal(char *s, ...) {
  */
 
 static int
-sseek(char *dev, unsigned int fd, unsigned long s) {
+sseek(char *dev, int fd, unsigned long s) {
     off_t in, out;
     in = ((off_t) s << 9);
     out = 1;
-- 
1.7.3.4

[PATCH 2/7] sfdisk: fix weird indentation in msdos_partition()

[Petr Uzel]

Signed-off-by: Petr Uzel <petr.uzel@suse.cz>
---
 fdisk/sfdisk.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/fdisk/sfdisk.c b/fdisk/sfdisk.c
index 81a50ee..2c9de8f 100644
--- a/fdisk/sfdisk.c
+++ b/fdisk/sfdisk.c
@@ -1550,14 +1550,13 @@ msdos_partition(char *dev, int fd, unsigned long start, struct disk_desc *z) {
 	do_warn(_("detected Disk Manager - unable to handle that\n"));
 	return 0;
     }
-    {
-	unsigned int sig = *(unsigned short *)(s->data + 2);
-	if (sig <= 0x1ae
-	    && *(unsigned short *)(s->data + sig) == 0x55aa
-	    && (1 & *(unsigned char *)(s->data + sig + 2))) {
+
+    unsigned int sig = *(unsigned short *)(s->data + 2);
+    if (sig <= 0x1ae
+	&& *(unsigned short *)(s->data + sig) == 0x55aa
+	&& (1 & *(unsigned char *)(s->data + sig + 2))) {
 	    do_warn(_("DM6 signature found - giving up\n"));
 	    return 0;
-	}
     }
 
     for (pno = 0; pno < 4; pno++, cp += sizeof(struct partition)) {
-- 
1.7.3.4

[PATCH 3/7] sfdisk: make the cylinder boundary check less fatal

[Petr Uzel]

If the specified format is not cylinders, make the cylinder boundary
check only print a warning and proceed anyways.

Signed-off-by: Petr Uzel <petr.uzel@suse.cz>
---
 fdisk/sfdisk.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fdisk/sfdisk.c b/fdisk/sfdisk.c
index 2c9de8f..820e23e 100644
--- a/fdisk/sfdisk.c
+++ b/fdisk/sfdisk.c
@@ -1313,12 +1313,14 @@ partitions_ok(struct disk_desc *z) {
 		    && (p->p.start_sect >= B.cylindersize)) {
 		    my_warn(_("Warning: partition %s does not start "
 			      "at a cylinder boundary\n"), PNO(p));
-		    return 0;
+		    if (specified_format == F_CYLINDER)
+			return 0;
 		}
 		if ((p->start + p->size) % B.cylindersize) {
 		    my_warn(_("Warning: partition %s does not end "
 			      "at a cylinder boundary\n"), PNO(p));
-		    return 0;
+		    if (specified_format == F_CYLINDER)
+			return 0;
 		}
 	    }
     }
-- 
1.7.3.4

[PATCH 4/7] sfdisk: get_disksize() returns long long

[Petr Uzel]

unsigned long is 4 bytes long on i586, which is not enough for big
HDD's with 512B sectors. Use unsigned long long, which is 8 bytes.

Signed-off-by: Petr Uzel <petr.uzel@suse.cz>
---
 fdisk/sfdisk.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fdisk/sfdisk.c b/fdisk/sfdisk.c
index 820e23e..9c94185 100644
--- a/fdisk/sfdisk.c
+++ b/fdisk/sfdisk.c
@@ -928,7 +928,7 @@ unitsize(int format) {
     }
 }
 
-static unsigned long
+static unsigned long long
 get_disksize(int format) {
     if (B.total_size && leave_last)
 	/* don't use last cylinder (--leave-last option) */
@@ -1266,7 +1266,7 @@ partitions_ok(struct disk_desc *z) {
 
     /* Do they start past zero and end before end-of-disk? */
     {
-	unsigned long ds = get_disksize(F_SECTOR);
+	unsigned long long ds = get_disksize(F_SECTOR);
 	for (p = partitions; p < partitions + partno; p++)
 	    if (p->size) {
 		if (p->start == 0) {
@@ -1925,7 +1925,7 @@ first_free(int pno, int is_extended, struct part_desc *ep, int format,
 static unsigned long
 max_length(int pno, int is_extended, struct part_desc *ep, int format,
 	   unsigned long start, struct disk_desc *z) {
-    unsigned long fu;
+    unsigned long long fu;
     unsigned long unit = unitsize(format);
     struct part_desc *partitions = &(z->partitions[0]), *pp = 0;
 
-- 
1.7.3.4

[PATCH 5/7] sfdisk: introduce get_ull() function

[Petr Uzel]

Signed-off-by: Petr Uzel <petr.uzel@suse.cz>
---
 fdisk/sfdisk.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/fdisk/sfdisk.c b/fdisk/sfdisk.c
index 9c94185..fe51908 100644
--- a/fdisk/sfdisk.c
+++ b/fdisk/sfdisk.c
@@ -1861,6 +1861,44 @@ get_ul(char *u, unsigned long *up, unsigned long def, int base) {
     return 0;
 }
 
+
+/* read a number, use default if absent */
+/* a sign gives an offset from the default */
+static int
+get_ull(char *u, unsigned long long *up, unsigned long long def, int base) {
+    char *nu;
+    int sign = 0;
+    unsigned long long val;
+
+    if (*u == '+') {
+	sign = 1;
+	u++;
+    } else if (*u == '-') {
+	sign = -1;
+	u++;
+    }
+    if (*u) {
+	errno = 0;
+	val = strtoull(u, &nu, base);
+	if (errno == ERANGE) {
+	    do_warn(_("number too big\n"));
+	    return -1;
+	}
+	if (*nu) {
+	    do_warn(_("trailing junk after number\n"));
+	    return -1;
+	}
+	if (sign == 1)
+	    val = def + val;
+	else if (sign == -1)
+	    val = def - val;
+	*up = val;
+    } else
+	*up = def;
+    return 0;
+}
+
+
 /* There are two common ways to structure extended partitions:
    as nested boxes, and as a chain. Sometimes the partitions
    must be given in order. Sometimes all logical partitions
-- 
1.7.3.4

[PATCH 6/7] sfdisk: use unsigned long long to internally represent sector number

[Petr Uzel]

Unsigned long is only 4 bytes long on i586, which is not enough to
represent sector number on todays large disks. Use unsigned long long,
which is 8 bytes long, to store the sector numbers internally, so that
we could later make some sanity checks and warn the user if the sector
numbers exceed the limits imposed by DOS partition table format.

Signed-off-by: Petr Uzel <petr.uzel@suse.cz>
---
 fdisk/sfdisk.c |   58 ++++++++++++++++++++++++++++----------------------------
 1 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/fdisk/sfdisk.c b/fdisk/sfdisk.c
index fe51908..d1ba9d6 100644
--- a/fdisk/sfdisk.c
+++ b/fdisk/sfdisk.c
@@ -186,7 +186,7 @@ sseek(char *dev, int fd, unsigned long s) {
  */
 struct sector {
     struct sector *next;
-    unsigned long sectornumber;
+    unsigned long long sectornumber;
     int to_be_written;
     char data[512];
 } *sectorhead;
@@ -203,7 +203,7 @@ free_sectors(void) {
 }
 
 static struct sector *
-get_sector(char *dev, int fd, unsigned long sno) {
+get_sector(char *dev, int fd, unsigned long long sno) {
     struct sector *s;
 
     for (s = sectorhead; s; s = s->next)
@@ -739,9 +739,9 @@ copy_from_part(struct partition *p, char *cp) {
    for equality with EXTENDED_PARTITION (and these Disk Manager types). */
 
 struct part_desc {
-    unsigned long start;
-    unsigned long size;
-    unsigned long sector, offset;	/* disk location of this info */
+    unsigned long long start;
+    unsigned long long size;
+    unsigned long long sector, offset;	/* disk location of this info */
     struct partition p;
     struct part_desc *ep;	/* extended partition containing this one */
     int ptype;
@@ -980,27 +980,27 @@ out_partition_header(char *dev, int format, struct geometry G) {
 }
 
 static void
-out_rounddown(int width, unsigned long n, unsigned long unit, int inc) {
-    printf("%*lu", width, inc + n / unit);
+out_rounddown(int width, unsigned long long n, unsigned long unit, int inc) {
+    printf("%*llu", width, inc + n / unit);
     if (unit != 1)
 	putchar((n % unit) ? '+' : ' ');
     putchar(' ');
 }
 
 static void
-out_roundup(int width, unsigned long n, unsigned long unit, int inc) {
-    if (n == (unsigned long)(-1))
+out_roundup(int width, unsigned long long n, unsigned long unit, int inc) {
+    if (n == (unsigned long long)(-1))
 	printf("%*s", width, "-");
     else
-	printf("%*lu", width, inc + n / unit);
+	printf("%*llu", width, inc + n / unit);
     if (unit != 1)
 	putchar(((n + 1) % unit) ? '-' : ' ');
     putchar(' ');
 }
 
 static void
-out_roundup_size(int width, unsigned long n, unsigned long unit) {
-    printf("%*lu", width, (n + unit - 1) / unit);
+out_roundup_size(int width, unsigned long long n, unsigned long unit) {
+    printf("%*llu", width, (n + unit - 1) / unit);
     if (unit != 1)
 	putchar((n % unit) ? '-' : ' ');
     putchar(' ');
@@ -1047,7 +1047,7 @@ get_fdisk_geometry(struct disk_desc *z) {
 static void
 out_partition(char *dev, int format, struct part_desc *p,
 	      struct disk_desc *z, struct geometry G) {
-    unsigned long start, end, size;
+    unsigned long long start, end, size;
     int pno, lpno;
 
     if (!format && !(format = specified_format))
@@ -1068,8 +1068,8 @@ out_partition(char *dev, int format, struct part_desc *p,
     size = p->size;
 
     if (dump) {
-	printf(" start=%9lu", start);
-	printf(", size=%9lu", size);
+	printf(" start=%9llu", start);
+	printf(", size=%9llu", size);
 	if (p->ptype == DOS_TYPE) {
 	    printf(", Id=%2x", p->p.sys_type);
 	    if (p->p.bootable == 0x80)
@@ -1258,7 +1258,7 @@ partitions_ok(struct disk_desc *z) {
 		if (is_extended(q->p.sys_type))
 		    if (p->start <= q->start && p->start + p->size > q->start) {
 			my_warn(_("Warning: partition %s contains part of "
-				  "the partition table (sector %lu),\n"
+				  "the partition table (sector %llu),\n"
 				  "and will destroy it when filled\n"),
 				PNO(p), q->start);
 			return 0;
@@ -1391,7 +1391,7 @@ static void
 extended_partition(char *dev, int fd, struct part_desc *ep, struct disk_desc *z) {
     char *cp;
     struct sector *s;
-    unsigned long start, here, next;
+    unsigned long long start, here, next;
     int i, moretodo = 1;
     struct partition p;
     struct part_desc *partitions = &(z->partitions[0]);
@@ -1489,7 +1489,7 @@ static void
 bsd_partition(char *dev, int fd, struct part_desc *ep, struct disk_desc *z) {
     struct bsd_disklabel *l;
     struct bsd_partition *bp, *bp0;
-    unsigned long start = ep->start;
+    unsigned long long start = ep->start;
     struct sector *s;
     struct part_desc *partitions = &(z->partitions[0]);
     size_t pno = z->partno;
@@ -1916,10 +1916,10 @@ int all_logicals_inside_outermost_extended = 1;
 enum { NESTED, CHAINED, ONESECTOR } boxes = NESTED;
 
 /* find the default value for <start> - assuming entire units */
-static unsigned long
+static unsigned long long
 first_free(int pno, int is_extended, struct part_desc *ep, int format,
-	   unsigned long mid, struct disk_desc *z) {
-    unsigned long ff, fff;
+	   unsigned long long mid, struct disk_desc *z) {
+    unsigned long long ff, fff;
     unsigned long unit = unitsize(format);
     struct part_desc *partitions = &(z->partitions[0]), *pp = 0;
 
@@ -1960,9 +1960,9 @@ first_free(int pno, int is_extended, struct part_desc *ep, int format,
 }
 
 /* find the default value for <size> - assuming entire units */
-static unsigned long
+static unsigned long long
 max_length(int pno, int is_extended, struct part_desc *ep, int format,
-	   unsigned long start, struct disk_desc *z) {
+	   unsigned long long start, struct disk_desc *z) {
     unsigned long long fu;
     unsigned long unit = unitsize(format);
     struct part_desc *partitions = &(z->partitions[0]), *pp = 0;
@@ -1992,7 +1992,7 @@ max_length(int pno, int is_extended, struct part_desc *ep, int format,
 /* ep is 0 or points to surrounding extended partition */
 static int
 compute_start_sect(struct part_desc *p, struct part_desc *ep) {
-    unsigned long base;
+    unsigned long long base;
     int inc = (DOS && B.sectors) ? B.sectors : 1;
     int delta;
 
@@ -2004,7 +2004,7 @@ compute_start_sect(struct part_desc *p, struct part_desc *ep) {
 	delta = 0;
 
     if (delta < 0) {
-	unsigned long old_size = p->size;
+	unsigned long long old_size = p->size;
 	p->start -= delta;
 	p->size += delta;
 	if (is_extended(p->p.sys_type) && boxes == ONESECTOR)
@@ -2073,7 +2073,7 @@ read_line(int pno, struct part_desc *ep, char *dev, int interactive,
     char *fields[11];
     int fno, pct = pno % 4;
     struct part_desc p, *orig;
-    unsigned long ff, ff1, ul, ml, ml1, def;
+    unsigned long long ff, ff1, ul, ml, ml1, def;
     int format, lpno, is_extd;
 
     if (eof || eob)
@@ -2144,7 +2144,7 @@ read_line(int pno, struct part_desc *ep, char *dev, int interactive,
 	ul = EXTENDED_PARTITION;
     else if (!strcmp(fields[2], "X"))
 	ul = LINUX_EXTENDED;
-    else if (get_ul(fields[2], &ul, LINUX_NATIVE, 16))
+    else if (get_ull(fields[2], &ul, LINUX_NATIVE, 16))
 	return 0;
     if (ul > 255) {
 	my_warn(_("Illegal type\n"));
@@ -2160,7 +2160,7 @@ read_line(int pno, struct part_desc *ep, char *dev, int interactive,
     if (fno < 1 || !*(fields[0]))
 	p.start = def;
     else {
-	if (get_ul(fields[0], &ul, def / unitsize(0), 0))
+	if (get_ull(fields[0], &ul, def / unitsize(0), 0))
 	    return 0;
 	p.start = ul * unitsize(0);
 	p.start -= (p.start % unitsize(format));
@@ -2175,7 +2175,7 @@ read_line(int pno, struct part_desc *ep, char *dev, int interactive,
     else if (!strcmp(fields[1], "+"))
 	p.size = ml1;
     else {
-	if (get_ul(fields[1], &ul, def / unitsize(0), 0))
+	if (get_ull(fields[1], &ul, def / unitsize(0), 0))
 	    return 0;
 	p.size = ul * unitsize(0) + unitsize(format) - 1;
 	p.size -= (p.size % unitsize(format));
-- 
1.7.3.4

[PATCH 7/7] sfdisk: warn if partition exceeds partition table limits

[Petr Uzel]

Warn the user if the partition to be created exceeds limits
imposed by the DOS partition table, which is:
* partition has to start on sector < 2^32
* partition size has to be < 2^32 sectors

For 512-byte logical sector size, these limits are ~2.2 TiB.

Signed-off-by: Petr Uzel <petr.uzel@suse.cz>
---
 fdisk/sfdisk.c |   44 +++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/fdisk/sfdisk.c b/fdisk/sfdisk.c
index d1ba9d6..db1a6d7 100644
--- a/fdisk/sfdisk.c
+++ b/fdisk/sfdisk.c
@@ -46,6 +46,7 @@
 #include <sys/ioctl.h>
 #include <sys/stat.h>
 #include <sys/utsname.h>
+#include <limits.h>
 
 #include "c.h"
 #include "nls.h"
@@ -1195,7 +1196,7 @@ pnumber(struct part_desc *p, struct disk_desc *z) {
 }
 
 static int
-partitions_ok(struct disk_desc *z) {
+partitions_ok(int fd, struct disk_desc *z) {
     struct part_desc *partitions = &(z->partitions[0]), *p, *q;
     int partno = z->partno;
 
@@ -1282,6 +1283,43 @@ partitions_ok(struct disk_desc *z) {
 	    }
     }
 
+    int sector_size;
+    if (blkdev_get_sector_size(fd, &sector_size) == -1)
+	sector_size = DEFAULT_SECTOR_SIZE;
+
+    /* Is the size of partitions less than 2^32 sectors limit? */
+    for (p = partitions; p < partitions + partno; p++)
+	if (p->size > UINT_MAX) {
+	    unsigned long long bytes = p->size * sector_size;
+	    int giga = bytes / 1000000000;
+	    int hectogiga = (giga + 50) / 100;
+	    my_warn(_("Warning: partition %s has size %d.%d TB (%llu bytes),\n"
+		      "which is larger than the %llu bytes limit imposed\n"
+		      "by the DOS partition table for %d-byte sectors\n"),
+		    PNO(p), hectogiga / 10, hectogiga % 10,
+		    bytes,
+		    (unsigned long long) UINT_MAX * sector_size,
+		    sector_size);
+	    return 0;
+	}
+
+    /* Do the partitions start below the 2^32 sectors limit? */
+    for (p = partitions; p < partitions + partno; p++)
+	if (p->start > UINT_MAX) {
+	    unsigned long long bytes = p->start * sector_size;
+	    int giga = bytes / 1000000000;
+	    int hectogiga = (giga + 50) / 100;
+	    my_warn(_("Warning: partition %s starts at sector %llu (%d.%d TB for %d-byte sectors),\n"
+		      "which exceeds the DOS partition table limit of %llu sectors\n"),
+		    PNO(p),
+		    p->start,
+		    hectogiga / 10,
+		    hectogiga % 10,
+		    sector_size,
+		    (unsigned long long) UINT_MAX);
+	    return 0;
+	}
+
     /* At most one chain of DOS extended partitions ? */
     /* It seems that the OS/2 fdisk has the additional requirement
        that the extended partition must be the fourth one */
@@ -2875,7 +2913,7 @@ do_list(char *dev, int silent) {
 	out_partitions(dev, z);
 
     if (verify) {
-	if (partitions_ok(z))
+	if (partitions_ok(fd, z))
 	    my_warn(_("%s: OK\n"), dev);
 	else
 	    exit_status = 1;
@@ -3212,7 +3250,7 @@ do_fdisk(char *dev) {
 	printf(_("New situation:\n"));
 	out_partitions(dev, z);
 
-	if (!partitions_ok(z) && !force) {
+	if (!partitions_ok(fd, z) && !force) {
 	    if (!interactive)
 		fatal(_("I don't like these partitions - nothing changed.\n"
 			"(If you really want this, use the --force option.)\n"));
-- 
1.7.3.4

Re: [PATCH 0/7] sfdisk improvements

[Karel Zak]

On Mon, Sep 19, 2011 at 03:29:08PM +0200, Petr Uzel wrote:
> Third one makes sfdisk not to issue a fatal warning (overridable, though),
> if the partition does not start/end on the cylinder boundary and
> the unit specified is other than cylinders. These days,
> we shouldn't care about the CHS that much IMO...

 Right, see also cc393e381def1540db7b1273dde9410bea616b22. Giulio has
 added some improvements for -L (Linux) option to disable some silly
 tests.

> I decided to use unsigned long long to represent the 64-bit integers,
> which might not be the best choice (maybe we should use off_t or uint64_t
> instead??). Comments are welcomed.

 Well, "unsigned long long" is horribly long, but probably good enough
 for now (as we already using this type in fdisk.c).

 The real solution is to start to work on a new fdisks...

 Applied, thanks.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com