fdisk geom fixes

2011-07-04 Thread andres . p 
cmd.c:

x86* maxhead should be 255, not 256

make geom less useless by using disklabel for cyl, if available

part.c:

x86* maxhead should be 254, not 255 (other arches?)

maxcyl gets set to 1023 before this function gets called, so it's a
redundant check

Index: src/sbin/fdisk/cmd.c
===
RCS file: /cvs/src/sbin/fdisk/cmd.c,v
retrieving revision 1.45
diff -p -u -r1.45 cmd.c
--- src/sbin/fdisk/cmd.c2 Jul 2010 02:54:09 -   1.45
+++ src/sbin/fdisk/cmd.c4 Jul 2011 20:55:22 -
@@ -67,9 +67,10 @@ Xreinit(cmd_t *cmd, disk_t *disk, mbr_t 
 int
 Xdisk(cmd_t *cmd, disk_t *disk, mbr_t *mbr, mbr_t *tt, int offset)
 {
-   int maxcyl  = 1024;
-   int maxhead = 256;
-   int maxsec  = 63;
+   u_int32_t maxcyl  = 1024;
+   u_int32_t maxhead = 255;
+   u_int32_t maxsec  = 63;
+   u_int32_t dmaxcyl = 0;
 
/* Print out disk info */
DISK_printmetrics(disk, cmd-args);
@@ -80,8 +81,15 @@ Xdisk(cmd_t *cmd, disk_t *disk, mbr_t *m
maxsec  = 999;
 #endif
 
+   if (disk-label)
+   dmaxcyl = disk-label-cylinders;
+
/* Ask for new info */
if (ask_yn(Change disk geometry?)) {
+   if (dmaxcyl  maxcyl) {
+   printf(Warning setting cyl beyond %u forces LBA\n, 
maxcyl);
+   maxcyl = dmaxcyl;
+   }
disk-real-cylinders = ask_num(BIOS Cylinders, ASK_DEC,
disk-real-cylinders, 1, maxcyl, NULL);
disk-real-heads = ask_num(BIOS Heads, ASK_DEC,
Index: src/sbin/fdisk/part.c
===
RCS file: /cvs/src/sbin/fdisk/part.c,v
retrieving revision 1.50
diff -p -u -r1.50 part.c
--- src/sbin/fdisk/part.c   29 Apr 2009 22:58:24 -  1.50
+++ src/sbin/fdisk/part.c   4 Jul 2011 20:38:24 -
@@ -212,12 +212,10 @@ PRT_parse(disk_t *disk, void *prt, off_t
 int
 PRT_check_chs(prt_t *partn)
 {
-   if ( (partn-shead  255) ||
+   if ( (partn-shead  254) ||
(partn-ssect 63) ||
-   (partn-scyl  1023) ||
-   (partn-ehead 255) ||
-   (partn-esect 63) ||
-   (partn-ecyl  1023) )
+   (partn-ehead 254) ||
+   (partn-esect 63) )
{
return 0;
}

Re: fdisk geom fixes

2011-07-04 Thread Kenneth R Westerback 
On Mon, Jul 04, 2011 at 02:26:42PM -0700, andre...@zoho.com wrote:
 cmd.c:
 
 x86* maxhead should be 255, not 256
 
 make geom less useless by using disklabel for cyl, if available

geom is useless, period. Making it easier will only encourage people
to use it.

 
 part.c:
 
 x86* maxhead should be 254, not 255 (other arches?)

But 6 lines ago you say maxhead should be 255 not 256? Perhaps you can
provide the valid ranges for all of these and your references for
the claim. An example on how you manage to cause yourself a problem
would also be helpful.

 
 maxcyl gets set to 1023 before this function gets called, so it's a
 redundant check
 
 Index: src/sbin/fdisk/cmd.c
 ===
 RCS file: /cvs/src/sbin/fdisk/cmd.c,v
 retrieving revision 1.45
 diff -p -u -r1.45 cmd.c
 --- src/sbin/fdisk/cmd.c  2 Jul 2010 02:54:09 -   1.45
 +++ src/sbin/fdisk/cmd.c  4 Jul 2011 20:55:22 -
 @@ -67,9 +67,10 @@ Xreinit(cmd_t *cmd, disk_t *disk, mbr_t 
  int
  Xdisk(cmd_t *cmd, disk_t *disk, mbr_t *mbr, mbr_t *tt, int offset)
  {
 - int maxcyl  = 1024;
 - int maxhead = 256;
 - int maxsec  = 63;
 + u_int32_t maxcyl  = 1024;
 + u_int32_t maxhead = 255;
 + u_int32_t maxsec  = 63;
 + u_int32_t dmaxcyl = 0;

What's the point of u_int32_t?

  
   /* Print out disk info */
   DISK_printmetrics(disk, cmd-args);
 @@ -80,8 +81,15 @@ Xdisk(cmd_t *cmd, disk_t *disk, mbr_t *m
   maxsec  = 999;
  #endif
  
 + if (disk-label)
 + dmaxcyl = disk-label-cylinders;
 +
   /* Ask for new info */
   if (ask_yn(Change disk geometry?)) {
 + if (dmaxcyl  maxcyl) {
 + printf(Warning setting cyl beyond %u forces LBA\n, 
 maxcyl);
 + maxcyl = dmaxcyl;
 + }


Since ask_num won't permit numbers  max, how will LBA be forced?

   disk-real-cylinders = ask_num(BIOS Cylinders, ASK_DEC,
   disk-real-cylinders, 1, maxcyl, NULL);
   disk-real-heads = ask_num(BIOS Heads, ASK_DEC,
 Index: src/sbin/fdisk/part.c
 ===
 RCS file: /cvs/src/sbin/fdisk/part.c,v
 retrieving revision 1.50
 diff -p -u -r1.50 part.c
 --- src/sbin/fdisk/part.c 29 Apr 2009 22:58:24 -  1.50
 +++ src/sbin/fdisk/part.c 4 Jul 2011 20:38:24 -
 @@ -212,12 +212,10 @@ PRT_parse(disk_t *disk, void *prt, off_t
  int
  PRT_check_chs(prt_t *partn)
  {
 - if ( (partn-shead  255) ||
 + if ( (partn-shead  254) ||
   (partn-ssect 63) ||
 - (partn-scyl  1023) ||
 - (partn-ehead 255) ||
 - (partn-esect 63) ||
 - (partn-ecyl  1023) )
 + (partn-ehead 254) ||
 + (partn-esect 63) )
   {
   return 0;
   }
 

 Ken

Re: fdisk geom fixes

2011-07-04 Thread Andres Perera 
On Mon, Jul 4, 2011 at 11:32 PM, Kenneth R Westerback
kwesterb...@rogers.com wrote:
 On Mon, Jul 04, 2011 at 09:38:37PM -0430, Andres Perera wrote:
 On Mon, Jul 4, 2011 at 8:56 PM, Kenneth R Westerback
 kwesterb...@rogers.com wrote:
  The use of CHS should be hard and attempted only by
  those who know all the rules already.

 which is made difficult by not being able to select a subset of cyls,
 since fdisk caps them at 1024 on amd64

 note that geom was, and continues to be, only relevant for the
 duration of that fdisk instance

 all the changes are just to help with partitioning, e.g. by using a
 percentage based on the subset of cyls that i could select, or just
 globbing the entire cyl range

 at the moment it is not for people that know the intricacies, as
 you're implying. it doesn't allow for anything useful

 if geom is removed from fdisk, then the obsd install ideally has to
 account for geom changes before making mbr partitions

 MBR partitions are created based solely on lba info, with the chs
 info just faked in. Removing geom would have no impact on the install
 as far as I know. At least for 99.9% of situations. That would be
 an interesting experiment.

  Ken

even though it's ultimately the same for people that don't change
them, mbr partitions are not created from lba in fdisk.

the data goes through this mangling:

1. lba (disklabel, which means that it's not necessarily lba)
2. boundless CHS
3. boundless CHS is influenced by geom (disk command)
4. 255/63 check (cylinders are ignored for the check and always written as 1024)
5. partitions are created based on boundless CHS, partition end is
usually aligned with last cylinder
6. dishonest bounded CHS gets written to mbr

from this point, it either writes 255 in all CHS fields except the 2
bit S field, or an almost honest representation of lba

since you can change the boundless CHS through geom, then CHS values
do influence, just not the ones in the MBR... it's important to make
that distinction in order to get anywhere with this mess

besides that,

for people that want to partition mbr after *really* changing CHS
values: during install they have to access the shell and change CHS
with disklabel before fdisk. that's fine since it is an unusual need:

disklabel (fill in boundstart and end, or don't partition yet) -
fdisk - disklabel

but two things need to be clarified: the order is assbackwards.
disklabel influences fdisk, and fdisk influences disklabel (to set
boundstart and boundend). this complexity is a problem no matter what
happens to geom with fdisk

the second thing is that the disk command is misleading since it
does not expose the relationship between disklabel and fdisk, in fact,
it actively confuses how the two relate. improving the disk command,
like i suggested + writing to disklabel, would lead to less circling
dependencies. but the better solution requires more drastic changes
than the ones i proposed: disklabel should not handle geom +
partitions, it's a mess. whatever handles geom comes before fdisk and
disklabel