Two bugs that cancel each other in chs_get_sector and chs_to_sector

[Håkon Løvdal]

Hi. This applies to parted 1.6.12.

The calculation "chs->sector % 0x40 - 1;" in chs_get_sector is wrong.
It should not subtract one here. Besides using the modulo operator
here is weird at best. The sector is stored in the 6 least significant
bits and a binary and operator together with a proper bitmask is the
Correct(TM) way to extract it...

Correspondingly there is a missing one subtraction in
ped_disk_print. With my correction the code now matches the
normal translation formula "c*H*S + h*S + (s-1)" (as mentioned at
http://www.win.tue.nl/~aeb/linux/Large-Disk-3.html for instance).

So the two functions chs_get_sector and chs_to_sector are very
straight forward. However the chs_get_sector function is also used in
probe_partition_for_geom and I updated correspondingly there, but to be
honest I did not quite understand the algorithm/formula. Could someone
enlighten me? Adding some printfs gave me the following for my disk:

c=0, h=1, s=1, a=63, a_=63, C=1019, H=15, S=63, A=1028159, A_=1028097
(A_ * h - a_ * H) = 1027152
(a_ * C - A_ * c) = 64197
(A_ * h - a_ * H) / (a_ * C - A_ * c) = 16.000000

Also I replaced a few zeros with the corresponding enum
value and replaced all usage of 1022 with 1023; it is
very close to Numeric Literals Coding Obfuscation, see
http://www.web-hits.org/txt/codingunmaintainable.html (and in fact
chs_to_sector used to give up one cylinder too early).

Maybe the last if test in chs_to_sector should be "PED_ASSERT(s ==
0, return 0);" instead?

Here is my patch:

--- parted-1.6.12.orig/libparted/disk_dos.c     2004-09-03
23:39:58.479970904 +0200
+++ parted-1.6.12/libparted/disk_dos.c  2004-09-04 00:54:00.901620160 +0200
@@ -84,12 +84,18 @@
 #define PARTITION_LINUX_RAID   0xfd
 #define PARTITION_LINUX_LVM_OLD 0xfe
 
 typedef struct _DosRawPartition                DosRawPartition;
 typedef struct _DosRawTable            DosRawTable;
 
+#define BIN(b7, b6, b5, b4,  b3, b2, b1, b0) \
+( \
+       ((b7)<<7) + ((b6)<<6) + ((b5)<<5) + ((b4)<<4) + \
+       ((b3)<<3) + ((b2)<<2) + ((b1)<<1) + ((b0)<<0)   \
+)
+
 /* note: lots of bit-bashing here, thus, you shouldn't look inside it.
  * Use chs_to_sector() and sector_to_chs() instead.
  */
 typedef struct {
        uint8_t         head;
        uint8_t         sector;
@@ -231,35 +237,35 @@
        return chs->head;
 }
 
 static int
 chs_get_sector (const RawCHS* chs)
 {
-       return chs->sector % 0x40 - 1;
+       return chs->sector & BIN(0,0,1,1, 1,1,1,1);
 }
 
 static PedSector
 chs_to_sector (PedDevice* dev, const PedCHSGeometry *bios_geom,
               const RawCHS* chs)
 {
-       PedSector       c;              /* not measured in sectors, but need */
-       PedSector       h;              /* lots of bits */
-       PedSector       s;
+       PedSector c;    /* instead of adding lots of typecasts in the return */
+       PedSector h;    /* statement to avoid overflows, just use the same */
+       PedSector s;    /* size as returned by the function for all variables */
 
        PED_ASSERT (bios_geom != NULL, return 0);
        PED_ASSERT (chs != NULL, return 0);
 
        c = chs_get_cylinder (chs);
        h = chs_get_head (chs);
        s = chs_get_sector (chs);
 
-       if (c >= 1022)                  /* MAGIC: C/H/S is irrelevant */
+       if (c >= 1023)                  /* MAGIC: C/H/S is irrelevant */
                return 0;
-       if (s < 0)
+       if (s == 0)                     /* is this test really necessary? */
                return 0;
-       return ((c * bios_geom->heads + h) * bios_geom->sectors + s)
+       return ((c * bios_geom->heads + h) * bios_geom->sectors + (s-1))
                * (dev->sector_size / 512);
 }
 
 static void
 sector_to_chs (PedDevice* dev, const PedCHSGeometry* bios_geom,
               PedSector sector, RawCHS* chs)
