NetBSD Problem Report #42555

From tsutsui@ceres.dti.ne.jp  Thu Dec 31 17:13:41 2009
Return-Path: <tsutsui@ceres.dti.ne.jp>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 92C1563B844
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 31 Dec 2009 17:13:41 +0000 (UTC)
Message-Id: <200912311713.nBVHDcgl004162@mirage.ceres.dti.ne.jp>
Date: Fri, 1 Jan 2010 02:13:38 +0900 (JST)
From: tsutsui@ceres.dti.ne.jp
Reply-To: tsutsui@ceres.dti.ne.jp
To: gnats-bugs@gnats.NetBSD.org
Cc: tsutsui@ceres.dti.ne.jp
Subject: installboot(8) should not refer DEV_BSIZE in src/tools
X-Send-Pr-Version: 3.95

>Number:         42555
>Category:       toolchain
>Synopsis:       installboot(8) should not refer DEV_BSIZE in src/tools
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    tsutsui
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Dec 31 17:15:00 +0000 2009
>Closed-Date:    Thu Jan 07 13:30:19 +0000 2010
>Last-Modified:  Thu Jan 07 13:35:01 +0000 2010
>Originator:     Izumi Tsutsui
>Release:        NetBSD 5.99.22
>Organization:
>Environment:
Build System: CYGWIN_NT-5.1-1.7.1 on Windows XP SP3
Architecture: i686
Machine: i386 (32bit)

Target System: NetBSD 5.99.22 updated around 20091227
Architecture: sparc 
Machine: sparc

>Description:
Some sources of installboot(8) refer DEV_BSIZE to get
hardware sector size to calculate sector numbers etc,
but DEV_BSIZE is not always a value for a target NetBSD system
but a value for host's environment.

Actually DEV_BSIZE is 1024 on Cygwin-1.7.1 so cross installboot(8)
for sparc on Cygwin fails to search a secondary boot file in
the target file system.

>How-To-Repeat:
Code inspection.
Or "build.sh -U -m sparc release" on Cygwin-1.7.1, then you will get:

---
/usr/src/obj.sparc/tooldir.CYGWIN_NT-5.1-1.7.1-i686/bin/nbinstallboot -v \
-m sparc miniroot.fs  /usr/src/obj.sparc/destdir.sparc/usr/mdec/bootxx /boot \
|| { ls -f miniroot.fs ; false; }
File system:         miniroot.fs
File system type:    ffs (blocksize 4096, needswap 1)
Primary bootstrap:   /usr/src/obj.sparc/destdir.sparc/usr/mdec/bootxx
Secondary bootstrap: /boot
nbinstallboot: Could not find secondary bootstrap `boot' in `miniroot.fs'
nbinstallboot: (Path must be relative to the file system in `miniroot.fs')
nbinstallboot: Set bootstrap operation failed
miniroot.fs

*** Failed target:  miniroot.fs
*** Failed command: \
/usr/src/obj.sparc/tooldir.CYGWIN_NT-5.1-1.7.1-i686/bin/nbinstallboot -v \
-m sparc miniroot.fs /usr/src/obj.sparc/destdir.sparc/usr/mdec/bootxx /boot \
|| { ls -f miniroot.fs ; false; }
*** Error code 1

Stop.
nbmake: stopped in /usr/src/distrib/miniroot
%

---

>Fix:
To specify sector size on installboot operation,
probably we have to add an option to pass sectorsize like newfs(8).
For now, it would be enough to add a sectorsize member into
struct ib_params, set it 512bytes by default and use it instead
of DEV_BSIZE value.


Index: ext2fs.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/installboot/ext2fs.c,v
retrieving revision 1.4
diff -u -r1.4 ext2fs.c
--- ext2fs.c	19 Oct 2009 18:41:17 -0000	1.4
+++ ext2fs.c	31 Dec 2009 16:31:49 -0000
@@ -108,7 +108,7 @@
 	assert(size > 0);
 	assert(blk != NULL);

-	rv = pread(params->fsfd, blk, size, blkno * DEV_BSIZE);
+	rv = pread(params->fsfd, blk, size, blkno * params->sectorsize);
 	if (rv == -1) {
 		warn("Reading block %llu in `%s'", 
 		    (unsigned long long)blkno, params->filesystem);
