Re: unlock mmap(2) for anonymous mappings

2022-01-27 Thread Klemens Nanni
On Tue, Jan 25, 2022 at 07:54:52AM +, Klemens Nanni wrote:
> > Could we use a vm_map_assert_locked() or something similar that reflect
> > the exclusive or shared (read) lock comments?  I don't trust comments.
> > It's too easy to miss a lock in a code path.
> 
> This survives desktop usage, running regress and building kernels on
> amd64.
> 
> I'll throw it at sparc64 soon.
> 
> > 
> > > So annotate functions using `size' wrt. the required lock.

Here's a better diff that asserts for read, write or any type of vm_map
lock.

vm_map_lock() and vm_map_lock_read() are used for exclusive and shared
access respectively;  unless I missed something, no code path is purely
protected by vm_map_lock_read() alone, i.e. functions called with a read
lock held by the callee are also called with a write lock elsewhere.

That means my new vm_map_assert_locked_read() is currently unused, but
vm_map_assert_locked_any() and vm_map_assert_locked_write() are now used
to validate existing function comments as well as a few new places.

amd64 and sparc64 are happy with this, both daily drivers as well as
build/regress machines.

I'd appreciate more tests and reports about running with this diff and
mmap(2) unlocked.

Index: uvm_map.c
===
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.282
diff -u -p -r1.282 uvm_map.c
--- uvm_map.c   21 Dec 2021 22:21:32 -  1.282
+++ uvm_map.c   27 Jan 2022 10:45:47 -
@@ -805,6 +805,8 @@ uvm_map_sel_limits(vaddr_t *min, vaddr_t
  * Fills in *start_ptr and *end_ptr to be the first and last entry describing
  * the space.
  * If called with prefilled *start_ptr and *end_ptr, they are to be correct.
+ *
+ * map must be at least read-locked.
  */
 int
 uvm_map_isavail(struct vm_map *map, struct uvm_addr_state *uaddr,
@@ -815,6 +817,8 @@ uvm_map_isavail(struct vm_map *map, stru
struct uvm_map_addr *atree;
struct vm_map_entry *i, *i_end;
 
+   vm_map_assert_locked_any(map);
+
if (addr + sz < addr)
return 0;
 
@@ -1821,6 +1825,8 @@ boolean_t
 uvm_map_lookup_entry(struct vm_map *map, vaddr_t address,
 struct vm_map_entry **entry)
 {
+   vm_map_assert_locked_any(map);
+
*entry = uvm_map_entrybyaddr(>addr, address);
return *entry != NULL && !UVM_ET_ISHOLE(*entry) &&
(*entry)->start <= address && (*entry)->end > address;
@@ -2206,6 +2212,8 @@ uvm_unmap_kill_entry(struct vm_map *map,
  * If remove_holes, then remove ET_HOLE entries as well.
  * If markfree, entry will be properly marked free, otherwise, no replacement
  * entry will be put in the tree (corrupting the tree).
+ *
+ * map must be locked.
  */
 void
 uvm_unmap_remove(struct vm_map *map, vaddr_t start, vaddr_t end,
@@ -2214,6 +,8 @@ uvm_unmap_remove(struct vm_map *map, vad
 {
struct vm_map_entry *prev_hint, *next, *entry;
 
+   vm_map_assert_locked_write(map);
+
start = MAX(start, map->min_offset);
end = MIN(end, map->max_offset);
if (start >= end)
@@ -2976,12 +2986,17 @@ uvm_tree_sanity(struct vm_map *map, char
UVM_ASSERT(map, addr == vm_map_max(map), file, line);
 }
 
+/*
+ * map must be at least read-locked.
+ */
 void
 uvm_tree_size_chk(struct vm_map *map, char *file, int line)
 {
struct vm_map_entry *iter;
vsize_t size;
 
+   vm_map_assert_locked_any(map);
+
size = 0;
RBT_FOREACH(iter, uvm_map_addr, >addr) {
if (!UVM_ET_ISHOLE(iter))
@@ -4268,7 +4283,7 @@ uvm_map_submap(struct vm_map *map, vaddr
  * uvm_map_checkprot: check protection in map
  *
  * => must allow specific protection in a fully allocated region.
- * => map mut be read or write locked by caller.
+ * => map must be read or write locked by caller.
  */
 boolean_t
 uvm_map_checkprot(struct vm_map *map, vaddr_t start, vaddr_t end,
@@ -4276,6 +4291,8 @@ uvm_map_checkprot(struct vm_map *map, va
 {
struct vm_map_entry *entry;
 
+   vm_map_assert_locked_any(map);
+
if (start < map->min_offset || end > map->max_offset || start > end)
return FALSE;
if (start == end)
@@ -5557,6 +5577,46 @@ vm_map_unbusy_ln(struct vm_map *map, cha
mtx_leave(>flags_lock);
if (oflags & VM_MAP_WANTLOCK)
wakeup(>flags);
+}
+
+void
+vm_map_assert_locked_any_ln(struct vm_map *map, char *file, int line)
+{
+   LPRINTF(("map assert read or write locked: %p (at %s %d)\n", map, file, 
line));
+   if ((map->flags & VM_MAP_INTRSAFE) == 0)
+   rw_assert_anylock(>lock);
+   else
+   MUTEX_ASSERT_LOCKED(>mtx);
+}
+
+void
+vm_map_assert_locked_read_ln(struct vm_map *map, char *file, int line)
+{
+   LPRINTF(("map assert read locked: %p (at %s %d)\n", map

Re: unlock mmap(2) for anonymous mappings

2022-01-24 Thread Klemens Nanni
On Mon, Jan 24, 2022 at 12:08:33PM -0300, Martin Pieuchot wrote:
> On 24/01/22(Mon) 12:06, Klemens Nanni wrote:
> > On Sun, Jan 16, 2022 at 09:22:50AM -0300, Martin Pieuchot wrote:
> > > IMHO this approach of let's try if it works now and revert if it isn't
> > > doesn't help us make progress.  I'd be more confident seeing diffs that
> > > assert for the right lock in the functions called by uvm_mapanon() and
> > > documentation about which lock is protecting which field of the data
> > > structures.
> > 
> > I picked `vm_map's `size' as first underdocumented member.
> > All accesses to it are protected by vm_map_lock(), either through the
> > function itself or implicitly by already requiring the calling function
> > to lock the map.
> 
> Could we use a vm_map_assert_locked() or something similar that reflect
> the exclusive or shared (read) lock comments?  I don't trust comments.
> It's too easy to miss a lock in a code path.

This survives desktop usage, running regress and building kernels on
amd64.

I'll throw it at sparc64 soon.

> 
> > So annotate functions using `size' wrt. the required lock.

Index: uvm_map.c
===
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.282
diff -u -p -r1.282 uvm_map.c
--- uvm_map.c   21 Dec 2021 22:21:32 -  1.282
+++ uvm_map.c   25 Jan 2022 07:37:32 -
@@ -805,6 +805,8 @@ uvm_map_sel_limits(vaddr_t *min, vaddr_t
  * Fills in *start_ptr and *end_ptr to be the first and last entry describing
  * the space.
  * If called with prefilled *start_ptr and *end_ptr, they are to be correct.
+ *
+ * map must be at least read-locked.
  */
 int
 uvm_map_isavail(struct vm_map *map, struct uvm_addr_state *uaddr,
@@ -815,6 +817,8 @@ uvm_map_isavail(struct vm_map *map, stru
struct uvm_map_addr *atree;
struct vm_map_entry *i, *i_end;
 
+   vm_map_assert_locked(map);
+
if (addr + sz < addr)
return 0;
 
@@ -986,6 +990,8 @@ uvm_mapanon(struct vm_map *map, vaddr_t 
vaddr_t  pmap_align, pmap_offset;
vaddr_t  hint;
 
+   vm_map_assert_unlocked(map);
+
KASSERT((map->flags & VM_MAP_ISVMSPACE) == VM_MAP_ISVMSPACE);
KASSERT(map != kernel_map);
KASSERT((map->flags & UVM_FLAG_HOLE) == 0);
@@ -1189,6 +1195,8 @@ uvm_map(struct vm_map *map, vaddr_t *add
vaddr_t  pmap_align, pmap_offset;
vaddr_t  hint;
 
+   vm_map_assert_unlocked(map);
+
if ((map->flags & VM_MAP_INTRSAFE) == 0)
splassert(IPL_NONE);
else
@@ -1821,6 +1829,8 @@ boolean_t
 uvm_map_lookup_entry(struct vm_map *map, vaddr_t address,
 struct vm_map_entry **entry)
 {
+   vm_map_assert_locked(map);
+
*entry = uvm_map_entrybyaddr(>addr, address);
return *entry != NULL && !UVM_ET_ISHOLE(*entry) &&
(*entry)->start <= address && (*entry)->end > address;
@@ -2206,6 +2216,8 @@ uvm_unmap_kill_entry(struct vm_map *map,
  * If remove_holes, then remove ET_HOLE entries as well.
  * If markfree, entry will be properly marked free, otherwise, no replacement
  * entry will be put in the tree (corrupting the tree).
+ *
+ * map must be locked.
  */
 void
 uvm_unmap_remove(struct vm_map *map, vaddr_t start, vaddr_t end,
@@ -2214,6 +2226,8 @@ uvm_unmap_remove(struct vm_map *map, vad
 {
struct vm_map_entry *prev_hint, *next, *entry;
 
+   vm_map_assert_locked(map);
+
start = MAX(start, map->min_offset);
end = MIN(end, map->max_offset);
if (start >= end)
@@ -2976,13 +2990,17 @@ uvm_tree_sanity(struct vm_map *map, char
UVM_ASSERT(map, addr == vm_map_max(map), file, line);
 }
 
+/*
+ * map must be at least read-locked.
+ */
 void
 uvm_tree_size_chk(struct vm_map *map, char *file, int line)
 {
struct vm_map_entry *iter;
-   vsize_t size;
+   vsize_t size = 0;
+
+   vm_map_assert_locked(map);
 
-   size = 0;
RBT_FOREACH(iter, uvm_map_addr, >addr) {
if (!UVM_ET_ISHOLE(iter))
size += iter->end - iter->start;
@@ -3320,6 +3338,8 @@ uvm_map_protect(struct vm_map *map, vadd
vsize_t dused;
int error;
 
+   vm_map_assert_unlocked(map);
+
if (start > end)
return EINVAL;
start = MAX(start, map->min_offset);
@@ -4096,6 +4116,7 @@ uvmspace_fork(struct process *pr)
struct vm_map_entry *old_entry, *new_entry;
struct uvm_map_deadq dead;
 
+   vm_map_assert_unlocked(old_map);
vm_map_lock(old_map);
 
vm2 = uvmspace_alloc(old_map->min_offset, old_map->max_offset,
@@ -4236,6 +4257,8 @@ uvm_map_submap(struct vm_map *map, vaddr
struct vm_map_entry *entry;
int

Re: unlock mmap(2) for anonymous mappings

2022-01-24 Thread Klemens Nanni
On Sun, Jan 16, 2022 at 09:22:50AM -0300, Martin Pieuchot wrote:
> IMHO this approach of let's try if it works now and revert if it isn't
> doesn't help us make progress.  I'd be more confident seeing diffs that
> assert for the right lock in the functions called by uvm_mapanon() and
> documentation about which lock is protecting which field of the data
> structures.

I picked `vm_map's `size' as first underdocumented member.
All accesses to it are protected by vm_map_lock(), either through the
function itself or implicitly by already requiring the calling function
to lock the map.

So annotate functions using `size' wrt. the required lock.

Index: uvm_map.c
===
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.282
diff -u -p -r1.282 uvm_map.c
--- uvm_map.c   21 Dec 2021 22:21:32 -  1.282
+++ uvm_map.c   21 Jan 2022 13:03:05 -
@@ -805,6 +805,8 @@ uvm_map_sel_limits(vaddr_t *min, vaddr_t
  * Fills in *start_ptr and *end_ptr to be the first and last entry describing
  * the space.
  * If called with prefilled *start_ptr and *end_ptr, they are to be correct.
+ *
+ * map must be at least read-locked.
  */
 int
 uvm_map_isavail(struct vm_map *map, struct uvm_addr_state *uaddr,
@@ -2206,6 +2208,8 @@ uvm_unmap_kill_entry(struct vm_map *map,
  * If remove_holes, then remove ET_HOLE entries as well.
  * If markfree, entry will be properly marked free, otherwise, no replacement
  * entry will be put in the tree (corrupting the tree).
+ *
+ * map must be locked.
  */
 void
 uvm_unmap_remove(struct vm_map *map, vaddr_t start, vaddr_t end,
@@ -2976,6 +2980,9 @@ uvm_tree_sanity(struct vm_map *map, char
UVM_ASSERT(map, addr == vm_map_max(map), file, line);
 }
 
+/*
+ * map must be at least read-locked.
+ */
 void
 uvm_tree_size_chk(struct vm_map *map, char *file, int line)
 {
Index: uvm_map.h
===
RCS file: /cvs/src/sys/uvm/uvm_map.h,v
retrieving revision 1.71
diff -u -p -r1.71 uvm_map.h
--- uvm_map.h   15 Dec 2021 12:53:53 -  1.71
+++ uvm_map.h   21 Jan 2022 12:51:26 -
@@ -272,7 +272,7 @@ struct vm_map {
 
struct uvm_map_addr addr;   /* [v] Entry tree, by addr */
 
-   vsize_t size;   /* virtual size */
+   vsize_t size;   /* [v] virtual size */
int ref_count;  /* [a] Reference count */
int flags;  /* flags */
struct mutexflags_lock; /* flags lock */



Re: dt: make vmm tracepoints amd64 only

2022-01-24 Thread Klemens Nanni
On Mon, Jan 17, 2022 at 04:51:37PM -0800, Philip Guenther wrote:
> On Mon, Jan 17, 2022 at 5:02 AM Klemens Nanni  wrote:
> 
> > These don't hurt on !VMM architectures but I was still surprised to see
> > them on e.g. sparc64:
> >
> > # arch -s ; btrace -l | grep vmm
> > sparc64
> > tracepoint:vmm:guest_enter
> > tracepoint:vmm:guest_exit
> >
> > Like some network drivers, we could use __amd64__ to limit those to
> > amd64 and save a few bits in all other kernels.
> >
> > Is this approach feasible or should we just ignore such one-offs?
> >
> 
> If there's a non-trivial number of trace points in drivers, then it would
> suggest that such tracepoints should be added/registered/pick-a-term by the
> driver's attach routine.

This needs a little bit of refactor around probe registration.

I plan to commit the initial `#ifdef __amd64__' diff with "OK pv" soon
unless I hear objections.



Re: 'pseudo-device dt' on macppc

2022-01-21 Thread Klemens Nanni
On Tue, Jan 18, 2022 at 07:59:20AM +0300, Andrew Krasavin wrote:
> Hello!
> 
> Is there a reason not to enable dt(4) on macppc by default? I have
> built a kernel with dt enabled for test purposes and it seems to work
> correctly - commands like 'btrace /usr/share/btrace/kprofile.bt'
> work and I get stacks.
> 
> Maybe I'm missing something and there are known problems? But if
> not - maybe dt(4) should be enabled by default on macppc?
> 
> The diff I used is obvious, but anyway:
> 
> --- sys/arch/macppc/conf/GENERIC.orig
> +++ sys/arch/macppc/conf/GENERIC
> @@ -406,4 +406,6 @@ owtemp* at onewire? # Temperature
>  owctr* at onewire? # Counter device
>  pseudo-device  hotplug 1   # devices hot plugging
> +pseudo-device  dt
> +
>  pseudo-device  wsmux   2   # mouse & keyboard multiplexor
> 
> In any case - thanks.

This gets us useful flamegraphs on macppc.
Feedback? Objections? OK?


Index: sys/arch/macppc/conf/GENERIC
===
RCS file: /cvs/src/sys/arch/macppc/conf/GENERIC,v
retrieving revision 1.275
diff -u -p -r1.275 GENERIC
--- sys/arch/macppc/conf/GENERIC21 Oct 2021 18:36:42 -  1.275
+++ sys/arch/macppc/conf/GENERIC21 Jan 2022 11:36:54 -
@@ -406,4 +406,5 @@ owtemp* at onewire? # Temperature
 owctr* at onewire? # Counter device
 
 pseudo-device  hotplug 1   # devices hot plugging
+pseudo-device  dt
 pseudo-device  wsmux   2   # mouse & keyboard multiplexor



Re: btrace: print probes sorted by name not internal id

2022-01-17 Thread Klemens Nanni
On Tue, Jan 18, 2022 at 01:03:04AM +0100, Alexander Hall wrote:
> On Mon, Jan 17, 2022 at 02:23:52PM +0000, Klemens Nanni wrote:
> > I keep doing `btrace -l | sort' to help myself searching for traces.
> > 
> > Would it be worh making btrace sort the list of probes
> > lexicographically in the first place or is there any value in seeing
> > them sorted by id?
> 
> I believe you're ruining the party for dtpi_get_by_id(), which only
> seems to care about the id.

Thanks, btrace would just print the probe'd id when using the `probe'
variable, i.e.
btrace -e 'syscall:ioctl:entry { @[probe] = count(); }'
would print
@[109]: 4
rather than
@[syscall:ioctl:entry]: 4


The new diff uses a second name based compare function to sort and
linear-searches `probe' names via now that the global list is sorted by
name not id anymore.

Index: btrace.c
===
RCS file: /cvs/src/usr.sbin/btrace/btrace.c,v
retrieving revision 1.61
diff -u -p -r1.61 btrace.c
--- btrace.c7 Dec 2021 22:17:03 -   1.61
+++ btrace.c18 Jan 2022 03:55:06 -
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -62,7 +63,7 @@ char  *read_btfile(const char *, size_t
  */
 voiddtpi_cache(int);
 voiddtpi_print_list(void);
-const char *dtpi_func(struct dtioc_probe_info *);
+const char *dtpi_func(const struct dtioc_probe_info *);
 int dtpi_is_unit(const char *);
 struct dtioc_probe_info*dtpi_get_by_value(const char *, const char *,
 const char *);
@@ -257,7 +258,7 @@ read_btfile(const char *filename, size_t
 }
 
 static int
-dtpi_cmp(const void *a, const void *b)
+dtpi_cmp_id(const void *a, const void *b)
 {
const struct dtioc_probe_info *ai = a, *bi = b;
 
@@ -267,6 +268,32 @@ dtpi_cmp(const void *a, const void *b)
return -1;
return 0;
 }
+static int
+dtpi_cmp_str(const void *a, const void *b)
+{
+   const struct dtioc_probe_info *ai = a, *bi = b;
+   int i;
+
+   i = strcmp(ai->dtpi_prov, bi->dtpi_prov);
+   if (i > 0)
+   return 1;
+   if (i < 0)
+   return -1;
+
+   i = strcmp(dtpi_func(ai), dtpi_func(bi));
+   if (i > 0)
+   return 1;
+   if (i < 0)
+   return -1;
+
+   i = strcmp(ai->dtpi_name, bi->dtpi_name);
+   if (i > 0)
+   return 1;
+   if (i < 0)
+   return -1;
+
+   return 0;
+}
 
 void
 dtpi_cache(int fd)
@@ -289,7 +316,7 @@ dtpi_cache(int fd)
if (ioctl(fd, DTIOCGPLIST, ))
err(1, "DTIOCGPLIST");
 
-   qsort(dt_dtpis, dt_ndtpi, sizeof(*dt_dtpis), dtpi_cmp);
+   qsort(dt_dtpis, dt_ndtpi, sizeof(*dt_dtpis), dtpi_cmp_str);
 }
 
 void
@@ -306,7 +333,7 @@ dtpi_print_list(void)
 }
 
 const char *
-dtpi_func(struct dtioc_probe_info *dtpi)
+dtpi_func(const struct dtioc_probe_info *dtpi)
 {
char *sysnb, func[DTNAMESIZE];
const char *errstr;
@@ -371,7 +398,7 @@ dtpi_get_by_id(unsigned int pbn)
struct dtioc_probe_info d;
 
d.dtpi_pbn = pbn;
-   return bsearch(, dt_dtpis, dt_ndtpi, sizeof(*dt_dtpis), dtpi_cmp);
+   return lfind(, dt_dtpis, _ndtpi, sizeof(*dt_dtpis), dtpi_cmp_id);
 }
 
 void



DDBPROF: move option to amd64,i386 GENERIC

2022-01-17 Thread Klemens Nanni
While intended for more architectures, DDBPROF is strictly amd64 and
i386 only, so the machine-independent sys/conf/GENERIC does not seem fit
(until all architectures are supported).

Index: conf/GENERIC
===
RCS file: /cvs/src/sys/conf/GENERIC,v
retrieving revision 1.281
diff -u -p -r1.281 GENERIC
--- conf/GENERIC23 Dec 2021 10:04:14 -  1.281
+++ conf/GENERIC18 Jan 2022 04:28:29 -
@@ -4,7 +4,6 @@
 #  GENERIC kernel
 
 option DDB # in-kernel debugger
-#optionDDBPROF # ddb(4) based profiling
 #optionDDB_SAFE_CONSOLE # allow break into ddb during boot
 #makeoptions   DEBUG=""# do not compile full symbol table
 #makeoptions   PROF="-pg"  # build profiled kernel
Index: arch/amd64/conf/GENERIC
===
RCS file: /cvs/src/sys/arch/amd64/conf/GENERIC,v
retrieving revision 1.510
diff -u -p -r1.510 GENERIC
--- arch/amd64/conf/GENERIC 4 Jan 2022 05:50:43 -   1.510
+++ arch/amd64/conf/GENERIC 18 Jan 2022 04:28:04 -
@@ -13,6 +13,8 @@ machine   amd64
 include"../../../conf/GENERIC"
 maxusers   80  # estimated number of users
 
+#optionDDBPROF # ddb(4) based profiling
+
 option USER_PCICONF# user-space PCI configuration
 
 option APERTURE# in-kernel aperture driver for XFree86
Index: arch/i386/conf/GENERIC
===
RCS file: /cvs/src/sys/arch/i386/conf/GENERIC,v
retrieving revision 1.860
diff -u -p -r1.860 GENERIC
--- arch/i386/conf/GENERIC  2 Jan 2022 23:14:27 -   1.860
+++ arch/i386/conf/GENERIC  18 Jan 2022 04:28:26 -
@@ -13,6 +13,8 @@ machine   i386
 include"../../../conf/GENERIC"
 maxusers   80  # estimated number of users
 
+#optionDDBPROF # ddb(4) based profiling
+
 option USER_PCICONF# user-space PCI configuration
 
 option APERTURE# in-kernel aperture driver for XFree86



btrace: print probes sorted by name not internal id

2022-01-17 Thread Klemens Nanni
I keep doing `btrace -l | sort' to help myself searching for traces.

Would it be worh making btrace sort the list of probes
lexicographically in the first place or is there any value in seeing
them sorted by id?

Index: btrace.c
===
RCS file: /cvs/src/usr.sbin/btrace/btrace.c,v
retrieving revision 1.61
diff -u -p -r1.61 btrace.c
--- btrace.c7 Dec 2021 22:17:03 -   1.61
+++ btrace.c17 Jan 2022 14:13:27 -
@@ -62,7 +62,7 @@ char  *read_btfile(const char *, size_t
  */
 voiddtpi_cache(int);
 voiddtpi_print_list(void);
-const char *dtpi_func(struct dtioc_probe_info *);
+const char *dtpi_func(const struct dtioc_probe_info *);
 int dtpi_is_unit(const char *);
 struct dtioc_probe_info*dtpi_get_by_value(const char *, const char *,
 const char *);
@@ -260,11 +260,26 @@ static int
 dtpi_cmp(const void *a, const void *b)
 {
const struct dtioc_probe_info *ai = a, *bi = b;
+   int i;
 
-   if (ai->dtpi_pbn > bi->dtpi_pbn)
+   i = strcmp(ai->dtpi_prov, bi->dtpi_prov);
+   if (i > 0)
return 1;
-   if (ai->dtpi_pbn < bi->dtpi_pbn)
+   if (i < 0)
return -1;
+
+   i = strcmp(dtpi_func(ai), dtpi_func(bi));
+   if (i > 0)
+   return 1;
+   if (i < 0)
+   return -1;
+
+   i = strcmp(ai->dtpi_name, bi->dtpi_name);
+   if (i > 0)
+   return 1;
+   if (i < 0)
+   return -1;
+
return 0;
 }
 
@@ -306,7 +321,7 @@ dtpi_print_list(void)
 }
 
 const char *
-dtpi_func(struct dtioc_probe_info *dtpi)
+dtpi_func(const struct dtioc_probe_info *dtpi)
 {
char *sysnb, func[DTNAMESIZE];
const char *errstr;



dt: make vmm tracepoints amd64 only

2022-01-17 Thread Klemens Nanni
These don't hurt on !VMM architectures but I was still surprised to see
them on e.g. sparc64:

# arch -s ; btrace -l | grep vmm
sparc64
tracepoint:vmm:guest_enter
tracepoint:vmm:guest_exit

Like some network drivers, we could use __amd64__ to limit those to
amd64 and save a few bits in all other kernels.

Is this approach feasible or should we just ignore such one-offs?

One can specify and "use" those tracepoints without errors in btrace(8),
but they won't ever hit on !amd64, of course.

Tested on amd64 and sparc64.

Index: dev/dt//dt_prov_static.c
===
RCS file: /cvs/src/sys/dev/dt/dt_prov_static.c,v
retrieving revision 1.11
diff -u -p -r1.11 dt_prov_static.c
--- dev/dt//dt_prov_static.c24 Nov 2021 09:47:49 -  1.11
+++ dev/dt//dt_prov_static.c17 Jan 2022 12:47:38 -
@@ -69,11 +69,13 @@ DT_STATIC_PROBE3(vfs, bufcache_rel, "lon
 DT_STATIC_PROBE3(vfs, bufcache_take, "long", "int", "int64_t");
 DT_STATIC_PROBE4(vfs, cleaner, "long", "int", "long", "long");
 
+#ifdef __amd64__
 /*
  * VMM
  */
 DT_STATIC_PROBE2(vmm, guest_enter, "void *", "void *");
 DT_STATIC_PROBE3(vmm, guest_exit, "void *", "void *", "uint64_t");
+#endif /* __amd64__ */
 
 /*
  * SMR
@@ -113,9 +115,11 @@ struct dt_probe *dtps_static[] = {
&_DT_STATIC_P(vfs, bufcache_rel),
&_DT_STATIC_P(vfs, bufcache_take),
&_DT_STATIC_P(vfs, cleaner),
+#ifdef __amd64__
/* VMM */
&_DT_STATIC_P(vmm, guest_enter),
&_DT_STATIC_P(vmm, guest_exit),
+#endif /* __amd64__ */
/* SMR */
&_DT_STATIC_P(smr, call),
&_DT_STATIC_P(smr, called),



uvm_flush: return int, sync with netbsd

2022-01-17 Thread Klemens Nanni
One small mechanical diff to get rid of the boolean_t signature and
reduce difference to NetBSD.

uvm_flush() aka. uao_flush()/udv_flush()/pgo_flush()/uvn_flush() returns
TRUE/FALSE only to make uvm_map_clean() conditionally return EFAULT.

This makes the *_flush() return 0/EFAULT themselves and thus sets
uvm_map_clean()'s `error' return value directly rather than using an
intermediate `rv' value to to the FALSE/EFAULT check/set.

Use parentheses (around function pointers) while here, reducing NetBSD
diff churn.

No functional change.
Feedback? Objection? OK?

Index: uvm_aobj.c
===
RCS file: /cvs/src/sys/uvm/uvm_aobj.c,v
retrieving revision 1.103
diff -u -p -r1.103 uvm_aobj.c
--- uvm_aobj.c  29 Dec 2021 20:22:06 -  1.103
+++ uvm_aobj.c  10 Jan 2022 10:10:29 -
@@ -144,7 +144,7 @@ struct pool uvm_aobj_pool;
 static struct uao_swhash_elt   *uao_find_swhash_elt(struct uvm_aobj *, int,
 boolean_t);
 static int  uao_find_swslot(struct uvm_object *, int);
-static boolean_tuao_flush(struct uvm_object *, voff_t,
+static int  uao_flush(struct uvm_object *, voff_t,
 voff_t, int);
 static void uao_free(struct uvm_aobj *);
 static int  uao_get(struct uvm_object *, voff_t,
@@ -861,11 +861,11 @@ uao_detach(struct uvm_object *uobj)
  * => NOTE: we are allowed to lock the page queues, so the caller
  * must not be holding the lock on them [e.g. pagedaemon had
  * better not call us with the queues locked]
- * => we return TRUE unless we encountered some sort of I/O error
+ * => we return 0 unless we encountered some sort of I/O error
  * XXXJRT currently never happens, as we never directly initiate
  * XXXJRT I/O
  */
-boolean_t
+static int
 uao_flush(struct uvm_object *uobj, voff_t start, voff_t stop, int flags)
 {
struct uvm_aobj *aobj = (struct uvm_aobj *) uobj;
@@ -893,7 +893,7 @@ uao_flush(struct uvm_object *uobj, voff_
 * or deactivating pages.
 */
if ((flags & (PGO_DEACTIVATE|PGO_FREE)) == 0) {
-   return TRUE;
+   return 0;
}
 
curoff = start;
@@ -971,7 +971,7 @@ uao_flush(struct uvm_object *uobj, voff_
}
}
 
-   return TRUE;
+   return 0;
 }
 
 /*
Index: uvm_device.c
===
RCS file: /cvs/src/sys/uvm/uvm_device.c,v
retrieving revision 1.66
diff -u -p -r1.66 uvm_device.c
--- uvm_device.c15 Dec 2021 12:53:53 -  1.66
+++ uvm_device.c10 Jan 2022 10:10:29 -
@@ -60,7 +60,7 @@ static void udv_detach(struc
 static int udv_fault(struct uvm_faultinfo *, vaddr_t,
   vm_page_t *, int, int, vm_fault_t,
   vm_prot_t, int);
-static boolean_tudv_flush(struct uvm_object *, voff_t, voff_t,
+static int udv_flush(struct uvm_object *, voff_t, voff_t,
   int);
 
 /*
@@ -290,11 +290,11 @@ again:
  *
  * flush pages out of a uvm object.   a no-op for devices.
  */
-static boolean_t
+static int
 udv_flush(struct uvm_object *uobj, voff_t start, voff_t stop, int flags)
 {
 
-   return(TRUE);
+   return 0;
 }
 
 /*
Index: uvm_fault.c
===
RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
retrieving revision 1.124
diff -u -p -r1.124 uvm_fault.c
--- uvm_fault.c 28 Dec 2021 13:16:28 -  1.124
+++ uvm_fault.c 10 Jan 2022 10:10:29 -
@@ -794,7 +794,7 @@ uvm_fault_check(struct uvm_faultinfo *uf
 
uoff = (flt->startva - ufi->entry->start) + 
ufi->entry->offset;
rw_enter(uobj->vmobjlock, RW_WRITE);
-   (void) uobj->pgops->pgo_flush(uobj, uoff, uoff +
+   (void) (uobj->pgops->pgo_flush)(uobj, uoff, uoff +
((vsize_t)nback << PAGE_SHIFT), PGO_DEACTIVATE);
rw_exit(uobj->vmobjlock);
}
Index: uvm_map.c
===
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.282
diff -u -p -r1.282 uvm_map.c
--- uvm_map.c   21 Dec 2021 22:21:32 -  1.282
+++ uvm_map.c   10 Jan 2022 10:10:29 -
@@ -4658,7 +4658,6 @@ uvm_map_clean(struct vm_map *map, vaddr_
vaddr_t cp_start, cp_end;
int refs;
int error;
-   boolean_t rv;
 
KASSERT((flags & (PGO_FREE|PGO_DEACTIVATE)) !=
(PGO_FREE|PGO_DEACTIVATE));
@@ -4786,18 +4785,15 @@ flush_object:
 ((entry->max_protection & PROT_WRITE) != 0 &&
  (entry->etype & UVM_ET_COPYONWRITE) == 0))) {
rw_enter(uobj->vmobjlock, RW_WRITE);
-   

Re: fw_update(8) improve verbose output

2022-01-16 Thread Klemens Nanni
On Wed, Jan 05, 2022 at 05:36:42PM -0800, Andrew Hewus Fresh wrote:
> After getting fw_update(8) into a state where it could get some testing,
> I found that the man page indicated that -v should indicate different
> levels of verbosity and I currently only had one.
> 
> This was useful as I didn't really like the output anyway.
> 
> Now one -v prints out an additional line when it's doing something to
> the firmware.  Two add a progress bar and mentions "detecting". Three
> provide a bit more debugging mostly from ftp(1).

If you want multiple levels, I'd treat VERBOSE as an integer.  Basically
use `((VERBOSE))' as "non-zero?", `((VERBOSE > 1))', etc.

I went through your patch (does not apply anymore after the other diffs
were committed) and did the mechanical conversion whilst introducing
your levels and messages.

`doas ./fw_update -n -v[[v]v]' shows different output for me, but I have
not tested it thoroughly.

> Also a couple extra small improvements, hiding errors from killing the
> ftp subprocess which could happen if it exits before we do, just
> using `firmware_devicename "$_d"` instead of `echo "${...}"`, and using
> a normal [=] comparison instead of [[=]] because we don't want pattern
> matching there.

I'd do that separately, the verbosity changes are big and noisy already.


Feel free to take over and commit any part of this diff, I just put it
here to demonstrate the shell bits.


Index: fw_update.sh
===
RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
retrieving revision 1.30
diff -u -p -r1.30 fw_update.sh
--- fw_update.sh12 Jan 2022 02:21:15 -  1.30
+++ fw_update.sh16 Jan 2022 16:27:09 -
@@ -35,7 +35,7 @@ FWURL=http://firmware.openbsd.org/firmwa
 FWPUB_KEY=${DESTDIR}/etc/signify/openbsd-${VERSION}-fw.pub
 
 DRYRUN=false
-VERBOSE=false
+integer VERBOSE=0
 DELETE=false
 DOWNLOAD=true
 INSTALL=true
@@ -69,18 +69,19 @@ tmpdir() {
 
 fetch() {
local _src="${FWURL}/${1##*/}" _dst=$1 _user=_file _exit _error=''
+   local _flags=-VM
+
+   ((VERBOSE > 1)) && _flags=-vm
 
# The installer uses a limited doas(1) as a tiny su(1)
set -o monitor # make sure ftp gets its own process group
(
-   flags=-VM
-   "$VERBOSE" && flags=-vm
if [ -x /usr/bin/su ]; then
exec /usr/bin/su -s /bin/ksh "$_user" -c \
-   "/usr/bin/ftp -N '${0##/}' -D 'Get/Verify' $flags -o- 
'$_src'" > "$_dst"
+   "/usr/bin/ftp -N '${0##/}' -D 'Get/Verify' $_flags -o- 
'$_src'" > "$_dst"
else
exec /usr/bin/doas -u "$_user" \
-   /usr/bin/ftp -N "${0##/}" -D 'Get/Verify' $flags -o- 
"$_src" > "$_dst"
+   /usr/bin/ftp -N "${0##/}" -D 'Get/Verify' $_flags -o- 
"$_src" > "$_dst"
fi
) & FTPPID=$!
set +o monitor
@@ -216,9 +217,9 @@ detect_firmware() {
 add_firmware () {
local _f="${1##*/}" _pkgname
FWPKGTMP="$( tmpdir "${DESTDIR}/var/db/pkg/.firmware" )"
-   local flags=-VM
-   "$VERBOSE" && flags=-vm
-   ftp -N "${0##/}" -D "Install" "$flags" -o- "file:${1}" |
+   local _flags=-VM
+   ((VERBOSE > 1)) && _flags=-Vm
+   ftp -N "${0##/}" -D "Install" "$_flags" -o- "file:${1}" |
tar -s ",^\+,${FWPKGTMP}/+," \
-s ",^firmware,${DESTDIR}/etc/firmware," \
-C / -zxphf - "+*" "firmware/*"
@@ -249,7 +250,7 @@ delete_firmware() {
local _cwd _pkg="$1" _pkgdir="${DESTDIR}/var/db/pkg"
 
# TODO: Check hash for files before deleting
-   "$VERBOSE" && echo "Uninstalling $_pkg"
+   ((VERBOSE > 2)) && echo "Uninstall $_pkg ..."
_cwd="${_pkgdir}/$_pkg"
 
if [ ! -e "$_cwd/+CONTENTS" ] ||
@@ -283,6 +284,8 @@ delete_firmware() {
rm -f "$_r"
fi
done
+
+   ((VERBOSE > 2)) && echo " done."
 }
 
 usage() {
@@ -300,7 +303,7 @@ do
F) OPT_F=true ;;
n) DRYRUN=true ;;
p) LOCALSRC="$OPTARG" ;;
-   v) VERBOSE=true ;;
+   v) ((++VERBOSE)) ;;
:)
echo "${0##*/}: option requires an argument -- -$OPTARG" >&2
usage 2
@@ -354,6 +357,9 @@ set -sA devices -- "$@"
 if "$DELETE"; then
[ "$OPT_F" ] && echo "Cannot use -F and -d" >&2 && usage 22
 
+   # Show the "Uninstall" message when just deleting not upgrading
+   ((VERBOSE > 1)) && VERBOSE=3
+
set -A installed
if [ "${devices[*]:-}" ]; then
"$ALL" && echo "Cannot use -a and devices/files" >&2 && usage 22
@@ -381,7 +387,7 @@ if "$DELETE"; then
if [ "${installed:-}" ]; then
for fw in "${installed[@]}"; do
if "$DRYRUN"; then
-   echo "Delete $fw"
+   ((VERBOSE)) && echo "Delete $fw"
else
delete_firmware "$fw" 

Re: unlock mmap(2) for anonymous mappings

2022-01-11 Thread Klemens Nanni
On Wed, Jan 12, 2022 at 02:32:31AM +0300, Vitaliy Makkoveev wrote:
> Does it make sense to document that kernel lock protects
> `ps_wxcounter’? We have such [K] in other structure definitions.
> 
> + u_int64_t ps_wxcounter; /* [K] number of W^X violations */

Yes, that would be in order.  I'll make sure to follow up with a comment
once/if progress is made -- until then I avoid changing the header so as
to avoid rebuilds due to a mere comment.

> The rest of diff looks good to me.

So far, this diff runs fine on amd64, sparc64, arm64 doing releases,
regress, ports builds and daily usage for me.

Others have reported no regression on their octeon and i386 boxes with
varying workloads, as well as additional builds on the architectures I
already tested.



Re: unlock mmap(2) for anonymous mappings

2022-01-11 Thread Klemens Nanni
On Tue, Jan 11, 2022 at 09:54:44AM -0700, Theo de Raadt wrote:
> > Now this is clearly a "slow" path.  I don't think there is any reason
> > not to put all the code in that if (uvw_wxabort) block under the
> > kernel lock.  For now I think making access to ps_wxcounter atomic is
> > just too fine grained.
> 
> Right.  Lock the whole block.

Thanks everyone, here's the combined diff for that.

Index: kern/syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.222
diff -u -p -r1.222 syscalls.master
--- kern/syscalls.master11 Jan 2022 08:09:14 -  1.222
+++ kern/syscalls.master11 Jan 2022 23:10:50 -
@@ -126,7 +126,7 @@
struct sigaction *osa); }
 47 STD NOLOCK  { gid_t sys_getgid(void); }
 48 STD NOLOCK  { int sys_sigprocmask(int how, sigset_t mask); }
