Re: CVS commit: src/sys/net

2021-12-03 Thread Tobias Nygren
On Mon, 15 Nov 2021 07:07:06 +
Shoichi YAMAGUCHI  wrote:

> Date: Mon Nov 15 07:07:06 UTC 2021
> 
> Modified Files:
>   src/sys/net: if_ether.h if_ethersubr.c if_vlan.c
>   src/sys/net/lagg: if_lagg.c
> 
> Log Message:
> introduced APIs to configure VLAN TAG to ethernet devices

Hello,

This change seems to have broke something MTU related when
more than two VLANs are configured on the same NIC.
The first VLAN inherits the parent's MTU but the subsequent
ones have 4 byte too small MTU causing network problems.

Kernel from 2021-11-15:

# ifconfig |grep mtu
wm0: flags=0x8843 mtu 1500
vlan100: flags=0x8843 mtu 1500
vlan101: flags=0x8843 mtu 1500
vlan102: flags=0x8843 mtu 1500
vlan103: flags=0x8843 mtu 1500
vlan104: flags=0x8843 mtu 1500
vlan105: flags=0x8843 mtu 1500
vlan106: flags=0x8843 mtu 1500

Kernel from 2021-11-16:

wm0: flags=0x8843 mtu 1500
vlan100: flags=0x8843 mtu 1500
vlan101: flags=0x8843 mtu 1496
vlan102: flags=0x8843 mtu 1496
vlan103: flags=0x8843 mtu 1496
vlan104: flags=0x8843 mtu 1496
vlan105: flags=0x8843 mtu 1496
vlan106: flags=0x8843 mtu 1496

I could not force increase the MTU either:

root@tinfoilhat-b:log> ifconfig vlan100 mtu 1500
root@tinfoilhat-b:log> ifconfig vlan101 mtu 1500
ifconfig: SIOCSIFMTU: Invalid argument

Kind regards,
-Tobias


Re: CVS commit: src/etc/rc.d

2021-11-30 Thread Martin Husemann
On Tue, Nov 30, 2021 at 10:11:36AM +, Stephen Borrill wrote:
> In our products, we have a standard rc.conf and then a series of build
> scripts that configure and enable/disable services as required. We can
> switch between npf and ipfilter with a one-line change in a settings file,
> for example. We heavily rely on rc.conf.d functionality for this. We may put
> flags in there too.

You could achive that too by including "local" files from your generic 
/etc/rc.conf (like: ". /etc/conf/firewall", maybe even guarded by tests
for existance).

> I don't think putting name=YES into /etc/rc.conf.d/name is wrong or working
> by luck in general even if it is working by implication.

If we want to support that, we should document it and have tests for
it. Currently I would not guess this could work after reading the manual,
and would not think about such usage when extending/modifying anything in
/etc/rc*.

> I think the "load_rc_config_var npf npf" line I've proposed in npf_boot is a
> neat solution (and similar for pf_boot). It basically says get the value of
> npf from wherever you may find it. It also avoids potential contamination of
> the environment compared to my original fix.

Yes, and I am not objecting that detail.

> The split of /etc/rc.d/npf into two stages is analogous to swap1 and swap2.
> In that case, those scripts explicitly load_rc_config swap and do not take
> name into account.

We should ammend the documentation Robert cited to say something like
"in general $name, but in exceptional cases a well known service name
is used instead (like: swap, npf, ...)".

Martin


Re: CVS commit: src/etc/rc.d

2021-11-30 Thread Stephen Borrill

On 30/11/2021 09:43, Martin Husemann wrote:

On Tue, Nov 30, 2021 at 09:10:35AM +, Stephen Borrill wrote:

In rc.conf, npf=YES is sufficient, but you are advocating the setting needs
to be duplicated if put into rc.conf.d.


I think the confusion starts with the idea of enabling NPF by just
putting the NPF=yes into scripts in /etc/rc.conf.d.

It might have worked by pure luck in older releases, but it was wrong there
too.

I would argue that to enable it you should have NPF=yes in /etc/rc.conf,
and to override special stuff in the $name script  (which I can't think
of reasonable uses for this case) you would put that overrides into
/etc/rc.conf.d/$name.


In our products, we have a standard rc.conf and then a series of build 
scripts that configure and enable/disable services as required. We can 
switch between npf and ipfilter with a one-line change in a settings 
file, for example. We heavily rely on rc.conf.d functionality for this. 
We may put flags in there too.


I don't think putting name=YES into /etc/rc.conf.d/name is wrong or 
working by luck in general even if it is working by implication.


I think the "load_rc_config_var npf npf" line I've proposed in npf_boot 
is a neat solution (and similar for pf_boot). It basically says get the 
value of npf from wherever you may find it. It also avoids potential 
contamination of the environment compared to my original fix.


The split of /etc/rc.d/npf into two stages is analogous to swap1 and 
swap2. In that case, those scripts explicitly load_rc_config swap and do 
not take name into account.



--
Stephen


Re: CVS commit: src/etc/rc.d

2021-11-30 Thread Martin Husemann
On Tue, Nov 30, 2021 at 09:10:35AM +, Stephen Borrill wrote:
> In rc.conf, npf=YES is sufficient, but you are advocating the setting needs
> to be duplicated if put into rc.conf.d.

I think the confusion starts with the idea of enabling NPF by just
putting the NPF=yes into scripts in /etc/rc.conf.d.

It might have worked by pure luck in older releases, but it was wrong there
too.

I would argue that to enable it you should have NPF=yes in /etc/rc.conf,
and to override special stuff in the $name script  (which I can't think
of reasonable uses for this case) you would put that overrides into
/etc/rc.conf.d/$name.


Martin


Re: CVS commit: src/etc/rc.d

2021-11-30 Thread Stephen Borrill

On 26/11/2021 17:52, Robert Elz wrote:

 Date:Fri, 26 Nov 2021 13:11:36 +
 From:"Stephen Borrill" 
 Message-ID:  <20211126131136.63fabf...@cvs.netbsd.org>

   | Load rc configuration based on rcvar, not name, so that correct settings
   | in /etc/rc.conf.d are loaded.

This looks wrong to me (and a pullup request so soon after making a
change, before it has had any time for testing in HEAD is a *really*
bad idea).

   | Usually this does not matter as rcvar and name are set to the same value.
   | For pf_boot and npf_boot, rcvar is set to pf and npf respectively.
   |
   | Prior to the change, if:
   | rc.conf contains npf=YES
   | rc.conf.d/npf does not exist

Nor should it, that's not the file that is supposed to be used.

In rc.conf(5):


 rc.d(8) scripts that use load_rc_config from rc.subr(8) also support
 sourcing an optional end-user provided per-script override file
 /etc/rc.conf.d/service, (where service is the contents of the name
 variable in the rc.d(8) script).

That is, what should happen to make this work...

   | If:
   | rc.conf contains npf=NO (or is not set)
   | rc.conf.d/npf contains npf=YES

is that rc.conf.d/npf_boot should contain npf=YES

The rc.conf.d files have the same names as the rc.d/script files in
general, for good reason, as while they often only contain this
rcvar setting, they can contain overrides to anything in the script.
Further, if there is more than one rcvar in a script (which I think
has happened once or twice) the settings for both of them would go
in the same file, not one file for each of them.

   | This means that in the latter case, at boot time the npfctl start command
   | is never run and the firewall is not operational.

Because of user error.

Please revert this change, and request the pullup be undone as well.
I don't think it is as simple as this in the case of npf and pf. What 
has happened here is that the functionality of /etc/rc.d/npf has 
effectively been split into two parts (one to run early, the other 
late). It does not make sense to run one without the other. It's not 
like nfs which is a stack of mountd, rpcbind, nfsd, etc. which may well 
want different flags for each.


In rc.conf, npf=YES is sufficient, but you are advocating the setting 
needs to be duplicated if put into rc.conf.d. When upgrading from 8 to 
9, this is a breaking change.


I propose an alternative fix which is to change /etc/rc.d/npf_boot to read:

load_rc_config $name
load_rc_config_var npf npf

--
Stephen


Re: CVS commit: src/sbin/cgdconfig

2021-11-29 Thread Joerg Sonnenberger
On Sun, Nov 28, 2021 at 07:42:55AM -0800, Jason Thorpe wrote:
> 
> 
> > On Nov 27, 2021, at 6:01 PM, Christos Zoulas  wrote:
> > 
> > Module Name:src
> > Committed By:   christos
> > Date:   Sun Nov 28 02:01:30 UTC 2021
> > 
> > Modified Files:
> > src/sbin/cgdconfig: Makefile
> > 
> > Log Message:
> > -lpthread to LDADD (fixes lint build)
> 
> This change is wrong.  The -pthread option to the compiler does more than 
> just add -lpthread to the link phase.

Yeah, but the other changes are pretty much useless.

Joerg


Re: CVS commit: src/sbin/cgdconfig

2021-11-28 Thread Christos Zoulas


> On Nov 28, 2021, at 11:57 AM, Roland Illig  wrote:
>
> Am 28.11.2021 um 17:37 schrieb Jason Thorpe:
>>> On Nov 28, 2021, at 8:05 AM, Christos Zoulas 
>>> wrote:
>>>
>>> 1. which compilation flag should we add -pthread to? CFLAGS or
>>> COPTS? What about c++?
>>
>> GCC defines some preprocessor macros in response to -pthread, so …
>> CPPFLAGS?  Perhaps a better choice is to have a USE_PTHREADS that
>> individual program / library Makefiles can set to YES to cause the
>> right magic to happen in bsd.sys.mk?
>
> I like the idea of USE_PTHREADS.
>
> The option -pthread is not specified by POSIX and the GCC manual doesn't
> define which exact macros -pthread defines. Sure, Clang is compatible
> with GCC, but PCC doesn't need to. I don't want to add support for 3
> different compilers to lint. Having all the magic hidden behind a simple
> flag sounds easiest to me.
>

I agree!

christos



signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sbin/cgdconfig

2021-11-28 Thread Roland Illig

Am 28.11.2021 um 17:37 schrieb Jason Thorpe:

On Nov 28, 2021, at 8:05 AM, Christos Zoulas 
wrote:

1. which compilation flag should we add -pthread to? CFLAGS or
COPTS? What about c++?


GCC defines some preprocessor macros in response to -pthread, so …
CPPFLAGS?  Perhaps a better choice is to have a USE_PTHREADS that
individual program / library Makefiles can set to YES to cause the
right magic to happen in bsd.sys.mk?


I like the idea of USE_PTHREADS.

The option -pthread is not specified by POSIX and the GCC manual doesn't
define which exact macros -pthread defines. Sure, Clang is compatible
with GCC, but PCC doesn't need to. I don't want to add support for 3
different compilers to lint. Having all the magic hidden behind a simple
flag sounds easiest to me.

Roland


Re: CVS commit: src/sbin/cgdconfig

2021-11-28 Thread Jason Thorpe


> On Nov 28, 2021, at 8:05 AM, Christos Zoulas  wrote:
> 
> The change is correct; this is how it is done everywhere else in the tree. 
> You are right about -pthread doing more than adding -lpthread, but
> in that case, the -pthread should be added to CFLAGS/COPTS etc, 
> not LDADD so that it is effective during the compilation phase too, 
> not just the link phase. When I made the change, I considered going
> through the tree and adding -pthread to the CFLAGS/COPTS in the
> Makefiles where -pthread is in LDADD, but I did not want to do a
> half-assed job without thinking about it more:
> 
> 1. which compilation flag should we add -pthread to? CFLAGS or 
>   COPTS? What about c++?

GCC defines some preprocessor macros in response to -pthread, so … CPPFLAGS?  
Perhaps a better choice is to have a USE_PTHREADS that individual program / 
library Makefiles can set to YES to cause the right magic to happen in 
bsd.sys.mk?

> 2. do we remove the LDADD/DPADD pthread settings? I am thinking
>perhaps not, it does  not hurt, plus the DPADD will cause a rebuild 
>when libpthread changes.

That could be hidden away by the above suggestion.

-- thorpej



Re: CVS commit: src/sbin/cgdconfig

2021-11-28 Thread Christos Zoulas
The change is correct; this is how it is done everywhere else in the tree. 
You are right about -pthread doing more than adding -lpthread, but
in that case, the -pthread should be added to CFLAGS/COPTS etc, 
not LDADD so that it is effective during the compilation phase too, 
not just the link phase. When I made the change, I considered going
through the tree and adding -pthread to the CFLAGS/COPTS in the
Makefiles where -pthread is in LDADD, but I did not want to do a
half-assed job without thinking about it more:

1. which compilation flag should we add -pthread to? CFLAGS or 
   COPTS? What about c++?
2. do we remove the LDADD/DPADD pthread settings? I am thinking
perhaps not, it does  not hurt, plus the DPADD will cause a rebuild 
when libpthread changes.

The libargon addition to cgdconfig broke lint building because lint h
as not been taught about -pthread yet, and fixing it the way I fixed it, 
makes the lint  build work again and is consistent with the rest of the tree.

Best,

christos

> On Nov 28, 2021, at 10:42 AM, Jason Thorpe  wrote:
> 
> 
> 
>> On Nov 27, 2021, at 6:01 PM, Christos Zoulas  wrote:
>> 
>> Module Name: src
>> Committed By:christos
>> Date:Sun Nov 28 02:01:30 UTC 2021
>> 
>> Modified Files:
>>  src/sbin/cgdconfig: Makefile
>> 
>> Log Message:
>> -lpthread to LDADD (fixes lint build)
> 
> This change is wrong.  The -pthread option to the compiler does more than 
> just add -lpthread to the link phase.
> 
> -- thorpej



