NetBSD Problem Report #48304

From www@NetBSD.org  Sat Oct 12 15:25:22 2013
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id A1C2B7259B
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 12 Oct 2013 15:25:22 +0000 (UTC)
Message-Id: <20131012152521.2765172648@mollari.NetBSD.org>
Date: Sat, 12 Oct 2013 15:25:21 +0000 (UTC)
From: sinic@sinic.name
Reply-To: sinic@sinic.name
To: gnats-bugs@NetBSD.org
Subject: 1st partition's offset is always 1MB
X-Send-Pr-Version: www-1.0

>Number:         48304
>Category:       install
>Synopsis:       1st partition's offset is always 1MB
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    tsutsui
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Oct 12 15:30:00 +0000 2013
>Closed-Date:    Sat Nov 02 17:42:30 +0000 2013
>Last-Modified:  Sat Nov 02 17:42:30 +0000 2013
>Originator:     Simon Nicolussi
>Release:        6.1.2
>Organization:
>Environment:
NetBSD rocknroll.sinic.name 6.1.2 NetBSD 6.1.2 (GENERIC) amd64
>Description:
sysinst is supposed to behave just like fdisk when it comes to partition alignment, that is: "If the first partition isn't defined then the alignment and offset for disks larger than 128GB is set to 2048 (1MB). In all other cases the alignment default [sic] to a cylinder and the offset to a track (both using the BIOS geometry)."

The expected behaviour is also confirmed by a source code comment in sysinst's mbr.c: "/* Use 1MB offset for large (>128GB) disks */"

Unfortunately the if clause below that comment is missing braces around the statements that set the alignment and offset, so the latter is set unconditionally.
>How-To-Repeat:
Start the installation process on a machine with a disk smaller than 128GB. The disk's MBR should be zeroed out, as not to give sysinst any data to reuse.

Go with the defaults. When presented with the final BSD disklabel partitioning overview, observe that partition a starts at sector 2048 instead of at sector 63.
>Fix:
Index: mbr.c
===================================================================
RCS file: /cvsroot/src/distrib/utils/sysinst/mbr.c,v
retrieving revision 1.89.2.2
diff -u -r1.89.2.2 mbr.c
--- mbr.c       12 Jun 2012 19:19:20 -0000      1.89.2.2
+++ mbr.c       10 Oct 2013 20:12:56 -0000
@@ -1888,10 +1888,7 @@
                                ptn_0_offset = ptn_0_base;
                        }
                }
-       } else {
+       } else if (dlsize > 2048 * 1024 * 128)
                /* Use 1MB offset for large (>128GB) disks */
-               if (dlsize > 2048 * 1024 * 128)
-                       ptn_alignment = 2048;
-                       ptn_0_offset = 2048;
-       }
+               ptn_alignment = ptn_0_offset = 2048;
 }

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: install-manager->tsutsui
Responsible-Changed-By: tsutsui@NetBSD.org
Responsible-Changed-When: Sat, 12 Oct 2013 16:20:11 +0000
Responsible-Changed-Why:
My bad.


State-Changed-From-To: open->analyzed
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Sat, 12 Oct 2013 16:20:11 +0000
State-Changed-Why:
Submitter's analysis (missing braces) is right.
(though I'd like to sync that function with fdisk(8))


From: David Laight <david@l8s.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: install/48304: 1st partition's offset is always 1MB
Date: Sun, 13 Oct 2013 16:22:54 +0100

 On Sat, Oct 12, 2013 at 03:30:00PM +0000, sinic@sinic.name wrote:
 > >Number:         48304
 > >Category:       install
 > >Synopsis:       1st partition's offset is always 1MB
 ...
 > Index: mbr.c
 >                 }
 > -       } else {
 > +       } else if (dlsize > 2048 * 1024 * 128)
 >                 /* Use 1MB offset for large (>128GB) disks */
 > -               if (dlsize > 2048 * 1024 * 128)
 > -                       ptn_alignment = 2048;
 > -                       ptn_0_offset = 2048;
 > -       }
 > +               ptn_alignment = ptn_0_offset = 2048;
 >  }

 Expect the correct fix is to add the extra { and } while leaving
 everything else alone.

 	David

 -- 
 David Laight: david@l8s.co.uk

