NetBSD Problem Report #46652

From spz@NetBSD.org  Tue Jul  3 18:26:50 2012
Return-Path: <spz@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id 0421663B882
	for <gnats-bugs@gnats.NetBSD.org>; Tue,  3 Jul 2012 18:26:49 +0000 (UTC)
Message-Id: <20120703182549.D198472@amdmin.netbsd.org>
Date: Tue,  3 Jul 2012 18:25:49 +0000 (UTC)
From: spz@amdmin.netbsd.org
Reply-To: spz@amdmin.netbsd.org
To: gnats-bugs@gnats.NetBSD.org
Subject: sparc ofwboot.net fails to load kernel via tftp
X-Send-Pr-Version: 3.95

>Number:         46652
>Category:       port-sparc64
>Synopsis:       sparc ofwboot.net fails to load kernel via tftp
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    tsutsui
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jul 03 18:30:00 +0000 2012
>Closed-Date:    Wed Jul 25 20:01:47 +0000 2012
>Last-Modified:  Wed Jul 25 20:01:47 +0000 2012
>Originator:     S.P.Zeidler
>Release:        NetBSD 6.0_BETA2
>Organization:
	dis-
>Environment:
System: NetBSD amdmin.netbsd.de 4.0_STABLE NetBSD 4.0_STABLE (GENERIC) #1: Thu Jun 14 20:00:29 UTC 2012 spz@amdmin.netbsd.de:/home/netbsd/6/amd64/kern-compile/GENERIC amd64
Architecture: x86_64
Machine: amd64
>Description:
	sparc + sparc64 ofwboot.net lost the ability to load a kernel via tftp
	The ofwboot.net crashes instead.
>How-To-Repeat:
	boot net tftp:netbsd.gz
	on a likely sparc in an environment where the kernel should be
	loaded via tftp.
>Fix:
roll back some changes:

Index: net.c
===================================================================
RCS file: /cvsroot/src/sys/arch/sparc/stand/ofwboot/net.c,v
retrieving revision 1.7
diff -u -p -r1.7 net.c
--- net.c	21 May 2011 15:50:42 -0000	1.7
+++ net.c	3 Jul 2012 18:05:55 -0000
@@ -181,13 +181,14 @@ net_mountroot_bootp(void)
 }

 int
-net_tftp_bootp(struct of_dev *op)
+net_tftp_bootp(int **sock)
 {

 	net_mountroot_bootp();
 	if (myip.s_addr == 0)
 		return(ENOENT);

+	*sock = &netdev_sock;
 	return (0);
 }

Index: net.h
===================================================================
RCS file: /cvsroot/src/sys/arch/sparc/stand/ofwboot/net.h,v
retrieving revision 1.2
diff -u -p -r1.2 net.h
--- net.h	21 May 2011 15:50:42 -0000	1.2
+++ net.h	3 Jul 2012 18:05:55 -0000
@@ -34,7 +34,7 @@

 int	net_open(struct of_dev *);
 int	net_close(struct of_dev *);
-int	net_tftp_bootp(struct of_dev *);
+int	net_tftp_bootp(int **);
 int	net_mountroot(void);

 #endif /* _OFWBOOT_NET_H */
