[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Two bugs that cancel each other in chs_get_sector and chs_to_sector
From: |
Håkon Løvdal |
Subject: |
Two bugs that cancel each other in chs_get_sector and chs_to_sector |
Date: |
Sat, 4 Sep 2004 02:28:48 +0200 |
User-agent: |
Internet Messaging Program (IMP) 3.1 |
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
- Two bugs that cancel each other in chs_get_sector and chs_to_sector,
Håkon Løvdal <=