Re: CVS commit: src/sbin/cgdconfig

2021-11-28 Thread Jason Thorpe



> On Nov 27, 2021, at 6:01 PM, Christos Zoulas  wrote:
> 
> Module Name:  src
> Committed By: christos
> Date: Sun Nov 28 02:01:30 UTC 2021
> 
> Modified Files:
>   src/sbin/cgdconfig: Makefile
> 
> Log Message:
> -lpthread to LDADD (fixes lint build)

This change is wrong.  The -pthread option to the compiler does more than just 
add -lpthread to the link phase.

-- thorpej



Re: CVS commit: src/sys/dev/tprof

2021-11-26 Thread Nick Hudson

On 26/11/2021 13:24, Christos Zoulas wrote:

Module Name:src
Committed By:   christos
Date:   Fri Nov 26 13:24:28 UTC 2021

Modified Files:
src/sys/dev/tprof: tprof_armv7.c tprof_armv8.c

Log Message:
declare xc


Thanks!

Nick


Re: CVS commit: src/etc/rc.d

2021-11-26 Thread Robert Elz
Date:Fri, 26 Nov 2021 13:11:36 +
From:"Stephen Borrill" 
Message-ID:  <20211126131136.63fabf...@cvs.netbsd.org>

  | Load rc configuration based on rcvar, not name, so that correct settings
  | in /etc/rc.conf.d are loaded.

This looks wrong to me (and a pullup request so soon after making a
change, before it has had any time for testing in HEAD is a *really*
bad idea).

  | Usually this does not matter as rcvar and name are set to the same value.
  | For pf_boot and npf_boot, rcvar is set to pf and npf respectively.
  |
  | Prior to the change, if:
  | rc.conf contains nfp=YES
[ignoring the typo there]
  | rc.conf.d/npf does not exist

Nor should it, that's not the file that is supposed to be used.

In rc.conf(5):


rc.d(8) scripts that use load_rc_config from rc.subr(8) also support
sourcing an optional end-user provided per-script override file
/etc/rc.conf.d/service, (where service is the contents of the name
variable in the rc.d(8) script).

That is, what should happen to make this work...

  | If:
  | rc.conf contains npf=NO (or is not set)
  | rc.conf.d/npf contains npf=YES

is that rc.conf.d/npf_boot should contain npf=YES

The rc.conf.d files have the same names as the rc.d/script files in
general, for good reason, as while they often only contain this
rcvar setting, they can contain overrides to anything in the script.
Further, if there is more than one rcvar in a script (which I think
has happened once or twice) the settings for both of them would go
in the same file, not one file for each of them.

  | This means that in the latter case, at boot time the npfctl start command
  | is never run and the firewall is not operational.

Because of user error.

Please revert this change, and request the pullup be undone as well.

kre



Re: CVS commit: src/sys

2021-11-19 Thread Rin Okuyama

On 2021/11/20 8:46, Rin Okuyama wrote:

Enable this quirk for "C600/X79 AHCI". Also add commented out quirk
entries for "Bay Trail SATA (AHCI)" and "Mobile AHCI SATA Controller",
for which non-reproducible failures worked around by extra delays have
been reported.


Oops, I meant:

"Mobile AHCI SATA Controller" --> "82801I Mobile AHCI SATA Controller"

Thanks,
rin


Re: CVS commit: src/bin/echo

2021-11-10 Thread Valery Ushakov
On Thu, Nov 11, 2021 at 05:03:41 +0700, Robert Elz wrote:

> Date:Wed, 10 Nov 2021 22:17:05 +0300
> From:Valery Ushakov 
> Message-ID:  
> 
>   | > in the sense that simply falling out of main() is exit(0)?
>   |
>   | Surprisingly - yes.
> 
> That's appalling, but perhaps not surprising.
> 
> It breaks code which believed what was promised, and did return n
> (n != 0) instead of exit(n).

The main can still return an int explicitly.

  If the return type of the main function is a type compatible with
  int, a return from the initial call to the main function is
  equivalent to calling the exit function with the value returned by
  the main function as its argument;

It's specifically the sloppy

int main() {}

without explicit return that gets a dispensation.

-uwe


Re: CVS commit: src/bin/echo

2021-11-10 Thread Robert Elz
Date:Wed, 10 Nov 2021 22:17:05 +0300
From:Valery Ushakov 
Message-ID:  

  | > in the sense that simply falling out of main() is exit(0)?
  |
  | Surprisingly - yes.

That's appalling, but perhaps not surprising.

It breaks code which believed what was promised, and did return n
(n != 0) instead of exit(n).

If the standards people wanted to make that change, they should have
done it properly, and made it be void main() instead of int main().
That would have meant changing the universe, but at least it would have
provided an opportunity to examine code that might have been broken
by this change.

That also perhaps indicates why things weren't breaking (why no warning
was being issued from gcc/clang about the code that was in echo).
New versions of the compiler probably have been taught that main is
special, and it isn't necessary to return a value, even though it is
a function which is supposed to return one.Sad indeed.
The compile I did with -Wall that produced the error was with a
slightly older compiler version than is used in HEAD.

kre



Re: CVS commit: src/bin/echo

2021-11-10 Thread Valery Ushakov
On Wed, Nov 10, 2021 at 17:35:45 +, Robert Elz wrote:

> And second, does C99 really promise:
>   Remove unnecessary call to exit(0); returning from main is equivalent
>   since C99.
> in the sense that simply falling out of main() is exit(0)?

Surprisingly - yes.

Derek M. Jones in his "The New C Standard.  An Economic and Cultural
Commentary." notes about the relevant passage in the standard text:

  5.1.2.2.3 Program termination

  [...]
  reaching the } that terminates the main function returns a value of 0.

  Commentary
  The standard finally having to bow to sloppy existing practices.

I'd say the previous change to take advantage of this was profoudly
misguided.

-uwe


Re: CVS commit: src/sys/arch/arm/sa11x0

2021-11-08 Thread Rin Okuyama

On 2021/11/09 8:57, Rin Okuyama wrote:

Module Name:src
Committed By:   rin
Date:   Mon Nov  8 23:57:23 UTC 2021

Modified Files:
src/sys/arch/arm/sa11x0: sa11x0_irq.S

Log Message:
irq_entry(): Do not clobber fp (= r11), in order not to confuse DDB.


(snip)


XXX
Rewrite this function by C. There seems no particular reason to
use assembler, and no major performance regression is expected.


No reason to use assembler, if converted to use arm/arm32/irq_dispatch.S.

Thanks,
rin


Re: CVS commit: src

2021-11-07 Thread Robert Elz
Date:Sat, 6 Nov 2021 10:40:58 -0700
From:Alistair Crooks 
Message-ID:  


  | The author of the software made a conscious decision  | to make the 
variable unsigned, sincr the length
  | would never be less than zero.
  |
  | The author then made a default definition for the
  | lower bound of the length, and made it 0.

Both of those are readonable.

  | I find it ironic that an overbearing,
  | over-eager compiler takes these,
  | decides that the condition could never be true,

I have not looked at the code, but this suggests
that with the decisions above, the code is then
going and checking if the impossible is true.
"since the length would never be less than 0"
the data type cannot store valuse less than 0,
so testing if such a value is less than 0 is
stupid, and proobabky indicates a logic error.

  | and so a cast to an integer type is now needed
  | to shut up the overeager compiler,

I agree with your implication, doing that would
be insane, and potentially break correct code,
if the var was ever > INT_MAX and <= UINT_MAX
then the test with the cast would indicate an
invalid length, which, perhaps, it is not.

The right thing to do is to delete the meaningless
test.

If you're concerned that the assumptions/decisions
above might one day be altered, then instead of
deleting the test, put it in a

#if MIN_VALUE != 0
#endif

block instead.

So:
  | Nevertheless, I'll make the changes you suggest

don't do that.

kre


Re: CVS commit: src

2021-11-07 Thread Alistair Crooks
On Fri, 5 Nov 2021 at 16:45, Joerg Sonnenberger  wrote:

> On Thu, Nov 04, 2021 at 06:17:20PM -0700, Alistair Crooks wrote:
> > I think you're misreading the diff - it will only wrap if the minimum
> size
> > is 0x, which is...ummm...highly unlikely (it's defined to be 0
> > right now, the type is unsigned)
>
> I'm not so much worried about the constant, but the right hand size.
> Without looking at the types, I can't be sure that "context->pwdlen + 1"
> doesn't overflow or that "context->m_cost - 1" can't underflow. Much
> easier and safer to just use a type cast...
>

The author of the software made a conscious decision to make the variable
unsigned, since the length would never be less than zero.

The author then made a default definition for the lower bound of the
length, and made it 0.

I find it ironic that an overbearing, over-eager compiler takes these,
decides that the condition could never be true, and so a cast to an integer
type is now needed to shut up the overeager compiler, thereby negating the
(completely valid) thought process the author went through.

Nevertheless, I'll make the changes you suggest


Re: CVS commit: src

2021-11-05 Thread Joerg Sonnenberger
On Thu, Nov 04, 2021 at 06:17:20PM -0700, Alistair Crooks wrote:
> I think you're misreading the diff - it will only wrap if the minimum size
> is 0x, which is...ummm...highly unlikely (it's defined to be 0
> right now, the type is unsigned)

I'm not so much worried about the constant, but the right hand size.
Without looking at the types, I can't be sure that "context->pwdlen + 1"
doesn't overflow or that "context->m_cost - 1" can't underflow. Much
easier and safer to just use a type cast...

Joerg


Re: CVS commit: src

2021-11-05 Thread Alistair Crooks
I think you're misreading the diff - it will only wrap if the minimum size
is 0x, which is...ummm...highly unlikely (it's defined to be 0
right now, the type is unsigned)

On Thu, 4 Nov 2021 at 15:56, Joerg Sonnenberger  wrote:

> On Mon, Nov 01, 2021 at 03:09:59AM +, Alistair G. Crooks wrote:
> > Module Name:  src
> > Committed By: agc
> > Date: Mon Nov  1 03:09:59 UTC 2021
> >
> > Modified Files:
> >   src/external/apache2/argon2/dist/phc-winner-argon2/src: argon2.c
> core.c
> >   src/lib/libcrypt: Makefile
> >
> > Log Message:
> > Remove the
> >
> >   COPTS.*+=   -Wno-error=.*
> >
> > lines for building argon2 sources, by fixing the problems at source.
> >
> > Addresses Rin Okuyama's concerns on tech-userlevel/tech-crypto in
> >
> >   Message-ID: 
>
> The +1 part seems questionable to me as it can actually introduce a wrap
> around, can't it?
>
> Joerg
>
>


Re: CVS commit: src

2021-11-04 Thread Joerg Sonnenberger
On Mon, Nov 01, 2021 at 03:09:59AM +, Alistair G. Crooks wrote:
> Module Name:  src
> Committed By: agc
> Date: Mon Nov  1 03:09:59 UTC 2021
> 
> Modified Files:
>   src/external/apache2/argon2/dist/phc-winner-argon2/src: argon2.c core.c
>   src/lib/libcrypt: Makefile
> 
> Log Message:
> Remove the
> 
>   COPTS.*+=   -Wno-error=.*
> 
> lines for building argon2 sources, by fixing the problems at source.
> 
> Addresses Rin Okuyama's concerns on tech-userlevel/tech-crypto in
> 
>   Message-ID: 

The +1 part seems questionable to me as it can actually introduce a wrap
around, can't it?

Joerg


Re: CVS commit: src

2021-11-04 Thread Rin Okuyama

On 2021/11/01 12:09, Alistair G. Crooks wrote:

Module Name:src
Committed By:   agc
Date:   Mon Nov  1 03:09:59 UTC 2021

Modified Files:
src/external/apache2/argon2/dist/phc-winner-argon2/src: argon2.c core.c
src/lib/libcrypt: Makefile

Log Message:
Remove the

COPTS.*+=   -Wno-error=.*

lines for building argon2 sources, by fixing the problems at source.

Addresses Rin Okuyama's concerns on tech-userlevel/tech-crypto in

Message-ID: 


Thanks! It got clearer!

rin


Re: CVS commit: src/sys/arch

2021-10-31 Thread Tobias Nygren
On Sun, 31 Oct 2021 16:23:48 +
Nick Hudson  wrote:

> Modified Files:
>   src/sys/arch/aarch64/include: cpu.h cpufunc.h db_machdep.h
...
> Log Message:
> Rework Arm (32bit and 64bit) AP startup so that cpu_hatch doesn't sleep.

Hi,

I'm afraid this broke the userland build.
I think db_machdep_init(...) should move to the ifdef _KERNEL block?

   compile  crash/db_autoconf.o
In file included from /usr/src/../obj/usr.sbin/crash/machine/db_machdep.h:4,
 from /usr/src/usr.sbin/crash/../../sys/ddb/ddb.h:35,
 from /usr/src/usr.sbin/crash/../../sys/ddb/db_autoconf.c:39:
/usr/src/../obj/usr.sbin/crash/aarch64/db_machdep.h:231:29: error: 'struct 
cpu_info' declared inside parameter list will not be visible outside of this 
definition or declaration [-Werror]
  231 | void db_machdep_init(struct cpu_info * const);

-Tobias


Re: CVS commit: src/sys/dev/sbus

2021-10-30 Thread Ryo Shimizu