@@ -441,26 +447,26 @@
 
        start_chs = &dos_data->orig->raw_part.chs_start;
        c = chs_get_cylinder (start_chs);
        h = chs_get_head (start_chs);
        s = chs_get_sector (start_chs);
        a = dos_data->orig->geom.start;
-       a_ = a - s;
+       a_ = a - (s - 1);
 
        end_chs = &dos_data->orig->raw_part.chs_end;
        C = chs_get_cylinder (end_chs);
        H = chs_get_head (end_chs);
        S = chs_get_sector (end_chs);
        A = dos_data->orig->geom.end;
-       A_ = A - S;
+       A_ = A - (S - 1);
 
        if (h < 0 || H < 0 || h > 254 || H > 254)
                return 0;
 
        /* Not enough information.  In theory, we can do better.  Should we? */
-       if (C > 1022 || a_ * C - A_ * c == 0 || A_ * h - a_ * H == 0)
+       if (C >= 1023 || a_ * C - A_ * c == 0 || A_ * h - a_ * H == 0)
                return 0;
 
        heads = (A_ * h - a_ * H) / (a_ * C - A_ * c);
        PED_ASSERT (heads > 0, return 0);
        PED_ASSERT (heads < 256, return 0);
 
@@ -697,13 +703,13 @@
 
                if (is_extended_table)
                        type = PED_PARTITION_LOGICAL;
                else if (raw_part_is_extended (raw_part))
                        type = PED_PARTITION_EXTENDED;
                else
-                       type = 0;
+                       type = PED_PARTITION_NORMAL;
 
                part = raw_part_parse (disk, raw_part, lba_offset, type);
                if (!part)
                        goto error;
                if (!is_extended_table)
                        part->num = i + 1;
@@ -1747,19 +1753,19 @@
        PED_ASSERT (disk != NULL, return 0);
        PED_ASSERT (disk->dev != NULL, return 0);
 
        dev = disk->dev;
        cyl_size = dev->bios_geom.sectors * dev->bios_geom.heads;
 
-       if (!add_metadata_part (disk, 0, 0, dev->bios_geom.sectors - 1))
+       if (!add_metadata_part (disk, PED_PARTITION_NORMAL, 0,
dev->bios_geom.sectors - 1))
                return 0;
 
        if (ped_round_down_to (dev->length, cyl_size) != dev->length) {
                if (!add_metadata_part (
                                disk,
-                               0,
+                               PED_PARTITION_NORMAL,
                                ped_round_down_to (dev->length, cyl_size),
                                dev->length - 1))
                        return 0;
        }
 
        ext_part = ped_disk_extended_partition (disk);




The disk I am using has the following partition table:

########################################################################
#                                                                      #
#       Partition table printout for /dev/hda (CHS=19386/16/63)        #
#         generated 2004-09-04 01:14 by printpar version 1.1.3         #
#                                                                      #
########################################################################

        Partition table at Master Boot Record (CHS=0/0/1) offset 0x1BE
+------+-----+--------------+-----+--------------+----------+----------+
|      |     |     Start    |     |      End     | Relative |Number of |
| Part |boot |Head Cyl Sect.|syst |Head Cyl Sect.|Start Sect| Sectors  |
+------+-----+--------------+-----+--------------+----------+----------+
| hda1 | 0x80|   1    0   1 | 0x06|  15 1019  63 |       63 |  1028097 |
| hda2 | 0x00|   0 1020   1 | 0x83|  15 1023  63 |  1028160 | 17463600 |
| hda3 | 0x00|  15 1023  63 | 0x82|  15 1023  63 | 18491760 |  1048320 |
| hda4 |    0|   0    0   0 |    0|   0    0   0 |        0 |        0 |
+------+-----+--------------+-----+--------------+----------+----------+


BR Håkon Løvdal
-- 
Linux - The Choice of a GNU Generation