Re: CVS commit: src/sys/dev
On Wed, May 25, 2011 at 04:33:38PM +, Masao Uebayashi wrote: Modified Files: src/sys/dev/bluetooth: bcsp.c bthub.c btuart.c src/sys/dev/ieee1394: fwdev.c fwmem.c fwohci.c Log Message: Declare cfdrivers using extern rather than including ioconf.h. This is wrong. Please revert it. The purpose of declaring things in header files is to make sure all uses are consistent. -- David A. Holland dholl...@netbsd.org
Re: CVS commit: src/sys/dev
On Thu, 26 May 2011, Iain Hibbert wrote: you might note that the bthub.o file (at least, I didn't check the others) does not actually use __func__ (unless DEBUG) and in fact the objdump does not change with the removal of ioconf.h sorry: does not change with the addition of no ehci iain
Re: CVS commit: src/sys/dev
On Thu, May 26, 2011 at 4:12 PM, David Holland dholland-sourcechan...@netbsd.org wrote: On Wed, May 25, 2011 at 04:33:38PM +, Masao Uebayashi wrote: Modified Files: src/sys/dev/bluetooth: bcsp.c bthub.c btuart.c src/sys/dev/ieee1394: fwdev.c fwmem.c fwohci.c Log Message: Declare cfdrivers using extern rather than including ioconf.h. This is wrong. Please revert it. The purpose of declaring things in header files is to make sure all uses are consistent. In general yes. In this paticular case no. -- David A. Holland dholl...@netbsd.org
Re: CVS commit: src/sys/dev
On Thu, 26 May 2011, Masao Uebayashi wrote: I wonder if that can be worked around by stripping or ignoring the thing you want to ignore (non-global symbols), rather than potentially introducing type inconsistency bugs? (objcopy -x will discard all non-global symbols, I don't know if that is too much) - I want to keep reproducibility even in intermediate files for better traceability. how do you detect different signatures? diff -I '__func__\.[0-9]*' may also suffice, if you were using objdump -D for that.. - I can't think of how cfdriver decls can lead to type inconsistency bugs. Ok, somebody makes a mistake and writes extern struct cfdata foo_cd; and it works by chance because the address was zero anyway, then later something else is changed around and all of a sudden, an innocent code causes a kernel panic.. you might note that the bthub.o file (at least, I didn't check the others) does not actually use __func__ (unless DEBUG) and in fact the objdump does not change with the removal of ioconf.h Right, bthub.o is not affected. fwmem.o is. ok but why change files not affected? You did not actually remove all inclusions of ioconf.h in any case.. (grep ioconf.h *.d finds two more in i386/GENERIC, I didn't look at any other arch) iain
Re: CVS commit: src/sys/dev
On Thu, May 26, 2011 at 5:39 PM, Iain Hibbert plu...@rya-online.net wrote: On Thu, 26 May 2011, Masao Uebayashi wrote: I wonder if that can be worked around by stripping or ignoring the thing you want to ignore (non-global symbols), rather than potentially introducing type inconsistency bugs? (objcopy -x will discard all non-global symbols, I don't know if that is too much) - I want to keep reproducibility even in intermediate files for better traceability. how do you detect different signatures? diff -I '__func__\.[0-9]*' may also suffice, if you were using objdump -D for that.. I did cmp(1), then objdump -D to see the real diff. - I can't think of how cfdriver decls can lead to type inconsistency bugs. Ok, somebody makes a mistake and writes extern struct cfdata foo_cd; and it works by chance because the address was zero anyway, then later something else is changed around and all of a sudden, an innocent code causes a kernel panic.. cfdriver must exist for either embedded/module forms. If not, it should cause link error (if I'm not missing anything). you might note that the bthub.o file (at least, I didn't check the others) does not actually use __func__ (unless DEBUG) and in fact the objdump does not change with the removal of ioconf.h Right, bthub.o is not affected. fwmem.o is. ok but why change files not affected? You did not actually remove all inclusions of ioconf.h in any case.. (grep ioconf.h *.d finds two more in i386/GENERIC, I didn't look at any other arch) Hopefully I'll remove all ioconf.h references in sys/ soon.
Re: CVS commit: src
On 5/26/11 5:25 AM, Masao Uebayashi wrote: Module Name:src Committed By: uebayasi Date: Thu May 26 04:25:28 UTC 2011 Modified Files: src/share/man/man5: boot.cfg.5 src/share/man/man8/man8.i386: boot.8 src/sys/arch/i386/stand/boot: boot2.c src/sys/arch/i386/stand/lib: bootmenu.c exec.c libi386.h src/sys/arch/i386/stand/pxeboot: main.c src/sys/arch/x86/include: bootinfo.h cpu.h src/sys/arch/x86/x86: x86_machdep.c src/sys/kern: init_main.c subr_userconf.c src/sys/sys: userconf.h Log Message: Support userconf(4) command in boot(8)/boot.cfg(5) on i386/amd64. From jmmv@, no objections seen in the proposed thread: http://mail-index.netbsd.org/tech-kern/2009/01/22/msg004081.html Wow. Looks like I didn't remember to submit this at the time. Thanks!
Re: CVS commit: src/sys/dev
On Thu, May 26, 2011 at 07:12:57AM +, David Holland wrote: On Wed, May 25, 2011 at 04:33:38PM +, Masao Uebayashi wrote: Modified Files: src/sys/dev/bluetooth: bcsp.c bthub.c btuart.c src/sys/dev/ieee1394: fwdev.c fwmem.c fwohci.c Log Message: Declare cfdrivers using extern rather than including ioconf.h. This is wrong. Please revert it. The purpose of declaring things in header files is to make sure all uses are consistent. I fully agree on this. Please back it out. Joerg
Re: CVS commit: src
On 05/26/11 06:25, Masao Uebayashi wrote:
Module Name: src
Committed By: uebayasi
Date: Thu May 26 04:25:28 UTC 2011
Modified Files:
src/share/man/man5: boot.cfg.5
src/share/man/man8/man8.i386: boot.8
src/sys/arch/i386/stand/boot: boot2.c
src/sys/arch/i386/stand/lib: bootmenu.c exec.c libi386.h
src/sys/arch/i386/stand/pxeboot: main.c
src/sys/arch/x86/include: bootinfo.h cpu.h
src/sys/arch/x86/x86: x86_machdep.c
src/sys/kern: init_main.c subr_userconf.c
src/sys/sys: userconf.h
Log Message:
Support userconf(4) command in boot(8)/boot.cfg(5) on i386/amd64.
From jmmv@, no objections seen in the proposed thread:
http://mail-index.netbsd.org/tech-kern/2009/01/22/msg004081.html
Hi,
with those changes I can't compile a kernel without USERCONF option set.
The changes in x86_machdep.c should be with in if defs on that options
Ok to commit the attached patch?
Lars
Index: x86_machdep.c
===
RCS file: /cvsroot/src/sys/arch/x86/x86/x86_machdep.c,v
retrieving revision 1.47
diff -u -p -r1.47 x86_machdep.c
--- x86_machdep.c 26 May 2011 04:25:28 - 1.47
+++ x86_machdep.c 26 May 2011 16:09:24 -
@@ -36,6 +36,7 @@ __KERNEL_RCSID(0, $NetBSD: x86_machdep.
#include opt_modular.h
#include opt_physmem.h
#include opt_splash.h
+#include opt_userconf.h
#include sys/types.h
#include sys/param.h
@@ -50,7 +51,10 @@ __KERNEL_RCSID(0, $NetBSD: x86_machdep.
#include sys/module.h
#include sys/sysctl.h
#include sys/extent.h
+
+#if defined(USERCONF)
#include sys/userconf.h
+#endif /* defined(USERCONF) */
#include x86/cpuvar.h
#include x86/cputypes.h
@@ -178,6 +182,7 @@ module_init_md(void)
}
#endif /* MODULAR */
+#if defined(USERCONF)
void
userconf_bootinfo(void)
{
@@ -197,6 +202,7 @@ userconf_bootinfo(void)
userconf_parse(bi-text);
}
}
+#endif /* defined USERCONF) */
void
cpu_need_resched(struct cpu_info *ci, int flags)
Re: CVS commit: src
On Fri, May 27, 2011 at 1:10 AM, Lars Heidieker l...@heidieker.de wrote: On 05/26/11 06:25, Masao Uebayashi wrote: Module Name: src Committed By: uebayasi Date: Thu May 26 04:25:28 UTC 2011 Modified Files: src/share/man/man5: boot.cfg.5 src/share/man/man8/man8.i386: boot.8 src/sys/arch/i386/stand/boot: boot2.c src/sys/arch/i386/stand/lib: bootmenu.c exec.c libi386.h src/sys/arch/i386/stand/pxeboot: main.c src/sys/arch/x86/include: bootinfo.h cpu.h src/sys/arch/x86/x86: x86_machdep.c src/sys/kern: init_main.c subr_userconf.c src/sys/sys: userconf.h Log Message: Support userconf(4) command in boot(8)/boot.cfg(5) on i386/amd64. From jmmv@, no objections seen in the proposed thread: http://mail-index.netbsd.org/tech-kern/2009/01/22/msg004081.html Hi, with those changes I can't compile a kernel without USERCONF option set. Oops. The changes in x86_machdep.c should be with in if defs on that options Ok to commit the attached patch? Looks good to me. Thanks. Masao
Re: CVS commit: src
On 5/26/11 5:10 PM, Lars Heidieker wrote:
Hi,
with those changes I can't compile a kernel without USERCONF option set.
The changes in x86_machdep.c should be with in if defs on that options
Ok to commit the attached patch?
@@ -178,6 +182,7 @@ module_init_md(void)
}
#endif/* MODULAR */
+#if defined(USERCONF)
void
userconf_bootinfo(void)
{
@@ -197,6 +202,7 @@ userconf_bootinfo(void)
userconf_parse(bi-text);
}
}
+#endif /* defined USERCONF) */
Missing ( before USERCONF.
Also, this is defining userconf_bootinfo conditionally. Did you
validate that all callers are protected by USERCONF as well? (An
alternative would be to make the body of userconf_bootinfo conditional,
not the function itself.
Re: CVS commit: src/sys/dev
On Thu, May 26, 2011 at 07:12:57AM +, David Holland wrote: On Wed, May 25, 2011 at 04:33:38PM +, Masao Uebayashi wrote: Modified Files: src/sys/dev/bluetooth: bcsp.c bthub.c btuart.c src/sys/dev/ieee1394: fwdev.c fwmem.c fwohci.c Log Message: Declare cfdrivers using extern rather than including ioconf.h. This is wrong. Please revert it. The purpose of declaring things in header files is to make sure all uses are consistent. Is there another header file that could contain: #define CFDRIVER(prefix) extern struct cfdriver prefix##_cd Then the bcsp.h could contain: CFDRIVER(bcsp); which gives (effectively) the same guarantee as including ioconf.h. I think something like that would help building drivers as kernel modules. David -- David Laight: da...@l8s.co.uk
Re: CVS commit: src
On Thu, May 26, 2011 at 06:10:42PM +0200, Lars Heidieker wrote: On 05/26/11 06:25, Masao Uebayashi wrote: Module Name:src Committed By: uebayasi Date: Thu May 26 04:25:28 UTC 2011 Modified Files: src/share/man/man5: boot.cfg.5 src/share/man/man8/man8.i386: boot.8 src/sys/arch/i386/stand/boot: boot2.c src/sys/arch/i386/stand/lib: bootmenu.c exec.c libi386.h src/sys/arch/i386/stand/pxeboot: main.c src/sys/arch/x86/include: bootinfo.h cpu.h src/sys/arch/x86/x86: x86_machdep.c src/sys/kern: init_main.c subr_userconf.c src/sys/sys: userconf.h Log Message: Support userconf(4) command in boot(8)/boot.cfg(5) on i386/amd64. From jmmv@, no objections seen in the proposed thread: http://mail-index.netbsd.org/tech-kern/2009/01/22/msg004081.html Hi, with those changes I can't compile a kernel without USERCONF option set. The changes in x86_machdep.c should be with in if defs on that options Ok to commit the attached patch? Conditional compilation clutters the code up. What if instead you add to kern_stub.c, #include sys/userconf.h __weak_alias(userconf_bootinfo, voidop); ? Dave -- David Young OJC Technologies dyo...@ojctech.com Urbana, IL * (217) 344-0444 x24
Re: CVS commit: src
On Thu, May 26, 2011 at 11:32:51AM -0500, David Young wrote: On Thu, May 26, 2011 at 06:10:42PM +0200, Lars Heidieker wrote: On 05/26/11 06:25, Masao Uebayashi wrote: ... with those changes I can't compile a kernel without USERCONF option set. The changes in x86_machdep.c should be with in if defs on that options Ok to commit the attached patch? Conditional compilation clutters the code up. What if instead you add to kern_stub.c, #include sys/userconf.h __weak_alias(userconf_bootinfo, voidop); I'd either: leave userconf_bootinfo() defined but with no body or: #define userconf_bootinfo() (or maybe both!) David -- David Laight: da...@l8s.co.uk
Re: CVS commit: src
On 05/26/11 18:30, Julio Merino wrote:
On 5/26/11 5:10 PM, Lars Heidieker wrote:
Hi,
with those changes I can't compile a kernel without USERCONF option set.
The changes in x86_machdep.c should be with in if defs on that options
Ok to commit the attached patch?
@@ -178,6 +182,7 @@ module_init_md(void)
}
#endif/* MODULAR */
+#if defined(USERCONF)
void
userconf_bootinfo(void)
{
@@ -197,6 +202,7 @@ userconf_bootinfo(void)
userconf_parse(bi-text);
}
}
+#endif /* defined USERCONF) */
Missing ( before USERCONF.
Also, this is defining userconf_bootinfo conditionally. Did you
validate that all callers are protected by USERCONF as well? (An
alternative would be to make the body of userconf_bootinfo conditional,
not the function itself.
Yes, all calls are under option USERCONF.
Re: CVS commit: src
On Thu, May 26, 2011 at 05:35:20PM +0100, David Laight wrote:
On Thu, May 26, 2011 at 11:32:51AM -0500, David Young wrote:
On Thu, May 26, 2011 at 06:10:42PM +0200, Lars Heidieker wrote:
On 05/26/11 06:25, Masao Uebayashi wrote:
...
with those changes I can't compile a kernel without USERCONF option set.
The changes in x86_machdep.c should be with in if defs on that options
Ok to commit the attached patch?
Conditional compilation clutters the code up. What if instead you add
to kern_stub.c,
#include sys/userconf.h
__weak_alias(userconf_bootinfo, voidop);
I'd either:
leave userconf_bootinfo() defined but with no body
or:
#define userconf_bootinfo()
(or maybe both!)
If you do both, ISTM that the preprocessor will turn this:
void
userconf_bootinfo(void)
{
#ifdef USERCONF
#endif /* USERCONF */
}
to this:
void
{
#ifdef USERCONF
#endif /* USERCONF */
}
which won't compile.
Leaving userconf_bootinfo() undefined but with no body seems equivalent
to aliasing to voidop() *except* that there's an extra 'ret' instruction
in the kernel somewhere. :-) Also, it precludes linking with an object
containing a real userconf_bootinfo(), later. There's still the #ifdef
clutter of userconf_bootinfo(), albeit greatly reduced.
Dave
--
David Young OJC Technologies
dyo...@ojctech.com Urbana, IL * (217) 344-0444 x24
Re: CVS commit: src/sys/dev
In article 20110526071257.ga16...@netbsd.org, David Holland dholland-sourcechan...@netbsd.org wrote: On Wed, May 25, 2011 at 04:33:38PM +, Masao Uebayashi wrote: Modified Files: src/sys/dev/bluetooth: bcsp.c bthub.c btuart.c src/sys/dev/ieee1394: fwdev.c fwmem.c fwohci.c Log Message: Declare cfdrivers using extern rather than including ioconf.h. This is wrong. Please revert it. The purpose of declaring things in header files is to make sure all uses are consistent. Agreed, what are you trying to achieve here? christos
Re: CVS commit: src/sys/dev
On Fri, May 27, 2011 at 1:30 AM, David Laight da...@l8s.co.uk wrote:
On Thu, May 26, 2011 at 07:12:57AM +, David Holland wrote:
On Wed, May 25, 2011 at 04:33:38PM +, Masao Uebayashi wrote:
Modified Files:
src/sys/dev/bluetooth: bcsp.c bthub.c btuart.c
src/sys/dev/ieee1394: fwdev.c fwmem.c fwohci.c
Log Message:
Declare cfdrivers using extern rather than including ioconf.h.
This is wrong. Please revert it.
The purpose of declaring things in header files is to make sure all
uses are consistent.
Is there another header file that could contain:
#define CFDRIVER(prefix) extern struct cfdriver prefix##_cd
sys/device.h has CFDRIVER_DECL, which defines (not declares) a
cfdriver struct. So something like
#ifndef _MODULE
#define CFDRIVER_DECL(x) extern struct cfdriver __CONCAT(x,_cd)
#else
#define CFDRIVER_DECL(x) \
struct cfdriver __CONCAT(x,_cd) = { ... }
#endif
would work. (Already working here.)
Then the bcsp.h could contain:
CFDRIVER(bcsp);
which gives (effectively) the same guarantee as including ioconf.h.
I think something like that would help building drivers as kernel modules.
David
--
David Laight: da...@l8s.co.uk
Re: CVS commit: src/sys/dev
On Fri, 27 May 2011, Masao Uebayashi wrote: On Thu, May 26, 2011 at 8:24 PM, Iain Hibbert plu...@rya-online.net wrote: On Thu, 26 May 2011, Masao Uebayashi wrote: On Thu, May 26, 2011 at 5:39 PM, Iain Hibbert plu...@rya-online.net wrote: On Thu, 26 May 2011, Masao Uebayashi wrote: - I can't think of how cfdriver decls can lead to type inconsistency bugs. Ok, somebody makes a mistake and writes extern struct cfdata foo_cd; and it works by chance because the address was zero anyway, then later something else is changed around and all of a sudden, an innocent code causes a kernel panic.. cfdriver must exist for either embedded/module forms. If not, it should cause link error (if I'm not missing anything). Of course, it would exist.. that is the bug file a: /* generated by config(1) */ struct cfdriver foo_cd; file b: /* generated by an inattentive programmer */ extern struct cfdata foo_cd; I see. Let's fix hundreds of these instances now. well, it is hypothetical of course - you wrote I can't think of how cfdriver decls can lead to type inconsistency bugs. and I have shown you one pretty obvious way in which that can happen.. iain