>Module Name:   src
>Committed By:  macallan
>Date:  Sat Oct 30 05:37:39 UTC 2021
>
>Modified Files:
>   src/sys/dev/sbus: mgx.c mgxreg.h
>
>Log Message:
>actually mmap() the blitter registers when asked to, while there do some
>magic number reduction
>
>
>To generate a diff of this commit:
>cvs rdiff -u -r1.17 -r1.18 src/sys/dev/sbus/mgx.c
>cvs rdiff -u -r1.5 -r1.6 src/sys/dev/sbus/mgxreg.h

@@ -684,6 +684,8 @@
uint32_t fg, bg;
int x, y, wi, he, rv;
 
+if (sc->sc_mode != WSDISPLAYIO_MODE_EMUL) return;
+
wi = font->fontwidth;
he = font->fontheight;
 
@@ -731,6 +733,8 @@
uint32_t fg, bg, scratch = ((sc->sc_stride * sc->sc_height) + 7) & ~7;
int x, y, wi, he, len, i;
 
+if (sc->sc_mode != WSDISPLAYIO_MODE_EMUL) return;
+
wi = font->fontwidth;
he = font->fontheight;
 

This seems to be a strange indent.
Or debugging code mixed in?

-- 
ryo shimizu


Re: CVS commit: src/usr.sbin/rpcbind

2021-10-30 Thread nia
On Sat, Oct 30, 2021 at 08:25:42PM +0900, Rin Okuyama wrote:
> We should not break from infinite loop here, even if svc_fdset_getmax()
> fails. Please restore the old behavior. And please be more careful before
> making such a non-trivial change.

Ah, thank you. I incorrectly assumed that "out" must be a label
at the end of a block. I've restored the behaviour and renamed
the label to "wait".


Re: CVS commit: src/usr.sbin/rpcbind

2021-10-30 Thread Rin Okuyama

On 2021/10/30 20:04, Nia Alarie wrote:

This function
previously tried to go to a label that doesn't exist. I wonder why
GCC didn't catch this before but does now.


This is not true. You deleted the label:


-   if (p == NULL) {
-out:
-   syslog(LOG_ERR, "Cannot allocate pollfds");
-   sleep(1);
-   continue;
-   }
if ((m = svc_fdset_getmax()) == NULL)
-   goto out;
+   break;


We should not break from infinite loop here, even if svc_fdset_getmax()
fails. Please restore the old behavior. And please be more careful before
making such a non-trivial change.

Thanks,
rin


Re: CVS commit: src

2021-10-29 Thread Roland Illig

Am 29.10.2021 um 22:40 schrieb Robert Elz:

 Date:Fri, 29 Oct 2021 17:50:38 +
 From:"Roland Illig" 
 Message-ID:  <20211029175038.33b08f...@cvs.netbsd.org>

   | Log Message:
   | indent: use prev/curr/next to refer to the current token
   |
   | The word 'last' just didn't match with 'next'.

That depends upon how it is used, last is one of those "flexible"
words which means different things, depending upon the context.


It's exactly this flexibility that I wanted to eliminate from the code.

For example, the function process_command has the variable last_blank
that remembers the index of the last (rightmost) seen blank in a line.
In that case, "previous" and "last seen" and "rightmost" are equivalent,
which is OK in that context.

To avoid confusion in the parser_state, I wanted to use a different word
to specify the directly adjacent item, and prev/curr/next just seemed to
be a good fit in a situation of iterating over tokens, to avoid the
ambiguity of first/last and last/next. The code is still complicated
enough that I welcome every little bit of disambiguation that I can get.

Roland


Re: CVS commit: src

2021-10-29 Thread Robert Elz
Date:Fri, 29 Oct 2021 17:50:38 +
From:"Roland Illig" 
Message-ID:  <20211029175038.33b08f...@cvs.netbsd.org>

  | Log Message:
  | indent: use prev/curr/next to refer to the current token
  |
  | The word 'last' just didn't match with 'next'.

That depends upon how it is used, last is one of those "flexible"
words which means different things, depending upon the context.

Eg: "last night" does not mean the night before doomsday, it means
the night that just passed (the previous night if you like, though
no-one would say that with that meaning).

With that usage, last and next fit together just fine, today is
Saturday Oct 30, where I am (my timezone), last Thursday was Oct 28,
next Thursday is Nov 4.   Perfectly clear, and normal usages.

On the other hand the "last apple" usually means there are none
left after this one, but this one can be ambiguous, and more
context is needed.   "I want the last apple" has that meaning.
"The last apple tasted sweeter than the others" probably
has the "previous" meaning (though might now), though again,
I doubt anyone would use "previous" in that kind of sentence,
"last" is normal, whether it means "the one I just ate" (however
many are left), or "the one after which there were no more".
That difference is usually immaterial, as if there are no more
the last one eaten would be the last one...

Note: I am not objecting to the change, just that perhaps sometimes
all that is not clear to you might be just fine for others.

kre



Re: CVS commit: src/lib/libc/string

2021-10-29 Thread Joerg Sonnenberger
On Fri, Oct 29, 2021 at 10:11:57AM +, Nia Alarie wrote:
> Module Name:  src
> Committed By: nia
> Date: Fri Oct 29 10:11:57 UTC 2021
> 
> Modified Files:
>   src/lib/libc/string: wcsdup.c
> 
> Log Message:
> wcsdup(3): use reallocarr to catch integer overflow

Except that no such integer overflow can happen, since the input string
already has done the same computation.

Joerg


Re: CVS commit: src

2021-10-26 Thread Ryo Shimizu


>>   | I don't think per-file profiling is that useful
>> 
>> Nor do I in general.   However, doing routine call counting on mcount
>> (or anything it calls) would simply be absurd.   If you want to
>> know how mant times mcount has been called, you simply sum all the other
>> counters.   (pc sampling, however it is done, detecting mcount is fine).
>
>The existing attribute already does that.

Hmmm, got it.
I'll revert my commit, and fix it to add "__always_inline" to functions
called from mcount().

Thanks,
-- 
ryo shimizu


Re: CVS commit: src/sbin/wsconsctl

2021-10-26 Thread Rin Okuyama

Thanks for the quick response!

rin

On 2021/10/27 2:35, Roland Illig wrote:

Am 26.10.2021 um 06:13 schrieb matthew green:

Rin Okuyama writes:

I don't think this is right thing to be committed.


i agree.


Thank you for the explanation, I just reverted the commit, resulting in
sbin/wsconsctl/Makefile 1.16.

Roland



Re: CVS commit: src

2021-10-26 Thread Joerg Sonnenberger
On Tue, Oct 26, 2021 at 10:48:45PM +0700, Robert Elz wrote:
> Date:Tue, 26 Oct 2021 15:37:56 +0200
> From:Joerg Sonnenberger 
> Message-ID:  
> 
>   | Personally, I would prefer to just kill -pg support completely, but
>   | that's a separate discussion.
> 
> Yes, it is.
> 
>   | I don't think per-file profiling is that useful
> 
> Nor do I in general.   However, doing routine call counting on mcount
> (or anything it calls) would simply be absurd.   If you want to
> know how mant times mcount has been called, you simply sum all the other
> counters.   (pc sampling, however it is done, detecting mcount is fine).

The existing attribute already does that.

Joerg


Re: CVS commit: src/bin/sh

2021-10-26 Thread Christos Zoulas
In article <20211026000538.893d6f...@cvs.netbsd.org>,
Robert Elz  wrote:
>-=-=-=-=-=-
>
>+  waspriv = privileged = (uid != geteuid()) || (gid != getegid());
>+

No issetugid()?

christos



Re: CVS commit: src/sbin/wsconsctl

2021-10-26 Thread Roland Illig
Am 26.10.2021 um 06:13 schrieb matthew green:
> Rin Okuyama writes:
>> I don't think this is right thing to be committed.
>
> i agree.

Thank you for the explanation, I just reverted the commit, resulting in
sbin/wsconsctl/Makefile 1.16.

Roland


Re: CVS commit: src/bin/sh

2021-10-26 Thread Robert Elz
ps: Also, the code is (while refactored a little) essentially the same
as  the -p code you added in 2015 ... just now avoiding repeatedly
calling geteuid() (etc) - the value it returns won't change unless the
code does something to change it.

kre



Re: CVS commit: src

2021-10-26 Thread Robert Elz
Date:Tue, 26 Oct 2021 15:37:56 +0200
From:Joerg Sonnenberger 
Message-ID:  

  | Personally, I would prefer to just kill -pg support completely, but
  | that's a separate discussion.

Yes, it is.

  | I don't think per-file profiling is that useful

Nor do I in general.   However, doing routine call counting on mcount
(or anything it calls) would simply be absurd.   If you want to
know how mant times mcount has been called, you simply sum all the other
counters.   (pc sampling, however it is done, detecting mcount is fine).

I agree that forcing inlining is a good idea, and should be done, but
I also feel that disabling profiling of mcount is also the right thing
to do.   I don't think we need a general ability to disable profiling on
any random file/function but if that is the easiest and cleanest way to
be able to disable profiling of mcount then I can accept it - provided
that's all it is actually used for,

kre



Re: CVS commit: src/bin/sh

2021-10-26 Thread Robert Elz
Date:Tue, 26 Oct 2021 15:07:23 - (UTC)
From:chris...@astron.com (Christos Zoulas)
Message-ID:  

  | No issetugid()?

No, because I'm not sure I understand that, nor that I believe:

   A process is tainted if [...] it has changed any of its real,
   effective or saved user or group ID's since it began execution.

nor how that would apply to sh.   Further:

   This system call exists so that library routines (e.g., libc)

where perhaps it might be useful (I really have no idea) but that's
not the situation of concern.

What matters is if a sh is run from a set[ug]id process (or stranger perhaps,
someone makes a copy of sh set[ug]id) that, at least until someone says
"this is safe now" it doesn't do anything too dangerous.   Just checking
the uid/gid against euid/egid seems adequate for that.

kre



Re: CVS commit: src

2021-10-26 Thread Joerg Sonnenberger
On Tue, Oct 26, 2021 at 04:00:41AM +0900, Ryo Shimizu wrote:
> 
> >On Mon, Oct 25, 2021 at 07:54:45AM +, Ryo Shimizu wrote:
> >> Module Name:   src
> >> Committed By:  ryo
> >> Date:  Mon Oct 25 07:54:44 UTC 2021
> >> 
> >> Modified Files:
> >>src/share/mk: bsd.README bsd.lib.mk
> >>src/sys/conf: Makefile.kern.inc
> >>src/sys/lib/libkern: Makefile.libkern
> >> 
> >> Log Message:
> >> In some arch, _mcount() would be called recursively when built with 
> >> COPTS=-O0.
> >> 
> >> Normally, functions called from mcount.c are expected to be expanded 
> >> inline,
> >> so _mcount() will never be called recursively. But when build with 
> >> COPTS=-O0,
> >> `static inline' functions aren't inlined, and _mcount() will be called
> >> recursively.
> >
> >So why not fix that by actually using always_inline (i.e.
> >__always_inline)?
> >
> >Joerg
> 
> Yes, that is correct. That method is also valid and should be done separately.
> 
> However, it is more direct to not add -pg to mcount.c.
> Also, I think this commit is valid because it is useful to be able to choose
> not to do per-file profiling.

Personally, I would prefer to just kill -pg support completely, but
that's a separate discussion. I don't think per-file profiling is that
useful and just begging for very confusing results. So I would strongly
prefer if this was done properly.

Joerg


re: CVS commit: src/sbin/wsconsctl

2021-10-25 Thread matthew green
Rin Okuyama writes:
> I don't think this is right thing to be committed.

i agree.

> By this way, we can add similar LINTFLAGS hacks every when
> kernel API is changed.
>
> Please keep this kinds of hacks within your local repository.
>
> > Running lint in non-tools mode picked up the header from the installed
> > system, not the one corresponding to the source code. The installed
> > header on NetBSD 9.99.88 does not define WSMOUSECFG_MAX yet.

i would go far to say that the above behaviour is 100%
_expected_.  if you're building in non tools mode then you
are using the installed system, and not using it would be
not how this has always worked.  this is why, whether you're
using build.sh or not, "make includes" early is a necessary
pre-condition for building.

> Thanks,
> rin


.mrg.

> On 2021/10/11 22:27, Roland Illig wrote:
> > Module Name:src
> > Committed By:   rillig
> > Date:   Mon Oct 11 13:27:47 UTC 2021
> > 
> > Modified Files:
> > src/sbin/wsconsctl: Makefile
> > 
> > Log Message:
> > wsconsctl: include correct header for lint
> > 
> > Running lint in non-tools mode picked up the header from the installed
> > system, not the one corresponding to the source code. The installed
> > header on NetBSD 9.99.88 does not define WSMOUSECFG_MAX yet.


Re: CVS commit: src/sbin/wsconsctl

2021-10-25 Thread Rin Okuyama

I don't think this is right thing to be committed.

By this way, we can add similar LINTFLAGS hacks every when
kernel API is changed.

Please keep this kinds of hacks within your local repository.

Thanks,
rin

On 2021/10/11 22:27, Roland Illig wrote:

Module Name:src
Committed By:   rillig
Date:   Mon Oct 11 13:27:47 UTC 2021

Modified Files:
src/sbin/wsconsctl: Makefile

Log Message:
wsconsctl: include correct header for lint

Running lint in non-tools mode picked up the header from the installed
system, not the one corresponding to the source code. The installed
header on NetBSD 9.99.88 does not define WSMOUSECFG_MAX yet.


To generate a diff of this commit:
cvs rdiff -u -r1.14 -r1.15 src/sbin/wsconsctl/Makefile

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


Re: CVS commit: src

2021-10-25 Thread Ryo Shimizu


>On Mon, Oct 25, 2021 at 07:54:45AM +, Ryo Shimizu wrote:
>> Module Name: src
>> Committed By:ryo
>> Date:Mon Oct 25 07:54:44 UTC 2021
>> 
>> Modified Files:
>>  src/share/mk: bsd.README bsd.lib.mk
>>  src/sys/conf: Makefile.kern.inc
>>  src/sys/lib/libkern: Makefile.libkern
>> 
>> Log Message:
>> In some arch, _mcount() would be called recursively when built with 
>> COPTS=-O0.
>> 
>> Normally, functions called from mcount.c are expected to be expanded inline,
>> so _mcount() will never be called recursively. But when build with COPTS=-O0,
>> `static inline' functions aren't inlined, and _mcount() will be called
>> recursively.
>
>So why not fix that by actually using always_inline (i.e.
>__always_inline)?
>
>Joerg

