Re: [PATCH] ifconfig: keep track of allowed ips pointer correctly
Hi!
On Tue, Oct 27, 2020 at 06:16:08PM +0100, Jason A. Donenfeld wrote:
> Somebody on IRC mentioned that using ifconfig to set wgallowedips wasn't
> working on macppc.
It isn't working on armv7 either. I posted on bugs@ earlier, but it seems
the problem I found might be the same as is being discussed here.
So, on "vanilla" 6.8-RELEASE I get the following execution:
+ ifconfig wg0 create
+ ifconfig wg0 wgkey eEcguPq8JcnbS5CwTP1WsGcKL8/aSD3eD+CSDwy3sLA=
+ ifconfig wg0 wgpeer s+zREKf1nsr9TV8Fbogv4c9Xkn0c305KF+6hOLx81Ho= wgaip
192.168.123.1/24
ifconfig: SIOCSWG: Address family not supported by protocol family
+ ifconfig wg0
wg0: flags=8082 mtu 1420
index 46 priority 0 llprio 3
wgpubkey g/V4mqfXffmIEm3ME7RKKvYE3As53mIfsRbYCNY9yTM=
wgpeer s+zREKf1nsr9TV8Fbogv4c9Xkn0c305KF+6hOLx81Ho=
tx: 0, rx: 0
groups: wg
+ ifconfig wg0 wgpeer s+zREKf1nsr9TV8Fbogv4c9Xkn0c305KF+6hOLx81Ho= wgaip
fd2b:a33a:77b0:450d::/64
ifconfig: SIOCSWG: Address family not supported by protocol family
+ ifconfig wg0
wg0: flags=8082 mtu 1420
index 46 priority 0 llprio 3
wgpubkey g/V4mqfXffmIEm3ME7RKKvYE3As53mIfsRbYCNY9yTM=
wgpeer s+zREKf1nsr9TV8Fbogv4c9Xkn0c305KF+6hOLx81Ho=
tx: 0, rx: 0
groups: wg
When I apply the patch you posted here, I get the following execution:
+ /usr/src/sbin/ifconfig/ifconfig wg0 create
+ /usr/src/sbin/ifconfig/ifconfig wg0 wgkey
wIcMGMrjswYhhVi4SQfLzMuhmPArJuY0avwKyEr1QF8=
+ /usr/src/sbin/ifconfig/ifconfig wg0 wgpeer
WDck6FQW6XBnSbJoYe6P6aRj3W2w37x2NQrFa+1zz5Y= wgaip 192.168.123.1/24
+ /usr/src/sbin/ifconfig/ifconfig wg0
wg0: flags=8082 mtu 1420
index 50 priority 0 llprio 3
wgpubkey aYVRr9XrQ09yWzKEHEShPq1472QL+A9pckPJv2HgOEA=
wgpeer WDck6FQW6XBnSbJoYe6P6aRj3W2w37x2NQrFa+1zz5Y=
tx: 0, rx: 0
wgaip /24
groups: wg
+ /usr/src/sbin/ifconfig/ifconfig wg0 wgpeer
WDck6FQW6XBnSbJoYe6P6aRj3W2w37x2NQrFa+1zz5Y= wgaip fd2b:a33a:77b0:450d::/64
+ /usr/src/sbin/ifconfig/ifconfig wg0
wg0: flags=8082 mtu 1420
index 50 priority 0 llprio 3
wgpubkey aYVRr9XrQ09yWzKEHEShPq1472QL+A9pckPJv2HgOEA=
wgpeer WDck6FQW6XBnSbJoYe6P6aRj3W2w37x2NQrFa+1zz5Y=
tx: 0, rx: 0
wgaip /64
groups: wg
so, the patch "improves" the situation in the sense that I don't get the
error message, but I still have the problem that wgallowedips are not set,
at least properly. The output fails like that since inet_ntop(3) fails with
EAFNOSUPPORT.
> I don't have a macppc to test this on, but it seems like the code is
> assuming that the two values printed out by this test program must always
> be the same:
>
> struct s {
> int i;
> };
>
> struct p {
> long l;
> char c;
> struct s a[];
> };
>
> int main(int argc, char *argv[])
> {
> printf("%zu %zu\n", sizeof(struct p), (size_t)&((struct p *)0)->a[0]);
> return 0;
> }
>
> But actually, on my amd64 system, that little test prints out "16 12".
> This patch fixes up ifconfig.c to do the right thing, so that it
> corresponds with how the kernel handles iteration.
>
> I don't have a macppc in order to test this, but it works on amd64.
The good news is that it also doesn't work in armv7 :P but I hope my report
helps somehow.
> ---
> sbin/ifconfig/ifconfig.c | 19 +++
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git sbin/ifconfig/ifconfig.c sbin/ifconfig/ifconfig.c
> index 2ccbac6f4ce..ade6e4943b7 100644
> --- sbin/ifconfig/ifconfig.c
> +++ sbin/ifconfig/ifconfig.c
> @@ -5696,11 +5696,10 @@ ensurewginterface(void)
> err(1, "calloc");
> }
>
> -void *
> +void
> growwgdata(size_t by)
> {
> ptrdiff_t peer_offset, aip_offset;
> - void *ret;
>
> if (wg_interface == NULL)
> wgdata.wgd_size = sizeof(*wg_interface);
> @@ -5721,16 +5720,18 @@ growwgdata(size_t by)
> if (wg_aip != NULL)
> wg_aip = (void *)wg_interface + aip_offset;
>
> -
Re: accton(8) requires a reboot after being enabled
Following diff changes accton(8) behavior.
If the file given as parameter doesn't exists, accton will create it.
Then it will check the ownership and will make it root:wheel if
it's different.
I added a check to be sure it's run as root because it's has no use if
not run as root.
I don't often write C, if the logic is good but the C implementation
wrong I'm open to critic.
The -f test and touch in /etc/rc won't be needed anymore with this
change.
Index: accton.8
===
RCS file: /home/reposync/src/usr.sbin/accton/accton.8,v
retrieving revision 1.11
diff -u -p -r1.11 accton.8
--- accton.810 Nov 2019 20:51:53 - 1.11
+++ accton.830 Oct 2020 17:27:36 -
@@ -36,7 +36,7 @@
.Nm accton
.Op Ar file
.Sh DESCRIPTION
-With an argument naming an existing
+With an argument naming a
.Ar file ,
.Nm
causes system accounting information for every process executed
Index: accton.c
===
RCS file: /home/reposync/src/usr.sbin/accton/accton.c,v
retrieving revision 1.8
diff -u -p -r1.8 accton.c
--- accton.c27 Oct 2009 23:59:50 - 1.8
+++ accton.c30 Oct 2020 17:26:31 -
@@ -27,6 +27,7 @@
* SUCH DAMAGE.
*/
+#include
#include
#include
#include
@@ -47,6 +48,10 @@ int
main(int argc, char *argv[])
{
int ch;
+ struct stat info_file;
+
+ if(getuid() != 0)
+ err(1, "need root privileges");
while ((ch = getopt(argc, argv, "")) != -1)
switch(ch) {
@@ -63,6 +68,15 @@ main(int argc, char *argv[])
err(1, NULL);
break;
case 1:
+ if(stat(*argv,_file) != 0)
+ if(fopen(*argv,"w"))
+ if(stat(*argv,_file))
+ err(1, "file %s couldn't be created",
*argv);
+
+ if (info_file.st_uid != 0 || info_file.st_gid != 0)
+ if(chown(*argv, 0, 0) != 0)
+ err(1, "Impossible to fix %s ownership", *argv);
+
if (acct(*argv))
err(1, "%s", *argv);
break;
Re: amap: introduce amap_adjref_anons()
On 23/10/20(Fri) 10:31, Martin Pieuchot wrote:
> More refactoring. This time let's introduce a helper to manipulate
> references. The goal is to reduce the upcoming diff adding locking.
>
> This is extracted from a bigger diff from guenther@ as well as some
> bits from NetBSD.
Now with the correct diff, ok?
Index: uvm/uvm_amap.c
===
RCS file: /cvs/src/sys/uvm/uvm_amap.c,v
retrieving revision 1.85
diff -u -p -r1.85 uvm_amap.c
--- uvm/uvm_amap.c 12 Oct 2020 08:44:45 - 1.85
+++ uvm/uvm_amap.c 23 Oct 2020 08:23:59 -
@@ -68,7 +68,23 @@ static inline void amap_list_remove(stru
struct vm_amap_chunk *amap_chunk_get(struct vm_amap *, int, int, int);
void amap_chunk_free(struct vm_amap *, struct vm_amap_chunk *);
-void amap_wiperange_chunk(struct vm_amap *, struct vm_amap_chunk *, int, int);
+
+/*
+ * if we enable PPREF, then we have a couple of extra functions that
+ * we need to prototype here...
+ */
+
+#ifdef UVM_AMAP_PPREF
+
+#define PPREF_NONE ((int *) -1)/* not using ppref */
+
+void amap_pp_adjref(struct vm_amap *, int, vsize_t, int);
+void amap_pp_establish(struct vm_amap *);
+void amap_wiperange_chunk(struct vm_amap *, struct vm_amap_chunk *, int,
+ int);
+void amap_wiperange(struct vm_amap *, int, int);
+
+#endif /* UVM_AMAP_PPREF */
static inline void
amap_list_insert(struct vm_amap *amap)
@@ -1153,6 +1169,32 @@ amap_unadd(struct vm_aref *aref, vaddr_t
}
/*
+ * amap_adjref_anons: adjust the reference count(s) on amap and its anons.
+ */
+static void
+amap_adjref_anons(struct vm_amap *amap, vaddr_t offset, vsize_t len,
+int refv, boolean_t all)
+{
+#ifdef UVM_AMAP_PPREF
+ if (amap->am_ppref == NULL && !all && len != amap->am_nslot) {
+ amap_pp_establish(amap);
+ }
+#endif
+
+ amap->am_ref += refv;
+
+#ifdef UVM_AMAP_PPREF
+ if (amap->am_ppref && amap->am_ppref != PPREF_NONE) {
+ if (all) {
+ amap_pp_adjref(amap, 0, amap->am_nslot, refv);
+ } else {
+ amap_pp_adjref(amap, offset, len, refv);
+ }
+ }
+#endif
+}
+
+/*
* amap_ref: gain a reference to an amap
*
* => "offset" and "len" are in units of pages
@@ -1162,51 +1204,36 @@ void
amap_ref(struct vm_amap *amap, vaddr_t offset, vsize_t len, int flags)
{
- amap->am_ref++;
if (flags & AMAP_SHARED)
amap->am_flags |= AMAP_SHARED;
-#ifdef UVM_AMAP_PPREF
- if (amap->am_ppref == NULL && (flags & AMAP_REFALL) == 0 &&
- len != amap->am_nslot)
- amap_pp_establish(amap);
- if (amap->am_ppref && amap->am_ppref != PPREF_NONE) {
- if (flags & AMAP_REFALL)
- amap_pp_adjref(amap, 0, amap->am_nslot, 1);
- else
- amap_pp_adjref(amap, offset, len, 1);
- }
-#endif
+ amap_adjref_anons(amap, offset, len, 1, (flags & AMAP_REFALL) != 0);
}
/*
* amap_unref: remove a reference to an amap
*
- * => caller must remove all pmap-level references to this amap before
- * dropping the reference
- * => called from uvm_unmap_detach [only] ... note that entry is no
- * longer part of a map
+ * => All pmap-level references to this amap must be already removed.
+ * => Called from uvm_unmap_detach(); entry is already removed from the map.
*/
void
amap_unref(struct vm_amap *amap, vaddr_t offset, vsize_t len, boolean_t all)
{
+ KASSERT(amap->am_ref > 0);
- /* if we are the last reference, free the amap and return. */
- if (amap->am_ref-- == 1) {
- amap_wipeout(amap); /* drops final ref and frees */
+ if (amap->am_ref == 1) {
+ /*
+* If the last reference - wipeout and destroy the amap.
+*/
+ amap->am_ref--;
+ amap_wipeout(amap);
return;
}
- /* otherwise just drop the reference count(s) */
- if (amap->am_ref == 1 && (amap->am_flags & AMAP_SHARED) != 0)
- amap->am_flags &= ~AMAP_SHARED; /* clear shared flag */
-#ifdef UVM_AMAP_PPREF
- if (amap->am_ppref == NULL && all == 0 && len != amap->am_nslot)
- amap_pp_establish(amap);
- if (amap->am_ppref && amap->am_ppref != PPREF_NONE) {
- if (all)
- amap_pp_adjref(amap, 0, amap->am_nslot, -1);
- else
- amap_pp_adjref(amap, offset, len, -1);
+ /*
+* Otherwise, drop the reference count(s) on anons.
+*/
+ if (amap->am_ref == 2 && (amap->am_flags & AMAP_SHARED) != 0) {
+ amap->am_flags &= ~AMAP_SHARED;
}
-#endif
+ amap_adjref_anons(amap, offset, len, -1, all);
}
Index: uvm/uvm_amap.h
===
RCS file: /cvs/src/sys/uvm/uvm_amap.h,v
retrieving
accton(8) requires a reboot after being enabled
reading accton(8) it's not clear that if you enable it you need to restart the system for accounting to be effective. Here is a change to add the explanation, but I'm not sure if the wording is correct. Index: accton.8 === RCS file: /home/reposync/src/usr.sbin/accton/accton.8,v retrieving revision 1.11 diff -u -p -r1.11 accton.8 --- accton.810 Nov 2019 20:51:53 - 1.11 +++ accton.830 Oct 2020 15:22:14 - @@ -43,6 +43,10 @@ causes system accounting information for to be placed at the end of the file. If no argument is given, accounting is turned off. .Pp +Accounting is done if it was enabled when system booted. +If you activate accounting, a reboot is required for it to become +effective. +.Pp To have .Nm enabled at boot time, use
Re: Please test: switch select(2) to kqfilters
On 26/10/20(Mon) 11:57, Scott Cheloha wrote:
> On Mon, Oct 12, 2020 at 11:11:36AM +0200, Martin Pieuchot wrote:
> [...]
> > +/*
> > + * Scan the kqueue, blocking if necessary until the target time is reached.
> > + * If tsp is NULL we block indefinitely. If tsp->ts_secs/nsecs are both
> > + * 0 we do not block at all.
> > + */
> > int
> > kqueue_scan(struct kqueue_scan_state *scan, int maxevents,
> > -struct kevent *ulistp, struct timespec *tsp, struct kevent *kev,
> > -struct proc *p, int *retval)
> > +struct kevent *kevp, struct timespec *tsp, struct proc *p, int *errorp)
>
> Is there any reason to change these argument names?
The array is no longer a user specified list because the interface is
now used by other kernel consumers.
> > @@ -618,6 +631,8 @@ dopselect(struct proc *p, int nd, fd_set
> > pobits[2] = (fd_set *)[5];
> > }
> >
> > + kqpoll_init(p);
> > +
> > #definegetbits(name, x) \
> > if (name && (error = copyin(name, pibits[x], ni))) \
> > goto done;
> > @@ -636,43 +651,63 @@ dopselect(struct proc *p, int nd, fd_set
> > if (sigmask)
> > dosigsuspend(p, *sigmask &~ sigcantmask);
> >
> > -retry:
> > - ncoll = nselcoll;
> > - atomic_setbits_int(>p_flag, P_SELECT);
> > - error = selscan(p, pibits[0], pobits[0], nd, ni, retval);
> > - if (error || *retval)
> > + /* Register kqueue events */
> > + if ((error = pselregister(p, pibits[0], nd, ni, ) != 0))
> > goto done;
> > - if (timeout == NULL || timespecisset(timeout)) {
> > - if (timeout != NULL) {
> > - getnanouptime();
> > - nsecs = MIN(TIMESPEC_TO_NSEC(timeout), MAXTSLP);
> > - } else
> > - nsecs = INFSLP;
> > - s = splhigh();
> > - if ((p->p_flag & P_SELECT) == 0 || nselcoll != ncoll) {
> > - splx(s);
> > - goto retry;
> > - }
> > - atomic_clearbits_int(>p_flag, P_SELECT);
> > - error = tsleep_nsec(, PSOCK | PCATCH, "select", nsecs);
>
> I need to clarify something.
>
> My understanding of the current state of poll/select is that all
> threads wait on the same channel, selwait. Sometimes, a thread will
> wakeup(9) *all* threads waiting on that channel. When this happens,
> most of the sleeping threads will recheck their conditions, see that
> nothing has changed, and go back to sleep.
>
> Right?
>
> Is that spurious wakeup case going away with this diff? That is, when
> a thread is sleeping in poll/select it will only be woken up by
> another thread if the condition for one of the descriptors of interest
> changes.
Your understanding matches mine, the removal of spurious wakeup might
be beneficial to some programs. I see this as a pro of this
implementation.
>
> > - splx(s);
> > - if (timeout != NULL) {
> > - getnanouptime();
> > - timespecsub(, , );
> > - timespecsub(timeout, , timeout);
> > - if (timeout->tv_sec < 0)
> > - timespecclear(timeout);
> > - }
> > - if (error == 0 || error == EWOULDBLOCK)
> > - goto retry;
> > +
> > + /*
> > +* The poll/select family of syscalls has been designed to
> > +* block when file descriptors are not available, even if
> > +* there's nothing to wait for.
> > +*/
> > + if (nevents == 0) {
> > + uint64_t nsecs = INFSLP;
> > +
> > + if (timeout != NULL)
> > + nsecs = MAX(1, MIN(TIMESPEC_TO_NSEC(timeout), MAXTSLP));
> > +
> > + error = tsleep_nsec(>p_kq, PSOCK | PCATCH, "kqsel", nsecs);
> > + /* select is not restarted after signals... */
> > + if (error == ERESTART)
> > + error = EINTR;
> > + if (error == EWOULDBLOCK)
> > + error = 0;
> > + goto done;
> > + }
>
> This is still wrong. If you want to isolate this case you can't block
> when the timeout is empty:
Thanks, fixed.
> > Index: sys/proc.h
> > ===
> > RCS file: /cvs/src/sys/sys/proc.h,v
> > retrieving revision 1.300
> > diff -u -p -r1.300 proc.h
> > --- sys/proc.h 16 Sep 2020 08:01:15 - 1.300
> > +++ sys/proc.h 12 Oct 2020 08:56:21 -
> > @@ -320,6 +320,7 @@ struct process {
> >
> > struct kcov_dev;
> > struct lock_list_entry;
> > +struct kqueue;
> >
> > struct p_inentry {
> > u_long ie_serial;
> > @@ -382,6 +383,8 @@ struct proc {
> > struct plimit *p_limit; /* [l] read ref. of p_p->ps_limit */
> > struct kcov_dev *p_kd; /* kcov device handle */
> > struct lock_list_entry *p_sleeplocks; /* WITNESS lock tracking */
> > + struct kqueue *p_kq; /* for select/poll */
> > + unsigned long p_kq_serial; /* for select/poll */
>
>
Re: Kernel hang under concurrent vfs ops on the same hierarchy.
I'm moving this to tech as I think the issue is best discussed there but I might be wrong. So, I did some slow progress on that hang. A recent snapshots now panics in ufs code with "ufs_rename: lost dir entry". That's the second one at ufs_vnops.c:1084. This means that the source of the rename was a directory and got either renamed or rm'ed concurrently. As a side note I didn't fully bisect it but I think it's rev 1.108 of uvm_vnode.c by anton@ that fixed the deadlock leaving us with the panic. Looking around I found that in 2001 dillon@fbsd removed the panic in [1] noting that those races did actually exist and were easy enough to trigger that it was a security problem (not sure about that solution to be honest as it hides the problem but w/e). Nowadays several os (linux and netbsd to name a few) choose the "easy" way and just use a mount-wide lock at a higher level (in sys_rename) in order to catch all those races in the locking mess that rename is. It was brought up recently in #dragonflybsd and it lead to the same fix in [2]. I've been meaning to try that approach on obsd but I lack the time so I'm dumping it all here in the hope that it will help someone! As a side note, the same test on a tmpfs mount point survives a full run. Here is the panic for reference: login: panic: ufs_rename: lost dir entry Stopped at db_enter+0x10: popq%rbp TIDPIDUID PRFLAGS PFLAGS CPU COMMAND 307330 34101 1000 0x3 0x4000 a.out 302047 34101 1000 0x3 0x4001 a.out *319683 34101 1000 0x3 0x4002K a.out 104491 34684 740x100012 0x4803 pflogd db_enter() at db_enter+0x10 panic(81decc5f) at panic+0x12a ufs_rename(800022b0afb8) at ufs_rename+0xcb6 VOP_RENAME(fd8118858d00,fd8118855a98,800022b0b198,fd8118858410, fd811885e010,800022b0b0c8) at VOP_RENAME+0x69 dorenameat(800022aec5c0,ff9c,58cd3c8dd30,ff9c,58cd3c8dd10) at doren ameat+0x1f6 syscall(800022b0b320) at syscall+0x389 Xsyscall() at Xsyscall+0x128 end of kernel end trace frame: 0x58cd3c8dcf0, count: 8 [1]: https://github.com/freebsd/freebsd/commit/dbebfe18a15ceac757fc126dd8caa59045ec9e47 [2]: https://gitweb.dragonflybsd.org/dragonfly.git/commit/ad1212685b9caac64c086a2363d15842dff21fd8 On 06 Oct 10:16, Mathieu - wrote: > >Synopsis:Concurrent rename/mkdir/rmdir on a hierarchy hangs the system > >Category:Kernel bug > >Environment: > System : OpenBSD 6.8 > Details : OpenBSD 6.8-current (GENERIC.MP) #7: Mon Oct 5 22:45:13 > CEST 2020 > > ptr@spear.local:/usr/src/sys/arch/amd64/compile/GENERIC.MP > > Architecture: OpenBSD.amd64 > Machine : amd64 > >Description: > Running dirconc.c [1] leads the system to hang under a minute, no ssh, no > serial it's a full freeze. Upon reboot the fs is obviously corrupted and needs > to go to single user mode to fix up. > > This is on a recent snapshot (dmesg indicates a compiled kernel cause I added > some debug code to no help). > > Tweaking a bit dirconc to lower the number of concurrent ops doesn't help. > Using > procs instead of threads also leads to the same hang. > > I intended to pinpoint it a bit better (that's why it's in a VM), but the hard > hang makes it really difficult and I lack the time and knowledge in that area > so I'm reporting it (I'll keep trying to debug it though and keep you > updated). > >How-To-Repeat: > Get dirconc.c [1], build it (cc dirconc.c -lpthread for the threaded > version), and > run dirconc against an empty directory. After a while the system will hang. > >Fix: > No known fix. > > [1]: https://www.netbsd.org/~riastradh/tmp/dirconc.c > > dmesg: > OpenBSD 6.8-current (GENERIC.MP) #7: Mon Oct 5 22:45:13 CEST 2020 > ptr@spear.local:/usr/src/sys/arch/amd64/compile/GENERIC.MP > real mem = 4278124544 (4079MB) > avail mem = 4133400576 (3941MB) > random: good seed from bootblocks > mpath0 at root > scsibus0 at mpath0: 256 targets > mainbus0 at root > bios0 at mainbus0: SMBIOS rev. 2.5 @ 0xe1000 (10 entries) > bios0: vendor innotek GmbH version "VirtualBox" date 12/01/2006 > bios0: innotek GmbH VirtualBox > acpi0 at bios0: ACPI 4.0 > acpi0: sleep states S0 S5 > acpi0: tables DSDT FACP APIC SSDT > acpi0: wakeup devices > acpitimer0 at acpi0: 3579545 Hz, 32 bits > acpimadt0 at acpi0 addr 0xfee0: PC-AT compat > cpu0 at mainbus0: apid 0 (boot processor) > cpu0: Intel(R) Core(TM) i5-6200U CPU @ 2.30GHz, 2403.41 MHz, 06-4e-03 > cpu0: > FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,PCLMUL,SSSE3,CX16,PCID,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,RDRAND,NXE,RDTSCP,LONG,LAHF,ABM,3DNOWP,ITSC,FSGSBASE,AVX2,INVPCID,RDSEED,CLFLUSHOPT,MELTDOWN > cpu0: 256KB 64b/line 8-way L2 cache > cpu0: smt 0, core 0, package 0 > mtrr: CPU supports MTRRs but not enabled by BIOS > cpu0: apic clock running at 1000MHz > cpu1 at
Re: accton(8) requires a reboot after being enabled
Yes, that diff is a whole bunch of TOCTUO. If this was going to be changed, it should be in the kernel. But I don't know if it should be changed yet, which is why I asked a bunch of questions. Stepping back to the man page change, we could decide that accton should continue to behave how it does, and this conversation started because the man page fell into the trap of documenting the rc scripts RATHER than documenting accton.
Re: accton(8) requires a reboot after being enabled
Hi Solene,
Solene Rapenne wrote on Fri, Oct 30, 2020 at 06:34:09PM +0100:
> Following diff changes accton(8) behavior.
>
> If the file given as parameter doesn't exists, accton will create it.
> Then it will check the ownership and will make it root:wheel if
> it's different.
>
> I added a check to be sure it's run as root because it's has no use if
> not run as root.
If it naturally runs into privileged system calls anyway and the
error message is comprehensible, that is not necessarily needed.
Currently, the error message seems OK to me:
$ accton
accton: Operation not permitted
> I don't often write C, if the logic is good but the C implementation
> wrong I'm open to critic.
>
> The -f test and touch in /etc/rc won't be needed anymore with this
> change.
>
>
> Index: accton.8
> ===
> RCS file: /home/reposync/src/usr.sbin/accton/accton.8,v
> retrieving revision 1.11
> diff -u -p -r1.11 accton.8
> --- accton.8 10 Nov 2019 20:51:53 - 1.11
> +++ accton.8 30 Oct 2020 17:27:36 -
> @@ -36,7 +36,7 @@
> .Nm accton
> .Op Ar file
> .Sh DESCRIPTION
> -With an argument naming an existing
> +With an argument naming a
> .Ar file ,
> .Nm
> causes system accounting information for every process executed
*If* we change accton(8) to create the file if it does not exist,
the manual should probably be more explicit and say that an
existing file is appended to, but that it is created if it does
not exist.
Maybe it should even say something like
Unlike other implementations, this version of
.Nm
creates the
.Ar file
if it does not exist.
in order to not set a trap for experienced users.
> Index: accton.c
> ===
> RCS file: /home/reposync/src/usr.sbin/accton/accton.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 accton.c
> --- accton.c 27 Oct 2009 23:59:50 - 1.8
> +++ accton.c 30 Oct 2020 17:26:31 -
> @@ -27,6 +27,7 @@
> * SUCH DAMAGE.
> */
>
> +#include
> #include
> #include
> #include
> @@ -47,6 +48,10 @@ int
> main(int argc, char *argv[])
> {
> int ch;
> + struct stat info_file;
> +
> + if(getuid() != 0)
> + err(1, "need root privileges");
>
> while ((ch = getopt(argc, argv, "")) != -1)
> switch(ch) {
> @@ -63,6 +68,15 @@ main(int argc, char *argv[])
> err(1, NULL);
> break;
> case 1:
> + if(stat(*argv,_file) != 0)
> + if(fopen(*argv,"w"))
Uh oh, that looks like a TOCTOU race condition to me.
> + if(stat(*argv,_file))
And that looks like another TOCTOU race condition.
Checking the return value of fopen(3) might be better.
Leaking a file descriptor from open(2) is also unusual.
Arguably, it may not be a problem here because the program
promptly exits anyway, but is is not nice for auditors.
Finally, what do you need fopen(3) for?
Wouldn't open(2) be sufficient?
> + err(1, "file %s couldn't be created",
> *argv);
> +
> + if (info_file.st_uid != 0 || info_file.st_gid != 0)
> + if(chown(*argv, 0, 0) != 0)
> + err(1, "Impossible to fix %s ownership", *argv);
> +
Isn't that yet another TOCTOU race condition?
I guess using fchown(2) might be more appropriate?
Also, fchmod(2) seems to be lacking, or even better, couldn't that
be covered by the third argument to open(2)?
> if (acct(*argv))
> err(1, "%s", *argv);
Arguably, there might also be a race condition between userland and
the kernel, but i'm not so sure about kernel land. Given how system
calls like open(2) work - taking a path, returning an int to avoid
race conditions - wouldn't it be more natural to have the acct(2)
system call create the file on the kernel side if necessary?
Yours,
Ingo
Re: accton(8) requires a reboot after being enabled
Hi Theo, Theo de Raadt wrote on Fri, Oct 30, 2020 at 09:59:09AM -0600: > With a careful reading of the current manual page, everything is there > and it is accurate. > > With an argument naming an existing file > > > Ok so let's create it with touch. Probably has wrong permissions. > But now accton to that file works. Or enable it and reboot, and now > disable it and reboot, and the file still exists, so now accton works > because it is an existing file (with the right permissions I guess). > > So it *IS* working as documented. It is just a bit weird, because the > accton command (and system call) do not create the file. > > > My problem with these changes is this is the man page for the accton(8) > command, it documents *the binary*. The manpage has already been subverted > talk about rcctl, and about how /etc/rc runs the command. But the man > page should first and foremost be about the command, not about /etc/rc > and rcctl, am i not right? You are right. The paragraph about rc.conf.local(8) is short (one sentence, one line and three half lines) and near the end of the DESCRIPTION, which is appropriate IMHO. It feels more prominent than it should only because the accton(8) command itself is so simple and straightforward that it can be described exhaustively in three lines of text. > For instance, the ntpd manual page has a tiny section about rc.conf. That paragraph has more or less the same length as the one in accton(8). > So in conclusion, I think both of you are writing text about the startup > subsystem, into the wrong manual page. I think both proposals are skewed. > > So questions. > > 1 - historically it requires a file to be pre-created. In the rc scripts, > this is a touch. That grabs the umask and ownership of root's run of > /etc/rc. > 2 - could we do better, in some way? > 3 - accton system call does not have a umask, is that where this design came > from? > 4 - could we improve upon this? > 5 - could accton always (attempt to) create the file, without harming > existing use cases, with proper owner and chmod flags? Maybe. It would change behaviour, though. Somebody might rely on the fact that accton(8) is a NOOP when the file given as an argument does not exist. I'm not saying that is necessarily a good idea, but someone might have done it, for example because they have to maintain a herd of similar machines, want accounting on some but not on others, deploy the same /etc/rc.conf.local to all and create or delete the file as desired. Having the kernel or accton(8) create the file if it does not exist would make the tool slightly easier to use for most users. Is that worth asking users doing soemthing like the above to change their deployment? Maybe, but i'm not sure. The advantages feel small either way. > 6 - or should that be tied to a flag, making it easier to document? I somewhat dislike that. Either we regard creating the file as part of the job. Then accton(8) ought to do it without a flag, and people who don't want accounting should not call it with an argument in the first place. Or we regard creating and deleting the file as a separate decision, like we do it now. Then accton(8) should not meddle with it, not even as a matter of a flag. Besides, creating a root:wheel 0644 file is not rocket science requiring an additional option in some specialized program to be accomplished. For that purpose, several adequate and flexible tools already exist, including install(1). Triggered by looking at this manual page: Let's trim the ridiculous HISTORY section. No, it didn't exist forever. No, the manual page isn't new. A predecessor "gacct(8)" may have existed briefly in v4, but i'm not wholly convinced. There is only a skeleton manual page https://minnie.tuhs.org/cgi-bin/utree.pl?file=V4/man/manx/gacct.8 not even containing a DESCRIPTION. I found no code, and it seems it probably wasn't contained in v5 nor in v6. So it seems best to not mention it. OK? Ingo Index: accton.8 === RCS file: /cvs/src/usr.sbin/accton/accton.8,v retrieving revision 1.11 diff -u -p -r1.11 accton.8 --- accton.810 Nov 2019 20:51:53 - 1.11 +++ accton.830 Oct 2020 16:43:55 - @@ -73,4 +73,5 @@ default accounting file .Sh HISTORY The .Nm -command has existed nearly forever, but this man page is new. +command first appeared in +.At v7 .
Re: dig(1): Extended DNS Error (RFC 8914)
On Fri, Oct 30, 2020 at 03:04:03PM +0100, Florian Obser wrote:
Love it,
-Otto
> $ obj/dig @1.1.1.1 dnssec-failed.org
>
> ; <<>> dig 9.10.8-P1 <<>> @1.1.1.1 dnssec-failed.org
> ; (1 server found)
> ;; global options: +cmd
> ;; Got answer:
> ;; ->>HEADER<<- opcode: QUERY, status: SERVFAIL, id: 26772
> ;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1
>
> ;; OPT PSEUDOSECTION:
> ; EDNS: version: 0, flags:; udp: 1232
> ; EDE: 6 (DNSSEC Bogus)
> ;; QUESTION SECTION:
> ;dnssec-failed.org. IN A
>
> ;; Query time: 244 msec
> ;; SERVER: 1.1.1.1#53(1.1.1.1)
> ;; WHEN: Fri Oct 30 14:59:09 CET 2020
> ;; MSG SIZE rcvd: 52
>
> Since I'm not aware of a server/query combination that responds with
> UTF-8 encoded EXTENDED-TEXT I didn't implement anything special for
> this so it will use the default renderer that's also used for NSIDs,
> printing a hexdump + printable ascii, e.g.:
>
> $ dig @k.root-servers.net +nsid . soa
> [...]
> ;; OPT PSEUDOSECTION:
> ; EDNS: version: 0, flags:; udp: 1232
> ; NSID: 6e 73 33 2e 6e 6c 2d 61 6d 73 2e 6b 2e 72 69 70 65 2e 6e 65 74
> ("ns3.nl-ams.k.ripe.net")
>
> OK?
>
> diff --git lib/dns/include/dns/message.h lib/dns/include/dns/message.h
> index 65ffcfd4c3f..a70720eee39 100644
> --- lib/dns/include/dns/message.h
> +++ lib/dns/include/dns/message.h
> @@ -104,6 +104,7 @@
> #define DNS_OPT_COOKIE 10 /*%< COOKIE opt code */
> #define DNS_OPT_PAD 12 /*%< PAD opt code */
> #define DNS_OPT_KEY_TAG 14 /*%< Key tag opt code */
> +#define DNS_OPT_EDE 15 /* RFC 8914 */
>
> /*%< The number of EDNS options we know about. */
> #define DNS_EDNSOPTIONS 4
> diff --git lib/dns/message.c lib/dns/message.c
> index 5e0fb167382..9721f9c0ef4 100644
> --- lib/dns/message.c
> +++ lib/dns/message.c
> @@ -2434,6 +2434,68 @@ render_ecs(isc_buffer_t *ecsbuf, isc_buffer_t *target)
> {
> return (ISC_R_SUCCESS);
> }
>
> +static const char *
> +ede_info_code2str(uint16_t info_code)
> +{
> + if (info_code > 49151)
> + return "Private Use";
> +
> + switch (info_code) {
> + case 0:
> + return "Other Error";
> + case 1:
> + return "Unsupported DNSKEY Algorithm";
> + case 2:
> + return "Unsupported DS Digest Type";
> + case 3:
> + return "Stale Answer";
> + case 4:
> + return "Forged Answer";
> + case 5:
> + return "DNSSEC Indeterminate";
> + case 6:
> + return "DNSSEC Bogus";
> + case 7:
> + return "Signature Expired";
> + case 8:
> + return "Signature Not Yet Valid";
> + case 9:
> + return "DNSKEY Missing";
> + case 10:
> + return "RRSIGs Missing";
> + case 11:
> + return "No Zone Key Bit Set";
> + case 12:
> + return "NSEC Missing";
> + case 13:
> + return "Cached Error";
> + case 14:
> + return "Not Ready";
> + case 15:
> + return "Blocked";
> + case 16:
> + return "Censored";
> + case 17:
> + return "Filtered";
> + case 18:
> + return "Prohibited";
> + case 19:
> + return "Stale NXDomain Answer";
> + case 20:
> + return "Not Authoritative";
> + case 21:
> + return "Not Supported";
> + case 22:
> + return "No Reachable Authority";
> + case 23:
> + return "Network Error";
> + case 24:
> + return "Invalid Data";
> + default:
> + return "Unassigned";
> + }
> +}
> +
> isc_result_t
> dns_message_pseudosectiontotext(dns_message_t *msg,
> dns_pseudosection_t section,
> @@ -2557,6 +2619,20 @@ dns_message_pseudosectiontotext(dns_message_t *msg,
> ADD_STRING(target, "\n");
> continue;
> }
> + } else if (optcode == DNS_OPT_EDE) {
> + uint16_t info_code;
> + ADD_STRING(target, "; EDE");
> + if (optlen >= 2) {
> + info_code =
> + isc_buffer_getuint16();
> + optlen -= 2;
> + snprintf(buf, sizeof(buf), ": %u (",
> + info_code);
> + ADD_STRING(target, buf);
> + ADD_STRING(target,
> + ede_info_code2str(info_code));
> + ADD_STRING(target, ")");
> + }
> } else {
> ADD_STRING(target, ";
Re: accton(8) requires a reboot after being enabled
On Fri, Oct 30, 2020 at 09:59:09AM -0600, Theo de Raadt wrote: > Jason McIntyre wrote: > > > On Fri, Oct 30, 2020 at 04:24:43PM +0100, Solene Rapenne wrote: > > > reading accton(8) it's not clear that if you > > > enable it you need to restart the system for > > > accounting to be effective. > > > > > > Here is a change to add the explanation, but > > > I'm not sure if the wording is correct. > > > > > > > hi. i think the text that follows is really trying to say the same thing > > (you enable it at boot, so until you boot it isn;t enabled). we could > > just try to make that one bit of text a bit clearer: > > > > .Nm > > should be enabled at boot time, either by running > > .Dq rcctl enable accounting > > or by setting > > .Dq accounting=YES > > in > > .Xr rc.conf.local 8 . > > With a careful reading of the current manual page, everything is there > and it is accurate. > > With an argument naming an existing file > > correct. i missed that accton could be started this way, and concentrated on how to start it at boot (which i guess is nromally the way it'll be run). > Ok so let's create it with touch. Probably has wrong permissions. > But now accton to that file works. Or enable it and reboot, and now > disable it and reboot, and the file still exists, so now accton works > because it is an existing file (with the right permissions I guess). > > So it *IS* working as documented. It is just a bit weird, because the > accton command (and system call) do not create the file. > yes, it works as advertised. and all the bits are there - i just thought solene's notes were a repetition on the current boot notes. > > My problem with these changes is this is the man page for the accton(8) > command, it documents *the binary*. The manpage has already been subverted > talk about rcctl, and about how /etc/rc runs the command. But the man > page should first and foremost be about the command, not about /etc/rc > and rcctl, am i not right? For instance, the ntpd manual page has a tiny > section about rc.conf. > the main text body is so slim that the boot notes look bigger in relation. ntpd has more to say. but rerading ntpd(8), i think it discusses the startup process in a better way. maybe we should move to a text more similar to that one/ i think it's still fair game to say how to start a process in its man page. if we don;t want such information, we have a lot of undoing to er, do. > So in conclusion, I think both of you are writing text about the startup > subsystem, into the wrong manual page. I think both proposals are skewed. > > So questions. > > 1 - historically it requires a file to be pre-created. In the rc scripts, > this is a touch. That grabs the umask and ownership of root's run of > /etc/rc. > 2 - could we do better, in some way? > 3 - accton system call does not have a umask, is that where this design came > from? > 4 - could we improve upon this? > 5 - could accton always (attempt to) create the file, without harming existing > use cases, with proper owner and chmod flags? > 6 - or should that be tied to a flag, making it easier to document? > i can;t answer these points. except with more questions: - is it fair game for a process to document how it fits into the startup process? right now, we broadly do that. i think it's fine. - somewhere we switched to telling people how to use rcctl. i never like this system, but it was broadly agreed. so now there are two ways to do things. should the pages describe both? it's hard to see we'd do one without the other. i note that ntpd(8) does not discuss rcctl(8)... jmc > > sth like that? > > jmc > > > > > Index: accton.8 > > > === > > > RCS file: /home/reposync/src/usr.sbin/accton/accton.8,v > > > retrieving revision 1.11 > > > diff -u -p -r1.11 accton.8 > > > --- accton.8 10 Nov 2019 20:51:53 - 1.11 > > > +++ accton.8 30 Oct 2020 15:22:14 - > > > @@ -43,6 +43,10 @@ causes system accounting information for > > > to be placed at the end of the file. > > > If no argument is given, accounting is turned off. > > > .Pp > > > +Accounting is done if it was enabled when system booted. > > > +If you activate accounting, a reboot is required for it to become > > > +effective. > > > +.Pp > > > To have > > > .Nm > > > enabled at boot time, use > > > > > >
Re: accton(8) requires a reboot after being enabled
Jason McIntyre wrote: > On Fri, Oct 30, 2020 at 04:24:43PM +0100, Solene Rapenne wrote: > > reading accton(8) it's not clear that if you > > enable it you need to restart the system for > > accounting to be effective. > > > > Here is a change to add the explanation, but > > I'm not sure if the wording is correct. > > > > hi. i think the text that follows is really trying to say the same thing > (you enable it at boot, so until you boot it isn;t enabled). we could > just try to make that one bit of text a bit clearer: > > .Nm > should be enabled at boot time, either by running > .Dq rcctl enable accounting > or by setting > .Dq accounting=YES > in > .Xr rc.conf.local 8 . With a careful reading of the current manual page, everything is there and it is accurate. With an argument naming an existing file Ok so let's create it with touch. Probably has wrong permissions. But now accton to that file works. Or enable it and reboot, and now disable it and reboot, and the file still exists, so now accton works because it is an existing file (with the right permissions I guess). So it *IS* working as documented. It is just a bit weird, because the accton command (and system call) do not create the file. My problem with these changes is this is the man page for the accton(8) command, it documents *the binary*. The manpage has already been subverted talk about rcctl, and about how /etc/rc runs the command. But the man page should first and foremost be about the command, not about /etc/rc and rcctl, am i not right? For instance, the ntpd manual page has a tiny section about rc.conf. So in conclusion, I think both of you are writing text about the startup subsystem, into the wrong manual page. I think both proposals are skewed. So questions. 1 - historically it requires a file to be pre-created. In the rc scripts, this is a touch. That grabs the umask and ownership of root's run of /etc/rc. 2 - could we do better, in some way? 3 - accton system call does not have a umask, is that where this design came from? 4 - could we improve upon this? 5 - could accton always (attempt to) create the file, without harming existing use cases, with proper owner and chmod flags? 6 - or should that be tied to a flag, making it easier to document? > sth like that? > jmc > > > Index: accton.8 > > === > > RCS file: /home/reposync/src/usr.sbin/accton/accton.8,v > > retrieving revision 1.11 > > diff -u -p -r1.11 accton.8 > > --- accton.810 Nov 2019 20:51:53 - 1.11 > > +++ accton.830 Oct 2020 15:22:14 - > > @@ -43,6 +43,10 @@ causes system accounting information for > > to be placed at the end of the file. > > If no argument is given, accounting is turned off. > > .Pp > > +Accounting is done if it was enabled when system booted. > > +If you activate accounting, a reboot is required for it to become > > +effective. > > +.Pp > > To have > > .Nm > > enabled at boot time, use > > >
Re: accton(8) requires a reboot after being enabled
On Fri, Oct 30, 2020 at 04:24:43PM +0100, Solene Rapenne wrote: > reading accton(8) it's not clear that if you > enable it you need to restart the system for > accounting to be effective. > > Here is a change to add the explanation, but > I'm not sure if the wording is correct. > hi. i think the text that follows is really trying to say the same thing (you enable it at boot, so until you boot it isn;t enabled). we could just try to make that one bit of text a bit clearer: .Nm should be enabled at boot time, either by running .Dq rcctl enable accounting or by setting .Dq accounting=YES in .Xr rc.conf.local 8 . sth like that? jmc > Index: accton.8 > === > RCS file: /home/reposync/src/usr.sbin/accton/accton.8,v > retrieving revision 1.11 > diff -u -p -r1.11 accton.8 > --- accton.8 10 Nov 2019 20:51:53 - 1.11 > +++ accton.8 30 Oct 2020 15:22:14 - > @@ -43,6 +43,10 @@ causes system accounting information for > to be placed at the end of the file. > If no argument is given, accounting is turned off. > .Pp > +Accounting is done if it was enabled when system booted. > +If you activate accounting, a reboot is required for it to become > +effective. > +.Pp > To have > .Nm > enabled at boot time, use >
dig(1): Extended DNS Error (RFC 8914)
$ obj/dig @1.1.1.1 dnssec-failed.org
; <<>> dig 9.10.8-P1 <<>> @1.1.1.1 dnssec-failed.org
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: SERVFAIL, id: 26772
;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1
;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; EDE: 6 (DNSSEC Bogus)
;; QUESTION SECTION:
;dnssec-failed.org. IN A
;; Query time: 244 msec
;; SERVER: 1.1.1.1#53(1.1.1.1)
;; WHEN: Fri Oct 30 14:59:09 CET 2020
;; MSG SIZE rcvd: 52
Since I'm not aware of a server/query combination that responds with
UTF-8 encoded EXTENDED-TEXT I didn't implement anything special for
this so it will use the default renderer that's also used for NSIDs,
printing a hexdump + printable ascii, e.g.:
$ dig @k.root-servers.net +nsid . soa
[...]
;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; NSID: 6e 73 33 2e 6e 6c 2d 61 6d 73 2e 6b 2e 72 69 70 65 2e 6e 65 74
("ns3.nl-ams.k.ripe.net")
OK?
diff --git lib/dns/include/dns/message.h lib/dns/include/dns/message.h
index 65ffcfd4c3f..a70720eee39 100644
--- lib/dns/include/dns/message.h
+++ lib/dns/include/dns/message.h
@@ -104,6 +104,7 @@
#define DNS_OPT_COOKIE 10 /*%< COOKIE opt code */
#define DNS_OPT_PAD12 /*%< PAD opt code */
#define DNS_OPT_KEY_TAG14 /*%< Key tag opt code */
+#define DNS_OPT_EDE15 /* RFC 8914 */
/*%< The number of EDNS options we know about. */
#define DNS_EDNSOPTIONS4
diff --git lib/dns/message.c lib/dns/message.c
index 5e0fb167382..9721f9c0ef4 100644
--- lib/dns/message.c
+++ lib/dns/message.c
@@ -2434,6 +2434,68 @@ render_ecs(isc_buffer_t *ecsbuf, isc_buffer_t *target) {
return (ISC_R_SUCCESS);
}
+static const char *
+ede_info_code2str(uint16_t info_code)
+{
+ if (info_code > 49151)
+ return "Private Use";
+
+ switch (info_code) {
+ case 0:
+ return "Other Error";
+ case 1:
+ return "Unsupported DNSKEY Algorithm";
+ case 2:
+ return "Unsupported DS Digest Type";
+ case 3:
+ return "Stale Answer";
+ case 4:
+ return "Forged Answer";
+ case 5:
+ return "DNSSEC Indeterminate";
+ case 6:
+ return "DNSSEC Bogus";
+ case 7:
+ return "Signature Expired";
+ case 8:
+ return "Signature Not Yet Valid";
+ case 9:
+ return "DNSKEY Missing";
+ case 10:
+ return "RRSIGs Missing";
+ case 11:
+ return "No Zone Key Bit Set";
+ case 12:
+ return "NSEC Missing";
+ case 13:
+ return "Cached Error";
+ case 14:
+ return "Not Ready";
+ case 15:
+ return "Blocked";
+ case 16:
+ return "Censored";
+ case 17:
+ return "Filtered";
+ case 18:
+ return "Prohibited";
+ case 19:
+ return "Stale NXDomain Answer";
+ case 20:
+ return "Not Authoritative";
+ case 21:
+ return "Not Supported";
+ case 22:
+ return "No Reachable Authority";
+ case 23:
+ return "Network Error";
+ case 24:
+ return "Invalid Data";
+ default:
+ return "Unassigned";
+ }
+}
+
isc_result_t
dns_message_pseudosectiontotext(dns_message_t *msg,
dns_pseudosection_t section,
@@ -2557,6 +2619,20 @@ dns_message_pseudosectiontotext(dns_message_t *msg,
ADD_STRING(target, "\n");
continue;
}
+ } else if (optcode == DNS_OPT_EDE) {
+ uint16_t info_code;
+ ADD_STRING(target, "; EDE");
+ if (optlen >= 2) {
+ info_code =
+ isc_buffer_getuint16();
+ optlen -= 2;
+ snprintf(buf, sizeof(buf), ": %u (",
+ info_code);
+ ADD_STRING(target, buf);
+ ADD_STRING(target,
+ ede_info_code2str(info_code));
+ ADD_STRING(target, ")");
+ }
} else {
ADD_STRING(target, "; OPT=");
snprintf(buf, sizeof(buf), "%u", optcode);
--
I'm not entirely sure you are real.
Re: Minor tweak relayd agentx manpage
sure Martijn van Duren(openbsd+t...@list.imperialat.at) on 2020.10.30 09:53:08 +0100: > I think metrics is a better word than statistics and it might help > people if they knew where to query for these metrics. > > OK? > martijn@ > > Index: relayd.conf.5 > === > RCS file: /cvs/src/usr.sbin/relayd/relayd.conf.5,v > retrieving revision 1.201 > diff -u -p -r1.201 relayd.conf.5 > --- relayd.conf.5 22 Oct 2020 08:00:24 - 1.201 > +++ relayd.conf.5 30 Oct 2020 08:48:23 - > @@ -121,10 +121,12 @@ Here are the settings that can be set gl > .It Ic agentx Oo Ic context Ar context Oc Oo Ic path Ar path Oc > Export > .Xr relayd 8 > -statistics via an agentx compatible > +metrics via an agentx compatible > .Pq snmp > daemon by connecting to > .Ar path . > +Metrics can be found under the relaydMIBObjects subtree > +.Pq enterprises.30155.3 . > If > .Ar path > is omitted it will default to >
Re: relayd(8) remove snmp keyword
ok benno@ and yes, add a line to current.html. Denis Fondras(open...@ledeuns.net) on 2020.10.30 10:13:56 +0100: > On Thu, Oct 29, 2020 at 03:51:24PM +0100, Martijn van Duren wrote: > > 6.8 is out in the wild. I guess this is as good a time as any to remove > > the old snmp keyword. > > > > OK? > > > > OK denis@ > > And while it is fresh, is this the right time to update plus.html and > current.html ? >
Re: Minor tweak relayd agentx manpage
On Fri, Oct 30, 2020 at 09:53:08AM +0100, Martijn van Duren wrote: > I think metrics is a better word than statistics and it might help > people if they knew where to query for these metrics. > > OK? I also find it more accurate. OK denis@ > martijn@ > > Index: relayd.conf.5 > === > RCS file: /cvs/src/usr.sbin/relayd/relayd.conf.5,v > retrieving revision 1.201 > diff -u -p -r1.201 relayd.conf.5 > --- relayd.conf.5 22 Oct 2020 08:00:24 - 1.201 > +++ relayd.conf.5 30 Oct 2020 08:48:23 - > @@ -121,10 +121,12 @@ Here are the settings that can be set gl > .It Ic agentx Oo Ic context Ar context Oc Oo Ic path Ar path Oc > Export > .Xr relayd 8 > -statistics via an agentx compatible > +metrics via an agentx compatible > .Pq snmp > daemon by connecting to > .Ar path . > +Metrics can be found under the relaydMIBObjects subtree > +.Pq enterprises.30155.3 . > If > .Ar path > is omitted it will default to >
Re: relayd(8) remove snmp keyword
On Thu, Oct 29, 2020 at 03:51:24PM +0100, Martijn van Duren wrote: > 6.8 is out in the wild. I guess this is as good a time as any to remove > the old snmp keyword. > > OK? > OK denis@ And while it is fresh, is this the right time to update plus.html and current.html ?
Minor tweak relayd agentx manpage
I think metrics is a better word than statistics and it might help people if they knew where to query for these metrics. OK? martijn@ Index: relayd.conf.5 === RCS file: /cvs/src/usr.sbin/relayd/relayd.conf.5,v retrieving revision 1.201 diff -u -p -r1.201 relayd.conf.5 --- relayd.conf.5 22 Oct 2020 08:00:24 - 1.201 +++ relayd.conf.5 30 Oct 2020 08:48:23 - @@ -121,10 +121,12 @@ Here are the settings that can be set gl .It Ic agentx Oo Ic context Ar context Oc Oo Ic path Ar path Oc Export .Xr relayd 8 -statistics via an agentx compatible +metrics via an agentx compatible .Pq snmp daemon by connecting to .Ar path . +Metrics can be found under the relaydMIBObjects subtree +.Pq enterprises.30155.3 . If .Ar path is omitted it will default to
