Re: Two bugs that cancel each other in chs_get_sector and chs_to_sector

[Andrew Clausen]

Hi Håkon,

On Sat, Sep 04, 2004 at 02:28:48AM +0200, Håkon Løvdal wrote:
> The calculation "chs->sector % 0x40 - 1;" in chs_get_sector is wrong.
> It should not subtract one here.

I think it should.  I think it is stupid to count from 1.  I
unilaterally decided to count from 0 (just like cylinders and heads
are).

> 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...

You convinced me here.

> 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).

I think it is a stupid formula (that matches the stupid situation).

> 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

It's just a bit of algebra to compute bios_geom.  You have two
equations, and you solve for heads and sectors.

I just added these assertions at the end:

        PED_ASSERT ((c * heads + h) * sectors + s == a, return 0);
        PED_ASSERT ((C * heads + H) * sectors + S == A, return 0);

c, h, s, a, C, H, S, A are the known variables.
heads, sectors are the unknown variables.

If we have two indepedent equations, we can solve for heads and sectors...

> Also I replaced a few zeros with the corresponding enum value

Good point.

> 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).

Fair enough.

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

Assertions are for bugs in Parted.  This is an error.

Thanks for your comments.
Andrew