Yes, that is correct. That method is also valid and should be done separately.

However, it is more direct to not add -pg to mcount.c.
Also, I think this commit is valid because it is useful to be able to choose
not to do per-file profiling.

Thanks,
-- 
ryo shimizu


Re: CVS commit: src

2021-10-25 Thread Joerg Sonnenberger
On Mon, Oct 25, 2021 at 07:54:45AM +, Ryo Shimizu wrote:
> Module Name:  src
> Committed By: ryo
> Date: Mon Oct 25 07:54:44 UTC 2021
> 
> Modified Files:
>   src/share/mk: bsd.README bsd.lib.mk
>   src/sys/conf: Makefile.kern.inc
>   src/sys/lib/libkern: Makefile.libkern
> 
> Log Message:
> In some arch, _mcount() would be called recursively when built with COPTS=-O0.
> 
> Normally, functions called from mcount.c are expected to be expanded inline,
> so _mcount() will never be called recursively. But when build with COPTS=-O0,
> `static inline' functions aren't inlined, and _mcount() will be called
> recursively.

So why not fix that by actually using always_inline (i.e.
__always_inline)?

Joerg


Re: CVS commit: src/sys/dev/pci

2021-10-21 Thread Jonathan A. Kollasch
On Thu, Oct 21, 2021 at 05:32:28AM +, Shoichi YAMAGUCHI wrote:
> Module Name:  src
> Committed By: yamaguchi
> Date: Thu Oct 21 05:32:27 UTC 2021
> 
> Modified Files:
>   src/sys/dev/pci: virtio.c virtio_pci.c virtiovar.h
> 
> Log Message:
> divide setup routine of virtio interrupts
> into establishment and device configuration
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.49 -r1.50 src/sys/dev/pci/virtio.c
> cvs rdiff -u -r1.30 -r1.31 src/sys/dev/pci/virtio_pci.c
> cvs rdiff -u -r1.20 -r1.21 src/sys/dev/pci/virtiovar.h

This seems to have broken the virtio_mmio build.


Re: CVS commit: src/sys/dev/pci

2021-10-18 Thread Kengo NAKAHARA

Hi,

Thank you for your quick commit!


Thanks,

On 2021/10/18 17:15, Juergen Hannken-Illjes wrote:

Module Name:src
Committed By:   hannken
Date:   Mon Oct 18 08:15:00 UTC 2021

Modified Files:
src/sys/dev/pci: xmm7360.c

Log Message:
Use a local static variable to hold "pktq_rps_hash_default"
like the other devices do.

Kernel ALL/amd64 compiles again.

OK: Kengo NAKAHARA 


To generate a diff of this commit:
cvs rdiff -u -r1.12 -r1.13 src/sys/dev/pci/xmm7360.c

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



--
//
Internet Initiative Japan Inc.

Device Engineering Section,
Product Division,
Technology Unit

Kengo NAKAHARA 




Re: CVS commit: src/sys/arch/x86/x86

2021-10-15 Thread Paul Goyette

hehehe - porn iterators - love it!


On Fri, 15 Oct 2021, Jason Thorpe wrote:


I demand this change be reverted.

(/s)


On Oct 15, 2021, at 11:12 AM, Jared D. McNeill  wrote:

Module Name:src
Committed By:   jmcneill
Date:   Fri Oct 15 18:12:48 UTC 2021

Modified Files:
src/sys/arch/x86/x86: tsc.c

Log Message:
Fix typo in comment: "porniters" -> "pointers"


To generate a diff of this commit:
cvs rdiff -u -r1.56 -r1.57 src/sys/arch/x86/x86/tsc.c

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



-- thorpej


!DSPAM:6169cf82254421105921466!




++--+--+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses:|
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com|
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org  |
| & Network Engineer |  | pgoyett...@gmail.com |
++--+--+


Re: CVS commit: src/sys/arch/x86/x86

2021-10-15 Thread Jason Thorpe
I demand this change be reverted.

(/s)

> On Oct 15, 2021, at 11:12 AM, Jared D. McNeill  wrote:
> 
> Module Name:  src
> Committed By: jmcneill
> Date: Fri Oct 15 18:12:48 UTC 2021
> 
> Modified Files:
>   src/sys/arch/x86/x86: tsc.c
> 
> Log Message:
> Fix typo in comment: "porniters" -> "pointers"
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.56 -r1.57 src/sys/arch/x86/x86/tsc.c
> 
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
> 

-- thorpej



Re: CVS commit: src/sys/dev/wscons

2021-10-11 Thread nia
On Sun, Oct 10, 2021 at 10:44:03PM +0900, Izumi Tsutsui wrote:
> > Module Name:src
> > Committed By:   nia
> > Date:   Tue Sep 28 06:14:28 UTC 2021
> > 
> > Modified Files:
> > src/sys/dev/wscons: wsconsio.h wsmouse.c wsmousevar.h
> > 
> > Log Message:
> > wsmouse: add support for "precision scrolling" events and (GET|SET)PARAMS
> 
> Could you please also update wsmouse(4) and wsmouse(9) man pages?
> Even the current version lacks various info (especially about ioctl(2))
> but no reason to make it worse.
> 
> Thanks,
> ---
> Izumi Tsutsui

Sure, I had been meaning to anyway but I clearly forgot to commit those
files...


Re: CVS commit: src/sys/arch

2021-10-11 Thread Nick Hudson




On 11/10/2021 08:14, Rin Okuyama wrote:

Hi,

On 2021/09/23 15:34, Nick Hudson wrote:

Module Name:    src
Committed By:    skrll
Date:    Thu Sep 23 06:34:00 UTC 2021

Modified Files:
src/sys/arch/aarch64/aarch64: cpufunc.c
src/sys/arch/arm/arm32: cpu.c

Log Message:
Print the cache information in similar formats and arm and aarch64, e.g.


For classic ARM CPUs, info->[id]cache_sets are not set. This results in

cpu0 at mainbus0 core 0: SA-1110 step B-5 (SA-1 V4 core)
cpu0: DC enabled IC enabled WB enabled LABT
cpu0: L1 16KB/32B 32-way (0 set) VIVT Instruction cache
cpu0: L1 8KB/32B 32-way (0 set) write-back VIVT Data cache

or

cpu0 at mainbus0 core 0: ARM926EJ-S r0p0 (ARM9EJ-S V5TEJ core)
cpu0: DC enabled IC enabled WB enabled LABT
cpu0: L1 32KB/32B 1-way (0 set) VIVT Instruction cache
cpu0: L1 32KB/32B 1-way (0 set) write-back-locking-C VIVT Data cache

Can I commit the attached patch? Or initialize these variables somewhere
else?


I think your patch is fine. Thanks for fixing this.

Nick


Re: CVS commit: src/sys/arch

2021-10-11 Thread Rin Okuyama

Thanks for quick response. Committed :)

rin

On 2021/10/11 16:27, Nick Hudson wrote:



On 11/10/2021 08:14, Rin Okuyama wrote:

Hi,

On 2021/09/23 15:34, Nick Hudson wrote:

Module Name:    src
Committed By:    skrll
Date:    Thu Sep 23 06:34:00 UTC 2021

Modified Files:
src/sys/arch/aarch64/aarch64: cpufunc.c
src/sys/arch/arm/arm32: cpu.c

Log Message:
Print the cache information in similar formats and arm and aarch64, e.g.


For classic ARM CPUs, info->[id]cache_sets are not set. This results in

cpu0 at mainbus0 core 0: SA-1110 step B-5 (SA-1 V4 core)
cpu0: DC enabled IC enabled WB enabled LABT
cpu0: L1 16KB/32B 32-way (0 set) VIVT Instruction cache
cpu0: L1 8KB/32B 32-way (0 set) write-back VIVT Data cache

or

cpu0 at mainbus0 core 0: ARM926EJ-S r0p0 (ARM9EJ-S V5TEJ core)
cpu0: DC enabled IC enabled WB enabled LABT
cpu0: L1 32KB/32B 1-way (0 set) VIVT Instruction cache
cpu0: L1 32KB/32B 1-way (0 set) write-back-locking-C VIVT Data cache

Can I commit the attached patch? Or initialize these variables somewhere
else?


I think your patch is fine. Thanks for fixing this.

Nick


Re: CVS commit: src/sys/arch

2021-10-11 Thread Rin Okuyama

Hi,

On 2021/09/23 15:34, Nick Hudson wrote:

Module Name:src
Committed By:   skrll
Date:   Thu Sep 23 06:34:00 UTC 2021

Modified Files:
src/sys/arch/aarch64/aarch64: cpufunc.c
src/sys/arch/arm/arm32: cpu.c

Log Message:
Print the cache information in similar formats and arm and aarch64, e.g.


For classic ARM CPUs, info->[id]cache_sets are not set. This results in

cpu0 at mainbus0 core 0: SA-1110 step B-5 (SA-1 V4 core)
cpu0: DC enabled IC enabled WB enabled LABT
cpu0: L1 16KB/32B 32-way (0 set) VIVT Instruction cache
cpu0: L1 8KB/32B 32-way (0 set) write-back VIVT Data cache

or

cpu0 at mainbus0 core 0: ARM926EJ-S r0p0 (ARM9EJ-S V5TEJ core)
cpu0: DC enabled IC enabled WB enabled LABT
cpu0: L1 32KB/32B 1-way (0 set) VIVT Instruction cache
cpu0: L1 32KB/32B 1-way (0 set) write-back-locking-C VIVT Data cache

Can I commit the attached patch? Or initialize these variables somewhere else?

Thanks,
rin
Index: sys/arch/arm/arm32/cpu.c
===
RCS file: /home/netbsd/src/sys/arch/arm/arm32/cpu.c,v
retrieving revision 1.149
diff -p -u -r1.149 cpu.c
--- sys/arch/arm/arm32/cpu.c23 Sep 2021 06:34:00 -  1.149
+++ sys/arch/arm/arm32/cpu.c8 Oct 2021 00:11:34 -
@@ -612,7 +612,9 @@ print_cache_info(device_t dv, struct arm
level + 1,
info->dcache_size / 1024,
info->dcache_line_size, info->dcache_ways,
-   info->dcache_sets,
+   info->dcache_sets ? info->dcache_sets :
+   info->dcache_size /
+   (info->dcache_line_size * info->dcache_ways),
wtnames[info->cache_type],
info->dcache_type & CACHE_TYPE_PIxx ? 'P' : 'V',
info->dcache_type & CACHE_TYPE_xxPT ? 'P' : 'V');
@@ -621,14 +623,18 @@ print_cache_info(device_t dv, struct arm
level + 1,
info->icache_size / 1024,
info->icache_line_size, info->icache_ways,
-   info->icache_sets,
+   info->icache_sets ? info->icache_sets :
+   info->icache_size /
+   (info->icache_line_size * info->icache_ways),
info->icache_type & CACHE_TYPE_PIxx ? 'P' : 'V',
info->icache_type & CACHE_TYPE_xxPT ? 'P' : 'V');
aprint_normal_dev(dv, "L%u %dKB/%dB %d-way (%u set) %s %cI%cT 
Data cache\n",
level + 1,
info->dcache_size / 1024,
info->dcache_line_size, info->dcache_ways,
-   info->dcache_sets,
+   info->dcache_sets ? info->dcache_sets :
+   info->dcache_size /
+   (info->dcache_line_size * info->dcache_ways),
wtnames[info->cache_type],
info->dcache_type & CACHE_TYPE_PIxx ? 'P' : 'V',
info->dcache_type & CACHE_TYPE_xxPT ? 'P' : 'V');


Re: CVS commit: src/sys/dev/wscons

2021-10-10 Thread Izumi Tsutsui
> Module Name:  src
> Committed By: nia
> Date: Tue Sep 28 06:14:28 UTC 2021
> 
> Modified Files:
>   src/sys/dev/wscons: wsconsio.h wsmouse.c wsmousevar.h
> 
> Log Message:
> wsmouse: add support for "precision scrolling" events and (GET|SET)PARAMS

Could you please also update wsmouse(4) and wsmouse(9) man pages?
Even the current version lacks various info (especially about ioctl(2))
but no reason to make it worse.

Thanks,
---
Izumi Tsutsui


Re: CVS commit: src/bin/sh

2021-10-10 Thread Tom Ivar Helbekkmo
Roland Illig  writes:

> Anyway, the code in question was more verbose than necessary, so I made
> it simpler. I also experimented with replacing the switch with a single
> if statement, but that would have become too dense and thus difficult to
> decipher.

Yes, the code in exec.c looks much better now.  The do .. while (0)
construct in jobs.c, however, looks like it could be improved by
removing the 'do' and 'while (0)' lines altogether, including the
misleading comment on the 'do' line.  That would also pull the goto
target 'out' one level up, making its context less confusing.

-tih
-- 
Most people who graduate with CS degrees don't understand the significance
of Lisp.  Lisp is the most important idea in computer science.  --Alan Kay


Re: CVS commit: src/bin/sh

2021-10-10 Thread Roland Illig
Am 10.10.2021 um 08:40 schrieb Tom Ivar Helbekkmo:
> Roland Illig  writes:
>
>> sh: ignore lint error about 'continue' in 'do while' loop
>>
>> exec.c(575): error: continue in 'do ... while (0)' loop [323]
>> jobs.c(203): error: continue in 'do ... while (0)' loop [323]
>>
>> It is certainly a rarely used feature, I saw it the first time today
>> and I don't see why a 'continue' statement in a 'do while' loop should
>> be an error.
>
> The only reason I can think of for using a 'do ... while (0)' construct
> outside of a '#define' is to have a block that's executed once, but that
> you can break out of without having to use 'goto'.  When you do this,
> the keywords 'break' and 'continue' both break out of the block.  I
> guess what lint is saying here is that because of this, you should use
> 'break', and not 'continue', to make it explicit what's happening.

Perfect guess, that's exactly the reason, as stated in
https://github.com/NetBSD/src/commit/d4d6980a4bb5db5188b919db from
2008-07-25.

At that time, the test suite for lint didn't contain a test for a
do-while-0 loop containing a switch statement containing a continue
statement, and this case where continue is actually different from break
was probably not on the author's mind.

I had not looked up the history of the message yesterday since I assumed
this error message had been there from the beginning. I should have
known the message ID 323 better since that looks really high. Oh well,
on a second look it's not that high either since even in 1995 there were
already the messages from 0 to 309.

Anyway, the code in question was more verbose than necessary, so I made
it simpler. I also experimented with replacing the switch with a single
if statement, but that would have become too dense and thus difficult to
decipher.

Roland


Re: CVS commit: src/bin/sh

2021-10-10 Thread Tom Ivar Helbekkmo
Roland Illig  writes:

> sh: ignore lint error about 'continue' in 'do while' loop
>
> exec.c(575): error: continue in 'do ... while (0)' loop [323]
> jobs.c(203): error: continue in 'do ... while (0)' loop [323]
>
> It is certainly a rarely used feature, I saw it the first time today
> and I don't see why a 'continue' statement in a 'do while' loop should
> be an error.

The only reason I can think of for using a 'do ... while (0)' construct
outside of a '#define' is to have a block that's executed once, but that
you can break out of without having to use 'goto'.  When you do this,
the keywords 'break' and 'continue' both break out of the block.  I
guess what lint is saying here is that because of this, you should use
'break', and not 'continue', to make it explicit what's happening.

-tih
-- 
Most people who graduate with CS degrees don't understand the significance
of Lisp.  Lisp is the most important idea in computer science.  --Alan Kay


Re: CVS commit: src/sys/arch/arm/arm

2021-10-07 Thread Rin Okuyama

Oops, forgot to mention: No binary changes.

On 2021/10/07 18:58, Rin Okuyama wrote:

Module Name:src
Committed By:   rin
Date:   Thu Oct  7 09:58:27 UTC 2021

Modified Files:
src/sys/arch/arm/arm: cpufunc_asm_armv5_ec.S

Log Message:
Reduce diff with cpufunc_asm_armv5.S, from which this file was derived.


To generate a diff of this commit:
cvs rdiff -u -r1.6 -r1.7 src/sys/arch/arm/arm/cpufunc_asm_armv5_ec.S

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


Re: CVS commit: src/sys

2021-10-06 Thread Taylor R Campbell
> Date: Wed, 6 Oct 2021 17:44:22 +0200
> From: Reinoud Zandijk 
> 
> Sorry for my ignorance but I thought that the ksyms list was static? ie
> read-only? Or is this change to support kernel modules symbols too and thus
> need rw access control?

The main kernel's symbols don't change, but the whole ksyms object
consists of the main kernel's symbol table and every module's symbol
table, which changes every time a module is loaded or unloaded.  This
has been the case ever since ksyms was first introduced in 2003.

Previously callers in the kernel getting at ksyms were required to
hold ksyms_lock, or have all other CPUs quiescent like in ddb, or,
realistically, just pray no modules get loaded or unloaded.  Now they
can do it reliably and scalably in a pserialize read section.


Re: CVS commit: src/sys

2021-10-06 Thread Reinoud Zandijk
On Wed, Sep 15, 2021 at 07:58:20PM +0900, Rin Okuyama wrote:
> On 2021/09/11 19:09, Taylor R Campbell wrote:
> > Module Name:src
> > Committed By:   riastradh
> > Date:   Sat Sep 11 10:09:55 UTC 2021
> > 
> > Modified Files:
> > src/sys/arch/sparc64/sparc64: machdep.c
> > src/sys/kern: kern_ksyms.c subr_csan.c subr_msan.c
> > src/sys/sys: ksyms.h
> > 
> > Log Message:
> > ksyms: Use pserialize(9) for kernel access to ksyms.

Sorry for my ignorance but I thought that the ksyms list was static? ie
read-only? Or is this change to support kernel modules symbols too and thus
need rw access control?

With regards,
Reinoud



Re: CVS commit: src/sys

2021-09-29 Thread Robert Elz
Date:Wed, 29 Sep 2021 08:42:12 -0700
From:Jason Thorpe 
Message-ID:  

  | Anything that depends on the new return value would have simply been
  | doing what the socket / fifo code was doing (groveling around in
  | selinfo internals), so it's not like they're broken as a result of
  | this change.

I agree with that.   The only potential issue I can see would be if
we had an architecture where the ABI specifies that the register normally
used to return a result is preserved across a call to a void function.

That would cause problems with this change - but absent any such architecture
there should be no issues requiring a bump.

kre



Re: CVS commit: src/sys

2021-09-29 Thread Jason Thorpe


> On Sep 29, 2021, at 8:15 AM, Robert Elz  wrote:
> 
>Date:Wed, 29 Sep 2021 05:37:44 -0700
>From:Jason Thorpe 
>Message-ID:  <39db6c46-94bf-4126-811b-466e5293b...@me.com>
> 
>  | Not needed in this case.
>  | No callers that need the return value are in a module.
> 
> The problem with this argument is that it assumes that you know
> every module that exists, anywhere.   It might be true of the
> modules distributed with NetBSD, without being true of some 3rd
> party module.

Anything that depends on the new return value would have simply been doing what 
the socket / fifo code was doing (groveling around in selinfo internals), so 
it's not like they're broken as a result of this change.

-- thorpej



Re: CVS commit: src/sys

2021-09-29 Thread Robert Elz
Date:Wed, 29 Sep 2021 05:37:44 -0700
From:Jason Thorpe 
Message-ID:  <39db6c46-94bf-4126-811b-466e5293b...@me.com>

  | Not needed in this case.
  | No callers that need the return value are in a module.

The problem with this argument is that it assumes that you know
every module that exists, anywhere.   It might be true of the
modules distributed with NetBSD, without being true of some 3rd
party module.

But for this particular change, I don't think I'd bother, now.

kre




Re: CVS commit: src/sys

2021-09-29 Thread Jason Thorpe


> On Sep 28, 2021, at 10:58 PM, matthew green  wrote:
> 
> "Jason R Thorpe" writes:
>> Module Name: src
>> Committed By:thorpej
>> Date:Wed Sep 29 02:47:22 UTC 2021
>> 
>> Modified Files:
>>  src/sys/kern: sys_select.c uipc_socket.c
>>  src/sys/miscfs/fifofs: fifo_vnops.c
>>  src/sys/sys: select.h
>> 
>> Log Message:
>> - Change selremove_knote() from returning void to bool, and return
>>  true if the last knote was removed and there are no more knotes
>>  on the selinfo.
>> - Use this new return value in filt_sordetach(), filt_sowdetach(),
>>  filt_fifordetach(), and filt_fifowdetach() to know when to clear
>>  SB_KOTE without having to know select/kqueue implementation details.
> 
> kernel bump?  it's certainly used in modules, but it's not
> clear that it would break anything..

Not needed in this case.  No callers that need the return value are in a module.

-- thorpej



re: CVS commit: src/sys

2021-09-28 Thread matthew green
"Jason R Thorpe" writes:
> Module Name:  src
> Committed By: thorpej
> Date: Wed Sep 29 02:47:22 UTC 2021
>
> Modified Files:
>   src/sys/kern: sys_select.c uipc_socket.c
>   src/sys/miscfs/fifofs: fifo_vnops.c
>   src/sys/sys: select.h
>
> Log Message:
> - Change selremove_knote() from returning void to bool, and return
>   true if the last knote was removed and there are no more knotes
>   on the selinfo.
> - Use this new return value in filt_sordetach(), filt_sowdetach(),
>   filt_fifordetach(), and filt_fifowdetach() to know when to clear
>   SB_KOTE without having to know select/kqueue implementation details.

kernel bump?  it's certainly used in modules, but it's not
clear that it would break anything..


.mrg.


Re: CVS commit: src/sys/arch

2021-09-23 Thread Izumi Tsutsui
> Module Name:  src
> Committed By: skrll
> Date: Thu Sep 23 06:34:00 UTC 2021
> 
> Modified Files:
>   src/sys/arch/aarch64/aarch64: cpufunc.c
>   src/sys/arch/arm/arm32: cpu.c
> 
> Log Message:
> Print the cache information in similar formats and arm and aarch64, e.g.
 :
> aarch64 before
> [   1.030] cpu1: L1 48KB/64B*256L*3W PIPT Instruction cache
> [   1.030] cpu1: L1 32KB/64B*256L*2W PIPT Data cache
> [   1.030] cpu1: L2 2048KB/64B*2048L*16W PIPT Unified cache
> 
> aarch64 after
> [   1.030] cpu1: L1 48KB/64B 3-way (256 set) PIPT Instruction cache
> [   1.030] cpu1: L1 32KB/64B 2-way (256 set) PIPT Data cache
> [   1.030] cpu1: L2 2048KB/64B 16-way (2048 set) PIPT Unified cache

I prefer "line(s)" as before rather than "set(s)" since
the latter implies "N-way set associative cache".

---
Izumi Tsutsui


Re: CVS commit: src/sys/kern

2021-09-22 Thread Rin Okuyama

On 2021/09/22 14:42, Rin Okuyama wrote:

Module Name:src
Committed By:   rin
Date:   Wed Sep 22 05:42:19 UTC 2021

Modified Files:
src/sys/kern: kern_ksyms.c

Log Message:
ksymsmmap: Add missing uao_reference(9) call for ks->ks_uobj.

Fix failure for savecore(8) and subsequent kernel panic, introduced to
kern_ksyms.c rev 1.03, at least for sh3 and alpha.


Oops, I meant rev 1.103 here.


For sh3 and alpha, savecore(8) supports coff and ecoff, respectively, via
libkvm via nlist(3). nlist(3) routines for coff and ecoff use mmap(2) and
munmap(2) for /dev/ksyms.

This munmap(2) decrements reference count for ks->ks_uobj. Unless it is
incremented in ksymsmmap(), ks->ks_uobj will be freed unexpectedly.


To generate a diff of this commit:
cvs rdiff -u -r1.104 -r1.105 src/sys/kern/kern_ksyms.c

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


re: CVS commit: src/external/gpl3/gcc/dist/gcc/config

2021-09-18 Thread matthew green
"Jared D. McNeill" writes:
> Module Name:  src
> Committed By: jmcneill
> Date: Sat Sep 18 10:45:11 UTC 2021
>
> Modified Files:
>   src/external/gpl3/gcc/dist/gcc/config: host-darwin.c
>   src/external/gpl3/gcc/dist/gcc/config/aarch64: aarch64.h
>
> Log Message:
> Fix build on macOS 11.6 arm64 hosts.
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.1.1.10 -r1.2 \
> src/external/gpl3/gcc/dist/gcc/config/host-darwin.c
> cvs rdiff -u -r1.1.1.13 -r1.2 \
> src/external/gpl3/gcc/dist/gcc/config/aarch64/aarch64.h

the 2nd change here likely broke netbsd, which also has
support for this.  it's probably fine to add || __NetBSD__
to this conditional since those are the only two ports
that support this feature.


.mrg.


Re: CVS commit: src/usr.sbin/acpitools/acpidump

2021-09-16 Thread Rin Okuyama

On 2021/09/15 8:30, Jason Thorpe wrote:




On Sep 14, 2021, at 1:34 PM, Roland Illig  wrote:

When lint runs on the code, it defines the preprocessor macro 'lint' to
be 1.  Due to that, this name cannot be used as a regular identifier.


Perhaps all of the "#ifdef lint" conditions should become "#ifdef __lint__"?


Agreed. lint should not pollute public namespace.

Thanks,
rin


Re: CVS commit: src/sys

2021-09-16 Thread Rin Okuyama

Hi,

For alpha, I've observed similar failure with rev 1.104; savecore fails and
panic occurs in pagedaemon after that.

Thanks,
rin

# /etc/rc.d/savecore onestart
Checking for core dump...
savecore: (null): kvm_nlist: bad namelist
savecore: (null): _dumpdev not in namelist
# dd if=/dev/zero of=/dev/null bs=2g
[ 727.7471213] CPU 0: fatal kernel trap:
^
[ 727.7946544] CPU 0trap entry = 0x2 (memory management fault)
[ 727.8653781] CPU 0a0 = 0x73
[ 727.9101025] CPU 0a1 = 0x1
[ 727.9537858] CPU 0a2 = 0x0
[ 727.9974700] CPU 0pc = 0xfca68898
[ 728.0567529] CPU 0ra = 0xfc00010b9598
[ 728.1160375] CPU 0pv = 0xfca68864
[ 728.1753220] CPU 0curlwp = 0xfc007f58e5c0
[ 728.2346053] CPU 0pid = 0, comm = system

[ 728.2907705] panic: trap
[ 728.3198911] cpu0: Begin traceback...
[ 728.3625359] alpha trace requires known PC =eject=
[ 728.4187019] cpu0: End traceback...
Stopped in pid 0.111 (system) atnetbsd:cpu_Debugger+0x4:ret
zero,(ra)
db{0}> bt
cpu_Debugger() at netbsd:cpu_Debugger+0x4
db_panic() at netbsd:db_panic+0xc8
vpanic() at netbsd:vpanic+0x108
panic() at netbsd:panic+0x58
trap() at netbsd:trap+0xb00
XentMM() at netbsd:XentMM+0x20
--- memory management fault (from ipl 0) ---
rw_tryenter() at netbsd:rw_tryenter+0x34
uvmpd_trylockowner() at netbsd:uvmpd_trylockowner+0x68
uvmpdpol_balancequeue() at netbsd:uvmpdpol_balancequeue+0x1b4
uvm_pageout() at netbsd:uvm_pageout+0x714
--- kernel thread backstop ---
db{0}>

On 2021/09/15 22:12, Rin Okuyama wrote:

On 2021/09/15 21:55, Taylor R Campbell wrote:

Date: Wed, 15 Sep 2021 19:58:20 +0900
From: Rin Okuyama 

login: [  95.5000696] panic: kernel diagnostic assertion "slock != NULL" failed: file 
"../../../../uvm/uvm_pdaemon.c", line 398 pg 0x8c66bd88 uobj 0x8fa7e400, NULL lock
[...]

It seems that I can avoid this panic if

- revert kern_ksyms.c to rev 1.98 (*), *or*
- set savecore=NO in rc.conf.

(*) I've not tested revs 1.99--1.103.

Can you please take a look into?


Interesting.  I'm not sure what's going on here.  The uvm panic looks
like a compounded symptom rather than the original problem.

Can you try booting with savecore=NO and see if `cat /dev/ksyms' or
`nm /dev/ksyms' works?

Can you also try rev. 1.103 and see if that makes a difference, just
to narrow it down?  (No point in trying the other revisions.  Unlikely
that 1.103 will work if 1.104 doesn't, but will help to confidently
narrow it down.)


'nm /dev/ksyms' works fine for 1.104. Also, panic does not occur after that,
as far as I can see.

For 1.103, savecore does not work and panic occurs during multiuser boot with
savecore=YES in the same manner as for 1.104. On the other hand,
'nm /dev/ksyms' works fine.

For 1.102 (== 1.98), whereas savecore works, nm complains as:


# nm /dev/ksyms
nm: warning: /dev/ksyms has a corrupt section with a size (18) larger than the 
file size
nm: warning: /dev/ksyms has a corrupt section with a size (4a560) larger than 
the file size
nm: warning: /dev/ksyms has a corrupt section with a size (46de5) larger than 
the file size
nm: warning: /dev/ksyms has a corrupt section with a size (40) larger than the 
file size
8c38fec8 t C.7
8c3e5b08 t CSWTCH.118
...


Thanks,
rin


Re: CVS commit: src/sys

2021-09-15 Thread Rin Okuyama

On 2021/09/15 21:55, Taylor R Campbell wrote:

Date: Wed, 15 Sep 2021 19:58:20 +0900
From: Rin Okuyama 

login: [  95.5000696] panic: kernel diagnostic assertion "slock != NULL" failed: file 
"../../../../uvm/uvm_pdaemon.c", line 398 pg 0x8c66bd88 uobj 0x8fa7e400, NULL lock
[...]

It seems that I can avoid this panic if

- revert kern_ksyms.c to rev 1.98 (*), *or*
- set savecore=NO in rc.conf.

(*) I've not tested revs 1.99--1.103.

Can you please take a look into?


Interesting.  I'm not sure what's going on here.  The uvm panic looks
like a compounded symptom rather than the original problem.

Can you try booting with savecore=NO and see if `cat /dev/ksyms' or
`nm /dev/ksyms' works?

Can you also try rev. 1.103 and see if that makes a difference, just
to narrow it down?  (No point in trying the other revisions.  Unlikely
that 1.103 will work if 1.104 doesn't, but will help to confidently
narrow it down.)


'nm /dev/ksyms' works fine for 1.104. Also, panic does not occur after that,
as far as I can see.

For 1.103, savecore does not work and panic occurs during multiuser boot with
savecore=YES in the same manner as for 1.104. On the other hand,
'nm /dev/ksyms' works fine.

For 1.102 (== 1.98), whereas savecore works, nm complains as:


# nm /dev/ksyms
nm: warning: /dev/ksyms has a corrupt section with a size (18) larger than the 
file size
nm: warning: /dev/ksyms has a corrupt section with a size (4a560) larger than 
the file size
nm: warning: /dev/ksyms has a corrupt section with a size (46de5) larger than 
the file size
nm: warning: /dev/ksyms has a corrupt section with a size (40) larger than the 
file size
8c38fec8 t C.7
8c3e5b08 t CSWTCH.118
...


Thanks,
rin


Re: CVS commit: src/sys

2021-09-15 Thread Taylor R Campbell
> Date: Wed, 15 Sep 2021 19:58:20 +0900
> From: Rin Okuyama 
> 
> login: [  95.5000696] panic: kernel diagnostic assertion "slock != NULL" 
> failed: file "../../../../uvm/uvm_pdaemon.c", line 398 pg 0x8c66bd88 uobj 
> 0x8fa7e400, NULL lock
> [...]
> 
> It seems that I can avoid this panic if
> 
> - revert kern_ksyms.c to rev 1.98 (*), *or*
> - set savecore=NO in rc.conf.
> 
> (*) I've not tested revs 1.99--1.103.
> 
> Can you please take a look into?

Interesting.  I'm not sure what's going on here.  The uvm panic looks
like a compounded symptom rather than the original problem.

Can you try booting with savecore=NO and see if `cat /dev/ksyms' or
`nm /dev/ksyms' works?

Can you also try rev. 1.103 and see if that makes a difference, just
to narrow it down?  (No point in trying the other revisions.  Unlikely
that 1.103 will work if 1.104 doesn't, but will help to confidently
narrow it down.)


Re: CVS commit: src/sys

2021-09-15 Thread Rin Okuyama

Hi,

On 2021/09/11 19:09, Taylor R Campbell wrote:

Module Name:src
Committed By:   riastradh
Date:   Sat Sep 11 10:09:55 UTC 2021

Modified Files:
src/sys/arch/sparc64/sparc64: machdep.c
src/sys/kern: kern_ksyms.c subr_csan.c subr_msan.c
src/sys/sys: ksyms.h

Log Message:
ksyms: Use pserialize(9) for kernel access to ksyms.

This makes it available in interrupt context, e.g. for printing
messages with kernel symbol names for return addresses as drm wants
to do.


To generate a diff of this commit:
cvs rdiff -u -r1.302 -r1.303 src/sys/arch/sparc64/sparc64/machdep.c
cvs rdiff -u -r1.103 -r1.104 src/sys/kern/kern_ksyms.c
cvs rdiff -u -r1.12 -r1.13 src/sys/kern/subr_csan.c
cvs rdiff -u -r1.16 -r1.17 src/sys/kern/subr_msan.c
cvs rdiff -u -r1.41 -r1.42 src/sys/sys/ksyms.h

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


kern_ksyms.c rev 1.104 causes this on landisk (sh3):


Checking for core dump...
savecore: (null): kvm_nlist: bad namelist
Sep 15 19:03:53 hdlu savecore: (null): kvm_nlist: bad namelist
savecore: (null): _dumpdev not in namelist
Sep 15 19:03:53 hdlu savecore: (null): _dumpdev not in namelist
Starting local daemons:.
Updating motd.
Starting ntpd.
Starting postfix.
Starting inetd.
Starting cron.
Wed Sep 15 19:04:33 JST 2021

NetBSD/landisk (hdlu) (constty)

login: [  95.5000696] panic: kernel diagnostic assertion "slock != NULL" failed: file 
"../../../../uvm/uvm_pdaemon.c", line 398 pg 0x8c66bd88 uobj 0x8fa7e400, NULL lock
[  95.6535669] cpu0: Begin traceback...
[  95.7031947] db_panic() at netbsd:vpanic+0xea
[  95.7500715] vpanic() at netbsd:kern_assert+0x24
[  95.7969493] kern_assert() at netbsd:uvmpd_page_owner_lock+0x56
[  95.8750798] uvmpd_page_owner_lock() at netbsd:uvmpd_trylockowner+0x3e
[  95.9531988] uvmpd_trylockowner() at netbsd:uvmpdpol_balancequeue+0x19c
[  96.0313226] uvmpdpol_balancequeue() at netbsd:uvm_pageout+0x5fa
[  96.0993345] uvm_pageout() at netbsd:lwp_trampoline+0xc
[  96.1568912] lwp_trampoline() at 0
[  96.1997961] cpu0: End traceback...
Stopped in pid 0.107 (system) atnetbsd:cpu_Debugger+0x6:mov
r14, r15
db>


It seems that I can avoid this panic if

- revert kern_ksyms.c to rev 1.98 (*), *or*
- set savecore=NO in rc.conf.

(*) I've not tested revs 1.99--1.103.

Can you please take a look into?

Thanks,
rin


Re: CVS commit: src/usr.sbin/acpitools/acpidump

2021-09-14 Thread Jason Thorpe



> On Sep 14, 2021, at 1:34 PM, Roland Illig  wrote:
> 
> When lint runs on the code, it defines the preprocessor macro 'lint' to
> be 1.  Due to that, this name cannot be used as a regular identifier.

Perhaps all of the "#ifdef lint" conditions should become "#ifdef __lint__"?

-- thorpej



Re: CVS commit: src/sys/arch/sh3/sh3

2021-09-08 Thread Rin Okuyama

On 2021/09/08 20:47, Izumi Tsutsui wrote:

Module Name:src
Committed By:   rin
Date:   Wed Sep  8 07:22:56 UTC 2021

Modified Files:
src/sys/arch/sh3/sh3: pmap.c

Log Message:
Redo a part of rev 1.89:

- Remove redundant parentheses/braces/comments.
- Fix indents.

No binary changes confirmed this time.


---
-   if (kva) {
+   if (kva)
entry |= PG_V | PG_SH |
((prot & VM_PROT_WRITE) ?
(PG_PR_KRW | PG_D) : PG_PR_KRO);
-   } else {
+   else
entry |= PG_V |
((prot & VM_PROT_WRITE) ?
(PG_PR_URW | PG_D) : PG_PR_URO);
-   }
}
---

This part doesn't match KNF:
  http://cvsweb.netbsd.org/bsdweb.cgi/src/share/misc/style#rev1.58


Update style around single-line braces according to discussion.

https://mail-index.netbsd.org/tech-userlevel/2020/07/12/msg012536.html
https://mail-index.netbsd.org/tech-kern/2020/07/12/msg026594.html

Retain some examples of technically unnecessary braces that likely
aid legibility from the previous commit.


So I don't think removing existing ones per "redundant" is a valid reason.


Oops, I misread style. These and one more similar braces have been restored.
Thanks for pointing it out!

rin


Re: CVS commit: src/sys/arch/sh3/sh3

2021-09-08 Thread Izumi Tsutsui
> Module Name:  src
> Committed By: rin
> Date: Wed Sep  8 07:22:56 UTC 2021
> 
> Modified Files:
>   src/sys/arch/sh3/sh3: pmap.c
> 
> Log Message:
> Redo a part of rev 1.89:
> 
> - Remove redundant parentheses/braces/comments.
> - Fix indents.
> 
> No binary changes confirmed this time.

---
-   if (kva) {
+   if (kva)
entry |= PG_V | PG_SH |
((prot & VM_PROT_WRITE) ?
(PG_PR_KRW | PG_D) : PG_PR_KRO);
-   } else {
+   else
entry |= PG_V |
((prot & VM_PROT_WRITE) ?
(PG_PR_URW | PG_D) : PG_PR_URO);
-   }
}
---

This part doesn't match KNF:
 http://cvsweb.netbsd.org/bsdweb.cgi/src/share/misc/style#rev1.58

> Update style around single-line braces according to discussion.
> 
> https://mail-index.netbsd.org/tech-userlevel/2020/07/12/msg012536.html
> https://mail-index.netbsd.org/tech-kern/2020/07/12/msg026594.html
> 
> Retain some examples of technically unnecessary braces that likely
> aid legibility from the previous commit.

So I don't think removing existing ones per "redundant" is a valid reason.

---
Izumi Tsutsui


Re: CVS commit: src/crypto/external/bsd/openssh/dist

2021-09-06 Thread Rin Okuyama

On 2021/09/06 23:31, Taylor R Campbell wrote:

Date: Mon, 6 Sep 2021 22:32:22 +0900
From: Rin Okuyama 

On 2021/09/06 22:11, Ryo ONODERA wrote:

Module Name:src
Committed By:   ryoon
Date:   Mon Sep  6 13:11:34 UTC 2021

Modified Files:
src/crypto/external/bsd/openssh/dist: dns.c

Log Message:
Make no diff to upstream


This diff from upstream is intentional. See:

http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.6
http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.7


At the time, I think upstream used memcmp, so anything different from
that was a local change.  Now our libc has consttime_memequal, and
upstream uses a similar function called timingsafe_bcmp, so as long as
timingsafe_bcmp is defined in terms of consttime_memequal (and not in
terms of memcmp or bcmp), reducing this local diff strikes me as an
improvement (speaking as the author of the original local change).


Yeah, I agree. I didn't notice the definitions in includes.h.
Thanks for your detailed explanation!

rin


Re: CVS commit: src/crypto/external/bsd/openssh/dist

2021-09-06 Thread Taylor R Campbell
> Date: Mon, 6 Sep 2021 22:32:22 +0900
> From: Rin Okuyama 
> 
> On 2021/09/06 22:11, Ryo ONODERA wrote:
> > Module Name:src
> > Committed By:   ryoon
> > Date:   Mon Sep  6 13:11:34 UTC 2021
> > 
> > Modified Files:
> > src/crypto/external/bsd/openssh/dist: dns.c
> > 
> > Log Message:
> > Make no diff to upstream
> 
> This diff from upstream is intentional. See:
> 
> http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.6
> http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.7

At the time, I think upstream used memcmp, so anything different from
that was a local change.  Now our libc has consttime_memequal, and
upstream uses a similar function called timingsafe_bcmp, so as long as
timingsafe_bcmp is defined in terms of consttime_memequal (and not in
terms of memcmp or bcmp), reducing this local diff strikes me as an
improvement (speaking as the author of the original local change).


Re: CVS commit: src/crypto/external/bsd/openssh/dist

2021-09-06 Thread Ryo ONODERA
Hi,

Rin Okuyama  writes:

> On 2021/09/06 23:11, Ryo ONODERA wrote:
>> Hi,
>> 
>> Rin Okuyama  writes:
>> 
>>> On 2021/09/06 22:11, Ryo ONODERA wrote:
 Module Name:   src
 Committed By:  ryoon
 Date:  Mon Sep  6 13:11:34 UTC 2021

 Modified Files:
src/crypto/external/bsd/openssh/dist: dns.c

 Log Message:
 Make no diff to upstream


 To generate a diff of this commit:
 cvs rdiff -u -r1.20 -r1.21 src/crypto/external/bsd/openssh/dist/dns.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.
>>>
>>> This diff from upstream is intentional. See:
>>>
>>> http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.6
>>> http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.7
>> 
>> Thanks for your pointer.
>> 
>> 
>> #define timingsafe_bcmp(a, b, c) (!consttime_memequal((a), (b), (c)))
>> 
>> is in src/crypto/external/bsd/openssh/dist/includes.h.
>> 
>> My change still uses consttime_memequal() practically like
>> other places in OpenSSH.
>
> Ah, I got it. Thanks for explanation, and sorry for the noise!

Sorry for my less explanation.
I should write more information in the commit message.

Thank you.

> rin

-- 
Ryo ONODERA // r...@tetera.org
PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB  FD1B F404 27FA C7D1 15F3


Re: CVS commit: src/crypto/external/bsd/openssh/dist

2021-09-06 Thread Rin Okuyama

On 2021/09/06 23:11, Ryo ONODERA wrote:

Hi,

Rin Okuyama  writes:


On 2021/09/06 22:11, Ryo ONODERA wrote:

Module Name:src
Committed By:   ryoon
Date:   Mon Sep  6 13:11:34 UTC 2021

Modified Files:
src/crypto/external/bsd/openssh/dist: dns.c

Log Message:
Make no diff to upstream


To generate a diff of this commit:
cvs rdiff -u -r1.20 -r1.21 src/crypto/external/bsd/openssh/dist/dns.c

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


This diff from upstream is intentional. See:

http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.6
http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.7


Thanks for your pointer.


#define timingsafe_bcmp(a, b, c) (!consttime_memequal((a), (b), (c)))

is in src/crypto/external/bsd/openssh/dist/includes.h.

My change still uses consttime_memequal() practically like
other places in OpenSSH.


Ah, I got it. Thanks for explanation, and sorry for the noise!

rin


Re: CVS commit: src/crypto/external/bsd/openssh/dist

2021-09-06 Thread Ryo ONODERA
Hi,

Rin Okuyama  writes:

> On 2021/09/06 22:11, Ryo ONODERA wrote:
>> Module Name: src
>> Committed By:ryoon
>> Date:Mon Sep  6 13:11:34 UTC 2021
>> 
>> Modified Files:
>>  src/crypto/external/bsd/openssh/dist: dns.c
>> 
>> Log Message:
>> Make no diff to upstream
>> 
>> 
>> To generate a diff of this commit:
>> cvs rdiff -u -r1.20 -r1.21 src/crypto/external/bsd/openssh/dist/dns.c
>> 
>> Please note that diffs are not public domain; they are subject to the
>> copyright notices on the relevant files.
>
> This diff from upstream is intentional. See:
>
> http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.6
> http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.7

Thanks for your pointer.


#define timingsafe_bcmp(a, b, c) (!consttime_memequal((a), (b), (c)))

is in src/crypto/external/bsd/openssh/dist/includes.h.

My change still uses consttime_memequal() practically like
other places in OpenSSH.

> Thanks,
> rin

-- 
Ryo ONODERA // r...@tetera.org
PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB  FD1B F404 27FA C7D1 15F3


Re: CVS commit: src/crypto/external/bsd/openssh/dist

2021-09-06 Thread Rin Okuyama

On 2021/09/06 22:11, Ryo ONODERA wrote:

Module Name:src
Committed By:   ryoon
Date:   Mon Sep  6 13:11:34 UTC 2021

Modified Files:
src/crypto/external/bsd/openssh/dist: dns.c

Log Message:
Make no diff to upstream


To generate a diff of this commit:
cvs rdiff -u -r1.20 -r1.21 src/crypto/external/bsd/openssh/dist/dns.c

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


This diff from upstream is intentional. See:

http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.6
http://cvsweb.netbsd.org/bsdweb.cgi/src/crypto/external/bsd/openssh/dist/dns.c#rev1.7

Thanks,
rin


Re: CVS commit: src

2021-08-31 Thread Roland Illig
Am 30.08.2021 um 11:11 schrieb matthew green:
> "Roland Illig" writes:
>> Module Name: src
>> Committed By:rillig
>> Date:Sun Aug 29 09:29:32 UTC 2021
>>
>> Modified Files:
>>  src/tests/usr.bin/xlint/lint1: msg_220.c msg_220.exp
>>  src/usr.bin/xlint/common: lint.h
>>  src/usr.bin/xlint/lint1: lex.c
>>
>> Log Message:
>> lint: allow 'fallthrough' as alternative spelling of FALLTHROUGH
>
> if you would like to see what GCC allows here, it's pretty involved.
> see external/gpl3/gcc/dist/libcpp/lex.c:fallthrough_comment_p().

Yep, I tried to read through these regular expressions myself but then
decided that I would focus on those few cases that we actually have in
the NetBSD tree.  I prefer not to use regular expressions in lint, to
keep it as simple as possible.

Roland


Re: CVS commit: src/usr.sbin/inetd

2021-08-31 Thread Roland Illig
Am 31.08.2021 um 18:29 schrieb Tobias Nygren:
> -   SWAP(int, cp->se_type, sep->se_type);
> +   SWAP(enum service_type, cp->se_type, sep->se_type);

Thanks for the note, I just fixed it.

I left out the 'enum' since there is a typedef for it.

Roland


Re: CVS commit: src/usr.sbin/inetd

2021-08-31 Thread Tobias Nygren
On Mon, 30 Aug 2021 18:21:11 +
Roland Illig  wrote:

> Module Name:  src
> Committed By: rillig
> Date: Mon Aug 30 18:21:11 UTC 2021
> 
> Modified Files:
>   src/usr.sbin/inetd: Makefile inetd.c parse_v2.c
> 
> Log Message:
> inetd: raise WARNS from 5 to 6

clang complains:

/work/src/usr.sbin/inetd/inetd.c:763:18: error: implicit conversion changes 
signedness: 'service_type' (aka 'enum service_type') to 'int' 
[-Werror,-Wsign-conversion]
SWAP(int, cp->se_type, sep->se_type);
~~^~

Works for me:

--- inetd.c 30 Aug 2021 18:21:11 -  1.131
+++ inetd.c 31 Aug 2021 16:26:45 -
@@ -760,7 +760,7 @@ config(void)
 #ifdef IPSEC
SWAP(char *, sep->se_policy, cp->se_policy);
 #endif
-   SWAP(int, cp->se_type, sep->se_type);
+   SWAP(enum service_type, cp->se_type, sep->se_type);
SWAP(size_t, cp->se_service_max, sep->se_service_max);
SWAP(size_t, cp->se_ip_max, sep->se_ip_max);


Re: CVS commit: src/sys/arch/evbarm/conf

2021-08-31 Thread Rin Okuyama

Hi,

On 2021/08/30 18:20, matthew green wrote:

hi.


nice work on BE marvell :)


Thanks!


"Rin Okuyama" writes:

Module Name:src
Committed By:   rin
Date:   Mon Aug 30 00:12:15 UTC 2021

Modified Files:
src/sys/arch/evbarm/conf: MARVELL_NAS

Log Message:
Enable FFS_EI and DISKLABEL_EI as this SoC supports both endians now.


personally, i think we should do these everywhere that isn't
size constrained.  they're useful for accessing other platform
disks quite often on all netbsd systems.


Yeah, I agree. Probably, we can have these options in
sys/conf/filesystems.config. Most powerful platforms have
already used that file:


$ cd /usr/src/sys/arch && grep filesystems.conf */conf/*
amd64/conf/GENERIC:include "conf/filesystems.config"
amd64/conf/XEN3_DOM0:include "conf/filesystems.config"
amd64/conf/XEN3_DOMU:include "conf/filesystems.config"
evbarm/conf/GENERIC.common:include "conf/filesystems.config"
i386/conf/GENERIC:include "conf/filesystems.config"
macppc/conf/GENERIC:include "conf/filesystems.config"
riscv/conf/GENERIC:include  "conf/filesystems.config"
sgimips/conf/GENERIC32_IP2x:include "conf/filesystems.config"
sgimips/conf/GENERIC32_IP3x:include "conf/filesystems.config"
sgimips/conf/GENERIC32_IP12:include "conf/filesystems.config"
sparc64/conf/GENERIC:include "conf/filesystems.config"


And we can switch other machines to use it.

Thanks,
rin


Re: CVS commit: src

2021-08-30 Thread Christos Zoulas
In article ,
Tom Ivar Helbekkmo   wrote:
>Christos Zoulas  writes:
>
>> Module Name: src
>> Committed By:christos
>> Date:Sun Aug 29 09:54:18 UTC 2021
>>
>> Modified Files:
>>  src/distrib/sets/lists/debug: mi
>>  src/distrib/sets/lists/tests: mi
>>  src/etc/mtree: NetBSD.dist.tests
>>  src/tests/usr.sbin: Makefile
>>  src/usr.sbin/inetd: Makefile inetd.8 inetd.c
>> Added Files:
>>  src/tests/usr.sbin/inetd: Makefile inetd_ratelimit.conf t_inetd.c
>>  test_server.c
>>  src/usr.sbin/inetd: inetd.h parse_v2.c
>
>parse_v2.c contains a couple of comparisons of char values to -1.  On
>arm, this fails to build, because the compiler observes that those
>comparisons are always false.
>
>-tih

Thanks for fixing it.

christos



re: CVS commit: src/sys/arch/evbarm/conf

2021-08-30 Thread matthew green
hi.


nice work on BE marvell :)

"Rin Okuyama" writes:
> Module Name:  src
> Committed By: rin
> Date: Mon Aug 30 00:12:15 UTC 2021
>
> Modified Files:
>   src/sys/arch/evbarm/conf: MARVELL_NAS
>
> Log Message:
> Enable FFS_EI and DISKLABEL_EI as this SoC supports both endians now.

personally, i think we should do these everywhere that isn't
size constrained.  they're useful for accessing other platform
disks quite often on all netbsd systems.


.mrg.


re: CVS commit: src

2021-08-30 Thread matthew green
"Roland Illig" writes:
> Module Name:  src
> Committed By: rillig
> Date: Sun Aug 29 09:29:32 UTC 2021
>
> Modified Files:
>   src/tests/usr.bin/xlint/lint1: msg_220.c msg_220.exp
>   src/usr.bin/xlint/common: lint.h
>   src/usr.bin/xlint/lint1: lex.c
>
> Log Message:
> lint: allow 'fallthrough' as alternative spelling of FALLTHROUGH

if you would like to see what GCC allows here, it's pretty involved.
see external/gpl3/gcc/dist/libcpp/lex.c:fallthrough_comment_p().

if the comments are to be believed, it does:

  /* -Wimplicit-fallthrough=2 looks for (case insensitive)
 .*falls?[ \t-]*thr(u|ough).* regex.  */

  /* Whole comment contents (regex):
 lint -fallthrough[ \t]*

  /* Whole comment contents (regex):
 [ \t]*FALLTHR(U|OUGH)[ \t]*

  /* Whole comment contents (regex):
 [ \t.!]*(ELSE,? |INTENTIONAL(LY)? )?FALL(S | |-)?THR(OUGH|U)[ 
\t.!]*(-[^\n\r]*)?
 [ \t.!]*(Else,? |Intentional(ly)? )?Fall((s | |-)[Tt]|t)hr(ough|u)[ 
\t.!]*(-[^\n\r]*)?
 [ \t.!]*([Ee]lse,? |[Ii]ntentional(ly)? )?fall(s | |-)?thr(ough|u)[ 
\t.!]*(-[^\n\r]*)?



.mrg.


Re: CVS commit: src

2021-08-30 Thread Roland Illig


Tom Ivar Helbekkmo  wrote:
> Christos Zoulas  writes:
>
>> Module Name:  src
>> Committed By: christos
>> Date:   Sun Aug 29 09:54:18 UTC 2021
>>
>> Modified Files:
>>   src/distrib/sets/lists/debug: mi
>>   src/distrib/sets/lists/tests: mi
>>   src/etc/mtree: NetBSD.dist.tests
>>   src/tests/usr.sbin: Makefile
>>   src/usr.sbin/inetd: Makefile inetd.8 inetd.c
>> Added Files:
>>   src/tests/usr.sbin/inetd: Makefile inetd_ratelimit.conf t_inetd.c
>>   test_server.c
>>   src/usr.sbin/inetd: inetd.h parse_v2.c
>
> parse_v2.c contains a couple of comparisons of char values to -1.  On
> arm, this fails to build, because the compiler observes that those
> comparisons are always false.

Nice catch. This is something that lint already warns about (see 
tests/usr.bin/xlint/lint1/msg_230.c), it's just not enabled for usr.sbin. A few 
days ago I enabled it for usr.bin, will do the same for other directories as 
well.


Re: CVS commit: src

2021-08-30 Thread Tom Ivar Helbekkmo
Christos Zoulas  writes:

> Module Name:  src
> Committed By: christos
> Date: Sun Aug 29 09:54:18 UTC 2021
>
> Modified Files:
>   src/distrib/sets/lists/debug: mi
>   src/distrib/sets/lists/tests: mi
>   src/etc/mtree: NetBSD.dist.tests
>   src/tests/usr.sbin: Makefile
>   src/usr.sbin/inetd: Makefile inetd.8 inetd.c
> Added Files:
>   src/tests/usr.sbin/inetd: Makefile inetd_ratelimit.conf t_inetd.c
>   test_server.c
>   src/usr.sbin/inetd: inetd.h parse_v2.c

parse_v2.c contains a couple of comparisons of char values to -1.  On
arm, this fails to build, because the compiler observes that those
comparisons are always false.

-tih
-- 
Most people who graduate with CS degrees don't understand the significance
of Lisp.  Lisp is the most important idea in computer science.  --Alan Kay


Re: CVS commit: src/sys

2021-08-25 Thread Jason Thorpe



> On Aug 24, 2021, at 10:21 PM, matthew green  wrote:
> 
>> - For alpha and sparc64, don't define MUTEX_CAS() in terms of their own
>>  _lock_cas(), which has its own memory barriers; the call sites in
>>  kern_mutex.c already have the appropriate memory barrier calls.  Thus,
>>  alpha and sparc64 can use default definition.
> 
> this seems to leave _lock_cas() unused on both platforms, should
> the backing code for _lock_cas() be removed now?

Yes, I intended for that to be a separate commit.

-- thorpej



re: CVS commit: src/sys

2021-08-24 Thread matthew green
> - For alpha and sparc64, don't define MUTEX_CAS() in terms of their own
>   _lock_cas(), which has its own memory barriers; the call sites in
>   kern_mutex.c already have the appropriate memory barrier calls.  Thus,
>   alpha and sparc64 can use default definition.

this seems to leave _lock_cas() unused on both platforms, should
the backing code for _lock_cas() be removed now?

i didn't look closely at other ports (though sh3 looks OK as-is.)

thanks.


.mrg.


re: CVS commit: src/external/mit/xorg/server/xorg-server/hw

2021-08-21 Thread matthew green
> XXX: I wonder if each Xorg server extension can be enabled/disabled
>  per ${MACHINE} basis rather than in MI include/dix-config.h header.

i had a look at this, and we could do it but it might require
generating a patched eg, xorg-config.h and maybe others, some
that are installed, some that are not installed.

not against the idea, just wasn't inspired for it yet :)

thanks for fixing the build.


.mrg.


Re: CVS commit: src/usr.bin/mkdep

2021-08-20 Thread bch
Hey Roland - friendly question:
I saw this error earlier today and threw in a temporary local fix where I
pushed the “const” qualifier deeper (into findcc.[ch]) rather than relax it
in mk dep.c. You choose the other route. Did you do that because you expect
to have that storage r/w, or something else, rather than enforcing a
restriction (that currently seems to hold) further into the code?

Signed Curious. 





On Thu, Aug 19, 2021 at 21:24 Roland Illig  wrote:

> Module Name:src
> Committed By:   rillig
> Date:   Fri Aug 20 04:23:56 UTC 2021
>
> Modified Files:
> src/usr.bin/mkdep: mkdep.c
>
> Log Message:
> mkdep: fix string constness in call to findcc
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.45 -r1.46 src/usr.bin/mkdep/mkdep.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>


Re: CVS commit: src/usr.bin

2021-08-20 Thread Roland Illig
Am 20.08.2021 um 03:12 schrieb Robert Elz:
> Date:Thu, 19 Aug 2021 21:21:04 +
> From:"Roland Illig" 
> Message-ID:  <20210819212104.7c965f...@cvs.netbsd.org>
>
>   | mkdep: fix prototype of findcc
>
> That broke the build.

I'm sorry for that.  I knew that mkdep and lint were the only programs
that use this code, so I ran "make" in these directories and then tested
the programs.  What I had missed was to first run "make depend" so that
findcc.c would be rebuilt as well.

> The right thing to do is to handle the FIXME in the comment,
> and fix it, instead of enshrining it by changing the prototype.

Thank you for the idea of how to fix it properly, I just did that and
extended the existing tests as well.

Roland


Re: CVS commit: src/usr.bin

2021-08-19 Thread Robert Elz
Date:Thu, 19 Aug 2021 21:21:04 +
From:"Roland Illig" 
Message-ID:  <20210819212104.7c965f...@cvs.netbsd.org>

  | mkdep: fix prototype of findcc

That broke the build.

  | A function that modifies a string argument must not declare that
  | argument as 'const char *', even if all callers (mkdep and lint) always
  | pass it a modifiable string.

Apparently they don't (but they probably don't pass unmodifiable
strings containing spaces).

The right thing to do is to handle the FIXME in the comment,
and fix it, instead of enshrining it by changing the prototype.

Something like

char *
findcc(const char *progname)
{
char   *path, *dir, *next;
char   buffer[MAXPATHLEN];
size_t len;

if ((next = strchr(progname, ' ')) != NULL) {
if (next > progname)
len = (size_t)(next - progname);
else
return NULL;
} else
len = strlen(progname);

/* there could be a test that len <= MAXINT, but really... */

if (memchr(progname, '/', len) != NULL) {
path = strndup(progname, len);

if (access(path, X_OK) == 0)
return path;
free(path);
return NULL;
}

if (((path = getenv("PATH")) == NULL) ||
((path = strdup(path)) == NULL))
return NULL;

dir = path;
while (dir != NULL) {
if ((next = strchr(dir, ':')) != NULL)
*next++ = '\0';

if (snprintf(buffer, sizeof(buffer),
"%s/%.*s", dir, (int)len, progname) < (int)sizeof(buffer)) {
if (!access(buffer, X_OK)) {
free(path);
return strdup(buffer);
}
}
dir = next;
}

free(path);
return NULL;
}

That's untested, not even compile tested, and it should be before being
committed (fully tested, in a release build, with ATF tests run), but
something like that should work, and work without modifying the input
string, which should immediately go back to being const while this is being
worked out, to unbreak the build.

kre

ps: I cut & pasted the code from an xterm, so tabs will have been lost
all over the place.   That needs fixing for sure.



Re: CVS commit: src/sys/kern

2021-08-18 Thread Michael van Elst
On Tue, Aug 17, 2021 at 11:39:26PM +, Taylor R Campbell wrote:
> > 
> > Log Message:
> > skip symbol tables that were unloaded again to avoid EFAULT when reading
> > ksyms.
> > 
> > also restore TAILQ_FOREACH idiom.
> 
> This change isn't quite right: Reading past st = ksyms_last_snapshot
> in ksymsread, or in any context where we rely on the snapshot without
> holding ksyms_lock, is not allowed -- it will lead to a corrupted view
> of the snapshot, and possibly end up reading uninitialized memory.
> 
> That's why this code didn't use TAILQ_FOREACH -- it must stop at
> ksyms_last_snapshot; if it gets to the end of the list, and witnesses
> st = NULL, then that's a bug.

TAILQ_FOREACH just adds another exit condition that prevents entering
the loop with st = NULL.

A problem might be to continue the loop in case ksyms_last_snapshot itself
is gone.

Something like:

if (st->sd_gone)
goto skip;
...
skip:
if (st == ksyms_last_snapshot)
break;

avoids that case.


> The code also didn't skip entries with st->sd_gone because we arrange
> for st->sd_nmap not to be freed until the last ksymsclose, at which
> point no more ksymsread can be active.

Keeping sd_nmap isn't sufficient, ksymsread failed with EFAULT as
you still derefence pointers to the unloaded module.
Skipping the unloaded module when reading the symbols avoids this.



Re: CVS commit: src/sys/kern

2021-08-17 Thread Taylor R Campbell
> Module Name:src
> Committed By:   mlelstv
> Date:   Sun Jul 18 06:57:28 UTC 2021
> 
> Modified Files:
> src/sys/kern: kern_ksyms.c
> 
> Log Message:
> skip symbol tables that were unloaded again to avoid EFAULT when reading
> ksyms.
> 
> also restore TAILQ_FOREACH idiom.

This change isn't quite right: Reading past st = ksyms_last_snapshot
in ksymsread, or in any context where we rely on the snapshot without
holding ksyms_lock, is not allowed -- it will lead to a corrupted view
of the snapshot, and possibly end up reading uninitialized memory.

That's why this code didn't use TAILQ_FOREACH -- it must stop at
ksyms_last_snapshot; if it gets to the end of the list, and witnesses
st = NULL, then that's a bug.

The code also didn't skip entries with st->sd_gone because we arrange
for st->sd_nmap not to be freed until the last ksymsclose, at which
point no more ksymsread can be active.  If you start skipping some
entries, that will cause assertions to fire about mismatched symbol
table size which we computed up front in ksymsopen.

I made a mistake in some previous changes: Although we defer freeing
st->sd_nmap while /dev/ksyms is open, we do not defer freeing
st->sd_strstart or st->sd_symstart.  And it is possible for a
concurrent module _unload_ to issue kobj_unload in the middle of
concurrent ksymsread and free st->sd_strstart or st->sd_symstart while
ksymsread is reading from them.  (My last round of changes was done to
fix bugs found during module _load_.)  The commit message doesn't say,
but I assume your change was meant to fix that bug.

Unfortunately, the change doesn't completely fix it -- we could easily
find ourselves in the following situation:

CPU 0   CPU 1
-   -
read from /dev/ksyms
modunload foo.kmod
load st->sd_gone = false
kobj_unload
store st->sd_gone = true
kobj_free(ko, ko->ko_symtab);
copy from st->sd_symstart -> FAULT

In order to avoid this situation, we either need to:

1. Make ksyms_modunload block until last /dev/ksyms close.

   => Easy enough: slap a condvar on ksyms_opencnt, wait for
  ksyms_opencnt == 0 in ksyms_modunload, notify if ksyms_opencnt
  == 0 in ksymsclose.

   => Holding /dev/ksyms open indefinitely can block module unload
  indefinitely; unclear if this might lead to problematic deadlock
  scenarios.

   => Bonus: Can eliminate some of the logic in ksymsclose, since
  ksyms_modunload will handle it synchronously before return
  anyway.

2. Convert ksymsread to use psref or localcount.

   => Takes a little more effort to convert tailq to pslist -- which
  requires some care because struct ksyms_symtab is shared with
  userland.  So we still have to keep the tailq entry records (or
  add disgusting compat code).  This is why I didn't go for
  pserialize in my last round of changes.

   => Complicated because we would only get a snapshot within a single
  ksymsread call, not from open to close of /dev/ksyms; this would
  require some kind of mechanism to persuade userland tools to
  restart if the snapshot has changed from read to read.

   => (Can't use pserialize because we have to hold the snapshot
  stable across copyout to userland, which may sleep, which is not
  allowed in pserialize read sections.)

3. Find some other way to copy the tables or something while
   /dev/ksyms is open so they won't go away when kobj_unload runs, or
   just keep copies of the strtab and symtab for ksyms that persist
   unload -- might cost a lot of kernel memory, probably not worth
   pursuing.

In any case this change should otherwise be reverted, since it is no
necessary (sd_symstart/sd_strstart will no longer go away during
ksymsread) and it is also wrong (iteration past ksyms_last_snapshot is
not allowed).

I'm leaning toward option (1); other thoughts welcome.


Re: CVS commit: src/sys/modules/lua

2021-08-08 Thread Rin Okuyama

On 2021/08/09 7:26, Rin Okuyama wrote:

- Initialize LIST_HEAD.


Hmm, since HEADs are static, this is not necessary. But, it should be
a good practice to initialize HEADs always, IMO...

Thanks,
rin


Re: CVS commit: src/sbin/devpubd/hooks

2021-08-08 Thread Robert Elz
Sorry, no idea what happened with that.   And thanks Martin for fixing it.

kre



  1   2   3   4   5   6   7   8   9   10   >