Re: CVS commit: src/sys (for mutex_ownable())
Module Name:src Committed By: pgoyette Date: Mon May 1 21:35:26 UTC 2017 Modified Files: src/sys/kern: kern_mutex.c subr_lockdebug.c src/sys/rump/librump/rumpkern: locks.c src/sys/sys: mutex.h Log Message: Introduce mutex_ownable() to determine if it is possible for the current process to acquire a mutex. XXX I don't think this needs a kernel version bump, but not 100% XXX certain. If anyone thinks that the new overloading of the XXX "shared" argument to lockdebug_wantlock() deserves a bump, XXX please let me know! +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org | +--+--++
Re: CVS commit: src/sys/net
@@ -3385,6 +3428,13 @@ if_mcast_op(ifnet_t *ifp, const unsigned
{
int rc;
struct ifreq ifr;
+ bool need_unlock = false;
+
+ /* XXX if_ioctl_lock may or may not be held here */
+ if (ifp->if_ioctl_lock != NULL && !mutex_owned(ifp->if_ioctl_lock)) {
+ mutex_enter(ifp->if_ioctl_lock);
+ need_unlock = true;
+ }
It is my understanding that using mutex_owned() to effect locking
decisions is forbidden.
I understand the intent of doing it this way, but perhaps we should
revisit the callers and enforce that they always take the lock?
On Wed, 5 Apr 2017, Ryota Ozaki wrote:
Module Name:src
Committed By: ozaki-r
Date: Wed Apr 5 03:47:51 UTC 2017
Modified Files:
src/sys/net: if.c if.h if_ethersubr.c link_proto.c
Log Message:
Make sure to hold if_ioctl_lock when calling ifp->if_ioctl
Unfortunately callers of ifp->if_ioctl (if_addr_init, if_flags_set
and if_mcast_op) may or may not hold if_ioctl_lock, so we have to
hold the lock only if it's not held.
To generate a diff of this commit:
cvs rdiff -u -r1.389 -r1.390 src/sys/net/if.c
cvs rdiff -u -r1.236 -r1.237 src/sys/net/if.h
cvs rdiff -u -r1.240 -r1.241 src/sys/net/if_ethersubr.c
cvs rdiff -u -r1.34 -r1.35 src/sys/net/link_proto.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
!DSPAM:58e49ead273461966420671!
+--+--+--------+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee dot com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd dot org |
+--+--++
Re: CVS commit: src/tests/lib/libc/sys
Module Name:src Committed By: martin Date: Fri Mar 24 08:18:27 UTC 2017 Modified Files: src/tests/lib/libc/sys: t_mprotect.c Log Message: Do not toggle global security.pax.mprotect state in an attempt to activate it for the current process. It does not work and tests should not change global system state anyway. Instead: skip the test is pax.mprotect is not globally enabled. We could use a better check for this (querying the current processes pax flags), but unfortunately we don't have one. I'll be committing a new sysctl variable proc.curproc.paxflags which will provide the ability to quest the current process's pax flags (or, indeed, the pax flags for any process which the caller is "allowed to see"). +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/dev
Nat,
@@ -5788,6 +5790,9 @@ audio_sysctl_frequency(SYSCTLFN_ARGS)
sc->sc_iffreq = t;
error = audio_set_vchan_defaults(sc, AUMODE_PLAY | AUMODE_PLAY_ALL
| AUMODE_RECORD, >sc_format[0], 0);
+ if (error)
+ aprint_error("Invalid channel format, please check hardware "
+"capabilities\n");
mutex_exit(sc->sc_lock);
return error;
@@ -5837,6 +5842,9 @@ audio_sysctl_precision(SYSCTLFN_ARGS)
error = audio_set_vchan_defaults(sc, AUMODE_PLAY | AUMODE_PLAY_ALL
| AUMODE_RECORD, >sc_format[0], 0);
+ if (error)
+ aprint_error("Invalid channel format, please check hardware "
+"capabilities\n");
mutex_exit(sc->sc_lock);
return error;
@@ -5875,10 +5883,63 @@ audio_sysctl_channels(SYSCTLFN_ARGS)
sc->sc_channels = t;
error = audio_set_vchan_defaults(sc, AUMODE_PLAY | AUMODE_PLAY_ALL
| AUMODE_RECORD, >sc_format[0], 0);
-
+ if (error)
+ aprint_error("Invalid channel format, please check hardware "
+"capabilities\n");
mutex_exit(sc->sc_lock);
return error;
}
Perhaps it might be more helpful if you used different error messages
for these, to help identify which one occurs? Perhaps you could also
report the specific audio device, using aprint_error_dev()?
On Sat, 21 Jan 2017, Nathanial Sloss wrote:
Module Name:src
Committed By: nat
Date: Sat Jan 21 22:22:41 UTC 2017
Modified Files:
src/sys/dev: audio.c
Log Message:
Add auto config for channel/hw format.
Addresses PR kern/51703.
Addresses PR kern/51760.
Addresses PR kern/51879.
To generate a diff of this commit:
cvs rdiff -u -r1.291 -r1.292 src/sys/dev/audio.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
!DSPAM:5883df5326278963216815!
+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/usr.bin/make
Module Name:src
Committed By: sjg
Date: Sat Jan 14 22:58:04 UTC 2017
Modified Files:
src/usr.bin/make: make.1 var.c
src/usr.bin/make/unit-tests: varmisc.exp varmisc.mk
Log Message:
Allow providing a utc value to :{gm,local}time
Reviewed by: christos
The man-page descriptions for these options are rather awkward:
:gmtime[=utc]
The value is a format string for strftime(3), using gmtime(3).
If a utc value is not provided or is 0, the current time is
used.
It's not really clear that "the value" refers to the value of the
variable being modified, rather than the value of the modification.
Perhaps using an example here could make it more clear? (The example
from the updated unit tests could be used, but with a utc value of 1
representing '1970-01-01 00:00:01 UTC'.) Or reword to something along
the lines of
Convert the specified utc value using the variable's value as
a format string for strftime(3). If no utc value is given or
is 0, the current gmtime(3) / localtime(3) is used.
Also, is the value passed to :localtime[=utc] really a utc value? Or is
it a "corrected" localtime(3) value?
+--+--+----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/tests/lib/libc/sys
Should address PR bin/51869 On Sat, 14 Jan 2017, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Sat Jan 14 03:59:58 UTC 2017 Modified Files: src/tests/lib/libc/sys: Makefile Log Message: Set FILESBUILD=yes to actually run the creation script for the file. Should fix the build by creating a file which install can then find. To generate a diff of this commit: cvs rdiff -u -r1.46 -r1.47 src/tests/lib/libc/sys/Makefile Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. !DSPAM:5879a267143851834045299! +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
re: CVS commit: src/sys/sys
On Fri, 6 Jan 2017, matthew green wrote:
Paul Goyette writes:
Yeah, I managed to type -m instead of -f
Should already be fixed (via cvs admin) in the repo.
can you update the mailing list with the fixed text please?
that's the normal method as well.
Sorry, here's the revised commit message:
The inline functions {ms,us,ns}2bintime() are currently defined
incorrectly, and always result in the bt.frac being set to zero.
Even correcting for left- vs right shifts, the results are low
precision with at most 32 significant bits.
Rewrite using 64-bit multiply rather than division to get proper
results. Also the multiply is less costly than a divide on most
platforms.
Unfortunately, the line breaks didn't survive my 'cvs admin' command
(I tried to put them in the message using \n but that didn't work.)
+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/sys/sys
On Thu, 5 Jan 2017, Christos Zoulas wrote:
In article <20170105225115.78e7df...@cvs.netbsd.org>,
Paul Goyette <source-changes-d@NetBSD.org> wrote:
-=-=-=-=-=-
Module Name:src
Committed By: pgoyette
Date: Thu Jan 5 22:51:15 UTC 2017
Modified Files:
src/sys/sys: time.h
Log Message:
/home/paul/time.msg
Perhaps #define some BINTIME_SCALE_{M,U,N}S constants since they are repeated
in the code?
Sure - sounds like a good idea to me!
+--+--+----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/sys
On Wed, 4 Jan 2017, Christos Zoulas wrote: On Jan 5, 9:28am, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys | On Thu, 5 Jan 2017, Paul Goyette wrote: | | > It seems that pretty much everyone is in favor of using bintime() for the | > "raw" timestamp data in kernhist. I'll go ahead and start working on the | > required changes. | | One last question: Does this require a kernel version bump? | | I didn't bump previously, due to limited exposure. But this time, both | vmstat(1) and crash(8) are affected, and must match with their kernels. Well, if they don't work with the new kernel we need a bump. Yup. old crash(8) and old vmstat(1) will not work with newer kernel (and the opposite is also true) due to changes in structure layouts. I will bump the kernel shortly. Does anyone else have something coming soon that would like to "ride the bump"? +--+--+--------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
On Thu, 5 Jan 2017, Paul Goyette wrote: It seems that pretty much everyone is in favor of using bintime() for the "raw" timestamp data in kernhist. I'll go ahead and start working on the required changes. One last question: Does this require a kernel version bump? I didn't bump previously, due to limited exposure. But this time, both vmstat(1) and crash(8) are affected, and must match with their kernels. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
On Wed, 4 Jan 2017, Frank Kardel wrote: Completely agreed, That's why I proposed to consider struct bintime. We can tackle various aspects of improving the implementations benefiting from the improved APIs at any time later. IMHO struct bintime is sufficiently powerful enough to cover the interface stability requirements (nsec are already being split by 4 with 4+GHz cycle counters - so something better than timespec should already be in scope). It seems that pretty much everyone is in favor of using bintime() for the "raw" timestamp data in kernhist. I'll go ahead and start working on the required changes. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
On Wed, 4 Jan 2017, Frank Kardel wrote:
On 01/04/17 09:37, Paul Goyette wrote:
On Wed, 4 Jan 2017, Frank Kardel wrote:
I would also suggest a 64 bit fraction like in struct bintime, which we
already have and use internally for timekeeping anyway along with the
timespec, timeval conversion functions.
Thus collecting can be faster as we don't need to make the conversions
when collectiing. I deem converting at userlevel the least worry.
For time stamp fetching see "man 9 microtime" or
kern/kern_tc.c:{get,}bin{,up}time(). "get"-versions are cheaper, but only
updated at hardclock(). The non-get ones are directly related
to the current time counter.
Using bintime as the "native" timestamp representation would require
changes to the in-kernel structures, rather than just those used for
I see - I didn't look at the event collection yet. bintime is the native
kernel timestamp format,
though it is not used everywhere.
exporting the data to userland. And if the in-kernel structures are
changed, then we'll have to be able to print them from within the
kernel.
For userland we could just do float- or double-divide to convert the
fraction to printable format;
Yep that's the usual procedure.
sys/time.h already has a non-fp conversion to timeval/timespec.
static __inline void
bintime2timespec(const struct bintime *bt, struct timespec *ts)
{
ts->tv_sec = bt->sec;
ts->tv_nsec =
(long)(((uint64_t)10 * (uint32_t)(bt->frac >>
32)) >> 32);
}
it's not clear how we could print these
values from within the kernel (where we're not allowed to use fp?).
Would converting to timespec/timeval and the usual %d.%06?d printf-fmt be an
option without creating a horrible data wrangling mess? (I don't know how
many
printfs would be affected).
There's not many printfs to worry about - exactly 1 in the kernel, and
only 1 that I know of in userland (in vmstat.c). (The kernel one is
intended to be called via ddb(4)'s 'show kernhist' command - or some
other debugger, but I guess you could figure out a way to call it from
somewhere else.)
As long as we can guarantee that a time_t is always 64-bits, I don't see
any reason why we can't use the conversion functions from sys/time.h in
both kernel and user side.
Of course, it all depends on whether we decide on bintime as the export
format.
Just out of curiousity, how expensive is bintime(9), compared to
microtime(9) or nanotime(9)? We're already doing microtime to get the
in-kernel timestamp, so if bintime(9) is the same cost, it would be
better (IMHO) to change the internal value now. Then the only time we
need any conversion is at printf() time.
Wow - I never imagined how many worms there were in this little can!
:)
+--+------++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/sys
On Wed, 4 Jan 2017, Robert Elz wrote: Date:Wed, 4 Jan 2017 05:52:45 + From:David Holland <dholland-sourcechan...@netbsd.org> Message-ID: <20170104055245.ga16...@netbsd.org> | also make it nsec... Since it is a 64 bt value, better would be pico-secs if it is going to continue to be a timeval/timespec kind of number - but better would be to make it be a binary fraction of a second (ie: fixed point with the point to the left of the MSBit position - then the precision can grow as needed (to absurd values) without canging the API) Either is possible, although converting to fixed-point fraction is marginally more expensive (requires a division vs multiplication). Does anyone else have any preferences? I'd prefer that the next "touch" on this be the last one, at least for a while. :) +--+--+--------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
On Wed, 4 Jan 2017, Christos Zoulas wrote:
@@ -80,13 +80,14 @@ struct sysctl_history_list_entry {
/* info for a single history event */
struct sysctl_history_event {
- struct timespec she_tspec;
+ time_t she_time_sec;
+ uint32_tshe_time_usec;
+ uint32_tshe_filler;
I would make those explicitly sized types, uint64_t x 2
Sure!
+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/external/gpl2/xcvs/dist/src
Per discussion on irc, shouldn't the default remain as-is, and a new option be used to "honor" server-provided timestamps for unchanged files? On Sun, 18 Dec 2016, Christos Zoulas wrote: Module Name:src Committed By: christos Date: Mon Dec 19 04:37:13 UTC 2016 Modified Files: src/external/gpl2/xcvs/dist/src: update.c Log Message: add -t to preserve timestamps. To generate a diff of this commit: cvs rdiff -u -r1.6 -r1.7 src/external/gpl2/xcvs/dist/src/update.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. !DSPAM:5857641e142881477110141! +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/arch (lapic va change)
Module Name:src Committed By: maxv Date: Sun Dec 11 08:31:53 UTC 2016 Modified Files: src/sys/arch/amd64/amd64: machdep.c src/sys/arch/i386/i386: machdep.c src/sys/arch/x86/x86: pmap.c Log Message: Kenter local_apic_va to a fake physical page, because our x86 implementation expects this va to be valid even if no lapic is present; which probably is a bug in itself, but let's just reproduce the old behavior and rehide that bug. This change (and the subsequent fix for the build break) breaks the installation process when running under pkgsrc/emulators/qemu-2.7.0nb1 Booting the install media image gets as far as the end of device auto-configuration, and hangs after reporting the boot device and /path/to/modules. It never gets to starting the init(8) process. Reverting this fix (and the build break fix) restores the ability to finish booting, and actually gets into sysinst. I don't know what the correct fix is, but I think we should revert this commit (at least for amd64, not sure about i386) until it is better understood and can be fixed properly. This problem doesn't seem to be affecting the rel-eng amd64 test-bed, probably because it is likely running an older version of qemu? XXX Note that this is not the only problem with amd64 on -HEAD. The rel-eng test-bed has not had a successful install for several days. It seems to "hang" and time-out while running 'progress cp /usr/mdec/boot /mnt2/boot' command. (My own system, after reverting the lapic changes, hangs at the exact same place.) Perhaps I should start a separate thread for this 2nd issue? +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/arch
On Mon, 12 Dec 2016, Paul Goyette wrote: On Mon, 12 Dec 2016, Paul Goyette wrote: I am also seeing this. It occurs while building the XEN3_INSTALL_DOMU kernel (part of build./sh release). FWIW, my encounter with this error is on amd64 ... I committed Martin's "hack" to the amd64/machdep.c and the build now works there, too. Hopefully someone(TM) will be able to fix this correctly in the near future. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/arch
On Mon, 12 Dec 2016, Paul Goyette wrote: I am also seeing this. It occurs while building the XEN3_INSTALL_DOMU kernel (part of build./sh release). FWIW, my encounter with this error is on amd64 ... On Sun, 11 Dec 2016, Maxime Villard wrote: Le 11/12/2016 ? 17:08, Maxime Villard a ?crit : Le 11/12/2016 ? 16:37, Martin Husemann a ?crit : On Sun, Dec 11, 2016 at 08:31:53AM +, Maxime Villard wrote: Module Name:src Committed By:maxv Date:Sun Dec 11 08:31:53 UTC 2016 Modified Files: src/sys/arch/amd64/amd64: machdep.c src/sys/arch/i386/i386: machdep.c src/sys/arch/x86/x86: pmap.c Log Message: Kenter local_apic_va to a fake physical page, because our x86 implementation expects this va to be valid even if no lapic is present; which probably is a bug in itself, but let's just reproduce the old behavior and rehide that bug. i386 kernels do not build for me with this change (local_apic_va being undefined in machdep.c). Really? I did test this change on both i386 and amd64, with and without lapic (with virtualbox). I don't have access to my main machine now; but simply looking at the code, machdep.c includes , which includes , which exports local_apic_va. It does indeed rely on NACPICA to be defined, but I don't think I changed my configuration, it's the default build. [I'm forwarding this to source-changes] Le 11/12/2016 ? 20:50, Martin Husemann a ?crit : http://releng.netbsd.org/builds/HEAD/201612111020Z/i386.build.failed building standard kern library building standard kern library /home/source/ab/HEAD/src/sys/arch/i386/i386/machdep.c: In function 'init386': /home/source/ab/HEAD/src/sys/arch/i386/i386/machdep.c:1305:17: error: 'local_apic_va' undeclared (first use in this function) pmap_kenter_pa(local_apic_va, local_apic_pa, ^ /home/source/ab/HEAD/src/sys/arch/i386/i386/machdep.c:1305:17: note: each undeclared identifier is reported only once for each function it appears in I saw it with a custom kernel, but same issue. Martin ? Well perhaps I was compiling with a different configuration, but that would surprise me. I'm not sure I'll be able to fix it in the next few days - feel free to do it if you want to, otherwise you'll probably have to wait a bit... (sorry about that) +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++ !DSPAM:584df7ef241341685685094! +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/arch
I am also seeing this. It occurs while building the XEN3_INSTALL_DOMU kernel (part of build./sh release). On Sun, 11 Dec 2016, Maxime Villard wrote: Le 11/12/2016 à 17:08, Maxime Villard a écrit : Le 11/12/2016 à 16:37, Martin Husemann a écrit : On Sun, Dec 11, 2016 at 08:31:53AM +, Maxime Villard wrote: Module Name:src Committed By:maxv Date:Sun Dec 11 08:31:53 UTC 2016 Modified Files: src/sys/arch/amd64/amd64: machdep.c src/sys/arch/i386/i386: machdep.c src/sys/arch/x86/x86: pmap.c Log Message: Kenter local_apic_va to a fake physical page, because our x86 implementation expects this va to be valid even if no lapic is present; which probably is a bug in itself, but let's just reproduce the old behavior and rehide that bug. i386 kernels do not build for me with this change (local_apic_va being undefined in machdep.c). Really? I did test this change on both i386 and amd64, with and without lapic (with virtualbox). I don't have access to my main machine now; but simply looking at the code, machdep.c includes , which includes , which exports local_apic_va. It does indeed rely on NACPICA to be defined, but I don't think I changed my configuration, it's the default build. [I'm forwarding this to source-changes] Le 11/12/2016 à 20:50, Martin Husemann a écrit : http://releng.netbsd.org/builds/HEAD/201612111020Z/i386.build.failed building standard kern library building standard kern library /home/source/ab/HEAD/src/sys/arch/i386/i386/machdep.c: In function 'init386': /home/source/ab/HEAD/src/sys/arch/i386/i386/machdep.c:1305:17: error: 'local_apic_va' undeclared (first use in this function) pmap_kenter_pa(local_apic_va, local_apic_pa, ^ /home/source/ab/HEAD/src/sys/arch/i386/i386/machdep.c:1305:17: note: each undeclared identifier is reported only once for each function it appears in I saw it with a custom kernel, but same issue. Martin ? Well perhaps I was compiling with a different configuration, but that would surprise me. I'm not sure I'll be able to fix it in the next few days - feel free to do it if you want to, otherwise you'll probably have to wait a bit... (sorry about that) !DSPAM:584df6ad266431147076208! +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: sys/modules/spkr
I'm going to wait for a while until things (appear to) settle down, and then I plan on making spkr_synth the default (renaming the module to spkr); the existing spkr module (which is built only for amd64 and i386) will become spkr_pcppi. On Sun, 11 Dec 2016, Nathanial Sloss wrote: Hi Paul, Thanks, for making a module of spkr_synth. Spkr and spkr synth are undergoing a lot of changes so I can't wait to see what happens and hopefully I'll be able to help the process along. Best regards, Nat On Fri, 9 Dec 2016 10:43:28 Paul Goyette wrote: On Fri, 9 Dec 2016, I wrote (but not cc'd to source-changes-d): I noticed that you updated spkr.c to retain the existing default behavior of "spkr* at pcppi?" Do you have any intention to provide a synth_spkr module which would use your new capabilities? It looks like it would be fairly trivial. Also, rather than using #ifdef PCPPISPEAKER to conditionalize large chunks of dev/isa/spkr.c it seems to me that a better approach would have been to split the existing file into common code, and adding a new file for the pcppi attachment stuff. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++ !DSPAM:584cfa6435751525017680! +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: sys/modules/spkr
On Fri, 9 Dec 2016, I wrote (but not cc'd to source-changes-d): I noticed that you updated spkr.c to retain the existing default behavior of "spkr* at pcppi?" Do you have any intention to provide a synth_spkr module which would use your new capabilities? It looks like it would be fairly trivial. Also, rather than using #ifdef PCPPISPEAKER to conditionalize large chunks of dev/isa/spkr.c it seems to me that a better approach would have been to split the existing file into common code, and adding a new file for the pcppi attachment stuff. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/kern
On Fri, 18 Nov 2016, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Fri Nov 18 02:37:33 UTC 2016 Modified Files: src/sys/kern: subr_bufq.c Log Message: By popular request, don't both initializing a static pointer to NULL. bother I will update the log message via cvs admin. +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
re: CVS commit: src/tests/kernel
On Thu, 10 Nov 2016, matthew green wrote: also, root can't attach to pid1 if securelevel is >= 0. To adjust securelevel this test would need to be modified to run under rump ... We wouldn't want the test to manipulate securelevel of the running system. s/wouldn't want/*can't* by design have/. i don't know that running under rump is useful here. i certainly would not trust ptrace tests in a rump to cover it properly. this test should just be skipped if securelevel >= 0. fact is that very few systems run with securelevel these days, so it's a small portion of systems that won't have it. Yeah, rump probably doesn't make much sense here. Skipping the test (with atf_tc_skip(...) of course) is likely the best solution. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
re: CVS commit: src/tests/kernel
On Thu, 10 Nov 2016, matthew green wrote: "Kamil Rytarowski" writes: Module Name:src Committed By: kamil Date: Sun Nov 6 16:24:16 UTC 2016 Modified Files: src/tests/kernel: t_ptrace.c Log Message: Add new tests attach_pid0 and attach_pid1 to t_ptrace attach_pid0 asserts that it is not valid to attach PID 0 as it is a special kernel process. assert_pid1 asserts that non-root user cannot attach to PID 1 as it is the /dev/init process. This tests is skipped if run as root. also, root can't attach to pid1 if securelevel is >= 0. To adjust securelevel this test would need to be modified to run under rump ... We wouldn't want the test to manipulate securelevel of the running system. I'm wondering how many of the other test cases would be better if running under rump? +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/dev/pci
On Tue, 1 Nov 2016, Christos Zoulas wrote: On Nov 2, 8:49am, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/dev/pci | > Why *len = 1 here? Shouldn't it be 0 since there is no more room left? | | No. :) | | The maximum number of characters actually written by vsnprintf() will | never exceed (len - 1). So, dest gets incremented by the max, and len | gets decremented by the max. | | There is always enough room left for vsnprintf() to create a new | trailing NUL. But that's doing extra work for no reason... Perhaps. But, since dest points to the last valid character of the output buffer (not to the first character after the buffer), there really is room for one more character. Even in the non-overflow case, if you write x-1 chars (not including the trailing NUL) to a buffer of size x, the updated value of len will be 1 (and dest would point to the last valid character of the buffer). We would need additional code in the non-overflow path if (*len == 1) *len = 0; to maintain consistency with the overflow path. +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/dev/pci
On Wed, 2 Nov 2016, Christos Zoulas wrote:
In article <20161102003956.35d12f...@cvs.netbsd.org>,
Paul Goyette <source-changes-d@NetBSD.org> wrote:
-=-=-=-=-=-
+ /* Handle overflow */
+ if ((size_t)count >= *len) {
+ *dest += *len - 1;
+ *len = 1;
Why *len = 1 here? Shouldn't it be 0 since there is no more room left?
No. :)
The maximum number of characters actually written by vsnprintf() will
never exceed (len - 1). So, dest gets incremented by the max, and len
gets decremented by the max.
There is always enough room left for vsnprintf() to create a new
trailing NUL.
+--+--+----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/sys/dev/pci
On Fri, 28 Oct 2016, Paul Goyette wrote:
Attached is a revised patch, which has a prototype/signature much more
similar to snprintf(), more appropriate variable types, and improved
comments/documentation.
I'll plan on committing this sometime late next week...
One more revision. This one updates the length parameter as well as the
dest pointer, so the caller doesn't need to recalculate on each call.
I've also changed it to return an int value rather than void:
x = 0 success
x < 0vsnprintf() failed (for userland code, errno will
contain details)
x > 0success, however the output was truncated to avoid
overflowing the output buffer.
This variant seems to work just fine, both in-kernel and userland.
+--+--+----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++Index: pci_subr.c
===
RCS file: /cvsroot/src/sys/dev/pci/pci_subr.c,v
retrieving revision 1.152
diff -u -p -r1.152 pci_subr.c
--- pci_subr.c 20 Oct 2016 04:11:02 - 1.152
+++ pci_subr.c 30 Oct 2016 03:00:18 -
@@ -54,6 +54,7 @@ __KERNEL_RCSID(0, "$NetBSD: pci_subr.c,v
#include
#else
#include
+#include
#include
#include
#include
@@ -567,6 +568,45 @@ static const struct pci_class pci_class[
DEV_VERBOSE_DEFINE(pci);
+/*
+ * Append a formatted string to dest without writing more than len
+ * characters (including the trailing NUL character). dest and len
+ * are updated for use in subsequent calls to snappendf().
+ *
+ * Returns 0 on success, a negative value if vnsprintf() fails, or
+ * a positive value if the dest buffer would have overflowed.
+ */
+static int
+snappendf(char **, size_t *, const char * restrict, ...) __printflike(3,4);
+
+static int
+snappendf(char **dest, size_t *len, const char * restrict fmt, ...)
+{
+ va_list ap;
+ int count;
+
+ va_start(ap, fmt);
+ count = vsnprintf(*dest, *len, fmt, ap);
+ va_end(ap);
+
+ /* Let vsnprintf() errors bubble up to caller */
+ if (count < 0 || *len == 0)
+ return count;
+
+ /* Handle overflow */
+ if ((size_t)count >= *len) {
+ *dest += *len - 1;
+ *len = 1;
+ return 1;
+ }
+
+ /* Update dest & len to point at trailing NUL */
+ *dest += count;
+ *len -= count;
+
+ return 0;
+}
+
void
pci_devinfo(pcireg_t id_reg, pcireg_t class_reg, int showclass, char *cp,
size_t l)
@@ -577,9 +617,6 @@ pci_devinfo(pcireg_t id_reg, pcireg_t cl
pci_revision_t revision;
char vendor[PCI_VENDORSTR_LEN], product[PCI_PRODUCTSTR_LEN];
const struct pci_class *classp, *subclassp, *interfacep;
- char *ep;
-
- ep = cp + l;
pciclass = PCI_CLASS(class_reg);
subclass = PCI_SUBCLASS(class_reg);
@@ -612,32 +649,31 @@ pci_devinfo(pcireg_t id_reg, pcireg_t cl
interfacep++;
}
- cp += snprintf(cp, ep - cp, "%s %s", vendor, product);
+ (void)snappendf(, , "%s %s", vendor, product);
if (showclass) {
- cp += snprintf(cp, ep - cp, " (");
+ (void)snappendf(, , " (");
if (classp->name == NULL)
- cp += snprintf(cp, ep - cp,
- "class 0x%02x, subclass 0x%02x", pciclass,
subclass);
+ (void)snappendf(, ,
+ "class 0x%02x, subclass 0x%02x",
+ pciclass, subclass);
else {
if (subclassp == NULL || subclassp->name == NULL)
- cp += snprintf(cp, ep - cp,
+ (void)snappendf(, ,
"%s, subclass 0x%02x",
classp->name, subclass);
else
- cp += snprintf(cp, ep - cp, "%s %s",
+ (void)snappendf(, , "%s %s",
subclassp->name, classp->name);
}
if ((interfacep == NULL) || (interfacep->name == NULL)) {
if (interface != 0)
- cp += snprintf(cp, ep - cp,
- ", interface 0x%02x", interface);
+ (void)snappendf(, , ", interface 0x%02x",
+ interface);
Re: CVS commit: src/sys/dev/pci
...
or write a function that appends and moves the pointer, like
snappendprintf(char **dest, const char *end, fmt, ...)
snappendprintf() seemed a rather unwieldly function name, so I called it
snappendf()!
The attached diffs implement this function and use in throughout
pci_devinfo().
At some time in the future we will eventually get products whose verbose
description exceed the currently-allocated 256-byte buffer, so I really think
we should implement this (or something similar) to protect against buffer
overflows. Does anyone have any major objections? Are there better
alternatives than snappendf() that should be pursued instead?
Attached is a revised patch, which has a prototype/signature much more
similar to snprintf(), more appropriate variable types, and improved
comments/documentation.
I'll plan on committing this sometime late next week...
+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++Index: pci_subr.c
===
RCS file: /cvsroot/src/sys/dev/pci/pci_subr.c,v
retrieving revision 1.152
diff -u -p -r1.152 pci_subr.c
--- pci_subr.c 20 Oct 2016 04:11:02 - 1.152
+++ pci_subr.c 28 Oct 2016 08:58:59 -
@@ -54,6 +54,7 @@ __KERNEL_RCSID(0, "$NetBSD: pci_subr.c,v
#include
#else
#include
+#include
#include
#include
#include
@@ -567,6 +568,33 @@ static const struct pci_class pci_class[
DEV_VERBOSE_DEFINE(pci);
+/*
+ * Append a formatted string into dest without writing more than len
+ * characters (including the trailing NUL character).
+ *
+ * If vsnprintf() returns an error, *dest is unmodified. Otherwise,
+ * *dest is updated to point to the last available character in the
+ * buffer (which is occupied by the string-terminating NUL). It is
+ * not possible to differentiate between an output buffer that is
+ * exactly filled to capacity and one that would have overflowed.
+ */
+static void
+snappendf(char **, size_t, const char * restrict, ...) __printflike(3,4);
+
+static void
+snappendf(char **dest, size_t len, const char * restrict fmt, ...)
+{
+ va_list ap;
+ int count;
+
+ va_start(ap, fmt);
+ count = vsnprintf(*dest, len, fmt, ap);
+ va_end(ap);
+ if (count < 0 || len == 0)
+ return;
+ *dest += MIN((size_t)count, len - 1);
+}
+
void
pci_devinfo(pcireg_t id_reg, pcireg_t class_reg, int showclass, char *cp,
size_t l)
@@ -612,32 +640,29 @@ pci_devinfo(pcireg_t id_reg, pcireg_t cl
interfacep++;
}
- cp += snprintf(cp, ep - cp, "%s %s", vendor, product);
+ snappendf(, ep - cp, "%s %s", vendor, product);
if (showclass) {
- cp += snprintf(cp, ep - cp, " (");
+ snappendf(, ep - cp, " (");
if (classp->name == NULL)
- cp += snprintf(cp, ep - cp,
- "class 0x%02x, subclass 0x%02x", pciclass,
subclass);
+ snappendf(, ep - cp, "class 0x%02x, subclass 0x%02x",
+ pciclass, subclass);
else {
if (subclassp == NULL || subclassp->name == NULL)
- cp += snprintf(cp, ep - cp,
- "%s, subclass 0x%02x",
+ snappendf(, ep - cp, "%s, subclass 0x%02x",
classp->name, subclass);
else
- cp += snprintf(cp, ep - cp, "%s %s",
+ snappendf(, ep - cp, "%s %s",
subclassp->name, classp->name);
}
if ((interfacep == NULL) || (interfacep->name == NULL)) {
if (interface != 0)
- cp += snprintf(cp, ep - cp,
- ", interface 0x%02x", interface);
+ snappendf(, ep - cp, ", interface 0x%02x",
+ interface);
} else if (strncmp(interfacep->name, "", 1) != 0)
- cp += snprintf(cp, ep - cp, ", %s",
- interfacep->name);
+ snappendf(, ep - cp, ", %s", interfacep->name);
if (revision != 0)
- cp += snprintf(cp, ep - cp, ", revision 0x%02x",
- revision);
- cp += snprintf(cp, ep - cp, ")");
+ snappendf(, ep - cp, ", revision 0x%02x", revision);
+ snappendf(, ep - cp, ")");
}
}
Re: CVS commit: src/sys/dev/pci
... I think that perhaps a variant that returns the
number of characters appended would be more useful. like
if (len >= max)
dest = end;
else
dest += len;
but, I would leave the whole thing alone instead :-)
or write a function that appends and moves the pointer, like
snappendprintf(char **dest, const char *end, fmt, ...)
snappendprintf() seemed a rather unwieldly function name, so I called it
snappendf()!
The attached diffs implement this function and use in throughout
pci_devinfo().
At some time in the future we will eventually get products whose verbose
description exceed the currently-allocated 256-byte buffer, so I really
think we should implement this (or something similar) to protect against
buffer overflows. Does anyone have any major objections? Are there
better alternatives than snappendf() that should be pursued instead?
+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++Index: pci_subr.c
===
RCS file: /cvsroot/src/sys/dev/pci/pci_subr.c,v
retrieving revision 1.152
diff -u -p -r1.152 pci_subr.c
--- pci_subr.c 20 Oct 2016 04:11:02 - 1.152
+++ pci_subr.c 28 Oct 2016 05:07:41 -
@@ -54,6 +54,7 @@ __KERNEL_RCSID(0, "$NetBSD: pci_subr.c,v
#include
#else
#include
+#include
#include
#include
#include
@@ -567,6 +568,25 @@ static const struct pci_class pci_class[
DEV_VERBOSE_DEFINE(pci);
+/*
+ * Append a formatted string into dest without writing beyond end, and
+ * update dest to point to next available character position
+ */
+static void
+snappendf(char **dest, const char *end, const char * restrict fmt, ...)
+{
+ va_list ap;
+ int count;
+ int len = end - *dest;
+
+ va_start(ap, fmt);
+ count = vsnprintf(*dest, len, fmt, ap);
+ va_end(ap);
+ if (count > len)
+ count = len;
+ *dest += count;
+}
+
void
pci_devinfo(pcireg_t id_reg, pcireg_t class_reg, int showclass, char *cp,
size_t l)
@@ -612,32 +632,29 @@ pci_devinfo(pcireg_t id_reg, pcireg_t cl
interfacep++;
}
- cp += snprintf(cp, ep - cp, "%s %s", vendor, product);
+ snappendf(, ep, "%s %s", vendor, product);
if (showclass) {
- cp += snprintf(cp, ep - cp, " (");
+ snappendf(, ep, " (");
if (classp->name == NULL)
- cp += snprintf(cp, ep - cp,
- "class 0x%02x, subclass 0x%02x", pciclass,
subclass);
+ snappendf(, ep, "class 0x%02x, subclass 0x%02x",
+ pciclass, subclass);
else {
if (subclassp == NULL || subclassp->name == NULL)
- cp += snprintf(cp, ep - cp,
- "%s, subclass 0x%02x",
+ snappendf(, ep, "%s, subclass 0x%02x",
classp->name, subclass);
else
- cp += snprintf(cp, ep - cp, "%s %s",
+ snappendf(, ep, "%s %s",
subclassp->name, classp->name);
}
if ((interfacep == NULL) || (interfacep->name == NULL)) {
if (interface != 0)
- cp += snprintf(cp, ep - cp,
- ", interface 0x%02x", interface);
+ snappendf(, ep, ", interface 0x%02x",
+ interface);
} else if (strncmp(interfacep->name, "", 1) != 0)
- cp += snprintf(cp, ep - cp, ", %s",
- interfacep->name);
+ snappendf(, ep, ", %s", interfacep->name);
if (revision != 0)
- cp += snprintf(cp, ep - cp, ", revision 0x%02x",
- revision);
- cp += snprintf(cp, ep - cp, ")");
+ snappendf(, ep, ", revision 0x%02x", revision);
+ snappendf(, ep, ")");
}
}
Re: CVS commit: src/sys/dev/pci
On Wed, 26 Oct 2016, Paul Goyette wrote: HOWEVER, Looking at dev_untokenstring() it would appear that there is a potential problem there! We have cp = buf + strlcat(buf, words + *token, len - 2); cp[0] = ' '; cp[1] = '\0'; Since strlcat(3) (quoting the man page) "return[s] the total length of the string [it] tried to create", it is entirely possible for cp to point beyond the end of the buffer. Thus the insertion of a trailing space-between-word and the NUL character could occur on some random location. It would seem to me the this code should be written as newlen = strlcat(buf, word + *token, len - 2); if (newlen > len - 2) newlen = len - 2; cp = buf + newlen; cp[0] = ' '; cp[1] = '\0'; I have confirmed that this actually is the root cause of the stack overflow problem. With this fix, and the original buffer size of 64, I get a truncated product description (as expected), but the stack overflow is gone. With this fix and the updated buffer size, I get the full description without any overflow. With the original code (smaller buffer, above fix not included) I got consistent stack overflow instead of proper printing of the product description. The above fix has been committed. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/dev/pci
On Wed, 26 Oct 2016, Christos Zoulas wrote:
In article <pine.neb.4.64.1610260555490.23...@speedy.whooppee.com>,
Paul Goyette <p...@whooppee.com> wrote:
On Wed, 26 Oct 2016, matthew green wrote:
i think you're right that the 'cp' manipulation is the problem.
snprintf() will return the "desired" size, so upon the first
attempted overflow the 'cp' is moved beyond 'ep', and then the
next snprintf() gets a negative aka extremely massive value
for the buffer length and actual overflow happens here, and ssp
detects it.
the fix would be to change this:
cp += snprintf(cp, ep - cp, ...);
into this:
len = snprintf(cp, ep - cp, ...);
if (len > ep - cp)
return;
cp += len;
which is annoying because there are a lot of the former.
There's only 9 snprintf() calls. I could simply provide a macro:
#define ADD_TEXT(dest, end, format, ...)\
{ \
int len, max = (end) - (dest); \
len = snprintf((dest), max, (format), __VA_ARGS__); \
if (len > max) \
return; \
(dest) += len; \
}
Then all of the snprintf() calls become simply
ADD_TEXT(cp, ep, , ...);
(Of course, after last use I'd add a #undef ADD_TEXT to clean up...)
Do you really want to stop from attaching the driver because you could
not print it's name? ...
I don't see where this would prevent the device from attaching. It would
simply return a truncated description...
... I think that perhaps a variant that returns the
number of characters appended would be more useful. like
if (len >= max)
dest = end;
else
dest += len;
but, I would leave the whole thing alone instead :-)
or write a function that appends and moves the pointer, like
snappendprintf(char **dest, const char *end, fmt, ...)
Well, as I have already pointed out to mrg@ in private Email, this
change doesn't really fix the problem anyway!
During my earlier debugging I had confirmed that the caller of
pci_devinfo() had provided a buffer of size 256, which is more than
enough to contain the entire device description. At no time did cp
point beyond the end of the buffer, not even at the very end of the
function.
The only actual "overflow" that occurs is within pci_findproduct(). It
is the call to this function that provides a PCI_PRODUCTSTR_LEN-sized
buffer. I have confirmed that pci_findproduct() properly truncates its
result and doesn't write beyond the buffer provided. pci_devinfo()
provides "char product[PCI_PRODUCTSTR_LEN]" (and similarly vendor[]).
With my specific device (0x8086, 0x6f8a),
"Core i7-6xxxK/Xeon-D Memory Cont"/* chars 0 - 31 */
"roller (Target Address, Thermal,"/* chars 32 - 63 */
" RAS)" /* chars 64 - 68 */
and setting the length back to 64, the returned value was truncated (by
dev_untokenstring() in src/dev/dev_verbose.c) at the 'm' in "Thermal".
So, I'm still unclear where the stack overflow actually occurs, and
until that question is resolved, I'm not going to make any changes!
HOWEVER,
Looking at dev_untokenstring() it would appear that there is a potential
problem there! We have
cp = buf + strlcat(buf, words + *token, len - 2);
cp[0] = ' ';
cp[1] = '\0';
Since strlcat(3) (quoting the man page) "return[s] the total length of
the string [it] tried to create", it is entirely possible for cp to
point beyond the end of the buffer. Thus the insertion of a trailing
space-between-word and the NUL character could occur on some random
location.
It would seem to me the this code should be written as
newlen = strlcat(buf, word + *token, len - 2);
if (newlen > len - 2)
newlen = len - 2;
cp = buf + newlen;
cp[0] = ' ';
cp[1] = '\0';
???
+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
re: CVS commit: src/sys/dev/pci
On Wed, 26 Oct 2016, matthew green wrote:
i think you're right that the 'cp' manipulation is the problem.
snprintf() will return the "desired" size, so upon the first
attempted overflow the 'cp' is moved beyond 'ep', and then the
next snprintf() gets a negative aka extremely massive value
for the buffer length and actual overflow happens here, and ssp
detects it.
the fix would be to change this:
cp += snprintf(cp, ep - cp, ...);
into this:
len = snprintf(cp, ep - cp, ...);
if (len > ep - cp)
return;
cp += len;
which is annoying because there are a lot of the former.
There's only 9 snprintf() calls. I could simply provide a macro:
#define ADD_TEXT(dest, end, format, ...)\
{ \
int len, max = (end) - (dest); \
len = snprintf((dest), max, (format), __VA_ARGS__); \
if (len > max) \
return; \
(dest) += len; \
}
Then all of the snprintf() calls become simply
ADD_TEXT(cp, ep, , ...);
(Of course, after last use I'd add a #undef ADD_TEXT to clean up...)
+--+--+----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
re: CVS commit: src/sys/dev/pci
On Wed, 26 Oct 2016, matthew green wrote: "Paul Goyette" writes: Module Name:src Committed By: pgoyette Date: Tue Oct 25 05:43:40 UTC 2016 Modified Files: src/sys/dev/pci: pci_verbose.h Log Message: Increase max string length for PCI Product names. Affects only kernels with PCIVERBOSE (or corresponding module). We currently have a few product names that exceed the old limit, and this is triggering an SSP check in pci_devinfo(). This commit doesn't directly address the SSP issue, but pushes the can down the road... i think this requires a kernel bump as the kernel can now write more bytes than the driver may have provided. Done - thanks for the reminder! +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/modules
On Tue, 27 Sep 2016, Christos Zoulas wrote: On Sep 27, 7:26pm, p...@whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys/modules | I agree in principle, but I am greatly concerned about the definition of | "for now". If they get disabled, they'll get forgotten. So few people | seem to care about modules, and those that try to create modules often | screw something up because they didn't actually build and load as a | module. You and I will not forget it :-) But are the two of us sufficient to actually make forward progress? :-) :-) | So, I will undertake the effort to identify the pci-capable platforms, | update the conditional compilation stuff in the modules/Makefile, and | update all of the appropriate modules/md.* sets lists. I was trying to avoid some work that would need to be undone in the future anyway, and would add fragility and complexity to the build. OK. "For now" I will simply disable all the modules (and mark them as "obsolete" in the sets lists). +--+--+--------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
re: CVS commit: src/sys/modules
This was actually a bug in the twa.c code, with a missing ) in the
invocation of bswap64() macro.
I've fixed this one, and the build proceeds from this point, only to
fail with a CTASSERT in viomb. Looks like sparc{,64} have different
values for PAGE_SIZE and VIRTIO_PAGE_SIZE, so the compile-time assert
fails.
I'll be adjusting build logic (and sets lists) for all these modules in
the morning. I'm too sleepy to trust myself to get it even half-way
right tonight.
On Tue, 27 Sep 2016, Paul Goyette wrote:
On Tue, 27 Sep 2016, matthew green wrote:
this change appears to have broken the build for at least sparc:
OK, I'm building for sparc now. Will fix as soon as I find the problem.
In file included from ./ioconf.c:1:0,
from /usr/src7/sys/dev/pci/twa.c:3153:
./ioconf.h: In function 'twa_read_capacity':
./ioconf.h:5:0: error: unterminated argument list invoking macro "bswap64"
extern struct cfdriver twa_cd;
^
/usr/src7/sys/dev/pci/twa.c:780:14: error: assignment makes integer from
pointer without a cast [-Werror=int-conversion]
array_size = bswap64(_8btol(
^
In file included from /usr/src7/sys/sys/mount.h:49:0,
from ./ioconf.c:12,
from /usr/src7/sys/dev/pci/twa.c:3153:
/usr/src7/sys/sys/fstypes.h:37:1: error: expected ';' before 'typedef'
typedef struct { int32_t __fsid_val[2]; } fsid_t; /* file system id type */
^
[ ... ]
nbmake[7]: stopped in /usr/src7/sys/modules/twa
(several interspersed messages appear in my log..)
.mrg.
+--+--+----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
!DSPAM:57ea291f42096258720985!
+--+--+----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/sys/modules
Well, you could look for and compile them for all the platforms that have PCI, but is the additional complexity worth it compared to the benefit? Disabling them for now is the easiest way because no matter what, the way they attach or the module is designed will change. I agree in principle, but I am greatly concerned about the definition of "for now". If they get disabled, they'll get forgotten. So few people seem to care about modules, and those that try to create modules often screw something up because they didn't actually build and load as a module. So, I will undertake the effort to identify the pci-capable platforms, update the conditional compilation stuff in the modules/Makefile, and update all of the appropriate modules/md.* sets lists. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/modules
On Tue, 27 Sep 2016, Christos Zoulas wrote: | > Some of these drivers require PCI on machines that don't have it. | > We need to re-think the bus-specific attachments on modules... | | Yes, I was afraid of this... | | An earlier suggestion (of having all the attachments provide a "bus" | interface (such as ld_bus) won't currently work because config(1) does | not allow the interface attribute as a pseudo-root... The expedient fix is to disable the pci dependency modules until we come up with something better (simpler than doing conditional compilation and playing games with the sets). I think that config will need some work no matter work. There is even more complexity for drivers that support multiple bus attachments. Do those present themselves as a single module, or multiple modules? The directory structure of modules is not scalable, I am not sure what to do about that yet. I'd rather not completely disable these, as I fear that a more proper solution will be a long time coming (if ever). So, rather than just disabling them, I'd prefer to make them specific to amd64 and i386. The initial module (for combined nvme and ld_nvme) was (and still is) built only for these two platforms. I look forward to a productive discussion on tech-kern for ways to move forward. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/modules
On Tue, 27 Sep 2016, Christos Zoulas wrote: In article <pine.neb.4.64.1609271607580.17...@speedy.whooppee.com>, Paul Goyette <p...@whooppee.com> wrote: On Tue, 27 Sep 2016, matthew green wrote: this change appears to have broken the build for at least sparc: OK, I'm building for sparc now. Will fix as soon as I find the problem. Some of these drivers require PCI on machines that don't have it. We need to re-think the bus-specific attachments on modules... Yes, I was afraid of this... An earlier suggestion (of having all the attachments provide a "bus" interface (such as ld_bus) won't currently work because config(1) does not allow the interface attribute as a pseudo-root... +--+--+--------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
re: CVS commit: src/sys/modules
On Tue, 27 Sep 2016, matthew green wrote:
this change appears to have broken the build for at least sparc:
OK, I'm building for sparc now. Will fix as soon as I find the problem.
In file included from ./ioconf.c:1:0,
from /usr/src7/sys/dev/pci/twa.c:3153:
./ioconf.h: In function 'twa_read_capacity':
./ioconf.h:5:0: error: unterminated argument list invoking macro "bswap64"
extern struct cfdriver twa_cd;
^
/usr/src7/sys/dev/pci/twa.c:780:14: error: assignment makes integer from
pointer without a cast [-Werror=int-conversion]
array_size = bswap64(_8btol(
^
In file included from /usr/src7/sys/sys/mount.h:49:0,
from ./ioconf.c:12,
from /usr/src7/sys/dev/pci/twa.c:3153:
/usr/src7/sys/sys/fstypes.h:37:1: error: expected ';' before 'typedef'
typedef struct { int32_t __fsid_val[2]; } fsid_t; /* file system id type */
^
[ ... ]
nbmake[7]: stopped in /usr/src7/sys/modules/twa
(several interspersed messages appear in my log..)
.mrg.
!DSPAM:57ea129d185595490261520!
+--+--+----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
re: CVS commit: src/sys/modules
On Tue, 27 Sep 2016, matthew green wrote: wow! this is ugly :) someone should make these ld attachments go via a "bus" like ata or scsi, such that the main driver can link to a common "bus" module and then ld to the bus. then all the "ld_xxx" modules can disappear. Yeah, it's not pretty. but it's certainly better than the initial ld+nvme combined module which would preclude any other ld Creating an ld_bus attribute is an exercise for some developer with way too much time on his/her hands (but not myself). +--+--+--------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/distrib/sets/lists/modules
On Sat, 17 Sep 2016, David Holland wrote: On Sat, Sep 17, 2016 at 02:27:19AM +, Paul Goyette wrote: > Modified Files: >src/distrib/sets/lists/modules: md.amd64 md.i386 mi > > Log Message: > Fix sets lists for nvme module. Since it is being built only for the > i386 and amd64 platforms, the entries belong in the md.xxx lists, not > in the mi list. Shouldn't it be built for all platforms that are capable of plugging one in? Probably. but Jaromir only set it up for i386/amd64, and I was just following his lead. +--+--+--------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
(Redirecting to tech-kern, and moving source-changes-d to bcc)
The attached diffs make a first pass at dealing with this mess. It
creates two new modules (ld common code, and ld_nvme attach specific),
and updates the sets lists as appropriate. It also modifies the nvme
code to be able to handle "bus rescan" - attaching a modular driver
requires rescan capability on the parent device. And finally, the
changes remove the ld code from the nvme module.
I'm not prepared to commit this yet, since I'm not in a position to do
any serious testing (for one thing, I don't have a nvme device...) I
have successfully built a release with these changes.
There's still more work that eventually needs to be done. First, the
other attachments for ld should be modularized, and the parents should
be taught how to do a rescan. Secondly, the nvme module itself needs to
be split into nvme-common and nvme-pci pieces. But neither of these is
urgent.
I'd very much appreciate it if someone with the appropriate hardware
could do some testing. Just build a kernel with neither ld nor nvme
devices included, and build the modules. Put everything in the right
places, reboot, and try to 'modload ld_nvme'. You should see other
modules get autoloaded (turn on sysctl kern.module.verbose for more
details), and hopefully the nvme device will get attached. (But I'd be
very surprised if this works on the first attempt!)
On Sat, 17 Sep 2016, Paul Goyette wrote:
On Fri, 16 Sep 2016, Paul Goyette wrote:
Log Message:
make it possible to load nvme(4) as module to ease testing; currently
somewhat
non-optimal, since it includes the ld(4) code also and hence requires the
kernel config to have 'no ld'
Wouldn't it make more sense to also enable ld(4) as its own module, and
then have nvme "require" ld?
Actually, it looks like we really should have four separate modules here...
* ldFor the common ld code
* ld_nvme For the 'ld* at nvme?' attachment code, requires "ld,nvme"
* nvme For the common nvme code
* nvme_pci For the 'nvme* at pci?' attachment code, requires "nvme,pci"
+--+------++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
+--+------++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++--- /dev/null 2016-09-17 15:14:24.114633403 +0800
+++ sys/modules/ld/Makefile 2016-09-17 15:09:06.018064405 +0800
@@ -0,0 +1,11 @@
+# $NetBSD$
+
+.include "../Makefile.inc"
+
+.PATH: ${S}/dev
+
+KMOD= ld
+
+SRCS+= ld.c
+
+.include
--- /dev/null 2016-09-17 15:14:24.114633403 +0800
+++ sys/modules/ld_nvme/Makefile2016-09-17 15:09:17.913260673 +0800
@@ -0,0 +1,12 @@
+# $NetBSD$
+
+.include "../Makefile.inc"
+
+.PATH: ${S}/dev/ic
+
+KMOD= ld_nvme
+IOCONF=ld_nvme.ioconf
+
+SRCS+= ld_nvme.c
+
+.include
--- /dev/null 2016-09-17 15:14:24.114633403 +0800
+++ sys/modules/ld_nvme/ld_nvme.ioconf 2016-09-17 15:09:34.530402225 +0800
@@ -0,0 +1,10 @@
+# $NetBSD$
+
+ioconf ld_nvme
+
+include "conf/files"
+include "dev/pci/files.pci"
+
+pseudo-root nvme*
+
+ld* at nvme? nsid ?
Index: distrib/sets/lists/modules/md.amd64
===
RCS file: /cvsroot/src/distrib/sets/lists/modules/md.amd64,v
retrieving revision 1.67
diff -u -p -r1.67 md.amd64
--- distrib/sets/lists/modules/md.amd64 17 Sep 2016 02:27:19 - 1.67
+++ distrib/sets/lists/modules/md.amd64 17 Sep 2016 07:14:44 -
@@ -111,6 +111,8 @@
./@MODULEDIR@/i915drmkms/i915drmkms.kmod base-kernel-modules kmod
./@MODULEDIR@/itesio base-kernel-modules kmod
./@MODULEDIR@/itesio/itesio.kmod base-kernel-modules kmod
+./@MODULEDIR@/ld_nvme base-kernel-modules kmod
+./@MODULEDIR@/ld_nvme/ld_nvme.kmod base-kernel-modules kmod
./@MODULEDIR@/lg3303 base-kernel-modules kmod
./@MODULEDIR@/lg3303/lg3303.kmod base-kernel-modules kmod
./@MODULEDIR@/lm base-kernel-modules kmod
Index: distrib/sets/lists/modules/md.i386
===
RCS file: /cvsroot/src/distrib/sets/lists/modules/md.i386,v
retrieving revision 1.69
diff -u -p -r1.69 md.i386
--- distrib/s
Re: CVS commit: src/sys
On Fri, 16 Sep 2016, Paul Goyette wrote: Log Message: make it possible to load nvme(4) as module to ease testing; currently somewhat non-optimal, since it includes the ld(4) code also and hence requires the kernel config to have 'no ld' Wouldn't it make more sense to also enable ld(4) as its own module, and then have nvme "require" ld? Actually, it looks like we really should have four separate modules here... * ldFor the common ld code * ld_nvme For the 'ld* at nvme?' attachment code, requires "ld,nvme" * nvme For the common nvme code * nvme_pci For the 'nvme* at pci?' attachment code, requires "nvme,pci" +--+--+--------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
On Fri, 16 Sep 2016, Jaromir Dolecek wrote: Module Name:src Committed By: jdolecek Date: Fri Sep 16 11:35:07 UTC 2016 Modified Files: src/sys/dev/pci: nvme_pci.c src/sys/modules: Makefile Added Files: src/sys/modules/nvme: Makefile nvme.ioconf Log Message: make it possible to load nvme(4) as module to ease testing; currently somewhat non-optimal, since it includes the ld(4) code also and hence requires the kernel config to have 'no ld' Wouldn't it make more sense to also enable ld(4) as its own module, and then have nvme "require" ld? +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
re: CVS commit: src/sys/miscfs/specfs
On Thu, 8 Sep 2016, matthew green wrote: "Paul Goyette" writes: Module Name:src Committed By: pgoyette Date: Thu Sep 8 00:07:48 UTC 2016 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: if_config processing wants to auto-load modules named with an if_ prefix, while specfc wants to auto-load modules without the prefix. For modules which can be loaded both ways (ie, if_tap and if_tun), provide a simple conversion table for specfs so it can auto-load the if_ module. This table should always be quite small, and the auto-load operation is relatively infrequent, so the additional overhead of comparing names should be tolerable. would you mind reverting this and implementing the "dependant" module model mlelstv proposed? the above is a hack and doesn't scale or work if a new module with the same "problem" is introduced, as it requires the base kernel to be patched, where as a pair of modules can be added much more easily. Sure, I can do that. Point well taken re requiring base kernel mods to add new table entries. It will take me a couple of days to complete and test. I've got a lot of issues dealing with my new machine... +--+--+--------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src
This is all somewhat related to Christos's introduction of the MODULAR config yesterday, in an effort to reduce kernel footprint and get the number of "things" down below dtrace's limits. :) I'm not intending to make any changes to GENERIC... On Sat, 6 Aug 2016, Robert Elz wrote: Date:Sat, 06 Aug 2016 16:26:24 +1000 From:matthew green <m...@eterna.com.au> Message-ID: <27309.1470464...@splode.eterna.com.au> | > For now, this is still included as a built-in module in GENERIC kernels. | | i don't understand this "for now". It may be because I read list messages in bizarre orders, but I had already seen Christos later commits to fix things so autoloading ppp would actually work, so when I read Paul's message I interpreted it more as Kernel support for ppp being autoloaded is now there, but you can't actually use it yet Which is (probably) now no longer true - no idea, I don't use modules at all. kre +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
re: CVS commit: src
Yup - bad wording on my part. Ignore the "for now" comment completely. On Sat, 6 Aug 2016, matthew green wrote: For now, this is still included as a built-in module in GENERIC kernels. i don't understand this "for now". GENERIC should continue to include all drivers. .mrg. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/dev
On Wed, 27 Jul 2016, co...@sdf.org wrote:
Log Message:
If we're going to check for a NULL pointer, do the check before we
dereference it (to get the lock address).
To generate a diff of this commit:
cvs rdiff -u -r1.76 -r1.77 src/sys/dev/md.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/md.c
diff -u src/sys/dev/md.c:1.76 src/sys/dev/md.c:1.77
--- src/sys/dev/md.c:1.76 Mon Jan 4 16:24:52 2016
+++ src/sys/dev/md.cWed Jul 27 01:09:44 2016
@@ -1,4 +1,4 @@
-/* $NetBSD: md.c,v 1.76 2016/01/04 16:24:52 hannken Exp $ */
+/* $NetBSD: md.c,v 1.77 2016/07/27 01:09:44 pgoyette Exp $ */
/*
* Copyright (c) 1995 Gordon W. Ross, Leo Weppelman.
@@ -40,7 +40,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: md.c,v 1.76 2016/01/04 16:24:52 hannken Exp
$");
+__KERNEL_RCSID(0, "$NetBSD: md.c,v 1.77 2016/07/27 01:09:44 pgoyette Exp
$");
#ifdef _KERNEL_OPT
#include "opt_md.h"
@@ -414,13 +414,13 @@ mdstrategy(struct buf *bp)
sc = device_lookup_private(_cd, MD_UNIT(bp->b_dev));
- mutex_enter(>sc_lock);
-
if (sc == NULL || sc->sc_type == MD_UNCONFIGURED) {
bp->b_error = ENXIO;
goto done;
}
+ mutex_enter(>sc_lock);
+
switch (sc->sc_type) {
#if MEMORY_DISK_SERVER
case MD_UMEM_SERVER:
goto done; will exit mutex not entered now, I think.
Yup - already fixed.
Well, I had fixed it in my localcount branch, but not on HEAD. It
should be fixed everywhere now.
Thanks for pointing this out.
+--+----------++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/sys/dev
On Wed, 27 Jul 2016, co...@sdf.org wrote:
Log Message:
If we're going to check for a NULL pointer, do the check before we
dereference it (to get the lock address).
To generate a diff of this commit:
cvs rdiff -u -r1.76 -r1.77 src/sys/dev/md.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/dev/md.c
diff -u src/sys/dev/md.c:1.76 src/sys/dev/md.c:1.77
--- src/sys/dev/md.c:1.76 Mon Jan 4 16:24:52 2016
+++ src/sys/dev/md.cWed Jul 27 01:09:44 2016
@@ -1,4 +1,4 @@
-/* $NetBSD: md.c,v 1.76 2016/01/04 16:24:52 hannken Exp $ */
+/* $NetBSD: md.c,v 1.77 2016/07/27 01:09:44 pgoyette Exp $ */
/*
* Copyright (c) 1995 Gordon W. Ross, Leo Weppelman.
@@ -40,7 +40,7 @@
*/
#include
-__KERNEL_RCSID(0, "$NetBSD: md.c,v 1.76 2016/01/04 16:24:52 hannken Exp $");
+__KERNEL_RCSID(0, "$NetBSD: md.c,v 1.77 2016/07/27 01:09:44 pgoyette Exp $");
#ifdef _KERNEL_OPT
#include "opt_md.h"
@@ -414,13 +414,13 @@ mdstrategy(struct buf *bp)
sc = device_lookup_private(_cd, MD_UNIT(bp->b_dev));
- mutex_enter(>sc_lock);
-
if (sc == NULL || sc->sc_type == MD_UNCONFIGURED) {
bp->b_error = ENXIO;
goto done;
}
+ mutex_enter(>sc_lock);
+
switch (sc->sc_type) {
#if MEMORY_DISK_SERVER
case MD_UMEM_SERVER:
goto done; will exit mutex not entered now, I think.
Yup - already fixed.
+--+----------++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/sys
On Tue, 26 Jul 2016, Christos Zoulas wrote:
In article <20160726014949.33644f...@cvs.netbsd.org>,
Paul Goyette <source-changes-d@NetBSD.org> wrote:
-=-=-=-=-=-
Module Name:src
Committed By: pgoyette
Date: Tue Jul 26 01:49:49 UTC 2016
Modified Files:
src/sys/dev: vnd.c
src/sys/rump/dev/lib/libvnd: vnd_component.c
Log Message:
When calling devsw_attach() we need to use the expected/official driver
name (as listed in the devsw_conv[] table) to get the expected device
majors. Once rump initialization is finished (ie, it has created its
required device nodes), we need to detach the [bc]devsw so the module
initialization code doesn't get EEXIST.
Looks like this code is going to be duplicated a lot (it is already
in cgd). Why don't you factor it out to a utility function?
Yeah, I've been thinking pretty much the same thing. Just need to
figure out the most appropriate place to put it, and whether or not it
needs to be "macro-ized" (for, for example, concatenating a macro arg
"drv" with the rest of the _{b,c}{devsw,major} symbol names!)
+--+------+----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
re: CVS commit: [pgoyette-localcount] src/sys
On Mon, 18 Jul 2016, matthew green wrote:
Log Message:
Rump drivers are always installed via devsw_attach() so we need to
always allocate a 'struct localcount' for these drivers whenever they
are built as modules.
can you explain why every driver needs to be modified for localcount?
naively, it seems like a rather odd thing to require changes to every
devsw driver.
The localcount(9) mechanism is being (or, going to be) used by
devsw_detach() to ensure that there are no active uses. Without this,
the driver can be detached while other uses are in-flight.
It would have been much easier if devsw_attach() could have magically
allocated the localcount on behalf of the drivers, but that wouldn't
work because the {b,c}devsw structures are marked const throughout the
kernel.
Note that the localcount is only allocated when the driver is being
built as a module (including rump component/module). Built-in drivers
will not incur any additional overhead.
There are currently "timing windows" (albeit short ones) in all of these
drivers where the device can be deallocated and data structure memory
re-used by "other things". Strange things can happen.
There are other timing windows, too. A driver's code/module currently
can be removed from the autoconf tree while the code is still using its
data structures - pointers to the driver's softc are an especially
vulnerable issue. The autoconf() stuff is also being adapted to use
localcount(9) to prevent this. (And a more-ambitious driver roto-till
will be needed to implement the autoconf use-case.)
One final note: as of this moment, the "localcount" mechanism is only a
proof-of-concept. I have no plans to merge this into main-line without
significant discussion. (And, since he designed and did the initial
implementation, I'd hope the Taylor takes a big part in those
discussions!)
Hope this addresses your concerns. If not, I know you'll follow up with
more questions!
+--+--+--------+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/sys/kern
On Fri, 15 Jul 2016, Christos Zoulas wrote: In article <20160714215706.414caf...@cvs.netbsd.org>, Paul Goyette <source-changes-d@NetBSD.org> wrote: -=-=-=-=-=- Module Name:src Committed By: pgoyette Date: Thu Jul 14 21:57:06 UTC 2016 Modified Files: src/sys/kern: subr_autoconf.c Log Message: Remove a call to panic() which duplicates the subsequent KASSERT()! Perhaps change to KASSERTMSG()? Yeah - let me compile-test before committing! +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: build break for postfix/libexec/smtpd
(Note I fixed the subject line!) Christos just fixed this. Thanks! On Thu, 30 Jun 2016, Paul Goyette wrote: Module Name:src Committed By: christos Date: Wed Jun 29 20:07:52 UTC 2016 Modified Files: src/external/ibm-public/postfix/libexec/smtpd: Makefile Log Message: Fix MKCRYPTO=no To generate a diff of this commit: cvs rdiff -u -r1.3 -r1.4 \ src/external/ibm-public/postfix/libexec/smtpd/Makefile This change breaks the build for both MKCRYPTO=yes and MKCRYPTO=no #create smtpd/smtpd.d CC=/build/netbsd-local/tools/x86_64/amd64/bin/x86_64--netbsd-gcc /build/netbsd-local/tools/x86_64/amd64/bin/nbmkdep -f smtpd.d.tmp -- -std=gnu99--sysroot=/build/netbsd-local/dest/amd64 -DNETBSD4 -DUSE_SASL_AUTH -I/build/netbsd-local/src/external/ibm-public/postfix/dist -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/dns -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/global -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/master -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/util -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/tls -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/milter -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/xsasl -DUSE_SASL_AUTH -DDEF_SERVER_SASL_TYPE=\"dovecot\" -DHAS_SQLITE -DDEF_HTML_DIR=\"/usr/share/doc/reference/ref8/postfix\" -DDEF_README_DIR=\"/usr/share/examples/postfix\" -DDEF_SAMPLE_DIR=\"/usr/share/examples/postfix\" -DDEF_MANPAGE_DIR=\"/usr/share/man\" /build/netbsd-local/src/external/ibm-public/postfix/dist/src/smtpd/smtpd.c && mv smtpd.d.tmp smtpd.d /build/netbsd-local/src/external/ibm-public/postfix/dist/src/smtpd/smtpd.c:1123:25: fatal error: smtpd_token.h: No such file or directory compilation terminated. nbmkdep: compile failed. +--+------++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++ +--+------++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
[no subject]
Module Name:src Committed By: christos Date: Wed Jun 29 20:07:52 UTC 2016 Modified Files: src/external/ibm-public/postfix/libexec/smtpd: Makefile Log Message: Fix MKCRYPTO=no To generate a diff of this commit: cvs rdiff -u -r1.3 -r1.4 \ src/external/ibm-public/postfix/libexec/smtpd/Makefile This change breaks the build for both MKCRYPTO=yes and MKCRYPTO=no #create smtpd/smtpd.d CC=/build/netbsd-local/tools/x86_64/amd64/bin/x86_64--netbsd-gcc /build/netbsd-local/tools/x86_64/amd64/bin/nbmkdep -f smtpd.d.tmp -- -std=gnu99--sysroot=/build/netbsd-local/dest/amd64 -DNETBSD4 -DUSE_SASL_AUTH -I/build/netbsd-local/src/external/ibm-public/postfix/dist -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/dns -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/global -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/master -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/util -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/tls -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/milter -I/build/netbsd-local/src/external/ibm-public/postfix/dist/src/xsasl -DUSE_SASL_AUTH -DDEF_SERVER_SASL_TYPE=\"dovecot\" -DHAS_SQLITE -DDEF_HTML_DIR=\"/usr/share/doc/reference/ref8/postfix\" -DDEF_README_DIR=\"/usr/share/examples/postfix\" -DDEF_SAMPLE_DIR=\"/usr/share/examples/postfix\" -DDEF_MANPAGE_DIR=\"/usr/share/man\" /build/netbsd-local/src/external/ibm-public/postfix/dist/src/smtpd/smtpd.c && mv smtpd.d.tmp smtpd.d /build/netbsd-local/src/external/ibm-public/postfix/dist/src/smtpd/smtpd.c:1123:25: fatal error: smtpd_token.h: No such file or directory compilation terminated. nbmkdep: compile failed. +--+------++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/tests/net/route
On Tue, 21 Jun 2016, Ryota Ozaki wrote: Module Name:src Committed By: ozaki-r Date: Tue Jun 21 10:18:27 UTC 2016 Modified Files: src/tests/net/route: t_route.sh Log Message: Tweak route get outputs to make tests work "expire" value of route get output is unexpectedly a negative value on rump kernel for some reasons and the tests almost always fail on babylon5. So just ignore it to make tests work for now. Should fix it in the future. Wouldn't it be better to mark the test as "Expected Failure" ? +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/usr.bin/find
On Sun, 12 Jun 2016, Robert Elz wrote:
Date:Sun, 12 Jun 2016 03:21:35 +
From:"Paul Goyette" <pgoye...@netbsd.org>
Message-ID: <20160612032135.502ddf...@cvs.netbsd.org>
| Change -{min,max}depth argument name from n to depth so that the
| earlier statement concerning n being prefaced by a plus or minus.
This essentially fixes PR bin/46158 - all the other moaning in that
PR should really be someplace else. So feel free to close it.
Thanks for the PR pointer. It has been closed.
+--+--+--------+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
re: CVS commit: src/sys/modules
On Thu, 9 Jun 2016, matthew green wrote: "Paul Goyette" writes: Module Name:src Committed By: pgoyette Date: Thu Jun 9 04:44:19 UTC 2016 Modified Files: src/sys/modules: Makefile Added Files: src/sys/modules/ipl: Makefile Log Message: New module for ipl (aka ipfilter). fwiw, the old lkm was called "if_ipl". Well, it's really not an interface, so that's not really appropriate. Since the functionality is accessed by opening the device (and to do various ioctl's), naming the module the same as the device makes the autoload stuff work (in spec_vnops.c). thanks for restoring this lost functionality since netbsd-5. One note - when you load the module, the functionality comes up in the ipf -D disabled mode. You need to explicitly enable it with ipf -E before loading the first rules. And the module can only be unloaded if it has been disabled (with -D) _and_ there are no discernible outstanding accesses. FWIW, I've passed the diffs along upstream (to Darren Reed), but I don't know if he'll pick them up into the master sources. Not that there is much new development going on for which we might need to re-import. :) +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
On Thu, 26 May 2016, Juergen Hannken-Illjes wrote: Module Name:src Committed By: hannken Date: Thu May 26 11:09:55 UTC 2016 Modified Files: src/sys/kern: vfs_vnode.c src/sys/sys: vnode.h Log Message: Use vnode state to replace VI_MARKER, VI_CHANGING, VI_XLOCK and VI_CLEAN. Presented on tech-kern@ Do we need a kernel version bump? +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/share/man/man4
On Wed, 6 Jan 2016, Christos Zoulas wrote: As a temporary hack it is probably good enough. Longer term filemon should be removed/rewritten, and the close ordering problem should be handled. I think that a "better" approach here is probably to remove filemon(4)'s SET_FD ioctl (for the log_file fd) completely, and use the fd on which /dev/filemon is open to deliver the activity/event records. But this will reduce performance, as each activity would require two transitions over the kernel/user-land boundary (one to deliver the data to the monitoring application, and another one to store the data somewhere). I changed my mind; it does not help because one can always dup2 that file descriptor later to a lower fd and cause trouble. It is also confusing to the user. So it is really not a worthwhile hack to put. Perhaps a better hack is to have a filemon_disestablish hook at process exit that does the preliminary cleanup. Yep, the user could always shoot himself (and the system) in the foot! Let me see if I can come up with a exit_cleanup() implementation that is sufficiently clean as to be acceptable. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/share/man/man4
On Tue, 5 Jan 2016, Christos Zoulas wrote:
| Unless I can reliably determine which fd the program is using for its
| access to /dev/filemon I don't have anything to which I can compare the
| requested activity_log fd.
You could scan the whole fd array and look for DT_MISC with fops ==
filemon ops and if you find one that would cause a deadlock, deny.
Again this is a hack... But at least it should prevent the deadlock.
Yeah, this is workable, even if it is a HACK ! :)
The attached patch borrows extensively from fd_free() routine in
kern/kern_descrip.c
Let me know if this looks "good enough" and I will commit it. (I'll
also update the BUGS section of the man-page to describe the hack.)
FWIW, I have tested with log-file fd values of 1 and 4, while the
/dev/filemon fd is always 3. In the "1" case the patch correctly
returned EINVAL (it would cause a deadlock), while the "4" case
succeeded and no deadlock occurred.
+--+--+--------+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++Index: filemon.c
===
RCS file: /cvsroot/src/sys/dev/filemon/filemon.c,v
retrieving revision 1.24
diff -u -p -r1.24 filemon.c
--- filemon.c 5 Jan 2016 22:08:54 - 1.24
+++ filemon.c 6 Jan 2016 05:05:04 -
@@ -279,8 +279,12 @@ static int
filemon_ioctl(struct file * fp, u_long cmd, void *data)
{
int error = 0;
+ int i, nf, fd;
struct filemon *filemon;
struct proc *tp;
+ fdfile_t *ff;
+ filedesc_t *fdp;
+ fdtab_t *dt;
#ifdef DEBUG
log(logLevel, "filemon_ioctl(%lu)", cmd);;
@@ -303,8 +307,41 @@ filemon_ioctl(struct file * fp, u_long c
if (filemon->fm_fp)
fd_putfile(filemon->fm_fd);
+ /*
+* XXX HACK XXX
+*
+* Due to our taking an extra reference on the
+* descriptor's struct file, we need to ensure that
+* the descriptor number is at least as large as
+* the one used to access /dev/filemon. Otherwise,
+* we get a deadlock during process exit, waiting
+* for the output file's reference count.
+*/
+ fd = *((int *) data);
+ fdp = curproc->p_fd;
+ dt = curlwp->l_fd->fd_dt;
+ nf = dt->dt_nfiles;
+ error = EINVAL;
+ for (i = 0; i < nf; i++) {
+ if (i >= fd)
+ break;
+ ff = dt->dt_ff[i];
+ KASSERT(fd >= NDFDFILE ||
+ ff == (fdfile_t *)fdp->fd_dfdfile[i]);
+ if (ff == NULL)
+ continue;
+
+ if (ff->ff_file->f_type == DTYPE_MISC &&
+ ff->ff_file->f_ops == _fileops) {
+ error = 0;
+ break;
+ }
+ }
+ if (error)
+ break;
+
/* Now set up the new one */
- filemon->fm_fd = *((int *) data);
+ filemon->fm_fd = fd;
if ((filemon->fm_fp = fd_getfile(filemon->fm_fd)) == NULL) {
error = EBADF;
break;
Re: CVS commit: src/sys/dev/raidframe
Module Name:src Committed By: pgoyette Date: Sat Dec 26 00:58:45 UTC 2015 Modified Files: src/sys/dev/raidframe: rf_driver.c rf_driver.h rf_netbsdkintf.c Log Message: Modularize the raidframe driver, including rework of the unit attach code to permit detaching (and possible module unloading). Also, convert tsleep()/wakeup() locking to use cv_wait_sig()/cv_broadcast(). Tested in non-modular, modular-builtin, and modular-loaded-at-runtime environments. Hmmm, looks like I need to do some more testing, this time in a rump environment. The most recent automated test run shows six new failed tests related to raidframe. I'm looking into it... +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/dev/raidframe
On Sun, 27 Dec 2015, Robert Elz wrote: Date:Sat, 26 Dec 2015 12:59:01 + From:"Paul Goyette" <pgoye...@netbsd.org> Message-ID: <20151226125901.230e1f...@cvs.netbsd.org> | if it does, all we really lose is auto-configuration of raid-sets. Auto-config of raid is what makes many of my systems function, if that fails, the system should panic, not just bindly charge ahead. The failure of config_finalize_register() was already only a WARNING. This commit just restores the original semantics. Additionally, the only failure that i can find for ...register() is the return of EEXIST if the finalizer is already registered. This really can't happen any more, since rev 1.238 of kern/subr_autoconf.c +--+--+--------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/miscfs/specfs
On Wed, 23 Dec 2015, Christos Zoulas wrote: In article <2015135437.b205bf...@cvs.netbsd.org>, Paul Goyette <source-changes-d@NetBSD.org> wrote: -=-=-=-=-=- Module Name:src Committed By: pgoyette Date: Tue Dec 22 23:54:37 UTC 2015 Modified Files: src/sys/miscfs/specfs: spec_vnops.c Log Message: If we attempt to autoload a driver module, make sure we return an error if it fails. Otherwise we might end up calling a builtin-but-disabled driver module and that can generate all sorts of issues... Is there a reason to override the error? No. And since the error is already being reported, the change was unneeded. The bug I was following is elsewhere, so this change was backed out. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src
I just committed this. Thanks for letting me know. On Mon, 7 Dec 2015, Paul Goyette wrote: On Mon, 7 Dec 2015, Ryo ONODERA wrote: Hi, This breaks the kernel build as follows. /usr/src/sys/dev/clockctl.c:155:1: error: no previous prototype for 'clockctl_init' [-Werror=missing-prototypes] clockctl_init(void) ^ cc1: all warnings being treated as errors *** [clockctl.o] Error code 1 Lacking like this? Yes - please go ahead and commit. Index: sys/sys/clockctl.h === RCS file: /cvsroot/src/sys/sys/clockctl.h,v retrieving revision 1.16 diff -u -r1.16 clockctl.h --- sys/sys/clockctl.h 6 Sep 2015 06:01:02 - 1.16 +++ sys/sys/clockctl.h 7 Dec 2015 05:00:06 - @@ -71,6 +71,7 @@ voidclockctlattach(int); int clockctlopen(dev_t, int, int, struct lwp *); int clockctlclose(dev_t, int, int, struct lwp *); +int clockctl_init(void); int clockctlioctl(dev_t, u_long, void *, int, struct lwp *); #endif From: "Paul Goyette" <pgoye...@netbsd.org>, Date: Mon, 7 Dec 2015 03:25:58 + Module Name:src Committed By: pgoyette Date: Mon Dec 7 03:25:58 UTC 2015 Modified Files: src/distrib/sets/lists/modules: mi src/sys/compat/common: kern_time_50.c src/sys/dev: clockctl.c src/sys/modules: Makefile Added Files: src/sys/modules/clockctl: Makefile clockctl.ioconf Log Message: Modularize the clockctl pseudo-device and link to the build. To generate a diff of this commit: cvs rdiff -u -r1.80 -r1.81 src/distrib/sets/lists/modules/mi cvs rdiff -u -r1.29 -r1.30 src/sys/compat/common/kern_time_50.c cvs rdiff -u -r1.32 -r1.33 src/sys/dev/clockctl.c cvs rdiff -u -r1.162 -r1.163 src/sys/modules/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/clockctl/Makefile \ src/sys/modules/clockctl/clockctl.ioconf Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. -- Ryo ONODERA // ryo...@yk.rim.or.jp PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3 +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++ +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src
On Mon, 7 Dec 2015, Ryo ONODERA wrote: Hi, This breaks the kernel build as follows. /usr/src/sys/dev/clockctl.c:155:1: error: no previous prototype for 'clockctl_init' [-Werror=missing-prototypes] clockctl_init(void) ^ cc1: all warnings being treated as errors *** [clockctl.o] Error code 1 Lacking like this? Yes - please go ahead and commit. Index: sys/sys/clockctl.h === RCS file: /cvsroot/src/sys/sys/clockctl.h,v retrieving revision 1.16 diff -u -r1.16 clockctl.h --- sys/sys/clockctl.h 6 Sep 2015 06:01:02 - 1.16 +++ sys/sys/clockctl.h 7 Dec 2015 05:00:06 - @@ -71,6 +71,7 @@ voidclockctlattach(int); int clockctlopen(dev_t, int, int, struct lwp *); int clockctlclose(dev_t, int, int, struct lwp *); +int clockctl_init(void); int clockctlioctl(dev_t, u_long, void *, int, struct lwp *); #endif From: "Paul Goyette" <pgoye...@netbsd.org>, Date: Mon, 7 Dec 2015 03:25:58 + Module Name:src Committed By: pgoyette Date: Mon Dec 7 03:25:58 UTC 2015 Modified Files: src/distrib/sets/lists/modules: mi src/sys/compat/common: kern_time_50.c src/sys/dev: clockctl.c src/sys/modules: Makefile Added Files: src/sys/modules/clockctl: Makefile clockctl.ioconf Log Message: Modularize the clockctl pseudo-device and link to the build. To generate a diff of this commit: cvs rdiff -u -r1.80 -r1.81 src/distrib/sets/lists/modules/mi cvs rdiff -u -r1.29 -r1.30 src/sys/compat/common/kern_time_50.c cvs rdiff -u -r1.32 -r1.33 src/sys/dev/clockctl.c cvs rdiff -u -r1.162 -r1.163 src/sys/modules/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/clockctl/Makefile \ src/sys/modules/clockctl/clockctl.ioconf Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. -- Ryo ONODERA // ryo...@yk.rim.or.jp PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3 +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/distrib/sets/lists/modules
On Tue, 1 Dec 2015, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Tue Dec 1 09:15:58 UTC 2015 Modified Files: src/distrib/sets/lists/modules: ad.arm ad.mips md.amd64 Log Message: Finish up with the new compat_netbsd32_nfssrv module by adding it to the sets lists. This set of commits together should address PRs kern/50410 and kern/50486. This should be "kern/50410 and kern/50468" This is also the first of several new modules to be created in response to PR kern/50489 Looks like this got logged in gnats only for the last-mentioned PR, so I'll manually add to the audit trails. To generate a diff of this commit: cvs rdiff -u -r1.5 -r1.6 src/distrib/sets/lists/modules/ad.arm \ src/distrib/sets/lists/modules/ad.mips cvs rdiff -u -r1.63 -r1.64 src/distrib/sets/lists/modules/md.amd64 Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
On Mon, 30 Nov 2015, Christos Zoulas wrote: In article <20151130224854.4e00...@cvs.netbsd.org>, Paul Goyette <source-changes-d@NetBSD.org> wrote: #include #include @@ -392,7 +392,7 @@ rump___sysimpl_getpid(void ) register_t retval[2]; pid_t rv = -1; - (void)rsys_syscall(SYS_getpid, NULL, 0, retval); + rsys_syscall(SYS_getpid, NULL, 0, retval); Great, now coverity will start bitching again about we have x hundred places checking the result and 5 not. Why did you change that? I didn't directly change this - perhaps it is one of the regenerated files? In which case the makesyscalls.sh script should be fixed... I will check and fix. +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
On Mon, 30 Nov 2015, Christos Zoulas wrote: +# autoloadprefix the prefix for the autoload table name Why isn't that the emulation name? It is actually expected to be the emulation name. I'll be happy to change the variable/parameter name to be more explicit, if you would like. +--+--++ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
On Mon, 30 Nov 2015, Christos Zoulas wrote: In article <20151130224854.4e00...@cvs.netbsd.org>, Paul Goyette <source-changes-d@NetBSD.org> wrote: #include #include @@ -392,7 +392,7 @@ rump___sysimpl_getpid(void ) register_t retval[2]; pid_t rv = -1; - (void)rsys_syscall(SYS_getpid, NULL, 0, retval); + rsys_syscall(SYS_getpid, NULL, 0, retval); Great, now coverity will start bitching again about we have x hundred places checking the result and 5 not. Why did you change that? I think this is fixed now. You shouldn't need to edit the generated file any more. :) +--+--+----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys
On Mon, 30 Nov 2015, Christos Zoulas wrote:
In article <20151130224719.7d1b...@cvs.netbsd.org>,
Paul Goyette <source-changes-d@NetBSD.org> wrote:
-=-=-=-=-=-
+ const struct sc_auto *auto_list;
+ int code;
That should be u_int.
Will fix.
- if (module_autoload(syscalls_autoload[i].al_module,
- MODULE_CLASS_ANY) != 0 ||
+ if (module_autoload(auto_list->al_module,
+ MODULE_CLASS_ANY) != 0 ||
KNF.
Will fix.
+# autoloadprefix the prefix for the autoload table name
Why isn't that the emulation name?
Let me check on this one...
+ * Autoloadable syscall definition
+ */
+struct sc_auto {
+ u_int al_code;
+ const char *al_module;
+};
A better name would be sc_autoload.
Will fix.
+--+--+----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
Re: CVS commit: src/sys
On Mon, 30 Nov 2015, Christos Zoulas wrote: On Dec 1, 7:33am, p...@vps1.whooppee.com (Paul Goyette) wrote: -- Subject: Re: CVS commit: src/sys | On Mon, 30 Nov 2015, Christos Zoulas wrote: | | >> +# autoloadprefix the prefix for the autoload table name | > | > Why isn't that the emulation name? | | It is actually expected to be the emulation name. I'll be happy to | change the variable/parameter name to be more explicit, if you would | like. I think it is better to change it to something like emulname or emul or prefix or something along those lines. This way we can change all the .conf files to have just a couple of lines instead of a dozen just to prefix everything with foo_ Done. I renamed it to "emulname" +--+--+--------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com | | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--++
Re: CVS commit: src/sys/opencrypto
On Fri, 27 Nov 2015, Christos Zoulas wrote: Module Name:src Committed By: christos Date: Sat Nov 28 03:40:43 UTC 2015 Modified Files: src/sys/opencrypto: crypto.c Log Message: fix the build Thanks, and sorry for the breakage. +--+--+-+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--+-+
Re: CVS commit: src/sys/nfs
On Mon, 2 Nov 2015, Greg Oster wrote: On Mon, 2 Nov 2015 09:57:43 + "Paul Goyette" <pgoye...@netbsd.org> wrote: Module Name:src Committed By: pgoyette Date: Mon Nov 2 09:57:43 UTC 2015 Modified Files: src/sys/nfs: nfs_vfsops.c Log Message: Don't forget to call nfs_fini() when we're finished. Without this, we leave a dangling pool nfsrvdescpl around. Is this a candidate for pullups? (sounds like a good fix to me! :) ) Considering that this actually fixes (at least) one of my three nasty module-related issues (see thread on current-users), I think it is an excellent candidate for pull-up. +--+--+-----+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--+-+
Re: CVS commit: src/lib/libpanel
On Sun, 1 Nov 2015, Robert Elz wrote: Date:Sun, 1 Nov 2015 08:54:37 + From:"Thomas Klausner" <w...@netbsd.org> Message-ID: <20151101085437.75bf...@cvs.netbsd.org> | Modified Files: | src/lib/libpanel: panel_above.3 | | Log Message: | Improve wording. It would be even better, more precise, and more useful, if it said ... The bottom and the top panels can be obtained by passing a Less wordy, and perhaps easier to say (at least for an American English speaker: The bottom and top panels can be obtained by passing a :) .Dv NULL argument to .Fn panel_above and .Fn panel_below , respectively. kre +--+--+---------+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--+-+
Re: More terminal iocctl's need to be PASSTHROUGH?
On Wed, 21 Oct 2015, Christos Zoulas wrote:
On Oct 21, 10:30am, p...@vps1.whooppee.com (Paul Goyette) wrote:
-- Subject: More terminal iocctl's need to be PASSTHROUGH?
| I noticed christos updated the list of PASSTHROGUH IOCCTLs the other
| day, so I figured that might reduce the boucing in/out of compat module.
|
| But it seems that there's more work to do here. In a xterm window, and
| running tcsh as my shell, simply typing a backspace character causes the
| module to bounce in.
|
Can you apply this and tell me what it prints? It does not print for me.
christos
Index: tty.c
===
RCS file: /cvsroot/src/sys/kern/tty.c,v
retrieving revision 1.269
diff -u -u -r1.269 tty.c
--- tty.c 18 Oct 2015 15:58:23 - 1.269
+++ tty.c 21 Oct 2015 11:52:44 -
@@ -1395,6 +1395,7 @@
return EPASSTHROUGH;
default:
+ printf("BARF 0x%lx\n", cmd);
#ifdef COMPAT_60
error = compat_60_ttioctl(tp, cmd, data, flag, l);
if (error != EPASSTHROUGH)
I just booted up with this, and here's what I got. The first of
these happens almost immediately after starting xdm ...
# dmesg | grep BARF
BARF 0x40245765
BARF 0x40087603
BARF 0x40087603
BARF 0x80087602
BARF 0x20004b0a
BARF 0x20004b42
BARF 0x40044b41
BARF 0x20004b42
BARF 0x20004b07
BARF 0x40044b41
BARF 0x20004b42
BARF 0x20004b07
BARF 0x20004b0a
BARF 0x20007604
BARF 0x20004b0a
BARF 0x20007604
BARF 0x20004b07
BARF 0x40044b41
BARF 0x20004b42
BARF 0x20004b42
BARF 0x20004b42
BARF 0x20004b42
#
+--+--+-----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--+-+
Re: More terminal iocctl's need to be PASSTHROUGH?
Please add the following to the list - these occur when I try to
edit a previous command using tcsh/'s history (I pressed Up-Arrow,
Back-Space, retyped a character, and then Enter)
BARF 0x20004b08
BARF 0x20004b08
BARF 0x20004b08
BARF 0x20004b08
BARF 0x20004b08
BARF 0x20004b08
On Thu, 22 Oct 2015, Paul Goyette wrote:
On Wed, 21 Oct 2015, Christos Zoulas wrote:
On Oct 21, 10:30am, p...@vps1.whooppee.com (Paul Goyette) wrote:
-- Subject: More terminal iocctl's need to be PASSTHROUGH?
| I noticed christos updated the list of PASSTHROGUH IOCCTLs the other
| day, so I figured that might reduce the boucing in/out of compat module.
|
| But it seems that there's more work to do here. In a xterm window, and
| running tcsh as my shell, simply typing a backspace character causes the
| module to bounce in.
|
Can you apply this and tell me what it prints? It does not print for me.
christos
Index: tty.c
===
RCS file: /cvsroot/src/sys/kern/tty.c,v
retrieving revision 1.269
diff -u -u -r1.269 tty.c
--- tty.c 18 Oct 2015 15:58:23 - 1.269
+++ tty.c 21 Oct 2015 11:52:44 -
@@ -1395,6 +1395,7 @@
return EPASSTHROUGH;
default:
+ printf("BARF 0x%lx\n", cmd);
#ifdef COMPAT_60
error = compat_60_ttioctl(tp, cmd, data, flag, l);
if (error != EPASSTHROUGH)
I just booted up with this, and here's what I got. The first of
these happens almost immediately after starting xdm ...
# dmesg | grep BARF
BARF 0x40245765
BARF 0x40087603
BARF 0x40087603
BARF 0x80087602
BARF 0x20004b0a
BARF 0x20004b42
BARF 0x40044b41
BARF 0x20004b42
BARF 0x20004b07
BARF 0x40044b41
BARF 0x20004b42
BARF 0x20004b07
BARF 0x20004b0a
BARF 0x20007604
BARF 0x20004b0a
BARF 0x20007604
BARF 0x20004b07
BARF 0x40044b41
BARF 0x20004b42
BARF 0x20004b42
BARF 0x20004b42
BARF 0x20004b42
#
+--+--+-----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--+-+
+--+--+-----+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--+-+
Re: More terminal iocctl's need to be PASSTHROUGH?
On Thu, 22 Oct 2015, Christos Zoulas wrote: Or sort -u KDGETLED KDSETLED KDSETMODE KDSKBMODE VT_GETMODE VT_RELDISP VT_SETMODE WSDISPLAYIO_GET_BUSID These have to do with the X server. The KD and VT ones are syscons/pcvt old ones. Are we still compiling with these enabled? Is that an old X server? The WS ones we could add.. This is an in-tree (xsrc) system with very recent userland (about one month old). /etc/ttys is also current: ... console "/usr/libexec/getty Pc" vt100 off secure constty "/usr/libexec/getty Pc" vt100 on secure ttyE0 "/usr/libexec/getty Pc" wsvt25 off secure ttyE1 "/usr/libexec/getty Pc" wsvt25 on secure ttyE2 "/usr/libexec/getty Pc" wsvt25 on secure ttyE3 "/usr/libexec/getty Pc" wsvt25 on secure ... +------+------+-+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--+-+
More terminal iocctl's need to be PASSTHROUGH?
I noticed christos updated the list of PASSTHROGUH IOCCTLs the other day, so I figured that might reduce the boucing in/out of compat module. But it seems that there's more work to do here. In a xterm window, and running tcsh as my shell, simply typing a backspace character causes the module to bounce in. Module Name:src Committed By: christos Date: Sun Oct 18 15:58:23 UTC 2015 Modified Files: src/sys/kern: tty.c Log Message: add the pty ioctls to pass through. To generate a diff of this commit: cvs rdiff -u -r1.268 -r1.269 src/sys/kern/tty.c +--+--+-+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--+-+
Re: CVS commit: src/sys/dev/gpio
Shouldn't we also add a dependency from the gpio module to require the sysmon_taskq module? On Thu, 15 Oct 2015, Jared D. McNeill wrote: Module Name:src Committed By: jmcneill Date: Thu Oct 15 09:07:49 UTC 2015 Modified Files: src/sys/dev/gpio: files.gpio Log Message: pull in sysmon_taskq To generate a diff of this commit: cvs rdiff -u -r1.13 -r1.14 src/sys/dev/gpio/files.gpio Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. +--+--+-+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--+-+
Re: CVS commit: src/tools/compat
(Resend with valid subject line) Module Name:src Committed By: christos Date: Thu Sep 17 02:22:47 UTC 2015 Modified Files: src/tools/compat: Makefile Log Message: make this more robust by using .CURDIR to cd. To generate a diff of this commit: cvs rdiff -u -r1.74 -r1.75 src/tools/compat/Makefile This seems to have broken the build for amd64. The following failure occurs while building tools target: ... # install /build/netbsd-local/tools/x86_64/amd64/include/compat/arpa /build/netbsd-local/obj/amd64/tools/binstall/xinstall -d /build/netbsd-local/tools/x86_64/amd64/include/compat/arpa # install /build/netbsd-local/tools/x86_64/amd64/include/compat/nbtool_config.h /build/netbsd-local/obj/amd64/tools/binstall/xinstall -c -r nbtool_config.h /build/netbsd-local/tools/x86_64/amd64/include/compat/nbtool_config.h cd: can't cd to /build/netbsd-local/src/tools/compat/include *** [includes] Error code 2 nbmake[2]: stopped in /build/netbsd-local/src/tools/compat 1 error +--+--+-+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--+-+
[no subject]
Module Name:src Committed By: christos Date: Thu Sep 17 02:22:47 UTC 2015 Modified Files: src/tools/compat: Makefile Log Message: make this more robust by using .CURDIR to cd. To generate a diff of this commit: cvs rdiff -u -r1.74 -r1.75 src/tools/compat/Makefile This seems to have broken the build for amd64. The following failure occurs while building tools target: ... # install /build/netbsd-local/tools/x86_64/amd64/include/compat/arpa /build/netbsd-local/obj/amd64/tools/binstall/xinstall -d /build/netbsd-local/tools/x86_64/amd64/include/compat/arpa # install /build/netbsd-local/tools/x86_64/amd64/include/compat/nbtool_config.h /build/netbsd-local/obj/amd64/tools/binstall/xinstall -c -r nbtool_config.h /build/netbsd-local/tools/x86_64/amd64/include/compat/nbtool_config.h cd: can't cd to /build/netbsd-local/src/tools/compat/include *** [includes] Error code 2 nbmake[2]: stopped in /build/netbsd-local/src/tools/compat 1 error +--+--+-+ | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | +--+--+-+
Re: CVS commit: src/sys/kern
On Wed, 19 Aug 2015, Masao Uebayashi wrote:
On Wed, Aug 19, 2015 at 5:31 AM, matthew green m...@eterna.com.au wrote:
Masao Uebayashi writes:
Module Name: src
Committed By: uebayasi
Date: Tue Aug 18 13:46:20 UTC 2015
Modified Files:
src/sys/kern: kern_cpu.c kern_drvctl.c
Log Message:
Convert pseudo attach functions to take no arguments, as some functions
(pppattach(), putterattach(), etc.) already do. This means that pseudo
attach function will be able to become a constructor.
how does this work?
these functions are called by ioconf.c that generated by config(1),
so i'm not sure how this can not be a build break, or at least a
confusing and inconsistent.
ioconf.c:void drvctlattach(int);
ioconf.c:const struct pdevinit pdevinit[] = {
...
ioconf.c: { drvctlattach, 1 },
subr_autoconf.c: for (pdev = pdevinit; pdev-pdev_attach != NULL; pdev++)
subr_autoconf.c: (*pdev-pdev_attach)(pdev-pdev_count);
what's the plan here? this reduction won't work for a number of
basic pdevs (like raid(4)) without updating it to not use the
count argument.
o I admit I did not check all, but 95% of pseudo attach functions do
not use `int n'.
o I assume that passing an `int' that is ignored by the callee
(func(void)) has proven to be harmless on all CPUs, because it has
worked.
o Killing `int n' is good for reducing kernel build complexity, and
reducing combinations of kernel configs.
o I am (too) optimistic that those pseudo attach functions that use
`int n' can be converted rather easily.
o For the first step, make it clear which functions use `int n' and not.
o If you are really unhappy of the `(void)', I can use `int n __unused'.
Wasn't the 'n' parameter supposed to be used to tell the driver how many
instances of the pseudo-device should be instantiated? At least, for
those drivers which do not automatically clone?
Have all of the pseudo-devices been modified to clone?
-
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
-
Re: CVS commit: src/sys/dev/sysmon
yeah - done! thanks! On Tue, 23 Jun 2015, Christoph Badura wrote: On Tue, Jun 23, 2015 at 10:41:42AM +, Paul Goyette wrote: Committed By: pgoyette Date: Tue Jun 23 10:41:42 UTC 2015 Modified Files: src/sys/dev/sysmon: sysmon_envsys_events.c Log Message: Fix the KASSERT - we want to make sure that _both_ pointers are non-NULL, not just that one or the other is non-NULL! Wouldn't it be better written as: KASSERT(sme != NULL); KASSERT(edata != NULL); Same effect, but when one of the triggers you know immediately which pointer was NULL. --chris - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys
On Mon, 18 May 2015, Joerg Sonnenberger wrote: On Sun, May 10, 2015 at 07:41:16AM +, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Sun May 10 07:41:16 UTC 2015 Modified Files: src/sys/compat/common: compat_mod.c sysv_ipc_50.c src/sys/kern: files.kern syscalls.master sysv_ipc.c src/sys/modules: Makefile src/sys/modules/compat: Makefile src/sys/sys: ipc.h Added Files: src/sys/compat/common: compat_sysv_mod.c src/sys/modules/compat_sysv: Makefile This broke sysutils/libgtop: /usr/include/sys/ipc.h:138:42: error: a parameter list without types is only allowed in a function definition void sysvipc50_set_compat_sysctl(int (*)(SYSCTLFN_PROTO)); Really? This line is wrapped inside '#ifdef _KERNEL' protection (see ipc.h:110) and should probably not be compiled for userland. Let me see if I can figure out what's going on... - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys
Hmmm, I just updated my pkgsrc tree and built libgtop-2.28.4nb7 without any problems. It doesn't look like anything in the package has been updated recently, so I don't understand why you got that error message. My build was running on a amd64-7.99.15 host. Can you retry? And provide complete log of the libgtop build? On Tue, 19 May 2015, Paul Goyette wrote: On Mon, 18 May 2015, Joerg Sonnenberger wrote: On Sun, May 10, 2015 at 07:41:16AM +, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Sun May 10 07:41:16 UTC 2015 Modified Files: src/sys/compat/common: compat_mod.c sysv_ipc_50.c src/sys/kern: files.kern syscalls.master sysv_ipc.c src/sys/modules: Makefile src/sys/modules/compat: Makefile src/sys/sys: ipc.h Added Files: src/sys/compat/common: compat_sysv_mod.c src/sys/modules/compat_sysv: Makefile This broke sysutils/libgtop: /usr/include/sys/ipc.h:138:42: error: a parameter list without types is only allowed in a function definition void sysvipc50_set_compat_sysctl(int (*)(SYSCTLFN_PROTO)); Really? This line is wrapped inside '#ifdef _KERNEL' protection (see ipc.h:110) and should probably not be compiled for userland. Let me see if I can figure out what's going on... - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | - - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/arch
Yes, if each entry in the path array were treated as a separate test case, then we could certainly skip the sysmon/power/wdog entries if the open returns ENODEV. However, there is one big list of paths, all tested in a single test case. It would not make sense to report the entire test case as skipped just because 1-3 of the many entries was ENODEV. So, even more arguably, the test itself should/could be rewritten to check each path separately, and then the individual sysmon-minor paths can skip on ENODEV. :) On Wed, 6 May 2015, Martin Husemann wrote: On Wed, May 06, 2015 at 07:43:02AM +0800, Paul Goyette wrote: Arguably, the atf test include/t_paths could have been updated to check for the specific error ENODEV and treat that as a PASS. Minor nit: it should skip, not pass. Martin - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
re: CVS commit: src/sys/arch
For i386 and amd64: non-XEN GENERIC is MODULAR, and the sysmon drivers will be autoloaded as necesary. Confirmed by successful running of atf test suite. For other platforms: MODULAR kernels are unaffected, as drivers will be autoloaded. Non-MODULAR kernels will contain sysmon sub-components only when there are active references. For example, if the kernel contains a watchdog device, sysmon_wdog will be included; if it contains a sensor device, sysmon_envsys will be included; and if the kernel contains a power button driver, sysmon_power will be included. I updated the x86/XEN configs because there are atf tests running, and those tests fail due to non-support of some sysmon sub-components. The only functionality being lost is the ability to run wdogctl(8) or envstat(8) only to find out that there are no watchdog timers or sensors included in the kernel. (Since the kernel is non-MODULAR, there's no chance of adding devices at run-time.) Opening the various sysmon minor device will return ENODEV, rather than having the GETxxx ioctl return empty results. Arguably, the atf test include/t_paths could have been updated to check for the specific error ENODEV and treat that as a PASS. On Wed, 6 May 2015, matthew green wrote: Paul Goyette writes: Module Name:src Committed By: pgoyette Date: Tue May 5 22:14:24 UTC 2015 Modified Files: src/sys/arch/amd64/conf: XEN3_DOMU src/sys/arch/i386/conf: XEN3_DOMU Log Message: For non-modular XEN3_DOMU kernels, include sysmon and all of its subcomponents. While the wdog and envsys subcomponents aren't terribly useful in DOMU environment, this restores functionality to previous (pre-modularized sysmon) state. does this mean that non-xen GENERIC is missing this functionality? that's a general regression. .mrg. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
re: CVS commit: src/sys/arch
GENERIC kernels already include at least one instance of sensor, watch-dog, and power-switch, so all the sysmon sub-component drivers are included and built-in whether or not the kernel is MODULAR. (Confirm by running modstat(8) and observe that all symon modules are built-in.) It seems to me that we should be (more) worried about autoloading isn't something you can depend on. Are there bugs or other issues concerning autoloading? If so, they need to be fixed. If it's just a matter of having autoload support in the kernel, how would that be any different than having support for all common drivers included/ built-in? :) On Wed, 6 May 2015, matthew green wrote: For i386 and amd64: non-XEN GENERIC is MODULAR, and the sysmon drivers will be autoloaded as necesary. Confirmed by successful running of atf test suite. it MODULAR, but it is supposed to have all common drivers in it. autoloading isn't something you can depend on. .mrg. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/dev/sysmon
On Tue, 5 May 2015, Christos Zoulas wrote: | If module_autoload() returns an error, just return that value instead | of overwriting with ENODEV. | | | Yes, but break before the mutex_enter... | | Need to hold the mutex so we can release it below... Return error then? Yeah, possible. let me have another look. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/dev/sysmon
On Tue, 5 May 2015, Christos Zoulas wrote: In article 20150505002825.9e50...@cvs.netbsd.org, Paul Goyette source-changes-d@NetBSD.org wrote: -=-=-=-=-=- Module Name:src Committed By: pgoyette Date: Tue May 5 00:28:25 UTC 2015 Modified Files: src/sys/dev/sysmon: sysmon.c Log Message: If module_autoload() returns an error, just return that value instead of overwriting with ENODEV. Thanks, christos! Yes, but break before the mutex_enter... Need to hold the mutex so we can release it below... - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/dev/sysmon
On Tue, 5 May 2015, Christos Zoulas wrote:
Module Name:src
Committed By: pgoyette
Date: Mon May 4 23:50:36 UTC 2015
Modified Files:
src/sys/dev/sysmon: sysmon.c
Log Message:
If autoload of the subcomponent module fails, don't try to call its
open routine. Just return an error.
Hopefully this will fix the recently reported issues with atf tests
running on xen guest.
-=-=-=-=-=-
#include sys/param.h
#include sys/conf.h
@@ -153,8 +153,10 @@ sysmonopen(dev_t dev, int flag, int mode
error = module_autoload(sysmon_mod[minor(dev)],
MODULE_CLASS_MISC);
I am confused; at this point if autoload returned an error, why bother
continuing and losing the error return from the autoload? Why not add:
if (error)
break;
mutex_enter(sysmon_minor_mtx);
- if (sysmon_opvec_table[minor(dev)] == NULL)
+ if (sysmon_opvec_table[minor(dev)] == NULL) {
error = ENODEV;
+ break;
+ }
}
error = (sysmon_opvec_table[minor(dev)]-so_open)(dev, flag,
mode, l);
-=-=-=-=-=-
Yeah, makes sense. I will update accordingly.
-
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com|
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
-
Re: CVS commit: src/sys/dev/sysmon
Well, it really is a (pseudo-)device, and even has its own major device number. We cannot avoid all of the device goop, since a non-built-in-module would still have to merge in the devsw. My goal is to eventually be able to build and run a kernel which has no portion of sysmon built-in, and still be able to load and unload the wdog/power/envsys components, along with the actual drivers for timers/power-switches/sensors. It's really not all that complicated. On Tue, 28 Apr 2015, Taylor R Campbell wrote: Date: Thu, 23 Apr 2015 23:22:03 + From: Paul Goyette pgoye...@netbsd.org Modularize sysmon and its components Why does this introduce a sysmon(4) autoconf device? It seems like needless bookkeeping -- you could just make the mutex global, like all the state it protects, and skip all the match/attach/detach/cfdriver autoconf business. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/dev/sysmon
Done. Doing some testing now but should be ok. On Sat, 25 Apr 2015, Christos Zoulas wrote: In article 1723.1429983...@splode.eterna.com.au, matthew green m...@eterna.com.au wrote: Christos Zoulas writes: Module Name:src Committed By: christos Date: Sat Apr 25 14:06:58 UTC 2015 Modified Files: src/sys/dev/sysmon: sysmon_envsys.c Log Message: make things boot again, from martin. thanks. updating to test now.. but shouldn't this use RUN_ONCE() and/or a lock/atomic? Yes, I am letting paul fix it properly the way he wants. christos - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/dev/sysmon
Thanks. :) I'll get this (and the corresponding changes to sysmon_power and sysmon_wdog subcomponents) done shortly. On Sat, 25 Apr 2015, Christos Zoulas wrote: In article 1723.1429983...@splode.eterna.com.au, matthew green m...@eterna.com.au wrote: Christos Zoulas writes: Module Name:src Committed By: christos Date: Sat Apr 25 14:06:58 UTC 2015 Modified Files: src/sys/dev/sysmon: sysmon_envsys.c Log Message: make things boot again, from martin. thanks. updating to test now.. but shouldn't this use RUN_ONCE() and/or a lock/atomic? Yes, I am letting paul fix it properly the way he wants. christos - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/dev/sysmon
Ooops - this should have been two commits. Log message is correct for sysmon.c For the others, message has been corrected to read: Handle early initialization requirements - thanks martin@ and others On Sat, 25 Apr 2015, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Sat Apr 25 23:40:09 UTC 2015 Modified Files: src/sys/dev/sysmon: sysmon.c sysmon_envsys.c sysmon_power.c sysmon_wdog.c Log Message: Correctly check return status when registering with pmf To generate a diff of this commit: cvs rdiff -u -r1.23 -r1.24 src/sys/dev/sysmon/sysmon.c cvs rdiff -u -r1.136 -r1.137 src/sys/dev/sysmon/sysmon_envsys.c cvs rdiff -u -r1.54 -r1.55 src/sys/dev/sysmon/sysmon_power.c cvs rdiff -u -r1.26 -r1.27 src/sys/dev/sysmon/sysmon_wdog.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/dev/sysmon
Yup - fixing now... On Sat, 25 Apr 2015, Jeff Rizzo wrote: On 4/25/15 3:46 PM, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Sat Apr 25 22:46:31 UTC 2015 + if (pmf_device_register(sysmon_dev, NULL, NULL)) + aprintf_error(%s: failed to register with pmf\n, I think you mean aprint_error. This doesn't link. +j - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/modules
Ooops - missed the log message - I will fix via cvs admin On Thu, 23 Apr 2015, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Thu Apr 23 23:22:28 UTC 2015 Modified Files: src/sys/modules: Makefile Added Files: src/sys/modules/sysmon: Makefile src/sys/modules/sysmon_envsys: Makefile src/sys/modules/sysmon_power: Makefile src/sys/modules/sysmon_taskq: Makefile src/sys/modules/sysmon_wdog: Makefile To generate a diff of this commit: cvs rdiff -u -r1.145 -r1.146 src/sys/modules/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/sysmon/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/sysmon_envsys/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/sysmon_power/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/sysmon_taskq/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/sysmon_wdog/Makefile Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
re: CVS commit: src/sys/modules
On Fri, 24 Apr 2015, matthew green wrote: Paul Goyette writes: Ooops - missed the log message - I will fix via cvs admin can you tell this list as well? :-) Sure. For all of these files, the corrected log message is Build modules for sysmon and its subcomponents On Thu, 23 Apr 2015, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Thu Apr 23 23:22:28 UTC 2015 Modified Files: src/sys/modules: Makefile Added Files: src/sys/modules/sysmon: Makefile src/sys/modules/sysmon_envsys: Makefile src/sys/modules/sysmon_power: Makefile src/sys/modules/sysmon_taskq: Makefile src/sys/modules/sysmon_wdog: Makefile To generate a diff of this commit: cvs rdiff -u -r1.145 -r1.146 src/sys/modules/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/sysmon/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/sysmon_envsys/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/sysmon_power/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/sysmon_taskq/Makefile cvs rdiff -u -r0 -r1.1 src/sys/modules/sysmon_wdog/Makefile Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | - - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
re: CVS commit: src/sys/sys
On Fri, 24 Apr 2015, matthew green wrote: Paul Goyette writes: Module Name:src Committed By: pgoyette Date: Thu Apr 23 23:23:20 UTC 2015 Modified Files: src/sys/sys: param.h Log Message: Welcome to 7.99.x and the modularization of sysmon! why bump the version? i didn't see any kernel ABI change. Lots of inter-module dependencies that won't get resolved properly if you mix new kernel with previous 7.99.10 modules. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
Re: CVS commit: src/sys/sys
I'll also fix this - we're now at 7.99.11 (not 'x') On Thu, 23 Apr 2015, Paul Goyette wrote: Module Name:src Committed By: pgoyette Date: Thu Apr 23 23:23:20 UTC 2015 Modified Files: src/sys/sys: param.h Log Message: Welcome to 7.99.x and the modularization of sysmon! To generate a diff of this commit: cvs rdiff -u -r1.472 -r1.473 src/sys/sys/param.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org | -
