Re: inteldrm(4) diff needs review and testing

[Andrew Marks]

On Mon, Jul 17, 2017 at 11:08:13AM +0900, Bryan Linton wrote:
> On 2017-07-16 15:19:41, Mark Kettenis  wrote:
> > Can somebody test the following diff on Ivy Bridge or Haswell (Intel
> > HD Graphics 2500/4000/4600/4700/5000/5100/5200)?
> > 
> > When I added support for the command parser, I took a bit of a
> > shortcut and implemented the hash tables as a single linked list.
> > This diff fixes that.
> > 
> > For the hash function I used a "mode (size-1)" approach that leaves
> > one of the hash table entries unused.  Perhaps somebody with a CS
> > background has a better idea that isn't too complicated to implement?
> > 
> 
> I haven't noticed any regressions or other negative effects due to
> this patch on an HD 4600 in a Thinkpad T440p.
> 
> One thing that may be worth mentioning, is that after the recent
> update to the DRM code, I get a lot of dmesg spam in the form of
> 
>   error: [drm:pid37898:intel_uncore_check_errors] *ERROR* Unclaimed 
> register before interrupt
> 
> after booting the system.
> 
> They are present with and without your patch, so it must be due to
> a separate issue.
> 
> They don't seem to be causing any negative effects (everything
> seems to be working just fine).  After booting, the system sits
> for about 15-20 seconds printing these errors to the console as
> fast as the system can print them.
> 
> After it's done, everything works normally so at least for me, it
> seems to be mostly a cosmetic issue.  However, I'm not sure if it
> may be pointing to something that may affect other users.
> 
> Both an actual dmesg and output from running "dmesg" are attached.
> As one can see, there are at least enough lines output to
> completely scroll the initial system dmesg out of the buffer.
> 

I experience the same! with and without the patch. It also fills the
dmesg buffer on my Dell M3800 but does not appear to cause any issues
other than a lot of logging.

I noticed it after I updated to a snapshot with random linking, not
saying its related, just mentioning when I noticed it first.

> --- /var/run/dmesg.boot ---
> 
> OpenBSD 6.1-current (GENERIC.MP-PPPOE_TERM_UNKNOWN_SESSIONS) #15: Mon Jul 17 
> 02:21:03 JST 2017
> 
> shoshon...@shoshoni-m.shoshoni.info:/usr/src/sys/arch/amd64/compile/GENERIC.MP-PPPOE_TERM_UNKNOWN_SESSIONS
> real mem = 12539871232 (11958MB)
> avail mem = 12154036224 (11590MB)
> mpath0 at root
> scsibus0 at mpath0: 256 targets
> mainbus0 at root
> bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xbcc0d000 (67 entries)
> bios0: vendor LENOVO version "GLET85WW (2.39 )" date 09/29/2016
> bios0: LENOVO 20AWS27D00
> acpi0 at bios0: rev 2
> acpi0: sleep states S0 S3 S4 S5
> acpi0: tables DSDT FACP SLIC DBGP ECDT HPET APIC MCFG SSDT SSDT SSDT SSDT 
> SSDT SSDT SSDT PCCT SSDT TCPA UEFI MSDM ASF! BATB FPDT UEFI DMAR
> acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) EXP2(S4) EXP3(S4) XHCI(S3) 
> EHC1(S3) EHC2(S3)
> acpitimer0 at acpi0: 3579545 Hz, 24 bits
> acpiec0 at acpi0
> acpihpet0 at acpi0: 14318179 Hz
> acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
> cpu0 at mainbus0: apid 0 (boot processor)
> cpu0: Intel(R) Core(TM) i5-4300M CPU @ 2.60GHz, 2594.38 MHz
> cpu0: 
> FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
> cpu0: 256KB 64b/line 8-way L2 cache
> cpu0: TSC frequency 2594384360 Hz
> cpu0: smt 0, core 0, package 0
> mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges
> cpu0: apic clock running at 99MHz
> cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4, IBE
> cpu1 at mainbus0: apid 1 (application processor)
> cpu1: Intel(R) Core(TM) i5-4300M CPU @ 2.60GHz, 2593.99 MHz
> cpu1: 
> FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
> cpu1: 256KB 64b/line 8-way L2 cache
> cpu1: smt 1, core 0, package 0
> cpu2 at mainbus0: apid 2 (application processor)
> cpu2: Intel(R) Core(TM) i5-4300M CPU @ 2.60GHz, 2593.99 MHz
> cpu2: 
> FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
> cpu2: 256KB 64b/line 8-way L2 cache
> cpu2: smt 0, core 1, package 0
> cpu3 at mainbus0: apid 3 (application 

Re: inteldrm(4) diff needs review and testing

[Bryan Linton]

On 2017-07-16 15:19:41, Mark Kettenis  wrote:
> Can somebody test the following diff on Ivy Bridge or Haswell (Intel
> HD Graphics 2500/4000/4600/4700/5000/5100/5200)?
> 
> When I added support for the command parser, I took a bit of a
> shortcut and implemented the hash tables as a single linked list.
> This diff fixes that.
> 
> For the hash function I used a "mode (size-1)" approach that leaves
> one of the hash table entries unused.  Perhaps somebody with a CS
> background has a better idea that isn't too complicated to implement?
> 

I haven't noticed any regressions or other negative effects due to
this patch on an HD 4600 in a Thinkpad T440p.

One thing that may be worth mentioning, is that after the recent
update to the DRM code, I get a lot of dmesg spam in the form of

error: [drm:pid37898:intel_uncore_check_errors] *ERROR* Unclaimed 
register before interrupt

after booting the system.

They are present with and without your patch, so it must be due to
a separate issue.

They don't seem to be causing any negative effects (everything
seems to be working just fine).  After booting, the system sits
for about 15-20 seconds printing these errors to the console as
fast as the system can print them.

After it's done, everything works normally so at least for me, it
seems to be mostly a cosmetic issue.  However, I'm not sure if it
may be pointing to something that may affect other users.

Both an actual dmesg and output from running "dmesg" are attached.
As one can see, there are at least enough lines output to
completely scroll the initial system dmesg out of the buffer.

--- /var/run/dmesg.boot ---

OpenBSD 6.1-current (GENERIC.MP-PPPOE_TERM_UNKNOWN_SESSIONS) #15: Mon Jul 17 
02:21:03 JST 2017

shoshon...@shoshoni-m.shoshoni.info:/usr/src/sys/arch/amd64/compile/GENERIC.MP-PPPOE_TERM_UNKNOWN_SESSIONS
real mem = 12539871232 (11958MB)
avail mem = 12154036224 (11590MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xbcc0d000 (67 entries)
bios0: vendor LENOVO version "GLET85WW (2.39 )" date 09/29/2016
bios0: LENOVO 20AWS27D00
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP SLIC DBGP ECDT HPET APIC MCFG SSDT SSDT SSDT SSDT SSDT 
SSDT SSDT PCCT SSDT TCPA UEFI MSDM ASF! BATB FPDT UEFI DMAR
acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) EXP2(S4) EXP3(S4) XHCI(S3) 
EHC1(S3) EHC2(S3)
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpiec0 at acpi0
acpihpet0 at acpi0: 14318179 Hz
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i5-4300M CPU @ 2.60GHz, 2594.38 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: TSC frequency 2594384360 Hz
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges
cpu0: apic clock running at 99MHz
cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4, IBE
cpu1 at mainbus0: apid 1 (application processor)
cpu1: Intel(R) Core(TM) i5-4300M CPU @ 2.60GHz, 2593.99 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu1: 256KB 64b/line 8-way L2 cache
cpu1: smt 1, core 0, package 0
cpu2 at mainbus0: apid 2 (application processor)
cpu2: Intel(R) Core(TM) i5-4300M CPU @ 2.60GHz, 2593.99 MHz
cpu2: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu2: 256KB 64b/line 8-way L2 cache
cpu2: smt 0, core 1, package 0
cpu3 at mainbus0: apid 3 (application processor)
cpu3: Intel(R) Core(TM) i5-4300M CPU @ 2.60GHz, 2593.99 MHz
cpu3: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SENSOR,ARAT
cpu3: 256KB 64b/line 8-way L2 cache
cpu3: smt 1, core 1, package 0
ioapic0 at mainbus0: apid 2 

Add machdep.lidaction=3 - powerdown laptop upon lid closing

[Marco Bonetti]

Hello tech@,
This is a diff that implements a new value (3) for machdep.lidaction to allow a 
laptop powerdown with a lid close. Since this functionality is very close to 
machdep.kbdreset=1, the diff follows the same behaviour and prevents setting 
machdep.lidaction when securelevel is greater than 0. Let me know what you 
think.

Cheers,
Marco

Index: etc/etc.amd64/sysctl.conf
===
RCS file: /cvs/src/etc/etc.amd64/sysctl.conf,v
retrieving revision 1.7
diff -u -p -u -p -r1.7 sysctl.conf
--- etc/etc.amd64/sysctl.conf   2 Mar 2017 10:38:09 -   1.7
+++ etc/etc.amd64/sysctl.conf   16 Jul 2017 23:38:33 -
@@ -1,3 +1,3 @@
 #machdep.allowaperture=2   # See xf86(4)
 #machdep.kbdreset=1# permit console CTRL-ALT-DEL to do a nice halt
-#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon lid closing
+#machdep.lidaction=0   # 1=suspend, 2=hibernate, 3=powerdown laptop 
upon lid closing
Index: etc/etc.i386/sysctl.conf
===
RCS file: /cvs/src/etc/etc.i386/sysctl.conf,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 sysctl.conf
--- etc/etc.i386/sysctl.conf2 Mar 2017 10:38:09 -   1.21
+++ etc/etc.i386/sysctl.conf16 Jul 2017 23:38:33 -
@@ -1,4 +1,4 @@
 #machdep.allowaperture=2   # See xf86(4)
 #machdep.apmhalt=1 # 1=powerdown hack, try if halt -p doesn't work
 #machdep.kbdreset=1# permit console CTRL-ALT-DEL to do a nice halt