-49 STD { void *sys_mmap(void *addr, size_t len, int prot, \
+49 STD NOLOCK  { void *sys_mmap(void *addr, size_t len, int prot, \
int flags, int fd, off_t pos); }
 50 STD { int sys_setlogin(const char *namebuf); }
 #ifdef ACCOUNTING
Index: uvm/uvm_mmap.c
===
RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v
retrieving revision 1.168
diff -u -p -r1.168 uvm_mmap.c
--- uvm/uvm_mmap.c  5 Jan 2022 17:53:44 -   1.168
+++ uvm/uvm_mmap.c  11 Jan 2022 23:02:13 -
@@ -183,12 +183,14 @@ uvm_wxcheck(struct proc *p, char *call)
return 0;
 
if (uvm_wxabort) {
+   KERNEL_LOCK();
/* Report W^X failures */
if (pr->ps_wxcounter++ == 0)
log(LOG_NOTICE, "%s(%d): %s W^X violation\n",
pr->ps_comm, pr->ps_pid, call);
/* Send uncatchable SIGABRT for coredump */
sigexit(p, SIGABRT);
+   KERNEL_UNLOCK();
}
 
return ENOTSUP;



Re: unlock mmap(2) for anonymous mappings

2022-01-11 Thread Klemens Nanni
On Mon, Jan 10, 2022 at 12:06:44PM +, Klemens Nanni wrote:
> On Fri, Dec 31, 2021 at 07:54:53PM +0300, Vitaliy Makkoveev wrote:
> > The uvm_wxabort path within uvm_wxcheck() looks not MP-safe.
> 
> Right, I did not pay enough attention to W^X handling.

New diff, either alone or on top of the mmap unlocking.

> I'm not entirely sure about the sigexit() path.

We can't dump core without the kernel lock, assertions do make sure...

> There's `ps_wxcounter' as u_int64_t which needs a lock or atomic
> operations.

This could look like this.  `ps_wxcounter' is not used anywhere else in
the tree;  it has been like this since import in 2016.

> The kernel lock could be pushed into uvm_wxabort() but there it'd still
> be grabbed for every mmap(2) call.

This means we're only grabbing the kernel lock if `kern.wxabort=1' is
set *and* there's a W^X violation -- much better.


Index: sys/proc.h
===
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.323
diff -u -p -r1.323 proc.h
--- sys/proc.h  10 Dec 2021 05:34:42 -  1.323
+++ sys/proc.h  9 Jan 2022 13:42:45 -
@@ -197,7 +197,7 @@ struct process {
time_t  ps_nextxcpu;/* when to send next SIGXCPU, */
/* in seconds of process runtime */
 
-   u_int64_t ps_wxcounter;
+   u_int64_t ps_wxcounter; /* [a] number of W^X violations */
 
struct unveil *ps_uvpaths;  /* unveil vnodes and names */
ssize_t ps_uvvcount;/* count of unveil vnodes held */
Index: uvm/uvm_mmap.c
===
RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v
retrieving revision 1.168
diff -u -p -r1.168 uvm_mmap.c
--- uvm/uvm_mmap.c  5 Jan 2022 17:53:44 -   1.168
+++ uvm/uvm_mmap.c  10 Jan 2022 15:48:03 -
@@ -184,11 +184,13 @@ uvm_wxcheck(struct proc *p, char *call)
 
if (uvm_wxabort) {
/* Report W^X failures */
-   if (pr->ps_wxcounter++ == 0)
+   if (atomic_inc_long_nv((unsigned long *)>ps_wxcounter) == 1)
log(LOG_NOTICE, "%s(%d): %s W^X violation\n",
pr->ps_comm, pr->ps_pid, call);
/* Send uncatchable SIGABRT for coredump */
+   KERNEL_LOCK();
sigexit(p, SIGABRT);
+   KERNEL_UNLOCK();
}
 
return ENOTSUP;



Re: unlock mmap(2) for anonymous mappings

2022-01-10 Thread Klemens Nanni
On Fri, Dec 31, 2021 at 09:54:23AM -0700, Theo de Raadt wrote:
> >Now that mpi has unlocked uvm's fault handler, we can unlock the mmap
> >syscall to handle MAP_ANON without the big lock.
> ...
> >So here's a first small step.  I've been running with this for months
> >on a few amd64, arm64 and sparc64 boxes without problems
> 
> So, 3 architectures have been tested.
> 
> I read your mail as a request for OKs to commit before the other architectures
> have been tested.
> 
> No way.  You first find people to help you test the others.  Or you ask for
> the whole community to help ensure the list is complete.  But you are begging
> for the wrong people to respond to you when you include this on the list of
> questions:
> 
> > Feedback?  Objections?  OK?
>   ^^^

Duly noted.  I use(d) this as a standard line for diffs, but you're
right in that it does not convey the necessary demand for testing in
such crucial cases.

I'll make sure to only ask for an OK next time afer enough feedbacka and
testing.



Re: unlock mmap(2) for anonymous mappings

2022-01-10 Thread Klemens Nanni
On Fri, Dec 31, 2021 at 07:54:53PM +0300, Vitaliy Makkoveev wrote:
> The uvm_wxabort path within uvm_wxcheck() looks not MP-safe.

Right, I did not pay enough attention to W^X handling.

I'm not entirely sure about the sigexit() path.

There's `ps_wxcounter' as u_int64_t which needs a lock or atomic
operations.

The kernel lock could be pushed into uvm_wxabort() but there it'd still
be grabbed for every mmap(2) call.

> 
> > On 31 Dec 2021, at 12:14, Klemens Nanni  wrote:
> > 
> > Now that mpi has unlocked uvm's fault handler, we can unlock the mmap
> > syscall to handle MAP_ANON without the big lock.
> > 
> > sys_mmap() still protects the !MAP_ANON case, i.e. file based mappings,
> > with the KERNEL_LOCK() itself, which is why unlocking the syscall will
> > only change locking behaviour for anonymous mappings.
> > 
> > A previous to unlock file based mappings was reverted, see the following
> > from https://marc.info/?l=openbsd-tech=160155434212888=2 :
> > 
> > commit 38802bc07455f2a4f8cdde272850a5eab5dfa6c8
> > from: mpi 
> > date: Wed Oct  7 12:26:20 2020 UTC
> > 
> > Do not release the KERNEL_LOCK() when mmap(2)ing files.
> > 
> > Previous attempt to unlock amap & anon exposed a race in vnode reference
> > counting.  So be conservative with the code paths that we're not fully 
> > moving
> > out of the KERNEL_LOCK() to allow us to concentrate on one area at a 
> > time.
> > ...
> > 
> > 
> > So here's a first small step.  I've been running with this for months
> > on a few amd64, arm64 and sparc64 boxes without problems;   they've been
> > daily drivers and/or have been building releases and ports.
> > 
> > Feedback?  Objections?  OK?
> > 
> > 
> > Index: sys/kern/syscalls.master
> > ===
> > RCS file: /cvs/src/sys/kern/syscalls.master,v
> > retrieving revision 1.221
> > diff -u -p -r1.221 syscalls.master
> > --- sys/kern/syscalls.master23 Dec 2021 18:50:31 -  1.221
> > +++ sys/kern/syscalls.master31 Dec 2021 09:14:00 -
> > @@ -126,7 +126,7 @@
> > struct sigaction *osa); }
> > 47  STD NOLOCK  { gid_t sys_getgid(void); }
> > 48  STD NOLOCK  { int sys_sigprocmask(int how, sigset_t mask); }
> > -49 STD { void *sys_mmap(void *addr, size_t len, int prot, \
> > +49 STD NOLOCK  { void *sys_mmap(void *addr, size_t len, int prot, \
> > int flags, int fd, off_t pos); }
> > 50  STD { int sys_setlogin(const char *namebuf); }
> > #ifdef ACCOUNTING
> > 
> 



unlock mmap(2) for anonymous mappings

2021-12-31 Thread Klemens Nanni
Now that mpi has unlocked uvm's fault handler, we can unlock the mmap
syscall to handle MAP_ANON without the big lock.

sys_mmap() still protects the !MAP_ANON case, i.e. file based mappings,
with the KERNEL_LOCK() itself, which is why unlocking the syscall will
only change locking behaviour for anonymous mappings.

A previous to unlock file based mappings was reverted, see the following
from https://marc.info/?l=openbsd-tech=160155434212888=2 :

commit 38802bc07455f2a4f8cdde272850a5eab5dfa6c8
from: mpi 
date: Wed Oct  7 12:26:20 2020 UTC

Do not release the KERNEL_LOCK() when mmap(2)ing files.

Previous attempt to unlock amap & anon exposed a race in vnode reference
counting.  So be conservative with the code paths that we're not fully 
moving
out of the KERNEL_LOCK() to allow us to concentrate on one area at a 
time.
...


So here's a first small step.  I've been running with this for months
on a few amd64, arm64 and sparc64 boxes without problems;   they've been
daily drivers and/or have been building releases and ports.

Feedback?  Objections?  OK?


Index: sys/kern/syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.221
diff -u -p -r1.221 syscalls.master
--- sys/kern/syscalls.master23 Dec 2021 18:50:31 -  1.221
+++ sys/kern/syscalls.master31 Dec 2021 09:14:00 -
@@ -126,7 +126,7 @@
struct sigaction *osa); }
 47 STD NOLOCK  { gid_t sys_getgid(void); }
 48 STD NOLOCK  { int sys_sigprocmask(int how, sigset_t mask); }
-49 STD { void *sys_mmap(void *addr, size_t len, int prot, \
+49 STD NOLOCK  { void *sys_mmap(void *addr, size_t len, int prot, \
int flags, int fd, off_t pos); }
 50 STD { int sys_setlogin(const char *namebuf); }
 #ifdef ACCOUNTING



Re: sndiod: -F does not switch back to preferred device

2021-12-25 Thread Klemens Nanni
On Sat, Dec 25, 2021 at 04:45:11PM +0100, Alexandre Ratchov wrote:
> On Sat, Dec 25, 2021 at 02:51:08PM +0000, Klemens Nanni wrote:
> > 
> > either "devices that are" or "device that is", I'd say the latter.
> > 
> 
> commited with "device that is".
> 
> BTW, the example of the -F option description seems more appropriate
> for the new hot plugging section.

Agreed, OK kn.



Re: sndiod: -F does not switch back to preferred device

2021-12-25 Thread Klemens Nanni
On Sat, Dec 25, 2021 at 03:46:48PM +0100, Alexandre Ratchov wrote:
> On Sat, Dec 25, 2021 at 01:34:19PM +0000, Klemens Nanni wrote:
> > 
> > This reads OK kn, but I suspect others will object to the hotplugd(8)
> > reference.
> > 
> 
> here's a new one, with attach/detach replaced by plug/unplug and no
> ref to hotplug(8)

OK kn

> diff --git a/sndiod/sndiod.8 b/sndiod/sndiod.8
> index 1f95097..c31da66 100644
> --- a/sndiod/sndiod.8
> +++ b/sndiod/sndiod.8
> @@ -185,8 +185,10 @@ Only the signedness and the precision are mandatory.
>  Examples:
>  .Va u8 , s16le , s24le3 , s24le4lsb .
>  .It Fl F Ar device
> -Specify an alternate device to use.
> -If it doesn't work, the one given with the previous
> +Same as
> +.Fl f
> +except that if the device is disconnected,
> +the one given with the previous
>  .Fl f
>  or
>  .Fl F
> @@ -196,11 +198,6 @@ PCI device allows
>  .Nm
>  to use the USB one preferably when it's connected
>  and to fall back to the PCI one when it's disconnected.
> -Alternate devices may be switched with the
> -.Va server.device
> -control of the
> -.Xr sndioctl 1
> -utility.
>  .It Fl f Ar device
>  Add this
>  .Xr sndio 7
> @@ -528,6 +525,20 @@ behave normally, while streams connected to
>  wait for the MMC start signal and start synchronously.
>  Regardless of which device a stream is connected to,
>  its playback volume knob is exposed.
> +.Sh HOT PLUGGING
> +If devices specified with
> +.Fl F
> +are unavailable when needed or unplugged at runtime,
> +.Nm
> +will attempt to seamlessly fall back to the last device specified.
> +.Pp
> +.Nm
> +will not automatically switch to specified devices that is plugged at 
> runtime.

either "devices that are" or "device that is", I'd say the latter.

> +Instead,
> +.Xr sndioctl 1
> +must be used to change the
> +.Va server.device
> +control.
>  .Sh EXAMPLES
>  Start server using default parameters, creating an
>  additional sub-device for output to channels 2:3 only (rear speakers
> 



Re: sndiod: -F does not switch back to preferred device

2021-12-25 Thread Klemens Nanni
On Mon, Dec 20, 2021 at 10:11:25AM +0100, Alexandre Ratchov wrote:
> On Fri, Dec 17, 2021 at 07:11:41PM +0000, Klemens Nanni wrote:
> > 
> > So we've concluded that the hotplpugging framework needs work, fine.
> > 
> > I'd still like to improve sndiod(8) regarding its own behaviour.
> > 
> > How about the same diff modulo hotplug referencing/examples?
> > This helps me understand `-f' and `-F' usage better.
> > 
> > Feedback? Objections? OK?
> >
> > Index: sndiod.8
> > ===
> > RCS file: /cvs/src/usr.bin/sndiod/sndiod.8,v
> > retrieving revision 1.11
> > diff -u -p -r1.11 sndiod.8
> > --- sndiod.816 Jul 2021 15:05:58 -  1.11
> > +++ sndiod.817 Dec 2021 19:02:21 -
> > @@ -185,7 +185,7 @@ Only the signedness and the precision ar
> >  Examples:
> >  .Va u8 , s16le , s24le3 , s24le4lsb .
> >  .It Fl F Ar device
> > -Specify an alternate device to use.
> > +Specify a preferred device to use on startup.
> >  If it doesn't work, the one given with the last
> >  .Fl f
> >  or
> 
> Recently we dropped the "alternate device name" hack and now -F and -f
> are the same, except that -f may fail, while -F tries the next
> device. AFAICS, -F will disappear completely soon.

Having just one flag in the future sounds good.

> What about just saying it's the same as -f?
> 
> (BTW, the paragraph about USB may be moved to the new "HOT PLUGGING"
> section, if we go this way)
> 
> > @@ -528,6 +528,24 @@ behave normally, while streams connected
> >  wait for the MMC start signal and start synchronously.
> >  Regardless of which device a stream is connected to,
> >  its playback volume knob is exposed.
> > +.Sh HOT PLUGGING
> > +.Nm
> > +If preferred devices specified with
> > +.Fl F
> > +are unavailable at startup or detach at runtime,
> > +.Nm
> > +will attempt to seamlessly fall back to the last device specified.
> > +.Pp
> > +.Nm
> > +will not automatically switch to specified devices that attach at runtime.
> > +Instead,
> > +.Xr sndioctl 1
> > +must be used to change the
> > +.Va server.device
> > +control.
> > +.Pp
> > +.Xr hotplugd 8
> > +can be used to implement seamless switching at runtime.
> >  .Sh EXAMPLES
> >  Start server using default parameters, creating an
> >  additional sub-device for output to channels 2:3 only (rear speakers
> > 
> > 
> 
> "At startup" suggests this is when the daemon starts, imho "When a
> device is needed" is more accurate (devices are kept closed when not
> used, so when a device is needed again, iteration over -Ff restarts).
> 
> With these tweaks, I end-up with the diff below.
> 
> As this is a the "hot plugging" section, would it make sense to use
> plugged/unplugged instead of attach/detach words?

Both works for me, whatever you prefer.

> diff --git a/sndiod/sndiod.8 b/sndiod/sndiod.8
> index 910ce12..7dd72e5 100644
> --- a/sndiod/sndiod.8
> +++ b/sndiod/sndiod.8
> @@ -185,8 +185,10 @@ Only the signedness and the precision are mandatory.
>  Examples:
>  .Va u8 , s16le , s24le3 , s24le4lsb .
>  .It Fl F Ar device
> -Specify an alternate device to use.
> -If it doesn't work, the one given with the previous
> +Same as
> +.Fl f
> +except that if the device is disconnected,
> +the one given with the previous
>  .Fl f
>  or
>  .Fl F
> @@ -196,11 +198,6 @@ PCI device allows
>  .Nm
>  to use the USB one preferably when it's connected
>  and to fall back to the PCI one when it's disconnected.
> -Alternate devices may be switched with the
> -.Va server.device
> -control of the
> -.Xr sndioctl 1
> -utility.
>  .It Fl f Ar device
>  Add this
>  .Xr sndio 7
> @@ -528,6 +525,23 @@ behave normally, while streams connected to
>  wait for the MMC start signal and start synchronously.
>  Regardless of which device a stream is connected to,
>  its playback volume knob is exposed.
> +.Sh HOT PLUGGING
> +If devices specified with
> +.Fl F
> +are unavailable when needed or detach at runtime,
> +.Nm
> +will attempt to seamlessly fall back to the last device specified.
> +.Pp
> +.Nm
> +will not automatically switch to specified devices that attach at runtime.
> +Instead,
> +.Xr sndioctl 1
> +must be used to change the
> +.Va server.device
> +control.
> +.Pp
> +.Xr hotplugd 8
> +can be used to implement seamless switching at runtime.
>  .Sh EXAMPLES
>  Start server using default parameters, creating an
>  additional sub-device for output to channels 2:3 only (rear speakers

This reads OK kn, but I suspect others will object to the hotplugd(8)
reference.

Thanks for following up on this.



Re: ip_deliver without kernel lock

2021-12-24 Thread Klemens Nanni
On Fri, Dec 24, 2021 at 11:17:46PM +0100, Alexander Bluhm wrote:
> ip_deliver() has been called without kernel lock from ip_ours() and
> ip6_ours() for a long time.  It looks like these two callers in ip6
> input were forgotten to be unlocked.

OK kn



Re: : provide ElfW() macro

2021-12-18 Thread Klemens Nanni
On Sat, Dec 18, 2021 at 04:37:17PM +0300, Andrew Krasavin wrote:
> On Fri, Dec 17, 2021 at 01:04:35PM +0000, Klemens Nanni wrote:
> > An upcoming port uses ElfW().  I first patched the port, but looking
> > around shows that both NetBSD and FreeBSD adopted the macro:
> > 
> > https://mail-index.netbsd.org/source-changes-hg/2018/07/31/msg027523.html
> > +#defineElfW(x) CONCAT(Elf,CONCAT(ELFSIZE,CONCAT(_,x)))
> > 
> > https://cgit.freebsd.org/src/commit/sys/sys/elf_generic.h?id=34e7e4b6a059eee5e4e3e34de5b9d5f0d6e589f9
> > +/* Define ElfW for compatibility with Linux, prefer __ElfN() in FreeBSD 
> > code */
> > +#defineElfW(x) __ElfN(x)
> > 
> > So I suggest providing it as well.
> > 
> > I have yet to put this through a release build on amd64 and/or sparc64
> > to check if there is any fallout, but I don't suspect there will be.
> > 
> > Feedback? Objections? OK (given the build succeeds)?
> > 
> > 
> > Index: sys/sys/exec_elf.h
> > ===
> > RCS file: /cvs/src/sys/sys/exec_elf.h,v
> > retrieving revision 1.93
> > diff -u -p -r1.93 exec_elf.h
> > --- sys/sys/exec_elf.h  7 Dec 2021 22:17:03 -   1.93
> > +++ sys/sys/exec_elf.h  17 Dec 2021 12:36:39 -
> > @@ -725,6 +725,7 @@ struct elf_args {
> > #define CONCAT(x,y) __CONCAT(x,y)
> > #define ELFNAME(x)  CONCAT(elf,CONCAT(ELFSIZE,CONCAT(_,x)))
> > #define ELFDEFNNAME(x)  CONCAT(ELF,CONCAT(ELFSIZE,CONCAT(_,x)))
> > +#define ElfW(x)CONCAT(Elf,CONCAT(ELFSIZE,CONCAT(_,x)))
> > #endif
> > 
> > #if defined(ELFSIZE) && (ELFSIZE == 32)
> > 
> 
> The code that requires this macro in sys/sys/exec_elf.h was
> eventually removed from the upcoming port (devel/abseil-cpp).
> 
> I have no opinion on what you should do with this patch now, but
> I feel it is necessary to report that this change is no longer
> needed for my port.

Even better!

Unless there are objections, I'll make sure that this doesn't break
anything and then commit it, so that future ports that do require it
will build -- there is no downside to providing it, imho.

> 
> -- 
> Wbr, Andrew Krasavin
> 



Re: sndiod: -F does not switch back to preferred device

2021-12-17 Thread Klemens Nanni
On Thu, Dec 09, 2021 at 11:29:04PM +, Klemens Nanni wrote:
> On Thu, Dec 09, 2021 at 10:21:56AM +0100, Alexandre Ratchov wrote:
> > On Wed, Dec 08, 2021 at 10:30:08PM +, Klemens Nanni wrote:
> > > Following https://www.openbsd.org/faq/faq13.html#usbaudio and reading
> > > sndiod(8)'s
> > > 
> > >   -F device
> > >   Specify an alternate device to use.  If it doesn't work, the one
> > >   given with the last -f or -F options will be used.  For 
> > > instance,
> > >   specifying a USB device following a PCI device allows sndiod to
> > >   use the USB one preferably when it's connected and to fall back
> > >   to the PCI one when it's disconnected.  Alternate devices may be
> > >   switched with the server.device control of the sndioctl(1)
> > >   utility.
> > > 
> > > I configured things as follows in order to play audio via USB and
> > > fall back to internal sound if USB is not available:
> > > 
> > >   $ rcctl get sndiod flags
> > >   -f rsnd/0 -F rsnd/1
> > > 
> > > Plugging in an USB headset and restarting sndiod or forcing the device
> > > with `sndioctl server.device=1' then plays sound via USB.
> > > 
> > > Unplugging the device makes playback fall back to internal sound.
> > > 
> > > But plugging USB back in does not prefer USB to internal as I'd expect
> > > now. I am currently resorting to the following hotplugd(8) script to
> > > always select the USB sound device whenever I plug it in:
> > > 
> > >   #!/bin/ksh
> > >   set -Cefu -o pipefail
> > > 
> > >   readonly DEVCLASS=$1 DEVNAME=$2
> > >   typeset -i devid
> > > 
> > >   case "${DEVCLASS}-${DEVNAME}" in
> > >   0-audio*)   # switch sndio(4) to USB headset when plugging it in
> > >   devid=${DEVNAME#audio}
> > >   sndioctl server.device=${devid}
> > >   ;;
> > >   esac
> > > 
> > 
> > IFAIU, audio devices always match the audio[0-9] pattern, so testing
> > the device class is not necessary, is it? FWIW, I use a similar
> > script.
> 
> I do it that way because /etc/hotplug/attach handles *all* events, so
> extending it with other cases where DEVCLASS matters would be obvious
> and audio(4) does not have to be treated specially.
> 
> > > I'd expect sndiod to *always* use USB whenever possible.
> > > 
> > > Is this uni-directional behaviour of sndiod intentional/by-design?
> > 
> > sndiod doesn't use directly hotplug(4), so it can't obtain by itself a
> > notification when a new device is plugged in order to switch to
> > it. Using hotplugd(8) is the right way to make audio hotplug work,
> > both daemons complete each other.
> 
> Alright, hotplugd it is, then.
> 
> > When sndiod needs the audio device (i.e. when the first client
> > connects), it will try all alternate devices (reverse -F options
> > order, the USB headset is first in your case). This gives a false
> > impression of hotplug support (indeed, you plug the USB headset, start
> > an audio player and it just works!). But that's just a side effect of
> > device priority/fail-over: if the internal device is already open when
> > you plug the USB headset, sndiod won't switch to it until it needs to
> > reopen the device.
> > 
> > Certain programs (including browsers), tend to keep the device open
> > even when they remain silent. So when actual playback starts sndiod
> > doesn't need to open a device and doesn't "see" the new USB headset. I
> > guess that's what happens on your system, but hotplugd(8) handles
> > this.
> > 
> > When devices are unplugged, we don't need hotplug because the device
> > stops working (input/output error) and sndiod switch to the next one
> > of the fail-over list.
> > 
> > > If so, can we clarify the manual?
> > 
> > Sure. While -F option description seems exact, maybe we need an extra
> > paragraph or FAQ entry to explain how to use it with other tools like
> > hotplugd and sndioctl. What about this wording?
> 
> Exact, but it could be clearer about the uni-directional fallack
> semantic, which is only relevant upon startup.  Then I wouldn't think
> it switches back and forth at runtime.
> 
> > HOT-PLUG SUPPORT
> >  If a device is unplugged while in use, sndiod will attempt to switch to
> >  one of the alternate devices (-F), if any.  This is seamless to 
> 

: provide ElfW() macro

2021-12-17 Thread Klemens Nanni
An upcoming port uses ElfW().  I first patched the port, but looking
around shows that both NetBSD and FreeBSD adopted the macro:

https://mail-index.netbsd.org/source-changes-hg/2018/07/31/msg027523.html
+#defineElfW(x) CONCAT(Elf,CONCAT(ELFSIZE,CONCAT(_,x)))

https://cgit.freebsd.org/src/commit/sys/sys/elf_generic.h?id=34e7e4b6a059eee5e4e3e34de5b9d5f0d6e589f9
+/* Define ElfW for compatibility with Linux, prefer __ElfN() in FreeBSD code */
+#defineElfW(x) __ElfN(x)

So I suggest providing it as well.

I have yet to put this through a release build on amd64 and/or sparc64
to check if there is any fallout, but I don't suspect there will be.

Feedback? Objections? OK (given the build succeeds)?


Index: sys/sys/exec_elf.h
===
RCS file: /cvs/src/sys/sys/exec_elf.h,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.h
--- sys/sys/exec_elf.h  7 Dec 2021 22:17:03 -   1.93
+++ sys/sys/exec_elf.h  17 Dec 2021 12:36:39 -
@@ -725,6 +725,7 @@ struct elf_args {
 #define CONCAT(x,y)__CONCAT(x,y)
 #define ELFNAME(x) CONCAT(elf,CONCAT(ELFSIZE,CONCAT(_,x)))
 #define ELFDEFNNAME(x) CONCAT(ELF,CONCAT(ELFSIZE,CONCAT(_,x)))
+#define ElfW(x)CONCAT(Elf,CONCAT(ELFSIZE,CONCAT(_,x)))
 #endif
 
 #if defined(ELFSIZE) && (ELFSIZE == 32)



Re: sndiod: -F does not switch back to preferred device

2021-12-09 Thread Klemens Nanni
On Thu, Dec 09, 2021 at 10:21:56AM +0100, Alexandre Ratchov wrote:
> On Wed, Dec 08, 2021 at 10:30:08PM +0000, Klemens Nanni wrote:
> > Following https://www.openbsd.org/faq/faq13.html#usbaudio and reading
> > sndiod(8)'s
> > 
> > -F device
> > Specify an alternate device to use.  If it doesn't work, the one
> > given with the last -f or -F options will be used.  For 
> > instance,
> > specifying a USB device following a PCI device allows sndiod to
> > use the USB one preferably when it's connected and to fall back
> > to the PCI one when it's disconnected.  Alternate devices may be
> > switched with the server.device control of the sndioctl(1)
> > utility.
> > 
> > I configured things as follows in order to play audio via USB and
> > fall back to internal sound if USB is not available:
> > 
> > $ rcctl get sndiod flags
> > -f rsnd/0 -F rsnd/1
> > 
> > Plugging in an USB headset and restarting sndiod or forcing the device
> > with `sndioctl server.device=1' then plays sound via USB.
> > 
> > Unplugging the device makes playback fall back to internal sound.
> > 
> > But plugging USB back in does not prefer USB to internal as I'd expect
> > now. I am currently resorting to the following hotplugd(8) script to
> > always select the USB sound device whenever I plug it in:
> > 
> > #!/bin/ksh
> > set -Cefu -o pipefail
> > 
> > readonly DEVCLASS=$1 DEVNAME=$2
> > typeset -i devid
> > 
> > case "${DEVCLASS}-${DEVNAME}" in
> > 0-audio*)   # switch sndio(4) to USB headset when plugging it in
> > devid=${DEVNAME#audio}
> > sndioctl server.device=${devid}
> > ;;
> > esac
> > 
> 
> IFAIU, audio devices always match the audio[0-9] pattern, so testing
> the device class is not necessary, is it? FWIW, I use a similar
> script.

I do it that way because /etc/hotplug/attach handles *all* events, so
extending it with other cases where DEVCLASS matters would be obvious
and audio(4) does not have to be treated specially.

> > I'd expect sndiod to *always* use USB whenever possible.
> > 
> > Is this uni-directional behaviour of sndiod intentional/by-design?
> 
> sndiod doesn't use directly hotplug(4), so it can't obtain by itself a
> notification when a new device is plugged in order to switch to
> it. Using hotplugd(8) is the right way to make audio hotplug work,
> both daemons complete each other.

Alright, hotplugd it is, then.

> When sndiod needs the audio device (i.e. when the first client
> connects), it will try all alternate devices (reverse -F options
> order, the USB headset is first in your case). This gives a false
> impression of hotplug support (indeed, you plug the USB headset, start
> an audio player and it just works!). But that's just a side effect of
> device priority/fail-over: if the internal device is already open when
> you plug the USB headset, sndiod won't switch to it until it needs to
> reopen the device.
> 
> Certain programs (including browsers), tend to keep the device open
> even when they remain silent. So when actual playback starts sndiod
> doesn't need to open a device and doesn't "see" the new USB headset. I
> guess that's what happens on your system, but hotplugd(8) handles
> this.
> 
> When devices are unplugged, we don't need hotplug because the device
> stops working (input/output error) and sndiod switch to the next one
> of the fail-over list.
> 
> > If so, can we clarify the manual?
> 
> Sure. While -F option description seems exact, maybe we need an extra
> paragraph or FAQ entry to explain how to use it with other tools like
> hotplugd and sndioctl. What about this wording?

Exact, but it could be clearer about the uni-directional fallack
semantic, which is only relevant upon startup.  Then I wouldn't think
it switches back and forth at runtime.

> HOT-PLUG SUPPORT
>  If a device is unplugged while in use, sndiod will attempt to switch to
>  one of the alternate devices (-F), if any.  This is seamless to programs
>  connected to sndiod.
> 
>  Later, when the device is connected again, the server.device control of
>  the sndioctl(1) utility could be used to switch back to it, without the
>  need to restart all audio programs.  This last step could be automated
>  using hotplugd(8).  For instance, if sndiod is started with:
> 
>$ sndiod -f rsnd/0 -F rsnd/1 -F rsnd/2 -F rsnd/3
> 
>  then, the following hotplugd(8) attach script could be used to
>  auto

sndiod: -F does not switch back to preferred device

2021-12-08 Thread Klemens Nanni
Following https://www.openbsd.org/faq/faq13.html#usbaudio and reading
sndiod(8)'s

-F device
Specify an alternate device to use.  If it doesn't work, the one
given with the last -f or -F options will be used.  For 
instance,
specifying a USB device following a PCI device allows sndiod to
use the USB one preferably when it's connected and to fall back
to the PCI one when it's disconnected.  Alternate devices may be
switched with the server.device control of the sndioctl(1)
utility.

I configured things as follows in order to play audio via USB and
fall back to internal sound if USB is not available:

$ rcctl get sndiod flags
-f rsnd/0 -F rsnd/1

Plugging in an USB headset and restarting sndiod or forcing the device
with `sndioctl server.device=1' then plays sound via USB.

Unplugging the device makes playback fall back to internal sound.

But plugging USB back in does not prefer USB to internal as I'd expect
now.  I am currently resorting to the following hotplugd(8) script to
always select the USB sound device whenever I plug it in:

#!/bin/ksh
set -Cefu -o pipefail

readonly DEVCLASS=$1 DEVNAME=$2
typeset -i devid

case "${DEVCLASS}-${DEVNAME}" in
0-audio*)   # switch sndio(4) to USB headset when plugging it in
devid=${DEVNAME#audio}
sndioctl server.device=${devid}
;;
esac

I'd expect sndiod to *always* use USB whenever possible.

Is this uni-directional behaviour of sndiod intentional/by-design?
If so, can we clarify the manual?



Re: netstart: create virtual interfaces upfront when passing specific ones

2021-12-07 Thread Klemens Nanni
On Tue, Nov 23, 2021 at 01:17:14AM +, Klemens Nanni wrote:
> On Tue, Nov 16, 2021 at 11:09:40PM +0000, Klemens Nanni wrote:
> > Run on boot without arguments, netstart(8) creates all virtual
> > interfaces *for which hostname.if files exist* before configuring them.
> > 
> > This prevents ordering problems with bridges and its members, as dlg's
> > commit message from 2018 reminds us.
> > 
> > But it also helps interface types like pair(4) which pair one another
> > in whatever way the user says:
> > 
> > $ cat /etc/hostname.pair1
> > patch pair2
> > $ cat /etc/hostname.pair2
> > rdomain 1
> > 
> > On boot this works, but `sh /etc/netstart pair1 pair2' won't work
> > because pair2 does not exist a creation time of pair1 because netstart
> > does not create virtual interfaces upfront.
> > 
> > I just hit this exact use case when setting up gelatod(8) (see ports@).
> > 
> > To fix this, pass the list of interfaces to vifscreate() and make it
> > create only those iff given.
> > 
> > Regular boot, i.e. `sh /etc/netstart', stays uneffected by this and
> > selective runs as shown work as expected without requring users to know
> > the order in which netstart creates/configures interfaces.
> > 
> > The installer's internal version of netstart doesn't need this at all;
> > neither does it have the selective semantic nor does vifscreate() exist.
> 
> Anyone?
> 
> It seems only logical to treat subsets of interfaces the same way as
> a full `sh /etc/netstart'.
> 
> A pair of pair(4) is one example, I'm certain there are more scenarios
> where you craft interfaces with `ifconfig ...' in the shell, then set up
> the hostname.* files and test them with `sh /etc/netstart bridge0 ...'
> where pseudo interfaces are involved.

Anyone?

This is really practical and fixes things at least for me when I destroy
interfaces, reconfigure and recreate them together, for example like so:

# ifconfig pair2 destroy
# ifconfig pair1 destroy
... edit hostname.*
# sh /etc/netstart pair1 pair2
ifconfig: patch pair2: No such file or directory
add net default: gateway 192.0.0.1

(redoing it because who knows what failed due to the order problem and
what didn't...)

# ifconfig pair2 destroy
# ifconfig pair1 destroy
# sh /usr/src/etc/netstart pair1 pair2
add net default: gateway 192.0.0.1

Feedback? Objection? OK?

Index: netstart
===
RCS file: /cvs/src/etc/netstart,v
retrieving revision 1.216
diff -u -p -r1.216 netstart
--- netstart2 Sep 2021 19:38:20 -   1.216
+++ netstart7 Dec 2021 20:08:16 -
@@ -94,9 +94,11 @@ ifcreate() {
 }
 
 # Create interfaces for network pseudo-devices referred to by hostname.if 
files.
-# Usage: vifscreate
+# Optionally, limit creation to given interfaces only.
+# Usage: vifscreate [if ...]
 vifscreate() {
-   local _vif _hn _if
+   local _vif _hn _if _ifs
+   set -A _ifs -- "$@"
 
for _vif in $(ifconfig -C); do
for _hn in /etc/hostname.${_vif}+([[:digit:]]); do
@@ -106,6 +108,9 @@ vifscreate() {
# loopback for routing domain is created by kernel
[[ -n ${_if##lo[1-9]*} ]] || continue
 
+   ((${#_ifs[*]} > 0)) && [[ ${_ifs[*]} != *${_if}* ]] &&
+   continue
+
if ! ifcreate $_if; then
print -u2 "${0##*/}: create for '$_if' failed."
fi
@@ -303,6 +308,7 @@ $PRINT_ONLY || [[ ! -f /etc/soii.key ]] 
 # If we were invoked with a list of interface names, just reconfigure these
 # interfaces (or bridges), add default routes and return.
 if (($# > 0)); then
+   vifscreate "$@"
for _if; do ifstart $_if; done
defaultroute
return



Re: Please test: UVM fault unlocking (aka vmobjlock)

2021-12-02 Thread Klemens Nanni
On Mon, Nov 29, 2021 at 10:50:32PM +0100, Martin Pieuchot wrote:
> On 24/11/21(Wed) 11:16, Martin Pieuchot wrote:
> > Diff below unlock the bottom part of the UVM fault handler.  I'm
> > interested in squashing the remaining bugs.  Please test with your usual
> > setup & report back.
> 
> Thanks to all the testers, here's a new version that includes a bug fix.
> 
> Tests on !x86 architectures are much appreciated!

GENERIC.MP build becomes noticably faster on a sparc64 T4-2 LDOM:


This diff survived a full regress as well as all kernel builds.
I've run regress and builds in parallel each, i.e. one `make' per
regress/ and sparc64/compile/ subdirectory.



Re: ipsec useless inner header

2021-12-02 Thread Klemens Nanni
On Thu, Dec 02, 2021 at 01:24:36AM +0100, Alexander Bluhm wrote:
> ipsec_common_input_cb() extracts the inner IP header of IPsec
> tunnels.  It is never used, so this is useless code.

OK kn



hotplugd: do not log device de/attach

2021-12-01 Thread Klemens Nanni
Just enabled hotplugd(8) to handle a single device I'm interested in and
saw it basically duplicating kernel de/attach messages in
/var/log/daemon:

Dec  1 23:36:27 eru hotplugd[26475]: started
Dec  1 23:36:27 eru hotplugd[26475]: uhidev4 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: wskbd3 attached, class 5
Dec  1 23:36:27 eru hotplugd[26475]: ucc0 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: uhid0 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: uhidev5 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: ugen1 detached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: sd2 attached, class 2
Dec  1 23:36:27 eru hotplugd[26475]: scsibus4 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: umass0 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: audio1 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: uaudio0 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: wskbd4 attached, class 5
Dec  1 23:36:27 eru hotplugd[26475]: ucc1 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: uhid1 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: uhid2 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: uhid3 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: uhid4 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: uhid5 attached, class 0
Dec  1 23:36:27 eru hotplugd[26475]: uhid6 attached, class 0

That's really what dmesg and/or /var/log/messages already tell me.
If I'm really interested in the class id (which is pretty obvious in
most -if not all- cases), I can add a logger(1) line to the hotplugd
`attach'/`detach' scripts and print my own info.

I consider this noise/spam, especially as default behaviour.
There is no option for verbose/quiet operation, but I also don't think
it is warranted.

Remove the unhelpful logging and keep the noise down.

Feedback? Objections? OK?


Index: hotplugd.c
===
RCS file: /cvs/src/usr.sbin/hotplugd/hotplugd.c,v
retrieving revision 1.17
diff -u -p -r1.17 hotplugd.c
--- hotplugd.c  12 Jul 2021 15:09:21 -  1.17
+++ hotplugd.c  1 Dec 2021 22:41:30 -
@@ -120,14 +120,10 @@ main(int argc, char *argv[])
 
switch (he.he_type) {
case HOTPLUG_DEVAT:
-   syslog(LOG_INFO, "%s attached, class %d",
-   he.he_devname, he.he_devclass);
exec_script(_PATH_ETC_HOTPLUG_ATTACH, he.he_devclass,
he.he_devname);
break;
case HOTPLUG_DEVDT:
-   syslog(LOG_INFO, "%s detached, class %d",
-   he.he_devname, he.he_devclass);
exec_script(_PATH_ETC_HOTPLUG_DETACH, he.he_devclass,
he.he_devname);
break;



Re: sppp(4)/pppoe(4) - avoid endless loop in remote ip negotiation

2021-12-01 Thread Klemens Nanni
On Fri, Nov 26, 2021 at 01:35:14PM +0100, Krzysztof Kanas wrote:
> Hi. When remote side in sppp doesn't reply for to PPP IPCP IP-Address 
> sppp will try to negotiate remote IP in endless loop. Instead use 
> 10.64.64.1 + if_index as remote IP.

Why add some arbitrary RFC 1918 IP if negotiation fails?  That's not
what users request or expect.

> While at it maybe it's worth to add that SPP is in RFC 1332 ?

sppp(4) STANDARDS mentions RFC 1332, if that's what you mean.

> Krzysztof Kanas
> 
> Index: share/man/man4/sppp.4
> ===
> RCS file: /home/cvs//src/share/man/man4/sppp.4,v
> retrieving revision 1.26
> diff -r1.26 sppp.4
> 284,285c284,285
> < Negotiation loop avoidance is not fully implemented.
> < If the negotiation doesn't converge, this can cause an endless loop.

Please send unified diffs, i.e. use `-u' with CVS diff(1).

> ---
> > In case when remote IP can't be negotiation after 10 retries pick 
> > 10.64.64.1 + if_index.
> Index: sys/net/if_sppp.h
> ===
> RCS file: /home/cvs//src/sys/net/if_sppp.h,v
> retrieving revision 1.30
> diff -r1.30 if_sppp.h
> 148a149,150
> > #define IPCP_HISADDR_COUNTER_MAX  10
> > u_int8_t hisaddr_counter;/* number of ipcp req for peer addr */
> Index: sys/net/if_spppsubr.c
> ===
> RCS file: /home/cvs//src/sys/net/if_spppsubr.c,v
> retrieving revision 1.190
> diff -r1.190 if_spppsubr.c
> 2466,2468c2466,2469
> <  * XXX This can result in an endless req - nak loop if peer
> <  * doesn't want to send us his address.  Q: What should we do
> <  * about it?  XXX  A: implement the max-failure counter.
> ---
> >  * This can result in an endless req - nak loop if peer
> >  * doesn't want to send us his address. Therefore we count
> >  * the number of request if it exceeds IPCP_HISADDR_COUNTER_MAX
> >  * assign remote address 10.64.64.1 + if_index.
> 2471,2479c2472,2492
> < buf[0] = IPCP_OPT_ADDRESS;
> < buf[1] = 6;
> < buf[2] = hisaddr >> 24;
> < buf[3] = hisaddr >> 16;
> < buf[4] = hisaddr >> 8;
> < buf[5] = hisaddr;
> < rlen = 6;
> < if (debug)
> < addlog("still need hisaddr ");
> ---
> > if (sp->ipcp.hisaddr_counter++ > IPCP_HISADDR_COUNTER_MAX) {
> > sp->ipcp.hisaddr_counter = 0;
> > desiredaddr = 10 << 24 | 64 << 16 | 64 << 8 | 1;
> > desiredaddr += sp->pp_if.if_index;
> > hisaddr = desiredaddr;
> > sp->ipcp.req_hisaddr = desiredaddr;
> > sp->ipcp.flags |= IPCP_HISADDR_SEEN;
> > if (debug)
> > addlog("%s guess ",
> > sppp_dotted_quad(desiredaddr));
> > } else {
> > buf[0] = IPCP_OPT_ADDRESS;
> > buf[1] = 6;
> > buf[2] = hisaddr >> 24;
> > buf[3] = hisaddr >> 16;
> > buf[4] = hisaddr >> 8;
> > buf[5] = hisaddr;
> > rlen = 6;
> > if (debug)
> > addlog("still need hisaddr ");
> > }
> 



Re: slaacd(8): prevent crash when interface disappears

2021-11-28 Thread Klemens Nanni
On Thu, Nov 18, 2021 at 09:02:00AM +0100, Florian Obser wrote:
> This is split in two for easier review and I also intend to commit it
> like this.
> 
> The first diff shuffles setting of if_index around so that it's
> available in all switch cases and uses it consistently instead of
> ifm->ifm_index.
> 
> That way we can copy the if_name == NULL check into each case block
> preventing crashes when an interface disappears at just the wrong
> moment.
> 
> OK?
> 
> (btw. I found this because I transmogrified slaacd into gelatod and kn@
> reported a crash while running in debug mode so I could spot what's
> wrong with it. Much harder to find in slaacd...)
> 
> commit 2880598cb424e5d889ecdbf06d9d72d777b11569
> Author: Florian Obser 
> Date:   Wed Nov 17 20:13:55 2021 +0100
> 
> normalize if_index handling in route messages

OK kn

> commit e8882f2329db1789914358c1c6b56ddec74fc35f
> Author: Florian Obser 
> Date:   Wed Nov 17 20:17:29 2021 +0100
> 
> Make sure the interface still exists before updating it.
> 
> When we get a route message, for example an address being added
> (RTM_NEWADDR, but the problem exists with most of the route messages)
> and the interface gets unplugged at just the right moment
> if_nametoindex(3) will return NULL. We will pass NULL through
> update_iface() to get_xflags() which will then crash because we
> dereference the NULL pointer there.

OK kn

> This might be the crash kn@ was seeing once in a blue moon.

I somewhat doubt it, since slaacd crashed on my notebook using trunk(4)
over em(4) and athn(4), none of these interface get destroyed.

I toggle the autoconf, set IPs, etc. but once trunk0 has its interface
index, it'll stay the same until reboot.



Re: vport: set UP on ip assign

2021-11-23 Thread Klemens Nanni
On Wed, Nov 24, 2021 at 02:30:08AM +0100, Klemens Nanni wrote:
> On Tue, Nov 16, 2021 at 09:22:26AM +1000, David Gwynne wrote:
> > On Mon, Nov 15, 2021 at 02:31:42PM +, Klemens Nanni wrote:
> > > On Mon, Nov 15, 2021 at 01:37:49PM +, Stuart Henderson wrote:
> > > > On 2021/11/15 12:27, Klemens Nanni wrote:
> > > > > On Sun, Nov 14, 2021 at 07:04:42PM -0700, Theo de Raadt wrote:
> > > > > > I think physical interfaces should come up when something is 
> > > > > > configured
> > > > > > on them, but virtual interfaces shouldn't -- mostly because the 
> > > > > > order of
> > > > > > configuration is often muddled.
> > > > > 
> > > > > So "inet6 2001:db8::1" in hostname.em0 will do the trick but
> > > > > hostname.vport0 would need another "up" for the same behaviour:  
> > > > > that's
> > > > > rather confusing me as a user.
> > > > 
> > > > hostname.* files are orthogonal to this; netstart can process all the 
> > > > lines,
> > > > then if it has seen a line doing address configuration and has not seen 
> > > > an
> > > > explicit "down", it can bring the interface up automatically at the end.
> > > > (if this changed, it would be a nightmare for users to do anything 
> > > > else).
> > > 
> > > Yes, netstart can and should deal with this correctly, just like you
> > > describe.
> > > 
> > > > Users would need to make sure they have a netstart which does that if
> > > > updating a kernel, but that's just a case of matching kernel+userland 
> > > > and is
> > > > nothing new for OpenBSD.
> > > > 
> > > > The different behaviour would be apparent with separate runs of 
> > > > ifconfig.
> > > > some scripts may need adapting and users might need to run "ifconfig XX 
> > > > up"
> > > > themselves but I don't think that would be a problem.
> > > 
> > > Agreed.
> > > 
> > > Having the implicit-up logic entirely contained in netstart would make
> > > lifer much easier, both for network stack hackers and users, imho.
> > 
> > this was my attempt at just that.
> 
> Given netstart(8) always pulls interfaces UP or DOWN as the last step,
> surely this wouldn't be all, right?

Put differently:  Such netstart change really just tries to abstract a
consistent behaviour across multiple interfaces doing it differently.

> Interfaces/drivers still pull themselves up, thus create route message
> churn, transition from initial DOWN to implicit UP due to address
> configuration to possibly administrative DOWN again due to "down" in
> hostname.*, etc.

So once/iff this change lands, I think all interfaces should do (or not
do) what netstart compensates for.

Once all interfaces avoid implicit state changes and only transition
upon administrative command (and possibly `autoconf'), the netstart
workaround --that's what it really is-- ought to be removed.

> If we decide to handle this in netstart alone, shouldn't all interfaces
> behave like vport(4) and not mess with their state unless explicitly
> requested to do so?

Then, finally, interfaces only go UP if users do `ifconfig ... up'
or hostname.* contain the word "up".  Otherwise they stay DOWN.

This would be a dead simple thing to reason.

The netstart workaround itself already changes behaviour and tells users
to add a very specific line to their configuration -- a format which
really shouldn't have such specific requirements but instead should work
like regular `ifconfig' lines on the shell, btw.

So If that already needs attention, the final solution of no interface
doing implicit changes and netstart not doing any implicit stuff would
also eventually result in the simplest and most intuitive form of
configuration:  "up" anywhere pulls UP, "down" anywhere pulls DOWN,
neither anywhere leaves interfaces at their creation default of DOWN.

> Not sure if the (now finally documented) implicit `up' in `autoconf'
> should stay after the netstart change, but that's another topic
> (pulling interfaces UP two times won't hurt, I guess).
> 
> > the installer has its own netstart though, right?
> 
> Yes, diff for that at the end after tweaking yours and adapting it.
> 
> > Index: etc/netstart
> > ===
> > RCS file: /cvs/src/etc/netstart,v
> > retrieving revision 1.216
> > diff -u -p -r1.216 netstart
> > --- etc/netstart2

Re: vport: set UP on ip assign

2021-11-23 Thread Klemens Nanni
On Tue, Nov 16, 2021 at 09:22:26AM +1000, David Gwynne wrote:
> On Mon, Nov 15, 2021 at 02:31:42PM +0000, Klemens Nanni wrote:
> > On Mon, Nov 15, 2021 at 01:37:49PM +, Stuart Henderson wrote:
> > > On 2021/11/15 12:27, Klemens Nanni wrote:
> > > > On Sun, Nov 14, 2021 at 07:04:42PM -0700, Theo de Raadt wrote:
> > > > > I think physical interfaces should come up when something is 
> > > > > configured
> > > > > on them, but virtual interfaces shouldn't -- mostly because the order 
> > > > > of
> > > > > configuration is often muddled.
> > > > 
> > > > So "inet6 2001:db8::1" in hostname.em0 will do the trick but
> > > > hostname.vport0 would need another "up" for the same behaviour:  that's
> > > > rather confusing me as a user.
> > > 
> > > hostname.* files are orthogonal to this; netstart can process all the 
> > > lines,
> > > then if it has seen a line doing address configuration and has not seen an
> > > explicit "down", it can bring the interface up automatically at the end.
> > > (if this changed, it would be a nightmare for users to do anything else).
> > 
> > Yes, netstart can and should deal with this correctly, just like you
> > describe.
> > 
> > > Users would need to make sure they have a netstart which does that if
> > > updating a kernel, but that's just a case of matching kernel+userland and 
> > > is
> > > nothing new for OpenBSD.
> > > 
> > > The different behaviour would be apparent with separate runs of ifconfig.
> > > some scripts may need adapting and users might need to run "ifconfig XX 
> > > up"
> > > themselves but I don't think that would be a problem.
> > 
> > Agreed.
> > 
> > Having the implicit-up logic entirely contained in netstart would make
> > lifer much easier, both for network stack hackers and users, imho.
> 
> this was my attempt at just that.

Given netstart(8) always pulls interfaces UP or DOWN as the last step,
surely this wouldn't be all, right?

Interfaces/drivers still pull themselves up, thus create route message
churn, transition from initial DOWN to implicit UP due to address
configuration to possibly administrative DOWN again due to "down" in
hostname.*, etc.

If we decide to handle this in netstart alone, shouldn't all interfaces
behave like vport(4) and not mess with their state unless explicitly
requested to do so?

Not sure if the (now finally documented) implicit `up' in `autoconf'
should stay after the netstart change, but that's another topic
(pulling interfaces UP two times won't hurt, I guess).

> the installer has its own netstart though, right?

Yes, diff for that at the end after tweaking yours and adapting it.

> Index: etc/netstart
> ===
> RCS file: /cvs/src/etc/netstart,v
> retrieving revision 1.216
> diff -u -p -r1.216 netstart
> --- etc/netstart  2 Sep 2021 19:38:20 -   1.216
> +++ etc/netstart  15 Nov 2021 23:20:00 -
> @@ -71,6 +71,9 @@ parse_hn_line() {
>   dhcp)   _cmds[${#_cmds[*]}]="ifconfig $_if inet autoconf"
>   V4_AUTOCONF=true
>   ;;
> + down)   _c[0]=

This reset seems unneeded, `_c' isn't used afterwards and I don't
undertand why you do it.

> + _ifup=down

So `_ifup' comes from ifstart() and not parse_hn_line().

We use the "_" prefix to denote local variables...

> + ;;
>   '!'*)   _cmd=$(print -- "${_c[@]}" | sed 's/\$if/'$_if'/g')
>   _cmds[${#_cmds[*]}]="${_cmd#!}"
>   ;;
> @@ -118,6 +121,7 @@ vifscreate() {
>  ifstart() {
>   local _if=$1 _hn=/etc/hostname.$1 _cmds _i=0 _line _stat
>   set -A _cmds
> + _ifup=up

except you define a global variable inside a function.

This should be local to ifstart() (deserving its prefix), which makes it
reach into the scope of parse_hn_line() (as is usual semantic with
`local' variables in at least ksh and bash).

I've added comments to reflect on this special use, as I'm unaware of
any other piece of shell code were we actively use function-local
variables up the call stack.

>  
>   # Interface names must be alphanumeric only.  We check to avoid
>   # configuring backup or temp files, and to catch the "*" case.
> @@ -145,6 +149,8 @@ ifstart() {
>   while IFS= read -- _line; do
>   parse_hn_line $_line
>   done <$_hn
> +
> + _cmds[${#_cmds[*]}]="ifconfig $_if $_ifup"

Lastly, I'd say `_ifstate' because that equally means "

Re: netstart: create virtual interfaces upfront when passing specific ones

2021-11-22 Thread Klemens Nanni
On Tue, Nov 16, 2021 at 11:09:40PM +, Klemens Nanni wrote:
> Run on boot without arguments, netstart(8) creates all virtual
> interfaces *for which hostname.if files exist* before configuring them.
> 
> This prevents ordering problems with bridges and its members, as dlg's
> commit message from 2018 reminds us.
> 
> But it also helps interface types like pair(4) which pair one another
> in whatever way the user says:
> 
>   $ cat /etc/hostname.pair1
>   patch pair2
>   $ cat /etc/hostname.pair2
>   rdomain 1
> 
> On boot this works, but `sh /etc/netstart pair1 pair2' won't work
> because pair2 does not exist a creation time of pair1 because netstart
> does not create virtual interfaces upfront.
> 
> I just hit this exact use case when setting up gelatod(8) (see ports@).
> 
> To fix this, pass the list of interfaces to vifscreate() and make it
> create only those iff given.
> 
> Regular boot, i.e. `sh /etc/netstart', stays uneffected by this and
> selective runs as shown work as expected without requring users to know
> the order in which netstart creates/configures interfaces.
> 
> The installer's internal version of netstart doesn't need this at all;
> neither does it have the selective semantic nor does vifscreate() exist.

Anyone?

It seems only logical to treat subsets of interfaces the same way as
a full `sh /etc/netstart'.

A pair of pair(4) is one example, I'm certain there are more scenarios
where you craft interfaces with `ifconfig ...' in the shell, then set up
the hostname.* files and test them with `sh /etc/netstart bridge0 ...'
where pseudo interfaces are involved.

Feedback? Objection? OK?


Index: netstart
===
RCS file: /cvs/src/etc/netstart,v
retrieving revision 1.216
diff -u -p -r1.216 netstart
--- netstart2 Sep 2021 19:38:20 -   1.216
+++ netstart16 Nov 2021 22:58:31 -
@@ -94,9 +94,11 @@ ifcreate() {
 }
 
 # Create interfaces for network pseudo-devices referred to by hostname.if 
files.
-# Usage: vifscreate
+# Optionally, limit creation to given interfaces only.
+# Usage: vifscreate [if ...]
 vifscreate() {
-   local _vif _hn _if
+   local _vif _hn _if _ifs
+   set -A _ifs -- "$@"
 
for _vif in $(ifconfig -C); do
for _hn in /etc/hostname.${_vif}+([[:digit:]]); do
@@ -106,6 +108,9 @@ vifscreate() {
# loopback for routing domain is created by kernel
[[ -n ${_if##lo[1-9]*} ]] || continue
 
+   ((${#_ifs[*]} > 0)) && [[ ${_ifs[*]} != *${_if}* ]] &&
+   continue
+
if ! ifcreate $_if; then
print -u2 "${0##*/}: create for '$_if' failed."
fi
@@ -303,6 +308,7 @@ $PRINT_ONLY || [[ ! -f /etc/soii.key ]] 
 # If we were invoked with a list of interface names, just reconfigure these
 # interfaces (or bridges), add default routes and return.
 if (($# > 0)); then
+   vifscreate "$@"
for _if; do ifstart $_if; done
defaultroute
return



Re: Print learned DNS from sppp(4) in ifconfig(8)

2021-11-17 Thread Klemens Nanni
On Wed, Nov 17, 2021 at 08:51:38AM +0100, Bjorn Ketelaars wrote:
> Like umb(4), sppp(4) natively learns DNS information. Among the
> differences between these two devices is that umb prints this
> information from ifconfig(8) and sppp does not. I would like to equalize
> this behaviour, and add the necessary bits to sppp(4) and ifconfig(8).
> 
> Diff below is largely based on a diff from Peter J. Philipp [0].
> 
> Comments/OK?
> 
> [0] https://marc.info/?l=openbsd-tech=159405677416423=2

Works for me, ifconfig bits don't effect SMALL build and output looks
like umb:

pppoe0: flags=8851 mtu 1492
description: 1und1 VDSL2
index 7 priority 0 llprio 3
dev: vlan7 state: session
sid: 0x47a6 PADI retries: 0 PADR retries: 0 time: 19d 05:13:56
sppp: phase network authproto pap authname "..."
dns: 82.145.9.8 82.144.41.8
groups: pppoe wan egress
status: active
inet6 fe80::618:d6ff:fea0:884e%pppoe0 -->  prefixlen 64 scopeid 
0x7
inet ... --> ... netmask 0x

OK kn



netstart: create virtual interfaces upfront when passing specific ones

2021-11-16 Thread Klemens Nanni
Run on boot without arguments, netstart(8) creates all virtual
interfaces *for which hostname.if files exist* before configuring them.

This prevents ordering problems with bridges and its members, as dlg's
commit message from 2018 reminds us.

But it also helps interface types like pair(4) which pair one another
in whatever way the user says:

$ cat /etc/hostname.pair1
patch pair2
$ cat /etc/hostname.pair2
rdomain 1

On boot this works, but `sh /etc/netstart pair1 pair2' won't work
because pair2 does not exist a creation time of pair1 because netstart
does not create virtual interfaces upfront.

I just hit this exact use case when setting up gelatod(8) (see ports@).

To fix this, pass the list of interfaces to vifscreate() and make it
create only those iff given.

Regular boot, i.e. `sh /etc/netstart', stays uneffected by this and
selective runs as shown work as expected without requring users to know
the order in which netstart creates/configures interfaces.

The installer's internal version of netstart doesn't need this at all;
neither does it have the selective semantic nor does vifscreate() exist.


Feedback? Objection? OK?

Index: netstart
===
RCS file: /cvs/src/etc/netstart,v
retrieving revision 1.216
diff -u -p -r1.216 netstart
--- netstart2 Sep 2021 19:38:20 -   1.216
+++ netstart16 Nov 2021 22:58:31 -
@@ -94,9 +94,11 @@ ifcreate() {
 }
 
 # Create interfaces for network pseudo-devices referred to by hostname.if 
files.
-# Usage: vifscreate
+# Optionally, limit creation to given interfaces only.
+# Usage: vifscreate [if ...]
 vifscreate() {
-   local _vif _hn _if
+   local _vif _hn _if _ifs
+   set -A _ifs -- "$@"
 
for _vif in $(ifconfig -C); do
for _hn in /etc/hostname.${_vif}+([[:digit:]]); do
@@ -106,6 +108,9 @@ vifscreate() {
# loopback for routing domain is created by kernel
[[ -n ${_if##lo[1-9]*} ]] || continue
 
+   ((${#_ifs[*]} > 0)) && [[ ${_ifs[*]} != *${_if}* ]] &&
+   continue
+
if ! ifcreate $_if; then
print -u2 "${0##*/}: create for '$_if' failed."
fi
@@ -303,6 +308,7 @@ $PRINT_ONLY || [[ ! -f /etc/soii.key ]] 
 # If we were invoked with a list of interface names, just reconfigure these
 # interfaces (or bridges), add default routes and return.
 if (($# > 0)); then
+   vifscreate "$@"
for _if; do ifstart $_if; done
defaultroute
return



Re: vport: set UP on ip assign

2021-11-15 Thread Klemens Nanni
On Mon, Nov 15, 2021 at 04:25:54PM -0700, Theo de Raadt wrote:
> > +   _cmds[${#_cmds[*]}]="ifconfig $_if $_ifup"
> 
> I will be surprised if you can simply add "up" to potential
> ifconfig commandlines.

This does not amend "ifconfig foo0 bar ..." to ifconfig foo0 bar ... up"
if that's what you mean.

It's a single "ifconfig foo0 up" at the end.
This does work in every hostname.* file.



Re: vport: set UP on ip assign

2021-11-15 Thread Klemens Nanni
On Mon, Nov 15, 2021 at 01:37:49PM +, Stuart Henderson wrote:
> On 2021/11/15 12:27, Klemens Nanni wrote:
> > On Sun, Nov 14, 2021 at 07:04:42PM -0700, Theo de Raadt wrote:
> > > I think physical interfaces should come up when something is configured
> > > on them, but virtual interfaces shouldn't -- mostly because the order of
> > > configuration is often muddled.
> > 
> > So "inet6 2001:db8::1" in hostname.em0 will do the trick but
> > hostname.vport0 would need another "up" for the same behaviour:  that's
> > rather confusing me as a user.
> 
> hostname.* files are orthogonal to this; netstart can process all the lines,
> then if it has seen a line doing address configuration and has not seen an
> explicit "down", it can bring the interface up automatically at the end.
> (if this changed, it would be a nightmare for users to do anything else).

Yes, netstart can and should deal with this correctly, just like you
describe.

> Users would need to make sure they have a netstart which does that if
> updating a kernel, but that's just a case of matching kernel+userland and is
> nothing new for OpenBSD.
> 
> The different behaviour would be apparent with separate runs of ifconfig.
> some scripts may need adapting and users might need to run "ifconfig XX up"
> themselves but I don't think that would be a problem.

Agreed.

Having the implicit-up logic entirely contained in netstart would make
lifer much easier, both for network stack hackers and users, imho.



Re: vport: set UP on ip assign

2021-11-15 Thread Klemens Nanni
On Sun, Nov 14, 2021 at 07:04:42PM -0700, Theo de Raadt wrote:
> I think physical interfaces should come up when something is configured
> on them, but virtual interfaces shouldn't -- mostly because the order of
> configuration is often muddled.

So "inet6 2001:db8::1" in hostname.em0 will do the trick but
hostname.vport0 would need another "up" for the same behaviour:  that's
rather confusing me as a user.


> David Gwynne  wrote:
> 
> > On Sat, Nov 13, 2021 at 11:59:59PM +, Klemens Nanni wrote:
> > > Practically all interfaces pull itself up when IPs get assigned, but
> > > vport(4) does not.
> > 
> > Yes, I do (or don't do) this very deliberately when I get the chance.
> > 
> > > This broke IPv4 networking for me on a router I switched from bridge(4)
> > > to veb(4) because hostname.vport0 only contained the equivalent of
> > > 
> > >   descr LAN
> > >   inet 192.0.2.1
> > >   inet6 2001:db8::1
> > > 
> > > e.g. the explicit "up" was missing.
> > > 
> > > dhcpd(8) only considers UP interfaces to listen on during start;
> > > being the only interface it could potentially listen on, dhcpd thus
> > > ignored vport0 and failed to start.
> > > 
> > > `ifconfig vport0 up && rcctl restart dhcpd' fixed this.
> > > Adding "up" to thostname.vport0 also fixed it.
> > > 
> > > Nonetheless, vport should UP itself as the rest does.
> > 
> > My counter argument is that no interface should implicitly bring itself
> > up when an address is configured because it has been a road block to
> > figuring out how to lock the ioctl paths better, and it confuses error
> > handling. If address config works, but the interface fails to come up,
> > the address remains configured but we report an error. But it looks like
> > configuring an address failed? Wat.
> > 
> > I've suggested previously that netstart should handle bringing an
> > interface up. look for "netstart: implicit up and explicit down for
> > hostname.if conf files" on tech@. I didn't hanve the energy to push
> > it forward though.
> > 
> > dhcpd should cope with an interface being down too. It should be about
> > whetherthe addresses are right more than if the interface is up or not.
> > 
> > > 
> > > OK?
> > > 
> > > Index: net/if_veb.c
> > > ===
> > > RCS file: /cvs/src/sys/net/if_veb.c,v
> > > retrieving revision 1.21
> > > diff -u -p -r1.21 if_veb.c
> > > --- net/if_veb.c  8 Nov 2021 04:15:46 -   1.21
> > > +++ net/if_veb.c  13 Nov 2021 23:47:58 -
> > > @@ -2122,6 +2122,10 @@ vport_ioctl(struct ifnet *ifp, u_long cm
> > >   return (ENXIO);
> > >  
> > >   switch (cmd) {
> > > + case SIOCSIFADDR:
> > > + ifp->if_flags |= IFF_UP;
> > > + /* FALLTHROUGH */
> > > +
> > >   case SIOCSIFFLAGS:
> > >   if (ISSET(ifp->if_flags, IFF_UP)) {
> > >   if (!ISSET(ifp->if_flags, IFF_RUNNING))
> > > 
> > 
> 



Re: vport: set UP on ip assign

2021-11-15 Thread Klemens Nanni
On Mon, Nov 15, 2021 at 12:00:18PM +1000, David Gwynne wrote:
> On Sat, Nov 13, 2021 at 11:59:59PM +0000, Klemens Nanni wrote:
> > Practically all interfaces pull itself up when IPs get assigned, but
> > vport(4) does not.
> 
> Yes, I do (or don't do) this very deliberately when I get the chance.
> 
> > This broke IPv4 networking for me on a router I switched from bridge(4)
> > to veb(4) because hostname.vport0 only contained the equivalent of
> > 
> > descr LAN
> > inet 192.0.2.1
> > inet6 2001:db8::1
> > 
> > e.g. the explicit "up" was missing.
> > 
> > dhcpd(8) only considers UP interfaces to listen on during start;
> > being the only interface it could potentially listen on, dhcpd thus
> > ignored vport0 and failed to start.
> > 
> > `ifconfig vport0 up && rcctl restart dhcpd' fixed this.
> > Adding "up" to thostname.vport0 also fixed it.
> > 
> > Nonetheless, vport should UP itself as the rest does.
> 
> My counter argument is that no interface should implicitly bring itself
> up when an address is configured because it has been a road block to
> figuring out how to lock the ioctl paths better, and it confuses error
> handling. If address config works, but the interface fails to come up,
> the address remains configured but we report an error. But it looks like
> configuring an address failed? Wat.

That's a valid point, although I must say I never ran into this issue.

I fear that users have come to rely on the implicit up semantics, i.e.
I doubt I'm the only one with hostname.* files lacking "up".

> I've suggested previously that netstart should handle bringing an
> interface up. look for "netstart: implicit up and explicit down for
> hostname.if conf files" on tech@. I didn't hanve the energy to push
> it forward though.

I'll do the digging and try to catch up, thanks.

> dhcpd should cope with an interface being down too. It should be about
> whetherthe addresses are right more than if the interface is up or not.
> 
> > 
> > OK?
> > 
> > Index: net/if_veb.c
> > ===
> > RCS file: /cvs/src/sys/net/if_veb.c,v
> > retrieving revision 1.21
> > diff -u -p -r1.21 if_veb.c
> > --- net/if_veb.c8 Nov 2021 04:15:46 -   1.21
> > +++ net/if_veb.c13 Nov 2021 23:47:58 -
> > @@ -2122,6 +2122,10 @@ vport_ioctl(struct ifnet *ifp, u_long cm
> > return (ENXIO);
> >  
> > switch (cmd) {
> > +   case SIOCSIFADDR:
> > +   ifp->if_flags |= IFF_UP;
> > +   /* FALLTHROUGH */
> > +
> > case SIOCSIFFLAGS:
> > if (ISSET(ifp->if_flags, IFF_UP)) {
> > if (!ISSET(ifp->if_flags, IFF_RUNNING))
> > 
> 



Re: let dhcpd start on down interfaces

2021-11-15 Thread Klemens Nanni
On Mon, Nov 15, 2021 at 02:08:33PM +1000, David Gwynne wrote:
> The subject line only tells half the story. The other half is that
> instead of hoping it only identifies Ethernet interfaces it now actually
> checks and restricts itself to Ethernet interfaces. I couldn't help
> myself.
> 
> If it helps my case for this code, I've been using this semantic for a
> few years in a home grown dhcp-relay we run on our firewalls at
> work. We used to run the dhcp-relay on a bunch of vlan(4) interfaces,
> but we currently run them on carp(4). There's no significant difference
> between using an Ethernet or carp interface from a dhcpd or relay point
> of view, so the code accepts either.

This sounds reasonable, but I have no means to test these setups.

> The kernel lets you bind to addresses on down interfaces, so I don't
> know why dhcpd has to be precious about it. We often start the
> firewalls up with whole groups of these interfaces forced down.
> We want DHCP packets to start moving when then interface comes up
> though, therefore the daemon should start and be running even if
> the interface is down.

I wondered about this as well but was hesitant to touch dhcpd(8) because
I rarely use it.

FWIW, ISC DHCP has the same code and comment wrt. interface check,
altough I didn't do any runtime tests.

> Should I warn if the interface is down on startup?

Wouldn't hurt, although you don't see it at boot time anyway, so not
much help, imho.

> There's some other questionable code in here, but I'm going to take
> some deep breaths and try to ignore them for now.

Here is the minimal diff I first used to fix my vport/DOWN issue,
before going the implicit UP route.

Regardless of vport's behaviour, this seemed sensible;  I mean, I can
`nc -l' on down interfaces, other daemons don't care about UP and I'd
rather have dhcpd running and pull interfaces UP to fix DHCP instead of
restarting failed dhcpd (which could still serve on other UP interfaces).


Index: dispatch.c
===
RCS file: /cvs/src/usr.sbin/dhcpd/dispatch.c,v
retrieving revision 1.43
diff -u -p -r1.43 dispatch.c
--- dispatch.c  12 Apr 2017 19:17:30 -  1.43
+++ dispatch.c  13 Nov 2021 23:04:36 -
@@ -112,13 +112,12 @@ discover_interfaces(int *rdomain)
for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) {
/*
 * See if this is the sort of interface we want to
-* deal with.  Skip loopback, point-to-point and down
+* deal with.  Skip loopback and point-to-point
 * interfaces, except don't skip down interfaces if we're
 * trying to get a list of configurable interfaces.
 */
if ((ifa->ifa_flags & IFF_LOOPBACK) ||
(ifa->ifa_flags & IFF_POINTOPOINT) ||
-   (!(ifa->ifa_flags & IFF_UP)) ||
(!(ifa->ifa_flags & IFF_BROADCAST)))
continue;
 

> OK?
> 
> Index: dispatch.c
> ===
> RCS file: /cvs/src/usr.sbin/dhcpd/dispatch.c,v
> retrieving revision 1.43
> diff -u -p -r1.43 dispatch.c
> --- dispatch.c12 Apr 2017 19:17:30 -  1.43
> +++ dispatch.c15 Nov 2021 03:50:46 -
> @@ -47,6 +47,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -112,14 +113,10 @@ discover_interfaces(int *rdomain)
>   for (ifa = ifap; ifa != NULL; ifa = ifa->ifa_next) {
>   /*
>* See if this is the sort of interface we want to
> -  * deal with.  Skip loopback, point-to-point and down
> -  * interfaces, except don't skip down interfaces if we're
> -  * trying to get a list of configurable interfaces.
> +  * deal with, which is basically Ethernet.
>*/
>   if ((ifa->ifa_flags & IFF_LOOPBACK) ||
> - (ifa->ifa_flags & IFF_POINTOPOINT) ||
> - (!(ifa->ifa_flags & IFF_UP)) ||
> - (!(ifa->ifa_flags & IFF_BROADCAST)))
> + (ifa->ifa_flags & IFF_POINTOPOINT))
>   continue;
>  
>   /* See if we've seen an interface that matches this one. */
> @@ -156,11 +153,27 @@ discover_interfaces(int *rdomain)
>   /* If we have the capability, extract link information
>  and record it in a linked list. */
>   if (ifa->ifa_addr->sa_family == AF_LINK) {
> - struct sockaddr_dl *sdl =
> - ((struct sockaddr_dl *)(ifa->ifa_addr));
> + struct if_data *ifi;
> + struct sockaddr_dl *sdl;
> +
> + ifi = (struct if_data *)ifa->ifa_data;
> + if (ifi->ifi_type != IFT_ETHER &&
> + ifi->ifi_type != IFT_CARP) {
> + /* this interface 

ifconfig.8: autoconf implies up

2021-11-13 Thread Klemens Nanni
Since march 2021 setting AUTOCONF{4,6} sets UP as well unconditionally.
I have lots of hostname.if files containing only "inet6 autoconf"
without "up" and it works, but I noticed this isn't documented anywhere.

Neither do we remove UP and/or RUNNING on `-autoconf' and neither do we
document this.

This is a good start, I think.


Feedback? OK?

Index: ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.379
diff -u -p -r1.379 ifconfig.8
--- ifconfig.8  11 Nov 2021 09:39:16 -  1.379
+++ ifconfig.8  13 Nov 2021 23:01:33 -
@@ -163,6 +163,9 @@ automatically configures IPv4 addresses 
 for interfaces with
 .Sy AUTOCONF4
 set.
+.Pp
+Automatically mark the interface as
+.Dq up .
 .It Cm -autoconf
 Unset the
 .Sy AUTOCONF4



vport: set UP on ip assign

2021-11-13 Thread Klemens Nanni
Practically all interfaces pull itself up when IPs get assigned, but
vport(4) does not.

This broke IPv4 networking for me on a router I switched from bridge(4)
to veb(4) because hostname.vport0 only contained the equivalent of

descr LAN
inet 192.0.2.1
inet6 2001:db8::1

e.g. the explicit "up" was missing.

dhcpd(8) only considers UP interfaces to listen on during start;
being the only interface it could potentially listen on, dhcpd thus
ignored vport0 and failed to start.

`ifconfig vport0 up && rcctl restart dhcpd' fixed this.
Adding "up" to hostname.vport0 also fixed it.

Nonetheless, vport should UP itself as the rest does.

OK?

Index: net/if_veb.c
===
RCS file: /cvs/src/sys/net/if_veb.c,v
retrieving revision 1.21
diff -u -p -r1.21 if_veb.c
--- net/if_veb.c8 Nov 2021 04:15:46 -   1.21
+++ net/if_veb.c13 Nov 2021 23:47:58 -
@@ -2122,6 +2122,10 @@ vport_ioctl(struct ifnet *ifp, u_long cm
return (ENXIO);
 
switch (cmd) {
+   case SIOCSIFADDR:
+   ifp->if_flags |= IFF_UP;
+   /* FALLTHROUGH */
+
case SIOCSIFFLAGS:
if (ISSET(ifp->if_flags, IFF_UP)) {
if (!ISSET(ifp->if_flags, IFF_RUNNING))



Re: installer: prompt for WEP only if available

2021-11-13 Thread Klemens Nanni
On Tue, Nov 02, 2021 at 05:43:03PM +, Klemens Nanni wrote:
> On Tue, Nov 02, 2021 at 05:26:17PM +0000, Klemens Nanni wrote:
> > At least bwfm(4) does not support WEP:
> > 
> > # ifconfig bwfm0 nwkey 12345  
> > ifconfig: SIOCS80211NWKEY: Operation not supported by device
> > # echo $?
> > 0
> > 
> > ifconfig(8) must return non-zero in this case.
> > 
> > This is relevant for an upcoming installer fix, but also worth itself.
> > 
> > OK?
> > 
> > Index: ifconfig.c
> > ===
> > RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> > retrieving revision 1.445
> > diff -u -p -r1.445 ifconfig.c
> > --- ifconfig.c  6 Oct 2021 06:14:08 -   1.445
> > +++ ifconfig.c  2 Nov 2021 17:20:01 -
> > @@ -2033,7 +2033,7 @@ setifnwkey(const char *val, int d)
> > }
> >  
> > if (ioctl(sock, SIOCS80211NWKEY, (caddr_t)) == -1)
> > -   warn("SIOCS80211NWKEY");
> > +   err("SIOCS80211NWKEY");
> >  }
> >  
> >  /* ARGSUSED */
> > 
> 
> I noticed the above mentioned error^Wwarning in the installer:
> 
>   Which network interface do you wish to configure? (or 'done') [bse0] 
> bwfm0
>   ifconfig: SIOCS80211NWKEY: Operation not supported by device
>   Access point? (ESSID, 'any', list# or '?') [any] 2
>   Security protocol? (O)pen, (W)EP, WPA-(P)SK [O] 
> 
> 
> This comes from ieee80211_scan():
> 
>   Reset 802.11 settings and determine WPA capability.   
>   ifconfig $_if -nwid -nwkey
>   ifconfig $_if -wpa 2>/dev/null && _has_wpa=1  
> 
> 
> With ifconfig and installer fixed, we can properly detect and skip WEP:
> 
>   Which network interface do you wish to configure? (or 'done') [bse0] 
> bwfm0
>   Access point? (ESSID, 'any', list# or '?') [any] 2
>   Security protocol? (O)pen, WPA-(P)SK [O] w
>   'w' is not a valid choice.
>   Security protocol? (O)pen, WPA-(P)SK [O] p
>   WPA passphrase? (will echo) 

So bwfm(4) still has not fix/support for WEP.

Whether accidentially as demonstrated above or potentially intentionally
by removing WEP from (new) drivers in order to deprecate it,
I think it is a good idea to probe for WEP support independently just
like we do for WPA already.

This way bwfm+WEP users won't run into install scenarios that are known
to be broken;  maybe other drivers have similar problems, so this diff
would reflect it.

If we ever^W^W^WOnce we deprecate WPA (on a per driver basis?), this
diff will have us prepared already.

I tested this on my Pinebook Pro and Pi 4 B which both come with bwfm.

This also nicely decouples the ifconfig(8) commands used to reset;
multiple commands together are not atomic and failure on one command
will effect execution of the others (or not, the past showed both ways),
so best reset step-by-step and be able to handle failure individually.


OK?

Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1184
diff -u -p -r1.1184 install.sub
--- install.sub 2 Nov 2021 16:54:01 -   1.1184
+++ install.sub 13 Nov 2021 13:11:34 -
@@ -1228,11 +1228,12 @@ ieee80211_scan() {
 # Configure 802.11 interface $1 and append ifconfig options to hostname.if $2.
 # Ask the user for the access point ESSID, the security protocol and a secret.
 ieee80211_config() {
-   local _if=$1 _hn=$2 _prompt _nwid _haswpa=0 _err
+   local _if=$1 _hn=$2 _prompt _nwid _has_wep=0 _has_wpa=0 _err
 
-   # Reset 802.11 settings and determine wpa capability.
-   ifconfig $_if -nwid -nwkey
-   ifconfig $_if -wpa 2>/dev/null && _haswpa=1
+   # Reset 802.11 settings and determine WEP and WPA capabilities.
+   ifconfig $_if -nwid
+   ifconfig $_if -nwkey 2>/dev/null && _has_wep=1
+   ifconfig $_if -wpa 2>/dev/null && _has_wpa=1
 
# Empty scan cache.
rm -f $WLANLIST
@@ -1256,17 +1257,19 @@ ieee80211_config() {
# 'any' implies that only open access points are considered.
if [[ $_nwid != any ]]; then
 
-   _prompt="Security protocol? (O)pen, (W)EP"
-   ((_haswpa == 1)) && _prompt="$_prompt, WPA-(P)SK"
+   _prompt="Security protocol? (O)pen"
+   ((_has_wep == 1)) && _prompt="$_prompt, (W)EP"
+   ((_has_wpa == 1)) && _prompt="$_prompt, WPA-(P)SK"
while :; do
ask_until "$_prompt" "O"
-   case "$_haswpa-$resp" in
- 

Re: make.1: sync variable substitution bits with NetBSD

2021-11-13 Thread Klemens Nanni
On Sat, Dec 26, 2020 at 05:19:55PM +0100, Klemens Nanni wrote:
> Our make(1) is behind NetBSD's and FreeBSD's make(1) on at least the
> rules of variable substitution.
> 
> Our DESCRIPION says
> 
>  There are seven different types of lines in a makefile: dependency lines,
>  shell commands, variable assignments, include statements, conditional
>  directives, for loops, and comments.  Of these, include statements,
>  conditional directives and for loops are extensions.
> 
> but our VARIABLES merely explains
> 
>  Variable substitution occurs at two distinct times, depending on where
>  the variable is being used.  Variables in dependency lines are expanded
>  as the line is read.  Variables in shell commands are expanded when the
>  shell command is executed.
> 
> and therefore fails to explain behaviour for the other five line types.
> 
> 
> http://man.netbsd.org/make.1#VARIABLE%20ASSIGNMENTS (same as FreeBSD in
> this regard) does go into more detail here:
> 
> Variable substitution occurs at three distinct times, depending on where
>  the variable is being used.
> 
>  1.   Variables in dependency lines are expanded as the line is read.
> 
>  2.   Variables in shell commands are expanded when the shell command 
> is
>   executed.
> 
>  3.   ``.for'' loop index variables are expanded on each loop 
> iteration.
>   Note that other variables are not expanded inside loops so the 
> fol-
>   lowing example code:
> 
>   [...]
> 
> 
> Diff below sync this list in VARIABLES as is.
> 
> After that I'd like to expand it and explain behaviour for other types;
> I've scratched my head on make's behaviour for too long and the manual
> failed to cover this completely.
> 
> Feedback? OK?

I came across this again, so I would really like to fix our manual.

In https://marc.info/?l=openbsd-tech=160935207004852=2
on 2020-12-30 espie said:
> I do think we want to write something specific for .for loop variables
> which are actually very special compared to the rest.
> 
> I'm not incredibly happy with the way netbsd explains it, not surprisingly.

which didn't result in any update to our manual.

I'm still in favour of syncing with NetBSD (first), they're wording and
example explains the missing pieces.

We can gladly polish things afterwards if someone comes up with
something better.

OK?

Index: make.1
===
RCS file: /cvs/src/usr.bin/make/make.1,v
retrieving revision 1.134
diff -u -p -r1.134 make.1
--- make.1  11 Nov 2021 20:42:54 -  1.134
+++ make.1  13 Nov 2021 09:58:34 -
@@ -600,11 +600,48 @@ If the variable name contains only a sin
 braces or parentheses are not required.
 This shorter form is not recommended.
 .Pp
-Variable substitution occurs at two distinct times, depending on where
+Variable substitution occurs at three distinct times, depending on where
 the variable is being used.
+.Bl -enum
+.It
 Variables in dependency lines are expanded as the line is read.
+.It
 Variables in shell commands are expanded when the shell command is
 executed.
+.It
+.Dq .for
+loop index variables are expanded on each loop iteration.
+Note that other variables are not expanded inside loops so
+the following example code:
+.Bd -literal -offset indent
+
+.Dv .for i in 1 2 3
+a+= ${i}
+j=  ${i}
+b+= ${j}
+.Dv .endfor
+
+all:
+   @echo ${a}
+   @echo ${b}
+
+.Ed
+will print:
+.Bd -literal -offset indent
+1 2 3
+3 3 3
+
+.Ed
+Because while ${a} contains
+.Dq 1 2 3
+after the loop is executed, ${b}
+contains
+.Dq ${j} ${j} ${j}
+which expands to
+.Dq 3 3 3
+since after the loop completes ${j} contains
+.Dq 3 .
+.El
 .Pp
 The four different classes of variables (in order of increasing precedence)
 are:



Re: [PATCH] [src] share/man/man5/hostname.if.5 - nwid -> join

2021-11-11 Thread Klemens Nanni
On Thu, Nov 11, 2021 at 08:13:13PM +, Jason McIntyre wrote:
> i don;t think this bit of text works well now because you say it runs
> ifconfig 3 times then ... respectively. but you only list two things
> (even though i understand dynamic config is two). since your text is
> already clear, i'd omit ", respectively". or maybe say "inet4 and inet6
> dynamic ..." to balance out the three times would be better.

Thanks, is this better?


Index: hostname.if.5
===
RCS file: /cvs/src/share/man/man5/hostname.if.5,v
retrieving revision 1.77
diff -u -p -r1.77 hostname.if.5
--- hostname.if.5   17 Jul 2021 15:28:31 -  1.77
+++ hostname.if.5   11 Nov 2021 20:30:05 -
@@ -67,21 +67,13 @@ inet 10.0.0.1 255.255.255.0 10.0.0.255 d
 Each line is processed separately and in order.
 For example:
 .Bd -literal -offset indent
-nwid mynwid wpakey mywpakey
+join mynwid wpakey mywpakey
 inet6 autoconf
 inet autoconf
 .Ed
 .Pp
-would run ifconfig three times to set the
-.Cm nwid
-and
-.Cm wpakey
-of the interface,
-the
-.Sy AUTOCONF6
-flag and the
-.Sy AUTOCONF4
-flag, respectively.
+would run ifconfig three times to add a wireless network using WPA to the
+join list and enable dynamic address configuration for IPv6 and IPv4.
 .Sh STATIC ADDRESS CONFIGURATION
 The following packed formats are valid for configuring network
 interfaces with static addresses.



Re: [PATCH] [src] share/man/man5/hostname.if.5 - nwid -> join

2021-11-11 Thread Klemens Nanni
On Thu, Nov 11, 2021 at 05:48:24PM +, Stuart Henderson wrote:
> On 2021/11/11 16:30, Klemens Nanni wrote:
> > On Thu, Nov 11, 2021 at 04:11:10PM +, Raf Czlonka wrote:
> > > Hello,
> > > 
> > > It seems like this has been missed in recent thread[0].
> > > 
> > > Not entirely sure whether the sentence "flows" any longer but here
> > > it goes anyway.
> > > 
> > > [0] https://marc.info/?l=openbsd-tech=163507448118443=2
> > 
> > Thanks, I missed that!
> > 
> > > Index: share/man/man5/hostname.if.5
> > > ===
> > > RCS file: /cvs/src/share/man/man5/hostname.if.5,v
> > > retrieving revision 1.77
> > > diff -u -p -r1.77 hostname.if.5
> > > --- share/man/man5/hostname.if.5  17 Jul 2021 15:28:31 -  1.77
> > > +++ share/man/man5/hostname.if.5  11 Nov 2021 16:09:33 -
> > > @@ -67,13 +67,13 @@ inet 10.0.0.1 255.255.255.0 10.0.0.255 d
> > >  Each line is processed separately and in order.
> > >  For example:
> > >  .Bd -literal -offset indent
> > > -nwid mynwid wpakey mywpakey
> > > +join mynwid wpakey mywpakey
> > >  inet6 autoconf
> > >  inet autoconf
> > >  .Ed
> > >  .Pp
> > >  would run ifconfig three times to set the
> > > -.Cm nwid
> > > +.Cm join
> > >  and
> > >  .Cm wpakey
> > >  of the interface,
> > 
> > Maybe this reads better in general?
> > 
> > would run ifconfig three times to join a wireless network using WPA and
> > to set the AUTOCONF6 and AUTOCONF4 flags, respectively.
> 
> It is more "add a network to the join list", join implies that it's
> connecting directly to that ssid (i.e. what "nwid" did).

Good point.
Also no markup as per jmc.

OK?


Index: hostname.if.5
===
RCS file: /cvs/src/share/man/man5/hostname.if.5,v
retrieving revision 1.77
diff -u -p -r1.77 hostname.if.5
--- hostname.if.5   17 Jul 2021 15:28:31 -  1.77
+++ hostname.if.5   11 Nov 2021 20:00:45 -
@@ -67,21 +67,13 @@ inet 10.0.0.1 255.255.255.0 10.0.0.255 d
 Each line is processed separately and in order.
 For example:
 .Bd -literal -offset indent
-nwid mynwid wpakey mywpakey
+join mynwid wpakey mywpakey
 inet6 autoconf
 inet autoconf
 .Ed
 .Pp
-would run ifconfig three times to set the
-.Cm nwid
-and
-.Cm wpakey
-of the interface,
-the
-.Sy AUTOCONF6
-flag and the
-.Sy AUTOCONF4
-flag, respectively.
+would run ifconfig three times to add a wireless network using WPA to the
+join list and enable dynamic address configuration, respectively.
 .Sh STATIC ADDRESS CONFIGURATION
 The following packed formats are valid for configuring network
 interfaces with static addresses.



Re: amd4/bsd.rd: make "config -e" work

2021-11-11 Thread Klemens Nanni
On Thu, Nov 04, 2021 at 10:49:46PM +, Klemens Nanni wrote:
> amd64, alpha, i386 and macppc strip all symbols off the ramdisk bsd.rd
> (before gzipping it) and thus break config(8)'s modification feature:
> 
>   $ gzcat bsd.rd > bsd.rd.raw
>   $ config -e bsd.rd.raw
>   WARNING no output file specified
>   WARNING this kernel doesn't contain all information needed!
>   WARNING the commands add and change might not work.
>   WARNING this kernel doesn't support pseudo devices.
>   WARNING this kernel doesn't support modification of NKMEMPAGES.
>   config: failed to get first cfdata
> 
> Needing 'disable xhci' on arm64/Pinebook Pro right now, I looked into
> all of bsd.re-config(5), UKC and how we build bsd.rd for all platforms.

Correction:  needed it for the Pi 4, the PBP had other problems...

> That's how I noticed modifying a ramdisk kernel on these for platforms
> wouldn't work in the first place.
> 
> Needing it to fix or upgrade systems can be crucial, so I figured it's
> worth addressing.
> 
> While `boot /bsd.rd -c' does work on amd64 and macppc regardless of
> symbols, its changes do not persist and `config -e' is more convenient.
> 
> So I propose to strip less symbols such to make this work.
> 
> On amd64, this accounts for 200K growth in bsd.gz according to `du -k':

New diff now that amd64 RAMDISK_CD contains igc(4):

-9168obj/bsd.strip
+9872obj/bsd.strip
-4272obj/bsd.gz
+4480obj/bsd.gz

> I cranked FSSIZE to the smallest possible volume that would build.
> The resulting miniroot70.img boots fine in vmm(4) and on a X250.

In 512-byte block counts (`du obj/bsd.{strip,gz}`):

-18336   obj/bsd.strip
+19744   obj/bsd.strip
-8544obj/bsd.gz
+8960obj/bsd.gz

So I first cranked FSSIZE by 8960 - 8544 = 416, from 10240 to 10686.
That built, but is more than required, so I bisected it to the minimal
working value of 10368.

amd64 builds, boots and works as expected.

> macppc is still building;  I have no alpha or i386.

macppc builds, boots and works as expected without cranking FSSIZE.


Feedback? Objections? OK?

Index: amd64/ramdisk_cd/Makefile
===
RCS file: /cvs/src/distrib/amd64/ramdisk_cd/Makefile,v
retrieving revision 1.32
diff -u -p -r1.32 Makefile
--- amd64/ramdisk_cd/Makefile   7 Nov 2021 15:50:15 -   1.32
+++ amd64/ramdisk_cd/Makefile   11 Nov 2021 19:43:56 -
@@ -1,7 +1,7 @@
 #  $OpenBSD: Makefile,v 1.32 2021/11/07 15:50:15 deraadt Exp $
 
 FS=miniroot${OSrev}.img
-FSSIZE=10240
+FSSIZE=10368
 FSDISKTYPE=mini34
 CDROM= cd${OSrev}.iso
 MOUNT_POINT=   /mnt
@@ -56,7 +56,7 @@ MRDISKTYPE=   rdrootb
 MRMAKEFSARGS=  -o disklabel=${MRDISKTYPE},minfree=0,density=4096
 
 bsd.gz: bsd.rd
-   objcopy -S -R .comment -R .SUNW_ctf \
+   objcopy -g -x -R .comment -R .SUNW_ctf \
-K rd_root_size -K rd_root_image \
bsd.rd bsd.strip
gzip -9cn bsd.strip > bsd.gz
Index: macppc/ramdisk/Makefile
===
RCS file: /cvs/src/distrib/macppc/ramdisk/Makefile,v
retrieving revision 1.51
diff -u -p -r1.51 Makefile
--- macppc/ramdisk/Makefile 26 Jul 2021 12:47:46 -  1.51
+++ macppc/ramdisk/Makefile 11 Nov 2021 19:44:14 -
@@ -35,7 +35,7 @@ bsd.gz: bsd.rd
gzip -9cn bsd.rd > bsd.gz
 
 bsd.rd: mr.fs bsd
-   objcopy -S -R .comment -R .SUNW_ctf \
+   objcopy -g -x -R .comment -R .SUNW_ctf \
-K rd_root_size -K rd_root_image \
bsd bsd.rd
rdsetroot bsd.rd mr.fs



Re: [PATCH] [src] share/man/man5/hostname.if.5 - nwid -> join

2021-11-11 Thread Klemens Nanni
On Thu, Nov 11, 2021 at 04:11:10PM +, Raf Czlonka wrote:
> Hello,
> 
> It seems like this has been missed in recent thread[0].
> 
> Not entirely sure whether the sentence "flows" any longer but here
> it goes anyway.
> 
> [0] https://marc.info/?l=openbsd-tech=163507448118443=2

Thanks, I missed that!

> Index: share/man/man5/hostname.if.5
> ===
> RCS file: /cvs/src/share/man/man5/hostname.if.5,v
> retrieving revision 1.77
> diff -u -p -r1.77 hostname.if.5
> --- share/man/man5/hostname.if.5  17 Jul 2021 15:28:31 -  1.77
> +++ share/man/man5/hostname.if.5  11 Nov 2021 16:09:33 -
> @@ -67,13 +67,13 @@ inet 10.0.0.1 255.255.255.0 10.0.0.255 d
>  Each line is processed separately and in order.
>  For example:
>  .Bd -literal -offset indent
> -nwid mynwid wpakey mywpakey
> +join mynwid wpakey mywpakey
>  inet6 autoconf
>  inet autoconf
>  .Ed
>  .Pp
>  would run ifconfig three times to set the
> -.Cm nwid
> +.Cm join
>  and
>  .Cm wpakey
>  of the interface,

Maybe this reads better in general?

would run ifconfig three times to join a wireless network using WPA and
to set the AUTOCONF6 and AUTOCONF4 flags, respectively.

No need to explain lines command by command.  Keep it simple;  users
must read ifconfig(8) anyway.

One might as well say to

... and
to enable automatic address configuration for IPv6 and IPv4, respectively.

Then we don't have the weird mix of conceptual words and technical bits,
i.e. "connect to wifi" vs. "set if flags [they're picked up by network
daemons, but we don't say which ones]".

DYNAMIC ADDRESS CONFIGURATION down below repeats all this, anyway.

Feedback? OK?


Index: hostname.if.5
===
RCS file: /cvs/src/share/man/man5/hostname.if.5,v
retrieving revision 1.77
diff -u -p -r1.77 hostname.if.5
--- hostname.if.5   17 Jul 2021 15:28:31 -  1.77
+++ hostname.if.5   11 Nov 2021 16:30:00 -
@@ -67,21 +67,15 @@ inet 10.0.0.1 255.255.255.0 10.0.0.255 d
 Each line is processed separately and in order.
 For example:
 .Bd -literal -offset indent
-nwid mynwid wpakey mywpakey
+join mynwid wpakey mywpakey
 inet6 autoconf
 inet autoconf
 .Ed
 .Pp
-would run ifconfig three times to set the
-.Cm nwid
-and
-.Cm wpakey
-of the interface,
-the
-.Sy AUTOCONF6
-flag and the
-.Sy AUTOCONF4
-flag, respectively.
+would run ifconfig three times to
+.Cm join
+a wireless network using WPA
+and enable dynamic address configuration, respectively.
 .Sh STATIC ADDRESS CONFIGURATION
 The following packed formats are valid for configuring network
 interfaces with static addresses.



Re: Teach manpages of resolv(8) and unwindctl(8) about sppp(4)

2021-11-11 Thread Klemens Nanni
On Thu, Nov 11, 2021 at 07:01:42AM +0100, Bjorn Ketelaars wrote:
> On Wed 10/11/2021 21:20, Klemens Nanni wrote:
> > I think only unwind(8) should list all the inputs and unwindctl(8)
> > should just say "Show learned nameservers".
> > 
> > unwind(8) is already incomplete regardless of sppp(4) and unwindctl(8)
> > is a poor duplicate of it.
> 
> I agree with unwindctl(8). For unwind(8) i would like to take it one
> step further and use resolvd(8) as single source of truth. What do you
> think of the diff below?

I'm not convinced:  users shouldn't read manuals of tools they don't use
in order to learn how the tools they do use are working.

> diff --git sbin/unwind/unwind.8 sbin/unwind/unwind.8
> index afa7d0ad2c1..38f7d1aaea8 100644
> --- sbin/unwind/unwind.8
> +++ sbin/unwind/unwind.8
> @@ -33,11 +33,8 @@ It is intended to run on client machines like workstations 
> or laptops and only
>  listens on localhost.
>  .Nm
>  sends DNS queries to nameservers to answer queries and switches to resolvers
> -learned from
> -.Xr dhclient 8 ,
> -.Xr dhcpleased 8
> -or
> -.Xr slaacd 8
> +learned by
> +.Xr resolvd 8
>  if it detects that DNS queries are blocked by the local network.
>  It periodically probes if DNS is no longer blocked and switches back to
>  querying nameservers itself.

This is wrong.  Stop resolvd and start unwind, you'll see that
`unwindctl status autoconf' shows learned nameservers.

> @@ -104,9 +101,7 @@ socket used for communication with
>  .El
>  .Sh SEE ALSO
>  .Xr unwind.conf 5 ,
> -.Xr dhclient 8 ,
> -.Xr dhcpleased 8 ,
> -.Xr slaacd 8 ,
> +.Xr resolvd 8 ,
>  .Xr unbound 8 ,
>  .Xr unwindctl 8
>  .Sh STANDARDS

> diff --git usr.sbin/unwindctl/unwindctl.8 usr.sbin/unwindctl/unwindctl.8
> index 1b1be4451e7..b8289a82bd9 100644
> --- usr.sbin/unwindctl/unwindctl.8
> +++ usr.sbin/unwindctl/unwindctl.8
> @@ -56,11 +56,7 @@ Reload the configuration file.
>  .It Cm status
>  Show a status summary.
>  .It Cm status autoconf
> -Show nameservers learned from
> -.Xr dhclient 8 ,
> -.Xr dhcpleased 8
> -or
> -.Xr slaacd 8 .
> +Show learned nameservers.
>  .It Cm status memory
>  Show memory consumption.
>  .El

That's what I have in my WIP diff already.



Re: Teach manpages of resolv(8) and unwindctl(8) about sppp(4)

2021-11-10 Thread Klemens Nanni
On Wed, Nov 10, 2021 at 04:28:10PM +0100, Bjorn Ketelaars wrote:
> Like umb(4), sppp(4) is natively learning DNS information. Diff below
> adds this information to the manpages of resolv(8) and unwindctl(8).
> While here, also mention umb(4) in unwindctl's manpage.
> 
> Thanks to kn@ for noticing the above.

I still want to polish all this in relation to route(8)'s `nameserver'
command... resolvd and unwind learn the same way but have different text
and they don't mention the static proposals...

But your diff goes in the right direction and I don't want to drag you
into this nit-picking business;  see inline.

> diff --git sbin/resolvd/resolvd.8 sbin/resolvd/resolvd.8
> index 71d0e40567e..d8c46d1972d 100644
> --- sbin/resolvd/resolvd.8
> +++ sbin/resolvd/resolvd.8
> @@ -48,6 +48,8 @@ also monitors the routing socket for proposals learned by
>  .Xr dhcpleased 8 ,
>  .Xr slaacd 8 ,
>  or network devices which natively learn DNS information such as
> +.Xr sppp 4
> +or
>  .Xr umb 4 .
>  Proposals can also be sent using the
>  .Xr route 8

OK kn for above hunk.

> diff --git usr.sbin/unwindctl/unwindctl.8 usr.sbin/unwindctl/unwindctl.8
> index 1b1be4451e7..a5c74c6303f 100644
> --- usr.sbin/unwindctl/unwindctl.8
> +++ usr.sbin/unwindctl/unwindctl.8
> @@ -58,9 +58,12 @@ Show a status summary.
>  .It Cm status autoconf
>  Show nameservers learned from
>  .Xr dhclient 8 ,
> -.Xr dhcpleased 8
> +.Xr dhcpleased 8 ,
> +.Xr slaacd 8 ,
> +or network devices which natively learn DNS information such as
> +.Xr sppp 4
>  or
> -.Xr slaacd 8 .
> +.Xr umb 4 .
>  .It Cm status memory
>  Show memory consumption.
>  .El

I think only unwind(8) should list all the inputs and unwindctl(8)
should just say "Show learned nameservers".

unwind(8) is already incomplete regardless of sppp(4) and unwindctl(8)
is a poor duplicate of it.



Re: sppp(4)/pppoe(4) - DNS configuration via resolvd(8)

2021-11-10 Thread Klemens Nanni
On Wed, Nov 10, 2021 at 07:35:26AM +0100, Bjorn Ketelaars wrote:
> On Mon 08/11/2021 11:52, Bjorn Ketelaars wrote:
> > Diff below does two things:
> > 1. add PPP IPCP extensions for name server addresses (rfc1877) to
> >sppp(4)
> > 2. propose negotiated name servers from sppp(4) to resolvd(8) using
> >RTM_PROPOSAL_STATIC route messages.
> 
> 
> Updated diff below, based on feedback from kn@ and claudio@:
> 
> - fix forgotten parentheses with `sizeof`
> - instead of using `u_int32_t` use `struct in_addr` for holding dns
>   addresses. Makes it more clear what the data is
> - decouple `IPCP_OPT` definitions from the bitmask values to
>   enable/disable an option. Makes the code look a bit better
> - use `memcpy`
> - fit code within 80 columns
> 
> While here add RFC to sppp(4)'s STANDARDS section.
> 
> @kn, is this still OK for you?

I agree with claudio's feedback on sizeof and semarie's point of using
a dedicated message, but these can be done as their own commits.

OK kn

> diff --git sys/net/if_sppp.h sys/net/if_sppp.h
> index ff559fcc369..5850a6da963 100644
> --- sys/net/if_sppp.h
> +++ sys/net/if_sppp.h
> @@ -132,6 +132,8 @@ struct sipcp {
> * original one here, in network byte order */
>   u_int32_t req_hisaddr;  /* remote address requested (IPv4) */
>   u_int32_t req_myaddr;   /* local address requested (IPv4) */
> +#define IPCP_MAX_DNSSRV  2
> + struct in_addr dns[IPCP_MAX_DNSSRV]; /* IPv4  DNS servers (RFC 1877) */

Double space in comment.

>  #ifdef INET6
>   struct in6_aliasreq req_ifid;   /* local ifid requested (IPv6) */
>  #endif



Re: arm64: new gpiokeys(4)

2021-11-10 Thread Klemens Nanni
On Wed, Nov 10, 2021 at 12:07:37AM +0100, Jan Stary wrote:
> On Nov 09 15:43:04, k...@openbsd.org wrote:
> > This populates `systat sensors' with the correct lid status on my
> > Pinebook Pro:
> > 
> > -gpio-key-lid at mainbus0 not configured
> > -gpio-key-power at mainbus0 not configured
> > +gpiokeys0 at mainbus0: "Lid"
> > +gpiokeys0 at mainbus0: "Power"
> 
> 
> This is what the diff added on RPI4 (dmesg below).
> 
> -gpioleds0 at mainbus0: no LEDs
> +gpioleds0 at mainbus0: "led0", "led1"

This is from another recent commit I made to gpioleds(4), specifically
to recognise LEDs on this machine.

It has nothing to do with gpiokeys(4).

> (Nothing new in systat sens,
> as leds are not sensors.)

Correct, that's because there are no GPIO keys on the Pi 4 (Model B),
i.e. there's nothing in the device tree to match gpiokeys(4).



Re: rpki-client ip_addr_print cleanup

2021-11-09 Thread Klemens Nanni
On Tue, Nov 09, 2021 at 08:05:10PM +0100, Claudio Jeker wrote:
> On Tue, Nov 09, 2021 at 07:44:41PM +0100, Claudio Jeker wrote:
> > ip_addr_print() can be simplified. ip4_addr2str() and ip6_addr2str() are
> > the same apart from the different AF argument to inet_ntop(). Just collaps
> > all into ip_addr_print().
> 
> This version is using a switch statement and fails hard for unknown AFIs.
> Suggested by Theo.

OK kn, one thing.

> @@ -277,11 +242,25 @@ void
>  ip_addr_print(const struct ip_addr *addr,
>  enum afi afi, char *buf, size_t bufsz)
>  {
> + char ipbuf[44];

Why not INET6_ADDRSTRLEN?

> + int ret, af;
> +
> + switch (afi) {
> + case AFI_IPV4:
> + af = AF_INET;
> + break;
> + case AFI_IPV6:
> + af = AF_INET6;
> + break;
> + default:
> + errx(1, "unsupported address family identifier");
> + }
>  
> - if (afi == AFI_IPV4)
> - ip4_addr2str(addr, buf, bufsz);
> - else
> - ip6_addr2str(addr, buf, bufsz);
> + if (inet_ntop(af, addr->addr, ipbuf, sizeof(ipbuf)) == NULL)
> + err(1, "inet_ntop");
> + ret = snprintf(buf, bufsz, "%s/%hhu", ipbuf, addr->prefixlen);
> + if (ret < 0 || (size_t)ret >= bufsz)
> + err(1, "malformed IP address");
>  }
>  
>  /*
> 



Re: rpki-client sync http escape handling with ftp(1)

2021-11-09 Thread Klemens Nanni
On Tue, Nov 09, 2021 at 07:52:06PM +0100, Claudio Jeker wrote:
> kn@ removed '~' from unsafe_chars but also changed the code at the same
> time. This tries to bring the version in rpki-client back in sync with the
> code in ftp(1).

Didn't know there was such a copy.

OK kn



arm64: new gpiokeys(4)

2021-11-09 Thread Klemens Nanni
This populates `systat sensors' with the correct lid status on my
Pinebook Pro:

-gpio-key-lid at mainbus0 not configured
-gpio-key-power at mainbus0 not configured
+gpiokeys0 at mainbus0: "Lid"
+gpiokeys0 at mainbus0: "Power"

0/1 <-> closed/open is mapped exactly like acpibtn(4) already does on
other machines, to maintain consistent user experience across devices.

Next steps are:
- hook up lid status with `machdep.lidaction', see acpibtn(4)
- handle power button -> hook up machdep.pwraction, see acpibtn(4)


Sort gpio{led,charger,keys} while here.

Feedback? OK?


diff 83b213946bcaccd148d4a46b6ebda89b120fc778 refs/heads/master
blob - ab52b74cf7e9895cbcb29532b9898cdaf33e89f3
blob + 0891a80caf135b983dea282f281d7f6089b52e30
--- distrib/sets/lists/man/mi
+++ distrib/sets/lists/man/mi
@@ -1423,6 +1423,7 @@
 ./usr/share/man/man4/gpiocharger.4
 ./usr/share/man/man4/gpiodcf.4
 ./usr/share/man/man4/gpioiic.4
+./usr/share/man/man4/gpiokeys.4
 ./usr/share/man/man4/gpioleds.4
 ./usr/share/man/man4/gpioow.4
 ./usr/share/man/man4/graphaudio.4
blob - c32bc8962cd103da4ed17ad63a2162f63a16e639
blob + 6057d2a559120ada9f43ae0f5e5e6eb63a514be8
--- share/man/man4/Makefile
+++ share/man/man4/Makefile
@@ -37,7 +37,7 @@ MAN=  aac.4 abcrtc.4 abl.4 ac97.4 acphy.4 acrtc.4 \
fuse.4 fxp.4 \
gdt.4 gentbi.4 gem.4 gfrtc.4 gif.4 glenv.4 glkgpio.4 gpio.4 \
gpiocharger.4 gpiodcf.4 \
-   gpioiic.4 gpioleds.4 gpioow.4 graphaudio.4 gre.4 gscsio.4 \
+   gpioiic.4 gpiokeys.4 gpioleds.4 gpioow.4 graphaudio.4 gre.4 gscsio.4 \
hds.4 hiclock.4 hidwusb.4 hil.4 hilid.4 hilkbd.4 hilms.4 \
hireset.4 hitemp.4 hme.4 hotplug.4 hsq.4 \
hvn.4 hvs.4 hyperv.4 \
blob - /dev/null
blob + 44c147f04672fa1b72820ebd7692d7efbe17ceae (mode 644)
--- /dev/null
+++ share/man/man4/gpiokeys.4
@@ -0,0 +1,50 @@
+.\"$OpenBSD: $
+.\"
+.\" Copyright (c) 2021 Klemens Nanni 
+.\"
+.\" Permission to use, copy, modify, and distribute this software for any
+.\" purpose with or without fee is hereby granted, provided that the above
+.\" copyright notice and this permission notice appear in all copies.
+.\"
+.\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+.\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+.\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+.\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+.\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+.\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+.\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+.\"
+.Dd $Mdocdate: September 02 2021 $
+.Dt GPIOKEYS 4
+.Os
+.Sh NAME
+.Nm gpiokeys
+.Nd GPIO keys
+.Sh SYNOPSIS
+.Cd "gpiokeys* at fdt?"
+.Sh DESCRIPTION
+The
+.Nm
+driver handles events triggered by GPIO keys.
+Currently, only lid status events are supported.
+.Pp
+The lid status is set up as a sensor and can be monitored using
+.Xr sysctl 8
+or
+.Xr sensorsd 8 .
+.Sh SEE ALSO
+.Xr gpio 4 ,
+.Xr intro 4 ,
+.Xr sensorsd 8 ,
+.Xr sysctl 8
+.Sh HISTORY
+The
+.Nm
+driver first appeared in
+.Ox 7.1 .
+.Sh AUTHORS
+.An -nosplit
+The
+.Nm
+driver was written by
+.An Klemens Nanni Aq Mt k...@openbsd.org .
blob - 809ece860c07df596da208638f2f3facc829cb72
blob + 0ceac9b6159e5cfe43fc4e601274100df1c40ffa
--- sys/arch/arm64/conf/GENERIC
+++ sys/arch/arm64/conf/GENERIC
@@ -131,8 +131,9 @@ amdgpu* at pci?
 drm*   at amdgpu?
 wsdisplay* at amdgpu?
 
-gpioleds*  at fdt?
 gpiocharger*   at fdt?
+gpiokeys*  at fdt?
+gpioleds*  at fdt?
 
 # Apple
 apldart*   at fdt?
blob - 301500e9b0c6501be4d55feb5135ca223d6337cc
blob + 3d73e22edf7802172b29a26cb8b503eaeb8f3aa9
--- sys/dev/fdt/files.fdt
+++ sys/dev/fdt/files.fdt
@@ -589,10 +589,14 @@ devicedapmic
 attach dapmic at i2c
 file   dev/fdt/dapmic.cdapmic
 
-device gpioleds
-attach gpioleds at fdt
-file   dev/fdt/gpioleds.c  gpioleds
-
 device gpiocharger
 attach gpiocharger at fdt
 file   dev/fdt/gpiocharger.c   gpiocharger
+
+device gpioleds
+attach gpioleds at fdt
+file   dev/fdt/gpioleds.c  gpioleds
+
+device gpiokeys
+attach gpiokeys at fdt
+file   dev/fdt/gpiokeys.c  gpiokeys
blob - /dev/null
blob + 4b4ca55a5c825ad8d66fbc8ec198bbcd13352b7c (mode 644)
--- /dev/null
+++ sys/dev/fdt/gpiokeys.c
@@ -0,0 +1,179 @@
+/* $OpenBSD: $ */
+/*
+ * Copyright (c) 2021 Klemens Nanni 
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTW

Re: regress/rpki-client: test openssl in regress target only

2021-11-08 Thread Klemens Nanni
On Mon, Nov 08, 2021 at 12:59:51AM +0100, Theo Buehler wrote:
> > rpki-client seems to be the one-off under regress/ in this regard.
> 
> Maybe. I wish I had a better idea than this

There is:  pull the test into the proper Makefile as done in
regress/sys/net/inetinet/pmtu/Makefile and not
regress/sys/net/inetinet/Makefile -- openssl11 is added last to SUBDIR,
`make obj' does only the symlink, `make' compiles and tries regress but
prints the SKIPPED iff eopenssl11 isn't there, all at the end.

> Index: Makefile
> ===
> RCS file: /cvs/src/regress/usr.sbin/rpki-client/Makefile,v
> retrieving revision 1.10
> diff -u -p -r1.10 Makefile
> --- Makefile  9 Nov 2020 15:49:48 -   1.10
> +++ Makefile  7 Nov 2021 23:59:10 -
> @@ -1,7 +1,8 @@
>  # $OpenBSD: Makefile,v 1.10 2020/11/09 15:49:48 tb Exp $
>  
>  SUBDIR += libressl
> -.if exists(/usr/local/bin/eopenssl11)
> +.if exists(/usr/local/bin/eopenssl11) || \
> + make(obj) || make(clean) || make(cleandir)
>  SUBDIR += openssl11
>  .else
>  .END:
> 

OK?

Index: Makefile
===
RCS file: /cvs/src/regress/usr.sbin/rpki-client/Makefile,v
retrieving revision 1.10
diff -u -p -r1.10 Makefile
--- Makefile9 Nov 2020 15:49:48 -   1.10
+++ Makefile8 Nov 2021 22:42:43 -
@@ -1,13 +1,6 @@
 # $OpenBSD: Makefile,v 1.10 2020/11/09 15:49:48 tb Exp $
 
 SUBDIR += libressl
-.if exists(/usr/local/bin/eopenssl11)
 SUBDIR += openssl11
-.else
-.END:
-   @echo 'Run "pkg_add openssl--%1.1" to run tests against OpenSSL 1.1'
-   @echo SKIPPED
-.endif
 
 .include 
-.include 
Index: openssl11/Makefile
===
RCS file: /cvs/src/regress/usr.sbin/rpki-client/openssl11/Makefile,v
retrieving revision 1.4
diff -u -p -r1.4 Makefile
--- openssl11/Makefile  7 Oct 2021 10:34:39 -   1.4
+++ openssl11/Makefile  8 Nov 2021 22:42:42 -
@@ -1,3 +1,9 @@
+.if ! exists(/usr/local/bin/eopenssl11)
+regress:
+   @echo 'Run "pkg_add openssl--%1.1" to run tests against OpenSSL 1.1'
+   @echo SKIPPED
+.endif
+
 LDADD +=   -Wl,-rpath,/usr/local/lib/eopenssl11 -L/usr/local/lib/eopenssl11
 CFLAGS +=  -I${.CURDIR}/ -I/usr/local/include/eopenssl11/
 



Re: sppp(4)/pppoe(4) - DNS configuration via resolvd(8)

2021-11-08 Thread Klemens Nanni
On Mon, Nov 08, 2021 at 11:52:52AM +0100, Bjorn Ketelaars wrote:
> Diff below does two things:
> 1. add PPP IPCP extensions for name server addresses (rfc1877) to
>sppp(4)

You could add that RFC to sppp(4)'s STANDARDS section.

> 2. propose negotiated name servers from sppp(4) to resolvd(8) using
>RTM_PROPOSAL_STATIC route messages.

Needs mentioning in resolvd(8)'s sentence about umb(4).
unwind(8) should mention it as well (it does not even mention umb at
this point).

> With this I'm able to use DNS servers as provided by my ISP who uses
> PPPoE. resolv.conf is updated by resolvd(8) as function of status
> changes of pppoe(4).

Works as expected on octeon behind german VDSL2.

I use unbound(8) with a static 'nameserver ::1' entry in resolv.conf(5)
and resolvd(8) is running but does not get any DNS proposals, hence the
unbound entry wins.

With this diff pppoe(4) sends two nameservers and thus wins, but that is
expected and setups such as mine must either
- disable resolvd
- enable resolvd but also enable unwind
  (unwind also learns DNS proposals but always wins in resolv.conf)
- enable resolvd and clear pppoe(4) proposals,
  e.g. `route nameserver pppoe0'
- do whatever else fits their setup

> rfc1877 implementation is derived from code from NetBSD, and inspired by
> [0] and [1]. Borrowed code from umb(4) for the route messages.
> 
> Opinions/comments/OK?

> [0] https://marc.info/?l=openbsd-tech=134943767022961=2
> [1] https://marc.info/?l=openbsd-tech=159405677416423=2

I guess the ifconfig(8) bits to print proposed nameservers are still
useful, even nowadays where resolvd/unwind pick them up and/or print
them (`unwindctl status autoconf').


As per above, I'd like to improve the bigger picture, but this diff is
OK kn as-is;  see inline comments.

> diff --git sys/net/if_sppp.h sys/net/if_sppp.h
> index ff559fcc369..f2dab61e46b 100644
> --- sys/net/if_sppp.h
> +++ sys/net/if_sppp.h
> @@ -132,6 +132,8 @@ struct sipcp {
> * original one here, in network byte order */
>   u_int32_t req_hisaddr;  /* remote address requested (IPv4) */
>   u_int32_t req_myaddr;   /* local address requested (IPv4) */
> +#define IPCP_MAX_DNSSRV  2
> + u_int32_t dns[IPCP_MAX_DNSSRV]; /* IPv4  DNS servers (RFC 1877) */
>  #ifdef INET6
>   struct in6_aliasreq req_ifid;   /* local ifid requested (IPv6) */
>  #endif
> diff --git sys/net/if_spppsubr.c sys/net/if_spppsubr.c
> index ac1dc9a709d..225ad8f5a3e 100644
> --- sys/net/if_spppsubr.c
> +++ sys/net/if_spppsubr.c
> @@ -132,6 +132,10 @@
>  #define IPCP_OPT_ADDRESSES   1   /* both IP addresses; deprecated */
>  #define IPCP_OPT_COMPRESSION 2   /* IP compression protocol (VJ) */
>  #define IPCP_OPT_ADDRESS 3   /* local IP address */
> +#define IPCP_OPT_PRIMDNS 129 /* primary remote dns address */
> +#define _IPCP_OPT_PRIMDNS_   4   /* set/check option */
> +#define IPCP_OPT_SECDNS  131 /* secondary remote dns address 
> */
> +#define _IPCP_OPT_SECDNS_5   /* set/check option */

This _*_ looks ugly.  NetBSD has `IPCP_OPT_PRIMDNS 129' and
`SPPP_IPCP_OPT_PRIMDNS __BIT(3)' which we don't have.

Optimally, we could even out this difference, but I have not yet looked
closer into this.

>  #define IPV6CP_OPT_IFID  1   /* interface identifier */
>  #define IPV6CP_OPT_COMPRESSION   2   /* IPv6 compression protocol */
> @@ -338,6 +342,8 @@ void sppp_update_gw(struct ifnet *ifp);
>  void sppp_set_ip_addrs(void *);
>  void sppp_clear_ip_addrs(void *);
>  void sppp_set_phase(struct sppp *sp);
> +void sppp_update_dns(struct ifnet *ifp);
> +void sppp_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt);
>  
>  /* our control protocol descriptors */
>  static const struct cp lcp = {
> @@ -701,6 +707,7 @@ sppp_attach(struct ifnet *ifp)
>  
>   sp->pp_if.if_type = IFT_PPP;
>   sp->pp_if.if_output = sppp_output;
> + sp->pp_if.if_rtrequest = sppp_rtrequest;
>   ifq_set_maxlen(>pp_if.if_snd, 50);
>   mq_init(>pp_cpq, 50, IPL_NET);
>   sp->pp_loopcnt = 0;
> @@ -2519,6 +2526,12 @@ sppp_ipcp_RCN_rej(struct sppp *sp, struct lcp_header 
> *h, int len)
>   sp->ipcp.opts &= ~(1 << IPCP_OPT_COMPRESS);
>   break;
>  #endif
> + case IPCP_OPT_PRIMDNS:
> + sp->ipcp.opts &= ~(1 << _IPCP_OPT_PRIMDNS_);
> + break;
> + case IPCP_OPT_SECDNS:
> + sp->ipcp.opts &= ~(1 << _IPCP_OPT_SECDNS_);
> + break;
>   }
>   }
>   if (debug)
> @@ -2584,6 +2597,16 @@ sppp_ipcp_RCN_nak(struct sppp *sp, struct lcp_header 
> *h, int len)
>*/
>   break;
>  #endif
> + case IPCP_OPT_PRIMDNS:
> + if (len >= 6 && p[1] == 6)
> + sp->ipcp.dns[0] = p[2] << 24 | p[3] << 16 |
> + p[4] << 8 

Re: regress/rpki-client: test openssl in regress target only

2021-11-07 Thread Klemens Nanni
On Sun, Nov 07, 2021 at 11:33:29PM +0100, Theo Buehler wrote:
> On Sun, Nov 07, 2021 at 10:28:22PM +0000, Klemens Nanni wrote:
> > On Sun, Nov 07, 2021 at 11:16:57PM +0100, Theo Buehler wrote:
> > > On Sun, Nov 07, 2021 at 10:13:44PM +0000, Klemens Nanni wrote:
> > > > Spotted in `make obj' from /usr/src:
> > > > 
> > > > ===> regress/usr.sbin/rpki-client/libressl
> > > > /usr/src/regress/usr.sbin/rpki-client/libressl/obj -> 
> > > > /usr/obj/regress/usr.sbin/rpki-client/libressl
> > > > Run "pkg_add openssl--%1.1" to run tests against OpenSSL 1.1
> > > > SKIPPED
> > > > ===> regress/usr.sbin/snmpd
> > > > 
> > > > OK?
> > > 
> > > That won't print if you only type 'make' and don't OpenSSL installed.
> > 
> > It does for me:
> > 
> > $ cd /usr/src/regress/usr.sbin/rpki-client
> > $ make
> > Run "pkg_add openssl--%1.1" to run tests against OpenSSL 1.1
> > SKIPPED
> 
> The point of putting it at the end is that it stands out.

Fair point, although nothing but the symlink creation should happen
during `make obj'.

rpki-client seems to be the one-off under regress/ in this regard.



armv7 and arm64 media: fail on missing packages

2021-11-07 Thread Klemens Nanni
I built arm64 media on a new machine and forgot to install firmware
from packages.

It took me some digging to find out which packages these were, plus it
failed in the middle of the build rather than up-front.

While here, I noticed that arm64 does not use the "dtb" package like
armv7 does, but it still defines an unused variable.

This makes `make obj' run and not warn but `make' in e.g. the ramdisk
directory will immediately warn and not build anything.

$ cd /usr/src/distrib
$ make obj ; echo $?
...
0
$ cd arm64/ramdisk
$ doas make
Run "pkg_add u-boot-aarch64 raspberrypi-firmware"
*** Error 1 in /usr/src/distrib/arm64/ramdisk (Makefile:46 'all': 
@false)
$ doas pkg_add  u-boot-aarch64 raspberrypi-firmware
u-boot-aarch64-2021.10p1: ok
raspberrypi-firmware-1.20210527: ok
$ doas make ; echo $?
...
0


Feedback? Objections? OK?


diff 08275eee86493afb82b5297b2f958fa9eeaedf41 /usr/src
blob - 68c30398cea3fff2184278e14d0c9fbea2154938
file + distrib/arm64/iso/Makefile
--- distrib/arm64/iso/Makefile
+++ distrib/arm64/iso/Makefile
@@ -24,7 +24,6 @@ FFSSTART!=expr ${MSDOSSTART} + ${MSDOSSIZE}
 NEWFS_ARGS_msdos=-L boot -c1 -F16
 MOUNT_ARGS_msdos=-o-l
 
-PDTB=  /usr/local/share/dtb/arm64
 PUBOOT=/usr/local/share/u-boot
 PRPI=  /usr/local/share/raspberrypi-firmware/boot
 
@@ -45,6 +44,12 @@ PIFILES=\
 PIDTBO=\
disable-bt.dtbo
 
+.if !exists(${PUBOOT}/rpi_arm64) || !exists(${PRPI})
+all:
+   @echo 'Run "pkg_add u-boot-aarch64 raspberrypi-firmware"'
+   @false
+.endif
+
 all: ${FS}
 
 ${FS}: ${BASE} ${XBASE}
blob - 5dda462c0f42314e7bb21d264e814903440baa02
file + distrib/arm64/ramdisk/Makefile
--- distrib/arm64/ramdisk/Makefile
+++ distrib/arm64/ramdisk/Makefile
@@ -14,7 +14,6 @@ FFSSTART!=expr ${MSDOSSTART} + ${MSDOSSIZE}
 NEWFS_ARGS_msdos=-L boot -c1 -F16
 MOUNT_ARGS_msdos=-o-l
 
-PDTB=  /usr/local/share/dtb/arm64
 PUBOOT=/usr/local/share/u-boot
 PRPI=  /usr/local/share/raspberrypi-firmware/boot
 
@@ -41,6 +40,12 @@ PIFILES=\
 PIDTBO=\
disable-bt.dtbo
 
+.if !exists(${PUBOOT}/rpi_arm64) || !exists(${PRPI})
+all:
+   @echo 'Run "pkg_add u-boot-aarch64 raspberrypi-firmware"'
+   @false
+.endif
+
 all: ${FS}
 
 ${FS}: bsd.rd
blob - bb00f4360a80fa3e93e426e67eaa635a958d0168
file + distrib/armv7/miniroot/Makefile.inc
--- distrib/armv7/miniroot/Makefile.inc
+++ distrib/armv7/miniroot/Makefile.inc
@@ -14,6 +14,12 @@ FFSSTART!=   expr ${MSDOSSTART} + ${MSDOSSIZE}
 PDTB=  /usr/local/share/dtb/arm
 PUBOOT=/usr/local/share/u-boot
 
+.if !exists(${PDTB}) || !exists(${PUBOOT}/wandboard)
+all:
+   @echo 'Run "pkg_add dtb u-boot-arm"'
+   @false
+.endif
+
 all: ${FS}
 
 ${FS}: bsd.rd



Re: regress/rpki-client: test openssl in regress target only

2021-11-07 Thread Klemens Nanni
On Sun, Nov 07, 2021 at 11:16:57PM +0100, Theo Buehler wrote:
> On Sun, Nov 07, 2021 at 10:13:44PM +0000, Klemens Nanni wrote:
> > Spotted in `make obj' from /usr/src:
> > 
> > ===> regress/usr.sbin/rpki-client/libressl
> > /usr/src/regress/usr.sbin/rpki-client/libressl/obj -> 
> > /usr/obj/regress/usr.sbin/rpki-client/libressl
> > Run "pkg_add openssl--%1.1" to run tests against OpenSSL 1.1
> > SKIPPED
> > ===> regress/usr.sbin/snmpd
> > 
> > OK?
> 
> That won't print if you only type 'make' and don't OpenSSL installed.

It does for me:

$ cd /usr/src/regress/usr.sbin/rpki-client
$ make
Run "pkg_add openssl--%1.1" to run tests against OpenSSL 1.1
SKIPPED
===> libressl
cc -O2 -pipe  -I/usr/src/regress/usr.sbin/rpki-client/libressl/.. 
-I/usr/src/regress/usr.sbin/rpki-client/libressl/../../../../usr.sbin/rpki-client
  -MD -MP  -c 
/usr/src/regress/usr.sbin/rpki-client/libressl/../../../../usr.sbin/rpki-client/cert.c
...

Adding to the regress target is what most (if not all) other regress
tests seem to do, e.g.

$ head /usr/src/regress/sys/netinet/pmtu/Makefile
#   $OpenBSD: Makefile,v 1.15 2020/12/30 21:40:33 kn Exp $

# The following ports must be installed:
#
# scapy   powerful interactive packet manipulation in python

.if ! exists(/usr/local/bin/scapy)
regress:
@echo Install scapy package to run this regress.
@echo SKIPPED


Am I missing something?



regress/rpki-client: test openssl in regress target only

2021-11-07 Thread Klemens Nanni
Spotted in `make obj' from /usr/src:

===> regress/usr.sbin/rpki-client/libressl
/usr/src/regress/usr.sbin/rpki-client/libressl/obj -> 
/usr/obj/regress/usr.sbin/rpki-client/libressl
Run "pkg_add openssl--%1.1" to run tests against OpenSSL 1.1
SKIPPED
===> regress/usr.sbin/snmpd

OK?

Index: Makefile
===
RCS file: /cvs/src/regress/usr.sbin/rpki-client/Makefile,v
retrieving revision 1.10
diff -u -p -r1.10 Makefile
--- Makefile9 Nov 2020 15:49:48 -   1.10
+++ Makefile7 Nov 2021 22:11:52 -
@@ -4,7 +4,7 @@ SUBDIR += libressl
 .if exists(/usr/local/bin/eopenssl11)
 SUBDIR += openssl11
 .else
-.END:
+regress:
@echo 'Run "pkg_add openssl--%1.1" to run tests against OpenSSL 1.1'
@echo SKIPPED
 .endif



Re: ftp: Print actually requested URLs

2021-11-06 Thread Klemens Nanni
On Sat, Nov 06, 2021 at 07:02:39PM +, Stuart Henderson wrote:
> On 2021/11/06 17:29, Klemens Nanni wrote:
> > Encoding URL paths changes the requested URL and therefore may yield
> > different responses (opposed to an unencoded URL), solely depending on
> > how the server implements de/encoding.
> 
> Makes sense as this matches what various other tools that fetch URLs do
> for printing. Or another approach would be to not output the URL at all
> (it's still visible with -d).

I explicitly didn't want to change ftp's default log-level, that's
another discussion, imho.



Re: better audio defaults: please test

2021-11-06 Thread Klemens Nanni
On Thu, Nov 04, 2021 at 04:21:12PM +0100, Alexandre Ratchov wrote:
> The current sndiod latency (minimum time between when the program
> plays something and when sound reaches Joe's ears) is too large and
> makes OpenBSD unpleasant to use for telephony, games, and makes
> controls of video players slugish.
> 
> The defaut latency (of 160ms) was set ~10 years ago to workaround
> various problems: KERNEL_LOCK used to block audio processing for very
> long, azalia(4) and uaudio(4) were unable to recover after an error,
> which aggravated the problem.
> 
> The kernel improved a lot the last decade and such large buffers are
> not necessary anymore. I think something between 20ms and 40ms is a
> better default for the average OpenBSD system:
> 
>  * audio-conferencing software and games requires no sndiod_flags
>tweaks anymore
> 
>  * on modern machines (like my 7 years old i5-2500K) building a kernel
>doesn't make audio stutterer
> 
>  * sndiod_flags tweaks will still be needed for:
>   - very slow or overloaded machines used for audio
>   - machines running heavy/bogus SMM code
>   - real-time synths & effects (20ms is still too small)
> 
> Please try to switch you system to 40ms buffers (i.e. 1920 samples at
> the default 48kHz rate), for instance either apply diff below or
> simply do:
> 
>   rcctl set sndiod flags -z 480 -b 1920
>   rcctl restart sndiod

This introduces noticable stuttering on my X230 runs a few xterms and
firefox with idle tabs.

The one thing I always use for testing audio is `pkg_add -u' while
playing `mpv songXY.ogg':  whether or not it installs pacakges does not
matter.  A noop run is enough to notice stuffer with this diff while the
current sndio defaults do not produce any stutter.

> then report any significant increase of stuttering, and what
> software/hardware triggers it. If you think 20ms or 30ms (i.e. 960 and
> 1440 sample buffers) are better, let me know as well.

Trying above rcctl lines with `-b 1440' and `-b 960' made it worse:
more stutter and noticably slower sound playback, in increasing order.

^Z the pkg_add process restores audio to normal. `fg' it brings stutter
back, i.e. without `pkg_add' running I hear no difference using any of
the three values.

> Note that OpenBSD is not real-time (neither are programs we run) so
> audio may stutter no matter how large the buffers are. The goal here
> is to get a ballance between disconfort caused by latency and
> probability of stuttering for the average OpenBSD system.
> 
> If we reach a consensus, here's the diff to make above settings the
> default.
> 
> OK?
> 
> Index: sndiod.c
> ===
> RCS file: /cvs/src/usr.bin/sndiod/sndiod.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 sndiod.c
> --- sndiod.c  1 Nov 2021 14:43:25 -   1.47
> +++ sndiod.c  1 Nov 2021 15:28:37 -
> @@ -82,7 +82,7 @@
>   * buffer size if neither ``-z'' nor ``-b'' is used
>   */
>  #ifndef DEFAULT_BUFSZ
> -#define DEFAULT_BUFSZ7680
> +#define DEFAULT_BUFSZ1920
>  #endif
>  
>  void sigint(int);
> 

So at least for my setup this is not an improvement.

I quickly tried the mid-value between 7680 and 1920, 4800, and with that
I hear no stutter while `pkg_add' is running!

Smaller values probably work but I didn't bisect it to the lowest
possible value.



Re: ftp: Print actually requested URLs

2021-11-06 Thread Klemens Nanni
On Sat, Nov 06, 2021 at 11:33:21AM -0600, Theo de Raadt wrote:
> > This matches exactly what is seen on the wire, e.g. with tshark(1).
> 
> I don't see why this is important.  Users don't need to see what is
> on the wire.
> 
> Why intentionaly expose them to a translation they are not supposed
> to know or care about?

Because I've run into a few encoding issues already (the last one being
the RFC update the I just committed) and I'd very much prefer accurate
information on such matter.

Take the example that led to the tilde fix:  prior to the last commit
ftp still encoded "~" to "%7e" and that's what got me a 404 instead of
an expected 200.

Here's ftp(1) without the tilde fix and without this diff:

$ ftp 
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download
Trying 64:ff9b::339f:1211...
Requesting 
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download
ftp: Error retrieving 
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download:
 404 Not Found

Neither did it request the URL with "~" nor did it get a 404 for the one
with "~" -- the output is plain wrong and I have to turn on `-d' to see
what's really going on.

Now with this diff applied, I do get the truth:

$ ./obj/ftp 
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download
Trying 64:ff9b::339f:1211...
Requesting 
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a%7e7726/revisions/9/patch?download
ftp: Error retrieving 
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a%7e7726/revisions/9/patch?download:
 404 Not Found

I feed it "~" but ftp encodes it as "%7e" and the server cannot serve
"%7e".  With this important difference between user input and HTTP
request I can reason about en/decoding issues *without having to
distrust* my tool's output, poking at debug logs and/or looking at the
source.



ftp: Print actually requested URLs

2021-11-06 Thread Klemens Nanni
Encoding URL paths changes the requested URL and therefore may yield
different responses (opposed to an unencoded URL), solely depending on
how the server implements de/encoding.

Thus it is imperative to inform users about the factually requested URL
in case servers behave unexpectedly.  Consider this example:

$ ftp http://openbsd.org/[
Trying 64:ff9b::8180:5c2...
Requesting http://openbsd.org/[
Redirected to https://www.openbsd.org/[
Trying 64:ff9b::8180:5c2...
Requesting https://www.openbsd.org/[
ftp: Error retrieving https://www.openbsd.org/[: 404 Not Found

ftp(1) actually encodes "[" into "%5b" before the request, but it
pretends to have sent "/[" in the GET request as per its output.


Fix this and always print exactly what was requested to help users debug
(server related) de/encoding issues:

./obj/ftp http://openbsd.org/[
Trying 64:ff9b::8180:5c2...
Requesting http://openbsd.org/%5b
Redirected to https://www.openbsd.org/[
Trying 64:ff9b::8180:5c2...
Requesting https://www.openbsd.org/%5b
ftp: Error retrieving https://www.openbsd.org/%5b: 404 Not Found

This matches exactly what is seen on the wire, e.g. with tshark(1).

In this case of openbsd.org, the server's 301 Moved Permanently's
Location header does contain the decoded path "/[", so that output is
indeed correct and ftp encodes it again as expected before following it.

Other servers, e.g. ccc.de, respont with 302 Moved Temporarily and retain
the decoded URL path:

$ ./obj/ftp http://ccc.de/[
Trying 2001:67c:20a0:2:0:164:0:39...
Requesting http://ccc.de/%5b
Redirected to https://www.ccc.de/%5b
Trying 2001:67c:20a0:2:0:164:0:39...
Requesting https://www.ccc.de/%5b
ftp: Error retrieving https://www.ccc.de/%5b: 404 Not Found

ftp follows the Location header but has nothing to (re)encode and with
this diff above about also matches exactly what is seen on the wire.

Feeback? Objections? OK?
NB:  I'd do media tests before committing since ftp grows a little and
this code lands in the SMALL version as well.

Index: fetch.c
===
RCS file: /cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.206
diff -u -p -r1.206 fetch.c
--- fetch.c 6 Nov 2021 14:27:45 -   1.206
+++ fetch.c 6 Nov 2021 16:58:14 -
@@ -320,6 +320,7 @@ url_get(const char *origline, const char
int isunavail = 0, retryafter = -1;
struct addrinfo hints, *res0, *res;
const char *savefile;
+   char *eurl = NULL;
char *pathbuf = NULL;
char *proxyurl = NULL;
char *credentials = NULL, *proxy_credentials = NULL;
@@ -713,10 +714,17 @@ noslash:
 #endif /* !NOSSL */
 
epath = url_encode(path);
+   if (asprintf(, "%s%s%s%s/%s",
+   scheme,
+   full_host,
+   portnum ? ":" : "",
+   portnum ? portnum : "",
+   epath) == -1)
+   errx(1, "Cannot build encoded URL");
if (proxyurl) {
if (verbose) {
fprintf(ttyout, "Requesting %s (via %s)\n",
-   origline, proxyurl);
+   eurl, proxyurl);
}
/*
 * Host: directive must use the destination host address for
@@ -735,7 +743,7 @@ noslash:
ftp_printf(fin, "\r\n");
} else {
if (verbose)
-   fprintf(ttyout, "Requesting %s\n", origline);
+   fprintf(ttyout, "Requesting %s\n", eurl);
 #ifndef SMALL
if (resume || timestamp) {
if (stat(savefile, ) == 0) {
@@ -833,7 +841,7 @@ noslash:
status = strtonum(ststr, 200, 503, );
if (errstr) {
strnvis(gerror, cp, sizeof gerror, VIS_SAFE);
-   warnx("Error retrieving %s: %s", origline, gerror);
+   warnx("Error retrieving %s: %s", eurl, gerror);
goto cleanup_url_get;
}
 
@@ -879,7 +887,7 @@ noslash:
break;
default:
strnvis(gerror, cp, sizeof gerror, VIS_SAFE);
-   warnx("Error retrieving %s: %s", origline, gerror);
+   warnx("Error retrieving %s: %s", eurl, gerror);
goto cleanup_url_get;
}
 
@@ -1012,7 +1020,7 @@ noslash:
if (isunavail) {
if (retried || retryafter != 0)
warnx("Error retrieving %s: 503 Service Unavailable",
-   origline);
+   eurl);
else {
if (verbose)
fprintf(ttyout, "Retrying %s\n", origline);
@@ -1134,6 +1142,7 @@ improper:
warnx("Improper response from %s", host);
 
 cleanup_url_get:
+   free(eurl);
 #ifndef SMALL
free(full_host);
 #endif 

Re: new site.8: document site*.tgz and /{upgrade,install}.site

2021-11-05 Thread Klemens Nanni
On Fri, Nov 05, 2021 at 05:36:48PM -0700, Andrew Hewus Fresh wrote:
> I like it, some comments in-line but overall I think this would have
> helped me get started with siteXX stuff, so OK afresh1@

Great, I'll commit with tweaks as per below, thanks!

> > +If existent and executable,
> 
> This whole sentence reads weird to me.  I don't know if this is better,
> but it confused me.
> 
>  If they exist and are executable at the end of the install or
>  upgrade process, /install.site or /upgrade.site respectively, are
>  run with chroot(8) based at the system's root.
> 
> > +.Pa /install.site
> > +and
> > +.Pa /upgrade.site
> > +are run last during install and upgrade, respectively, with
> > +.Xr chroot 8
> > +based at the system's root.

I'll adapt this to

If they exist and are executable, /install.site and /upgrade.site are
run at the end of the install and upgrade process, respectively, with
chroot(8) based at the system's root.

> > +.Sh FILES
> > +.Bl -tag -width "site${VERSION}-$(hostname -s).tgz" -compact
> > +.It Pa site${ Ns Va VERSION Ns }.tgz
> > +Generic set.
> > +.It Pa site${ Ns Va VERSION Ns }-$( Ns Ic hostname Fl s Ns ).tgz
> > +Host-specific set.
> > +.It Pa /upgrade.site
> > +Generic post-upgrade script.
> > +.It Pa /install.site
> > +Generic post-install script.
> > +.El
> > +.Sh EXAMPLES
> > +Create
> > +.Nm
> > +sets and update the index:
> > +.Bd -literal -offset indent
> > +# tar -czhf site70.tgz generic/
> > +# tar -czhf site70-puffy.tgz puffy/
> > +# ls -lT > index.txt
> > +.Ed
> > +.Pp
> > +Trigger an upgrade of
> > +.Xr packages 7
> > +upon next boot:
> > +.Bd -literal -offset indent
> > +echo 'pkg_add -Iu' >>/etc/rc.firsttime
> 
> I think some clarity that this should go into one of those siteXX.tgz
> files as "/upgrade.site" would help folks connect how to get those files
> onto the install media and make it clearer that those files get
> extracted into the chroot and that you don't have to figure out how to
> get them into the root of the install media.

That's one way and certainly the only one for fresh installs (unless
you modify the install media, which seems pointless), but for upgrades
you can create /upgrade.site and the installer will pick it up from
there.

That is,

cat << EOF >/upgrade.site
echo 'pkg_add -Iu' >>/etc/rc.firsttime
EOF
chmod +x /upgrade.site
sysupgrade

will upgrade packages after sysupgrade reboots -- no need for dealing
with siteXX.tgz on your existing install.

But you're right, I completely missed creating /upgrade.site in that
example, so I'll fix that.

> Perhaps that is clear enough from "run [...] with chroot", but I think
> this example makes things less clear, at least the way it is.

So I'll leave that for now.  Again, we can polish in-tree.



ftp: do not URI encode tilde as per RFC 2396

2021-11-05 Thread Klemens Nanni
ftp(1) implements RFC 1738 from Dec. 1994 but RFC 2396 from Aug. 1998
updated this and the tl;dr is:  do not encode the tilde character.

In theory, this shouldn't make a difference as servers should decode
"%7e" to "~", BUT not all servers do so and thus some respond with 404.

I've hit this in the past already but didn't look at the code.
Now I came across this link that made me fix it:
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download

curl(1) and wget(1) both fetch this file successfully, i.e. they behave
identicall wrt. "%" and send it as-is:

$ 
url='https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download'

$ curl -I -sS -w '%{url}\n' "$url" | tail -1
200 
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download

$ wget -d "$url" 2>&1 | grep -Ew 'GET|OK'
GET /changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download 
HTTP/1.1
HTTP/1.1 200 OK
200 OK

ftp(1) still sticks with RFC 1738 and encodes it:

ftp -d "$url"
host review.trustedfirmware.org, port https, path 
changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download, save as 
patch?download, auth none.
Trying 64:ff9b::339f:1211...
Requesting 
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download
GET 
/changes/TF-A%2Ftrusted-firmware-a%7e7726/revisions/9/patch?download HTTP/1.1
Connection: close
Host: review.trustedfirmware.org
User-Agent: OpenBSD ftp

received 'HTTP/1.1 404 Not Found'
ftp: Error retrieving 
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download:
 404 Not Found

RFC 2396 2.4.2. When to Escape and Unescape says:

   In some cases, data that could be represented by an unreserved
   character may appear escaped; for example, some of the unreserved
   "mark" characters are automatically escaped by some systems.  If the
   given URI scheme defines a canonicalization algorithm, then
   unreserved characters may be unescaped according to that algorithm.
   For example, "%7e" is sometimes used instead of "~" in an http URL
   path, but the two are equivalent for an http URL.

So they're equivalent and tilde does not need encoding.  Again, servers
should always decode it anyway, but not all of them do, so better send
it as-is/unencoded.

Diff below is effectively a one-character change that removes "~" from
`*unsafe_chars' but it also updates the comments to use RFC 2396 wording
and order of characters for easier code-to-standard comparison.

This allows me to fetch this patch:

$ ./obj/ftp -d "$url"
host review.trustedfirmware.org, port https, path 
changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download, save as 
patch?download, auth none.
Trying 64:ff9b::339f:1211...
Requesting 
https://review.trustedfirmware.org/changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download
GET /changes/TF-A%2Ftrusted-firmware-a~7726/revisions/9/patch?download 
HTTP/1.1
Connection: close
Host: review.trustedfirmware.org
User-Agent: OpenBSD ftp

received 'HTTP/1.1 200 OK'
received 'Date: Fri, 05 Nov 2021 23:34:49 GMT'
received 'Server: Apache'
received 'X-Frame-Options: DENY'
received 'Content-Disposition: attachment; 
filename="155911c.diff.base64"'
received 'X-Content-Type-Options: nosniff'
received 'Cache-Control: private'
received 'Pragma: no-cache'
received 'Expires: Mon, 01 Jan 1990 00:00:00 GMT'
received 'X-FYI-Content-Encoding: base64'
received 'X-FYI-Content-Type: application/mbox'
received 'Content-Type: text/plain;charset=iso-8859-1'
received 'Vary: Accept-Encoding'
received 'Connection: close'
received 'Transfer-Encoding: chunked'
9544 bytes received in 0.00 seconds (12.21 MB/s)


Looking at wget's source somewhat backs up this way of handling tilde
by explicitly mentioning broken "%7e" decoding, i.e. servers expecting
an unencoded "~" as-is:

>From wget-1.2.1/src/url.c 103f:

/* Table of "reserved" and "unsafe" characters.  Those terms are
   rfc1738-speak, as such largely obsoleted by rfc2396 and later
   specs, but the general idea remains.

   A reserved character is the one that you can't decode without
   changing the meaning of the URL.  For example, you can't decode
   "/foo/%2f/bar" into "/foo///bar" because the number and contents of
   path components is different.  Non-reserved characters can be
   changed, so "/foo/%78/bar" is safe to change to "/foo/x/bar".  The
   unsafe characters are loosely based on rfc1738, plus "$" and ",",
   as recommended by rfc2396, and minus "~", which is very frequently
   used (and sometimes unrecognized as %7E by 

Re: new site.8: document site*.tgz and /{upgrade,install}.site

2021-11-05 Thread Klemens Nanni
On Wed, Oct 27, 2021 at 07:35:28PM -0500, Aaron Poffenberger wrote:
> Looks good. Nice to see this moving forward. Thanks.
> 
> On 2021-10-27 21:13 +, Klemens Nanni  wrote:
> > On Mon, Sep 06, 2021 at 02:29:50PM -0500, Aaron Poffenberger wrote:
> > > Ping.
> > > 
> > > Will someone commit this? Seems like no one knows about /upgrade.site and 
> > > it
> > > fits well with sysupgrade(8).
> > 
> > sysupgrade(8) is one potential /upgrade.site user but I disagree about
> > documenting the latter through the former.
> > 
> > Here is a new manual roughly merging the FAQ bits with what you, Aaron,
> > provided.
> > 
> > site(8) is a lame but fitting name;  autoinstall(8) and sysypgrade(8)
> > reference it and both sets and scripts are documented.
> > 
> > Examples are intentionally brief to be shorter and more concise than the
> > FAQ while showing enough to get going.
> > 
> > 
> > I am quite certain that wording, examples and the references from other
> > manuals can be tweaked, but this diff looks like a good enough start
> > and --if this is the way to go-- I'd prefer to commit and keep polishing
> > in-tree.
> > 
> > Feedback? Objections? OK?

I have not received any feedback on this except from Aaron.

Anyone? Anything?

Index: distrib/sets/lists/man/mi
===
RCS file: /cvs/src/distrib/sets/lists/man/mi,v
retrieving revision 1.1645
diff -u -p -r1.1645 mi
--- distrib/sets/lists/man/mi   2 Nov 2021 22:07:33 -   1.1645
+++ distrib/sets/lists/man/mi   3 Nov 2021 02:26:16 -
@@ -2561,6 +2561,7 @@
 ./usr/share/man/man8/security.8
 ./usr/share/man/man8/sendmail.8
 ./usr/share/man/man8/sensorsd.8
+./usr/share/man/man8/site.8
 ./usr/share/man/man8/sftp-server.8
 ./usr/share/man/man8/showmount.8
 ./usr/share/man/man8/shutdown.8
Index: usr.sbin/sysupgrade/sysupgrade.8
===
RCS file: /cvs/src/usr.sbin/sysupgrade/sysupgrade.8,v
retrieving revision 1.10
diff -u -p -r1.10 sysupgrade.8
--- usr.sbin/sysupgrade/sysupgrade.83 Oct 2019 12:43:58 -   1.10
+++ usr.sbin/sysupgrade/sysupgrade.827 Oct 2021 20:42:26 -
@@ -67,6 +67,10 @@ This is the default if the system is cur
 Upgrade to a snapshot.
 This is the default if the system is currently running a snapshot.
 .El
+.Pp
+See
+.Xr site 8
+for how to customize the upgrade process.
 .Sh FILES
 .Bl -tag -width "/auto_upgrade.conf" -compact
 .It Pa /auto_upgrade.conf
@@ -83,7 +87,8 @@ Directory the upgrade is downloaded to.
 .Xr signify 1 ,
 .Xr installurl 5 ,
 .Xr autoinstall 8 ,
-.Xr release 8
+.Xr release 8 ,
+.Xr site 8
 .Sh HISTORY
 .Nm
 first appeared in
Index: share/man/man8/Makefile
===
RCS file: /cvs/src/share/man/man8/Makefile,v
retrieving revision 1.102
diff -u -p -r1.102 Makefile
--- share/man/man8/Makefile 1 May 2021 16:11:10 -   1.102
+++ share/man/man8/Makefile 27 Oct 2021 20:38:11 -
@@ -6,7 +6,7 @@ MAN=afterboot.8 autoinstall.8 boot_conf
crash.8 daily.8 \
diskless.8 genassym.sh.8 intro.8 netstart.8 rc.8 \
rc.conf.8 rc.d.8 rc.shutdown.8 rc.subr.8 release.8 \
-   security.8 ssl.8 starttls.8 sticky.8 yp.8
+   security.8 site.8 ssl.8 starttls.8 sticky.8 yp.8
 
 SUBDIR=man8.alpha man8.amd64 man8.arm64 man8.armv7 \
man8.hppa man8.i386 man8.landisk \
Index: share/man/man8/autoinstall.8
===
RCS file: /cvs/src/share/man/man8/autoinstall.8,v
retrieving revision 1.23
diff -u -p -r1.23 autoinstall.8
--- share/man/man8/autoinstall.818 Jul 2021 11:08:34 -  1.23
+++ share/man/man8/autoinstall.827 Oct 2021 20:42:24 -
@@ -32,6 +32,10 @@ file and HTTP to fetch the file.
 If that fails, the installer asks for the location which can either be
 a URL or a local path.
 .Pp
+See
+.Xr site 8
+for how to provide custom configuration.
+.Pp
 To start unattended installation or upgrade choose '(A)utoinstall' at the
 install prompt.
 If there is only one network interface, the installer fetches the response
@@ -235,7 +239,8 @@ host foo {
 .Sh SEE ALSO
 .Xr dhcp-options 5 ,
 .Xr dhcpd.conf 5 ,
-.Xr diskless 8
+.Xr diskless 8 ,
+.Xr site 8
 .Sh HISTORY
 The
 .Nm
Index: share/man/man8/site.8
===
RCS file: share/man/man8/site.8
diff -N share/man/man8/site.8
--- /dev/null   1 Jan 1970 00:00:00 -
+++ share/man/man8/site.8   27 Oct 2021 21:11:48 -
@@ -0,0 +1,87 @@
+.\" $OpenBSD: $
+.\"
+.\" Copyright (c) 2021 Klemens Nanni 
+.\"
+.\" Permission to use, copy, modify, and distribute this software for any
+.\" purp

Re: amd4/bsd.rd: make "config -e" work

2021-11-04 Thread Klemens Nanni
On Thu, Nov 04, 2021 at 10:49:46PM +, Klemens Nanni wrote:
> amd64, alpha, i386 and macppc strip all symbols off the ramdisk bsd.rd
> (before gzipping it) and thus break config(8)'s modification feature:
> 
>   $ gzcat bsd.rd > bsd.rd.raw
>   $ config -e bsd.rd.raw
>   WARNING no output file specified
>   WARNING this kernel doesn't contain all information needed!
>   WARNING the commands add and change might not work.
>   WARNING this kernel doesn't support pseudo devices.
>   WARNING this kernel doesn't support modification of NKMEMPAGES.
>   config: failed to get first cfdata
> 
> Needing 'disable xhci' on arm64/Pinebook Pro right now, I looked into
> all of bsd.re-config(5), UKC and how we build bsd.rd for all platforms.
> 
> That's how I noticed modifying a ramdisk kernel on these for platforms
> wouldn't work in the first place.
> 
> Needing it to fix or upgrade systems can be crucial, so I figured it's
> worth addressing.
> 
> While `boot /bsd.rd -c' does work on amd64 and macppc regardless of
> symbols, its changes do not persist and `config -e' is more convenient.
> 
> So I propose to strip less symbols such to make this work.
> 
> On amd64, this accounts for 200K growth in bsd.gz according to `du -k':
> 
>   -9144   obj/bsd.strip
>   +9840   obj/bsd.strip
>   -4264   obj/bsd.gz
>   +4464   obj/bsd.gz
> 
> I cranked FSSIZE to the smallest possible volume that would build.
> The resulting miniroot70.img boots fine in vmm(4) and on a X250.
> 
> macppc is still building;  I have no alpha or i386.

macppc builds and boots on a Mac without FSSIZE cranking and I can edit
it with `config -e' as expected.

> Feedback? Objections? OK?


Index: amd64/ramdisk_cd/Makefile
===
RCS file: /cvs/src/distrib/amd64/ramdisk_cd/Makefile,v
retrieving revision 1.31
diff -u -p -r1.31 Makefile
--- amd64/ramdisk_cd/Makefile   26 Jul 2021 12:47:45 -  1.31
+++ amd64/ramdisk_cd/Makefile   4 Nov 2021 18:42:51 -
@@ -1,7 +1,7 @@
 #  $OpenBSD: Makefile,v 1.31 2021/07/26 12:47:45 kn Exp $
 
 FS=miniroot${OSrev}.img
-FSSIZE=9920
+FSSIZE=10368
 FSDISKTYPE=mini34
 CDROM= cd${OSrev}.iso
 MOUNT_POINT=   /mnt
@@ -56,7 +56,7 @@ MRDISKTYPE=   rdrootb
 MRMAKEFSARGS=  -o disklabel=${MRDISKTYPE},minfree=0,density=4096
 
 bsd.gz: bsd.rd
-   objcopy -S -R .comment -R .SUNW_ctf \
+   objcopy -g -x -R .comment -R .SUNW_ctf \
-K rd_root_size -K rd_root_image \
bsd.rd bsd.strip
gzip -9cn bsd.strip > bsd.gz
Index: macppc/ramdisk/Makefile
===
RCS file: /cvs/src/distrib/macppc/ramdisk/Makefile,v
retrieving revision 1.51
diff -u -p -r1.51 Makefile
--- macppc/ramdisk/Makefile 26 Jul 2021 12:47:46 -  1.51
+++ macppc/ramdisk/Makefile 5 Nov 2021 00:02:12 -
@@ -35,7 +35,7 @@ bsd.gz: bsd.rd
gzip -9cn bsd.rd > bsd.gz
 
 bsd.rd: mr.fs bsd
-   objcopy -S -R .comment -R .SUNW_ctf \
+   objcopy -g -x -R .comment -R .SUNW_ctf \
-K rd_root_size -K rd_root_image \
bsd bsd.rd
rdsetroot bsd.rd mr.fs



amd4/bsd.rd: make "config -e" work

2021-11-04 Thread Klemens Nanni
amd64, alpha, i386 and macppc strip all symbols off the ramdisk bsd.rd
(before gzipping it) and thus break config(8)'s modification feature:

$ gzcat bsd.rd > bsd.rd.raw
$ config -e bsd.rd.raw
WARNING no output file specified
WARNING this kernel doesn't contain all information needed!
WARNING the commands add and change might not work.
WARNING this kernel doesn't support pseudo devices.
WARNING this kernel doesn't support modification of NKMEMPAGES.
config: failed to get first cfdata

Needing 'disable xhci' on arm64/Pinebook Pro right now, I looked into
all of bsd.re-config(5), UKC and how we build bsd.rd for all platforms.

That's how I noticed modifying a ramdisk kernel on these for platforms
wouldn't work in the first place.

Needing it to fix or upgrade systems can be crucial, so I figured it's
worth addressing.

While `boot /bsd.rd -c' does work on amd64 and macppc regardless of
symbols, its changes do not persist and `config -e' is more convenient.

So I propose to strip less symbols such to make this work.

On amd64, this accounts for 200K growth in bsd.gz according to `du -k':

-9144   obj/bsd.strip
+9840   obj/bsd.strip
-4264   obj/bsd.gz
+4464   obj/bsd.gz

I cranked FSSIZE to the smallest possible volume that would build.
The resulting miniroot70.img boots fine in vmm(4) and on a X250.

macppc is still building;  I have no alpha or i386.


Feedback? Objections? OK?

Index: Makefile
===
RCS file: /cvs/src/distrib/amd64/ramdisk_cd/Makefile,v
retrieving revision 1.31
diff -u -p -r1.31 Makefile
--- Makefile26 Jul 2021 12:47:45 -  1.31
+++ Makefile4 Nov 2021 18:42:51 -
@@ -1,7 +1,7 @@
 #  $OpenBSD: Makefile,v 1.31 2021/07/26 12:47:45 kn Exp $
 
 FS=miniroot${OSrev}.img
-FSSIZE=9920
+FSSIZE=10368
 FSDISKTYPE=mini34
 CDROM= cd${OSrev}.iso
 MOUNT_POINT=   /mnt
@@ -56,7 +56,7 @@ MRDISKTYPE=   rdrootb
 MRMAKEFSARGS=  -o disklabel=${MRDISKTYPE},minfree=0,density=4096
 
 bsd.gz: bsd.rd
-   objcopy -S -R .comment -R .SUNW_ctf \
+   objcopy -g -x -R .comment -R .SUNW_ctf \
-K rd_root_size -K rd_root_image \
bsd.rd bsd.strip
gzip -9cn bsd.strip > bsd.gz



distrib/CDs: typofix in application ID

2021-11-04 Thread Klemens Nanni
All volume IDs use a dash and now I can't unsee it missing.

OK?

Index: alpha/miniroot/Makefile
===
RCS file: /cvs/src/distrib/alpha/miniroot/Makefile,v
retrieving revision 1.23
diff -u -p -r1.23 Makefile
--- alpha/miniroot/Makefile 26 Jul 2021 12:47:44 -  1.23
+++ alpha/miniroot/Makefile 4 Nov 2021 18:45:35 -
@@ -46,7 +46,7 @@ ${CDROM}: bsd.gz
cp ${DESTDIR}/usr/mdec/bootxx ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}
 
(mkhybrid -a -R -T -L -d -D -N -o ${.OBJDIR}/${CDROM} -v -v -v \
-   -A "OpenBSD ${OSREV} ${MACHINE} bootonly CD" \
+   -A "OpenBSD ${OSREV} ${MACHINE} boot-only CD" \
-P "Copyright (c) `date +%Y` Theo de Raadt, The OpenBSD project" \
-p "Theo de Raadt " \
-V "OpenBSD/${MACHINE}   ${OSREV} boot-only CD" \
Index: amd64/ramdisk_cd/Makefile
===
RCS file: /cvs/src/distrib/amd64/ramdisk_cd/Makefile,v
retrieving revision 1.31
diff -u -p -r1.31 Makefile
--- amd64/ramdisk_cd/Makefile   26 Jul 2021 12:47:45 -  1.31
+++ amd64/ramdisk_cd/Makefile   4 Nov 2021 18:42:51 -
@@ -45,7 +45,7 @@ ${CDROM}: bsd.rd
cp ${DESTDIR}/usr/mdec/cdbr ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}
cp ${DESTDIR}/usr/mdec/cdboot 
${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}/cdboot
mkhybrid -a -R -T -L -l -d -D -N -o ${.OBJDIR}/${CDROM} \
-   -A "OpenBSD ${OSREV} ${MACHINE} bootonly CD" \
+   -A "OpenBSD ${OSREV} ${MACHINE} boot-only CD" \
-P "Copyright (c) `date +%Y` Theo de Raadt, The OpenBSD project" \
-p "Theo de Raadt " \
-V "OpenBSD/${MACHINE}   ${OSREV} boot-only CD" \
Index: hppa/ramdisk/Makefile
===
RCS file: /cvs/src/distrib/hppa/ramdisk/Makefile,v
retrieving revision 1.48
diff -u -p -r1.48 Makefile
--- hppa/ramdisk/Makefile   26 Jul 2021 12:47:45 -  1.48
+++ hppa/ramdisk/Makefile   4 Nov 2021 18:45:35 -
@@ -20,7 +20,7 @@ ${CDROM}: bsd.rd
rm -rf ${.OBJDIR}/cd-dir/
mkdir -p ${.OBJDIR}/cd-dir/
cp bsd.rd ${.OBJDIR}/cd-dir/bsd.rd
-   mkhybrid -A "OpenBSD ${OSREV} ${MACHINE} bootonly CD" \
+   mkhybrid -A "OpenBSD ${OSREV} ${MACHINE} boot-only CD" \
-P "Copyright (c) `date +%Y` Theo de Raadt, The OpenBSD project" \
-p "Theo de Raadt " \
-V "OpenBSD/${MACHINE} ${OSREV} boot-only CD" \
Index: i386/ramdisk_cd/Makefile
===
RCS file: /cvs/src/distrib/i386/ramdisk_cd/Makefile,v
retrieving revision 1.26
diff -u -p -r1.26 Makefile
--- i386/ramdisk_cd/Makefile4 Oct 2021 17:02:21 -   1.26
+++ i386/ramdisk_cd/Makefile4 Nov 2021 18:45:35 -
@@ -42,7 +42,7 @@ ${CDROM}: bsd.rd
cp ${DESTDIR}/usr/mdec/cdbr ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}
cp ${DESTDIR}/usr/mdec/cdboot 
${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}/cdboot
mkhybrid -a -R -T -L -l -d -D -N -o ${.OBJDIR}/${CDROM} \
-   -A "OpenBSD ${OSREV} ${MACHINE} bootonly CD" \
+   -A "OpenBSD ${OSREV} ${MACHINE} boot-only CD" \
-P "Copyright (c) `date +%Y` Theo de Raadt, The OpenBSD project" \
-p "Theo de Raadt " \
-V "OpenBSD/${MACHINE}${OSREV} boot-only CD" \
Index: loongson/ramdisk/Makefile
===
RCS file: /cvs/src/distrib/loongson/ramdisk/Makefile,v
retrieving revision 1.31
diff -u -p -r1.31 Makefile
--- loongson/ramdisk/Makefile   26 Jul 2021 12:47:46 -  1.31
+++ loongson/ramdisk/Makefile   4 Nov 2021 18:45:35 -
@@ -38,7 +38,7 @@ ${CDROM}: bsd.rd
echo "set image /${OSREV}/${MACHINE}/bsd.rd" > 
${.OBJDIR}/cd-dir/etc/boot.conf
cp ${.OBJDIR}/bsd.rd ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}
mkhybrid -a -R -T -L -l -d -D -N -o ${.OBJDIR}/${CDROM} \
-   -A "OpenBSD ${OSREV} ${MACHINE} bootonly CD" \
+   -A "OpenBSD ${OSREV} ${MACHINE} boot-only CD" \
-P "Copyright (c) `date +%Y` Theo de Raadt, The OpenBSD project" \
-p "Theo de Raadt " \
-V "OpenBSD/${MACHINE} ${OSREV} boot CD" \
Index: macppc/ramdisk/Makefile
===
RCS file: /cvs/src/distrib/macppc/ramdisk/Makefile,v
retrieving revision 1.51
diff -u -p -r1.51 Makefile
--- macppc/ramdisk/Makefile 26 Jul 2021 12:47:46 -  1.51
+++ macppc/ramdisk/Makefile 4 Nov 2021 18:45:35 -
@@ -22,7 +22,7 @@ ${CDROM}: bsd.gz
mkhybrid -r -part -hfs \
-hfs-bless ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE} \
-map ${.CURDIR}/../../../gnu/usr.sbin/mkhybrid/src/more.mapping \
-   -A "OpenBSD ${OSREV} ${MACHINE} bootonly CD" \
+   -A "OpenBSD ${OSREV} ${MACHINE} boot-only CD" \
-P "Copyright (c) 

libc/asr: remove unused variables

2021-11-04 Thread Klemens Nanni
Old diff laying around.

OK?

Index: libc/asr/asr.c
===
RCS file: /cvs/src/lib/libc/asr/asr.c,v
retrieving revision 1.65
diff -u -p -r1.65 asr.c
--- libc/asr/asr.c  6 Jan 2021 19:54:17 -   1.65
+++ libc/asr/asr.c  5 Apr 2021 21:33:06 -
@@ -131,8 +131,6 @@ _asr_resolver_done(void *arg)
 static void
 _asr_resolver_done_tp(void *arg)
 {
-   char buf[100];
-   int len;
struct asr **priv = arg;
struct asr *asr;
 



gpioleds: fallack to label property

2021-11-03 Thread Klemens Nanni
Some device trees such as on my Raspberry Pi 4b use the deprecated
"label" property rather than "function".

Fall back to it such that LEDs won't be skipped:

-gpioleds0 at mainbus0: no LEDs
+gpioleds0 at mainbus0: "led0", "led1"

OK?

Index: gpioleds.c
===
RCS file: /cvs/src/sys/dev/fdt/gpioleds.c,v
retrieving revision 1.1
diff -u -p -r1.1 gpioleds.c
--- gpioleds.c  25 Sep 2021 10:43:24 -  1.1
+++ gpioleds.c  3 Nov 2021 18:41:44 -
@@ -60,15 +60,20 @@ gpioleds_attach(struct device *parent, s
struct fdt_attach_args  *faa = aux;
uint32_t*led_pin;
char*function, *default_state;
+   char*function_prop = "function";
int  function_len, default_state_len, gpios_len;
int  node, leds = 0;
 
pinctrl_byname(faa->fa_node, "default");
 
for (node = OF_child(faa->fa_node); node; node = OF_peer(node)) {
-   function_len = OF_getproplen(node, "function");
-   if (function_len <= 0)
-   continue;
+   function_len = OF_getproplen(node, function_prop);
+   if (function_len <= 0) {
+   function_prop = "label";
+   function_len = OF_getproplen(node, function_prop);
+   if (function_len <= 0)
+   continue;
+   }
default_state_len = OF_getproplen(node, "default-state");
if (default_state_len <= 0)
continue;
@@ -77,7 +82,7 @@ gpioleds_attach(struct device *parent, s
continue;
 
function = malloc(function_len, M_TEMP, M_WAITOK);
-   OF_getprop(node, "function", function, function_len);
+   OF_getprop(node, function_prop, function, function_len);
default_state = malloc(default_state_len, M_TEMP, M_WAITOK);
OF_getprop(node, "default-state", default_state, 
default_state_len);
led_pin = malloc(gpios_len, M_TEMP, M_WAITOK);
@@ -94,9 +99,7 @@ gpioleds_attach(struct device *parent, s
free(led_pin, M_TEMP, gpios_len);
}
 
-   if (leds == 0) {
-   printf(": no LEDs\n");
-   return;
-   }
+   if (leds == 0)
+   printf(": no LEDs");
printf("\n");
 }



ifconfig: zap dead code

2021-11-02 Thread Klemens Nanni
No idea what it was supposed to do back then;  cvs blame points at

revision 1.19
date: 1998/09/03 06:24:18;  author: jason;  state: Exp;  lines: +502 
-38;
o OpenBSD gets if_media support (from NetBSD)
o rework/simplify if_xl to use 

OK?

Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.447
diff -u -p -r1.447 ifconfig.c
--- ifconfig.c  2 Nov 2021 23:39:27 -   1.447
+++ ifconfig.c  2 Nov 2021 23:46:28 -
@@ -411,11 +411,6 @@ const struct   cmd {
{ "alias",  IFF_UP, 0,  notealias },
{ "-alias", -IFF_UP,0,  notealias },
{ "delete", -IFF_UP,0,  notealias },
-#ifdef notdef
-#defineEN_SWABIPS  0x1000
-   { "swabips",EN_SWABIPS, 0,  setifflags },
-   { "-swabips",   -EN_SWABIPS,0,  setifflags },
-#endif /* notdef */
{ "netmask",NEXTARG,0,  setifnetmask },
{ "mtu",NEXTARG,0,  setifmtu },
{ "nwid",   NEXTARG,0,  setifnwid },



config.8: add EXIT STATUS

2021-11-02 Thread Klemens Nanni
I looked at 
I had to look at the source to be sure what the three different return
codes 0, 1 and 2 would mean and if there was more to it.

Depending on the error, config(8) exists 1 or 2.  0 on success as usual.

OK?

Index: config.8
===
RCS file: /cvs/src/usr.sbin/config/config.8,v
retrieving revision 1.73
diff -u -p -r1.73 config.8
--- config.813 Sep 2021 17:43:26 -  1.73
+++ config.82 Nov 2021 22:17:14 -
@@ -322,6 +322,8 @@ Show all devices for which attribute
 has the value
 .Ar val .
 .El
+.Sh EXIT STATUS
+.Ex -std
 .Sh EXAMPLES
 The Ethernet card is not detected at boot because the kernel configuration
 does not match the physical hardware configuration,



Re: ifconfig: return non-zero on failed "nwkey"

2021-11-02 Thread Klemens Nanni
On Tue, Nov 02, 2021 at 06:38:24PM +0100, Stefan Sperling wrote:
> It looks like bwfm(4) does support WEP but the C_WEP capability is missing
> from ic_caps so net80211 believes WEP was not supported by this driver.
> 
> I don't think we have any supported wifi device that does not support WEP.
> Even an(4) supports WEP!

I considered this a feature not a bug!



Re: ifconfig: return non-zero on failed "nwkey"

2021-11-02 Thread Klemens Nanni
On Tue, Nov 02, 2021 at 11:30:11AM -0600, Theo de Raadt wrote:
> Klemens Nanni  wrote:
> 
> > At least bwfm(4) does not support WEP:
> > 
> > # ifconfig bwfm0 nwkey 12345  
> > ifconfig: SIOCS80211NWKEY: Operation not supported by device
> > # echo $?
> > 0
> > 
> > ifconfig(8) must return non-zero in this case.
> > 
> > This is relevant for an upcoming installer fix, but also worth itself.
> > 
> > OK?
> > 
> > Index: ifconfig.c
> > ===
> > RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> > retrieving revision 1.445
> > diff -u -p -r1.445 ifconfig.c
> > --- ifconfig.c  6 Oct 2021 06:14:08 -   1.445
> > +++ ifconfig.c  2 Nov 2021 17:20:01 -
> > @@ -2033,7 +2033,7 @@ setifnwkey(const char *val, int d)
> > }
> >  
> > if (ioctl(sock, SIOCS80211NWKEY, (caddr_t)) == -1)
> > -   warn("SIOCS80211NWKEY");
> > +   err("SIOCS80211NWKEY");
> >  }
> 
> 
> If you do this, then a multi-parameter ifconfig will fail, without
> trying other operations.

I'm happy to fail early instead of humping along best-effort.

Which multiple parameters would you do in one ifconfig invocation?
Maybe WEP, then autoconf/address setup?  I'd assume they somewhat depend
on each other or are logically grouped (in hostname.if).

> That might be desireable.  Or it might not be.

Better fail hard and early.  If a long `ifconfig ...' command "worked"
now while it didn't, I'm happy to show users now that it never worked.



Re: ifconfig: return non-zero on failed "nwkey"

2021-11-02 Thread Klemens Nanni
On Tue, Nov 02, 2021 at 05:26:17PM +, Klemens Nanni wrote:
> At least bwfm(4) does not support WEP:
> 
>   # ifconfig bwfm0 nwkey 12345  
>   ifconfig: SIOCS80211NWKEY: Operation not supported by device
>   # echo $?
>   0
> 
> ifconfig(8) must return non-zero in this case.
> 
> This is relevant for an upcoming installer fix, but also worth itself.
> 
> OK?
> 
> Index: ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.445
> diff -u -p -r1.445 ifconfig.c
> --- ifconfig.c6 Oct 2021 06:14:08 -   1.445
> +++ ifconfig.c2 Nov 2021 17:20:01 -
> @@ -2033,7 +2033,7 @@ setifnwkey(const char *val, int d)
>   }
>  
>   if (ioctl(sock, SIOCS80211NWKEY, (caddr_t)) == -1)
> - warn("SIOCS80211NWKEY");
> + err("SIOCS80211NWKEY");
>  }
>  
>  /* ARGSUSED */
> 

I noticed the above mentioned error^Wwarning in the installer:

Which network interface do you wish to configure? (or 'done') [bse0] 
bwfm0
ifconfig: SIOCS80211NWKEY: Operation not supported by device
Access point? (ESSID, 'any', list# or '?') [any] 2
Security protocol? (O)pen, (W)EP, WPA-(P)SK [O] 


This comes from ieee80211_scan():

Reset 802.11 settings and determine WPA capability.   
ifconfig $_if -nwid -nwkey
ifconfig $_if -wpa 2>/dev/null && _has_wpa=1  


With ifconfig and installer fixed, we can properly detect and skip WEP:

Which network interface do you wish to configure? (or 'done') [bse0] 
bwfm0
Access point? (ESSID, 'any', list# or '?') [any] 2
Security protocol? (O)pen, WPA-(P)SK [O] w
'w' is not a valid choice.
Security protocol? (O)pen, WPA-(P)SK [O] p
WPA passphrase? (will echo) 

OK?

Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1184
diff -u -p -r1.1184 install.sub
--- install.sub 2 Nov 2021 16:54:01 -   1.1184
+++ install.sub 2 Nov 2021 17:26:40 -
@@ -1228,11 +1228,12 @@ ieee80211_scan() {
 # Configure 802.11 interface $1 and append ifconfig options to hostname.if $2.
 # Ask the user for the access point ESSID, the security protocol and a secret.
 ieee80211_config() {
-   local _if=$1 _hn=$2 _prompt _nwid _haswpa=0 _err
+   local _if=$1 _hn=$2 _prompt _nwid _has_wep=0 _has_wpa=0 _err
 
-   # Reset 802.11 settings and determine wpa capability.
-   ifconfig $_if -nwid -nwkey
-   ifconfig $_if -wpa 2>/dev/null && _haswpa=1
+   # Reset 802.11 settings and determine WEP and WPA capabilities.
+   ifconfig $_if -nwid
+   ifconfig $_if -nwkey 2>/dev/null && _has_wep=1
+   ifconfig $_if -wpa 2>/dev/null && _has_wpa=1
 
# Empty scan cache.
rm -f $WLANLIST
@@ -1256,17 +1257,19 @@ ieee80211_config() {
# 'any' implies that only open access points are considered.
if [[ $_nwid != any ]]; then
 
-   _prompt="Security protocol? (O)pen, (W)EP"
-   ((_haswpa == 1)) && _prompt="$_prompt, WPA-(P)SK"
+   _prompt="Security protocol? (O)pen"
+   ((_has_wep == 1)) && _prompt="$_prompt, (W)EP"
+   ((_has_wpa == 1)) && _prompt="$_prompt, WPA-(P)SK"
while :; do
ask_until "$_prompt" "O"
-   case "$_haswpa-$resp" in
-   ?-[Oo]) # No further questions
+   case "${_has_wep}${_has_wpa}-${resp}" in
+   ??-[Oo]) # No further questions
ifconfig $_if join "$_nwid"
quote join "$_nwid" >>$_hn
break
;;
-   ?-[Ww]) ask_passphrase "WEP key?"
+   1?-[Ww])
+   ask_passphrase "WEP key?"
# Make sure ifconfig accepts the key.
if _err=$(ifconfig $_if join "$_nwid" nwkey 
"$_passphrase" 2>&1) &&
[[ -z $_err ]]; then
@@ -1275,7 +1278,8 @@ ieee80211_config() {
fi
echo "$_err"
;;
-   1-[Pp]) ask_passphrase "WPA passphrase?"
+   ?1-[Pp])
+   ask_passphrase "WPA passphrase?"
# Make sure ifconfig accepts the key.
if ifconfig $_if join "$_nwid" wpakey 
"$_passphrase"; then
quote join "$_nwid" wpakey 
"$_passphrase" >>$_hn



ifconfig: return non-zero on failed "nwkey"

2021-11-02 Thread Klemens Nanni
At least bwfm(4) does not support WEP:

# ifconfig bwfm0 nwkey 12345  
ifconfig: SIOCS80211NWKEY: Operation not supported by device
# echo $?
0

ifconfig(8) must return non-zero in this case.

This is relevant for an upcoming installer fix, but also worth itself.

OK?

Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.445
diff -u -p -r1.445 ifconfig.c
--- ifconfig.c  6 Oct 2021 06:14:08 -   1.445
+++ ifconfig.c  2 Nov 2021 17:20:01 -
@@ -2033,7 +2033,7 @@ setifnwkey(const char *val, int d)
}
 
if (ioctl(sock, SIOCS80211NWKEY, (caddr_t)) == -1)
-   warn("SIOCS80211NWKEY");
+   err("SIOCS80211NWKEY");
 }
 
 /* ARGSUSED */



Re: unwind.conf.5: Zap duplicate bits, mention route nameserver

2021-11-02 Thread Klemens Nanni
On Tue, Oct 26, 2021 at 01:24:30PM +, Klemens Nanni wrote:
> Mentioning `route nameserver' relevance made it obvious that the
> `preference' block duplicates lots of information and I despise adding
> to that.

route(8) is fixed/polished, unwind.conf(5) still lacks behind.

> So rearrange the list of types such that conceptually related ones are
> subsequent and can reference each user to not repeat things.
> 
> This looks like this:
> 
>  preference {type ...}
>A list of DNS name server types to specify the order in which
>name servers are picked when measured round-trip time medians are
>equal.  Additionally, the first mentioned type gets a time bonus.
>Validating name servers are always picked over non-validating
>name servers.  DNS name server types are:
> 
>autoconfName servers learned via DHCP, SLAAC or route
>nameserver.
>oDoT-autoconf   autoconf with opportunistic DNS over TLS.
>stubautoconf via libc functions.  See asr_run(3).
>Will never validate.  Useful when running behind
>broken middle boxes that do not like edns0.  DNS
>answers from stub name servers are not cached.
>forwarder   Name servers configured in unwind.conf.
>DoT forwarder with DNS over TLS.
>oDoT-forwarder  Opportunistic DoT.
>recursorRecursively resolve names.
> 
> Emphasize "edns0" while here and simplify "unwind does X" to just "X".
> 
> Fist I tried listing types in the default order such that the sentence
> afterwards is obsoleted by the self-documenting manner, but that
> conflicts with the logical order I picked above.
> 
> Feedback? Objections? OK?

Anyone?


Index: unwind.conf.5
===
RCS file: /cvs/src/sbin/unwind/unwind.conf.5,v
retrieving revision 1.31
diff -u -p -r1.31 unwind.conf.5
--- unwind.conf.5   24 Oct 2021 15:57:17 -  1.31
+++ unwind.conf.5   26 Oct 2021 13:17:56 -
@@ -93,33 +93,32 @@ Validating name servers are always picke
 DNS name server types are:
 .Pp
 .Bl -tag -width "oDoT-forwarder" -compact
+.It Ic autoconf
+Name servers learned via DHCP, SLAAC or
+.Cm route nameserver .
+.It Ic oDoT-autoconf
+.Ic autoconf
+with opportunistic DNS over TLS.
 .It Ic stub
-Name servers learned via DHCP or SLAAC, queried using the libc functions.
+.Ic autoconf
+via libc functions.
 See
 .Xr asr_run 3 .
 Will never validate.
-Useful when running behind broken middle boxes that do not like edns0.
+Useful when running behind broken middle boxes that do not like
+.Cm edns0 .
 DNS answers from stub name servers are not cached.
-.It Ic autoconf
-Name servers learned via DHCP or SLAAC.
-.It Ic oDoT-autoconf
-Name servers learned via DHCP or SLAAC.
-.Nm unwind
-tries to opportunistically use DNS over TLS.
-.It Ic DoT
-DNS over TLS name servers configured in
-.Nm .
 .It Ic forwarder
 Name servers configured in
 .Nm .
+.It Ic DoT
+.Ic forwarder
+with DNS over TLS.
 .It Ic oDoT-forwarder
-Name servers configured in
-.Nm .
-.Nm unwind
-tries to opportunistically use DNS over TLS.
+Opportunistic
+.Ic DoT .
 .It Ic recursor
-.Nm unwind
-itself recursively resolves names.
+Recursively resolve names.
 .El
 .Pp
 The default preference is



Re: demystify vport(4) in vport(4) and ifconfig(8)

2021-10-30 Thread Klemens Nanni
On Fri, Oct 29, 2021 at 08:18:43AM -0600, Theo de Raadt wrote:
> Please don't do this.

Agreed.  Please don't add such quirks to bend over backwards for pagers
that misbehave.

> Ingo Schwarze  wrote:
> 
> > Hi Stuart,
> > 
> > Stuart Henderson wrote on Fri, Oct 29, 2021 at 01:59:38PM +0100:
> > > On 2021/10/29 14:08, Ingo Schwarze wrote:
> > >> Stuart Henderson wrote on Fri, Oct 29, 2021 at 10:53:41AM +0100:
> > >>> On 2021/10/28 23:19, Klemens Nanni wrote:
> > >>>> On Fri, Oct 29, 2021 at 12:57:54AM +0200, Ingo Schwarze wrote:
> > 
> > >>>>>   MANPAGER=firefox man -T html $(ifconfig -C)
> > 
> > >>>> This doesn't work if firefox is already running as the MANPAGER firefox
> > >>>> process exits immediately after sending the file/link to the running
> > >>>> process, which causes mandoc to exit after removing the temporary file,
> > >>>> by which time firefox fails to open the no longer exiting file.
> > 
> > >>> I use mutt_bgrun for things like this
> > 
> > >> I don't see how that can possibly help.
> > 
> > > Oh, of course you're right - I was confused between my helper
> > > scripts. The one I actually use for html in mutt looks like this,
> > > so it's not any user for MANPAGER..
> > > 
> > > tmp=`mktemp -d /tmp/mutthtml.X` || exit 1
> > > mhonarc -rcfile ~/.m2h_rcfile -single > $tmp/mail.html
> > > (chrome $tmp/mail.html 2>/dev/null; sleep 30; rm -r $tmp) &
> > 
> > Indeed, if i added sleep(30) to the place in question
> > in usr.bin/mandoc/main.c, i would expect quite an outrage...
> > A protest rally on my front lawn or something like that...
> > 
> > Then again, the man(1) process that is about to exit could maybe fork
> > a child before exiting.  The parent exiting right away makes sure that
> > the user gets back their shell prompt instantly after exiting the pager.
> > The child process, now being in the background, could sleep a few seconds
> > before deleting the temporary output files, then exit, too.
> > 
> > This does appear to successfully work around the firefox bug.
> > With the patch below, "MANPAGER=firefox man -T html ..."
> > now works reliably for me.
> > 
> > It does seem somewhat ugly, though, so i'm not sure whether i should
> > commit it.  In particular, fork(2)ing merely to go to the background
> > seems both cumbersome and expensive - admittedly, your script cited
> > above does the same, but that doesn't imply i have to like that, right?
> > 
> > Is there a cleaner way to let man(1) go into the background and
> > let the shell offer a fresh prompt while man(1) still sleeps,
> > without needing to fork(2)?
> > 
> > Yours,
> >   Ingo
> > 
> > 
> > Index: main.c
> > ===
> > RCS file: /cvs/src/usr.bin/mandoc/main.c,v
> > retrieving revision 1.262
> > diff -u -p -r1.262 main.c
> > --- main.c  4 Oct 2021 21:28:50 -   1.262
> > +++ main.c  29 Oct 2021 14:12:08 -
> > @@ -1203,6 +1203,7 @@ woptions(char *arg, enum mandoc_os *os_e
> >  static void
> >  run_pager(struct outstate *outst, char *tag_target)
> >  {
> > +   const struct timespec delay = { 5, 0 };
> > int  signum, status;
> > pid_tman_pgid, tc_pgid;
> > pid_tpager_pid, wait_pid;
> > @@ -1245,13 +1246,16 @@ run_pager(struct outstate *outst, char *
> > if (wait_pid == -1) {
> > mandoc_msg(MANDOCERR_WAIT, 0, 0,
> > "%s", strerror(errno));
> > -   break;
> > +   return;
> > }
> > if (!WIFSTOPPED(status))
> > break;
> >  
> > signum = WSTOPSIG(status);
> > }
> > +   if (fork() > 0)
> > +   _exit(mandoc_msg_getrc());
> > +   nanosleep(, NULL);
> >  }
> >  
> >  static pid_t
> > 
> 



Re: demystify vport(4) in vport(4) and ifconfig(8)

2021-10-28 Thread Klemens Nanni
On Fri, Oct 29, 2021 at 12:57:54AM +0200, Ingo Schwarze wrote:
>   MANPAGER=firefox man -T html $(ifconfig -C)

This doesn't work if firefox is already running as the MANPAGER firefox
process exits immediately after sending the file/link to the running
process, which causes mandoc to exit after removing the temporary file,
by which time firefox fails to open the no longer exiting file.



new site.8: document site*.tgz and /{upgrade,install}.site

2021-10-27 Thread Klemens Nanni
On Mon, Sep 06, 2021 at 02:29:50PM -0500, Aaron Poffenberger wrote:
> Ping.
> 
> Will someone commit this? Seems like no one knows about /upgrade.site and it
> fits well with sysupgrade(8).

sysupgrade(8) is one potential /upgrade.site user but I disagree about
documenting the latter through the former.

Here is a new manual roughly merging the FAQ bits with what you, Aaron,
provided.

site(8) is a lame but fitting name;  autoinstall(8) and sysypgrade(8)
reference it and both sets and scripts are documented.

Examples are intentionally brief to be shorter and more concise than the
FAQ while showing enough to get going.


I am quite certain that wording, examples and the references from other
manuals can be tweaked, but this diff looks like a good enough start
and --if this is the way to go-- I'd prefer to commit and keep polishing
in-tree.

Feedback? Objections? OK?


Index: distrib/sets/lists/man/mi
===
RCS file: /cvs/src/distrib/sets/lists/man/mi,v
retrieving revision 1.1643
diff -u -p -r1.1643 mi
--- distrib/sets/lists/man/mi   21 Oct 2021 18:36:41 -  1.1643
+++ distrib/sets/lists/man/mi   27 Oct 2021 20:39:11 -
@@ -2558,6 +2558,7 @@
 ./usr/share/man/man8/security.8
 ./usr/share/man/man8/sendmail.8
 ./usr/share/man/man8/sensorsd.8
+./usr/share/man/man8/site.8
 ./usr/share/man/man8/sftp-server.8
 ./usr/share/man/man8/showmount.8
 ./usr/share/man/man8/shutdown.8
Index: usr.sbin/sysupgrade/sysupgrade.8
===
RCS file: /cvs/src/usr.sbin/sysupgrade/sysupgrade.8,v
retrieving revision 1.10
diff -u -p -r1.10 sysupgrade.8
--- usr.sbin/sysupgrade/sysupgrade.83 Oct 2019 12:43:58 -   1.10
+++ usr.sbin/sysupgrade/sysupgrade.827 Oct 2021 20:42:26 -
@@ -67,6 +67,10 @@ This is the default if the system is cur
 Upgrade to a snapshot.
 This is the default if the system is currently running a snapshot.
 .El
+.Pp
+See
+.Xr site 8
+for how to customize the upgrade process.
 .Sh FILES
 .Bl -tag -width "/auto_upgrade.conf" -compact
 .It Pa /auto_upgrade.conf
@@ -83,7 +87,8 @@ Directory the upgrade is downloaded to.
 .Xr signify 1 ,
 .Xr installurl 5 ,
 .Xr autoinstall 8 ,
-.Xr release 8
+.Xr release 8 ,
+.Xr site 8
 .Sh HISTORY
 .Nm
 first appeared in
Index: share/man/man8/Makefile
===
RCS file: /cvs/src/share/man/man8/Makefile,v
retrieving revision 1.102
diff -u -p -r1.102 Makefile
--- share/man/man8/Makefile 1 May 2021 16:11:10 -   1.102
+++ share/man/man8/Makefile 27 Oct 2021 20:38:11 -
@@ -6,7 +6,7 @@ MAN=afterboot.8 autoinstall.8 boot_conf
crash.8 daily.8 \
diskless.8 genassym.sh.8 intro.8 netstart.8 rc.8 \
rc.conf.8 rc.d.8 rc.shutdown.8 rc.subr.8 release.8 \
-   security.8 ssl.8 starttls.8 sticky.8 yp.8
+   security.8 site.8 ssl.8 starttls.8 sticky.8 yp.8
 
 SUBDIR=man8.alpha man8.amd64 man8.arm64 man8.armv7 \
man8.hppa man8.i386 man8.landisk \
Index: share/man/man8/autoinstall.8
===
RCS file: /cvs/src/share/man/man8/autoinstall.8,v
retrieving revision 1.23
diff -u -p -r1.23 autoinstall.8
--- share/man/man8/autoinstall.818 Jul 2021 11:08:34 -  1.23
+++ share/man/man8/autoinstall.827 Oct 2021 20:42:24 -
@@ -32,6 +32,10 @@ file and HTTP to fetch the file.
 If that fails, the installer asks for the location which can either be
 a URL or a local path.
 .Pp
+See
+.Xr site 8
+for how to provide custom configuration.
+.Pp
 To start unattended installation or upgrade choose '(A)utoinstall' at the
 install prompt.
 If there is only one network interface, the installer fetches the response
@@ -235,7 +239,8 @@ host foo {
 .Sh SEE ALSO
 .Xr dhcp-options 5 ,
 .Xr dhcpd.conf 5 ,
-.Xr diskless 8
+.Xr diskless 8 ,
+.Xr site 8
 .Sh HISTORY
 The
 .Nm
Index: share/man/man8/site.8
===
RCS file: share/man/man8/site.8
diff -N share/man/man8/site.8
--- /dev/null   1 Jan 1970 00:00:00 -
+++ share/man/man8/site.8   27 Oct 2021 21:11:48 -
@@ -0,0 +1,87 @@
+.\" $OpenBSD: $
+.\"
+.\" Copyright (c) 2021 Klemens Nanni 
+.\"
+.\" Permission to use, copy, modify, and distribute this software for any
+.\" purpose with or without fee is hereby granted, provided that the above
+.\" copyright notice and this permission notice appear in all copies.
+.\"
+.\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+.\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+.\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+.\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+.\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR

Re: route.8: nameserver command is not resolvd(8) specific

2021-10-26 Thread Klemens Nanni
On Tue, Oct 26, 2021 at 04:06:20PM +0100, Jason McIntyre wrote:
> On Tue, Oct 26, 2021 at 08:57:40AM -0600, Theo de Raadt wrote:
> > Jason McIntyre  wrote:
> > 
> > > On Tue, Oct 26, 2021 at 12:21:52PM +, Klemens Nanni wrote:
> > > > While designed to populate resolv.conf(5), there is nothing resolvd(8)
> > > > specific about the route message route(8) sends.
> > > > 
> > > > At least unwind(8) consumes and acts upon it;  there might as well be
> > > > other programs in ports that do such things, so describe it generically
> > > > while going into required detail for our selected base daemons.
> > > > 
> > > > OK?
> > > > 
> > > > NB:  I'll fix unwind.conf(5) wrt. `route nameserver' separately.
> > > > 
> > > 
> > > hi.
> > > 
> > > i think it sounds weird to talk about sending things without saying
> > > where they're going. maybe there is a better word than send? maybe this
> > > command is just recording, or specifying, or ...? i don;t have a good
> > > grasp of the process here.
> > 
> > "to the route socket"
> > 
> > Maybe rather than 'send', does it explain things better to say 'broadcast',
> > 
> 
> i think either saying to the route socket or broadcast are clear and
> would read better.

The latter is great, thanks.

OK?


Index: route.8
===
RCS file: /cvs/src/sbin/route/route.8,v
retrieving revision 1.101
diff -u -p -r1.101 route.8
--- route.8 22 Oct 2021 18:31:12 -  1.101
+++ route.8 26 Oct 2021 15:30:04 -
@@ -169,11 +169,16 @@ only changes in that routing table will 
 .Ar interface
 .Op Ar address ...
 .Xc
-Send a list of up to five nameserver address proposals to
-.Xr resolvd 8 .
+Broadcast a list of up to five nameserver address proposals.
+.Pp
+.Xr unwind 8
+will learn them and act according to
+.Xr unwind.conf 5 .
+.Pp
 .Xr resolvd 8
 will replace all existing nameservers for the given interface in
 .Xr resolv.conf 5 .
+.Pp
 If no
 .Ar address
 argument is given, a request to remove the nameservers previously entered for



unwind.conf.5: Zap duplicate bits, mention route nameserver

2021-10-26 Thread Klemens Nanni
Mentioning `route nameserver' relevance made it obvious that the
`preference' block duplicates lots of information and I despise adding
to that.

So rearrange the list of types such that conceptually related ones are
subsequent and can reference each user to not repeat things.

This looks like this:

 preference {type ...}
 A list of DNS name server types to specify the order in which
 name servers are picked when measured round-trip time medians are
 equal.  Additionally, the first mentioned type gets a time bonus.
 Validating name servers are always picked over non-validating
 name servers.  DNS name server types are:

 autoconfName servers learned via DHCP, SLAAC or route
 nameserver.
 oDoT-autoconf   autoconf with opportunistic DNS over TLS.
 stubautoconf via libc functions.  See asr_run(3).
 Will never validate.  Useful when running behind
 broken middle boxes that do not like edns0.  DNS
 answers from stub name servers are not cached.
 forwarder   Name servers configured in unwind.conf.
 DoT forwarder with DNS over TLS.
 oDoT-forwarder  Opportunistic DoT.
 recursorRecursively resolve names.

Emphasize "edns0" while here and simplify "unwind does X" to just "X".

Fist I tried listing types in the default order such that the sentence
afterwards is obsoleted by the self-documenting manner, but that
conflicts with the logical order I picked above.

Feedback? Objections? OK?

Index: unwind.conf.5
===
RCS file: /cvs/src/sbin/unwind/unwind.conf.5,v
retrieving revision 1.31
diff -u -p -r1.31 unwind.conf.5
--- unwind.conf.5   24 Oct 2021 15:57:17 -  1.31
+++ unwind.conf.5   26 Oct 2021 13:17:56 -
@@ -93,33 +93,32 @@ Validating name servers are always picke
 DNS name server types are:
 .Pp
 .Bl -tag -width "oDoT-forwarder" -compact
+.It Ic autoconf
+Name servers learned via DHCP, SLAAC or
+.Cm route nameserver .
+.It Ic oDoT-autoconf
+.Ic autoconf
+with opportunistic DNS over TLS.
 .It Ic stub
-Name servers learned via DHCP or SLAAC, queried using the libc functions.
+.Ic autoconf
+via libc functions.
 See
 .Xr asr_run 3 .
 Will never validate.
-Useful when running behind broken middle boxes that do not like edns0.
+Useful when running behind broken middle boxes that do not like
+.Cm edns0 .
 DNS answers from stub name servers are not cached.
-.It Ic autoconf
-Name servers learned via DHCP or SLAAC.
-.It Ic oDoT-autoconf
-Name servers learned via DHCP or SLAAC.
-.Nm unwind
-tries to opportunistically use DNS over TLS.
-.It Ic DoT
-DNS over TLS name servers configured in
-.Nm .
 .It Ic forwarder
 Name servers configured in
 .Nm .
+.It Ic DoT
+.Ic forwarder
+with DNS over TLS.
 .It Ic oDoT-forwarder
-Name servers configured in
-.Nm .
-.Nm unwind
-tries to opportunistically use DNS over TLS.
+Opportunistic
+.Ic DoT .
 .It Ic recursor
-.Nm unwind
-itself recursively resolves names.
+Recursively resolve names.
 .El
 .Pp
 The default preference is



route.8: nameserver command is not resolvd(8) specific

2021-10-26 Thread Klemens Nanni
While designed to populate resolv.conf(5), there is nothing resolvd(8)
specific about the route message route(8) sends.

At least unwind(8) consumes and acts upon it;  there might as well be
other programs in ports that do such things, so describe it generically
while going into required detail for our selected base daemons.

OK?

NB:  I'll fix unwind.conf(5) wrt. `route nameserver' separately.

Index: route.8
===
RCS file: /cvs/src/sbin/route/route.8,v
retrieving revision 1.101
diff -u -p -r1.101 route.8
--- route.8 22 Oct 2021 18:31:12 -  1.101
+++ route.8 26 Oct 2021 12:18:31 -
@@ -169,11 +169,16 @@ only changes in that routing table will 
 .Ar interface
 .Op Ar address ...
 .Xc
-Send a list of up to five nameserver address proposals to
-.Xr resolvd 8 .
+Send a list of up to five nameserver address proposals.
+.Pp
+.Xr unwind 8
+will learn them and act according to
+.Xr unwind.conf 5 .
+.Pp
 .Xr resolvd 8
 will replace all existing nameservers for the given interface in
 .Xr resolv.conf 5 .
+.Pp
 If no
 .Ar address
 argument is given, a request to remove the nameservers previously entered for



/usr/src/*bin: remove unused variables

2021-10-25 Thread Klemens Nanni
Patrick's "bootloader: remove unsued variables" mail reminded me about
this diff I still have.

M sbin/vnconfig/vnconfig.c
M usr.sbin/pcidump/pcidump.c
M usr.sbin/wsfontload/wsfontload.c
M usr.bin/less/line.c
M usr.bin/make/engine.c
M usr.bin/systat/cpu.c
M usr.bin/ctfconv/dw.c

I think I gathered it from warnings during builds, not sure...

All tools build with this diff, no fancy macros/defines that pull these
normally unused variables/functions in.

OK?


Index: sbin/vnconfig/vnconfig.c
===
RCS file: /cvs/src/sbin/vnconfig/vnconfig.c,v
retrieving revision 1.6
diff -u -p -r1.6 vnconfig.c
--- sbin/vnconfig/vnconfig.c22 Sep 2021 20:43:16 -  1.6
+++ sbin/vnconfig/vnconfig.c24 Sep 2021 10:30:49 -
@@ -332,7 +332,7 @@ int
 unconfig(char *vnd)
 {
struct vnd_ioctl vndio;
-   int fd, rv = -1, unit;
+   int fd, rv = -1;
char *rdev;
 
if ((fd = opendev(vnd, O_RDONLY, OPENDEV_PART, )) == -1)
Index: usr.sbin/pcidump/pcidump.c
===
RCS file: /cvs/src/usr.sbin/pcidump/pcidump.c,v
retrieving revision 1.67
diff -u -p -r1.67 pcidump.c
--- usr.sbin/pcidump/pcidump.c  24 Oct 2021 21:24:19 -  1.67
+++ usr.sbin/pcidump/pcidump.c  25 Oct 2021 19:36:31 -
@@ -377,7 +377,7 @@ print_vpd(const uint8_t *buf, size_t len
const struct pci_vpd *vpd;
char key0[8];
char key1[8];
-   size_t vlen, i;
+   size_t vlen;
 
while (len > 0) {
if (len < sizeof(*vpd))
@@ -411,7 +411,6 @@ dump_vpd(int bus, int dev, int func)
uint32_t data[64]; /* XXX this can be up to 32k of data */
uint8_t *buf = (uint8_t *)data;
size_t len = sizeof(data);
-   size_t i;
 
bzero(, sizeof(io));
io.pv_sel.pc_bus = bus;
Index: usr.sbin/wsfontload/wsfontload.c
===
RCS file: /cvs/src/usr.sbin/wsfontload/wsfontload.c,v
retrieving revision 1.24
diff -u -p -r1.24 wsfontload.c
--- usr.sbin/wsfontload/wsfontload.c24 Oct 2021 21:24:19 -  1.24
+++ usr.sbin/wsfontload/wsfontload.c25 Oct 2021 19:36:31 -
@@ -77,7 +77,7 @@ main(int argc, char *argv[])
char *wsdev, *infile, *p;
struct wsdisplay_font f;
struct wsdisplay_screentype screens;
-   int c, res, wsfd, ffd, type, list, i;
+   int c, res, wsfd, ffd, list, i;
int defwidth, defheight;
struct stat stat;
size_t len;
Index: usr.bin/less/line.c
===
RCS file: /cvs/src/usr.bin/less/line.c,v
retrieving revision 1.33
diff -u -p -r1.33 line.c
--- usr.bin/less/line.c 3 Sep 2019 23:08:42 -   1.33
+++ usr.bin/less/line.c 5 Apr 2021 21:02:40 -
@@ -434,7 +434,7 @@ static int
 backc(void)
 {
wchar_t  ch, prev_ch;
-   int  i, len, width;
+   int  len, width;
 
if ((len = mbtowc_left(, linebuf + curr, curr)) <= 0)
return (0);
Index: usr.bin/make/engine.c
===
RCS file: /cvs/src/usr.bin/make/engine.c,v
retrieving revision 1.69
diff -u -p -r1.69 engine.c
--- usr.bin/make/engine.c   26 Jan 2020 12:41:21 -  1.69
+++ usr.bin/make/engine.c   5 Apr 2021 21:04:13 -
@@ -685,7 +685,6 @@ handle_job_status(Job *job, int status)
 int
 run_gnode(GNode *gn)
 {
-   Job *j;
if (!gn || (gn->type & OP_DUMMY))
return NOSUCHNODE;
 
Index: usr.bin/systat/cpu.c
===
RCS file: /cvs/src/usr.bin/systat/cpu.c,v
retrieving revision 1.10
diff -u -p -r1.10 cpu.c
--- usr.bin/systat/cpu.c28 Jun 2019 13:35:04 -  1.10
+++ usr.bin/systat/cpu.c5 Apr 2021 21:11:34 -
@@ -192,7 +192,7 @@ initcpu(void)
 {
field_view  *v;
size_t   size = sizeof(cpu_count);
-   int  mib[2], i;
+   int  mib[2];
 
mib[0] = CTL_HW;
mib[1] = HW_NCPU;
Index: usr.bin/ctfconv/dw.c
===
RCS file: /cvs/src/usr.bin/ctfconv/dw.c,v
retrieving revision 1.4
diff -u -p -r1.4 dw.c
--- usr.bin/ctfconv/dw.c27 Sep 2017 08:59:38 -  1.4
+++ usr.bin/ctfconv/dw.c5 Apr 2021 21:25:45 -
@@ -50,10 +50,6 @@ static intdw_read_buf(struct dwbuf *, 
 
 static int  dw_skip_bytes(struct dwbuf *, size_t);
 
-static int  dw_read_filename(struct dwbuf *, const char **, const char **,
-uint8_t, uint64_t);
-
-
 static int  dw_attr_parse(struct dwbuf *, struct dwattr *, uint8_t,
 struct dwaval_queue *);
 static void dw_attr_purge(struct dwaval_queue *);
@@ -167,55 +163,6 @@ dw_skip_bytes(struct dwbuf *d, size_t n)
   

Re: installer/wifi drivers: use join by default

2021-10-24 Thread Klemens Nanni
On Sun, Oct 24, 2021 at 01:24:23PM +0100, Stuart Henderson wrote:
> On 2021/10/24 11:57, Klemens Nanni wrote:
> > On Sun, Oct 24, 2021 at 12:41:11PM +0100, Stuart Henderson wrote:
> > > On 2021/10/24 11:20, Klemens Nanni wrote:
> > > > @@ -174,7 +174,7 @@ The following
> > > >  example creates a host-based access point on boot:
> > > >  .Bd -literal -offset indent
> > > >  mediaopt hostap
> > > > -nwid mynwid nwkey mywepkey
> > > > +join mynwid nwkey mywepkey
> > > >  inet 192.168.1.1 255.255.255.0
> > > 
> > > that's not right for hostap
> > 
> > Thanks for eagle-eying this!
> > 
> > OK?
> 
> yes. (I don't use join everywhere, but it seems like a pretty good thing
> to use by default).

Thanks.

> > --- share/man/man4/an.4 15 Oct 2021 08:10:44 -  1.45
> > +++ share/man/man4/an.4 24 Oct 2021 10:17:23 -
> > @@ -134,7 +134,7 @@ using WEP key
> >  .Dq mywepkey ,
> >  obtaining an IP address using DHCP:
> >  .Bd -literal -offset indent
> > -nwid mynwid nwkey mywepkey
> > +join mynwid nwkey mywepkey
> >  inet autoconf
> >  .Ed
> >  .Sh DIAGNOSTICS
> 
> wondering if it really makes sense to include these examples sections
> in wlan drivers any more, but that's orthogonal to this diff.

I'm unsure about this as well, but didn't bother enough (yet) to polish
this rather irrelevant turd.



Re: installer/wifi drivers: use join by default

2021-10-24 Thread Klemens Nanni
On Sun, Oct 24, 2021 at 12:41:11PM +0100, Stuart Henderson wrote:
> On 2021/10/24 11:20, Klemens Nanni wrote:
> > @@ -174,7 +174,7 @@ The following
> >  example creates a host-based access point on boot:
> >  .Bd -literal -offset indent
> >  mediaopt hostap
> > -nwid mynwid nwkey mywepkey
> > +join mynwid nwkey mywepkey
> >  inet 192.168.1.1 255.255.255.0
> 
> that's not right for hostap

Thanks for eagle-eying this!

OK?


Index: distrib/miniroot/install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1182
diff -u -p -r1.1182 install.sub
--- distrib/miniroot/install.sub24 Oct 2021 10:11:24 -  1.1182
+++ distrib/miniroot/install.sub24 Oct 2021 11:17:29 -
@@ -1241,23 +1241,23 @@ ieee80211_config() {
ask_until "$_prompt" "O"
case "$_haswpa-$resp" in
?-[Oo]) # No further questions
-   ifconfig $_if nwid "$_nwid"
-   quote nwid "$_nwid" >>$_hn
+   ifconfig $_if join "$_nwid"
+   quote join "$_nwid" >>$_hn
break
;;
?-[Ww]) ask_until "WEP key? (will echo)"
# Make sure ifconfig accepts the key.
-   if _err=$(ifconfig $_if nwid "$_nwid" nwkey 
"$resp" 2>&1) &&
+   if _err=$(ifconfig $_if join "$_nwid" nwkey 
"$resp" 2>&1) &&
[[ -z $_err ]]; then
-   quote nwid "$_nwid" nwkey "$resp" >>$_hn
+   quote join "$_nwid" nwkey "$resp" >>$_hn
break
fi
echo "$_err"
;;
1-[Pp]) ask_until "WPA passphrase? (will echo)"
# Make sure ifconfig accepts the key.
-   if ifconfig $_if nwid "$_nwid" wpakey "$resp"; 
then
-   quote nwid "$_nwid" wpakey "$resp" 
>>$_hn
+   if ifconfig $_if join "$_nwid" wpakey "$resp"; 
then
+   quote join "$_nwid" wpakey "$resp" 
>>$_hn
break
fi
;;
Index: share/man/man4/an.4
===
RCS file: /cvs/src/share/man/man4/an.4,v
retrieving revision 1.45
diff -u -p -r1.45 an.4
--- share/man/man4/an.4 15 Oct 2021 08:10:44 -  1.45
+++ share/man/man4/an.4 24 Oct 2021 10:17:23 -
@@ -134,7 +134,7 @@ using WEP key
 .Dq mywepkey ,
 obtaining an IP address using DHCP:
 .Bd -literal -offset indent
-nwid mynwid nwkey mywepkey
+join mynwid nwkey mywepkey
 inet autoconf
 .Ed
 .Sh DIAGNOSTICS
Index: share/man/man4/atu.4
===
RCS file: /cvs/src/share/man/man4/atu.4,v
retrieving revision 1.40
diff -u -p -r1.40 atu.4
--- share/man/man4/atu.415 Oct 2021 08:10:44 -  1.40
+++ share/man/man4/atu.424 Oct 2021 10:17:23 -
@@ -178,7 +178,7 @@ using WEP key
 .Dq mywepkey ,
 obtaining an IP address using DHCP:
 .Bd -literal -offset indent
-nwid mynwid nwkey mywepkey
+join mynwid nwkey mywepkey
 inet autoconf
 .Ed
 .Sh SEE ALSO
Index: share/man/man4/atw.4
===
RCS file: /cvs/src/share/man/man4/atw.4,v
retrieving revision 1.37
diff -u -p -r1.37 atw.4
--- share/man/man4/atw.415 Oct 2021 08:10:44 -  1.37
+++ share/man/man4/atw.424 Oct 2021 10:17:23 -
@@ -150,7 +150,7 @@ using WEP key
 .Dq mywepkey ,
 obtaining an IP address using DHCP:
 .Bd -literal -offset indent
-nwid mynwid nwkey mywepkey
+join mynwid nwkey mywepkey
 inet autoconf
 .Ed
 .Sh DIAGNOSTICS
Index: share/man/man4/bwi.4
===
RCS file: /cvs/src/share/man/man4/bwi.4,v
retrieving revision 1.45
diff -u -p -r1.45 bwi.4
--- share/man/man4/bwi.415 Oct 2021 08:10:44 -  1.45
+++ share/man/man4/bwi.424 Oct 2021 10:17:23 -
@@ -125,7 +125,7 @@ using WPA key
 .Dq mywpakey ,
 obtaining an IP address using DHCP:
 .Bd -literal -offset indent
-nwid mynwid wpakey mywpakey
+join mynw

installer/wifi drivers: use join by default

2021-10-24 Thread Klemens Nanni
I reckon the adoption of `join' was fairly successful and lots of people
use it exclusively, so upon install I suggest to provide hostname.if(5)
using it instead of `nwid'.

Likewise, reflect that trend in our driver manuals.

Feedback? Objection? OK?


Index: distrib/miniroot/install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1182
diff -u -p -r1.1182 install.sub
--- distrib/miniroot/install.sub24 Oct 2021 10:11:24 -  1.1182
+++ distrib/miniroot/install.sub24 Oct 2021 11:17:29 -
@@ -1241,23 +1241,23 @@ ieee80211_config() {
ask_until "$_prompt" "O"
case "$_haswpa-$resp" in
?-[Oo]) # No further questions
-   ifconfig $_if nwid "$_nwid"
-   quote nwid "$_nwid" >>$_hn
+   ifconfig $_if join "$_nwid"
+   quote join "$_nwid" >>$_hn
break
;;
?-[Ww]) ask_until "WEP key? (will echo)"
# Make sure ifconfig accepts the key.
-   if _err=$(ifconfig $_if nwid "$_nwid" nwkey 
"$resp" 2>&1) &&
+   if _err=$(ifconfig $_if join "$_nwid" nwkey 
"$resp" 2>&1) &&
[[ -z $_err ]]; then
-   quote nwid "$_nwid" nwkey "$resp" >>$_hn
+   quote join "$_nwid" nwkey "$resp" >>$_hn
break
fi
echo "$_err"
;;
1-[Pp]) ask_until "WPA passphrase? (will echo)"
# Make sure ifconfig accepts the key.
-   if ifconfig $_if nwid "$_nwid" wpakey "$resp"; 
then
-   quote nwid "$_nwid" wpakey "$resp" 
>>$_hn
+   if ifconfig $_if join "$_nwid" wpakey "$resp"; 
then
+   quote join "$_nwid" wpakey "$resp" 
>>$_hn
break
fi
;;
Index: share/man/man4/acx.4
===
RCS file: /cvs/src/share/man/man4/acx.4,v
retrieving revision 1.49
diff -u -p -r1.49 acx.4
--- share/man/man4/acx.415 Oct 2021 08:10:44 -  1.49
+++ share/man/man4/acx.424 Oct 2021 10:17:23 -
@@ -165,7 +165,7 @@ using WEP key
 .Dq mywepkey ,
 obtaining an IP address using DHCP:
 .Bd -literal -offset indent
-nwid mynwid nwkey mywepkey
+join mynwid nwkey mywepkey
 inet autoconf
 .Ed
 .Pp
@@ -174,7 +174,7 @@ The following
 example creates a host-based access point on boot:
 .Bd -literal -offset indent
 mediaopt hostap
-nwid mynwid nwkey mywepkey
+join mynwid nwkey mywepkey
 inet 192.168.1.1 255.255.255.0
 .Ed
 .Sh SEE ALSO
Index: share/man/man4/an.4
===
RCS file: /cvs/src/share/man/man4/an.4,v
retrieving revision 1.45
diff -u -p -r1.45 an.4
--- share/man/man4/an.4 15 Oct 2021 08:10:44 -  1.45
+++ share/man/man4/an.4 24 Oct 2021 10:17:23 -
@@ -134,7 +134,7 @@ using WEP key
 .Dq mywepkey ,
 obtaining an IP address using DHCP:
 .Bd -literal -offset indent
-nwid mynwid nwkey mywepkey
+join mynwid nwkey mywepkey
 inet autoconf
 .Ed
 .Sh DIAGNOSTICS
Index: share/man/man4/ath.4
===
RCS file: /cvs/src/share/man/man4/ath.4,v
retrieving revision 1.68
diff -u -p -r1.68 ath.4
--- share/man/man4/ath.415 Oct 2021 08:10:44 -  1.68
+++ share/man/man4/ath.424 Oct 2021 10:17:23 -
@@ -213,7 +213,7 @@ using WPA key
 .Dq mywpakey ,
 obtaining an IP address using DHCP:
 .Bd -literal -offset indent
-nwid mynwid wpakey mywpakey
+join mynwid wpakey mywpakey
 inet autoconf
 .Ed
 .Pp
@@ -222,7 +222,7 @@ The following
 example creates a host-based access point on boot:
 .Bd -literal -offset indent
 mediaopt hostap
-nwid mynwid wpakey mywpakey
+join mynwid wpakey mywpakey
 inet 192.168.1.1 255.255.255.0
 .Ed
 .Sh DIAGNOSTICS
Index: share/man/man4/athn.4
===
RCS file: /cvs/src/share/man/man4/athn.4,v
retrieving revision 1.42
diff -u -p -r1.42 athn.4
--- share/man/man4/athn.4   15 Oct 2021 08:10:44 -  1.42
+++ share/man/man4/athn.4   24 Oct 2021 10:17:23 -
@@ -206,7 +206,7 @@ using WPA key
 .Dq mywpakey ,
 obtaining an IP address using DHCP:
 .Bd -literal -offset indent
-nwid mynwid wpakey mywpakey
+join mynwid wpakey mywpakey
 inet autoconf
 .Ed
 .Pp
@@ -215,7 +215,7 @@ The following
 example 

Re: httpd request body too large in log

2021-10-23 Thread Klemens Nanni
On Sat, Oct 23, 2021 at 05:21:53PM +0200, Sebastian Benoit wrote:
> differentiate the third 413 from the other two in httpd.

OK kn



Re: new option for rcctl ls

2021-10-22 Thread Klemens Nanni
On Fri, Oct 22, 2021 at 07:27:28PM +0100, Stuart Henderson wrote:
> On 2021/10/22 12:56, Stuart Henderson wrote:
> > On 2021/10/22 12:20, Antoine Jacoutot wrote:
> > > On Thu, Oct 21, 2021 at 04:45:47PM +0100, Stuart Henderson wrote:
> > > > Sometimes I find it useful to list daemons which are set to 'disabled'
> > > > but are actually running. Either those where I have started them by hand
> > > > forgotten to enable in rc.conf.local, or to check for services which
> > > > shouldn't be running but which are anyway. Any comments on this diff
> > > > to add it to rcctl? It's pretty much the opposite of "rcctl ls failed".
> > > 
> > > Hi.
> > > 
> > > I have never had a use for this, so I don't really have an opinion...
> > > I am not super fan of the "off-but-started" option name though.
> > 
> > I hate the name but every other idea I had was worse :)
> 
> Ingo had a nice suggestion:

OK kn



Re: unwind(8): give the first available resolver strategy a bit more time

2021-10-22 Thread Klemens Nanni
On Fri, Oct 22, 2021 at 06:10:32PM +0200, Florian Obser wrote:
> 
> unwind(8) gives the most preferred resolver strategy a bit more time
> (200ms) to answer before trying the next strategy. However, we need to
> skip strategies that are not available. In the default configuration,
> without a config file, unwind(8) would give DoT 200ms more time, but no
> DoT forwarders are known, so this is useless.

OK kn



unwind.conf.5: use braces in config examples

2021-10-22 Thread Klemens Nanni
We document them as explicitly required, `unwind -dnvf...' spits them
out like this and the last `force' example uses them as well.

OK?

Index: unwind.conf.5
===
RCS file: /cvs/src/sbin/unwind/unwind.conf.5,v
retrieving revision 1.30
diff -u -p -r1.30 unwind.conf.5
--- unwind.conf.5   31 Aug 2021 20:28:45 -  1.30
+++ unwind.conf.5   22 Oct 2021 14:28:37 -
@@ -150,8 +150,8 @@ block list "/etc/blocklist" log
 .Pp
 Define a DNS over TLS (DoT) forwarder and make it the preferred resolver:
 .Bd -literal -offset indent
-forwarder 192.168.1.250 port 8080 authentication name "resolver.local" DoT
-preference DoT
+forwarder { 192.168.1.250 port 8080 authentication name "resolver.local" DoT }
+preference { DoT }
 .Ed
 .Pp
 Where a domain requires a specific nameserver



Re: ixl(4): add checksum receive offloading

2021-10-22 Thread Klemens Nanni
On Fri, Oct 22, 2021 at 01:39:47PM +0200, Jan Klemkow wrote:
> On Fri, Oct 22, 2021 at 12:01:41PM +0200, Hrvoje Popovski wrote:
> > On 22.10.2021. 11:25, Jan Klemkow wrote:
> > > this diff add hardware checksum offloading for the receive path of
> > > ixl(4) interfaces.
> > > 
> > > Tested on:
> > > ixl1 at pci3 dev 0 function 1 "Intel X710 SFP+" rev 0x02: port 1, FW 
> > > 6.0.48442 API 1.7, msix, 8 queues, address 40:a6:b7:02:38:3d
> > > 
> > > OK?
> > 
> > I've applied this diff and i can't see anything regarding offload with
> > ifconfig ixl hwfeatures?
> 
> Hi Hrvoje,
> 
> Thats because, you only see this flags, if the checksum offloading is
> enabled for "sending".

Then ifconfig.8 should be say that this is TX only, no?

 hwfeatures  Display the interface hardware features:

   CSUM_IPv4   The device supports IPv4 checksum
   offload.



Re: fix symlink read in openrsync

2021-10-22 Thread Klemens Nanni
On Fri, Oct 22, 2021 at 11:50:03AM +0200, Claudio Jeker wrote:
> flist_gen_dirent() does a fts_read and inside that tries to read the
> symlink information. Now since fts_open did not specifiy FTS_NOCHDIR
> the symlink_read call needs to use ent->fts_accpath instead of f->path
> which was based on ent->fts_path.

OK kn



Re: xen.4: document how to inform Xen host of IP in VM

2021-10-21 Thread Klemens Nanni
On Thu, Oct 21, 2021 at 01:30:27PM +0200, Denis Fondras wrote:
> Document commands used to send VM IP to Xen host.

OK kn



Re: [PATCH] usr.sbin/ldapd: Match bind DN by suffix instead of complete DN.

2021-10-15 Thread Klemens Nanni
On Sun, Oct 03, 2021 at 10:05:56AM +, Klemens Nanni wrote:
> On Sat, Oct 02, 2021 at 07:03:21PM +0200, vifino wrote:
> > On Sat Oct 2, 2021 at 6:36 PM CEST, Raf Czlonka wrote:
> > > On Sat, Oct 02, 2021 at 02:15:53PM BST, vifino wrote:
> > > > Index: ldapd.conf.5
> > > > ===
> > > > RCS file: /cvs/src/usr.sbin/ldapd/ldapd.conf.5,v
> > > > retrieving revision 1.27
> > > > diff -u -p -u -p -r1.27 ldapd.conf.5
> > > > --- ldapd.conf.524 Jun 2020 07:20:47 -  1.27
> > > > +++ ldapd.conf.52 Oct 2021 12:43:29 -
> > > > @@ -270,7 +270,7 @@ Finally, the filter rule can match a bin
> > > >  The filter rule matches by any bind dn, including anonymous binds.
> > > >  .It by Ar DN
> > > >  The filter rule matches only if the requestor has previously performed
> > > > -a bind as the specified distinguished name.
> > > > +a bind as the specified distinguished name or a decendant.
> > >^
> > > A spellchecker[0] would have caught that ;^)
> > Ah, yes, of course. The one thing I spent zero effort on.
> > I haven't quite grokked the workflow, first submitted patch and all.
> > I'll certainly run `spell` next time.
> > 
> > I'll keep this in mind for the next one. ;)
> > >
> > > [0] https://manpages.bsd.lv/part3-3-2.html
> > >
> > > Regards,
> > >
> > > Raf
> > 
> > Revised patch below, not that it's necessary.
> > - vifino
> 
> The patch doesn't apply (for me) as your mail is quoted, here's your
> diff in 7bit.
> 
> Makes sense and works as expected.
> OK kn if some other LDAP hacker wants to commit, otherwise I'd make sure
> that this lands unless there are objections.

Any takers?  I plan to commit this by the end of the weekend.


Index: auth.c
===
RCS file: /cvs/src/usr.sbin/ldapd/auth.c,v
retrieving revision 1.14
diff -u -p -r1.14 auth.c
--- auth.c  24 Oct 2019 12:39:26 -  1.14
+++ auth.c  3 Oct 2021 09:25:10 -
@@ -94,8 +94,13 @@ aci_matches(struct aci *aci, struct conn
if (strcmp(aci->subject, "@") == 0) {
if (strcmp(dn, conn->binddn) != 0)
return 0;
-   } else if (strcmp(aci->subject, conn->binddn) != 0)
-   return 0;
+   } else {
+   key.size = strlen(conn->binddn);
+   key.data = conn->binddn;
+
+   if (!has_suffix(, aci->subject))
+   return 0;
+   }
}
 
if (aci->attribute != NULL) {
Index: ldapd.conf.5
===
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.conf.5,v
retrieving revision 1.27
diff -u -p -r1.27 ldapd.conf.5
--- ldapd.conf.524 Jun 2020 07:20:47 -  1.27
+++ ldapd.conf.53 Oct 2021 09:22:34 -
@@ -270,7 +270,7 @@ Finally, the filter rule can match a bin
 The filter rule matches by any bind dn, including anonymous binds.
 .It by Ar DN
 The filter rule matches only if the requestor has previously performed
-a bind as the specified distinguished name.
+a bind as the specified distinguished name or a descendant.
 .It by self
 The filter rule matches only if the requestor has previously performed
 a bind as the distinguished name that is being requested.



  1   2   3   4   5   6   7   8   9   10   >