Re: Better handling of short reads

[Amit Kulkarni]

On Wed, 7 Jun 2017 21:27:27 -0500
Amit Kulkarni  wrote:

> On Thu, 8 Jun 2017 01:57:25 +0200
> Mike Belopuhov  wrote:
> 
> > On Wed, Jun 07, 2017 at 18:35 -0500, Amit Kulkarni wrote:
> > > Wow, please get this in!!!
> > > 
> > > This fixes cvs update on hard disks, to go much much faster. When I am
> > > updating the entire set of cvs trees: www, src, xenocara, ports, I can
> > > still use firefox and have it perfectly usable. There's a night and
> > > day improvement, before and after. Thanks for debugging and fixing
> > > this.
> > >
> > 
> > What kind of broken hardware do you have that this diff helps you?
> > Can you show us your dmesg?
> > 

Please ignore previous dmesg, it was incomplete.

OpenBSD 6.1-current (GENERIC.MP) #0: Wed Jun  7 18:11:29 CDT 2017
a...@pilloo-saru.my.domain:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 4183433216 (3989MB)
avail mem = 4050853888 (3863MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.8 @ 0xecc70 (82 entries)
bios0: vendor LENOVO version "M0KKT23A" date 02/19/2016
bios0: LENOVO 10HS007RUS
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP APIC FPDT DBGP SLIC MSDM SSDT SSDT MCFG HPET SSDT SSDT 
BGRT LUFT
acpi0: wakeup devices PXSX(S4) RP01(S4) PXSX(S4) PXSX(S4) RP03(S4) PXSX(S4) 
PXSX(S4) RP05(S4) PXSX(S4) PXSX(S4) PXSX(S4) GLAN(S4) EHC1(S3) EHC2(S3) 
XHC_(S3) HDEF(S4) [...]
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i3-4170 CPU @ 3.70GHz, 3692.03 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,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,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 3692032960 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 2 (application processor)
cpu1: Intel(R) Core(TM) i3-4170 CPU @ 3.70GHz, 3691.46 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,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,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 0, core 1, package 0
cpu2 at mainbus0: apid 1 (application processor)
cpu2: Intel(R) Core(TM) i3-4170 CPU @ 3.70GHz, 3691.46 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,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,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 1, core 0, package 0
cpu3 at mainbus0: apid 3 (application processor)
cpu3: Intel(R) Core(TM) i3-4170 CPU @ 3.70GHz, 3691.46 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,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,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 8 pa 0xfec0, version 20, 24 pins
acpimcfg0 at acpi0 addr 0xf800, bus 0-63
acpihpet0 at acpi0: 14318179 Hz
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus 1 (RP01)
acpiprt2 at acpi0: bus 2 (RP03)
acpiprt3 at acpi0: bus 3 (RP05)
acpiprt4 at acpi0: bus -1 (PEG0)
acpiprt5 at acpi0: bus -1 (PEG1)
acpiprt6 at acpi0: bus -1 (PEG2)
acpiec0 at acpi0: not present
acpicpu0 at acpi0: C2(200@148 mwait.1@0x31), C1(1000@1 mwait.1), PSS
acpicpu1 at acpi0: C2(200@148 mwait.1@0x31), C1(1000@1 mwait.1), PSS
acpicpu2 at acpi0: C2(200@148 mwait.1@0x31), C1(1000@1 mwait.1), PSS
acpicpu3 at acpi0: C2(200@148 mwait.1@0x31), C1(1000@1 mwait.1), PSS
acpipwrres0 at acpi0: FN00, resource for FAN0
acpipwrres1 at acpi0: FN01, resource for FAN1
acpipwrres2 at acpi0: FN02, resource for FAN2
acpipwrres3 at acpi0: FN03, resource for FAN3
acpipwrres4 at acpi0: FN04, resource for FAN4
acpitz0 at acpi0: critical temperature is 105 degC
acpitz1 at acpi0: critical temperature is 105 degC
"INT3F0D" at acpi0 not configured
"PNP0501" at acpi0 not configured
acpibtn0 at acpi0: PWRB

Re: Better handling of short reads

[Amit Kulkarni]

On Thu, 8 Jun 2017 01:57:25 +0200
Mike Belopuhov  wrote:

> On Wed, Jun 07, 2017 at 18:35 -0500, Amit Kulkarni wrote:
> > Wow, please get this in!!!
> > 
> > This fixes cvs update on hard disks, to go much much faster. When I am
> > updating the entire set of cvs trees: www, src, xenocara, ports, I can
> > still use firefox and have it perfectly usable. There's a night and
> > day improvement, before and after. Thanks for debugging and fixing
> > this.
> >
> 
> What kind of broken hardware do you have that this diff helps you?
> Can you show us your dmesg?
> 


OpenBSD 6.1-current (GENERIC.MP) #0: Wed Jun  7 18:11:29 CDT 2017
a...@pilloo-saru.my.domain:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 4183433216 (3989MB)
avail mem = 4050853888 (3863MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.8 @ 0xecc70 (82 entries)
bios0: vendor LENOVO version "M0KKT23A" date 02/19/2016
bios0: LENOVO 10HS007RUS
acpi0 at bios0: rev 2
acpi0: sleep states S0 S3 S4 S5
acpi0: tables DSDT FACP APIC FPDT DBGP SLIC MSDM SSDT SSDT MCFG HPET SSDT SSDT 
BGRT LUFT
acpi0: wakeup devices PXSX(S4) RP01(S4) PXSX(S4) PXSX(S4) RP03(S4) PXSX(S4) 
PXSX(S4) RP05(S4) PXSX(S4) PXSX(S4) PXSX(S4) GLAN(S4) EHC1(S3) EHC2(S3) 
XHC_(S3) HDEF(S4) [...]
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Core(TM) i3-4170 CPU @ 3.70GHz, 3691.97 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,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,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 3691968320 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 2 (application processor)
cpu1: Intel(R) Core(TM) i3-4170 CPU @ 3.70GHz, 3691.45 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,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,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 0, core 1, package 0
cpu2 at mainbus0: apid 1 (application processor)
cpu2: Intel(R) Core(TM) i3-4170 CPU @ 3.70GHz, 3691.45 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,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,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 1, core 0, package 0
cpu3 at mainbus0: apid 3 (application processor)
cpu3: Intel(R) Core(TM) i3-4170 CPU @ 3.70GHz, 3691.45 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,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,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 8 pa 0xfec0, version 20, 24 pins
acpimcfg0 at acpi0 addr 0xf800, bus 0-63
acpihpet0 at acpi0: 14318179 Hz
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus 1 (RP01)
acpiprt2 at acpi0: bus 2 (RP03)
acpiprt3 at acpi0: bus 3 (RP05)
acpiprt4 at acpi0: bus -1 (PEG0)
acpiprt5 at acpi0: bus -1 (PEG1)
acpiprt6 at acpi0: bus -1 (PEG2)
acpiec0 at acpi0: not present
acpicpu0 at acpi0: C2(200@148 mwait.1@0x31), C1(1000@1 mwait.1), PSS
acpicpu1 at acpi0: C2(200@148 mwait.1@0x31), C1(1000@1 mwait.1), PSS
acpicpu2 at acpi0: C2(200@148 mwait.1@0x31), C1(1000@1 mwait.1), PSS
acpicpu3 at acpi0: C2(200@148 mwait.1@0x31), C1(1000@1 mwait.1), PSS
acpipwrres0 at acpi0: FN00, resource for FAN0
acpipwrres1 at acpi0: FN01, resource for FAN1
acpipwrres2 at acpi0: FN02, resource for FAN2
acpipwrres3 at acpi0: FN03, resource for FAN3
acpipwrres4 at acpi0: FN04, resource for FAN4
acpitz0 at acpi0: critical temperature is 105 degC
acpitz1 at acpi0: critical temperature is 105 degC
"INT3F0D" at acpi0 not configured
"PNP0501" at acpi0 not configured
acpibtn0 at acpi0: PWRB
"PNP0C0B" at acpi0 not configured
"PNP0C0B" at acpi0 not configured
"PNP0C0B" at acpi0 not configured
"PNP0C0B" at acpi0 not configured
"PNP0C0B" at acpi0 not 

Re: diff: add missing rtm_send to nd6

[Alexander Bluhm]

On Thu, Jun 08, 2017 at 12:47:00AM +0200, Jan Klemkow wrote:
> This diff adds a missing routing message to the neighbor discovery code.
> The message informs the userland about new reachable IPv6 nodes on the
> network.  The IPv6 network stack acts more like the IPv4 port by this
> diff.  Now, the behavior is like arpcache() in netinet/if_ether.c on
> line 653.

I like consistency between IPv4 and IPv6.
OK bluhm@

> Index: nd6_nbr.c
> ===
> RCS file: /cvs/src/sys/netinet6/nd6_nbr.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 nd6_nbr.c
> --- nd6_nbr.c 16 May 2017 12:24:04 -  1.116
> +++ nd6_nbr.c 7 Jun 2017 22:11:31 -
> @@ -715,6 +715,8 @@ nd6_na_input(struct mbuf *m, int off, in
>   if (is_solicited) {
>   ln->ln_state = ND6_LLINFO_REACHABLE;
>   ln->ln_byhint = 0;
> + /* Notify userland that a new ND entry is reachable. */
> + rtm_send(rt, RTM_RESOLVE, ifp->if_rdomain);
>   if (!ND6_LLINFO_PERMANENT(ln)) {
>   nd6_llinfo_settimer(ln,
>   ND_IFINFO(ifp)->reachable);

Re: Better handling of short reads

[Mike Belopuhov]

On Wed, Jun 07, 2017 at 18:35 -0500, Amit Kulkarni wrote:
> Wow, please get this in!!!
> 
> This fixes cvs update on hard disks, to go much much faster. When I am
> updating the entire set of cvs trees: www, src, xenocara, ports, I can
> still use firefox and have it perfectly usable. There's a night and
> day improvement, before and after. Thanks for debugging and fixing
> this.
>

What kind of broken hardware do you have that this diff helps you?
Can you show us your dmesg?

> amit
> 
> On Wed, Jun 7, 2017 at 12:29 PM, Mike Belopuhov  wrote:
> > Hi,
> >
> > I've discovered that short reads (nonzero b_resid) aren't
> > handled very well in our kernel and I've proposed a diff
> > like this to handle short reads of buffercache read-ahead
> > buffers:
> >
[...]

Re: Better handling of short reads

[Amit Kulkarni]

Wow, please get this in!!!

This fixes cvs update on hard disks, to go much much faster. When I am
updating the entire set of cvs trees: www, src, xenocara, ports, I can
still use firefox and have it perfectly usable. There's a night and
day improvement, before and after. Thanks for debugging and fixing
this.

amit

On Wed, Jun 7, 2017 at 12:29 PM, Mike Belopuhov  wrote:
> Hi,
>
> I've discovered that short reads (nonzero b_resid) aren't
> handled very well in our kernel and I've proposed a diff
> like this to handle short reads of buffercache read-ahead
> buffers:
>
> diff --git sys/kern/vfs_bio.c sys/kern/vfs_bio.c
> index 95bc80bc0e6..1cc1943d752 100644
> --- sys/kern/vfs_bio.c
> +++ sys/kern/vfs_bio.c
> @@ -534,11 +534,27 @@ bread_cluster_callback(struct buf *bp)
>  */
> buf_fix_mapping(bp, newsize);
> bp->b_bcount = newsize;
> }
>
> -   for (i = 1; xbpp[i] != 0; i++) {
> +   /* Invalidate read-ahead buffers if read short */
> +   if (bp->b_resid > 0) {
> +   for (i = 0; xbpp[i] != NULL; i++)
> +   continue;
> +   for (i = i - 1; i != 0; i--) {
> +   if (xbpp[i]->b_bufsize <= bp->b_resid) {
> +   bp->b_resid -= xbpp[i]->b_bufsize;
> +   SET(xbpp[i]->b_flags, B_INVAL);
> +   } else if (bp->b_resid > 0) {
> +   bp->b_resid = 0;
> +   SET(xbpp[i]->b_flags, B_INVAL);
> +   } else
> +   break;
> +   }
> +   }
> +
> +   for (i = 1; xbpp[i] != NULL; i++) {
> if (ISSET(bp->b_flags, B_ERROR))
> SET(xbpp[i]->b_flags, B_INVAL | B_ERROR);
> biodone(xbpp[i]);
> }
>
>
> Now I said before that the only issue that this diff didn't
> fix was with the xbpp[0] aka the buf we return to FFS: if we
> have a 64k sized cluster on our filesystem then we've never
> created read-ahead bufs and thus this code never runs and we
> never account for the b_resid.  However, this is thankfully
> not correct as FFS handles short reads itself (except one
> small detail...). Here's a chunk from ffs_read:
>
> if (lblktosize(fs, nextlbn) >= DIP(ip, size))
> error = bread(vp, lbn, size, );
> else
> error = bread_cluster(vp, lbn, size, );
>
> if (error)
> break;
>
> /*
>  * We should only get non-zero b_resid when an I/O error
>  * has occurred, which should cause us to break above.
>  * However, if the short read did not cause an error,
>  * then we want to ensure that we do not uiomove bad
>  * or uninitialized data.
>  */
> size -= bp->b_resid;
> if (size < xfersize) {
> if (size == 0)
> break;
> xfersize = size;
> }
> error = uiomove(bp->b_data + blkoffset, xfersize, uio);
>
> As you can see it copies (size - bp->b_resid) into the uio.
> That would be OK if b_resid was as large as the 'size'. But
> due to how bread_cluster extends the b_count to cover for
> all additional read-ahead buffers, the transfer in the end
> can have a b_resid anywhere in the interval of [0, MAXPHYS]
> which can be larger than 'size' that FFS has asked for.
>
> This leads to 'size' underflow because it's an integer and
> then uiomove gets a negative value for xfersize which gets
> converted to a very large unsigned long (size_t) parameter
> for uiomove. And this is bad.  Therefore, additionally I'd
> like to assert this in the FFS code itself.  If this is the
> way to go, I'll look into other filesystems and propose a
> similar check.
>
> diff --git sys/ufs/ffs/ffs_vnops.c sys/ufs/ffs/ffs_vnops.c
> index 160e187820f..56c222612a2 100644
> --- sys/ufs/ffs/ffs_vnops.c
> +++ sys/ufs/ffs/ffs_vnops.c
> @@ -244,10 +244,11 @@ ffs_read(void *v)
>  * has occurred, which should cause us to break above.
>  * However, if the short read did not cause an error,
>  * then we want to ensure that we do not uiomove bad
>  * or uninitialized data.
>  */
> +   KASSERT(bp->b_resid <= size);
> size -= bp->b_resid;
> if (size < xfersize) {
> if (size == 0)
> break;
> xfersize = size;
>
>
> So to make it clear: I'd like to commit both changes and
> if that's something we agree upon, I'll look into other
> filesystems and make sure that they implement similar
> assertions.
>
> Opinions?
>

diff: add missing rtm_send to nd6

[Jan Klemkow]

Hi,

This diff adds a missing routing message to the neighbor discovery code.
The message informs the userland about new reachable IPv6 nodes on the
network.  The IPv6 network stack acts more like the IPv4 port by this
diff.  Now, the behavior is like arpcache() in netinet/if_ether.c on
line 653.

Bye,
Jan

Index: nd6_nbr.c
===
RCS file: /cvs/src/sys/netinet6/nd6_nbr.c,v
retrieving revision 1.116
diff -u -p -r1.116 nd6_nbr.c
--- nd6_nbr.c   16 May 2017 12:24:04 -  1.116
+++ nd6_nbr.c   7 Jun 2017 22:11:31 -
@@ -715,6 +715,8 @@ nd6_na_input(struct mbuf *m, int off, in
if (is_solicited) {
ln->ln_state = ND6_LLINFO_REACHABLE;
ln->ln_byhint = 0;
+   /* Notify userland that a new ND entry is reachable. */
+   rtm_send(rt, RTM_RESOLVE, ifp->if_rdomain);
if (!ND6_LLINFO_PERMANENT(ln)) {
nd6_llinfo_settimer(ln,
ND_IFINFO(ifp)->reachable);

Re: brconfig: Unify/fix strtoul(3) handling

[Klemens Nanni]

On Wed, Jun 07, 2017 at 01:17:10PM -0400, Ted Unangst wrote:

Klemens Nanni wrote:

Besides fixing a tautological 'v < 0' and using more descriptive/less
errorprone sizeof(target) this patch unifies strtoul(3) handling both
logically and cosmetically.


A great deal of these look like exactly the situation strtonum was desinged to
cope with. this means dropping support for hex bases, but i'm happy telling
people they need to learn decimal. :)


Leave error checks to strtonum(3) and use data types properly reflecting
those of the target's struct member. This also fixes my previously
introduced systematic flaw:

sizeof(int) != INT_MAX

sizeof is clearly smaller; in fact reliably finding a variable's maximum 
value at runtime is impossible in C.



We might also pass the limits to strtonum directly so ifconfig can tell
us what's wrong, e.g.

-   v = strtonum(value, 0, UINT8_MAX, );
+   v = strtonum(value, 1, 10, );

# ifconfig bridge0 holdcnt 0
-   ifconfig: too small arg for holdcnt: 0  # strtonum()
+   ifconfig: bridge0: Invalid argument # ioctl()

This is left unchanged for now since it would also mean having our
limits defined twice which isn't neccessarily a good idea with regard to
future changes.

Feedback/OK?


Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.15
diff -u -p -r1.15 brconfig.c
--- brconfig.c  7 Jun 2017 16:47:29 -   1.15
+++ brconfig.c  7 Jun 2017 21:30:56 -
@@ -220,7 +220,6 @@ bridge_ifsetflag(const char *ifsname, u_
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);

req.ifbr_ifsflags |= flag & ~IFBIF_RO_MASK;
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -232,12 +231,10 @@ bridge_ifclrflag(const char *ifsname, u_

strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
strlcpy(req.ifbr_ifsname, ifsname, sizeof(req.ifbr_ifsname));
-
if (ioctl(s, SIOCBRDGGIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);

req.ifbr_ifsflags &= ~(flag | IFBIF_RO_MASK);
-
if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
}
@@ -400,15 +397,12 @@ void
bridge_timeout(const char *arg, int d)
{
struct ifbrparam bp;
-   long newtime;
-   char *endptr;
+   int newtime;
+   const char *errstr;

-   errno = 0;
-   newtime = strtol(arg, , 0);
-   if (arg[0] == '\0' || endptr[0] != '\0' ||
-   (newtime & ~INT_MAX) != 0L ||
-   (errno == ERANGE && newtime == LONG_MAX))
-   errx(1, "invalid arg for timeout: %s", arg);
+   newtime = strtonum(arg, 0, INT_MAX, );
+   if (errstr != NULL)
+   errx(1, "%s arg for timeout: %s", errstr, arg);

strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_ctime = newtime;
@@ -420,14 +414,12 @@ void
bridge_maxage(const char *arg, int d)
{
struct ifbrparam bp;
-   unsigned long v;
-   char *endptr;
+   uint8_t v;
+   const char *errstr;

-   errno = 0;
-   v = strtoul(arg, , 0);
-   if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
-   (errno == ERANGE && v == ULONG_MAX))
-   errx(1, "invalid arg for maxage: %s", arg);
+   v = strtonum(arg, 0, UINT8_MAX, );
+   if (errstr != NULL)
+   errx(1, "%s arg for maxage: %s", errstr, arg);

strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_maxage = v;
@@ -439,14 +431,12 @@ void
bridge_priority(const char *arg, int d)
{
struct ifbrparam bp;
-   unsigned long v;
-   char *endptr;
+   uint16_t v;
+   const char *errstr;

-   errno = 0;
-   v = strtoul(arg, , 0);
-   if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xUL ||
-   (errno == ERANGE && v == ULONG_MAX))
-   errx(1, "invalid arg for spanpriority: %s", arg);
+   v = strtonum(arg, 0, UINT16_MAX, );
+   if (errstr != NULL)
+   errx(1, "%s arg for spanpriority: %s", errstr, arg);

strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
bp.ifbrp_prio = v;
@@ -478,14 +468,12 @@ void
bridge_fwddelay(const char *arg, int d)
{
struct ifbrparam bp;
-   unsigned long v;
-   char *endptr;
+   uint8_t v;
+   const char *errstr;

-   errno = 0;
-   v = strtoul(arg, , 0);
-   if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
-   (errno == ERANGE && v == ULONG_MAX))
-   errx(1, "invalid arg for fwddelay: %s", arg);
+   v = strtonum(arg, 0, UINT8_MAX, );
+   if (errstr != NULL)
+   errx(1, "%s arg for fwddelay: %s", errstr, arg);

strlcpy(bp.ifbrp_name, name, sizeof(bp.ifbrp_name));
   

Re: amd64: EFI boot over network try to load kernel from hd0

[Patrick Wildt]

On Wed, Jun 07, 2017 at 03:58:01PM +0200, Sven-Volker Nowarra wrote:
> Hi,
> 
> I setup a PXE boot server, to load my different clients. Now I have 2 single 
> EFI devices, which do not support PXE (both are Macminis). They prefer "EFI" 
> files. Both are capable running 64bit software, but require 32bit EFI 
> loaders. So I added a line in my dhcpd.conf to supply “BOOTIA32.EFI” file.  
> It get’s loaded via tftp, and displays on the clients console the well known 
> bsd prompt for booting: 
> 
>probing: pc0 mem 1576K 64K 2015M 11M 64K 52KI
>disk: hd0* hd1*
>>> OpenBSD/amd64 BOOTIA32 3.30
>open(hd0a:/etc/boot.conf): invalid argument
>boot> _
> 
> Here the client stops, for sure there is no harddrive. Somehow the efiboot 
> file does not provide the option to load the kernel via network, only hd0 or 
> hd1 is offered. I looked up the files in 
> /sys/arch/amd64/stand/efiboot/efiboot.c and the header files, but couldn't 
> find the references to IPv4 (or IPv6) booting, though the header files in 
> /usr/src/sys/stand/efi/include contain the structures for IPv4_DEVICE_PATH. 
> Can someone point me into the right direction?  
> 
> ---
> 
> On dhcp server: Macs boot differently over the net with dhcp and bootp. I use 
> a class definition, and inside s.th . like this:
> 
> class "AppleNBI-i386" {
> match if substring (option vendor-class-identifier, 0, 14) = "AAPLBSDPC/i386";
> option dhcp-parameter-request-list 1,3,17,43,60;
> if (option dhcp-message-type = 1)
>{
>option vendor-class-identifier "AAPLBSDPC/i386";
>}
> if (option dhcp-message-type = 1)
>{
>option vendor-encapsulated-options 08:04:81:00:00:67;
>}
> if (substring (option vendor-class-identifier, 15, 10) = "Macmini1,1")
>{
>log(info, concat("DHCPd: Macmini1,1: vendor-class-identifier = ", option 
> vendor-class-identifier));
>filename "amd64/BOOTIA32.EFI";
>}
> ...
> 
> So I didn't need to setup dhcp with "option arch code 93", as described here:
> http://marc.info/?l=openbsd-tech=148517258726719=2
> 

Do you want it to run diskless with root on NFS?  Well, I haven't
implemented that, but here's a diff that should allow you to load
the kernel from a TFTP server.

I have a similar diff on arm64 (which probably doesn't work with
u-boot's EFI API, but works on EDK2 machines) which I use for testing
kernels more easily.

I'm not sure how helpful this feature is for most people.  It's probably
nice for developers who want to boot different kernels over network, but
so far it does not support root on NFS, or only accidentally.

Patrick

diff --git a/sys/arch/amd64/stand/efiboot/Makefile.common 
b/sys/arch/amd64/stand/efiboot/Makefile.common
index d821e0bc39a..7742e201683 100644
--- a/sys/arch/amd64/stand/efiboot/Makefile.common
+++ b/sys/arch/amd64/stand/efiboot/Makefile.common
@@ -24,7 +24,7 @@ AFLAGS+=  -pipe -fPIC
 
 .PATH: ${.CURDIR}/..
 SRCS+= self_reloc.c
-SRCS+= efiboot.c efidev.c
+SRCS+= efiboot.c efidev.c efipxe.c
 SRCS+= conf.c
 
 .PATH: ${S}/stand/boot
diff --git a/sys/arch/amd64/stand/efiboot/conf.c 
b/sys/arch/amd64/stand/efiboot/conf.c
index 3b2059e414f..55ab425fab2 100644
--- a/sys/arch/amd64/stand/efiboot/conf.c
+++ b/sys/arch/amd64/stand/efiboot/conf.c
@@ -31,12 +31,14 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
 #include "disk.h"
 #include "efiboot.h"
 #include "efidev.h"
+#include "efipxe.h"
 
 const char version[] = "3.33";
 
@@ -51,7 +53,7 @@ void (*i386_probe1[])(void) = {
cninit, efi_memprobe
 };
 void (*i386_probe2[])(void) = {
-   efi_diskprobe, diskprobe
+   efi_pxeprobe, efi_diskprobe, diskprobe
 };
 
 struct i386_boot_probes probe_list[] = {
@@ -62,6 +64,8 @@ int nibprobes = nitems(probe_list);
 
 
 struct fs_ops file_system[] = {
+   { tftp_open,   tftp_close,   tftp_read,   tftp_write,   tftp_seek,
+ tftp_stat,   tftp_readdir   },
{ ufs_open,ufs_close,ufs_read,ufs_write,ufs_seek,
  ufs_stat,ufs_readdir},
{ cd9660_open, cd9660_close, cd9660_read, cd9660_write, cd9660_seek,
@@ -76,10 +80,8 @@ struct fs_ops file_system[] = {
 int nfsys = nitems(file_system);
 
 struct devsw   devsw[] = {
-   { "EFI", efistrategy, efiopen, eficlose, efiioctl },
-#if 0
{ "TFTP", tftpstrategy, tftpopen, tftpclose, tftpioctl },
-#endif
+   { "EFI", efistrategy, efiopen, eficlose, efiioctl },
 };
 int ndevs = nitems(devsw);
 
diff --git a/sys/arch/amd64/stand/efiboot/efiboot.c 
b/sys/arch/amd64/stand/efiboot/efiboot.c
index 9b6d5fc00fd..d753ffbf919 100644
--- a/sys/arch/amd64/stand/efiboot/efiboot.c
+++ b/sys/arch/amd64/stand/efiboot/efiboot.c
@@ -52,9 +52,8 @@ static EFI_GUIDblkio_guid = BLOCK_IO_PROTOCOL;
 static EFI_GUID devp_guid = DEVICE_PATH_PROTOCOL;
 u_long  efi_loadaddr;
 
-static int  efi_device_path_depth(EFI_DEVICE_PATH *dp, int);
-static int  efi_device_path_ncmp(EFI_DEVICE_PATH *, 

Re: amd64: EFI boot over network try to load kernel from hd0

[Ted Unangst]

Sven-Volker Nowarra wrote:
> Here the client stops, for sure there is no harddrive. Somehow the efiboot 
> file does not provide the option to load the kernel via network, only hd0 or 
> hd1 is offered. I looked up the files in 
> /sys/arch/amd64/stand/efiboot/efiboot.c and the header files, but couldn't 
> find the references to IPv4 (or IPv6) booting, though the header files in 
> /usr/src/sys/stand/efi/include contain the structures for IPv4_DEVICE_PATH. 

Indeed, as you noticed, the EFI bootloader doesn't have network support.

Re: mg backup directory

[Lucas Gabriel Vuotto]

bump because I forgot to CC florian@

On 26/05/17 18:11, Lucas Gabriel Vuotto wrote:

On 26/05/17 12:38, Florian Obser wrote:

On Fri, May 19, 2017 at 02:11:22PM -0300, Lucas Gabriel Vuotto wrote:

Previous patch shall be ignored, as it was an ugly hack. Below is a patch that 
is simpler and fixes expandtilde instead, so it fixes the problem in other 
situations (writing files to ~, for example). The only thing that I'm not sure 
is whether to use getuid or geteuid. Any suggestion / explanation is welcome.


what is emacs doing in this case?



It reads HOME from environment (from lisp--arghhh) if the path is ~/foo, or 
calls getpwnam if the path is ~user/foo.

amd64: EFI boot over network try to load kernel from hd0

[Sven-Volker Nowarra]

Hi,

I setup a PXE boot server, to load my different clients. Now I have 2 single 
EFI devices, which do not support PXE (both are Macminis). They prefer "EFI" 
files. Both are capable running 64bit software, but require 32bit EFI loaders. 
So I added a line in my dhcpd.conf to supply “BOOTIA32.EFI” file.  It get’s 
loaded via tftp, and displays on the clients console the well known bsd prompt 
for booting: 

   probing: pc0 mem 1576K 64K 2015M 11M 64K 52KI
   disk: hd0* hd1*
   >> OpenBSD/amd64 BOOTIA32 3.30
   open(hd0a:/etc/boot.conf): invalid argument
   boot> _

Here the client stops, for sure there is no harddrive. Somehow the efiboot file 
does not provide the option to load the kernel via network, only hd0 or hd1 is 
offered. I looked up the files in /sys/arch/amd64/stand/efiboot/efiboot.c and 
the header files, but couldn't find the references to IPv4 (or IPv6) booting, 
though the header files in /usr/src/sys/stand/efi/include contain the 
structures for IPv4_DEVICE_PATH. 
Can someone point me into the right direction?  

---

On dhcp server: Macs boot differently over the net with dhcp and bootp. I use a 
class definition, and inside s.th . like this:

class "AppleNBI-i386" {
match if substring (option vendor-class-identifier, 0, 14) = "AAPLBSDPC/i386";
option dhcp-parameter-request-list 1,3,17,43,60;
if (option dhcp-message-type = 1)
   {
   option vendor-class-identifier "AAPLBSDPC/i386";
   }
if (option dhcp-message-type = 1)
   {
   option vendor-encapsulated-options 08:04:81:00:00:67;
   }
if (substring (option vendor-class-identifier, 15, 10) = "Macmini1,1")
   {
   log(info, concat("DHCPd: Macmini1,1: vendor-class-identifier = ", option 
vendor-class-identifier));
   filename "amd64/BOOTIA32.EFI";
   }
...

So I didn't need to setup dhcp with "option arch code 93", as described here:
http://marc.info/?l=openbsd-tech=148517258726719=2

Better handling of short reads

[Mike Belopuhov]

Hi,

I've discovered that short reads (nonzero b_resid) aren't
handled very well in our kernel and I've proposed a diff
like this to handle short reads of buffercache read-ahead
buffers:

diff --git sys/kern/vfs_bio.c sys/kern/vfs_bio.c
index 95bc80bc0e6..1cc1943d752 100644
--- sys/kern/vfs_bio.c
+++ sys/kern/vfs_bio.c
@@ -534,11 +534,27 @@ bread_cluster_callback(struct buf *bp)
 */
buf_fix_mapping(bp, newsize);
bp->b_bcount = newsize;
}
 
-   for (i = 1; xbpp[i] != 0; i++) {
+   /* Invalidate read-ahead buffers if read short */
+   if (bp->b_resid > 0) {
+   for (i = 0; xbpp[i] != NULL; i++)
+   continue;
+   for (i = i - 1; i != 0; i--) {
+   if (xbpp[i]->b_bufsize <= bp->b_resid) {
+   bp->b_resid -= xbpp[i]->b_bufsize;
+   SET(xbpp[i]->b_flags, B_INVAL);
+   } else if (bp->b_resid > 0) {
+   bp->b_resid = 0;
+   SET(xbpp[i]->b_flags, B_INVAL);
+   } else
+   break;
+   }
+   }
+
+   for (i = 1; xbpp[i] != NULL; i++) {
if (ISSET(bp->b_flags, B_ERROR))
SET(xbpp[i]->b_flags, B_INVAL | B_ERROR);
biodone(xbpp[i]);
}
 

Now I said before that the only issue that this diff didn't
fix was with the xbpp[0] aka the buf we return to FFS: if we
have a 64k sized cluster on our filesystem then we've never
created read-ahead bufs and thus this code never runs and we
never account for the b_resid.  However, this is thankfully
not correct as FFS handles short reads itself (except one
small detail...). Here's a chunk from ffs_read:

if (lblktosize(fs, nextlbn) >= DIP(ip, size))
error = bread(vp, lbn, size, );
else
error = bread_cluster(vp, lbn, size, );

if (error)
break;

/*
 * We should only get non-zero b_resid when an I/O error
 * has occurred, which should cause us to break above.
 * However, if the short read did not cause an error,
 * then we want to ensure that we do not uiomove bad
 * or uninitialized data.
 */
size -= bp->b_resid;
if (size < xfersize) {
if (size == 0)
break;
xfersize = size;
}
error = uiomove(bp->b_data + blkoffset, xfersize, uio);

As you can see it copies (size - bp->b_resid) into the uio.
That would be OK if b_resid was as large as the 'size'. But
due to how bread_cluster extends the b_count to cover for
all additional read-ahead buffers, the transfer in the end
can have a b_resid anywhere in the interval of [0, MAXPHYS]
which can be larger than 'size' that FFS has asked for.

This leads to 'size' underflow because it's an integer and
then uiomove gets a negative value for xfersize which gets
converted to a very large unsigned long (size_t) parameter
for uiomove. And this is bad.  Therefore, additionally I'd
like to assert this in the FFS code itself.  If this is the
way to go, I'll look into other filesystems and propose a
similar check.

diff --git sys/ufs/ffs/ffs_vnops.c sys/ufs/ffs/ffs_vnops.c
index 160e187820f..56c222612a2 100644
--- sys/ufs/ffs/ffs_vnops.c
+++ sys/ufs/ffs/ffs_vnops.c
@@ -244,10 +244,11 @@ ffs_read(void *v)
 * has occurred, which should cause us to break above.
 * However, if the short read did not cause an error,
 * then we want to ensure that we do not uiomove bad
 * or uninitialized data.
 */
+   KASSERT(bp->b_resid <= size);
size -= bp->b_resid;
if (size < xfersize) {
if (size == 0)
break;
xfersize = size;


So to make it clear: I'd like to commit both changes and
if that's something we agree upon, I'll look into other
filesystems and make sure that they implement similar
assertions.

Opinions?

Re: brconfig: Unify/fix strtoul(3) handling

[Ted Unangst]

Klemens Nanni wrote:
> Besides fixing a tautological 'v < 0' and using more descriptive/less
> errorprone sizeof(target) this patch unifies strtoul(3) handling both
> logically and cosmetically.

A great deal of these look like exactly the situation strtonum was desinged to
cope with. this means dropping support for hex bases, but i'm happy telling
people they need to learn decimal. :)

> 
> Index: brconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 brconfig.c
> --- brconfig.c28 Nov 2016 10:12:50 -  1.14
> +++ brconfig.c2 Jun 2017 15:55:29 -
> @@ -220,7 +220,6 @@ bridge_ifsetflag(const char *ifsname, u_
>   err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
>  
>   req.ifbr_ifsflags |= flag & ~IFBIF_RO_MASK;
> -
>   if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
>   err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
>  }
> @@ -232,12 +231,10 @@ bridge_ifclrflag(const char *ifsname, u_
>  
>   strlcpy(req.ifbr_name, name, sizeof(req.ifbr_name));
>   strlcpy(req.ifbr_ifsname, ifsname, sizeof(req.ifbr_ifsname));
> -
>   if (ioctl(s, SIOCBRDGGIFFLGS, (caddr_t)) < 0)
>   err(1, "%s: ioctl SIOCBRDGGIFFLGS %s", name, ifsname);
>  
>   req.ifbr_ifsflags &= ~(flag | IFBIF_RO_MASK);
> -
>   if (ioctl(s, SIOCBRDGSIFFLGS, (caddr_t)) < 0)
>   err(1, "%s: ioctl SIOCBRDGSIFFLGS %s", name, ifsname);
>  }
> @@ -425,7 +422,8 @@ bridge_maxage(const char *arg, int d)
>  
>   errno = 0;
>   v = strtoul(arg, , 0);
> - if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
> + if (arg[0] == '\0' || endptr[0] != '\0' ||
> + v == 0 || v > sizeof(bp.ifbrp_maxage) ||
>   (errno == ERANGE && v == ULONG_MAX))
>   errx(1, "invalid arg for maxage: %s", arg);
>  
> @@ -444,7 +442,8 @@ bridge_priority(const char *arg, int d)
>  
>   errno = 0;
>   v = strtoul(arg, , 0);
> - if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xUL ||
> + if (arg[0] == '\0' || endptr[0] != '\0' ||
> + v == 0 || v > sizeof(bp.ifbrp_prio) ||
>   (errno == ERANGE && v == ULONG_MAX))
>   errx(1, "invalid arg for spanpriority: %s", arg);
>  
> @@ -483,7 +482,8 @@ bridge_fwddelay(const char *arg, int d)
>  
>   errno = 0;
>   v = strtoul(arg, , 0);
> - if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
> + if (arg[0] == '\0' || endptr[0] != '\0' ||
> + v == 0 || v > sizeof(bp.ifbrp_fwddelay) ||
>   (errno == ERANGE && v == ULONG_MAX))
>   errx(1, "invalid arg for fwddelay: %s", arg);
>  
> @@ -502,7 +502,8 @@ bridge_hellotime(const char *arg, int d)
>  
>   errno = 0;
>   v = strtoul(arg, , 0);
> - if (arg[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
> + if (arg[0] == '\0' || endptr[0] != '\0' ||
> + v == 0 || v > sizeof(bp.ifbrp_hellotime) ||
>   (errno == ERANGE && v == ULONG_MAX))
>   errx(1, "invalid arg for hellotime: %s", arg);
>  
> @@ -521,7 +522,8 @@ bridge_maxaddr(const char *arg, int d)
>  
>   errno = 0;
>   newsize = strtoul(arg, , 0);
> - if (arg[0] == '\0' || endptr[0] != '\0' || newsize > 0xUL ||
> + if (arg[0] == '\0' || endptr[0] != '\0' ||
> + newsize == 0 || newsize > sizeof(bp.ifbrp_csize) ||
>   (errno == ERANGE && newsize == ULONG_MAX))
>   errx(1, "invalid arg for maxaddr: %s", arg);
>  
> @@ -537,13 +539,12 @@ bridge_deladdr(const char *addr, int d)
>   struct ifbareq ifba;
>   struct ether_addr *ea;
>  
> - strlcpy(ifba.ifba_name, name, sizeof(ifba.ifba_name));
>   ea = ether_aton(addr);
>   if (ea == NULL)
>   err(1, "Invalid address: %s", addr);
>  
> + strlcpy(ifba.ifba_name, name, sizeof(ifba.ifba_name));
>   bcopy(ea, _dst, sizeof(struct ether_addr));
> -
>   if (ioctl(s, SIOCBRDGDADDR, ) < 0)
>   err(1, "%s: %s", name, addr);
>  }
> @@ -555,16 +556,16 @@ bridge_ifprio(const char *ifname, const 
>   unsigned long v;
>   char *endptr;
>  
> - strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name));
> - strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname));
> -
>   errno = 0;
>   v = strtoul(val, , 0);
> - if (val[0] == '\0' || endptr[0] != '\0' || v > 0xffUL ||
> + if (val[0] == '\0' || endptr[0] != '\0' ||
> + v == 0 || v > sizeof(breq.ifbr_priority) ||
>   (errno == ERANGE && v == ULONG_MAX))
> - err(1, "invalid arg for ifpriority: %s", val);
> - breq.ifbr_priority = v;
> + errx(1, "invalid arg for ifpriority: %s", val);
>  
> + strlcpy(breq.ifbr_name, name, sizeof(breq.ifbr_name));
> + strlcpy(breq.ifbr_ifsname, ifname, sizeof(breq.ifbr_ifsname));
> + breq.ifbr_priority = v;
>   if (ioctl(s, SIOCBRDGSIFPRIO, 

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Ted Unangst]

Michal Mazurek wrote:
> Yes, the function seems a bit inconsistent, in that "bcrypt" means "bcrypt,a"
> but NULL means "bcrypt,8". awolk@ points out that the function is used in
> just a few places - src and some ports patches, so we should be able to
> change it. Judging by the commit message the author was aware of this
> discrepancy though, and marked is as "for now", so let's see what others say.

this was complicated because it's trying to unify several different programs
and everybody has their own idea of what the default should be. so it tries to
squeeze in some useful behaviors without changing everything about how all the
calling programs behave. at least now we're in a situation where we should be
able to discuss changing one program at a time. 

Re: ifconfig: fix warning (tautological compare)

[Theo de Raadt]

Code like this should match the EXAMPLE in the strtoul manual page.

This shit happens everytime someone tries to take a shortcut,
sadly.

> Fix clang warning about tautological compare: an unsigned long can't
> be negative.
> 
> ok?  Or dlg@ can slip it in with his next ifconfig change.
> 
> Index: brconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 brconfig.c
> --- brconfig.c28 Nov 2016 10:12:50 -  1.14
> +++ brconfig.c7 Jun 2017 15:54:40 -
> @@ -581,8 +581,7 @@ bridge_ifcost(const char *ifname, const 
>  
>   errno = 0;
>   v = strtoul(val, , 0);
> - if (val[0] == '\0' || endptr[0] != '\0' ||
> - v < 0 || v > 0xUL ||
> + if (val[0] == '\0' || endptr[0] != '\0' || v > 0xUL ||
>   (errno == ERANGE && v == ULONG_MAX))
>   errx(1, "invalid arg for ifcost: %s", val);
>  
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 

ifconfig: fix warning (tautological compare)

[Christian Weisgerber]

Fix clang warning about tautological compare: an unsigned long can't
be negative.

ok?  Or dlg@ can slip it in with his next ifconfig change.

Index: brconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
retrieving revision 1.14
diff -u -p -r1.14 brconfig.c
--- brconfig.c  28 Nov 2016 10:12:50 -  1.14
+++ brconfig.c  7 Jun 2017 15:54:40 -
@@ -581,8 +581,7 @@ bridge_ifcost(const char *ifname, const 
 
errno = 0;
v = strtoul(val, , 0);
-   if (val[0] == '\0' || endptr[0] != '\0' ||
-   v < 0 || v > 0xUL ||
+   if (val[0] == '\0' || endptr[0] != '\0' || v > 0xUL ||
(errno == ERANGE && v == ULONG_MAX))
errx(1, "invalid arg for ifcost: %s", val);
 
-- 
Christian "naddy" Weisgerber  na...@mips.inka.de

Re: ifconfig.8 doco for vnetid and parent options

[Claudio Jeker]

On Wed, Jun 07, 2017 at 05:58:42PM +1000, David Gwynne wrote:
> On Wed, Jun 07, 2017 at 07:32:18AM +0100, Jason McIntyre wrote:
> > On Wed, Jun 07, 2017 at 02:51:42PM +1000, David Gwynne wrote:
> > > > 
> > > > you are breaking the format of this page if you start to move specific
> > > > subsytems into the main body.
> > > > 
> > > > that makes things less clear. for example, can i use the parent-device
> > > > stuff on any type of interface or just vlan? if it's vlan related, how
> > > > can i tell that?
> > > 
> > > the "parent" ioctls are generic, but it is only implemented in vlan 
> > > currently. i have a plan to replace "carpdev", "syncdev", and "pppoedev" 
> > > with it, but id like to finish the vlan stuff before going further.
> > > 
> > 
> > so it sounds like it should be in vlan for now.
> 
> cool.
> 
> > 
> > > vnetid is used by both vlan and vxlan at the moment. i have a diff to add 
> > > support for it to gre, while adding egre (ethernet over gre) and nvgre 
> > > interfaces that also support a network identifier.
> > > 
> > > > 
> > > > the text reads fine, but i'd rather you tried to integrate it into the
> > > > vlan section. if there are errors in there let's remove them.
> > > > 
> > > > but if it's the case that none of this stuff is specific to vlan, then i
> > > > guess it makes sense to do it your way (but then you'd have to take care
> > > > when documenting stuff like parent-device to say in what situations it
> > > > makes sense).
> > > 
> > > im happy to take direction here. i could also move things like the 
> > > ethernet options (arp, -arp, instance, wol, -wol) into their own section.
> > > 
> > 
> > well, that would be a separate diff.
> > 
> > > how would you say that vnetid is supported by both vlan and some tunnel 
> > > interfaces? would you put it in the tunnel section, the vlan section, or 
> > > both, or a new section?
> > > 
> > > dlg
> > > 
> > 
> > just like it is just now.
> > 
> > i know that the way that ifconfig(8) is split up is not perfect. that is
> > really just a reflection of how complex a tool ifconfig is. by complex i
> > mean it can do a lot of different stuff. the format of the page as-is
> > now just kind of happened. i'm not saying we can't (or shouldn;t) change
> > it, just that if we do, we'd want to try and keep it consistent.
> > 
> > why not just add your one option to the vlan section, which is what it's
> > relevant to, for now. if in the future you make it work in other areas,
> > we can see whether it warrants a shuffle.
> 
> well, vnetid and parent replace the vlan and vlandev parameters.
> 
> how about this as a start?
> 
> as an aside, should i fix vlan.4 so it uses capitals on the 802.1
> things consistently? capitals are preferred, right?
> 

Be careful, AFAIK the capitalisation of IEEE standards does matter.
You're right 802.1Q is the correct spelling but not for 802.1ad (where the
lowercase version is the offical standard). IIRC the status of the
standard is what causes the letters to become uppercase.

-- 
:wq Claudio

Re: g/c ASPICFLAG

[Martin Pieuchot]

On 06/06/17(Tue) 19:13, Miod Vallat wrote:
> This used to be necessary in the gcc 2.95 days. Nowadays nothing in base
> or X uses it.

ok mpi@

> Index: share/mk/bsd.own.mk
> ===
> RCS file: /OpenBSD/src/share/mk/bsd.own.mk,v
> retrieving revision 1.184
> diff -u -p -r1.184 bsd.own.mk
> --- share/mk/bsd.own.mk   18 Apr 2017 14:03:08 -  1.184
> +++ share/mk/bsd.own.mk   6 Jun 2017 19:12:04 -
> @@ -129,10 +129,6 @@ PICFLAG?=-fPIC
>  PICFLAG?=-fpic
>  .endif
>  
> -.if ${MACHINE_ARCH} == "sparc64"
> -ASPICFLAG=-KPIC
> -.endif
> -
>  .if ${MACHINE_ARCH} == "alpha" || ${MACHINE_ARCH} == "powerpc" || \
>  ${MACHINE_ARCH} == "sparc64"
>  # big PIE
> 

Fail adding the queue for an interface that doesn't exist

[Mike Belopuhov]

This might not be the fix we want in the long run, but it surely
prevents frustration when making a typo in the interface name.

As reported by Sebastien Marie and claudio@.

OK?

diff --git sys/net/pf_ioctl.c sys/net/pf_ioctl.c
index 43cccdb2efa..c563a439c45 100644
--- sys/net/pf_ioctl.c
+++ sys/net/pf_ioctl.c
@@ -582,11 +582,11 @@ pf_ifp2q(struct pf_queue_if *list, struct ifnet *ifp)
 int
 pf_create_queues(void)
 {
struct pf_queuespec *q;
struct ifnet*ifp;
-   struct pf_queue_if  *list = NULL, *qif;
+   struct pf_queue_if  *list = NULL, *qif;
int  error;
 
/*
 * Find root queues and allocate traffic conditioner
 * private data for these interfaces
@@ -1128,20 +1128,19 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, 
struct proc *p)
bcopy(>queue, qs, sizeof(*qs));
qs->qid = pf_qname2qid(qs->qname, 1);
if (qs->parent[0] && (qs->parent_qid =
pf_qname2qid(qs->parent, 0)) == 0) {
pool_put(_queue_pl, qs);
-   error = ESRCH;
+   error = EINVAL;
break;
}
qs->kif = pfi_kif_get(qs->ifname);
-   if (qs->kif == NULL) {
+   if (qs->kif == NULL || qs->kif->pfik_ifp == NULL) {
pool_put(_queue_pl, qs);
-   error = ESRCH;
+   error = EINVAL;
break;
}
-   /* XXX resolve bw percentage specs */
pfi_kif_ref(qs->kif, PFI_KIF_REF_RULE);
 
TAILQ_INSERT_TAIL(pf_queues_inactive, qs, entries);
 
break;

vic: stop using ifq_deq_rollback

[Mike Belopuhov]

Hi,

This is a straightforward diff moving invariant chunks before
dequeue operation.

OK?

diff --git sys/dev/pci/if_vic.c sys/dev/pci/if_vic.c
index e34a1aa4f27..bc1e600d8bc 100644
--- sys/dev/pci/if_vic.c
+++ sys/dev/pci/if_vic.c
@@ -1049,43 +1049,35 @@ vic_start(struct ifnet *ifp)
if (VIC_TXURN(sc)) {
ifq_set_oactive(>if_snd);
break;
}
 
-   m = ifq_deq_begin(>if_snd);
-   if (m == NULL)
-   break;
-
idx = sc->sc_data->vd_tx_nextidx;
if (idx >= sc->sc_data->vd_tx_length) {
-   ifq_deq_rollback(>if_snd, m);
printf("%s: tx idx is corrupt\n", DEVNAME(sc));
ifp->if_oerrors++;
break;
}
 
txd = >sc_txq[idx];
txb = >sc_txbuf[idx];
 
if (txb->txb_m != NULL) {
-   ifq_deq_rollback(>if_snd, m);
printf("%s: tx ring is corrupt\n", DEVNAME(sc));
sc->sc_data->vd_tx_stopped = 1;
ifp->if_oerrors++;
break;
}
 
-   /*
-* we're committed to sending it now. if we cant map it into
-* dma memory then we drop it.
-*/
-   ifq_deq_commit(>if_snd, m);
+   m = ifq_dequeue(>if_snd);
+   if (m == NULL)
+   break;
+
if (vic_load_txb(sc, txb, m) != 0) {
m_freem(m);
ifp->if_oerrors++;
-   /* continue? */
-   break;
+   continue;
}
 
 #if NBPFILTER > 0
if (ifp->if_bpf)
bpf_mtap(ifp->if_bpf, txb->txb_m, BPF_DIRECTION_OUT);

Re: chmod.1: Correct =X behaviour

[Jason McIntyre]

On Sat, Jun 03, 2017 at 03:15:42PM +0200, Klemens Nanni wrote:
> =X actually works quite fine despite the mentioned condition:
> 
>   $ touch f; mkdir d
>   $ chmod =X f d
>   $ ls -ld f d
>   d--x--x--x  2 kle  wheel  512 Jun  3 15:11 d/
>   --  1 kle  wheel0 Jun  3 15:11 f
> 
> One of the given examples has been updated to reflect this.
> 
> Whether to implement/fix the desired(?) limitation is another question.
> 

fixed, thanks.
jmc

> Index: chmod.1
> ===
> RCS file: /cvs/src/bin/chmod/chmod.1,v
> retrieving revision 1.41
> diff -u -p -r1.41 chmod.1
> --- chmod.1   31 Dec 2015 23:38:16 -  1.41
> +++ chmod.1   3 Jun 2017 13:10:02 -
> @@ -287,15 +287,6 @@ Execute/search bits.
> .It X
> The execute/search bits if the file is a directory or any of the
> execute/search bits are set in the original (unmodified) mode.
> -Operations with the
> -.Ar perm
> -symbol
> -.Sq X
> -are only meaningful in conjunction with the
> -.Ar op
> -symbol
> -.Sq + ,
> -and are ignored in all other cases.
> .It u
> User permission bits in the mode of the original file.
> .It g
> @@ -333,7 +324,7 @@ Deny write permission to group and other
> Set the read and write permissions to the usual defaults, but
> retain any execute permissions that are currently set:
> .Pp
> -.Dl $ chmod =rw,+X file
> +.Dl $ chmod =rwX file
> .Pp
> Make a directory or file searchable/executable by everyone if it is
> already searchable/executable by anyone:
> 

Re: ifconfig.8 doco for vnetid and parent options

[Jason McIntyre]

On Wed, Jun 07, 2017 at 07:17:16PM +1000, David Gwynne wrote:
> 
> tweaked diff
> 

yes, looks good.
jmc

> Index: ifconfig.8
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.282
> diff -u -p -r1.282 ifconfig.8
> --- ifconfig.812 May 2017 15:11:02 -  1.282
> +++ ifconfig.87 Jun 2017 09:16:11 -
> @@ -1666,46 +1666,48 @@ device will try to establish a data conn
>  .Bk -words
>  .Nm ifconfig
>  .Ar vlan-interface
> -.Op Cm vlan Ar vlan-tag
> -.Op Oo Fl Oc Ns Cm vlandev Ar parent-interface
> +.Oo Fl Oc Ns Cm parent Ar parent-interface
> +.Oo Fl Oc Ns Cm vnetid Ar vlan-tag
>  .Ek
>  .nr nS 0
>  .Pp
> -The following options are available for a
> +The following options are available for
>  .Xr vlan 4
> -interface:
> +and
> +.Xr svlan 4
> +VLAN interfaces:
>  .Bl -tag -width Ds
> -.It Cm vlan Ar vlan-tag
> -Set the vlan tag value
> -to
> -.Ar vlan-tag .
> -This value is a 12-bit number which is used to create an 802.1Q
> -vlan header for packets sent from the vlan interface.
> -This value cannot be changed once it is set for an interface.
> -.It Cm vlandev Ar parent-interface
> -Associate with interface
> +.It Cm parent Ar parent-interface
> +Associate the VLAN interface with the interface
>  .Ar parent-interface .
> -Packets transmitted through the vlan interface will be
> -diverted to the specified interface
> -.Ar parent-interface
> -with 802.1Q vlan tagging.
> -Packets with 802.1Q tagging received
> -by the parent interface with the correct vlan tag will be diverted to
> -the associated vlan pseudo-device.
> -The vlan interface is assigned a
> -copy of the parent interface's flags and the parent's Ethernet address.
> -If
> -.Cm vlandev
> -and
> -.Cm vlan
> -are not set at the same time, the vlan tag will be inferred from
> -the interface name, for instance
> -.Cm vlan5
> -will be assigned 802.1Q tag 5.
> -.It Cm -vlandev
> +Packets transmitted on
> +.Xr vlan 4
> +or
> +.Xr svlan 4
> +interfaces will be tagged with 802.1Q or 802.1AD headers respectively
> +and transmitted on the specified parent interface.
> +Packets with 802.1Q or 802.1AD tags received
> +by the parent interface with the specified VLAN tag will be diverted to
> +the associated VLAN interface.
> +Unless a custom Ethernet address is assigned to the VLAN interface,
> +it will inherit a copy of the parent interface's Ethernet address.
> +.It Cm -parent
>  Disassociate from the parent interface.
> -This breaks the link between the vlan interface and its parent,
> -clears its vlan tag, flags, and link address, and shuts the interface down.
> +This breaks the link between the VLAN interface and its parent.
> +.It Cm vnetid Ar vlan-tag
> +Set the VLAN tag value to
> +.Ar vlan-tag .
> +This value is a 12-bit number which is used in the 802.1Q or 802.1AD
> +headers in packets handled by
> +.Xr vlan 4
> +or
> +.Xr svlan 4
> +interfaces respectively.
> +Valid tag values are from 1 to 4095 inclusive.
> +.It Cm -vnetid
> +Clear the tag value.
> +Packets on a VLAN interface without a tag set will use a value of
> +0 in their headers.
>  .El
>  .Sh EXAMPLES
>  Assign the
> 

Re: ifconfig.8 doco for vnetid and parent options

[David Gwynne]

On Wed, Jun 07, 2017 at 10:01:41AM +0100, Jason McIntyre wrote:
> On Wed, Jun 07, 2017 at 05:58:42PM +1000, David Gwynne wrote:
> > > 
> > > why not just add your one option to the vlan section, which is what it's
> > > relevant to, for now. if in the future you make it work in other areas,
> > > we can see whether it warrants a shuffle.
> > 
> > well, vnetid and parent replace the vlan and vlandev parameters.
> > 
> > how about this as a start?
> > 
> 
> looks good. some comments inline.
> 
> > as an aside, should i fix vlan.4 so it uses capitals on the 802.1
> > things consistently? capitals are preferred, right?
> > 
> 
> i'm not sure what is considered correct, but i think we generally do
> uppercase these. it would be good if both pages were consistent, yes.
> adding to that, vlan(4) consistently uppercases VLAN, so maybe your diff
> should too?

tweaked diff

Index: ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.282
diff -u -p -r1.282 ifconfig.8
--- ifconfig.8  12 May 2017 15:11:02 -  1.282
+++ ifconfig.8  7 Jun 2017 09:16:11 -
@@ -1666,46 +1666,48 @@ device will try to establish a data conn
 .Bk -words
 .Nm ifconfig
 .Ar vlan-interface
-.Op Cm vlan Ar vlan-tag
-.Op Oo Fl Oc Ns Cm vlandev Ar parent-interface
+.Oo Fl Oc Ns Cm parent Ar parent-interface
+.Oo Fl Oc Ns Cm vnetid Ar vlan-tag
 .Ek
 .nr nS 0
 .Pp
-The following options are available for a
+The following options are available for
 .Xr vlan 4
-interface:
+and
+.Xr svlan 4
+VLAN interfaces:
 .Bl -tag -width Ds
-.It Cm vlan Ar vlan-tag
-Set the vlan tag value
-to
-.Ar vlan-tag .
-This value is a 12-bit number which is used to create an 802.1Q
-vlan header for packets sent from the vlan interface.
-This value cannot be changed once it is set for an interface.
-.It Cm vlandev Ar parent-interface
-Associate with interface
+.It Cm parent Ar parent-interface
+Associate the VLAN interface with the interface
 .Ar parent-interface .
-Packets transmitted through the vlan interface will be
-diverted to the specified interface
-.Ar parent-interface
-with 802.1Q vlan tagging.
-Packets with 802.1Q tagging received
-by the parent interface with the correct vlan tag will be diverted to
-the associated vlan pseudo-device.
-The vlan interface is assigned a
-copy of the parent interface's flags and the parent's Ethernet address.
-If
-.Cm vlandev
-and
-.Cm vlan
-are not set at the same time, the vlan tag will be inferred from
-the interface name, for instance
-.Cm vlan5
-will be assigned 802.1Q tag 5.
-.It Cm -vlandev
+Packets transmitted on
+.Xr vlan 4
+or
+.Xr svlan 4
+interfaces will be tagged with 802.1Q or 802.1AD headers respectively
+and transmitted on the specified parent interface.
+Packets with 802.1Q or 802.1AD tags received
+by the parent interface with the specified VLAN tag will be diverted to
+the associated VLAN interface.
+Unless a custom Ethernet address is assigned to the VLAN interface,
+it will inherit a copy of the parent interface's Ethernet address.
+.It Cm -parent
 Disassociate from the parent interface.
-This breaks the link between the vlan interface and its parent,
-clears its vlan tag, flags, and link address, and shuts the interface down.
+This breaks the link between the VLAN interface and its parent.
+.It Cm vnetid Ar vlan-tag
+Set the VLAN tag value to
+.Ar vlan-tag .
+This value is a 12-bit number which is used in the 802.1Q or 802.1AD
+headers in packets handled by
+.Xr vlan 4
+or
+.Xr svlan 4
+interfaces respectively.
+Valid tag values are from 1 to 4095 inclusive.
+.It Cm -vnetid
+Clear the tag value.
+Packets on a VLAN interface without a tag set will use a value of
+0 in their headers.
 .El
 .Sh EXAMPLES
 Assign the

Re: htpasswd: use crypt_newhash instead of bcrypt API

[Adam Wolk]

On Tue, Jun 06, 2017 at 08:29:23PM +, Florian Obser wrote:
> On Tue, Jun 06, 2017 at 08:49:32PM +0200, Adam Wolk wrote:
> > On Tue, Jun 06, 2017 at 12:28:59PM -0600, Theo de Raadt wrote:
> > > > The only thing against using automatic rounds would be having them 
> > > > guessed on a
> > > > weaker machine and used on a more powerful server - doubt though that 
> > > > would ever
> > > > pick something below 8 rounds.
> > > 
> > > I don't see the concern.  It has a lower bound.
> > 
> > Attaching the diff with rounds changed to auto, results with 9 rounds on my 
> > server.
> > 
> 
> OK florian@
> 

Committed, thanks!

Re: ifconfig.8 doco for vnetid and parent options

[Jason McIntyre]

On Wed, Jun 07, 2017 at 05:58:42PM +1000, David Gwynne wrote:
> > 
> > why not just add your one option to the vlan section, which is what it's
> > relevant to, for now. if in the future you make it work in other areas,
> > we can see whether it warrants a shuffle.
> 
> well, vnetid and parent replace the vlan and vlandev parameters.
> 
> how about this as a start?
> 

looks good. some comments inline.

> as an aside, should i fix vlan.4 so it uses capitals on the 802.1
> things consistently? capitals are preferred, right?
> 

i'm not sure what is considered correct, but i think we generally do
uppercase these. it would be good if both pages were consistent, yes.
adding to that, vlan(4) consistently uppercases VLAN, so maybe your diff
should too?

> Index: ifconfig.8
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.282
> diff -u -p -r1.282 ifconfig.8
> --- ifconfig.812 May 2017 15:11:02 -  1.282
> +++ ifconfig.87 Jun 2017 07:54:52 -
> @@ -1666,46 +1666,48 @@ device will try to establish a data conn
>  .Bk -words
>  .Nm ifconfig
>  .Ar vlan-interface
> -.Op Cm vlan Ar vlan-tag
> -.Op Oo Fl Oc Ns Cm vlandev Ar parent-interface
> +.Oo Fl Oc Ns Cm vnetid Ar vlan-tag
> +.Oo Fl Oc Ns Cm parent Ar parent-interface

normally parent would go before vnetid.

>  .Ek
>  .nr nS 0
>  .Pp
> -The following options are available for a
> +The following options are available for
>  .Xr vlan 4
> -interface:
> +and
> +.Xr svlan 4
> +interfaces:
>  .Bl -tag -width Ds
> -.It Cm vlan Ar vlan-tag
> -Set the vlan tag value
> -to
> +.It Cm vnetid Ar vlan-tag
> +Set the vlan tag value to

so that would be uppercase

>  .Ar vlan-tag .
> -This value is a 12-bit number which is used to create an 802.1Q
> -vlan header for packets sent from the vlan interface.
> -This value cannot be changed once it is set for an interface.
> -.It Cm vlandev Ar parent-interface
> -Associate with interface
> +This value is a 12-bit number which is used in the 802.1Q or 802.1AD
> +headers in packets handled by
> +.Xr vlan 4
> +or
> +.Xr svlan 4
> +interfaces respectively.
> +Valid tag values are from 1 to 4095 inclusive.
> +.It Cm Fl vnetid

there's different ways to do it, but this page uses

.It Cm -vnetid

the way you've done it will cause a mandoc warning (that it's skipping Cm)

> +Clear the tag value.
> +Packets on a vlan interface without a tag set will use a value of
> +0 in their headers.
> +.It Cm parent Ar parent-interface

again, parent before vnetid

> +Associate the vlan interface with the interface

again uppercase

>  .Ar parent-interface .
> -Packets transmitted through the vlan interface will be
> -diverted to the specified interface
> -.Ar parent-interface
> -with 802.1Q vlan tagging.
> -Packets with 802.1Q tagging received
> +Packets transmitted on
> +.Xr vlan 4
> +or
> +.Xr svlan 4
> +interfaces will be tagged with 802.1Q or 802.1AD headers respectively
> +and transmitted on the specified parent interface.
> +Packets with 802.1Q or 802.1AD tags received
>  by the parent interface with the correct vlan tag will be diverted to

uppercase

> -the associated vlan pseudo-device.
> -The vlan interface is assigned a
> -copy of the parent interface's flags and the parent's Ethernet address.
> -If
> -.Cm vlandev
> -and
> -.Cm vlan
> -are not set at the same time, the vlan tag will be inferred from
> -the interface name, for instance
> -.Cm vlan5
> -will be assigned 802.1Q tag 5.
> -.It Cm -vlandev
> +the associated vlan interface.

uppercase

> +Unless a custom Ethernet address is assigned to the vlan interface,

again

> +it will inherit a copy of the parent interface's Ethernet address.
> +.It Cm Fl parent

.It Cm -parent

>  Disassociate from the parent interface.
> -This breaks the link between the vlan interface and its parent,
> -clears its vlan tag, flags, and link address, and shuts the interface down.
> +This breaks the link between the vlan interface and its parent.

uppercase

>  .El
>  .Sh EXAMPLES
>  Assign the
> 

jmc

Re: ifconfig.8 doco for vnetid and parent options

[David Gwynne]

On Wed, Jun 07, 2017 at 07:32:18AM +0100, Jason McIntyre wrote:
> On Wed, Jun 07, 2017 at 02:51:42PM +1000, David Gwynne wrote:
> > > 
> > > you are breaking the format of this page if you start to move specific
> > > subsytems into the main body.
> > > 
> > > that makes things less clear. for example, can i use the parent-device
> > > stuff on any type of interface or just vlan? if it's vlan related, how
> > > can i tell that?
> > 
> > the "parent" ioctls are generic, but it is only implemented in vlan 
> > currently. i have a plan to replace "carpdev", "syncdev", and "pppoedev" 
> > with it, but id like to finish the vlan stuff before going further.
> > 
> 
> so it sounds like it should be in vlan for now.

cool.

> 
> > vnetid is used by both vlan and vxlan at the moment. i have a diff to add 
> > support for it to gre, while adding egre (ethernet over gre) and nvgre 
> > interfaces that also support a network identifier.
> > 
> > > 
> > > the text reads fine, but i'd rather you tried to integrate it into the
> > > vlan section. if there are errors in there let's remove them.
> > > 
> > > but if it's the case that none of this stuff is specific to vlan, then i
> > > guess it makes sense to do it your way (but then you'd have to take care
> > > when documenting stuff like parent-device to say in what situations it
> > > makes sense).
> > 
> > im happy to take direction here. i could also move things like the ethernet 
> > options (arp, -arp, instance, wol, -wol) into their own section.
> > 
> 
> well, that would be a separate diff.
> 
> > how would you say that vnetid is supported by both vlan and some tunnel 
> > interfaces? would you put it in the tunnel section, the vlan section, or 
> > both, or a new section?
> > 
> > dlg
> > 
> 
> just like it is just now.
> 
> i know that the way that ifconfig(8) is split up is not perfect. that is
> really just a reflection of how complex a tool ifconfig is. by complex i
> mean it can do a lot of different stuff. the format of the page as-is
> now just kind of happened. i'm not saying we can't (or shouldn;t) change
> it, just that if we do, we'd want to try and keep it consistent.
> 
> why not just add your one option to the vlan section, which is what it's
> relevant to, for now. if in the future you make it work in other areas,
> we can see whether it warrants a shuffle.

well, vnetid and parent replace the vlan and vlandev parameters.

how about this as a start?

as an aside, should i fix vlan.4 so it uses capitals on the 802.1
things consistently? capitals are preferred, right?

Index: ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.282
diff -u -p -r1.282 ifconfig.8
--- ifconfig.8  12 May 2017 15:11:02 -  1.282
+++ ifconfig.8  7 Jun 2017 07:54:52 -
@@ -1666,46 +1666,48 @@ device will try to establish a data conn
 .Bk -words
 .Nm ifconfig
 .Ar vlan-interface
-.Op Cm vlan Ar vlan-tag
-.Op Oo Fl Oc Ns Cm vlandev Ar parent-interface
+.Oo Fl Oc Ns Cm vnetid Ar vlan-tag
+.Oo Fl Oc Ns Cm parent Ar parent-interface
 .Ek
 .nr nS 0
 .Pp
-The following options are available for a
+The following options are available for
 .Xr vlan 4
-interface:
+and
+.Xr svlan 4
+interfaces:
 .Bl -tag -width Ds
-.It Cm vlan Ar vlan-tag
-Set the vlan tag value
-to
+.It Cm vnetid Ar vlan-tag
+Set the vlan tag value to
 .Ar vlan-tag .
-This value is a 12-bit number which is used to create an 802.1Q
-vlan header for packets sent from the vlan interface.
-This value cannot be changed once it is set for an interface.
-.It Cm vlandev Ar parent-interface
-Associate with interface
+This value is a 12-bit number which is used in the 802.1Q or 802.1AD
+headers in packets handled by
+.Xr vlan 4
+or
+.Xr svlan 4
+interfaces respectively.
+Valid tag values are from 1 to 4095 inclusive.
+.It Cm Fl vnetid
+Clear the tag value.
+Packets on a vlan interface without a tag set will use a value of
+0 in their headers.
+.It Cm parent Ar parent-interface
+Associate the vlan interface with the interface
 .Ar parent-interface .
-Packets transmitted through the vlan interface will be
-diverted to the specified interface
-.Ar parent-interface
-with 802.1Q vlan tagging.
-Packets with 802.1Q tagging received
+Packets transmitted on
+.Xr vlan 4
+or
+.Xr svlan 4
+interfaces will be tagged with 802.1Q or 802.1AD headers respectively
+and transmitted on the specified parent interface.
+Packets with 802.1Q or 802.1AD tags received
 by the parent interface with the correct vlan tag will be diverted to
-the associated vlan pseudo-device.
-The vlan interface is assigned a
-copy of the parent interface's flags and the parent's Ethernet address.
-If
-.Cm vlandev
-and
-.Cm vlan
-are not set at the same time, the vlan tag will be inferred from
-the interface name, for instance
-.Cm vlan5
-will be assigned 802.1Q tag 5.
-.It Cm -vlandev
+the associated vlan interface.
+Unless a custom Ethernet address is assigned to 

Re: ifconfig.8 doco for vnetid and parent options

[Jason McIntyre]

On Wed, Jun 07, 2017 at 02:51:42PM +1000, David Gwynne wrote:
> > 
> > you are breaking the format of this page if you start to move specific
> > subsytems into the main body.
> > 
> > that makes things less clear. for example, can i use the parent-device
> > stuff on any type of interface or just vlan? if it's vlan related, how
> > can i tell that?
> 
> the "parent" ioctls are generic, but it is only implemented in vlan 
> currently. i have a plan to replace "carpdev", "syncdev", and "pppoedev" with 
> it, but id like to finish the vlan stuff before going further.
> 

so it sounds like it should be in vlan for now.

> vnetid is used by both vlan and vxlan at the moment. i have a diff to add 
> support for it to gre, while adding egre (ethernet over gre) and nvgre 
> interfaces that also support a network identifier.
> 
> > 
> > the text reads fine, but i'd rather you tried to integrate it into the
> > vlan section. if there are errors in there let's remove them.
> > 
> > but if it's the case that none of this stuff is specific to vlan, then i
> > guess it makes sense to do it your way (but then you'd have to take care
> > when documenting stuff like parent-device to say in what situations it
> > makes sense).
> 
> im happy to take direction here. i could also move things like the ethernet 
> options (arp, -arp, instance, wol, -wol) into their own section.
> 

well, that would be a separate diff.

> how would you say that vnetid is supported by both vlan and some tunnel 
> interfaces? would you put it in the tunnel section, the vlan section, or 
> both, or a new section?
> 
> dlg
> 

just like it is just now.

i know that the way that ifconfig(8) is split up is not perfect. that is
really just a reflection of how complex a tool ifconfig is. by complex i
mean it can do a lot of different stuff. the format of the page as-is
now just kind of happened. i'm not saying we can't (or shouldn;t) change
it, just that if we do, we'd want to try and keep it consistent.

why not just add your one option to the vlan section, which is what it's
relevant to, for now. if in the future you make it work in other areas,
we can see whether it warrants a shuffle.

jmc