-#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon lid closing
+#machdep.lidaction=0   # 1=suspend, 2=hibernate, 3=powerdown laptop 
upon lid closing
Index: etc/etc.loongson/sysctl.conf
===
RCS file: /cvs/src/etc/etc.loongson/sysctl.conf,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 sysctl.conf
--- etc/etc.loongson/sysctl.conf2 Mar 2017 10:38:09 -   1.4
+++ etc/etc.loongson/sysctl.conf16 Jul 2017 23:38:33 -
@@ -1 +1 @@
-#machdep.lidaction=0   # 1=suspend, 2=hibernate laptop upon lid closing
+#machdep.lidaction=0   # 1=suspend, 2=hibernate, 3=powerdown laptop 
upon lid closing
Index: sys/arch/amd64/amd64/machdep.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
retrieving revision 1.231
diff -u -p -u -p -r1.231 machdep.c
--- sys/arch/amd64/amd64/machdep.c  12 Jul 2017 06:26:32 -  1.231
+++ sys/arch/amd64/amd64/machdep.c  16 Jul 2017 23:38:42 -
@@ -477,15 +477,20 @@ cpu_sysctl(int *name, u_int namelen, voi
case CPU_XCRYPT:
return (sysctl_rdint(oldp, oldlenp, newp, amd64_has_xcrypt));
case CPU_LIDACTION:
-   val = lid_action;
-   error = sysctl_int(oldp, oldlenp, newp, newlen, );
-   if (!error) {
-   if (val < 0 || val > 2)
-   error = EINVAL;
-   else
-   lid_action = val;
-   }
-   return (error);
+   if (securelevel > 0)
+   return (sysctl_rdint(oldp, oldlenp, newp,
+   lid_action));
+else {
+   val = lid_action;
+   error = sysctl_int(oldp, oldlenp, newp, newlen, );
+   if (!error) {
+   if (val < 0 || val > 3)
+   error = EINVAL;
+   else
+   lid_action = val;
+   }
+   return (error);
+}
 #if NPCKBC > 0 && NUKBD > 0
case CPU_FORCEUKBD:
if (forceukbd)
Index: sys/arch/i386/i386/machdep.c
===
RCS file: /cvs/src/sys/arch/i386/i386/machdep.c,v
retrieving revision 1.604
diff -u -p -u -p -r1.604 machdep.c
--- sys/arch/i386/i386/machdep.c12 Jul 2017 06:26:32 -  1.604
+++ sys/arch/i386/i386/machdep.c16 Jul 2017 23:38:42 -
@@ -3551,15 +3551,20 @@ cpu_sysctl(int *name, u_int namelen, voi
case CPU_XCRYPT:
return (sysctl_rdint(oldp, oldlenp, newp, i386_has_xcrypt));
case CPU_LIDACTION:
-   val = lid_action;
-   error = sysctl_int(oldp, oldlenp, newp, newlen, );
-   if (!error) {
-   if (val < 0 || val > 2)
-   error = EINVAL;
-   else
-   lid_action = val;
-   }
-   return (error);
+   if (securelevel > 0)
+   return (sysctl_rdint(oldp, oldlenp, newp,
+   lid_action));
+   else {
+   

Re: inteldrm(4) diff needs review and testing

[Andrew Marks]

On Sun, Jul 16, 2017 at 08:45:40PM +0200, Mark Kettenis wrote:
> > Date: Sun, 16 Jul 2017 10:05:54 -0700
> > From: Andrew Marks 
> > 
> > Hello,
> > 
> > I happen to have a Haswell 4600 so I tried to apply this patch, I am
> > running a snapshot from 16 Jul 2017 and just updated my src.  My
> > drm_linux.h looks much different than the one your patch was meant for.
> > cvs log shows it to be revision 1.56 so I'm not sure where the
> > discrepancy lies. My drm_linux.h does not contain the lines provided in
> > the context of the diff.
> > 
> > Is there a tag or branch I should be pulling from? I've followed the
> > instructions following current.
> 
> No the diff is defenitely against rev 1.56 of drm_linux.h.
> 

It was my CVS error, wasn't on current

> > This is the first time I've tried to apply a patch from this mailing
> > list so I suspect I am doing something wrong, any pointers?
> 
> # cd /usr/src/sys
> # cat ~/patch | patch -p0
> 
> should do the trick.  Could be your mail client is mangling the diff
> though.
> 

I don't notice anything differet on this Dell M3800 Laptop, let me know
if there is something specific you'd like me to test.

> > On Sun, Jul 16, 2017 at 03:19:41PM +0200, Mark Kettenis wrote:
> > > Can somebody test the following diff on Ivy Bridge or Haswell (Intel
> > > HD Graphics 2500/4000/4600/4700/5000/5100/5200)?
> > > 
> > > When I added support for the command parser, I took a bit of a
> > > shortcut and implemented the hash tables as a single linked list.
> > > This diff fixes that.
> > > 
> > > For the hash function I used a "mode (size-1)" approach that leaves
> > > one of the hash table entries unused.  Perhaps somebody with a CS
> > > background has a better idea that isn't too complicated to implement?
> > > 
> > > Paul, Stuart, there is a small chance that this will improve the
> > > vncviewer performance.
> > > 
> > > 
> > > Index: dev/pci/drm/drm_linux.h
> > > ===
> > > RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.h,v
> > > retrieving revision 1.56
> > > diff -u -p -r1.56 drm_linux.h
> > > --- dev/pci/drm/drm_linux.h   14 Jul 2017 11:18:04 -  1.56
> > > +++ dev/pci/drm/drm_linux.h   16 Jul 2017 12:54:51 -
> > > @@ -40,6 +40,7 @@
> > >  
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  /* The Linux code doesn't meet our usual standards! */
> > >  #ifdef __clang__
> > > @@ -202,16 +203,42 @@ bitmap_weight(void *p, u_int n)
> > >   return sum;
> > >  }
> > >  
> > > -#define DECLARE_HASHTABLE(x, y) struct hlist_head x;
> > > +#define DECLARE_HASHTABLE(name, bits) struct hlist_head name[1 << (bits)]
> > >  
> > > -#define hash_init(x) INIT_HLIST_HEAD(&(x))
> > > -#define hash_add(x, y, z)hlist_add_head(y, &(x))
> > > -#define hash_del(x)  hlist_del_init(x)
> > > -#define hash_empty(x)hlist_empty(&(x))
> > > -#define hash_for_each_possible(a, b, c, d) \
> > > - hlist_for_each_entry(b, &(a), c)
> > > -#define hash_for_each_safe(a, b, c, d, e) (void)(b); \
> > > - hlist_for_each_entry_safe(d, c, &(a), e)
> > > +static inline void
> > > +__hash_init(struct hlist_head *table, u_int size)
> > > +{
> > > + u_int i;
> > > +
> > > + for (i = 0; i < size; i++)
> > > + INIT_HLIST_HEAD([i]);
> > > +}
> > > +
> > > +static inline bool
> > > +__hash_empty(struct hlist_head *table, u_int size)
> > > +{
> > > + u_int i;
> > > +
> > > + for (i = 0; i < size; i++) {
> > > + if (!hlist_empty([i]))
> > > + return false;
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > +#define __hash(table, key)   [key % (nitems(table) - 1)]
> > > +
> > > +#define hash_init(table) __hash_init(table, nitems(table))
> > > +#define hash_add(table, node, key) \
> > > + hlist_add_head(node, __hash(table, key))
> > > +#define hash_del(node)   hlist_del_init(node)
> > > +#define hash_empty(table)__hash_empty(table, nitems(table))
> > > +#define hash_for_each_possible(table, obj, member, key) \
> > > + hlist_for_each_entry(obj, __hash(table, key), member)
> > > +#define hash_for_each_safe(table, i, tmp, obj, member)   \
> > > + for (i = 0; i < nitems(table); i++) \
> > > +hlist_for_each_entry_safe(obj, tmp, [i], member)
> > >  
> > >  #define ACCESS_ONCE(x)   (x)
> > >  
> > > 
> > 
> > 

Re: rc: Use here document for temporary pf rule set

[Klemens Nanni]

On Sun, Jul 16, 2017 at 08:15:38PM +, Robert Peichaer wrote:
> > +   ifconfig lo0 inet6 >/dev/null 2>&1 &&
> 
> Please leave the if-then-fi construct intact. This short form is
> mostly used for on-line commands (with only a few exceptions).
OK.

> > +   RULES="$RULES"'
> 
> What is the reason to use double quotes and single quotes here?
> Why not just use double quotes like this?
Personal preference to make clear nothing inside the rules gets
substituted. Using double quotes only will work just fine here.

> This is not equivalent to the existing code.
> 
> > +   pass in proto { tcp, udp } from any port { sunrpc, nfsd } to any
> > +   pass out proto { tcp, udp } from any to any port { sunrpc, nfsd 
> > } !received-on any'
> > +   print -- "$RULES" | pfctl -nf -
Of course, fixed. Thanks!

> Unless one of the pf people speaks up in favour of combining it,
> I'd like to leave the two steps separated as you noted in your
> original email too.
Sure.

This is hopefully the final version of my diff. After all it now only
merges consecutive assignments of RULE into single ones.

Feedback?

Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc  4 Jul 2017 19:02:11 -   1.507
+++ rc  16 Jul 2017 21:10:48 -
@@ -402,28 +399,35 @@ wsconsctl_conf
 
 # Set initial temporary pf rule set.
 if [[ $pf != NO ]]; then
-   RULES="block all"
-   RULES="$RULES\npass on lo0"
-   RULES="$RULES\npass in proto tcp from any to any port ssh keep state"
-   RULES="$RULES\npass out proto { tcp, udp } from any to any port domain 
keep state"
-   RULES="$RULES\npass out inet proto icmp all icmp-type echoreq keep 
state"
-   RULES="$RULES\npass out inet proto udp from any port bootpc to any port 
bootps"
-   RULES="$RULES\npass in inet proto udp from any port bootps to any port 
bootpc"
+   RULES='
+   block all
+   pass on lo0
+   pass in proto tcp from any to any port ssh keep state
+   pass out proto { tcp, udp } from any to any port domain keep state
+   pass out inet proto icmp all icmp-type echoreq keep state
+   pass out inet proto udp from any port bootpc to any port bootps
+   pass in inet proto udp from any port bootps to any port bootpc'
+
if ifconfig lo0 inet6 >/dev/null 2>&1; then
-   RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type 
neighbrsol"
-   RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type 
neighbradv"
-   RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type 
routersol"
-   RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type 
routeradv"
-   RULES="$RULES\npass out inet6 proto udp from any port 
dhcpv6-client to any port dhcpv6-server"
-   RULES="$RULES\npass in inet6 proto udp from any port 
dhcpv6-server to any port dhcpv6-client"
+   RULES="$RULES
+   pass out inet6 proto icmp6 all icmp6-type neighbrsol
+   pass in inet6 proto icmp6 all icmp6-type neighbradv
+   pass out inet6 proto icmp6 all icmp6-type routersol
+   pass in inet6 proto icmp6 all icmp6-type routeradv
+   pass out inet6 proto udp from any port dhcpv6-client to any 
port dhcpv6-server
+   pass in inet6 proto udp from any port dhcpv6-server to any port 
dhcpv6-client"
fi
-   RULES="$RULES\npass in proto carp keep state (no-sync)"
-   RULES="$RULES\npass out proto carp !received-on any keep state 
(no-sync)"
+
+   RULES="$RULES
+   pass in proto carp keep state (no-sync)
+   pass out proto carp !received-on any keep state (no-sync)"
+
+   # Don't kill NFS.
if [[ $(sysctl vfs.mounts.nfs 2>/dev/null) == *[1-9]* ]]; then
-   # Don't kill NFS.
-   RULES="set reassemble yes no-df\n$RULES"
-   RULES="$RULES\npass in proto { tcp, udp } from any port { 
sunrpc, nfsd } to any"
-   RULES="$RULES\npass out proto { tcp, udp } from any to any port 
{ sunrpc, nfsd } !received-on any"
+   RULES="set reassemble yes no-df
+   $RULES
+   pass in proto { tcp, udp } from any port { sunrpc, nfsd } to any
+   pass out proto { tcp, udp } from any to any port { sunrpc, nfsd 
} !received-on any"
fi
print -- "$RULES" | pfctl -f -
pfctl -e

Re: armv7 sunxi i2c+pmic(=shutdown -p)

[Mark Kettenis]

> Date: Sun, 9 Jul 2017 20:34:29 +0300
> From: Artturi Alm 
> 
> Hi,
> 
> revived the diff below, i2c tested via pmic's shutdown(), for working
> "shutdown -p now" operation.
> there was only two i2c's w/"status: 'okay'" in the FDT, so not all of
> them do attach.
> 
> related part of dmesg:
> 
> com0: console
> sxitwi0 at simplebus0
> iic0 at sxitwi0
> axppmic0 at iic0 addr 0x34: AXP209, ACIN
> sxitwi1 at simplebus0
> iic1 at sxitwi1
> dwge0 at simplebus0
> 
> Comments?
> -Artturi

It's a pity that the PSCI "firmware" doesn't do an actual shutdown.
But having i2c support is worth having in its own right.

A bit of a step backwards to add code under the old-style 4-clause BSD
license, but I believe that is still acceptable.

I don't think we'll ever support the Marvell Discovery hardware, so
I'd just fold the gttwsi_core.c code into sxitwi.c and get rid of the
GTTWSI_ALLWINNER hack.

A few more comments inline below.

On my (Allwinner A20) Banana Pi this seems to work, although the
Ethernet link status LED turns back on shortly after the board powers
down.  I guess it gets power from the link.  The power LED stays on as
well, but that is just leakage from the serial console.  The green LED
that is gpio controllable turns off when the board powers down.


> diff --git a/sys/arch/armv7/conf/GENERIC b/sys/arch/armv7/conf/GENERIC
> index af71a6c4835..d8762ba394c 100644
> --- a/sys/arch/armv7/conf/GENERIC
> +++ b/sys/arch/armv7/conf/GENERIC
> @@ -99,6 +99,8 @@ ehci*   at fdt? # EHCI (shim)
>  usb* at ehci?#flags 0x1
>  #ohci*   at sunxi?
>  #usb*at ohci?
> +sxitwi*  at fdt? # Two-Wire Serial Interface
> +iic* at sxitwi?  # I2C bus
>  
>  # ARM Versatile Express
>  sysreg*  at fdt?
> @@ -148,6 +150,7 @@ mvxhci*   at fdt?
>  usb* at mvxhci?
>  mvahci*  at fdt?
>  
> +axppmic* at iic? # axp209 pmic
>  crosec*  at iic?
>  wskbd*   at crosec? mux 1
>  pcfrtc*  at iic?
> diff --git a/sys/arch/armv7/conf/RAMDISK b/sys/arch/armv7/conf/RAMDISK
> index 56e64893df6..0c5f8aa4e1f 100644
> --- a/sys/arch/armv7/conf/RAMDISK
> +++ b/sys/arch/armv7/conf/RAMDISK
> @@ -99,6 +99,8 @@ ehci*   at fdt? # EHCI (shim)
>  usb* at ehci?#flags 0x1
>  #ohci*   at sunxi?
>  #usb*at ohci?
> +sxitwi*  at fdt? # Two-Wire Serial Interface
> +iic* at sxitwi?  # I2C bus
>  
>  # ARM Versatile Express
>  sysreg*  at fdt?
> @@ -145,6 +147,7 @@ mvxhci*   at fdt?
>  usb* at mvxhci?
>  mvahci*  at fdt?
>  
> +axppmic* at iic? # axp209 pmic
>  crosec*  at iic?
>  wskbd*   at crosec? mux 1
>  pcfrtc*  at iic?
> diff --git a/sys/dev/fdt/axp20x.c b/sys/dev/fdt/axp20x.c
> new file mode 100644
> index 000..833038f0eff
> --- /dev/null
> +++ b/sys/dev/fdt/axp20x.c
> @@ -0,0 +1,160 @@
> +/*
> + * Copyright (c) 2014,2016 Artturi Alm
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +#include/* needed for powerdownfn */
> +
> +/* Power Status Register / Input power status */
> +#define  AXP209_PSR  0x00
> +#define  AXP209_PSR_ACIN (1 << 7)/* ACIN Exists */
> +#define  AXP209_PSR_VBUS (1 << 5)/* VBUS Exists */
> +
> +/* Shutdown settings, battery detection, and CHGLED Pin control */
> +#define  AXP209_SDR  0x32
> +#define  AXP209_SDR_SHUTDOWN (1 << 7)/* Shutdown Control */
> +
> +#define DNAME(sc)((sc)->sc_dev.dv_xname)

This macro is unused.

> +struct axp20x_softc {
> + struct device   sc_dev;
> + i2c_tag_t   sc_i2c;
> + i2c_addr_t  sc_addr;
> +};
> +
> +int  axp20x_match(struct device *, void *, void *);
> +void axp20x_attach(struct device *, struct device *, void *);
> +
> +int  axp20x_readb(u_char, u_char *);
> +int  axp20x_writeb(u_char, 

Re: pledge ifstated

[Rob Pierce]

On Thu, Jul 13, 2017 at 09:16:14PM -0400, Rob Pierce wrote:
> On Mon, Jul 10, 2017 at 01:21:58PM -0400, Rob Pierce wrote:
> > The following diff is loosely based on the approach that was taken for
> > pledging mountd. Other code/approaches leveraged from various networking
> > daemons.
> > 
> > This first step moves the ioctl with SIOCGIFDATA call to a privileged
> > child so we can at least pledge "stdio rpath dns inet proc exec" without
> > (intentionally) changing existing behaviour.
> > 
> > Comments and suggestions welcome.
> > 
> > Thanks!
> > 
> > Rob
> 
> An unnecessary call to log_info snuck in. Here is a clean diff.
> 
> Rob

My original diff to initially pledge ifstated with "stdio rpath dns inet proc
exec" was incorrectly polling from fetch_ifstate which resulted in the delayed
completion of that function. This new diff resolves the problem.

I also changed one instance of fatal to fatalx in sigchld_handler.

Regards,

Rob

Index: Makefile
===
RCS file: /cvs/src/usr.sbin/ifstated/Makefile,v
retrieving revision 1.10
diff -u -p -r1.10 Makefile
--- Makefile20 Jul 2010 02:05:15 -  1.10
+++ Makefile16 Jul 2017 20:38:48 -
@@ -8,7 +8,7 @@ CFLAGS+= -Wmissing-declarations
 CFLAGS+= -Wshadow -Wpointer-arith -Wcast-qual
 YFLAGS=
 MAN= ifstated.8 ifstated.conf.5
-LDADD+=-levent
+LDADD+=-levent -lutil
 DPADD+= ${LIBEVENT}
 
 .include 
Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.50
diff -u -p -r1.50 ifstated.c
--- ifstated.c  4 Jul 2017 21:09:52 -   1.50
+++ ifstated.c  16 Jul 2017 20:38:48 -
@@ -25,13 +25,16 @@
 #include 
 #include 
 #include 
+#include 
 #include 
+#include 
 #include 
 
 #include 
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -39,6 +42,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -47,7 +51,9 @@
 #include "log.h"
 
 struct  ifsd_config *conf = NULL, *newconf = NULL;
+struct  imsgbuf ibuf;
 
+pid_t   privchild_pid;
 int opts = 0;
 int opt_inhibit = 0;
 char   *configfile = "/etc/ifstated.conf";
@@ -74,6 +80,7 @@ void  do_action(struct ifsd_action *);
 void   remove_action(struct ifsd_action *, struct ifsd_state *);
 void   remove_expression(struct ifsd_expression *,
struct ifsd_state *);
+void   privchild(int);
 
 __dead void
 usage(void)
@@ -89,6 +96,7 @@ int
 main(int argc, char *argv[])
 {
struct timeval tv;
+   int socks[2];
int ch, rt_fd;
int debug = 0;
unsigned int rtfilter;
@@ -143,6 +151,22 @@ main(int argc, char *argv[])
if (!debug)
daemon(1, 0);
 
+   if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, socks) == -1) {
+   fatal("%s: socketpair", __func__);
+   }
+
+   switch (privchild_pid = fork()) {
+   case -1:
+   fatal("%s: fork", __func__);
+   case 0:
+   close(socks[0]);
+   privchild(socks[1]);
+   }
+   close(socks[1]);
+
+   imsg_init(, socks[0]);
+   setproctitle("parent");
+
event_init();
log_init(debug, LOG_DAEMON);
log_setverbose(opts & IFSD_OPT_VERBOSE);
@@ -160,6 +184,9 @@ main(int argc, char *argv[])
, sizeof(rtfilter)) == -1) /* not fatal */
log_warn("%s: setsockopt tablefilter", __func__);
 
+   if (pledge("stdio rpath dns inet proc exec", NULL) == -1)
+   fatal("pledge");
+
signal_set(_ev, SIGCHLD, sigchld_handler, NULL);
signal_add(_ev, NULL);
 
@@ -252,6 +279,16 @@ rt_msg_handler(int fd, short event, void
 void
 sigchld_handler(int fd, short event, void *arg)
 {
+   pid_t pid;
+   int status;
+
+   do {
+   pid = waitpid(privchild_pid, , WNOHANG);
+   if (pid > 0 && (WIFEXITED(status) || WIFSIGNALED(status)))
+   fatalx("privchild %i exited due to receipt of signal 
%i",
+   privchild_pid, WTERMSIG(status));
+   } while (pid == -1 && errno == EINTR);
+
check_external_status(>initstate);
if (conf->curstate != NULL)
check_external_status(conf->curstate);
@@ -599,29 +636,60 @@ do_action(struct ifsd_action *action)
 void
 fetch_ifstate(void)
 {
+   struct imsg imsg;
struct ifaddrs *ifap, *ifa;
char *oname = NULL;
int sock = socket(AF_INET, SOCK_DGRAM, 0);
+   int done = 0, state, rv;
+   ssize_t n;
 
if (getifaddrs() != 0)
err(1, "getifaddrs");
 
for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
struct ifreq ifr;
-   struct if_data ifrdat;
 
if (oname && !strcmp(oname, ifa->ifa_name))
continue;
oname = ifa->ifa_name;
 

Re: rc: Use here document for temporary pf rule set

[Robert Peichaer]

On Sun, Jul 16, 2017 at 03:24:19PM +0200, Klemens Nanni wrote:
> On Sun, Jul 16, 2017 at 12:41:09PM +, Robert Peichaer wrote:
> > On Sun, Jul 16, 2017 at 02:28:59PM +0200, Klemens Nanni wrote:
> > > On Sun, Jul 16, 2017 at 12:11:55PM +, Robert Peichaer wrote:
> > > > On Sun, Jul 16, 2017 at 01:37:56PM +0200, Klemens Nanni wrote:
> > > > > This removes on level of indent, avoids the ugly RULES="$RULES ..."
> > > > > repitition and spares a print.
> > > > > 
> > > > > We could do a 'pfctl -ef -' right away but I kept changing and 
> > > > > enabling
> > > > > clearly seperated. Regarding the leading newlines and tabs of the 
> > > > > inner
> > > > > echo: pf perfectly munges those, no need to clear them.
> > > > > 
> > > > > The "don't" -> "do not" is neccessary since otherwise the shell would
> > > > > choke on it as opening quote.
> > > > > 
> > > > > 
> > > > > Feedback? Comments?
> > > > 
> > > > Nice idea. The only maby irrelevant concern I have is, that using the
> > > > here-document approach uses a temporary file and if that for some reason
> > > > fails, we end up without this or mangled rules.
> > > sh reads the temporary file in 512 bytes chunks, the here document is
> > > about 2.0K in size.
> > > 
> > > I didn't bother intercepting sh with gdb and simulating a scenario where
> > > the temporary file cannot be written but in case the user has no disk
> > > space left I'd expect it to not be created at all since.
> > > 
> > > In general I'd say that if /tmp doesn't have 2.0K left users probably
> > > have more serious problems anyway.
> > 
> > Have you thought about diskless(8) setups?
> If /tmp is served via NFS and the here document's IO fails, 'pfctl -f -'
> won't see any rule as it's not being executed.
> 
> I still think running out of space would cause more problems than just
> this one but I shall be glad to be corrected.
> 
> Here's another approach still using $RULES but with saner quoting and
> without the potentially dangerous here document.
> 
> 
> Index: rc
> ===
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.507
> diff -u -p -r1.507 rc
> --- rc4 Jul 2017 19:02:11 -   1.507
> +++ rc16 Jul 2017 13:23:02 -
> @@ -402,31 +399,35 @@ wsconsctl_conf
>  
>  # Set initial temporary pf rule set.
>  if [[ $pf != NO ]]; then
> - RULES="block all"
> - RULES="$RULES\npass on lo0"
> - RULES="$RULES\npass in proto tcp from any to any port ssh keep state"
> - RULES="$RULES\npass out proto { tcp, udp } from any to any port domain 
> keep state"
> - RULES="$RULES\npass out inet proto icmp all icmp-type echoreq keep 
> state"
> - RULES="$RULES\npass out inet proto udp from any port bootpc to any port 
> bootps"
> - RULES="$RULES\npass in inet proto udp from any port bootps to any port 
> bootpc"
> - if ifconfig lo0 inet6 >/dev/null 2>&1; then
> - RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type 
> neighbrsol"
> - RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type 
> neighbradv"
> - RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type 
> routersol"
> - RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type 
> routeradv"
> - RULES="$RULES\npass out inet6 proto udp from any port 
> dhcpv6-client to any port dhcpv6-server"
> - RULES="$RULES\npass in inet6 proto udp from any port 
> dhcpv6-server to any port dhcpv6-client"
> - fi
> - RULES="$RULES\npass in proto carp keep state (no-sync)"
> - RULES="$RULES\npass out proto carp !received-on any keep state 
> (no-sync)"
> - if [[ $(sysctl vfs.mounts.nfs 2>/dev/null) == *[1-9]* ]]; then
> - # Don't kill NFS.
> - RULES="set reassemble yes no-df\n$RULES"
> - RULES="$RULES\npass in proto { tcp, udp } from any port { 
> sunrpc, nfsd } to any"
> - RULES="$RULES\npass out proto { tcp, udp } from any to any port 
> { sunrpc, nfsd } !received-on any"
> - fi
> - print -- "$RULES" | pfctl -f -
> - pfctl -e
> + RULES='
> + block all
> + pass on lo0
> + pass in proto tcp from any to any port ssh keep state
> + pass out proto { tcp, udp } from any to any port domain keep state
> + pass out inet proto icmp all icmp-type echoreq keep state
> + pass out inet proto udp from any port bootpc to any port bootps
> + pass in inet proto udp from any port bootps to any port bootpc'
> +
> + ifconfig lo0 inet6 >/dev/null 2>&1 &&

Please leave the if-then-fi construct intact. This short form is
mostly used for on-line commands (with only a few exceptions).

> + RULES="$RULES"'

What is the reason to use double quotes and single quotes here?
Why not just use double quotes like this?

RULES="$RULES
..."

> + pass out inet6 proto icmp6 all icmp6-type neighbrsol
> + pass in inet6 proto 

Re: ospfd: add IMSG_IFADDRADD to deal with "sh /etc/netstart if"

[Remi Locherer]

On Tue, Jul 04, 2017 at 11:00:18PM +0200, Remi Locherer wrote:
> On Sun, Jun 25, 2017 at 11:47:09PM +0200, Remi Locherer wrote:
> > Hi,
> > 
> > ospfd does not react nicely when running "sh /etc/netstart if".
> > 
> > This is because adding the same address again do an interface results
> > in RTM_DELADDR and RTM_NEWADDR. ospfd handles the former but the later.
> > If this happens ospfd says "interface vether0:192.168.250.1 gone".
> > Adjacencies on that interface are down and ospfd can not recover.
> > 
> > The below patch adds IMSG_IFADDRADD to deal with that. With it ospfd
> > logs the following after "ifconfig vether0 192.168.250.1/24" (same address
> > as active before):
> > 
> > orig_rtr_lsa: area 0.0.0.0
> > orig_rtr_lsa: stub net, interface iwm0
> > orig_rtr_lsa: stub net, interface vether0
> > orig_rtr_lsa: transit net, interface pair0
> > if_fsm: event DOWN resulted in action RESET and changing state for 
> > interface vether0 from DR to DOWN
> > interface vether0:192.168.250.1 gone
> > orig_rtr_lsa: area 0.0.0.0
> > orig_rtr_lsa: stub net, interface iwm0
> > orig_rtr_lsa: stub net, interface vether0
> > orig_rtr_lsa: transit net, interface pair0
> > if_fsm: event UP resulted in action START and changing state for interface 
> > vether0 from DOWN to WAIT
> > interface vether0:192.168.250.1 returned
> > 
> > 
> > In addition the patch also deals with a changed netmask if the address
> > stays the same.
> > 
> > A complete new address still needs a change in ospfd.conf and a reload.
> > 
> > Remi
> > 
> 
> ping.
> 
> Any comments, opinions or improvements about this patch?
> 

It would be nice to get feedback on this. I think it is a real improvement
for ospfd. It would have saved me some troubles.

For easier review here again the patch.

Thank you,
Remi


Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
retrieving revision 1.107
diff -u -p -r1.107 kroute.c
--- kroute.c27 Dec 2016 09:15:16 -  1.107
+++ kroute.c16 Jul 2017 19:37:18 -
@@ -1046,6 +1046,7 @@ if_newaddr(u_short ifindex, struct socka
 {
struct kif_node *kif;
struct kif_addr *ka;
+   struct ifaddrnew ifn;
 
if (ifa == NULL || ifa->sin_family != AF_INET)
return;
@@ -1066,6 +1067,12 @@ if_newaddr(u_short ifindex, struct socka
ka->dstbrd.s_addr = INADDR_NONE;
 
TAILQ_INSERT_TAIL(>addrs, ka, entry);
+
+   ifn.addr = ka->addr;
+   ifn.mask = ka->mask;
+   ifn.dst = ka->dstbrd;
+   ifn.ifindex = ifindex;
+   main_imsg_compose_ospfe(IMSG_IFADDRADD, 0, , sizeof(ifn));
 }
 
 void
Index: ospfd.h
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v
retrieving revision 1.97
diff -u -p -r1.97 ospfd.h
--- ospfd.h 24 Jan 2017 04:24:25 -  1.97
+++ ospfd.h 16 Jul 2017 19:37:18 -
@@ -132,6 +132,7 @@ enum imsg_type {
IMSG_RECONF_REDIST,
IMSG_RECONF_END,
IMSG_DEMOTE,
+   IMSG_IFADDRADD,
IMSG_IFADDRDEL
 };
 
@@ -358,6 +359,13 @@ struct iface {
u_int8_t linkstate;
u_int8_t priority;
u_int8_t passive;
+};
+
+struct ifaddrnew {
+   struct in_addr  addr;
+   struct in_addr  mask;
+   struct in_addr  dst;
+   unsigned intifindex;
 };
 
 struct ifaddrdel {
Index: ospfe.c
===
RCS file: /cvs/src/usr.sbin/ospfd/ospfe.c,v
retrieving revision 1.99
diff -u -p -r1.99 ospfe.c
--- ospfe.c 24 Jan 2017 04:24:25 -  1.99
+++ ospfe.c 16 Jul 2017 19:37:18 -
@@ -278,6 +278,7 @@ ospfe_dispatch_main(int fd, short event,
 {
static struct area  *narea;
static struct iface *niface;
+   struct ifaddrnew*ifn;
struct ifaddrdel*ifc;
struct imsg  imsg;
struct imsgev   *iev = bula;
@@ -345,6 +346,28 @@ ospfe_dispatch_main(int fd, short event,
" down",
iface->name);
}
+   }
+   }
+   }
+   break;
+   case IMSG_IFADDRADD:
+   if (imsg.hdr.len != IMSG_HEADER_SIZE +
+   sizeof(struct ifaddrnew))
+   fatalx("IFADDRADD imsg with wrong len");
+   ifn = imsg.data;
+
+   LIST_FOREACH(area, >area_list, entry) {
+   LIST_FOREACH(iface, >iface_list, entry) {
+   if (ifn->ifindex == iface->ifindex &&
+   ifn->addr.s_addr ==
+

Re: inteldrm(4) diff needs review and testing

[Mark Kettenis]

> Date: Sun, 16 Jul 2017 10:05:54 -0700
> From: Andrew Marks 
> 
> Hello,
> 
> I happen to have a Haswell 4600 so I tried to apply this patch, I am
> running a snapshot from 16 Jul 2017 and just updated my src.  My
> drm_linux.h looks much different than the one your patch was meant for.
> cvs log shows it to be revision 1.56 so I'm not sure where the
> discrepancy lies. My drm_linux.h does not contain the lines provided in
> the context of the diff.
> 
> Is there a tag or branch I should be pulling from? I've followed the
> instructions following current.

No the diff is defenitely against rev 1.56 of drm_linux.h.

> This is the first time I've tried to apply a patch from this mailing
> list so I suspect I am doing something wrong, any pointers?

# cd /usr/src/sys
# cat ~/patch | patch -p0

should do the trick.  Could be your mail client is mangling the diff
though.

> On Sun, Jul 16, 2017 at 03:19:41PM +0200, Mark Kettenis wrote:
> > Can somebody test the following diff on Ivy Bridge or Haswell (Intel
> > HD Graphics 2500/4000/4600/4700/5000/5100/5200)?
> > 
> > When I added support for the command parser, I took a bit of a
> > shortcut and implemented the hash tables as a single linked list.
> > This diff fixes that.
> > 
> > For the hash function I used a "mode (size-1)" approach that leaves
> > one of the hash table entries unused.  Perhaps somebody with a CS
> > background has a better idea that isn't too complicated to implement?
> > 
> > Paul, Stuart, there is a small chance that this will improve the
> > vncviewer performance.
> > 
> > 
> > Index: dev/pci/drm/drm_linux.h
> > ===
> > RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.h,v
> > retrieving revision 1.56
> > diff -u -p -r1.56 drm_linux.h
> > --- dev/pci/drm/drm_linux.h 14 Jul 2017 11:18:04 -  1.56
> > +++ dev/pci/drm/drm_linux.h 16 Jul 2017 12:54:51 -
> > @@ -40,6 +40,7 @@
> >  
> >  #include 
> >  #include 
> > +#include 
> >  
> >  /* The Linux code doesn't meet our usual standards! */
> >  #ifdef __clang__
> > @@ -202,16 +203,42 @@ bitmap_weight(void *p, u_int n)
> > return sum;
> >  }
> >  
> > -#define DECLARE_HASHTABLE(x, y) struct hlist_head x;
> > +#define DECLARE_HASHTABLE(name, bits) struct hlist_head name[1 << (bits)]
> >  
> > -#define hash_init(x)   INIT_HLIST_HEAD(&(x))
> > -#define hash_add(x, y, z)  hlist_add_head(y, &(x))
> > -#define hash_del(x)hlist_del_init(x)
> > -#define hash_empty(x)  hlist_empty(&(x))
> > -#define hash_for_each_possible(a, b, c, d) \
> > -   hlist_for_each_entry(b, &(a), c)
> > -#define hash_for_each_safe(a, b, c, d, e) (void)(b); \
> > -   hlist_for_each_entry_safe(d, c, &(a), e)
> > +static inline void
> > +__hash_init(struct hlist_head *table, u_int size)
> > +{
> > +   u_int i;
> > +
> > +   for (i = 0; i < size; i++)
> > +   INIT_HLIST_HEAD([i]);
> > +}
> > +
> > +static inline bool
> > +__hash_empty(struct hlist_head *table, u_int size)
> > +{
> > +   u_int i;
> > +
> > +   for (i = 0; i < size; i++) {
> > +   if (!hlist_empty([i]))
> > +   return false;
> > +   }
> > +
> > +   return true;
> > +}
> > +
> > +#define __hash(table, key) [key % (nitems(table) - 1)]
> > +
> > +#define hash_init(table)   __hash_init(table, nitems(table))
> > +#define hash_add(table, node, key) \
> > +   hlist_add_head(node, __hash(table, key))
> > +#define hash_del(node) hlist_del_init(node)
> > +#define hash_empty(table)  __hash_empty(table, nitems(table))
> > +#define hash_for_each_possible(table, obj, member, key) \
> > +   hlist_for_each_entry(obj, __hash(table, key), member)
> > +#define hash_for_each_safe(table, i, tmp, obj, member) \
> > +   for (i = 0; i < nitems(table); i++) \
> > +  hlist_for_each_entry_safe(obj, tmp, [i], member)
> >  
> >  #define ACCESS_ONCE(x) (x)
> >  
> > 
> 
> 

Re: RFC 7217: sysctl [1/8]

[Leighton Sheppard]

This is great newsi, thanks for your efforts implementing this. It is also 
encouraging to hear this should be in 6.2 release. I'll commit to testing on my 
hardware and report any bugs.


Regards,
Leighton
On Sat, Jul 15, 2017 at 05:05:52PM +, Florian Obser wrote:
> I didn't not hear any objections to RFC 7217 support, so I guess it's
> time to get this thing in to get some operational experience with it
> before 6.2 is cut.
> Also the big diff gets a bit unwieldy, further improvement can happen
> in-tree.
> 
> This is the sysctl part for "net.inet6.ip6.soiikey", written by dlg
> with a few tweaks by me. Therefore he should commit it.
> This is OK florian@, anyone else?
> 
> diff --git sbin/sysctl/sysctl.c sbin/sysctl/sysctl.c
> index 94f78c0d673..cbacaca19d2 100644
> --- sbin/sysctl/sysctl.c
> +++ sbin/sysctl/sysctl.c
> @@ -212,7 +212,7 @@ int sysctl_chipset(char *, char **, int *, int, int *);
>  #endif
>  void vfsinit(void);
>  
> -char *equ = "=";
> +const char *equ = "=";
>  
>  int
>  main(int argc, char *argv[])
> @@ -286,6 +286,53 @@ listall(char *prefix, struct list *lp)
>   }
>  }
>  
> +int
> +parse_hex_char(char ch)
> +{
> + if (ch >= '0' && ch <= '9')
> + return (ch - '0');
> + if (ch >= 'a' && ch <= 'f')
> + return (ch - 'a' + 10);
> + if (ch >= 'A' && ch <= 'F')
> + return (ch - 'A' + 10);
> +
> + return (-1);
> +}
> +
> +ssize_t
> +parse_hex_string(unsigned char *dst, size_t dstlen, const char *src)
> +{
> + ssize_t len = 0;
> + int digit;
> +
> + while (len < dstlen) {
> + if (*src == '\0')
> + return (len);
> +
> + digit = parse_hex_char(*src++);
> + if (digit == -1)
> + return (-1);
> + dst[len] = digit << 4;
> +
> + digit = parse_hex_char(*src++);
> + if (digit == -1)
> + return (-1);
> + 
> + dst[len] |= digit;
> + len++;
> + }
> +
> + while (*src != '\0') {
> + if (parse_hex_char(*src++) == -1 ||
> + parse_hex_char(*src++) == -1)
> + return (-1);
> +
> + len++;
> + }
> +
> + return (len);
> +}
> +
>  /*
>   * Parse a name into a MIB entry.
>   * Lookup and print out the MIB entry if it exists.
> @@ -302,6 +349,7 @@ parse(char *string, int flags)
>   struct list *lp;
>   int mib[CTL_MAXNAME];
>   char *cp, *bufp, buf[SYSCTL_BUFSIZ];
> + unsigned char hex[SYSCTL_BUFSIZ];
>  
>   (void)strlcpy(buf, string, sizeof(buf));
>   bufp = buf;
> @@ -566,6 +614,9 @@ parse(char *string, int flags)
>   len = sysctl_inet6(string, , mib, flags, );
>   if (len < 0)
>   return;
> + if (mib[2] == IPPROTO_IPV6 &&
> + mib[3] == IPV6CTL_SOIIKEY)
> + special |= HEX;
>  
>   if ((mib[2] == IPPROTO_IPV6 && mib[3] == 
> IPV6CTL_MRTMFC) ||
>   (mib[2] == IPPROTO_IPV6 && mib[3] == 
> IPV6CTL_MRTMIF) ||
> @@ -717,6 +768,27 @@ parse(char *string, int flags)
>   newval = 
>   newsize = sizeof(quadval);
>   break;
> + case CTLTYPE_STRING:
> + if (special & HEX) {
> + ssize_t len;
> +
> + len = parse_hex_string(hex, sizeof(hex),
> + newval);
> + if (len == -1) {
> + warnx("%s: hex string %s: invalid",
> + string, newval);
> + return;
> + }
> + if (len > sizeof(hex)) {
> + warnx("%s: hex string %s: too long",
> + string, newval);
> + return;
> + }
> +
> + newval = hex;
> + newsize = len;
> + }
> + break;
>   }
>   }
>   size = (special & SMALLBUF) ? 512 : SYSCTL_BUFSIZ;
> @@ -936,13 +1008,30 @@ parse(char *string, int flags)
>   if (newval == NULL) {
>   if (!nflag)
>   (void)printf("%s%s", string, equ);
> - (void)puts(buf);
> - } else {
> - if (!qflag) {
> - if (!nflag)
> - (void)printf("%s: %s -> ", string, buf);
> - (void)puts((char *)newval);
> + if (special & HEX) {
> + size_t i;
> + for (i = 0; i < 

Re: inteldrm(4) diff needs review and testing

[Andrew Marks]

Hello,

I happen to have a Haswell 4600 so I tried to apply this patch, I am
running a snapshot from 16 Jul 2017 and just updated my src.  My
drm_linux.h looks much different than the one your patch was meant for.
cvs log shows it to be revision 1.56 so I'm not sure where the
discrepancy lies. My drm_linux.h does not contain the lines provided in
the context of the diff.

Is there a tag or branch I should be pulling from? I've followed the
instructions following current.

This is the first time I've tried to apply a patch from this mailing
list so I suspect I am doing something wrong, any pointers?

On Sun, Jul 16, 2017 at 03:19:41PM +0200, Mark Kettenis wrote:
> Can somebody test the following diff on Ivy Bridge or Haswell (Intel
> HD Graphics 2500/4000/4600/4700/5000/5100/5200)?
> 
> When I added support for the command parser, I took a bit of a
> shortcut and implemented the hash tables as a single linked list.
> This diff fixes that.
> 
> For the hash function I used a "mode (size-1)" approach that leaves
> one of the hash table entries unused.  Perhaps somebody with a CS
> background has a better idea that isn't too complicated to implement?
> 
> Paul, Stuart, there is a small chance that this will improve the
> vncviewer performance.
> 
> 
> Index: dev/pci/drm/drm_linux.h
> ===
> RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.h,v
> retrieving revision 1.56
> diff -u -p -r1.56 drm_linux.h
> --- dev/pci/drm/drm_linux.h   14 Jul 2017 11:18:04 -  1.56
> +++ dev/pci/drm/drm_linux.h   16 Jul 2017 12:54:51 -
> @@ -40,6 +40,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  /* The Linux code doesn't meet our usual standards! */
>  #ifdef __clang__
> @@ -202,16 +203,42 @@ bitmap_weight(void *p, u_int n)
>   return sum;
>  }
>  
> -#define DECLARE_HASHTABLE(x, y) struct hlist_head x;
> +#define DECLARE_HASHTABLE(name, bits) struct hlist_head name[1 << (bits)]
>  
> -#define hash_init(x) INIT_HLIST_HEAD(&(x))
> -#define hash_add(x, y, z)hlist_add_head(y, &(x))
> -#define hash_del(x)  hlist_del_init(x)
> -#define hash_empty(x)hlist_empty(&(x))
> -#define hash_for_each_possible(a, b, c, d) \
> - hlist_for_each_entry(b, &(a), c)
> -#define hash_for_each_safe(a, b, c, d, e) (void)(b); \
> - hlist_for_each_entry_safe(d, c, &(a), e)
> +static inline void
> +__hash_init(struct hlist_head *table, u_int size)
> +{
> + u_int i;
> +
> + for (i = 0; i < size; i++)
> + INIT_HLIST_HEAD([i]);
> +}
> +
> +static inline bool
> +__hash_empty(struct hlist_head *table, u_int size)
> +{
> + u_int i;
> +
> + for (i = 0; i < size; i++) {
> + if (!hlist_empty([i]))
> + return false;
> + }
> +
> + return true;
> +}
> +
> +#define __hash(table, key)   [key % (nitems(table) - 1)]
> +
> +#define hash_init(table) __hash_init(table, nitems(table))
> +#define hash_add(table, node, key) \
> + hlist_add_head(node, __hash(table, key))
> +#define hash_del(node)   hlist_del_init(node)
> +#define hash_empty(table)__hash_empty(table, nitems(table))
> +#define hash_for_each_possible(table, obj, member, key) \
> + hlist_for_each_entry(obj, __hash(table, key), member)
> +#define hash_for_each_safe(table, i, tmp, obj, member)   \
> + for (i = 0; i < nitems(table); i++) \
> +hlist_for_each_entry_safe(obj, tmp, [i], member)
>  
>  #define ACCESS_ONCE(x)   (x)
>  
> 

Re: crossing makegap.sh

[Theo de Raadt]

I've ok'd this type of diff from visa@ before.  This piece is worth
doing.

However there are further problems to be resolved in this area, it is
not sufficient for all the problems. lld/binutils have a pile of
problems in this area.

Also, please don't send diffs which fix only one architecture, but
require fixes on all architectures.

>quite certain i won't come up w/anything better than what i have now,
>can't get over my loathe towards Makefiles
>
>So, given above, i guess this is just for the archives, but if anyone is
>up for fixing it for real, you'll have my thanks. :)
>-Artturi
>
>
>diff --git a/sys/conf/makegap.sh b/sys/conf/makegap.sh
>index e104163fb48..4142c1e0dd0 100644
>--- a/sys/conf/makegap.sh
>+++ b/sys/conf/makegap.sh
>@@ -68,4 +68,4 @@ SECTIONS {
> }
> __EOF__
> 
>-ld -r gap.link $GAPDUMMY -o gap.o
>+${LD} -r gap.link $GAPDUMMY -o gap.o
>diff --git a/sys/arch/amd64/conf/Makefile.amd64 
>b/sys/arch/amd64/conf/Makefile.amd64
>index 9234ad69f20..711f58da7e9 100644
>--- a/sys/arch/amd64/conf/Makefile.amd64
>+++ b/sys/arch/amd64/conf/Makefile.amd64
>@@ -128,7 +128,7 @@ makegap.sh:
>   cp $S/conf/makegap.sh $@
> 
> gap.o:Makefile makegap.sh
>-  sh makegap.sh 0x
>+  LD=${LD} sh makegap.sh 0x
> 
> vers.o: ${SYSTEM_DEP:Ngap.o}
>   sh $S/conf/newvers.sh
>diff --git a/sys/arch/armv7/conf/Makefile.armv7 
>b/sys/arch/armv7/conf/Makefile.armv7
>index 3d874f06593..677424a8298 100644
>--- a/sys/arch/armv7/conf/Makefile.armv7
>+++ b/sys/arch/armv7/conf/Makefile.armv7
>@@ -130,7 +130,7 @@ makegap.sh:
>   cp $S/conf/makegap.sh $@
> 
> gap.o:Makefile makegap.sh gapdummy.o
>-  sh makegap.sh 0x gapdummy.o
>+  LD=${LD} sh makegap.sh 0x gapdummy.o
> 
> vers.o: ${SYSTEM_DEP:Ngap.o}
>   sh $S/conf/newvers.sh
>
>

athn: fix performance regression on 2Ghz

[Stefan Sperling]

This diff fixes an athn(4) performance regression on 2GHz.

Before I added 11n support athn(4), the driver used to send RTS frames
at 2 Mbit/s while operating in the 2GHz band.
I changed this to 1 Mbit/s for consistency with other drivers.
However, it turns out that the chip does not do what I am asking for.

My athn(4) AP for some reason ends up sending RTS at frames 9 Mbit/s (OFDM).
Such frames get lost easily under poor radio conditions, which delays
data frames and hurts throughput. This fix restores the previous behaviour.

Index: ar5008.c
===
RCS file: /cvs/src/sys/dev/ic/ar5008.c,v
retrieving revision 1.44
diff -u -p -r1.44 ar5008.c
--- ar5008.c26 Apr 2017 07:53:17 -  1.44
+++ ar5008.c16 Jul 2017 13:00:37 -
@@ -1601,7 +1601,7 @@ ar5008_tx(struct athn_softc *sc, struct 
}
/* Select protection rate (suboptimal but ok). */
protridx = IEEE80211_IS_CHAN_5GHZ(ni->ni_chan) ?
-   ATHN_RIDX_OFDM6 : ATHN_RIDX_CCK1;
+   ATHN_RIDX_OFDM6 : ATHN_RIDX_CCK2;
if (ds->ds_ctl0 & AR_TXC0_RTS_ENABLE) {
/* Account for CTS duration. */
dur += athn_txtime(sc, IEEE80211_ACK_LEN,

Re: rc: Use here document for temporary pf rule set

[Klemens Nanni]

On Sun, Jul 16, 2017 at 03:24:19PM +0200, Klemens Nanni wrote:
> + print -- "$RULES" | pfctl -nf -
That should be 'pfctl -ef -' of course.

Re: rc: Use here document for temporary pf rule set

[Klemens Nanni]

On Sun, Jul 16, 2017 at 12:41:09PM +, Robert Peichaer wrote:
> On Sun, Jul 16, 2017 at 02:28:59PM +0200, Klemens Nanni wrote:
> > On Sun, Jul 16, 2017 at 12:11:55PM +, Robert Peichaer wrote:
> > > On Sun, Jul 16, 2017 at 01:37:56PM +0200, Klemens Nanni wrote:
> > > > This removes on level of indent, avoids the ugly RULES="$RULES ..."
> > > > repitition and spares a print.
> > > > 
> > > > We could do a 'pfctl -ef -' right away but I kept changing and enabling
> > > > clearly seperated. Regarding the leading newlines and tabs of the inner
> > > > echo: pf perfectly munges those, no need to clear them.
> > > > 
> > > > The "don't" -> "do not" is neccessary since otherwise the shell would
> > > > choke on it as opening quote.
> > > > 
> > > > 
> > > > Feedback? Comments?
> > > 
> > > Nice idea. The only maby irrelevant concern I have is, that using the
> > > here-document approach uses a temporary file and if that for some reason
> > > fails, we end up without this or mangled rules.
> > sh reads the temporary file in 512 bytes chunks, the here document is
> > about 2.0K in size.
> > 
> > I didn't bother intercepting sh with gdb and simulating a scenario where
> > the temporary file cannot be written but in case the user has no disk
> > space left I'd expect it to not be created at all since.
> > 
> > In general I'd say that if /tmp doesn't have 2.0K left users probably
> > have more serious problems anyway.
> 
> Have you thought about diskless(8) setups?
If /tmp is served via NFS and the here document's IO fails, 'pfctl -f -'
won't see any rule as it's not being executed.

I still think running out of space would cause more problems than just
this one but I shall be glad to be corrected.

Here's another approach still using $RULES but with saner quoting and
without the potentially dangerous here document.


Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc  4 Jul 2017 19:02:11 -   1.507
+++ rc  16 Jul 2017 13:23:02 -
@@ -402,31 +399,35 @@ wsconsctl_conf
 
 # Set initial temporary pf rule set.
 if [[ $pf != NO ]]; then
-   RULES="block all"
-   RULES="$RULES\npass on lo0"
-   RULES="$RULES\npass in proto tcp from any to any port ssh keep state"
-   RULES="$RULES\npass out proto { tcp, udp } from any to any port domain 
keep state"
-   RULES="$RULES\npass out inet proto icmp all icmp-type echoreq keep 
state"
-   RULES="$RULES\npass out inet proto udp from any port bootpc to any port 
bootps"
-   RULES="$RULES\npass in inet proto udp from any port bootps to any port 
bootpc"
-   if ifconfig lo0 inet6 >/dev/null 2>&1; then
-   RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type 
neighbrsol"
-   RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type 
neighbradv"
-   RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type 
routersol"
-   RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type 
routeradv"
-   RULES="$RULES\npass out inet6 proto udp from any port 
dhcpv6-client to any port dhcpv6-server"
-   RULES="$RULES\npass in inet6 proto udp from any port 
dhcpv6-server to any port dhcpv6-client"
-   fi
-   RULES="$RULES\npass in proto carp keep state (no-sync)"
-   RULES="$RULES\npass out proto carp !received-on any keep state 
(no-sync)"
-   if [[ $(sysctl vfs.mounts.nfs 2>/dev/null) == *[1-9]* ]]; then
-   # Don't kill NFS.
-   RULES="set reassemble yes no-df\n$RULES"
-   RULES="$RULES\npass in proto { tcp, udp } from any port { 
sunrpc, nfsd } to any"
-   RULES="$RULES\npass out proto { tcp, udp } from any to any port 
{ sunrpc, nfsd } !received-on any"
-   fi
-   print -- "$RULES" | pfctl -f -
-   pfctl -e
+   RULES='
+   block all
+   pass on lo0
+   pass in proto tcp from any to any port ssh keep state
+   pass out proto { tcp, udp } from any to any port domain keep state
+   pass out inet proto icmp all icmp-type echoreq keep state
+   pass out inet proto udp from any port bootpc to any port bootps
+   pass in inet proto udp from any port bootps to any port bootpc'
+
+   ifconfig lo0 inet6 >/dev/null 2>&1 &&
+   RULES="$RULES"'
+   pass out inet6 proto icmp6 all icmp6-type neighbrsol
+   pass in inet6 proto icmp6 all icmp6-type neighbradv
+   pass out inet6 proto icmp6 all icmp6-type routersol
+   pass in inet6 proto icmp6 all icmp6-type routeradv
+   pass out inet6 proto udp from any port dhcpv6-client to any 
port dhcpv6-server
+   pass in inet6 proto udp from any port dhcpv6-server to any port 
dhcpv6-client'
+
+   RULES="$RULES"'
+   pass in proto carp keep state (no-sync)
+   pass out proto carp !received-on any keep state (no-sync)'
+
+   # Don't kill 

inteldrm(4) diff needs review and testing

[Mark Kettenis]

Can somebody test the following diff on Ivy Bridge or Haswell (Intel
HD Graphics 2500/4000/4600/4700/5000/5100/5200)?

When I added support for the command parser, I took a bit of a
shortcut and implemented the hash tables as a single linked list.
This diff fixes that.

For the hash function I used a "mode (size-1)" approach that leaves
one of the hash table entries unused.  Perhaps somebody with a CS
background has a better idea that isn't too complicated to implement?

Paul, Stuart, there is a small chance that this will improve the
vncviewer performance.


Index: dev/pci/drm/drm_linux.h
===
RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.h,v
retrieving revision 1.56
diff -u -p -r1.56 drm_linux.h
--- dev/pci/drm/drm_linux.h 14 Jul 2017 11:18:04 -  1.56
+++ dev/pci/drm/drm_linux.h 16 Jul 2017 12:54:51 -
@@ -40,6 +40,7 @@
 
 #include 
 #include 
+#include 
 
 /* The Linux code doesn't meet our usual standards! */
 #ifdef __clang__
@@ -202,16 +203,42 @@ bitmap_weight(void *p, u_int n)
return sum;
 }
 
-#define DECLARE_HASHTABLE(x, y) struct hlist_head x;
+#define DECLARE_HASHTABLE(name, bits) struct hlist_head name[1 << (bits)]
 
-#define hash_init(x)   INIT_HLIST_HEAD(&(x))
-#define hash_add(x, y, z)  hlist_add_head(y, &(x))
-#define hash_del(x)hlist_del_init(x)
-#define hash_empty(x)  hlist_empty(&(x))
-#define hash_for_each_possible(a, b, c, d) \
-   hlist_for_each_entry(b, &(a), c)
-#define hash_for_each_safe(a, b, c, d, e) (void)(b); \
-   hlist_for_each_entry_safe(d, c, &(a), e)
+static inline void
+__hash_init(struct hlist_head *table, u_int size)
+{
+   u_int i;
+
+   for (i = 0; i < size; i++)
+   INIT_HLIST_HEAD([i]);
+}
+
+static inline bool
+__hash_empty(struct hlist_head *table, u_int size)
+{
+   u_int i;
+
+   for (i = 0; i < size; i++) {
+   if (!hlist_empty([i]))
+   return false;
+   }
+
+   return true;
+}
+
+#define __hash(table, key) [key % (nitems(table) - 1)]
+
+#define hash_init(table)   __hash_init(table, nitems(table))
+#define hash_add(table, node, key) \
+   hlist_add_head(node, __hash(table, key))
+#define hash_del(node) hlist_del_init(node)
+#define hash_empty(table)  __hash_empty(table, nitems(table))
+#define hash_for_each_possible(table, obj, member, key) \
+   hlist_for_each_entry(obj, __hash(table, key), member)
+#define hash_for_each_safe(table, i, tmp, obj, member) \
+   for (i = 0; i < nitems(table); i++) \
+  hlist_for_each_entry_safe(obj, tmp, [i], member)
 
 #define ACCESS_ONCE(x) (x)
 

crossing makegap.sh

[Artturi Alm]

Hi,

quite certain i won't come up w/anything better than what i have now,
can't get over my loathe towards Makefiles

So, given above, i guess this is just for the archives, but if anyone is
up for fixing it for real, you'll have my thanks. :)
-Artturi


diff --git a/sys/conf/makegap.sh b/sys/conf/makegap.sh
index e104163fb48..4142c1e0dd0 100644
--- a/sys/conf/makegap.sh
+++ b/sys/conf/makegap.sh
@@ -68,4 +68,4 @@ SECTIONS {
 }
 __EOF__
 
-ld -r gap.link $GAPDUMMY -o gap.o
+${LD} -r gap.link $GAPDUMMY -o gap.o
diff --git a/sys/arch/amd64/conf/Makefile.amd64 
b/sys/arch/amd64/conf/Makefile.amd64
index 9234ad69f20..711f58da7e9 100644
--- a/sys/arch/amd64/conf/Makefile.amd64
+++ b/sys/arch/amd64/conf/Makefile.amd64
@@ -128,7 +128,7 @@ makegap.sh:
cp $S/conf/makegap.sh $@
 
 gap.o: Makefile makegap.sh
-   sh makegap.sh 0x
+   LD=${LD} sh makegap.sh 0x
 
 vers.o: ${SYSTEM_DEP:Ngap.o}
sh $S/conf/newvers.sh
diff --git a/sys/arch/armv7/conf/Makefile.armv7 
b/sys/arch/armv7/conf/Makefile.armv7
index 3d874f06593..677424a8298 100644
--- a/sys/arch/armv7/conf/Makefile.armv7
+++ b/sys/arch/armv7/conf/Makefile.armv7
@@ -130,7 +130,7 @@ makegap.sh:
cp $S/conf/makegap.sh $@
 
 gap.o: Makefile makegap.sh gapdummy.o
-   sh makegap.sh 0x gapdummy.o
+   LD=${LD} sh makegap.sh 0x gapdummy.o
 
 vers.o: ${SYSTEM_DEP:Ngap.o}
sh $S/conf/newvers.sh

Re: [PATCH] etc/ksh.kshrc - unify command substitution

[Raf Czlonka]

Hi all,

Further simplification - 'ps | grep' can be replaced by pgrep(1)
and if-then-fi by &&.

> While there:
> 
> - remove ':' (null utility) from the very first line of the file -
>   I *do* understand what it does but it doesn't seem like it's needed
>   at all, unless I'm missing something (as is the case with some idioms)
> [...]
> 

As it transpired, this does indeed seem to be an old idiom denoting
a Bourne shell script.

To quote rpe@: "I guess it's fine to remove the : line in 2017."

I agree.

Thanks again to Robert for all the feedback and suggestions.

Regards,

Raf

Index: etc/ksh.kshrc
===
RCS file: /cvs/src/etc/ksh.kshrc,v
retrieving revision 1.28
diff -u -p -r1.28 ksh.kshrc
--- etc/ksh.kshrc   15 Jul 2017 07:11:42 -  1.28
+++ etc/ksh.kshrc   16 Jul 2017 11:49:55 -
@@ -1,4 +1,3 @@
-:
 #  $OpenBSD: ksh.kshrc,v 1.28 2017/07/15 07:11:42 tb Exp $
 #
 # NAME:
@@ -74,9 +73,7 @@ case "$-" in
xterm*)
ILS='\033]1;'; ILE='\007'
WLS='\033]2;'; WLE='\007'
-   if ps -p $PPID -o command | grep -q telnet; then
-   export TERM=xterms
-   fi
+   pgrep -qxs $PPID telnet && export TERM=xterms
;;
*)  ;;
esac

Re: rc: Use here document for temporary pf rule set

[Robert Peichaer]

On Sun, Jul 16, 2017 at 02:28:59PM +0200, Klemens Nanni wrote:
> On Sun, Jul 16, 2017 at 12:11:55PM +, Robert Peichaer wrote:
> > On Sun, Jul 16, 2017 at 01:37:56PM +0200, Klemens Nanni wrote:
> > > This removes on level of indent, avoids the ugly RULES="$RULES ..."
> > > repitition and spares a print.
> > > 
> > > We could do a 'pfctl -ef -' right away but I kept changing and enabling
> > > clearly seperated. Regarding the leading newlines and tabs of the inner
> > > echo: pf perfectly munges those, no need to clear them.
> > > 
> > > The "don't" -> "do not" is neccessary since otherwise the shell would
> > > choke on it as opening quote.
> > > 
> > > 
> > > Feedback? Comments?
> > 
> > Nice idea. The only maby irrelevant concern I have is, that using the
> > here-document approach uses a temporary file and if that for some reason
> > fails, we end up without this or mangled rules.
> sh reads the temporary file in 512 bytes chunks, the here document is
> about 2.0K in size.
> 
> I didn't bother intercepting sh with gdb and simulating a scenario where
> the temporary file cannot be written but in case the user has no disk
> space left I'd expect it to not be created at all since.
> 
> In general I'd say that if /tmp doesn't have 2.0K left users probably
> have more serious problems anyway.

Have you thought about diskless(8) setups?

-- 
-=[rpe]=-

Re: rc: Use here document for temporary pf rule set

[Klemens Nanni]

On Sun, Jul 16, 2017 at 12:11:55PM +, Robert Peichaer wrote:
> On Sun, Jul 16, 2017 at 01:37:56PM +0200, Klemens Nanni wrote:
> > This removes on level of indent, avoids the ugly RULES="$RULES ..."
> > repitition and spares a print.
> > 
> > We could do a 'pfctl -ef -' right away but I kept changing and enabling
> > clearly seperated. Regarding the leading newlines and tabs of the inner
> > echo: pf perfectly munges those, no need to clear them.
> > 
> > The "don't" -> "do not" is neccessary since otherwise the shell would
> > choke on it as opening quote.
> > 
> > 
> > Feedback? Comments?
> 
> Nice idea. The only maby irrelevant concern I have is, that using the
> here-document approach uses a temporary file and if that for some reason
> fails, we end up without this or mangled rules.
sh reads the temporary file in 512 bytes chunks, the here document is
about 2.0K in size.

I didn't bother intercepting sh with gdb and simulating a scenario where
the temporary file cannot be written but in case the user has no disk
space left I'd expect it to not be created at all since.

In general I'd say that if /tmp doesn't have 2.0K left users probably
have more serious problems anyway.

Re: rc: Use here document for temporary pf rule set

[Robert Peichaer]

On Sun, Jul 16, 2017 at 01:37:56PM +0200, Klemens Nanni wrote:
> This removes on level of indent, avoids the ugly RULES="$RULES ..."
> repitition and spares a print.
> 
> We could do a 'pfctl -ef -' right away but I kept changing and enabling
> clearly seperated. Regarding the leading newlines and tabs of the inner
> echo: pf perfectly munges those, no need to clear them.
> 
> The "don't" -> "do not" is neccessary since otherwise the shell would
> choke on it as opening quote.
> 
> 
> Feedback? Comments?

Nice idea. The only maby irrelevant concern I have is, that using the
here-document approach uses a temporary file and if that for some reason
fails, we end up without this or mangled rules.

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

[Klemens Nanni]

On Sun, Jul 16, 2017 at 11:55:09AM +, Robert Peichaer wrote:
> To the contrary what my previous answer might indicate, I don't care
> that much either. If you want to explicitely remove the blank, go for
> it.
Most problems I ever encountered while writing shell code were related
to (nested) quoting and unexpected characters (mostly whitespaces).

Leaving the resulting $_libas exactly as one would expect it to be
doesn't hurt plus the code's intent is perfectly clear while reading it.

See the updated diff.

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

[Robert Peichaer]

On Sun, Jul 16, 2017 at 01:55:02PM +0200, Klemens Nanni wrote:
> On Sun, Jul 16, 2017 at 10:26:25AM +, Robert Peichaer wrote:
> > But I'd like to stay strict matching the filenames.
> > 
> > +   for _liba in /usr/lib/lib{c,crypto}; do
> > +   _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> > +   done
> > 
> > > + _libas=${_libas# }
> Agreed, I had a similiar approach first but then tried to reduce the
> differences to the essentials.
> 
> Here's an updated diff taking this into while also dropping $_l together
> with this hunk instead of the other one.
> 
> Feedback?
> 
> Index: rc
> ===
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.507
> diff -u -p -r1.507 rc
> --- rc4 Jul 2017 19:02:11 -   1.507
> +++ rc16 Jul 2017 11:54:36 -
> @@ -158,7 +158,7 @@ make_keys() {
>  
>  # Re-link libraries, placing the objects in a random order.
>  reorder_libs() {
> - local _dkdev _l _liba _libas _mp _tmpdir _remount=false _error=false
> + local _dkdev _liba _libas _mp _tmpdir _remount=false _error=false
>  
>   [[ $library_aslr == NO ]] && return
>  
> @@ -171,13 +171,10 @@ reorder_libs() {
>   echo -n 'reordering libraries:'
>  
>   # Only choose the latest version of the libraries.
> - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> - for _l in $_libas; do
> - [[ $_l == $_liba ]] && continue 2
> - done
> - _libas="$_libas $_liba"
> + for _liba in /usr/lib/lib{c,crypto}; do
> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
>   done
> + _libas=${_libas# }
>  
>   # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
>   if [[ $_mp == *' type ffs '*'read-only'* ]]; then

Provided you tested this throughly, OK.

-- 
-=[rpe]=-

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

[Robert Peichaer]

On Sun, Jul 16, 2017 at 01:23:00PM +0200, Klemens Nanni wrote:
> On Sun, Jul 16, 2017 at 10:26:25AM +, Robert Peichaer wrote:
> > On Sun, Jul 16, 2017 at 03:37:15AM +0200, Klemens Nanni wrote:
> > > Why looping over all existing archives, picking the latest version of
> > > the current archive, skipping it in case it's already in our list of
> > > selected latest versions or adding it otherwise?
> > > 
> > > The current code runs ls|sort|tail about n * (v - 1) times for n
> > > different libraries and v versions respectively since the globbed list
> > > is almost always sorted already, effectively adding the latest versions
> > > after skipping all others.
> > 
> > Yeah well, until the globbing isn't sorting as expected.
> > 
> > $ ls -1 libc.so.*.a
> > libc.so.89.10.a
> > libc.so.89.6.a
> > libc.so.89.7.a
> > libc.so.89.8.a
> > libc.so.89.9.a
> Exactly, that's why sort(1) is used.
> > 
> > But that's not the point anyway in your otherwise perfectly valid change.
> >  
> > > This diff makes it much clearer and simpler by sorting and picking
> > > only as many versions as there are libraries to reorder (two). Globbing
> > > is done within the loop so future libraries with different naming
> > > schemes comes at no cost.
> > > 
> > > Applies cleanly to both the current revision as well as my previous diff
> > > but the previous one will fail on top of this one.
> > > 
> > > Feedback? Comments?
> > > 
> > > Index: rc
> > > ===
> > > RCS file: /cvs/src/etc/rc,v
> > > retrieving revision 1.507
> > > diff -u -p -r1.507 rc
> > > --- rc4 Jul 2017 19:02:11 -   1.507
> > > +++ rc16 Jul 2017 01:15:43 -
> > > @@ -171,13 +171,10 @@ reorder_libs() {
> > >   echo -n 'reordering libraries:'
> > >  
> > >   # Only choose the latest version of the libraries.
> > > - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> > > - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> > > - for _l in $_libas; do
> > > - [[ $_l == $_liba ]] && continue 2
> > > - done
> > > - _libas="$_libas $_liba"
> > > + for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
> > > + _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
> > >   done
> > 
> > That's definitely a much better approach.
> > 
> > But I'd like to stay strict matching the filenames.
> > 
> > +   for _liba in /usr/lib/lib{c,crypto}; do
> > +   _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> > +   done
> > 
> > > + _libas=${_libas# }
> > 
> > This would be another way of suppressing the blank right away.
> > But it's not really necessary anyway, because the leading blank is
> > removed in the list of the other for-loop.
> > 
> > _libas="${_libas:+$_libas }$(ls $_liba.so.+([0-9.]).a | sort -V | tail 
> > -1)"
> I explicitly avoided this since stripping the leading space in the end
> seemed clearer about what's going on. Also, this check is done for each
> library whereas only the first one is true.
> 
> I also thought about leaving it in but stripping it makes clear I know
> what I'm doing and future changes where $_libas might get quoted will
> be working as expected.

To the contrary what my previous answer might indicate, I don't care
that much either. If you want to explicitely remove the blank, go for
it.

-- 
-=[rpe]=-

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

[Klemens Nanni]

On Sun, Jul 16, 2017 at 10:26:25AM +, Robert Peichaer wrote:
> But I'd like to stay strict matching the filenames.
> 
> + for _liba in /usr/lib/lib{c,crypto}; do
> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> + done
> 
> > +   _libas=${_libas# }
Agreed, I had a similiar approach first but then tried to reduce the
differences to the essentials.

Here's an updated diff taking this into while also dropping $_l together
with this hunk instead of the other one.

Feedback?

Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc  4 Jul 2017 19:02:11 -   1.507
+++ rc  16 Jul 2017 11:54:36 -
@@ -158,7 +158,7 @@ make_keys() {
 
 # Re-link libraries, placing the objects in a random order.
 reorder_libs() {
-   local _dkdev _l _liba _libas _mp _tmpdir _remount=false _error=false
+   local _dkdev _liba _libas _mp _tmpdir _remount=false _error=false
 
[[ $library_aslr == NO ]] && return
 
@@ -171,13 +171,10 @@ reorder_libs() {
echo -n 'reordering libraries:'
 
# Only choose the latest version of the libraries.
-   for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
-   _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
-   for _l in $_libas; do
-   [[ $_l == $_liba ]] && continue 2
-   done
-   _libas="$_libas $_liba"
+   for _liba in /usr/lib/lib{c,crypto}; do
+   _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
done
+   _libas=${_libas# }
 
# Remount read-write, if /usr/lib is on a read-only ffs filesystem.
if [[ $_mp == *' type ffs '*'read-only'* ]]; then

rc: Use here document for temporary pf rule set

[Klemens Nanni]

This removes on level of indent, avoids the ugly RULES="$RULES ..."
repitition and spares a print.

We could do a 'pfctl -ef -' right away but I kept changing and enabling
clearly seperated. Regarding the leading newlines and tabs of the inner
echo: pf perfectly munges those, no need to clear them.

The "don't" -> "do not" is neccessary since otherwise the shell would
choke on it as opening quote.


Feedback? Comments?

Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.507
diff -u -p -r1.507 rc
--- rc  4 Jul 2017 19:02:11 -   1.507
+++ rc  16 Jul 2017 11:34:09 -
@@ -402,30 +402,33 @@ wsconsctl_conf
 
 # Set initial temporary pf rule set.
 if [[ $pf != NO ]]; then
-   RULES="block all"
-   RULES="$RULES\npass on lo0"
-   RULES="$RULES\npass in proto tcp from any to any port ssh keep state"
-   RULES="$RULES\npass out proto { tcp, udp } from any to any port domain 
keep state"
-   RULES="$RULES\npass out inet proto icmp all icmp-type echoreq keep 
state"
-   RULES="$RULES\npass out inet proto udp from any port bootpc to any port 
bootps"
-   RULES="$RULES\npass in inet proto udp from any port bootps to any port 
bootpc"
-   if ifconfig lo0 inet6 >/dev/null 2>&1; then
-   RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type 
neighbrsol"
-   RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type 
neighbradv"
-   RULES="$RULES\npass out inet6 proto icmp6 all icmp6-type 
routersol"
-   RULES="$RULES\npass in inet6 proto icmp6 all icmp6-type 
routeradv"
-   RULES="$RULES\npass out inet6 proto udp from any port 
dhcpv6-client to any port dhcpv6-server"
-   RULES="$RULES\npass in inet6 proto udp from any port 
dhcpv6-server to any port dhcpv6-client"
-   fi
-   RULES="$RULES\npass in proto carp keep state (no-sync)"
-   RULES="$RULES\npass out proto carp !received-on any keep state 
(no-sync)"
-   if [[ $(sysctl vfs.mounts.nfs 2>/dev/null) == *[1-9]* ]]; then
-   # Don't kill NFS.
-   RULES="set reassemble yes no-df\n$RULES"
-   RULES="$RULES\npass in proto { tcp, udp } from any port { 
sunrpc, nfsd } to any"
-   RULES="$RULES\npass out proto { tcp, udp } from any to any port 
{ sunrpc, nfsd } !received-on any"
-   fi
-   print -- "$RULES" | pfctl -f -
+   pfctl -f - <<- __EOF__
+block all
+pass on lo0
+pass in proto tcp from any to any port ssh keep state
+pass out proto { tcp, udp } from any to any port domain keep state
+pass out inet proto icmp all icmp-type echoreq keep state
+pass out inet proto udp from any port bootpc to any port bootps
+pass in inet proto udp from any port bootps to any port bootpc
+$(if ifconfig lo0 inet6 >/dev/null 2>&1; then
+   echo '
+   pass out inet6 proto icmp6 all icmp6-type neighbrsol
+   pass in inet6 proto icmp6 all icmp6-type neighbradv
+   pass out inet6 proto icmp6 all icmp6-type routersol
+   pass in inet6 proto icmp6 all icmp6-type routeradv
+   pass out inet6 proto udp from any port dhcpv6-client to any port 
dhcpv6-server
+   pass in inet6 proto udp from any port dhcpv6-server to any port 
dhcpv6-client'
+fi)
+pass in proto carp keep state (no-sync)
+pass out proto carp !received-on any keep state (no-sync)
+$(if [[ $(sysctl vfs.mounts.nfs 2>/dev/null) == *[1-9]* ]]; then
+   # Do not kill NFS.
+   echo '
+   set reassemble yes no-df
+   pass in proto { tcp, udp } from any port { sunrpc, nfsd } to any
+   pass out proto { tcp, udp } from any to any port { sunrpc, nfsd } 
!received-on any'
+fi)
+__EOF__
pfctl -e
 fi
 

Re: rc: reorder_libs: [1/2] Drop unused _l, exit early on failure

[Klemens Nanni]

On Sun, Jul 16, 2017 at 07:18:00AM +0200, Theo Buehler wrote:
> On Sun, Jul 16, 2017 at 03:34:07AM +0200, Klemens Nanni wrote:
> > $_l is not used and picking the latest archive versions is of no use
> > if /usr/lib cannot be written to.
> > 
> > This patch applies cleanly before my next one but not vice versa.
> > 
> > Feedback? OK?
Oh, that OK should've been gone.

> _l is only unused after your second patch :)
Indeed, a bit ugly now that I'm looking at it again but that's why those
two should only go together.


> hoisting the remount over picking the library version makes sense,
> but you should keep it after the "echo -n 'reordering libraries:'"
Fair point.

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

[Klemens Nanni]

On Sun, Jul 16, 2017 at 10:26:25AM +, Robert Peichaer wrote:
> On Sun, Jul 16, 2017 at 03:37:15AM +0200, Klemens Nanni wrote:
> > Why looping over all existing archives, picking the latest version of
> > the current archive, skipping it in case it's already in our list of
> > selected latest versions or adding it otherwise?
> > 
> > The current code runs ls|sort|tail about n * (v - 1) times for n
> > different libraries and v versions respectively since the globbed list
> > is almost always sorted already, effectively adding the latest versions
> > after skipping all others.
> 
> Yeah well, until the globbing isn't sorting as expected.
> 
> $ ls -1 libc.so.*.a
> libc.so.89.10.a
> libc.so.89.6.a
> libc.so.89.7.a
> libc.so.89.8.a
> libc.so.89.9.a
Exactly, that's why sort(1) is used.
> 
> But that's not the point anyway in your otherwise perfectly valid change.
>  
> > This diff makes it much clearer and simpler by sorting and picking
> > only as many versions as there are libraries to reorder (two). Globbing
> > is done within the loop so future libraries with different naming
> > schemes comes at no cost.
> > 
> > Applies cleanly to both the current revision as well as my previous diff
> > but the previous one will fail on top of this one.
> > 
> > Feedback? Comments?
> > 
> > Index: rc
> > ===
> > RCS file: /cvs/src/etc/rc,v
> > retrieving revision 1.507
> > diff -u -p -r1.507 rc
> > --- rc  4 Jul 2017 19:02:11 -   1.507
> > +++ rc  16 Jul 2017 01:15:43 -
> > @@ -171,13 +171,10 @@ reorder_libs() {
> > echo -n 'reordering libraries:'
> >  
> > # Only choose the latest version of the libraries.
> > -   for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> > -   _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> > -   for _l in $_libas; do
> > -   [[ $_l == $_liba ]] && continue 2
> > -   done
> > -   _libas="$_libas $_liba"
> > +   for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
> > +   _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
> > done
> 
> That's definitely a much better approach.
> 
> But I'd like to stay strict matching the filenames.
> 
> + for _liba in /usr/lib/lib{c,crypto}; do
> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> + done
> 
> > +   _libas=${_libas# }
> 
> This would be another way of suppressing the blank right away.
> But it's not really necessary anyway, because the leading blank is
> removed in the list of the other for-loop.
> 
>   _libas="${_libas:+$_libas }$(ls $_liba.so.+([0-9.]).a | sort -V | tail 
> -1)"
I explicitly avoided this since stripping the leading space in the end
seemed clearer about what's going on. Also, this check is done for each
library whereas only the first one is true.

I also thought about leaving it in but stripping it makes clear I know
what I'm doing and future changes where $_libas might get quoted will
be working as expected.

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

[Theo Buehler]

> But I'd like to stay strict matching the filenames.
> 
> + for _liba in /usr/lib/lib{c,crypto}; do
> + _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
> + done

Yes, this is indeed better.

So Klemens, can you write a patch that uses this and removes the then
unused _l. We can think about the hoisting in a second step.

> > +   _libas=${_libas# }
> 
> This would be another way of suppressing the blank right away.
> But it's not really necessary anyway, because the leading blank is
> removed in the list of the other for-loop.
> 
>   _libas="${_libas:+$_libas }$(ls $_liba.so.+([0-9.]).a | sort -V | tail 
> -1)"

While this is is clever, it does a bit too much in one line for my taste.

I'm fine with Klemens's version of zapping the blank after the for loop,
but we can also omit this, I don't really care.

>   
> > # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
> > if [[ $_mp == *' type ffs '*'read-only'* ]]; then
> > 
> 
> -- 
> -=[rpe]=-
> 

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

[Robert Peichaer]

On Sun, Jul 16, 2017 at 03:37:15AM +0200, Klemens Nanni wrote:
> Why looping over all existing archives, picking the latest version of
> the current archive, skipping it in case it's already in our list of
> selected latest versions or adding it otherwise?
> 
> The current code runs ls|sort|tail about n * (v - 1) times for n
> different libraries and v versions respectively since the globbed list
> is almost always sorted already, effectively adding the latest versions
> after skipping all others.

Yeah well, until the globbing isn't sorting as expected.

$ ls -1 libc.so.*.a
libc.so.89.10.a
libc.so.89.6.a
libc.so.89.7.a
libc.so.89.8.a
libc.so.89.9.a

But that's not the point anyway in your otherwise perfectly valid change.
 
> This diff makes it much clearer and simpler by sorting and picking
> only as many versions as there are libraries to reorder (two). Globbing
> is done within the loop so future libraries with different naming
> schemes comes at no cost.
> 
> Applies cleanly to both the current revision as well as my previous diff
> but the previous one will fail on top of this one.
> 
> Feedback? Comments?
> 
> Index: rc
> ===
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.507
> diff -u -p -r1.507 rc
> --- rc4 Jul 2017 19:02:11 -   1.507
> +++ rc16 Jul 2017 01:15:43 -
> @@ -171,13 +171,10 @@ reorder_libs() {
>   echo -n 'reordering libraries:'
>  
>   # Only choose the latest version of the libraries.
> - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> - for _l in $_libas; do
> - [[ $_l == $_liba ]] && continue 2
> - done
> - _libas="$_libas $_liba"
> + for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
> + _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
>   done

That's definitely a much better approach.

But I'd like to stay strict matching the filenames.

+   for _liba in /usr/lib/lib{c,crypto}; do
+   _libas="$_libas $(ls $_liba.so.+([0-9.]).a | sort -V | tail -1)"
+   done

> + _libas=${_libas# }

This would be another way of suppressing the blank right away.
But it's not really necessary anyway, because the leading blank is
removed in the list of the other for-loop.

_libas="${_libas:+$_libas }$(ls $_liba.so.+([0-9.]).a | sort -V | tail 
-1)"
  
>   # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
>   if [[ $_mp == *' type ffs '*'read-only'* ]]; then
> 

-- 
-=[rpe]=-

Re: rc: reorder_libs: [1/2] Drop unused _l, exit early on failure

[Robert Peichaer]

On Sun, Jul 16, 2017 at 07:18:00AM +0200, Theo Buehler wrote:
> On Sun, Jul 16, 2017 at 03:34:07AM +0200, Klemens Nanni wrote:
> > $_l is not used and picking the latest archive versions is of no use
> > if /usr/lib cannot be written to.
> > 
> > This patch applies cleanly before my next one but not vice versa.
> > 
> > Feedback? OK?
> 
> _l is only unused after your second patch :)
> 
> hoisting the remount over picking the library version makes sense,
> but you should keep it after the "echo -n 'reordering libraries:'"

The rationale to picking the library versions before remounting was
to keep the time window having rw /usr as small as possible.
If that's deemed ok, I'm too OK with switching the steps.

Re: rc: reorder_libs: [2/2] Pick archive versions more efficiently

[Theo Buehler]

On Sun, Jul 16, 2017 at 03:37:15AM +0200, Klemens Nanni wrote:
> Why looping over all existing archives, picking the latest version of
> the current archive, skipping it in case it's already in our list of
> selected latest versions or adding it otherwise?
> 
> The current code runs ls|sort|tail about n * (v - 1) times for n
> different libraries and v versions respectively since the globbed list
> is almost always sorted already, effectively adding the latest versions
> after skipping all others.
> 
> This diff makes it much clearer and simpler by sorting and picking
> only as many versions as there are libraries to reorder (two). Globbing
> is done within the loop so future libraries with different naming
> schemes comes at no cost.
> 
> Applies cleanly to both the current revision as well as my previous diff
> but the previous one will fail on top of this one.
> 
> Feedback? Comments?

I like this. It's much clearer.

I think it's ok to commit this, but I wonder if we should test it in
snapshots first.

> 
> Index: rc
> ===
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.507
> diff -u -p -r1.507 rc
> --- rc4 Jul 2017 19:02:11 -   1.507
> +++ rc16 Jul 2017 01:15:43 -
> @@ -171,13 +171,10 @@ reorder_libs() {
>   echo -n 'reordering libraries:'
>  
>   # Only choose the latest version of the libraries.
> - for _liba in /usr/lib/libc.so.*.a /usr/lib/libcrypto.so.*.a; do
> - _liba=$(ls ${_liba%%.[0-9]*}*.a | sort -V | tail -1)
> - for _l in $_libas; do
> - [[ $_l == $_liba ]] && continue 2
> - done
> - _libas="$_libas $_liba"
> + for _liba in 'libc.so.*.a' 'libcrypto.so.*.a'; do
> + _libas="$_libas $(ls /usr/lib/$_liba | sort -V | tail -1)"
>   done
> + _libas=${_libas# }
>  
>   # Remount read-write, if /usr/lib is on a read-only ffs filesystem.
>   if [[ $_mp == *' type ffs '*'read-only'* ]]; then
>