vmd(8): fix device state on vm receive

2021-03-21 Thread Dave Voutila
I was using the send/dump feature and noticed that when restoring a
guest that the console connection would "stutter." (Wasn't printing
fully without hitting return.)

The following diff properly restores the vm_pipe structures for the
uart, pit, and rtc devices when using `vmctl receive`. The pipe readable
event is now removed during calls a device's *_stop() function since it
needs to be re-added in calls to *_start().

I also reordered the device restore logic for the emulated ns8250 so it
more closely resembles the initialization logic to make it easier to
debug in the future.

-Dave


Index: usr.sbin/vmd/i8253.c
===
RCS file: /cvs/src/usr.sbin/vmd/i8253.c,v
retrieving revision 1.32
diff -u -p -r1.32 i8253.c
--- usr.sbin/vmd/i8253.c28 Jun 2020 16:52:45 -  1.32
+++ usr.sbin/vmd/i8253.c22 Mar 2021 02:41:58 -
@@ -412,6 +412,9 @@ i8253_restore(int fd, uint32_t vm_id)
_channel[i]);
i8253_reset(i);
}
+
+   vm_pipe_init(_pipe, i8253_pipe_dispatch);
+
return (0);
 }

@@ -421,6 +424,7 @@ i8253_stop()
int i;
for (i = 0; i < 3; i++)
evtimer_del(_channel[i].timer);
+   event_del(_pipe.read_ev);
 }

 void
@@ -430,4 +434,5 @@ i8253_start()
for (i = 0; i < 3; i++)
if (i8253_channel[i].in_use)
i8253_reset(i);
+   event_add(_pipe.read_ev, NULL);
 }
Index: usr.sbin/vmd/mc146818.c
===
RCS file: /cvs/src/usr.sbin/vmd/mc146818.c,v
retrieving revision 1.22
diff -u -p -r1.22 mc146818.c
--- usr.sbin/vmd/mc146818.c 28 Jun 2020 16:52:45 -  1.22
+++ usr.sbin/vmd/mc146818.c 22 Mar 2021 02:41:58 -
@@ -368,6 +368,9 @@ mc146818_restore(int fd, uint32_t vm_id)
memset(, 0, sizeof(struct event));
evtimer_set(, rtc_fire1, NULL);
evtimer_set(, rtc_fireper, (void *)(intptr_t)rtc.vm_id);
+
+   vm_pipe_init(_pipe, mc146818_pipe_dispatch);
+
return (0);
 }

@@ -376,11 +379,13 @@ mc146818_stop()
 {
evtimer_del();
evtimer_del();
+   event_del(_pipe.read_ev);
 }

 void
 mc146818_start()
 {
evtimer_add(, _tv);
+   event_add(_pipe.read_ev, NULL);
rtc_reschedule_per();
 }