From: "Izumi Tsutsui" <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48304 CVS commit: src/distrib/utils/sysinst
Date: Sun, 13 Oct 2013 15:32:14 +0000

 Module Name:	src
 Committed By:	tsutsui
 Date:		Sun Oct 13 15:32:14 UTC 2013

 Modified Files:
 	src/distrib/utils/sysinst: mbr.c

 Log Message:
 Fix another botch of my dumb patch in PR/45990; add missing braces.

 The offset of MBR partition 0 was unintentionally set to 2048 even on
 small (<=128GB) disks.  Probably we should rethink the threshold,
 but anyway sysinst(8) should follow fdisk(8) default.
 http://nxr.NetBSD.org/xref/src/sbin/fdisk/fdisk.c?r=1.145#1199
 http://cvsweb.NetBSD.org/bsdweb.cgi/src/sbin/fdisk/fdisk.c#rev1.129

 The problem is pointed out and analyzed by Simon Nicolussi in PR/48304.

 Should be pulled up to all netbsd-6* branches.


 To generate a diff of this commit:
 cvs rdiff -u -r1.91 -r1.92 src/distrib/utils/sysinst/mbr.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: analyzed->pending-pullups
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Sat, 19 Oct 2013 09:19:37 +0000
State-Changed-Why:
pullup-6 #972


From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48304 CVS commit: [netbsd-6] src/distrib/utils/sysinst
Date: Sun, 20 Oct 2013 13:43:17 +0000

 Module Name:	src
 Committed By:	bouyer
 Date:		Sun Oct 20 13:43:17 UTC 2013

 Modified Files:
 	src/distrib/utils/sysinst [netbsd-6]: mbr.c

 Log Message:
 Pull up following revision(s) (requested by tsutsui in ticket #972):
 	distrib/utils/sysinst/mbr.c: revision 1.92
 Fix another botch of my dumb patch in PR/45990; add missing braces.
 The offset of MBR partition 0 was unintentionally set to 2048 even on
 small (<=128GB) disks.  Probably we should rethink the threshold,
 but anyway sysinst(8) should follow fdisk(8) default.
 http://nxr.NetBSD.org/xref/src/sbin/fdisk/fdisk.c?r=1.145#1199
 http://cvsweb.NetBSD.org/bsdweb.cgi/src/sbin/fdisk/fdisk.c#rev1.129
 The problem is pointed out and analyzed by Simon Nicolussi in PR/48304.
 Should be pulled up to all netbsd-6* branches.


 To generate a diff of this commit:
 cvs rdiff -u -r1.89.2.2 -r1.89.2.3 src/distrib/utils/sysinst/mbr.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48304 CVS commit: [netbsd-6-1] src/distrib/utils/sysinst
Date: Sun, 20 Oct 2013 13:43:23 +0000

 Module Name:	src
 Committed By:	bouyer
 Date:		Sun Oct 20 13:43:23 UTC 2013

 Modified Files:
 	src/distrib/utils/sysinst [netbsd-6-1]: mbr.c

 Log Message:
 Pull up following revision(s) (requested by tsutsui in ticket #972):
 	distrib/utils/sysinst/mbr.c: revision 1.92
 Fix another botch of my dumb patch in PR/45990; add missing braces.
 The offset of MBR partition 0 was unintentionally set to 2048 even on
 small (<=128GB) disks.  Probably we should rethink the threshold,
 but anyway sysinst(8) should follow fdisk(8) default.
 http://nxr.NetBSD.org/xref/src/sbin/fdisk/fdisk.c?r=1.145#1199
 http://cvsweb.NetBSD.org/bsdweb.cgi/src/sbin/fdisk/fdisk.c#rev1.129
 The problem is pointed out and analyzed by Simon Nicolussi in PR/48304.
 Should be pulled up to all netbsd-6* branches.


 To generate a diff of this commit:
 cvs rdiff -u -r1.89.2.2 -r1.89.2.2.6.1 src/distrib/utils/sysinst/mbr.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48304 CVS commit: [netbsd-6-0] src/distrib/utils/sysinst
Date: Sun, 20 Oct 2013 13:43:26 +0000

 Module Name:	src
 Committed By:	bouyer
 Date:		Sun Oct 20 13:43:26 UTC 2013

 Modified Files:
 	src/distrib/utils/sysinst [netbsd-6-0]: mbr.c

 Log Message:
 Pull up following revision(s) (requested by tsutsui in ticket #972):
 	distrib/utils/sysinst/mbr.c: revision 1.92
 Fix another botch of my dumb patch in PR/45990; add missing braces.
 The offset of MBR partition 0 was unintentionally set to 2048 even on
 small (<=128GB) disks.  Probably we should rethink the threshold,
 but anyway sysinst(8) should follow fdisk(8) default.
 http://nxr.NetBSD.org/xref/src/sbin/fdisk/fdisk.c?r=1.145#1199
 http://cvsweb.NetBSD.org/bsdweb.cgi/src/sbin/fdisk/fdisk.c#rev1.129
 The problem is pointed out and analyzed by Simon Nicolussi in PR/48304.
 Should be pulled up to all netbsd-6* branches.


 To generate a diff of this commit:
 cvs rdiff -u -r1.89.2.2 -r1.89.2.2.4.1 src/distrib/utils/sysinst/mbr.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sat, 02 Nov 2013 17:42:30 +0000
State-Changed-Why:
Pulled up.


>Unformatted:

Home	PR Database Search