Wolfgang Solfrank <ws@tools.de> writes:

> Hi,

> I'm not doing any second guessing.  After my change, our fdisk is more or
> less similar to what DOS fdisk does.  That is:

> 2. If it doesn't find a valid mbr (i.e. the magic number in the last two
> bytes of sector 0 are not 55 aa, the mbr is initialized with valid boot
> code _and_ a zeroed out partition table.  This is almost what was done
> 
> 3. fdisk -i initializes the code part of the mbr, but leaves the partition
> table alone.

As I said, making fdisk behave like DOS fdisk is completely reasonable.
1., 2. & 3. are fine.

> Hmm, I really think that fdisk -i shouldn't be done by sysinst.  The way
> this is done now, seems perfectly valid to me.  And, since it's similar to
> the way DOS does this for years, it probably conforms to the principle of
> least surprise for people familiar with PCs.

This is what needs to be discussed.  I believe the sysinst goal was to
do a complete install.  You are proposing that the mbr initialization
should not be done by sysinst.  On the plus side, this is safe.  You
are much less likely to blow away other OS's, if the sysinst tool just
installs netbsd into a preestablished area.  On the minus side, the
boot diskette can not immediately start up sysinst.  You need to go to
a shell so the user can do a fdisk if necessary.  Then he would run
sysinst by hand.

> I really think that the current code is correct.  IMHO it would be bad to
> have two places with heuristics determining the presence/absence of a
> valid boot sector.

I believe that with the current scheme, only one place decides if there
is valid boot sector, fdisk(8).  The issue is just whether sysinst
should take action based on it.  The existing sysinst code reads:
        /* Check fdisk information */
        if (part[0][ID] == 0 && part[1][ID] == 0 && part[2][ID] == 0 &&
            part[3][ID] == 0) {
                mbr_present = 0;
                process_menu(MENU_nobiosgeom);
        } else {
                mbr_present = 1;
                msg_display(MSG_confirmbiosgeom);
                process_menu(MENU_confirmbiosgeom);
        }
It should be:
        /* Check fdisk information */
        if (part[0][ID] == 0 && part[1][ID] == 0 && part[2][ID] == 0 &&
            part[3][ID] == 0) {
                process_menu(MENU_nobiosgeom);
        }
        msg_display(MSG_confirmbiosgeom);
        process_menu(MENU_confirmbiosgeom);
And "new" the nobiosgeom menu should explains that there is no mbr
info and asks to abort to do an fdisk -i.  Then we proceeds to the geometry query.
I think this will make everything clear and everyone happy.

> 
> As an aside, with the previous implementation of fdisk, the flag mbr_present
> was never being set.  The fdisk code, when determining that the magic number
  ...
> this incore table.  So the code that determines mbr_present = 0 will never
> trigger, as it checks for all four partition types being zero.
Sigh, I should look harder at function vs intent.  Though it is clear
now with your change (2) above that the test would indeed return mbr_present = 0.
Again, the big bug -- which i DID confirm -- was that whenever you reset
the bios geometry (even if you accidentally entered the menu and did
not change anything), it set bstuffset and then eventually run:
         if (bstuffset)
                run_prog ("/sbin/fdisk -i -f -b %d/%d/%d /dev/r%sd",
                          bcyl, bhead, bsec, diskdev);
Clearly, the old code was broken and in a number of other ways, too.  Let's
just fix it.