Index: usr.sbin/vmd/ns8250.c
===
RCS file: /cvs/src/usr.sbin/vmd/ns8250.c,v
retrieving revision 1.29
diff -u -p -r1.29 ns8250.c
--- usr.sbin/vmd/ns8250.c   28 Jun 2020 16:52:45 -  1.29
+++ usr.sbin/vmd/ns8250.c   22 Mar 2021 02:41:58 -
@@ -694,9 +694,7 @@ ns8250_restore(int fd, int con_fd, uint3
com1_dev.byte_out = 0;
com1_dev.regs.divlo = 1;
com1_dev.baudrate = 115200;
-   com1_dev.rate_tv.tv_usec = 1;
com1_dev.pause_ct = (com1_dev.baudrate / 8) / 1000 * 10;
-   evtimer_set(_dev.rate, ratelimit, NULL);

event_set(_dev.event, com1_dev.fd, EV_READ | EV_PERSIST,
com_rcv_event, (void *)(intptr_t)vmid);
@@ -704,6 +702,12 @@ ns8250_restore(int fd, int con_fd, uint3
event_set(_dev.wake, com1_dev.fd, EV_WRITE,
com_rcv_event, (void *)(intptr_t)vmid);

+   timerclear(_dev.rate_tv);
+   com1_dev.rate_tv.tv_usec = 1;
+   evtimer_set(_dev.rate, ratelimit, NULL);
+
+   vm_pipe_init(_pipe, ns8250_pipe_dispatch);
+
return (0);
 }

@@ -712,6 +716,7 @@ ns8250_stop()
 {
if(event_del(_dev.event))
log_warn("could not delete ns8250 event handler");
+   event_del(_pipe.read_ev);
evtimer_del(_dev.rate);
 }

@@ -720,5 +725,6 @@ ns8250_start()
 {
event_add(_dev.event, NULL);
event_add(_dev.wake, NULL);
+   event_add(_pipe.read_ev, NULL);
evtimer_add(_dev.rate, _dev.rate_tv);
 }



Re: arm64: make cwfg(4) report battery information to apm(4)

2021-03-21 Thread Theo de Raadt
Klemens Nanni  wrote:

> > So which is better:
> > 
> > (1) try to emit some information for the people who quicky-use the apm 
> > interface
> > 
> > (2) change apm to not print those lines on architectures where we are 
> > unsure.
> > 
> > I think (1) is acceptable for a tool which has never promised perfect 
> > accuracy. 
> I concur (obviously).
> 
> > I suspect hard-wiring this to one driver is going to be better than scanning
> > the sensor list and heuristically determining which specific sensors to 
> > look at,
> > because the only good selector now is strcmp(sensor->desc, "battery 
> > remaining minutes")
> > yuck.
> To be fair, kettenis proposed a rough idea to have simpler/faster checks
> than strcmp().  With a simple flag you wouldn't need any heuristics
> either but let the sensor framework -which has all the pieces- do the
> work and put a stamp on the ready-to-use value saying "use this as
> battery level".

that simple flag will probably be too simple, and it won't be long
before someone in another architecture wants it to mean something more,
or wants another flag, and before long there will be zeal to flag carrying to
the sensor framework.

and what happens if two drivers set the flag?

Another way of doing this is for an platform to name the sensor driver which
has the info, and then search the sensor free.

Or, simply write per-platform code that finds it's own stuff.



Re: arm64: make cwfg(4) report battery information to apm(4)

2021-03-21 Thread Jeremie Courreges-Anglas
On Sun, Mar 21 2021, Mark Kettenis  wrote:
>> Date: Sun, 21 Mar 2021 17:22:05 +0100
>> From: Klemens Nanni 
>> 
>> On Sun, Mar 21, 2021 at 02:02:00PM +0100, Mark Kettenis wrote:
>> > > Date: Sun, 21 Mar 2021 01:02:53 +0100
>> > > From: Klemens Nanni 
>> > > 
>> > > apm(4/arm64) merely provides an all zero/unknown stub for those values,
>> > > e.g. apm(8) output is useless.
>> > > 
>> > > Hardware sensors however provide the information:
>> > > 
>> > >  $ sysctl hw.sensors
>> > >  hw.sensors.rktemp0.temp0=32.22 degC (CPU)
>> > >  hw.sensors.rktemp0.temp1=33.89 degC (GPU)
>> > >  hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage)
>> > >  hw.sensors.cwfg0.raw0=259 (battery remaining minutes)
>> > >  hw.sensors.cwfg0.percent0=58.00% (battery percent)
>> > > 
>> > > Diff below simply copies them over using apm_setinfohook()
>> > > (I've looked at sys/arch/loongson/dev/kb3310.c which does the same):
>> > > 
>> > >  $ apm
>> > >  Battery state: high, 58% remaining, 259 minutes life estimate
>> > >  A/C adapter state: not known
>> > >  Performance adjustment mode: auto (408 MHz)
>> > > 
>> > > Feedback? OK?
>> > 
>> > This doesn't scale.
>> What exactly does not scale?  The way how various drivers hook into
>> apm(4)?  I'm not familiar enough (yet) with the framework(s) to judge
>> their overall design and/or interop strategy off hand.
>
> Since there is no firmware abstraction like on i386 (and to some
> extent amd64) there will be many drivers that provide battery
> informarion on armv7/arm64.
>
>> > The whole apm(4) interface is too closely tied to the original APM
>> > BIOS model.  Even with acpi(4) it is a bit of a square peg for a round
>> > hole, for example on machines with more than one battery.
>> Right, there is hardware which doesn't fit this model and our apm* is
>> not exactly up to speed with those, but I still don't see actual issues
>> (as per above).
>> 
>> > I'm not even sure apm(8) should bother reporting some sort of battery
>> > status.
>> apm* showing battery information seems only naturally to me since that
>> is the driver/daemon/tool one issues power commands with, often based
>> on such information - think of `apmd -z 15':  given that I want my box
>> to safely suspend around 15% remaining battery, it seems only
>> consequential that apm* is the same tool I query such information with.
>
> apmd(8) and apm(8) are not the same tool though.  Having a deamon to
> run scripts on certain power-state changes makes some sense, but I'd
> argue that sensorsd(8) could be a reasonable place to implement this
> as well.

When I dived into the apm(4) and apmd(8) code I noted how redundant it
was considering sensorsd(8).  But the user interface of sensorsd(8)
needs a fair bit of pondering before it can be proposed as
a replacement.  sensorsd(8) is fairly generic and its configuration
format, while extensible, isn't easy to use.  To be fair I didn't even
try to make it execute stuff based on battery level.  I suspect it
would just be useful to execute commands based on changes in one driver,
when what you need to mimic apmd(8) is to merge the values coming from
different drivers.

> The need to run apmd(8) in order to suspend/hibernate is a bit of an
> historical wart.

It's not clear to me right now why this was needed and why it's not
needed anymore.

(I'm just talking about basic suspend/hibernate, not about the
nifty -z/-Z additions in apmd(8)).

> And we already removed powermanagement state support
> from apmd(8).  These days, users should probably just use
> /etc/sysctl.conf to set the powermanagement state instead of starting
> apmd(8) with the appropriate -A, -H or -L option.

Indeed, -A/-H/-L are just syntactical sugar.  I wouldn't mind seeing
them go.

>> > But if it does, my suggestion would be to make it use the
>> > sensors framework.  That would need some work though such that drivers
>> > can mark sensors as providing battery information.  Maybe add a
>> > SENSOR_FBATTERY flag?

Why the 'F' and not just SENSOR_BATTERY?

>> You mean apm(8) directly using sensor_find(9) or so to look for
>> suitable sensors instead of relaying values throuh apm(4)?
>
> That's a possibility.  But having apm(4) do that might be a useful
> middle ground as it would mean you don't have to change the userland
> tools just yet.

I'm not familiar at all with the sensors framework, but if presenting
the same simple apm(4) interface is doable using sensors, then you can
count me as a reviewer! :)

(I don't know where the apm(4) interface comes from, but it looks like
we have a bunch of ports also using it.)

> My main concern with your diff is having to add APM
> hooks in all the drivers...

Or maybe we need a nicer way to register APM hooks.  IIUC
apm_setinfohook was copied from miod's work on loongson where a single
driver is in charge, depending on the underlying hardware. So it
probably can't solve all use cases as is.

>> It sounds simpler in principal but that's just my naive guess;
>> I'll take a curious 

Re: arm64: make cwfg(4) report battery information to apm(4)

2021-03-21 Thread Klemens Nanni
On Sun, Mar 21, 2021 at 11:56:42AM -0600, Theo de Raadt wrote:
> The sensor framework generally does not know where a sensor is.  A
> sensor could reside on some device which has been plugged in, rather
> than be the primary sensor.
> 
> But the users of apm are only hoping for best effort.
> 
> meaning "some information", not "perfect information".  Many PC BIOS
> have lied in these fields before also, yet all the apm command users
> survived.
That's what my diff does really: best effort for some information.

> So which is better:
> 
> (1) try to emit some information for the people who quicky-use the apm 
> interface
> 
> (2) change apm to not print those lines on architectures where we are unsure.
> 
> I think (1) is acceptable for a tool which has never promised perfect 
> accuracy. 
I concur (obviously).

> I suspect hard-wiring this to one driver is going to be better than scanning
> the sensor list and heuristically determining which specific sensors to look 
> at,
> because the only good selector now is strcmp(sensor->desc, "battery remaining 
> minutes")
> yuck.
To be fair, kettenis proposed a rough idea to have simpler/faster checks
than strcmp().  With a simple flag you wouldn't need any heuristics
either but let the sensor framework -which has all the pieces- do the
work and put a stamp on the ready-to-use value saying "use this as
battery level".



Re: arm64: make cwfg(4) report battery information to apm(4)

2021-03-21 Thread Jeremie Courreges-Anglas
On Sun, Mar 21 2021, Klemens Nanni  wrote:
> apm(4/arm64) merely provides an all zero/unknown stub for those values,
> e.g. apm(8) output is useless.
>
> Hardware sensors however provide the information:
>
>   $ sysctl hw.sensors
>   hw.sensors.rktemp0.temp0=32.22 degC (CPU)
>   hw.sensors.rktemp0.temp1=33.89 degC (GPU)
>   hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage)
>   hw.sensors.cwfg0.raw0=259 (battery remaining minutes)
>   hw.sensors.cwfg0.percent0=58.00% (battery percent)
>
> Diff below simply copies them over using apm_setinfohook()
> (I've looked at sys/arch/loongson/dev/kb3310.c which does the same):
>
>   $ apm
>   Battery state: high, 58% remaining, 259 minutes life estimate
>   A/C adapter state: not known
>   Performance adjustment mode: auto (408 MHz)
>
> Feedback? OK?

Feedback:

> Index: dev/fdt/cwfg.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 cwfg.c
> --- dev/fdt/cwfg.c10 Jun 2020 17:51:21 -  1.1
> +++ dev/fdt/cwfg.c20 Mar 2021 23:43:13 -
> @@ -32,12 +32,15 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  
>  #include 
>  
>  #include 
>  
> +#include "apm.h"
> +
>  #define  VERSION_REG 0x00
>  #define  VCELL_HI_REG0x02
>  #define   VCELL_HI_MASK  0x3f
> @@ -119,6 +122,18 @@ struct cfdriver cwfg_cd = {
>   NULL, "cwfg", DV_DULL
>  };
>  
> +int cwfg_apminfo(struct apm_power_info *info);

Why out of the #if NAPM > 0 check?

> +#if NAPM > 0
> +struct apm_power_info cwfg_power;
> +
> +int
> +cwfg_apminfo(struct apm_power_info *info)
> +{
> + bcopy(_power, info, sizeof(*info));

There's no overlap between source and destination, better use memcpy.
(Else, better use memmove.)

> + return 0;
> +}
> +#endif
> +
>  int
>  cwfg_match(struct device *parent, void *match, void *aux)
>  {
> @@ -202,6 +217,12 @@ cwfg_attach(struct device *parent, struc
>  
>   sensor_task_register(sc, cwfg_update_sensors, 5);
>  
> +#if NAPM > 0
> + /* make sure we have the apm state initialized before apm attaches */
> + cwfg_update_sensors(sc);
> + apm_setinfohook(cwfg_apminfo);
> +#endif
> +
>   printf("\n");
>  }
>  
> @@ -324,6 +345,7 @@ cwfg_update_sensors(void *arg)
>   uint32_t vcell, rtt, tmp;
>   uint8_t val;
>   int error, n;
> + u_char bat_percent;
>  
>   if ((error = cwfg_lock(sc)) != 0)
>   return;
> @@ -348,6 +370,7 @@ cwfg_update_sensors(void *arg)
>   if ((error = cwfg_read(sc, SOC_HI_REG, )) != 0)
>   goto done;
>   if (val != 0xff) {
> + bat_percent = val;
>   sc->sc_sensor[CWFG_SENSOR_SOC].value = val * 1000;
>   sc->sc_sensor[CWFG_SENSOR_SOC].flags &= ~SENSOR_FINVALID;
>   }

If val == 0xff bat_percent is unitialized.  Note that in this error
condition the sensors code leaves sc->sc_sensor[CWFG_SENSOR_SOC].value
alone.

> @@ -363,6 +386,14 @@ cwfg_update_sensors(void *arg)
>   sc->sc_sensor[CWFG_SENSOR_RTT].value = rtt;
>   sc->sc_sensor[CWFG_SENSOR_RTT].flags &= ~SENSOR_FINVALID;
>   }
> +
> +#if NAPM > 0
> + cwfg_power.battery_state = bat_percent > sc->sc_alert_level ?
> + APM_BATT_HIGH : APM_BATT_LOW;

It's cool that this driver keeps track of the "alert level".  acpibat(4)
also does that on amd64, but the apm(4)-through-acpi(4) userland
interface just hardcodes levels:

/* running on battery */
pi->battery_life = remaining * 100 / capacity;
if (pi->battery_life > 50)
pi->battery_state = APM_BATT_HIGH;
else if (pi->battery_life > 25)
pi->battery_state = APM_BATT_LOW;
else
pi->battery_state = APM_BATT_CRITICAL;

Maybe the levels reported by hardware couldn't be trusted?  Or maybe
those hardcoded values were deemed good enough, back then.  Anyway, on
this new hardware I think it makes sense to just trust the exported
values and later act if we get reports.

> + cwfg_power.ac_state = APM_AC_UNKNOWN;

This kinda points out the need for an AC driver on this platform.
If done in another driver, the code will need to be changed.  But this
looks acceptable to me for now.

> + cwfg_power.battery_life = bat_percent;

So to keep apm and sensors in sync I think it would be better to reuse
the cached sc->sc_sensor[CWFG_SENSOR_SOC].value.

I don't know whether the error condition mentioned above happens
frequently with this driver.  Reporting APM_BATT_ABSENT (and similar for
sensors) would be useful, and could be done in a subsequent step.

What's the underlying hardware, does it involve a pluggable battery?

> + cwfg_power.minutes_left = sc->sc_sensor[CWFG_SENSOR_RTT].value;
> +#endif
>  
>  done:
>   cwfg_unlock(sc);
>

-- 
jca | PGP : 

Re: OPENBSD-PF-MIB, use DisplayString not OCTET STRING

2021-03-21 Thread Martijn van Duren
There's two things I'd like to know before going further with this:

1) according to RFC2578 section 3.6:
(1)  registration: the definition of a particular item is registered as
 a particular OBJECT IDENTIFIER value, and associated with a
 particular descriptor.  After such a registration, the semantics
 thereby associated with the value are not allowed to change, the
 OBJECT IDENTIFIER can not be used for any other registration, and
 the descriptor can not be changed nor associated with any other
 registration.  The following macros result in a registration:

  OBJECT-TYPE, MODULE-IDENTITY, NOTIFICATION-TYPE, OBJECT-GROUP,
  OBJECT-IDENTITY, NOTIFICATION-GROUP, MODULE-COMPLIANCE,
  AGENT-CAPABILITIES.

Unless I'm misreading this section this would require a registration of
a new object to strictly comply with SMIv2. Is this change minor enough
to allow it?

2) Afaik any byte sequence is technically allowed for these in pf.conf.
(at least for pfLabelName and pfTableName I manage to make UTF-8 work),
so changing this would technically mean that we wouldn't be able to
export these tables and labels anymore. Note that RFC2579 section 3.1
bullet 3 of octet-format doesn't specify how to handle invalid ascii or
UTF-8. In snmp(1) we went with the replacement character, but how any
other pieces of software would handle that, I don't know. Is this
change safe enough in all situations?

Pending the answers, from my limited knowledge of pf and the MIB I
guess the impact on pfLogIfName and pfIfDescr is limited enough since
they show the actual interface name, which is always ascii anyway.
For pfTableName and pfLabelName it might be an idea to register a new
object which returns something like a best effort UTF-8 presentation
of the actual name and go with SnmpAdminString.

On Sat, 2021-03-20 at 14:29 +, Stuart Henderson wrote:
> DisplayString seems a better type for pfIfDescr and some of the other
> objects; this improves the display format in Prometheus snmp_exporter
> which uses hex values for OCTET STRING.
> 
> OK?
> 
> 
> Index: OPENBSD-PF-MIB.txt
> ===
> RCS file: /cvs/src/share/snmp/OPENBSD-PF-MIB.txt,v
> retrieving revision 1.6
> diff -u -p -r1.6 OPENBSD-PF-MIB.txt
> --- OPENBSD-PF-MIB.txt  19 Jun 2018 10:08:45 -  1.6
> +++ OPENBSD-PF-MIB.txt  20 Mar 2021 14:24:11 -
> @@ -23,7 +23,7 @@ IMPORTS
> TimeTicks, enterprises
> FROM SNMPv2-SMI
>  
> -   TruthValue
> +   TruthValue, DisplayString
> FROM SNMPv2-TC
> 
> openBSD
> @@ -33,7 +33,7 @@ IMPORTS
> FROM SNMPv2-CONF;
>  
>  pfMIBObjects MODULE-IDENTITY
> -    LAST-UPDATED "201506091728Z"
> +    LAST-UPDATED "202103201421Z"
>  ORGANIZATION "OpenBSD"
>  CONTACT-INFO "
>    Author: Joel Knight
> @@ -43,6 +43,8 @@ pfMIBObjects MODULE-IDENTITY
>  DESCRIPTION "The MIB module for gathering information from
> OpenBSD's packet filter.
>  "
> +    REVISION "202103201421Z"
> +    DESCRIPTION "Use DisplayString not OCTET STRING where appropriate"
>  REVISION "201506091728Z"
>  DESCRIPTION "Add separate counter for failed 'route-to' applications"
>  REVISION "201308310446Z"
> @@ -300,7 +302,7 @@ pfStateRemovals OBJECT-TYPE
>  -- pfLogInterface
>  
>  pfLogIfName OBJECT-TYPE
> -    SYNTAX  OCTET STRING
> +    SYNTAX  DisplayString
>  MAX-ACCESS  read-only
>  STATUS  current
>  DESCRIPTION
> @@ -683,7 +685,7 @@ pfIfEntry OBJECT-TYPE
>  PfIfEntry ::=
> SEQUENCE {
> pfIfIndex   Integer32,
> -   pfIfDescr   OCTET STRING,
> +   pfIfDescr   DisplayString,
> pfIfTypeINTEGER,
> pfIfRefsUnsigned32,
> pfIfRules   Unsigned32,
> @@ -719,7 +721,7 @@ pfIfIndex OBJECT-TYPE
> ::= { pfIfEntry 1 }
>  
>  pfIfDescr OBJECT-TYPE
> -   SYNTAX  OCTET STRING
> +   SYNTAX  DisplayString
> MAX-ACCESS  read-only
> STATUS  current
> DESCRIPTION
> @@ -913,7 +915,7 @@ pfTblEntry OBJECT-TYPE
>  TblEntry ::=
> SEQUENCE {
> pfTblIndex  Integer32,
> -   pfTblName   OCTET STRING,
> +   pfTblName   DisplayString,
> pfTblAddresses  Integer32,
> pfTblAnchorRefs Integer32,
> pfTblRuleRefs   Integer32,
> @@ -947,7 +949,7 @@ pfTblIndex OBJECT-TYPE
> ::= { pfTblEntry 1 }
>  
>  pfTblName OBJECT-TYPE
> -   SYNTAX  OCTET STRING
> +   SYNTAX  DisplayString
> MAX-ACCESS  read-only
> STATUS  current
> DESCRIPTION
> @@ -1363,7 +1365,7 @@ pfLabelEntry 

Re: httpd.conf grammar

2021-03-21 Thread Laurence Tratt
On Sun, Mar 21, 2021 at 10:58:02PM +, Raf Czlonka wrote:

Hello Raf,

> I'd simply use the existing wording, without getting into details.
>
> While there, "braces" dominate the manual page, with a single occurrence of
> "brackets" so let's change that too, for consistency.

That works for me!


Laurie



Re: httpd.conf grammar

2021-03-21 Thread Raf Czlonka
On Sun, Mar 21, 2021 at 05:08:00PM GMT, Laurence Tratt wrote:
> I wanted to use httpd's fastcgi "socket" and "strip" options and based upon
> the man page's brief text:
> 
>  [no] fastcgi [option]
>  Enable FastCGI instead of serving files.  Valid options are:
> 
> tried "obvious" permutations such as:
> 
>   fastcgi strip 1 socket "..."
>   fastcgi socket "..." strip 1
>   fastcgi socket "...", strip 1
> 
> but with each was greeted by a terse "syntax error".
> 
> After hunting around in the relevant parse.y file, it transpires that the
> grammar allows, roughly speaking, the following:
> 
>   fastcgi
>   fastcgi option
>   fastcgi { option ((',' '\n'? | '\n') option)* }
> 
> In other words, if you want to use more than one option you *have* to use
> the {...} notation, but there's more than one way for options inside curly
> brackets to be separated. In my case I can specify:
> 
>   fastcgi {
> socket "..."
> strip 1
>   }
> 
> or:
> 
>   fastcgi {
> socket "...", strip 1
>   }
> 
> or:
> 
>   fastcgi {
> socket "...",
> strip 1
>   }
> 
> This raised a couple of questions in my mind.
> 
> First, stylistically, I'm not quite sure if having three slightly different
> ways of separating multiple options is useful or not. That said, I assume
> that some people might already be taking advantage of this flexibility, so
> perhaps worrying about it now is pointless.
> 
> Second, is it worthwhile giving users a hint about what to do when multiple
> options need to be specified? For example, something like:
> 
>  [no] fastcgi [option]
>  Enable FastCGI instead of serving files.  If more than option
>  is specified, they must be included inside { ... }, with each
>  option separated by a comma or newline.  Valid options are:
> 
> I'm happy to raise a patch if other people think this is worth fixing,
> although I'm not entirely sure if we want to make people aware of the full
> extent of the grammar, or something a little less complete such as the
> suggestion above.
> 
> 
> Laurie
> 

Hi Laurie,

I'd simply use the existing wording, without getting into details.

While there, "braces" dominate the manual page, with a single
occurrence of "brackets" so let's change that too, for consistency.

Regards,

Raf

Index: usr.sbin/httpd/httpd.conf.5
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.conf.5,v
retrieving revision 1.114
diff -u -p -r1.114 httpd.conf.5
--- usr.sbin/httpd/httpd.conf.5 29 Oct 2020 12:30:52 -  1.114
+++ usr.sbin/httpd/httpd.conf.5 21 Mar 2021 22:56:53 -
@@ -155,7 +155,7 @@ see
 .Xr patterns 7 .
 .El
 .Pp
-Followed by a block of options that is enclosed in curly brackets:
+Followed by a block of options that is enclosed in curly braces:
 .Bl -tag -width Ds
 .It Ic alias Ar name
 Specify an additional alias
@@ -282,6 +282,7 @@ will neither display nor generate a dire
 .El
 .It Oo Ic no Oc Ic fastcgi Oo Ar option Oc
 Enable FastCGI instead of serving files.
+Multiple options may be specified within curly braces.
 Valid options are:
 .Bl -tag -width Ds
 .It Ic socket Oo Cm tcp Oc Ar socket Oo Ar port Oc



Re: httpd.conf grammar

2021-03-21 Thread Andrew Grillet
I favour a fuller explanation, this one is fine by me.

On Sun, 21 Mar 2021 at 17:20, Laurence Tratt  wrote:
>
> I wanted to use httpd's fastcgi "socket" and "strip" options and based upon
> the man page's brief text:
>
>  [no] fastcgi [option]
>  Enable FastCGI instead of serving files.  Valid options are:
>
> tried "obvious" permutations such as:
>
>   fastcgi strip 1 socket "..."
>   fastcgi socket "..." strip 1
>   fastcgi socket "...", strip 1
>
> but with each was greeted by a terse "syntax error".
>
> After hunting around in the relevant parse.y file, it transpires that the
> grammar allows, roughly speaking, the following:
>
>   fastcgi
>   fastcgi option
>   fastcgi { option ((',' '\n'? | '\n') option)* }
>
> In other words, if you want to use more than one option you *have* to use
> the {...} notation, but there's more than one way for options inside curly
> brackets to be separated. In my case I can specify:
>
>   fastcgi {
> socket "..."
> strip 1
>   }
>
> or:
>
>   fastcgi {
> socket "...", strip 1
>   }
>
> or:
>
>   fastcgi {
> socket "...",
> strip 1
>   }
>
> This raised a couple of questions in my mind.
>
> First, stylistically, I'm not quite sure if having three slightly different
> ways of separating multiple options is useful or not. That said, I assume
> that some people might already be taking advantage of this flexibility, so
> perhaps worrying about it now is pointless.
>
> Second, is it worthwhile giving users a hint about what to do when multiple
> options need to be specified? For example, something like:
>
>  [no] fastcgi [option]
>  Enable FastCGI instead of serving files.  If more than option
>  is specified, they must be included inside { ... }, with each
>  option separated by a comma or newline.  Valid options are:
>
> I'm happy to raise a patch if other people think this is worth fixing,
> although I'm not entirely sure if we want to make people aware of the full
> extent of the grammar, or something a little less complete such as the
> suggestion above.
>
>
> Laurie
>



Re: syslogd kernel timestamp

2021-03-21 Thread Scott Cheloha
On Thu, Mar 18, 2021 at 05:57:45PM +0100, Alexander Bluhm wrote:
> Hi,
> 
> Since we stash log messages in the kernel, the timestamps added by
> syslogd are delayed.  The kernel could add the timestamp when it
> receives the message by sendsyslog(2).  This is more precise and
> can be expressed by more digits in the ISO timestamp.

Better timestamp accuracy would be nice here.

The obvious follow-on question is: why not grab a timestamp in
userspace during syslog(3) instead of waiting until we reach the
kernel?

I imagine it might complicate things.  Or maybe the accuracy
improvement is tiny.  Maybe both.  And then you (maybe) have the
additional round trip to the kernel in gettimeofday(2).

All of that aside, I'd like to hear your reasoning for preferring a
kernel timestamp over a userspace timestamp, if only so that there is
some record of your thinking.

> I have to copyin(9) at the beginning of sendsyslog(2) to have both
> kernel timestamp and log message in kernel space.  This makes the
> code easier, but requires a malloc(9) for each log message.
> 
> Do we want to go into this direction?
> 
> Note that old syslogd and new kernel produces ugly messages.
> Do we need an update path?

I can't speak on any of these items.  I do have other questions + nits
below.

> Index: sys/kern/subr_log.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/subr_log.c,v
> retrieving revision 1.74
> diff -u -p -r1.74 subr_log.c
> --- sys/kern/subr_log.c   18 Mar 2021 08:43:38 -  1.74
> +++ sys/kern/subr_log.c   18 Mar 2021 13:06:13 -
> @@ -108,7 +108,7 @@ const struct filterops logread_filtops =
>   .f_event= filt_logread,
>  };
>  
> -int dosendsyslog(struct proc *, const char *, size_t, int, enum uio_seg);
> +int dosendsyslog(struct proc *, char *, size_t, int, struct timeval *);
>  void logtick(void *);
>  size_t msgbuf_getlen(struct msgbuf *);
>  void msgbuf_putchar_locked(struct msgbuf *, const char);
> @@ -468,14 +468,15 @@ logioctl(dev_t dev, u_long com, caddr_t 
>   * If syslogd is not running, temporarily store a limited amount of messages
>   * in kernel.  After log stash is full, drop messages and count them.  When
>   * syslogd is available again, next log message will flush the stashed
> - * messages and insert a message with drop count.  Calls to malloc(9) and
> - * copyin(9) may sleep, protect data structures with rwlock.
> + * messages and insert a message with drop count.  Calls to malloc(9) may
> + * sleep, protect data structures with rwlock.
>   */
>  
>  #define LOGSTASH_SIZE100
>  struct logstash_message {
>   char*lgs_buffer;
>   size_t   lgs_size;
> + struct   timeval lgs_time;

Is there any reason to use a timeval here over a timespec?

My preference is that new code use nanosecond precision unless there
is a strong historical reason to a timeval.

We have timecounters with nanosecond resolution.  New syscalls will
all use timespecs or nanosecond resolution.  Etc.

The only code I've seen with said strong historical reason is in
sys/net* because the BSD networking stack predates POSIX and the
timespec.  So all networking ioctls use timevals because it's
tradition to do so.

I see that syslogd(8) is using gettimeofday(2) and is already using
timevals in various spots.  I don't know how painful it would be to
change syslogd(8) to use a timespec, but the main goal in grabbing the
timestamp in the kernel is better accuracy so I think it'd make sense
to rip the bandaid off now and switch to nanosecond resolution while
all this code is being modified to accomodate the kernel timestamp.

Or maybe not.  What do you think?

>  } logstash_messages[LOGSTASH_SIZE];
>  
>  struct   logstash_message *logstash_in = _messages[0];
> @@ -485,9 +486,9 @@ structrwlock logstash_rwlock = RWLOCK_I
>  
>  int  logstash_dropped, logstash_error, logstash_pid;
>  
> -int  logstash_insert(const char *, size_t, int, pid_t);
> -void logstash_remove(void);
> -int  logstash_sendsyslog(struct proc *);
> +void logstash_insert(char *, size_t, int, pid_t, struct timeval *);
> +void logstash_remove(struct timeval *);
> +int  logstash_sendsyslog(struct proc *, struct timeval *);
>  
>  static inline int
>  logstash_full(void)
> @@ -511,11 +512,10 @@ logstash_increment(struct logstash_messa
>   (*msg)++;
>  }
>  
> -int
> -logstash_insert(const char *buf, size_t nbyte, int logerror, pid_t pid)
> +void
> +logstash_insert(char *buffer, size_t nbyte, int logerror, pid_t pid,
> +struct timeval *now)
>  {
> - int error;
> -
>   rw_enter_write(_rwlock);
>  
>   if (logstash_full()) {
> @@ -524,29 +524,22 @@ logstash_insert(const char *buf, size_t 
>   logstash_pid = pid;
>   }
>   logstash_dropped++;
> + free(buffer, M_LOG, nbyte);
>  
>   rw_exit(_rwlock);
> - return (0);
> + return;
> 

Re: arm64: make cwfg(4) report battery information to apm(4)

2021-03-21 Thread Theo de Raadt
The sensor framework generally does not know where a sensor is.  A
sensor could reside on some device which has been plugged in, rather
than be the primary sensor.

But the users of apm are only hoping for best effort.

meaning "some information", not "perfect information".  Many PC BIOS
have lied in these fields before also, yet all the apm command users
survived.

So which is better:

(1) try to emit some information for the people who quicky-use the apm interface

(2) change apm to not print those lines on architectures where we are unsure.

I think (1) is acceptable for a tool which has never promised perfect accuracy. 

I suspect hard-wiring this to one driver is going to be better than scanning
the sensor list and heuristically determining which specific sensors to look at,
because the only good selector now is strcmp(sensor->desc, "battery remaining 
minutes")
yuck.


Mark Kettenis  wrote:

> > Date: Sun, 21 Mar 2021 17:22:05 +0100
> > From: Klemens Nanni 
> > 
> > On Sun, Mar 21, 2021 at 02:02:00PM +0100, Mark Kettenis wrote:
> > > > Date: Sun, 21 Mar 2021 01:02:53 +0100
> > > > From: Klemens Nanni 
> > > > 
> > > > apm(4/arm64) merely provides an all zero/unknown stub for those values,
> > > > e.g. apm(8) output is useless.
> > > > 
> > > > Hardware sensors however provide the information:
> > > > 
> > > > $ sysctl hw.sensors
> > > > hw.sensors.rktemp0.temp0=32.22 degC (CPU)
> > > > hw.sensors.rktemp0.temp1=33.89 degC (GPU)
> > > > hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage)
> > > > hw.sensors.cwfg0.raw0=259 (battery remaining minutes)
> > > > hw.sensors.cwfg0.percent0=58.00% (battery percent)
> > > > 
> > > > Diff below simply copies them over using apm_setinfohook()
> > > > (I've looked at sys/arch/loongson/dev/kb3310.c which does the same):
> > > > 
> > > > $ apm
> > > > Battery state: high, 58% remaining, 259 minutes life estimate
> > > > A/C adapter state: not known
> > > > Performance adjustment mode: auto (408 MHz)
> > > > 
> > > > Feedback? OK?
> > > 
> > > This doesn't scale.
> > What exactly does not scale?  The way how various drivers hook into
> > apm(4)?  I'm not familiar enough (yet) with the framework(s) to judge
> > their overall design and/or interop strategy off hand.
> 
> Since there is no firmware abstraction like on i386 (and to some
> extent amd64) there will be many drivers that provide battery
> informarion on armv7/arm64.
> 
> > > The whole apm(4) interface is too closely tied to the original APM
> > > BIOS model.  Even with acpi(4) it is a bit of a square peg for a round
> > > hole, for example on machines with more than one battery.
> > Right, there is hardware which doesn't fit this model and our apm* is
> > not exactly up to speed with those, but I still don't see actual issues
> > (as per above).
> > 
> > > I'm not even sure apm(8) should bother reporting some sort of battery
> > > status.
> > apm* showing battery information seems only naturally to me since that
> > is the driver/daemon/tool one issues power commands with, often based
> > on such information - think of `apmd -z 15':  given that I want my box
> > to safely suspend around 15% remaining battery, it seems only
> > consequential that apm* is the same tool I query such information with.
> 
> apmd(8) and apm(8) are not the same tool though.  Having a deamon to
> run scripts on certain power-state changes makes some sense, but I'd
> argue that sensorsd(8) could be a reasonable place to implement this
> as well.
> 
> The need to run apmd(8) in order to suspend/hibernate is a bit of an
> historical wart.  And we already removed powermanagement state support
> from apmd(8).  These days, users should probably just use
> /etc/sysctl.conf to set the powermanagement state instead of starting
> apmd(8) with the appropriate -A, -H or -L option.
> 
> > > But if it does, my suggestion would be to make it use the
> > > sensors framework.  That would need some work though such that drivers
> > > can mark sensors as providing battery information.  Maybe add a
> > > SENSOR_FBATTERY flag?
> > You mean apm(8) directly using sensor_find(9) or so to look for
> > suitable sensors instead of relaying values throuh apm(4)?
> 
> That's a possibility.  But having apm(4) do that might be a useful
> middle ground as it would mean you don't have to change the userland
> tools just yet.  My main concern with your diff is having to add APM
> hooks in all the drivers...
> 
> > It sounds simpler in principal but that's just my naive guess;
> > I'll take a curious look into whether/how this could work.
> 



Re: arm64: make cwfg(4) report battery information to apm(4)

2021-03-21 Thread Mark Kettenis
> Date: Sun, 21 Mar 2021 17:22:05 +0100
> From: Klemens Nanni 
> 
> On Sun, Mar 21, 2021 at 02:02:00PM +0100, Mark Kettenis wrote:
> > > Date: Sun, 21 Mar 2021 01:02:53 +0100
> > > From: Klemens Nanni 
> > > 
> > > apm(4/arm64) merely provides an all zero/unknown stub for those values,
> > > e.g. apm(8) output is useless.
> > > 
> > > Hardware sensors however provide the information:
> > > 
> > >   $ sysctl hw.sensors
> > >   hw.sensors.rktemp0.temp0=32.22 degC (CPU)
> > >   hw.sensors.rktemp0.temp1=33.89 degC (GPU)
> > >   hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage)
> > >   hw.sensors.cwfg0.raw0=259 (battery remaining minutes)
> > >   hw.sensors.cwfg0.percent0=58.00% (battery percent)
> > > 
> > > Diff below simply copies them over using apm_setinfohook()
> > > (I've looked at sys/arch/loongson/dev/kb3310.c which does the same):
> > > 
> > >   $ apm
> > >   Battery state: high, 58% remaining, 259 minutes life estimate
> > >   A/C adapter state: not known
> > >   Performance adjustment mode: auto (408 MHz)
> > > 
> > > Feedback? OK?
> > 
> > This doesn't scale.
> What exactly does not scale?  The way how various drivers hook into
> apm(4)?  I'm not familiar enough (yet) with the framework(s) to judge
> their overall design and/or interop strategy off hand.

Since there is no firmware abstraction like on i386 (and to some
extent amd64) there will be many drivers that provide battery
informarion on armv7/arm64.

> > The whole apm(4) interface is too closely tied to the original APM
> > BIOS model.  Even with acpi(4) it is a bit of a square peg for a round
> > hole, for example on machines with more than one battery.
> Right, there is hardware which doesn't fit this model and our apm* is
> not exactly up to speed with those, but I still don't see actual issues
> (as per above).
> 
> > I'm not even sure apm(8) should bother reporting some sort of battery
> > status.
> apm* showing battery information seems only naturally to me since that
> is the driver/daemon/tool one issues power commands with, often based
> on such information - think of `apmd -z 15':  given that I want my box
> to safely suspend around 15% remaining battery, it seems only
> consequential that apm* is the same tool I query such information with.

apmd(8) and apm(8) are not the same tool though.  Having a deamon to
run scripts on certain power-state changes makes some sense, but I'd
argue that sensorsd(8) could be a reasonable place to implement this
as well.

The need to run apmd(8) in order to suspend/hibernate is a bit of an
historical wart.  And we already removed powermanagement state support
from apmd(8).  These days, users should probably just use
/etc/sysctl.conf to set the powermanagement state instead of starting
apmd(8) with the appropriate -A, -H or -L option.

> > But if it does, my suggestion would be to make it use the
> > sensors framework.  That would need some work though such that drivers
> > can mark sensors as providing battery information.  Maybe add a
> > SENSOR_FBATTERY flag?
> You mean apm(8) directly using sensor_find(9) or so to look for
> suitable sensors instead of relaying values throuh apm(4)?

That's a possibility.  But having apm(4) do that might be a useful
middle ground as it would mean you don't have to change the userland
tools just yet.  My main concern with your diff is having to add APM
hooks in all the drivers...

> It sounds simpler in principal but that's just my naive guess;
> I'll take a curious look into whether/how this could work.



httpd.conf grammar

2021-03-21 Thread Laurence Tratt
I wanted to use httpd's fastcgi "socket" and "strip" options and based upon
the man page's brief text:

 [no] fastcgi [option]
 Enable FastCGI instead of serving files.  Valid options are:

tried "obvious" permutations such as:

  fastcgi strip 1 socket "..."
  fastcgi socket "..." strip 1
  fastcgi socket "...", strip 1

but with each was greeted by a terse "syntax error".

After hunting around in the relevant parse.y file, it transpires that the
grammar allows, roughly speaking, the following:

  fastcgi
  fastcgi option
  fastcgi { option ((',' '\n'? | '\n') option)* }

In other words, if you want to use more than one option you *have* to use
the {...} notation, but there's more than one way for options inside curly
brackets to be separated. In my case I can specify:

  fastcgi {
socket "..."
strip 1
  }

or:

  fastcgi {
socket "...", strip 1
  }

or:

  fastcgi {
socket "...",
strip 1
  }

This raised a couple of questions in my mind.

First, stylistically, I'm not quite sure if having three slightly different
ways of separating multiple options is useful or not. That said, I assume
that some people might already be taking advantage of this flexibility, so
perhaps worrying about it now is pointless.

Second, is it worthwhile giving users a hint about what to do when multiple
options need to be specified? For example, something like:

 [no] fastcgi [option]
 Enable FastCGI instead of serving files.  If more than option
 is specified, they must be included inside { ... }, with each
 option separated by a comma or newline.  Valid options are:

I'm happy to raise a patch if other people think this is worth fixing,
although I'm not entirely sure if we want to make people aware of the full
extent of the grammar, or something a little less complete such as the
suggestion above.


Laurie



Re: apm, apmd: ship manual pages on powerpc64

2021-03-21 Thread Theo de Raadt
Jason McIntyre  wrote:

> On Sat, Mar 20, 2021 at 07:29:11PM -0600, Theo de Raadt wrote:
> > There is a pattern we've followed in the past, that when a manpage applies
> > to more than 2 (or 3?) architectures, then we simply make it MI.
> > 
> > So MANSUBDIR would get deleted, and then the sets would be updated.
> > 
> > People with old systems will retain the old files, but I think man(1)
> > selects the MI ones over the MD ones (I think we have no override 
> > capability).
> > 
> 
> man chooses the MD page over the MI page, i think. from man(1):
> 
>   Machine specific areas are searched before general areas.
> 
> so you'd get the old page unless you deleted it. i don;t know if you can
> override that.

oh well, eventually people reinstall.  noone is going to get severely hurt
because old pages are lying around, we've been here before with far more
important pages, and barreled though anyways, because it makes the build
system simpler.



Re: arm64: make cwfg(4) report battery information to apm(4)

2021-03-21 Thread Klemens Nanni
On Sun, Mar 21, 2021 at 02:02:00PM +0100, Mark Kettenis wrote:
> > Date: Sun, 21 Mar 2021 01:02:53 +0100
> > From: Klemens Nanni 
> > 
> > apm(4/arm64) merely provides an all zero/unknown stub for those values,
> > e.g. apm(8) output is useless.
> > 
> > Hardware sensors however provide the information:
> > 
> > $ sysctl hw.sensors
> > hw.sensors.rktemp0.temp0=32.22 degC (CPU)
> > hw.sensors.rktemp0.temp1=33.89 degC (GPU)
> > hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage)
> > hw.sensors.cwfg0.raw0=259 (battery remaining minutes)
> > hw.sensors.cwfg0.percent0=58.00% (battery percent)
> > 
> > Diff below simply copies them over using apm_setinfohook()
> > (I've looked at sys/arch/loongson/dev/kb3310.c which does the same):
> > 
> > $ apm
> > Battery state: high, 58% remaining, 259 minutes life estimate
> > A/C adapter state: not known
> > Performance adjustment mode: auto (408 MHz)
> > 
> > Feedback? OK?
> 
> This doesn't scale.
What exactly does not scale?  The way how various drivers hook into
apm(4)?  I'm not familiar enough (yet) with the framework(s) to judge
their overall design and/or interop strategy off hand.

> The whole apm(4) interface is too closely tied to the original APM
> BIOS model.  Even with acpi(4) it is a bit of a square peg for a round
> hole, for example on machines with more than one battery.
Right, there is hardware which doesn't fit this model and our apm* is
not exactly up to speed with those, but I still don't see actual issues
(as per above).

> I'm not even sure apm(8) should bother reporting some sort of battery
> status.
apm* showing battery information seems only naturally to me since that
is the driver/daemon/tool one issues power commands with, often based
on such information - think of `apmd -z 15':  given that I want my box
to safely suspend around 15% remaining battery, it seems only
consequential that apm* is the same tool I query such information with.

> But if it does, my suggestion would be to make it use the
> sensors framework.  That would need some work though such that drivers
> can mark sensors as providing battery information.  Maybe add a
> SENSOR_FBATTERY flag?
You mean apm(8) directly using sensor_find(9) or so to look for
suitable sensors instead of relaying values throuh apm(4)?

It sounds simpler in principal but that's just my naive guess;
I'll take a curious look into whether/how this could work.



Re: fork(2), PT_ATTACH & SIGTRAP

2021-03-21 Thread Martin Pieuchot
On 21/03/21(Sun) 13:42, Mark Kettenis wrote:
> > Date: Sat, 20 Mar 2021 13:10:17 +0100
> > From: Martin Pieuchot 
> > 
> > On SP systems, like bluhm@'s armv7 regression machine, the kern/ptrace2
> > test is failing due to a subtle behavior.  Diff below makes it pass.
> > 
> > http://bluhm.genua.de/regress/results/2021-03-19T15%3A17%3A02Z/logs/sys/kern/ptrace2/make.log
> > 
> > The failing test does a fork(2) and the parent issues a PT_ATTACH on the
> > child before it has been scheduled for the first time.  Then the parent
> > goes to sleep in waitpid() and when the child starts executing the check
> > below overwrites the ptrace(2)-received SIGSTOP by a SIGTRAP.
> > 
> > This scenario doesn't seem to happen on MP machine because the child
> > starts to execute itself on a different core right after sys_fork() is
> > finished.
> > 
> > What is the purpose of this check?  Should it be relaxed or removed?
> 
> This is part of PT_SET_EVENT_MASK/PTRACE_FORK support:
> 
> https://github.com/openbsd/src/commit/f38bed7f869bd3503530c554b4860228ea4e8641
> 
> When reporting of the PTRACE_FORK event is requested, the debugger
> expects to see a SIGTRAP in both the parent and the child.  The code
> expects that the only way to have PS_TRACED set in the child from the
> start is when PTRACE_FORK is requested.  But the failing test shows
> there is a race with PT_ATTACH.

Thanks for the explanation.

> I think the solution is to have fork1() only run fork_return() if the
> FORK_PTRACE flag is set, and use run child_return() otherwise.

Diff below does that and prevent the race, ok?

Index: kern/kern_fork.c
===
RCS file: /cvs/src/sys/kern/kern_fork.c,v
retrieving revision 1.234
diff -u -p -r1.234 kern_fork.c
--- kern/kern_fork.c15 Feb 2021 09:35:59 -  1.234
+++ kern/kern_fork.c21 Mar 2021 15:55:26 -
@@ -95,12 +95,15 @@ fork_return(void *arg)
 int
 sys_fork(struct proc *p, void *v, register_t *retval)
 {
+   void (*func)(void *) = child_return;
int flags;
 
flags = FORK_FORK;
-   if (p->p_p->ps_ptmask & PTRACE_FORK)
+   if (p->p_p->ps_ptmask & PTRACE_FORK) {
flags |= FORK_PTRACE;
-   return fork1(p, flags, fork_return, NULL, retval, NULL);
+   func = fork_return;
+   }
+   return fork1(p, flags, func, NULL, retval, NULL);
 }
 
 int



veb(4) exceeds 1514 byte frame size while bridge(4) doesn't?

2021-03-21 Thread Jurjen Oskam
Hi,

When trying out veb(4), I ran into a situation where TCP sessions across a
veb(4) bridge stalled while the exact same config using bridge(4) worked fine.

After some investigation, it seems that veb(4) adds an FCS to the outgoing
frame, while bridge(4) doesn't. When this causes the outgoing frame to exceed
1514 bytes, the destination doesn't receive it.

I must note that I was using USB NICs, one of them being quite old.

Am I doing something wrong, or is the problem in (one of) the NIC(s)?



The configuration of the veb(4) bridge was as follows (full dmesg below):

openbsd$ ls -l /etc/hostname.*
-rw-r-  1 root  wheel   3 Mar 19 10:08 /etc/hostname.axe0
-rw-r-  1 root  wheel   3 Mar 19 10:08 /etc/hostname.ure0
-rw-r-  1 root  wheel  21 Mar 21 16:16 /etc/hostname.veb0
openbsd$ cat /etc/hostname.axe0
up
openbsd$ cat /etc/hostname.ure0
up
openbsd$ cat /etc/hostname.veb0
add ure0
add axe0
up

The configuration of bridge(4) was exactly the same, I simply renamed
hostname.veb0 to hostname.bridge0 and rebooted.

On the axe(4) side of the bridge was my Windows 10 PC, on the ure(4) side of
the bridge was my home network.

On the Windows 10 PC, I used ICMP echo requests to the default gateway to
diagnose the problem.

Using bridge(4) it went as expected:

* When sending an echo request with a payload of 1473 and DF set, Windows
  wouldn't send the packet: "Packet needs to be fragmented but DF set." 

* Payload 1472 and DF set: no packet loss


Using veb(4), this happened:

* Payload 1473, DF set: "Packet needs to be fragmented" (expected)

* Payload 1472, 1471, 1470 or 1469 and DF set: 100% packet loss (not expected)

* Payload <=1468 and DF set: no packet loss (expected)


On the bridge, I ran Wireshark on axe0 (where the Windows PC was attached). I
noticed that the echo reply was logged by Wireshark when using bridge(4) as
well as when using veb(4). When using bridge(4) the frame had a size of 1514,
but when using veb(4) the frame had a size of 1518. Inspecting the differences
between the frames with the echo response revealed that veb(4) seemed to
insert the FCS (on the right) while bridge(4) doesn't:

Frame 55: 1514 bytes on wire (12112 bi | Frame 34: 1518 bytes on wire (12144 bi
Interface id: 0 (axe0)   Interface id: 0 (axe0)
Interface name: axe0 Interface name: axe0
Encapsulation type: Ethernet (1) Encapsulation type: Ethernet (1)
Arrival Time: Mar 21, 2021 16:21:0 | Arrival Time: Mar 21, 2021 16:29:3
[Time shift for this packet: 0.000   [Time shift for this packet: 0.000
Epoch Time: 1616340062.761312000 s | Epoch Time: 1616340579.977005000 s
[Time delta from previous captured | [Time delta from previous captured
[Time delta from previous displaye   [Time delta from previous displaye
[Time since reference or first fra | [Time since reference or first fra
Frame Number: 55   | Frame Number: 34
Frame Length: 1514 bytes (12112 bi | Frame Length: 1518 bytes (12144 bi
Capture Length: 1514 bytes (12112  | Capture Length: 1518 bytes (12144
[Frame is marked: False] [Frame is marked: False]
[Frame is ignored: False][Frame is ignored: False]
[Protocols in frame: eth:ethertype   [Protocols in frame: eth:ethertype
Ethernet II, Src: Shuttle_f1:11:c1 (80   Ethernet II, Src: Shuttle_f1:11:c1 (80
Destination: Giga-Byt_37:28:2b (18   Destination: Giga-Byt_37:28:2b (18
Address: Giga-Byt_37:28:2b (18   Address: Giga-Byt_37:28:2b (18
 ..0.     ..0.    
 ...0     ...0    
Source: Shuttle_f1:11:c1 (80:ee:73   Source: Shuttle_f1:11:c1 (80:ee:73
Address: Shuttle_f1:11:c1 (80:   Address: Shuttle_f1:11:c1 (80:
 ..0.     ..0.    
 ...0     ...0    
Type: IPv4 (0x0800)  Type: IPv4 (0x0800)
   > Frame check sequence: 0x13a4f5bc [
   > [FCS Status: Unverified]
Internet Protocol Version 4, Src: 192.   Internet Protocol Version 4, Src: 192.
0100  = Version: 4   0100  = Version: 4
 0101 = Header Length: 20 byte    0101 = Header Length: 20 byte
Differentiated Services Field: 0x0   Differentiated Services Field: 0x0
 00.. = Differentiated Ser    00.. = Differentiated Ser
 ..00 = Explicit Congestio    ..00 = Explicit Congestio
Total Length: 1500   Total Length: 1500
Identification: 0xc088 (49288) | Identification: 0x221d (8733)
Flags: 0x40, Don't fragment  Flags: 0x40, Don't fragment
0...  

Re: slaacd(8): pltime 0 and temporary addresses

2021-03-21 Thread Otto Moerbeek
On Sun, Mar 21, 2021 at 02:38:42PM +0100, Florian Obser wrote:

> 
> Don't warn that we can't form a temporary address when a router
> deprecates a prefix by sending a pltime of 0, this is normal.
> Continue warning when the pltime is smaller than 5 as this is almost
> certainly a configuration error.
> 
> OK?

yes,

-Otto

> 
> diff --git engine.c engine.c
> index 7b49b330328..94a4a232d6a 100644
> --- engine.c
> +++ engine.c
> @@ -1932,14 +1932,15 @@ update_iface_ra_prefix(struct slaacd_iface *iface, 
> struct radv *ra,
>  
>   /* privacy addresses do not depend on eui64 */
>   if (!found_privacy && iface->autoconfprivacy) {
> - if (prefix->pltime < PRIV_REGEN_ADVANCE) {
> + if (prefix->pltime >= PRIV_REGEN_ADVANCE) {
> + /* new privacy proposal */
> + gen_address_proposal(iface, ra, prefix, 1);
> + } else if (prefix->pltime > 0) {
>   log_warnx("%s: pltime from %s is too small: %d < %d; "
>   "not generating privacy address", __func__,
>   sin6_to_str(>from), prefix->pltime,
>   PRIV_REGEN_ADVANCE);
> - } else
> - /* new privacy proposal */
> - gen_address_proposal(iface, ra, prefix, 1);
> + }
>   }
>  }
>  
> 
> 
> -- 
> I'm not entirely sure you are real.
> 



slaacd(8): pltime 0 and temporary addresses

2021-03-21 Thread Florian Obser


Don't warn that we can't form a temporary address when a router
deprecates a prefix by sending a pltime of 0, this is normal.
Continue warning when the pltime is smaller than 5 as this is almost
certainly a configuration error.

OK?

diff --git engine.c engine.c
index 7b49b330328..94a4a232d6a 100644
--- engine.c
+++ engine.c
@@ -1932,14 +1932,15 @@ update_iface_ra_prefix(struct slaacd_iface *iface, 
struct radv *ra,
 
/* privacy addresses do not depend on eui64 */
if (!found_privacy && iface->autoconfprivacy) {
-   if (prefix->pltime < PRIV_REGEN_ADVANCE) {
+   if (prefix->pltime >= PRIV_REGEN_ADVANCE) {
+   /* new privacy proposal */
+   gen_address_proposal(iface, ra, prefix, 1);
+   } else if (prefix->pltime > 0) {
log_warnx("%s: pltime from %s is too small: %d < %d; "
"not generating privacy address", __func__,
sin6_to_str(>from), prefix->pltime,
PRIV_REGEN_ADVANCE);
-   } else
-   /* new privacy proposal */
-   gen_address_proposal(iface, ra, prefix, 1);
+   }
}
 }
 


-- 
I'm not entirely sure you are real.



Re: arm64: make cwfg(4) report battery information to apm(4)

2021-03-21 Thread Mark Kettenis
> Date: Sun, 21 Mar 2021 01:02:53 +0100
> From: Klemens Nanni 
> 
> apm(4/arm64) merely provides an all zero/unknown stub for those values,
> e.g. apm(8) output is useless.
> 
> Hardware sensors however provide the information:
> 
>   $ sysctl hw.sensors
>   hw.sensors.rktemp0.temp0=32.22 degC (CPU)
>   hw.sensors.rktemp0.temp1=33.89 degC (GPU)
>   hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage)
>   hw.sensors.cwfg0.raw0=259 (battery remaining minutes)
>   hw.sensors.cwfg0.percent0=58.00% (battery percent)
> 
> Diff below simply copies them over using apm_setinfohook()
> (I've looked at sys/arch/loongson/dev/kb3310.c which does the same):
> 
>   $ apm
>   Battery state: high, 58% remaining, 259 minutes life estimate
>   A/C adapter state: not known
>   Performance adjustment mode: auto (408 MHz)
> 
> Feedback? OK?

This doesn't scale.

The whole apm(4) interface is too closely tied to the original APM
BIOS model.  Even with acpi(4) it is a bit of a square peg for a round
hole, for example on machines with more than one battery.

I'm not even sure apm(8) should bother reporting some sort of battery
status.  But if it does, my suggestion would be to make it use the
sensors framework.  That would need some work though such that drivers
can mark sensors as providing battery information.  Maybe add a
SENSOR_FBATTERY flag?


> Index: dev/fdt/cwfg.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 cwfg.c
> --- dev/fdt/cwfg.c10 Jun 2020 17:51:21 -  1.1
> +++ dev/fdt/cwfg.c20 Mar 2021 23:43:13 -
> @@ -32,12 +32,15 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  
>  #include 
>  
>  #include 
>  
> +#include "apm.h"
> +
>  #define  VERSION_REG 0x00
>  #define  VCELL_HI_REG0x02
>  #define   VCELL_HI_MASK  0x3f
> @@ -119,6 +122,18 @@ struct cfdriver cwfg_cd = {
>   NULL, "cwfg", DV_DULL
>  };
>  
> +int cwfg_apminfo(struct apm_power_info *info);
> +#if NAPM > 0
> +struct apm_power_info cwfg_power;
> +
> +int
> +cwfg_apminfo(struct apm_power_info *info)
> +{
> + bcopy(_power, info, sizeof(*info));
> + return 0;
> +}
> +#endif
> +
>  int
>  cwfg_match(struct device *parent, void *match, void *aux)
>  {
> @@ -202,6 +217,12 @@ cwfg_attach(struct device *parent, struc
>  
>   sensor_task_register(sc, cwfg_update_sensors, 5);
>  
> +#if NAPM > 0
> + /* make sure we have the apm state initialized before apm attaches */
> + cwfg_update_sensors(sc);
> + apm_setinfohook(cwfg_apminfo);
> +#endif
> +
>   printf("\n");
>  }
>  
> @@ -324,6 +345,7 @@ cwfg_update_sensors(void *arg)
>   uint32_t vcell, rtt, tmp;
>   uint8_t val;
>   int error, n;
> + u_char bat_percent;
>  
>   if ((error = cwfg_lock(sc)) != 0)
>   return;
> @@ -348,6 +370,7 @@ cwfg_update_sensors(void *arg)
>   if ((error = cwfg_read(sc, SOC_HI_REG, )) != 0)
>   goto done;
>   if (val != 0xff) {
> + bat_percent = val;
>   sc->sc_sensor[CWFG_SENSOR_SOC].value = val * 1000;
>   sc->sc_sensor[CWFG_SENSOR_SOC].flags &= ~SENSOR_FINVALID;
>   }
> @@ -363,6 +386,14 @@ cwfg_update_sensors(void *arg)
>   sc->sc_sensor[CWFG_SENSOR_RTT].value = rtt;
>   sc->sc_sensor[CWFG_SENSOR_RTT].flags &= ~SENSOR_FINVALID;
>   }
> +
> +#if NAPM > 0
> + cwfg_power.battery_state = bat_percent > sc->sc_alert_level ?
> + APM_BATT_HIGH : APM_BATT_LOW;
> + cwfg_power.ac_state = APM_AC_UNKNOWN;
> + cwfg_power.battery_life = bat_percent;
> + cwfg_power.minutes_left = sc->sc_sensor[CWFG_SENSOR_RTT].value;
> +#endif
>  
>  done:
>   cwfg_unlock(sc);
> 
> 



Re: fork(2), PT_ATTACH & SIGTRAP

2021-03-21 Thread Mark Kettenis
> Date: Sat, 20 Mar 2021 13:10:17 +0100
> From: Martin Pieuchot 
> 
> On SP systems, like bluhm@'s armv7 regression machine, the kern/ptrace2
> test is failing due to a subtle behavior.  Diff below makes it pass.
> 
> http://bluhm.genua.de/regress/results/2021-03-19T15%3A17%3A02Z/logs/sys/kern/ptrace2/make.log
> 
> The failing test does a fork(2) and the parent issues a PT_ATTACH on the
> child before it has been scheduled for the first time.  Then the parent
> goes to sleep in waitpid() and when the child starts executing the check
> below overwrites the ptrace(2)-received SIGSTOP by a SIGTRAP.
> 
> This scenario doesn't seem to happen on MP machine because the child
> starts to execute itself on a different core right after sys_fork() is
> finished.
> 
> What is the purpose of this check?  Should it be relaxed or removed?

This is part of PT_SET_EVENT_MASK/PTRACE_FORK support:

https://github.com/openbsd/src/commit/f38bed7f869bd3503530c554b4860228ea4e8641

When reporting of the PTRACE_FORK event is requested, the debugger
expects to see a SIGTRAP in both the parent and the child.  The code
expects that the only way to have PS_TRACED set in the child from the
start is when PTRACE_FORK is requested.  But the failing test shows
there is a race with PT_ATTACH.

I think the solution is to have fork1() only run fork_return() if the
FORK_PTRACE flag is set, and use run child_return() otherwise.


> Index: kern/kern_fork.c
> ===
> RCS file: /cvs/src/sys/kern/kern_fork.c,v
> retrieving revision 1.234
> diff -u -p -r1.234 kern_fork.c
> --- kern/kern_fork.c  15 Feb 2021 09:35:59 -  1.234
> +++ kern/kern_fork.c  20 Mar 2021 11:59:18 -
> @@ -86,9 +86,6 @@ fork_return(void *arg)
>  {
>   struct proc *p = (struct proc *)arg;
>  
> - if (p->p_p->ps_flags & PS_TRACED)
> - psignal(p, SIGTRAP);
> -
>   child_return(p);
>  }
>  
> 
> 



Re: apm, apmd: ship manual pages on powerpc64

2021-03-21 Thread Klemens Nanni
On Sun, Mar 21, 2021 at 06:50:23AM +, Jason McIntyre wrote:
> On Sat, Mar 20, 2021 at 07:29:11PM -0600, Theo de Raadt wrote:
> > There is a pattern we've followed in the past, that when a manpage applies
> > to more than 2 (or 3?) architectures, then we simply make it MI.
> > 
> > So MANSUBDIR would get deleted, and then the sets would be updated.
That makes sense.

> > People with old systems will retain the old files, but I think man(1)
> > selects the MI ones over the MD ones (I think we have no override 
> > capability).
> > 
> 
> man chooses the MD page over the MI page, i think. from man(1):
> 
>   Machine specific areas are searched before general areas.
> 
> so you'd get the old page unless you deleted it. i don;t know if you can
> override that.
Isn't that what we have current.html and sysclean(1) for?

If so, I'd commit the following and mention the MD manuals for removal
in current.html just like we do when removing old files after upgrading
perl or so.

Feedback? OK?


Index: usr.sbin/apm/Makefile
===
RCS file: /cvs/src/usr.sbin/apm/Makefile,v
retrieving revision 1.19
diff -u -p -r1.19 Makefile
--- usr.sbin/apm/Makefile   15 Jul 2020 22:46:52 -  1.19
+++ usr.sbin/apm/Makefile   21 Mar 2021 12:21:33 -
@@ -19,6 +19,5 @@ NOPROG=yes
 .endif
 
 MAN=   apm.8
-MANSUBDIR=arm64 amd64 i386 loongson macppc sparc64
 
 .include 
Index: usr.sbin/apmd/Makefile
===
RCS file: /cvs/src/usr.sbin/apmd/Makefile,v
retrieving revision 1.15
diff -u -p -r1.15 Makefile
--- usr.sbin/apmd/Makefile  15 Jul 2020 22:46:52 -  1.15
+++ usr.sbin/apmd/Makefile  21 Mar 2021 12:23:15 -
@@ -13,6 +13,5 @@ NOPROG=yes
 .endif
 
 MAN=   apmd.8
-MANSUBDIR= arm64 amd64 i386 loongson macppc sparc64
 
 .include 
Index: distrib/sets/lists/man/mi
===
RCS file: /cvs/src/distrib/sets/lists/man/mi,v
retrieving revision 1.1616
diff -u -p -r1.1616 mi
--- distrib/sets/lists/man/mi   1 Mar 2021 23:26:59 -   1.1616
+++ distrib/sets/lists/man/mi   21 Mar 2021 12:21:03 -
@@ -2296,8 +2296,6 @@
 ./usr/share/man/man8/alpha/setnetbootinfo.8
 ./usr/share/man/man8/amd.8
 ./usr/share/man/man8/amd64/MAKEDEV.8
-./usr/share/man/man8/amd64/apm.8
-./usr/share/man/man8/amd64/apmd.8
 ./usr/share/man/man8/amd64/biosboot.8
 ./usr/share/man/man8/amd64/boot.8
 ./usr/share/man/man8/amd64/boot_amd64.8
@@ -2307,9 +2305,9 @@
 ./usr/share/man/man8/amd64/memconfig.8
 ./usr/share/man/man8/amd64/pxeboot.8
 ./usr/share/man/man8/amq.8
+./usr/share/man/man8/apm.8
+./usr/share/man/man8/apmd.8
 ./usr/share/man/man8/arm64/MAKEDEV.8
-./usr/share/man/man8/arm64/apm.8
-./usr/share/man/man8/arm64/apmd.8
 ./usr/share/man/man8/arm64/eeprom.8
 ./usr/share/man/man8/arm64/gpioctl.8
 ./usr/share/man/man8/armv7/MAKEDEV.8
@@ -2383,8 +2381,6 @@
 ./usr/share/man/man8/hppa/mkboot.8
 ./usr/share/man/man8/httpd.8
 ./usr/share/man/man8/i386/MAKEDEV.8
-./usr/share/man/man8/i386/apm.8
-./usr/share/man/man8/i386/apmd.8
 ./usr/share/man/man8/i386/biosboot.8
 ./usr/share/man/man8/i386/boot.8
 ./usr/share/man/man8/i386/boot_i386.8
@@ -2431,15 +2427,11 @@
 ./usr/share/man/man8/login_token.8
 ./usr/share/man/man8/login_yubikey.8
 ./usr/share/man/man8/loongson/MAKEDEV.8
-./usr/share/man/man8/loongson/apm.8
-./usr/share/man/man8/loongson/apmd.8
 ./usr/share/man/man8/lpc.8
 ./usr/share/man/man8/lpd.8
 ./usr/share/man/man8/luna88k/MAKEDEV.8
 ./usr/share/man/man8/luna88k/boot_luna88k.8
 ./usr/share/man/man8/macppc/MAKEDEV.8
-./usr/share/man/man8/macppc/apm.8
-./usr/share/man/man8/macppc/apmd.8
 ./usr/share/man/man8/macppc/boot.8
 ./usr/share/man/man8/macppc/boot_macppc.8
 ./usr/share/man/man8/macppc/eeprom.8
@@ -2583,8 +2575,6 @@
 ./usr/share/man/man8/spamdb.8
 ./usr/share/man/man8/spamlogd.8
 ./usr/share/man/man8/sparc64/MAKEDEV.8
-./usr/share/man/man8/sparc64/apm.8
-./usr/share/man/man8/sparc64/apmd.8
 ./usr/share/man/man8/sparc64/boot_sparc64.8
 ./usr/share/man/man8/sparc64/eeprom.8
 ./usr/share/man/man8/sparc64/ldomctl.8



Re: [OpenBSD -current] Change event timer in main loop with kqueue

2021-03-21 Thread Visa Hankala
On Sat, Feb 27, 2021 at 01:36:29PM +, Visa Hankala wrote:
> The kernel does not reschedule the timer when the user changes the
> timeout period. The new period will take effect only after the current
> period has expired. This is not explained in the manual page, though.
> 
> With the recent kqueue changes, it is straightforward to make the kernel
> modify an existing timer. I think the clearest behaviour is to reset the
> timer completely when it is modified. If there are pending events, they
> should be cancelled because they do not necessarily correspond to the
> new settings.
> 
> When f_modify and f_process are present in kqread_filtops, filt_timer
> is not used. filt_timerexpire() activates timer knotes directly using
> knote_activate() instead of KNOTE().
> 
> However, the current behaviour has been around so long that one can
> argue that it is an actual feature. BSDs are not consistent with this,
> though. FreeBSD resets the timer immediately, whereas NetBSD and
> DragonFly BSD apply the new period after expiry.
> 
> I guess the resetting is harmless in most cases but might wreak havoc
> at least with software that keeps poking its timers before expiry.

I have received too little feedback to commit this.

The most important question is, should the timer behaviour be changed?

> Index: lib/libc/sys/kqueue.2
> ===
> RCS file: src/lib/libc/sys/kqueue.2,v
> retrieving revision 1.43
> diff -u -p -r1.43 kqueue.2
> --- lib/libc/sys/kqueue.2 14 Nov 2020 10:16:15 -  1.43
> +++ lib/libc/sys/kqueue.2 27 Feb 2021 12:54:27 -
> @@ -468,6 +468,11 @@ contains the number of times the timeout
>  This filter automatically sets the
>  .Dv EV_CLEAR
>  flag internally.
> +.Pp
> +If an existing timer is re-added, the existing timer and related pending 
> events
> +will be cancelled.
> +The timer will be re-started using the timeout period
> +.Fa data .
>  .It Dv EVFILT_DEVICE
>  Takes a descriptor as the identifier and the events to watch for in
>  .Fa fflags ,
> Index: sys/kern/kern_event.c
> ===
> RCS file: src/sys/kern/kern_event.c,v
> retrieving revision 1.161
> diff -u -p -r1.161 kern_event.c
> --- sys/kern/kern_event.c 24 Feb 2021 14:59:52 -  1.161
> +++ sys/kern/kern_event.c 27 Feb 2021 12:54:27 -
> @@ -135,7 +135,8 @@ int   filt_fileattach(struct knote *kn);
>  void filt_timerexpire(void *knx);
>  int  filt_timerattach(struct knote *kn);
>  void filt_timerdetach(struct knote *kn);
> -int  filt_timer(struct knote *kn, long hint);
> +int  filt_timermodify(struct kevent *kev, struct knote *kn);
> +int  filt_timerprocess(struct knote *kn, struct kevent *kev);
>  void filt_seltruedetach(struct knote *kn);
>  
>  const struct filterops kqread_filtops = {
> @@ -163,7 +164,9 @@ const struct filterops timer_filtops = {
>   .f_flags= 0,
>   .f_attach   = filt_timerattach,
>   .f_detach   = filt_timerdetach,
> - .f_event= filt_timer,
> + .f_event= NULL,
> + .f_modify   = filt_timermodify,
> + .f_process  = filt_timerprocess,
>  };
>  
>  struct   pool knote_pool;
> @@ -444,15 +447,48 @@ filt_timerdetach(struct knote *kn)
>   struct timeout *to;
>  
>   to = (struct timeout *)kn->kn_hook;
> - timeout_del(to);
> + timeout_del_barrier(to);
>   free(to, M_KEVENT, sizeof(*to));
>   kq_ntimeouts--;
>  }
>  
>  int
> -filt_timer(struct knote *kn, long hint)
> +filt_timermodify(struct kevent *kev, struct knote *kn)
> +{
> + struct timeout *to = kn->kn_hook;
> + int s;
> +
> + /* Reset the timer. Any pending events are discarded. */
> +
> + timeout_del_barrier(to);
> +
> + s = splhigh();
> + if (kn->kn_status & KN_QUEUED)
> + knote_dequeue(kn);
> + kn->kn_status &= ~KN_ACTIVE;
> + splx(s);
> +
> + kn->kn_data = 0;
> + knote_modify(kev, kn);
> + /* Reinit timeout to invoke tick adjustment again. */
> + timeout_set(to, filt_timerexpire, kn);
> + filt_timer_timeout_add(kn);
> +
> + return (0);
> +}
> +
> +int
> +filt_timerprocess(struct knote *kn, struct kevent *kev)
>  {
> - return (kn->kn_data != 0);
> + int active, s;
> +
> + s = splsoftclock();
> + active = (kn->kn_data != 0);
> + if (active)
> + knote_submit(kn, kev);
> + splx(s);
> +
> + return (active);
>  }
>  
>  



Re: syslogd kernel timestamp

2021-03-21 Thread Visa Hankala
On Thu, Mar 18, 2021 at 05:57:45PM +0100, Alexander Bluhm wrote:
> Since we stash log messages in the kernel, the timestamps added by
> syslogd are delayed.  The kernel could add the timestamp when it
> receives the message by sendsyslog(2).  This is more precise and
> can be expressed by more digits in the ISO timestamp.
> 
> I have to copyin(9) at the beginning of sendsyslog(2) to have both
> kernel timestamp and log message in kernel space.  This makes the
> code easier, but requires a malloc(9) for each log message.
> 
> Do we want to go into this direction?

I think this makes the code nicer. It is good that the userspace-kernel
copying becomes simpler and more uniform.

> Note that old syslogd and new kernel produces ugly messages.
> Do we need an update path?

I don't think it is very important. The appearance of the kernel
timestamp might confuse things that parse logs. However, as sysupgrade
and manual upgrade through bsd.rd upgrade both the kernel and syslogd
at the same time, the issue does not seem big.

Actually, the increase of timestamp precision might have a higher risk.
To keep timestamp format consistent, and maybe easier for parsers,
shouldn't syslogd always use microsecond precision with -Z?



Re: iwn(4): clear frames before firmware's BA window only

2021-03-21 Thread Paco Esteban
On Thu, 18 Mar 2021, Stefan Sperling wrote:

> Our iwn(4) driver tries to be a bit smart and clears already acknowledged
> frames from Tx aggregation queues, even if those frames are still in the
> firmware's current block ack window.
> 
> Instead, the driver can simply clear frames before the firmware's BA window.
> This matches what the Linux driver does and makes our driver code simpler.
> 
> The firmware, not our driver/stack, controls the BA window and shifts it
> forwards. our driver and stack can simply follow along. The firmware should
> be smart enough to avoid needless retransmissions of frames which have
> already been acknowledged.
> 
> Also, our Tx rate control code relies on sequence numbers falling into
> the BA window, so skip rate control for frames before this window.
> 
> Please test this patch on iwn devices and check for regressions. Thanks!
> iwn's Tx aggregation code will eventually be ported to iwm(4), so testing
> this patch will benefit iwm(4) indirectly.

Tested on a t430s.  All seems good to me.  Also did a suspend/resume
changing APs without issues (I do this quite often at home).  APs used
were a Linksys E1200 which gives me around 35Mbps both directions (more
or less like before) and a Ubiquiti AC Pro which maxes at 52Mbps both
directions.

Cheers,

-- 
Paco Esteban.
0x5818130B8A6DBC03



Re: apm, apmd: ship manual pages on powerpc64

2021-03-21 Thread Jason McIntyre
On Sat, Mar 20, 2021 at 07:29:11PM -0600, Theo de Raadt wrote:
> There is a pattern we've followed in the past, that when a manpage applies
> to more than 2 (or 3?) architectures, then we simply make it MI.
> 
> So MANSUBDIR would get deleted, and then the sets would be updated.
> 
> People with old systems will retain the old files, but I think man(1)
> selects the MI ones over the MD ones (I think we have no override capability).
> 

man chooses the MD page over the MI page, i think. from man(1):

Machine specific areas are searched before general areas.

so you'd get the old page unless you deleted it. i don;t know if you can
override that.

jmc

> Klemens Nanni  wrote:
> 
> > It looks like an oversight when those were hooked up, but I don't have
> > access to that platform to test myself.
> > 
> > Feedback? OK?
> > 
> > Index: usr.sbin/apm/Makefile
> > ===
> > RCS file: /cvs/src/usr.sbin/apm/Makefile,v
> > retrieving revision 1.19
> > diff -u -p -r1.19 Makefile
> > --- usr.sbin/apm/Makefile   15 Jul 2020 22:46:52 -  1.19
> > +++ usr.sbin/apm/Makefile   20 Mar 2021 20:57:23 -
> > @@ -19,6 +19,6 @@ NOPROG=yes
> >  .endif
> >  
> >  MAN=   apm.8
> > -MANSUBDIR=arm64 amd64 i386 loongson macppc sparc64
> > +MANSUBDIR=arm64 amd64 i386 loongson macppc sparc64 powerpc64
> >  
> >  .include 
> > Index: usr.sbin/apmd/Makefile
> > ===
> > RCS file: /cvs/src/usr.sbin/apmd/Makefile,v
> > retrieving revision 1.15
> > diff -u -p -r1.15 Makefile
> > --- usr.sbin/apmd/Makefile  15 Jul 2020 22:46:52 -  1.15
> > +++ usr.sbin/apmd/Makefile  20 Mar 2021 21:00:37 -
> > @@ -13,6 +13,6 @@ NOPROG=yes
> >  .endif
> >  
> >  MAN=   apmd.8
> > -MANSUBDIR= arm64 amd64 i386 loongson macppc sparc64
> > +MANSUBDIR= arm64 amd64 i386 loongson macppc sparc64 powerpc64
> >  
> >  .include 
> > Index: distrib/sets/lists/man/mi
> > ===
> > RCS file: /cvs/src/distrib/sets/lists/man/mi,v
> > retrieving revision 1.1616
> > diff -u -p -r1.1616 mi
> > --- distrib/sets/lists/man/mi   1 Mar 2021 23:26:59 -   1.1616
> > +++ distrib/sets/lists/man/mi   20 Mar 2021 21:00:15 -
> > @@ -2511,6 +2511,8 @@
> >  ./usr/share/man/man8/pkg_check.8
> >  ./usr/share/man/man8/portmap.8
> >  ./usr/share/man/man8/powerpc64/MAKEDEV.8
> > +./usr/share/man/man8/powerpc64/apm.8
> > +./usr/share/man/man8/powerpc64/apmd.8
> >  ./usr/share/man/man8/powerpc64/eeprom.8
> >  ./usr/share/man/man8/pppd.8
> >  ./usr/share/man/man8/pppstats.8
> > 
>