Index: ofdev.c
===================================================================
RCS file: /cvsroot/src/sys/arch/sparc/stand/ofwboot/ofdev.c,v
retrieving revision 1.32
diff -u -p -r1.32 ofdev.c
--- ofdev.c	1 Jun 2011 11:42:18 -0000	1.32
+++ ofdev.c	3 Jul 2012 18:05:55 -0000
@@ -536,7 +536,7 @@ open_again:
 		if (!strncmp(*file,"/tftp:",6)) {
 			*file += 6;
 			memcpy(&file_system[0], &file_system_tftp, sizeof file_system[0]);
-			if (net_tftp_bootp(of->f_devdata)) {
+			if (net_tftp_bootp((int **) &of->f_devdata)) {
 				net_close(&ofdev);
 				goto bad;
 			}

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: port-sparc64-maintainer->tsutsui
Responsible-Changed-By: spz@NetBSD.org
Responsible-Changed-When: Tue, 03 Jul 2012 20:25:58 +0000
Responsible-Changed-Why:
hand to the developer whose commit broke the functionality


State-Changed-From-To: open->feedback
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Wed, 04 Jul 2012 20:44:24 +0900
State-Changed-Why:
Modified patch suggested.


From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: gnats-bugs@NetBSD.org
Cc: tsutsui@NetBSD.org, port-sparc64-maintainer@NetBSD.org,
        netbsd-bugs@NetBSD.org, spz@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: port-sparc64/46652 (sparc ofwboot.net fails to load kernel via
	 tftp)
Date: Wed, 4 Jul 2012 20:43:27 +0900

 Ok, tftp requires own initialization (net_sock in of->f_devdata),
 but the previous code (using int**) was a bit confusing.

 How about passing (struct open_file *) as
 lib/libsa/tftp.c:tftp_open() does?

 ---
 Izumi Tsutsui

 Index: net.c
 ===================================================================
 RCS file: /cvsroot/src/sys/arch/sparc/stand/ofwboot/net.c,v
 retrieving revision 1.7
 diff -u -p -r1.7 net.c
 --- net.c	21 May 2011 15:50:42 -0000	1.7
 +++ net.c	4 Jul 2012 11:39:38 -0000
 @@ -181,13 +181,14 @@ net_mountroot_bootp(void)
  }

  int
 -net_tftp_bootp(struct of_dev *op)
 +net_tftp_bootp(struct open_file *f)
  {

  	net_mountroot_bootp();
  	if (myip.s_addr == 0)
  		return(ENOENT);

 +	f->f_devdata = &netdev_sock;
  	return (0);
  }

 Index: net.h
 ===================================================================
 RCS file: /cvsroot/src/sys/arch/sparc/stand/ofwboot/net.h,v
 retrieving revision 1.2
 diff -u -p -r1.2 net.h
 --- net.h	21 May 2011 15:50:42 -0000	1.2
 +++ net.h	4 Jul 2012 11:39:38 -0000
 @@ -34,7 +34,7 @@

  int	net_open(struct of_dev *);
  int	net_close(struct of_dev *);
 -int	net_tftp_bootp(struct of_dev *);
 +int	net_tftp_bootp(struct open_file *);
  int	net_mountroot(void);

  #endif /* _OFWBOOT_NET_H */
 Index: ofdev.c
 ===================================================================
 RCS file: /cvsroot/src/sys/arch/sparc/stand/ofwboot/ofdev.c,v
 retrieving revision 1.32
 diff -u -p -r1.32 ofdev.c
 --- ofdev.c	1 Jun 2011 11:42:18 -0000	1.32
 +++ ofdev.c	4 Jul 2012 11:39:38 -0000
 @@ -536,7 +536,7 @@ open_again:
  		if (!strncmp(*file,"/tftp:",6)) {
  			*file += 6;
  			memcpy(&file_system[0], &file_system_tftp, sizeof file_system[0]);
 -			if (net_tftp_bootp(of->f_devdata)) {
 +			if (net_tftp_bootp(of)) {
  				net_close(&ofdev);
  				goto bad;
  			}

 ---
 Izumi Tsutsui

From: "S.P.Zeidler" <spz@serpens.de>
To: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Cc: gnats-bugs@NetBSD.org, port-sparc64-maintainer@NetBSD.org,
        netbsd-bugs@NetBSD.org
Subject: Re: port-sparc64/46652 (sparc ofwboot.net fails to load kernel via
 tftp)
Date: Wed, 4 Jul 2012 15:43:41 +0200

 Hi Tsutsui-san,

 Thus wrote Izumi Tsutsui (tsutsui@ceres.dti.ne.jp):

 > Ok, tftp requires own initialization (net_sock in of->f_devdata),
 > but the previous code (using int**) was a bit confusing.
 > 
 > How about passing (struct open_file *) as
 > lib/libsa/tftp.c:tftp_open() does?

 Why do you want to obscure what kind of data gets carried from
 generator to consumer in f_devdata in the tftp case?
 Using int ** is a (poor, but present) type check by the compiler,
 after all.

 If the reason why it's int ** is too confusing, a comment should be added.

 The code would be less clear to me, not more, if you handed over the
 pointer to the entire open_file struct when all you want to do is set
 one member of it (which is a dedicated "I'm void * so you can stick in
 whatever both ends need", too).

 regards,
 	spz
 -- 
 spz@serpens.de (S.P.Zeidler)

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: spz@serpens.de
Cc: gnats-bugs@NetBSD.org, port-sparc64-maintainer@NetBSD.org,
        netbsd-bugs@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: port-sparc64/46652 (sparc ofwboot.net fails to load kernel viatftp)
Date: Thu, 5 Jul 2012 01:13:17 +0900

 > > Ok, tftp requires own initialization (net_sock in of->f_devdata),
 > > but the previous code (using int**) was a bit confusing.
 > > 
 > > How about passing (struct open_file *) as
 > > lib/libsa/tftp.c:tftp_open() does?
 > 
 > Why do you want to obscure what kind of data gets carried from
 > generator to consumer in f_devdata in the tftp case?
 > Using int ** is a (poor, but present) type check by the compiler,
 > after all.

 See actual consumer of the f_devdata, sys/lib/libsa/tftp.c:tftp_open():
 ---
 __compactcall int
 tftp_open(const char *path, struct open_file *f)
 {

  :

 	tftpfile->iodesc = io = socktodesc(*(int *)(f->f_devdata));
 ---
 which indicates what should be initialized.

 On the other hand, nfs.c (which also use net_open()) doesn't require
 any data in f_devdata at all and it confused me.

 > If the reason why it's int ** is too confusing, a comment should be added.
 > 
 > The code would be less clear to me, not more, if you handed over the
 > pointer to the entire open_file struct when all you want to do is set
 > one member of it (which is a dedicated "I'm void * so you can stick in
 > whatever both ends need", too).

 What kind of comment is reasonable for you?
 Even if you don't want void *, Why not int * but int **?
 I don't see any reason to prefer int **.

 ---
 Izumi Tsutsui

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: spz@serpens.de
Cc: gnats-bugs@NetBSD.org, port-sparc64-maintainer@NetBSD.org,
        netbsd-bugs@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: port-sparc64/46652 (sparc ofwboot.net fails to load kernel viatftp)
Date: Thu, 5 Jul 2012 02:35:10 +0900

 I wrote:
 > > If the reason why it's int ** is too confusing, a comment should be added.
 > > 
 > > The code would be less clear to me, not more, if you handed over the
 > > pointer to the entire open_file struct when all you want to do is set
 > > one member of it (which is a dedicated "I'm void * so you can stick in
 > > whatever both ends need", too).
 > 
 > What kind of comment is reasonable for you?
 > Even if you don't want void *, Why not int * but int **?
 > I don't see any reason to prefer int **.

 Ah, my bad, int * won't work here. I was actually confused...

 But I still prefer passing struct open_file * as tftp_open().

 ---
 Izumi Tsutsui

From: "S.P.Zeidler" <spz@serpens.de>
To: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Cc: gnats-bugs@NetBSD.org, port-sparc64-maintainer@NetBSD.org,
        netbsd-bugs@NetBSD.org
Subject: Re: port-sparc64/46652 (sparc ofwboot.net fails to load kernel
 viatftp)
Date: Thu, 5 Jul 2012 00:52:37 +0200

 Hi Tsutsui-san,

 Thus wrote Izumi Tsutsui (tsutsui@ceres.dti.ne.jp):

 > What kind of comment is reasonable for you?

 From this discussion, it might need this:

 /*
  * libsa's tftp_open expects a pointer to netdev_sock, i.e. an (int *),
  * in f_devdata, a pointer to which gets handed down from devopen().
  *
  * Do not expect booting via different methods to have the same
  * requirements or semantics.
  *
  * net_tftp_bootp uses net_mountroot_bootp because that incidentially does
  * most of what it needs to do. It of course in no manner actually mounts
  * anything, all that routine actually does is prepare the socket for the
  * necessary net access, and print info for the user.
  */

 int
 net_tftp_bootp(struct of_dev *op)
 {
 [...]

 regards,
 	spz
 -- 
 spz@serpens.de (S.P.Zeidler)

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: spz@serpens.de
Cc: gnats-bugs@NetBSD.org, port-sparc64-maintainer@NetBSD.org,
        tsutsui@ceres.dti.ne.jp
Subject: Re: port-sparc64/46652 (sparc ofwboot.net fails to load kernelviatftp)
Date: Fri, 6 Jul 2012 21:24:52 +0900

 > net_tftp_bootp(struct of_dev *op)

 Do you still prefer (int **) for net_tftp_bootp() to return
 netev_sock pointer, or (struct open_file *) is acceptable?

 ---
 Izumi Tsutsui

From: "S.P.Zeidler" <spz@serpens.de>
To: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Cc: gnats-bugs@NetBSD.org, port-sparc64-maintainer@NetBSD.org
Subject: Re: port-sparc64/46652 (sparc ofwboot.net fails to load
 kernelviatftp)
Date: Fri, 13 Jul 2012 20:46:20 +0200

 Thus wrote Izumi Tsutsui (tsutsui@ceres.dti.ne.jp):

 > > net_tftp_bootp(struct of_dev *op)
 > 
 > Do you still prefer (int **) for net_tftp_bootp() to return
 > netev_sock pointer, or (struct open_file *) is acceptable?

 Sorry for the late answer.
 I prefer (int **) but (struct open_file *) is also acceptable

 regards,
 	spz
 -- 
 spz@serpens.de (S.P.Zeidler)

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: spz@serpens.de
Cc: gnats-bugs@NetBSD.org, port-sparc64-maintainer@NetBSD.org,
        tsutsui@ceres.dti.ne.jp
Subject: Re: port-sparc64/46652 (sparc ofwboot.net fails to loadkernelviatftp)
Date: Sat, 14 Jul 2012 09:42:04 +0900

 spz@ wrote:

 > Thus wrote Izumi Tsutsui (tsutsui@ceres.dti.ne.jp):
 > 
 > > > net_tftp_bootp(struct of_dev *op)
 > > 
 > > Do you still prefer (int **) for net_tftp_bootp() to return
 > > netev_sock pointer, or (struct open_file *) is acceptable?
 > 
 > Sorry for the late answer.
 > I prefer (int **) but (struct open_file *) is also acceptable

 I notice old gcc 4.1.3 complained about (int **) with WARNS=2:
 ---
 cc1: warnings being treated as errors
 /usr/src/sys/arch/sparc/stand/ofwboot/ofdev.c: In function 'devopen':
 /usr/src/sys/arch/sparc/stand/ofwboot/ofdev.c:539: warning: dereferencing type-punned pointer will break strict-aliasing rules
 ---
 (probably that was the reason I changed it)

 gcc 4.5.3 doesn't warn it so is it okay to assume it was a false alarm?

 ---
 Izumi Tsutsui

From: "S.P.Zeidler" <spz@serpens.de>
To: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Cc: gnats-bugs@NetBSD.org, port-sparc64-maintainer@NetBSD.org
Subject: Re: port-sparc64/46652 (sparc ofwboot.net fails to loadkernelviatftp)
Date: Sun, 15 Jul 2012 15:11:48 +0200

 Thus wrote Izumi Tsutsui (tsutsui@ceres.dti.ne.jp):

 > I notice old gcc 4.1.3 complained about (int **) with WARNS=2:
 > ---
 > cc1: warnings being treated as errors
 > /usr/src/sys/arch/sparc/stand/ofwboot/ofdev.c: In function 'devopen':
 > /usr/src/sys/arch/sparc/stand/ofwboot/ofdev.c:539: warning: dereferencing type-punned pointer will break strict-aliasing rules
 > ---
 > (probably that was the reason I changed it)
 > 
 > gcc 4.5.3 doesn't warn it so is it okay to assume it was a false alarm?

 gcc 4.5 seems to be smarter about it.
 putting an int * into a location an int ** points to shouldn't cause grief :)

 I would assume that the danger of someone building Sparc bootblocks on a
 Vax is pretty low, too.

 regards,
 	spz
 -- 
 spz@serpens.de (S.P.Zeidler)

From: matthew green <mrg@eterna.com.au>
To: gnats-bugs@NetBSD.org
Cc: tsutsui@NetBSD.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
    spz@NetBSD.org
Subject: re: port-sparc64/46652 (sparc ofwboot.net fails to loadkernelviatftp)
Date: Mon, 16 Jul 2012 05:27:36 +1000

 >  > I notice old gcc 4.1.3 complained about (int **) with WARNS=2:
 >  > ---
 >  > cc1: warnings being treated as errors
 >  > /usr/src/sys/arch/sparc/stand/ofwboot/ofdev.c: In function 'devopen':
 >  > /usr/src/sys/arch/sparc/stand/ofwboot/ofdev.c:539: warning: dereferencing type-punned pointer will break strict-aliasing rules
 >  > ---
 >  > (probably that was the reason I changed it)
 >  > 
 >  > gcc 4.5.3 doesn't warn it so is it okay to assume it was a false alarm?
 >  
 >  gcc 4.5 seems to be smarter about it.
 >  putting an int * into a location an int ** points to shouldn't cause grief :)
 >  
 >  I would assume that the danger of someone building Sparc bootblocks on a
 >  Vax is pretty low, too.

 if necessary, i'd put a gcc 4.1-only Makefile conditional to disable
 this warning or feature.


 .mrg.

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: spz@NetBSD.org, mrg@eterna.com.au
Cc: gnats-bugs@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: port-sparc64/46652 (sparc ofwboot.net fails to loadkernelviatftp)
Date: Mon, 16 Jul 2012 11:47:23 +0900

 mrg@ wrote:

 > >  > I notice old gcc 4.1.3 complained about (int **) with WARNS=2:
 > >  > ---
 > >  > cc1: warnings being treated as errors
 > >  > /usr/src/sys/arch/sparc/stand/ofwboot/ofdev.c: In function 'devopen':
 > >  > /usr/src/sys/arch/sparc/stand/ofwboot/ofdev.c:539: warning: dereferencing type-punned pointer will break strict-aliasing rules
 > >  > ---
 > >  > (probably that was the reason I changed it)
 > >  > 
 > >  > gcc 4.5.3 doesn't warn it so is it okay to assume it was a false alarm?
 > >  
 > >  gcc 4.5 seems to be smarter about it.
 > >  putting an int * into a location an int ** points to shouldn't cause grief :)
 > >  
 > >  I would assume that the danger of someone building Sparc bootblocks on a
 > >  Vax is pretty low, too.

 Gcc version is specified by target or HAVE_GCC variable, not host,
 and I have not checked other compilers.

 > if necessary, i'd put a gcc 4.1-only Makefile conditional to disable
 > this warning or feature.

 Thanks.

 I'll apply the patch in PR with comment.

 ---
 Izumi Tsutsui

From: "Izumi Tsutsui" <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/46652 CVS commit: src/sys/arch/sparc/stand/ofwboot
Date: Mon, 16 Jul 2012 11:26:28 +0000

 Module Name:	src
 Committed By:	tsutsui
 Date:		Mon Jul 16 11:26:28 UTC 2012

 Modified Files:
 	src/sys/arch/sparc/stand/ofwboot: net.c net.h ofdev.c

 Log Message:
 Fix tftpboot which was broken by my botched WARNSfy in last year.
 Also add comments that mention libsa tftp requires network device socket
 in f_devdata in struct open_file, from spz@ in PR port-sparc64/46652.
 Briefly tested tftpboot and nfsboot on Ultra5.


 To generate a diff of this commit:
 cvs rdiff -u -r1.7 -r1.8 src/sys/arch/sparc/stand/ofwboot/net.c
 cvs rdiff -u -r1.2 -r1.3 src/sys/arch/sparc/stand/ofwboot/net.h
 cvs rdiff -u -r1.32 -r1.33 src/sys/arch/sparc/stand/ofwboot/ofdev.c

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

State-Changed-From-To: feedback->pending-pullups
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Sat, 21 Jul 2012 04:10:13 +0900
State-Changed-Why:
pullup-6 #433


From: "Jeff Rizzo" <riz@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/46652 CVS commit: [netbsd-6] src/sys/arch/sparc/stand/ofwboot
Date: Sat, 21 Jul 2012 00:04:56 +0000

 Module Name:	src
 Committed By:	riz
 Date:		Sat Jul 21 00:04:56 UTC 2012

 Modified Files:
 	src/sys/arch/sparc/stand/ofwboot [netbsd-6]: net.c net.h ofdev.c

 Log Message:
 Pull up following revision(s) (requested by tsutsui in ticket #433):
 	sys/arch/sparc/stand/ofwboot/net.h: revision 1.3
 	sys/arch/sparc/stand/ofwboot/net.c: revision 1.8
 	sys/arch/sparc/stand/ofwboot/ofdev.c: revision 1.33
 Fix tftpboot which was broken by my botched WARNSfy in last year.
 Also add comments that mention libsa tftp requires network device socket
 in f_devdata in struct open_file, from spz@ in PR port-sparc64/46652.
 Briefly tested tftpboot and nfsboot on Ultra5.


 To generate a diff of this commit:
 cvs rdiff -u -r1.7 -r1.7.10.1 src/sys/arch/sparc/stand/ofwboot/net.c
 cvs rdiff -u -r1.2 -r1.2.14.1 src/sys/arch/sparc/stand/ofwboot/net.h
 cvs rdiff -u -r1.32 -r1.32.10.1 src/sys/arch/sparc/stand/ofwboot/ofdev.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: spz@NetBSD.org
State-Changed-When: Wed, 25 Jul 2012 20:01:47 +0000
State-Changed-Why:
pullups done, all is well


>Unformatted:

Home	PR Database Search