@@ -127,7 +127,7 @@
 {
 	uint8_t sbbuf[SBSIZE];

-	if (ext2fs_read_disk_block(params, SBOFF / DEV_BSIZE, SBSIZE,
+	if (ext2fs_read_disk_block(params, SBOFF / params->sectorsize, SBSIZE,
 	    sbbuf) == 0)

 	e2fs_sbload((void *)sbbuf, &fs->e2fs);
Index: ffs.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/installboot/ffs.c,v
retrieving revision 1.27
diff -u -r1.27 ffs.c
--- ffs.c	5 Apr 2009 12:03:48 -0000	1.27
+++ ffs.c	31 Dec 2009 16:31:49 -0000
@@ -97,7 +97,7 @@
 	assert(size > 0);
 	assert(blk != NULL);

-	rv = pread(params->fsfd, blk, size, blkno * DEV_BSIZE);
+	rv = pread(params->fsfd, blk, size, blkno * params->sectorsize);
 	if (rv == -1) {
 		warn("Reading block %llu in `%s'", 
 		    (unsigned long long)blkno, params->filesystem);
@@ -475,10 +475,10 @@
 raid_match(ib_params *params)
 {
 	/* XXX Assumes 512 bytes / sector */
-	if (DEV_BSIZE != 512) {
+	if (params->sectorsize != 512) {
 		warnx("Media is %d bytes/sector."
 			"  RAID is only supported on 512 bytes/sector media.",
-			DEV_BSIZE);
+			params->sectorsize);
 		return 0;
 	}
 	return ffs_match_common(params, (off_t) RF_PROTECTED_SECTORS);
@@ -497,7 +497,7 @@

 	fs = (struct fs *)sbbuf;
 	for (i = 0; sblock_try[i] != -1; i++) {
-		loc = sblock_try[i] / DEV_BSIZE + offset;
+		loc = sblock_try[i] / params->sectorsize + offset;
 		if (!ffs_read_disk_block(params, loc, SBLOCKSIZE, sbbuf))
 			continue;
 		switch (fs->fs_magic) {
Index: fstypes.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/installboot/fstypes.c,v
retrieving revision 1.11
diff -u -r1.11 fstypes.c
--- fstypes.c	28 Apr 2008 20:24:16 -0000	1.11
+++ fstypes.c	31 Dec 2009 16:31:49 -0000
@@ -95,7 +95,7 @@

 	for (i = 0; i < nblk; i++) {
 		blocks[i].block = params->s2start +
-		    i * (params->fstype->blocksize / 512);
+		    i * (params->fstype->blocksize / params->sectorsize);
 		blocks[i].blocksize = params->fstype->blocksize;
 	}
 	*maxblk = nblk;
Index: installboot.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/installboot/installboot.c,v
retrieving revision 1.31
diff -u -r1.31 installboot.c
--- installboot.c	5 Apr 2009 11:55:39 -0000	1.31
+++ installboot.c	31 Dec 2009 16:31:50 -0000
@@ -92,6 +92,8 @@
 #undef OFFSET
 #define OPTION(params, type, opt) (*(type *)((char *)(params) + (opt)->offset))

+#define DFL_SECSIZE	512	/* Don't use DEV_BSIZE. It's host's value. */
+
 int
 main(int argc, char *argv[])
 {
@@ -234,6 +236,8 @@
 		op = "write";
 		mode = O_RDWR;
 	}
+	/* XXX should be specified via option */
+	params->sectorsize = DFL_SECSIZE;
 	if ((params->fsfd = open(params->filesystem, mode, 0600)) == -1)
 		err(1, "Opening file system `%s' read-%s",
 		    params->filesystem, op);
Index: installboot.h
===================================================================
RCS file: /cvsroot/src/usr.sbin/installboot/installboot.h,v
retrieving revision 1.35
diff -u -r1.35 installboot.h
--- installboot.h	28 Apr 2008 20:24:16 -0000	1.35
+++ installboot.h	31 Dec 2009 16:31:50 -0000
@@ -80,6 +80,7 @@
 	uint64_t	 s1start;	/*  start block of stage1 */
 	const char	*stage2;	/* name of stage2 bootstrap */
 	uint64_t	 s2start;	/*  start block of stage2 */
+	uint32_t	 sectorsize;	/* sector size of target fs */
 		/* parsed -o option=value data */
 	const char	*command;	/* name of command string */
 	const char	*console;	/* name of console */
Index: arch/hp300.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/installboot/arch/hp300.c,v
retrieving revision 1.11
diff -u -r1.11 hp300.c
--- arch/hp300.c	5 Apr 2009 11:55:39 -0000	1.11
+++ arch/hp300.c	31 Dec 2009 16:31:50 -0000
@@ -78,8 +78,7 @@
 	int		i;
 	unsigned int	secsize = HP300_SECTSIZE;
 	uint64_t	boot_size, boot_offset;
-	char		label_buf[DEV_BSIZE];
-	struct disklabel *label = (void *)label_buf;
+	struct disklabel *label;

 	assert(params != NULL);
 	assert(params->fsfd != -1);
@@ -90,6 +89,12 @@
 	retval = 0;
 	bootstrap = MAP_FAILED;

+	label = malloc(params->sectorsize);
+	if (label == NULL) {
+		warn("Failed to allocate memory for disklabel");
+		goto done;
+	}
+
 	if (params->flags & IB_APPEND) {
 		if (!S_ISREG(params->fsstat.st_mode)) {
 			warnx(
@@ -103,8 +108,8 @@
 		 * The bootstrap can be well over 8k, and must go into a BOOT
 		 * partition. Read NetBSD label to locate BOOT partition.
 		 */
-		if (pread(params->fsfd, label, DEV_BSIZE, 2 * DEV_BSIZE)
-								!= DEV_BSIZE) {
+		if (pread(params->fsfd, label, params->sectorsize, LABELSECTOR)
+		    != (ssize_t)params->sectorsize) {
 			warn("reading disklabel");
 			goto done;
 		}
@@ -200,6 +205,8 @@
 	retval = 1;

  done:
+	if (label != NULL)
+		free(label);
 	if (bootstrap != MAP_FAILED)
 		munmap(bootstrap, params->s1stat.st_size);
 	return retval;
Index: arch/next68k.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/installboot/arch/next68k.c,v
retrieving revision 1.6
diff -u -r1.6 next68k.c
--- arch/next68k.c	5 Apr 2009 11:55:39 -0000	1.6
+++ arch/next68k.c	31 Dec 2009 16:31:50 -0000
@@ -51,8 +51,6 @@

 #include "installboot.h"

-#define	SECTOR_SIZE	DEV_BSIZE
-
 static uint16_t nextstep_checksum(const void *, const void *);
 static int next68k_setboot(ib_params *);

@@ -106,7 +104,7 @@
 	 * Read in the next68k disklabel
 	 */
 	rv = pread(params->fsfd, next68klabel, NEXT68K_LABEL_SIZE,
-	    NEXT68K_LABEL_SECTOR * SECTOR_SIZE + NEXT68K_LABEL_OFFSET);
+	    NEXT68K_LABEL_SECTOR * params->sectorsize + NEXT68K_LABEL_OFFSET);
 	if (rv == -1) {
 		warn("Reading `%s'", params->filesystem);
 		goto done;
@@ -128,7 +126,7 @@
 	}

 	cd_secsize = be32toh(next68klabel->cd_secsize);
-	sec_netonb_mult = (cd_secsize / SECTOR_SIZE);
+	sec_netonb_mult = (cd_secsize / params->sectorsize);

 	/*
 	 * Allocate a buffer, with space to round up the input file
@@ -177,7 +175,8 @@
 			b0 = b1 = NEXT68K_LABEL_SIZE / cd_secsize;
 		else {
 			if (2 * bootsize > (fp * cd_secsize - 
-				NEXT68K_LABEL_DEFAULTBOOT0_1 * SECTOR_SIZE))
+				NEXT68K_LABEL_DEFAULTBOOT0_1 *
+				params->sectorsize))
 				/* can fit two copies starting after label */
 				b0 = NEXT68K_LABEL_SIZE / cd_secsize;
 			else
@@ -220,7 +219,8 @@
 		*checksum = htobe16(nextstep_checksum (next68klabel,
 					checksum));
 		rv = pwrite(params->fsfd, next68klabel, NEXT68K_LABEL_SIZE,
-		    NEXT68K_LABEL_SECTOR * SECTOR_SIZE + NEXT68K_LABEL_OFFSET);
+		    NEXT68K_LABEL_SECTOR * params->sectorsize +
+		    NEXT68K_LABEL_OFFSET);
 		if (rv == -1) {
 			warn("Writing `%s'", params->filesystem);
 			goto done;
@@ -240,7 +240,8 @@
 	for (;;) {
 		if (params->flags & IB_VERBOSE)
 			printf ("Writing boot program at %d\n", b0);
-		rv = pwrite(params->fsfd, bootbuf, bootsize, b0 * SECTOR_SIZE);
+		rv = pwrite(params->fsfd, bootbuf, bootsize,
+		    b0 * params->sectorsize);
 		if (rv == -1) {
 			warn("Writing `%s' at %d", params->filesystem, b0);
 			goto done;

>Release-Note:

>Audit-Trail:
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: toolchain/42555: installboot(8) should not refer DEV_BSIZE in
	src/tools
Date: Thu, 31 Dec 2009 17:19:50 +0000

 On Thu, Dec 31, 2009 at 05:15:01PM +0000, tsutsui@ceres.dti.ne.jp wrote:
  > Some sources of installboot(8) refer DEV_BSIZE to get
  > hardware sector size to calculate sector numbers etc,
  > but DEV_BSIZE is not always a value for a target NetBSD system
  > but a value for host's environment.
  > [...]
  >
  > Index: ext2fs.c
  > ===================================================================
  > RCS file: /cvsroot/src/usr.sbin/installboot/ext2fs.c,v
  > retrieving revision 1.4
  > diff -u -r1.4 ext2fs.c
  > [...]

 Looks reasonable to me.

 -- 
 David A. Holland
 dholland@netbsd.org

Responsible-Changed-From-To: toolchain-manager->tsutsui
Responsible-Changed-By: tsutsui@NetBSD.org
Responsible-Changed-When: Sat, 02 Jan 2010 17:58:38 +0900
Responsible-Changed-Why:
I wrote patch


State-Changed-From-To: open->analyzed
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Sat, 02 Jan 2010 17:58:38 +0900
State-Changed-Why:
Will be checked in if no further particular comments in a few days.


State-Changed-From-To: analyzed->closed
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Thu, 07 Jan 2010 22:30:19 +0900
State-Changed-Why:
Fix committed.


From: Izumi Tsutsui <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/42555 CVS commit: src/usr.sbin/installboot
Date: Thu, 7 Jan 2010 13:26:01 +0000

 Module Name:	src
 Committed By:	tsutsui
 Date:		Thu Jan  7 13:26:00 UTC 2010

 Modified Files:
 	src/usr.sbin/installboot: ext2fs.c ffs.c fstypes.c installboot.c
 	    installboot.h
 	src/usr.sbin/installboot/arch: hp300.c next68k.c

 Log Message:
 Add a sectorsize parameter member in struct ib_params and use it
 where sector size (disk block size) is required, instead of
 DEV_BSIZE constant which means device I/O block size.

 "Looks reasonable" from dholland@, and fixes tools installboot(8)
 on Cygwin where DEV_BSIZE != 512 as mentioned in PR toolchain/42555.


 To generate a diff of this commit:
 cvs rdiff -u -r1.4 -r1.5 src/usr.sbin/installboot/ext2fs.c
 cvs rdiff -u -r1.27 -r1.28 src/usr.sbin/installboot/ffs.c
 cvs rdiff -u -r1.11 -r1.12 src/usr.sbin/installboot/fstypes.c
 cvs rdiff -u -r1.31 -r1.32 src/usr.sbin/installboot/installboot.c
 cvs rdiff -u -r1.35 -r1.36 src/usr.sbin/installboot/installboot.h
 cvs rdiff -u -r1.11 -r1.12 src/usr.sbin/installboot/arch/hp300.c
 cvs rdiff -u -r1.6 -r1.7 src/usr.sbin/installboot/arch/next68k.c

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

>Unformatted:

Home	PR Database Search