Re: ksh tab completion bug

2020-11-10 Thread Anton Lindqvist
On Tue, Nov 10, 2020 at 06:49:28PM +0100, Sven M. Hallberg wrote:
> Apologies for jumping in as a bystander, but if I may comment:
> 
> Anton Lindqvist on Tue, Nov 10 2020:
> > Been brought up before[1] and rejected[2][3].
> 
> Anton Lindqvist on Sun, Jul 02, 2017:
> > diff below in which slashes are discarded when comparing the length. I
> > don't know if any other character should be discarded as well, if true
> > then it might be worth passing the input buffer through ksh's own
> > lexer and [...]
> 
> The patch then seems to have been rejected for further complicating some
> already hairy code and the above does sound rather unsafe.
> 
> Michael's aim here appears to be to simplify things:
> 
> Michael Forney on Mon, Nov 09, 2020:
> > emacs.c:do_complete() using `end - start` for olen (which counts the
> > backslashes), but nlen (the longest prefix length) does not. [...]
> >
> > I don't believe vi.c:complete_word() has this issue [...]
> > 
> > [my diff] is longer than I had hoped. Perhaps there is a simpler patch
> > to make emacs.c:do_complete() use the same approach as vi completion.
> 
> Maybe some iteration is all that is needed.

I not saying the diff is bad in anyway, just sharing a related data
point. I do think this would be an improvement and the approach looks
better than mine.

As I would address this, the numbers of arguments passed to the
completion related routines is already painfully^W long. I would rather
take a step back and introduce a `struct globstate' (just an example,
could be renamed) which includes all the necessary parameters. Getting
such refactoring in place first would make the diff even smaller.

Also, emacs mode has its own regress suite located in
regress/bin/ksh/edit/emacs.sh. Tricky logic like this should be covered
in my opinion. Below is a rough diff how to test file name completion.

Michael, is this something you would like to continue work on?

Index: emacs.sh
===
RCS file: /cvs/src/regress/bin/ksh/edit/emacs.sh,v
retrieving revision 1.11
diff -u -p -r1.11 emacs.sh
--- emacs.sh3 Apr 2019 14:59:34 -   1.11
+++ emacs.sh10 Nov 2020 19:53:30 -
@@ -139,7 +139,11 @@ testseq "a\0033#" " # a\r # #a \0010\001
 
 # XXX ^[^[: complete
 # XXX ^X^[: complete-command
-# XXX ^[^X: complete-file
+
+# ^[^X: complete-file
+: >abc
+testseq "ls \0033\0030" " # abc"
+
 # XXX ^I, ^[=: complete-list
 
 # [n] ERASE, ^?, ^H: delete-char-backward
Index: subr.sh
===
RCS file: /cvs/src/regress/bin/ksh/edit/subr.sh,v
retrieving revision 1.8
diff -u -p -r1.8 subr.sh
--- subr.sh 21 Nov 2017 19:25:46 -  1.8
+++ subr.sh 10 Nov 2020 19:53:30 -
@@ -18,7 +18,7 @@
 testseq() {
stdin=$1
exp=$(echo "$2")
-   act=$(echo -n "$stdin" | ./edit -p "$PS1" ${KSH:-/bin/ksh} -r 2>&1)
+   act=$(echo -n "$stdin" | "$EDIT" -p "$PS1" ${KSH:-/bin/ksh} -r 2>&1)
[ $? = 0 ] && [ "$exp" = "$act" ] && return 0
 
dump input "$stdin"
@@ -32,3 +32,9 @@ dump() {
printf '%s:\n>>>%s<<<\n' "$1" "$(echo -n "$2" | vis -ol)"
echo -n "$2" | hexdump -Cv
 }
+
+EDIT="${PWD}/edit"
+
+TMP="$(mktemp -dt ksh.XX)"
+trap 'rm -r $TMP' EXIT
+cd "$TMP"



Re: ksh tab completion bug

2020-11-09 Thread Anton Lindqvist
On Mon, Nov 09, 2020 at 11:15:36PM -0800, Michael Forney wrote:
> I noticed some strange behavior of ksh in emacs mode when completing
> file names that contain spaces (or other characters that need
> escaping).
> 
> To illustrate the problem, consider two files 'a b c test1' and
> 'a b c test2'. ksh will correctly complete `a` and `a\ b\ c\ ` to
> the common prefix `a\ b\ c\ test`. However, all of the following
> do not get completed:
> 
> * `a\ b\ c\ t`
> * `a\ b\ c\ te`
> * `a\ b\ c\ tes`
> 
> I did some debugging, and I think this is due to emacs.c:do_complete()
> using `end - start` for olen (which counts the backslashes), but
> nlen (the longest prefix length) does not. So the completion only
> occurs when the current word is shorter than the common prefix
> length minus the number of backslashes in the word.
> 
> I don't believe vi.c:complete_word() has this issue since it always
> replaces the word with the longest prefix.
> 
> I wrote a (only lightly tested) patch to make x_cf_glob return the
> unescaped length as well as the start and end positions, and use
> that for olen instead of `end - start`. This seems to fix the issue,
> but it is longer than I had hoped. Perhaps there is a simpler patch
> to make emacs.c:do_complete() use the same approach as vi completion.

Been brought up before[1] and rejected[2][3].

[1] https://marc.info/?l=openbsd-bugs=149902839123905=2
[2] https://marc.info/?l=openbsd-bugs=149925960003395=2
[3] https://marc.info/?l=openbsd-bugs=149925991603581=2



Re: process: annotate locking for setitimer(2) state

2020-08-11 Thread Anton Lindqvist
On Sun, Aug 09, 2020 at 05:33:58PM +0200, Mark Kettenis wrote:
> > Date: Sun, 9 Aug 2020 10:02:38 -0500
> > From: Scott Cheloha 
> > 
> > On Sun, Aug 09, 2020 at 04:43:24PM +0200, Mark Kettenis wrote:
> > > > Date: Sat, 8 Aug 2020 19:46:14 -0500
> > > > From: Scott Cheloha 
> > > > 
> > > > Hi,
> > > > 
> > > > I want to annotate the locking for the per-process interval timers.
> > > > 
> > > > In the process struct, the ITIMER_REAL itimerspec and the ps_itimer_to
> > > > timeout are protected by the kernel lock.  These should be annotated
> > > > with "K", right?
> > > > 
> > > > Also in the process struct, the ITIMER_VIRTUAL and ITIMER_PROF
> > > > itimerspecs are protected by the global itimer_mtx.
> > > > 
> > > > However, I don't think "itimer_mtx" isn't the best name for it, as it
> > > > doesn't protect state for *all* per-process interval timers.  Just the
> > > > virtual ones.
> > > > 
> > > > Could I rename the mutex to "virtual_itimer_mtx"?  Then I can annotate
> > > > the state protected by it with "V", as shown here in this patch.
> > > 
> > > That's quite a long variable name though.  And it also protects
> > > ITIMER_PROF.  So I'd say the name would be at least as misleading as
> > > the current one and perhaps even more so.  You can just use "I" as the
> > > annotation perhaps?
> > 
> > The convention is to use "I" for immutable variables.  We do it
> > everywhere.  I don't think we should buck convention here.
> > 
> > I also proposed using "i" in a prior patch to annotate these
> > variables, but mpi@ said it was too close to "I".  Also, it's a global
> > lock, and we have settled on only annotate global locks with capital
> > letters.
> > 
> > If you don't want to rename the mutex I guess we could use "T" for
> > "timer".  We use "T" for other global locks (tc_lock, timeout_mutex)
> > but not in this context.
> > 
> > However, there are only so many letters.  Eventually this scheme will
> > run afoul of that limitation.  An idea I had re. the letter shortage
> > was to use two letters where necessary.  So instead of "I" you could
> > use "It" for "itimer".  We annotate locking hierarchies with commas so
> > there isn't an ambiguity when reading it.
> > 
> > For example, if the code for writing a hypothetical "ps_foo" process
> > struct member was:
> > 
> > KERNEL_LOCK();
> > mtx_enter(_mtx);
> > ps.ps_foo = 10;
> > mtx_leave(_mtx);
> > KERNEL_UNLOCK();
> > 
> > You could annotate it like this:
> > 
> > /*
> >  * Locks used to protect process struct members:
> >  *
> >  *  It  itimer_mtx
> >  *  K   kernel lock
> >  */
> > struct process {
> > /* [...] */
> > int ps_foo; /* [K,It] per-process foobar */
> > /* [...] */
> > };
> > 
> > anton@, mpi@: is that too radical or easily misread?
> > 
> > Sorry if this all seems fussy, but I'd like to get this right the
> > first time.
> 
> 'T' is fine with me.  But I'm clearly not an authority here.  Anyway,
> renaming variables because you don't have a matching letter to
> annotate the lock doesn't feel right.

I'm also fine with T.



harmonize locking annotations

2020-06-29 Thread Anton Lindqvist
Hi,
I think we all agree that global locks should be represented using
uppercase letters in locking annotations. This is an attempt to
harmonize the existing annotations.

Comments? OK?

Index: dev/dt/dt_dev.c
===
RCS file: /cvs/src/sys/dev/dt/dt_dev.c,v
retrieving revision 1.7
diff -u -p -r1.7 dt_dev.c
--- dev/dt/dt_dev.c 27 Jun 2020 07:22:09 -  1.7
+++ dev/dt/dt_dev.c 29 Jun 2020 18:47:30 -
@@ -74,19 +74,19 @@
  *
  *  Locks used to protect struct members in this file:
  * m   per-softc mutex
- * k   kernel lock
+ * K   kernel lock
  */
 struct dt_softc {
-   SLIST_ENTRY(dt_softc)ds_next;   /* [k] descriptor list */
+   SLIST_ENTRY(dt_softc)ds_next;   /* [K] descriptor list */
int  ds_unit;   /* [I] D_CLONE unique unit */
pid_tds_pid;/* [I] PID of tracing program */
 
struct mutex ds_mtx;
 
-   struct dt_pcb_list   ds_pcbs;   /* [k] list of enabled PCBs */
-   struct dt_evt   *ds_bufqueue;   /* [k] copy evts to userland */
-   size_t   ds_bufqlen;/* [k] length of the queue */
-   int  ds_recording;  /* [k] currently recording? */
+   struct dt_pcb_list   ds_pcbs;   /* [K] list of enabled PCBs */
+   struct dt_evt   *ds_bufqueue;   /* [K] copy evts to userland */
+   size_t   ds_bufqlen;/* [K] length of the queue */
+   int  ds_recording;  /* [K] currently recording? */
int  ds_evtcnt; /* [m] # of readable evts */
 
/* Counters */
@@ -94,7 +94,7 @@ struct dt_softc {
uint64_t ds_dropevt;/* [m] # of events dropped */
 };
 
-SLIST_HEAD(, dt_softc) dtdev_list; /* [k] list of open /dev/dt nodes */
+SLIST_HEAD(, dt_softc) dtdev_list; /* [K] list of open /dev/dt nodes */
 
 /*
  * Probes are created during dt_attach() and never modified/freed during
@@ -104,7 +104,7 @@ unsigned intdt_nprobes; /* [I] 
# of p
 SIMPLEQ_HEAD(, dt_probe)   dt_probe_list;  /* [I] list of probes */
 
 struct rwlock  dt_lock = RWLOCK_INITIALIZER("dtlk");
-volatile uint32_t  dt_tracing = 0; /* [k] # of processes tracing */
+volatile uint32_t  dt_tracing = 0; /* [K] # of processes tracing */
 
 void   dtattach(struct device *, struct device *, void *);
 intdtopen(dev_t, int, int, struct proc *);
Index: dev/dt/dtvar.h
===
RCS file: /cvs/src/sys/dev/dt/dtvar.h,v
retrieving revision 1.3
diff -u -p -r1.3 dtvar.h
--- dev/dt/dtvar.h  28 Mar 2020 15:42:25 -  1.3
+++ dev/dt/dtvar.h  29 Jun 2020 18:47:30 -
@@ -159,14 +159,14 @@ int   dtioc_req_isvalid(struct dtioc_req 
  *
  *  Locks used to protect struct members in this file:
  * I   immutable after creation
- * k   kernel lock
- * k,s kernel lock for writting and SMR for reading
+ * K   kernel lock
+ * K,S kernel lock for writting and SMR for reading
  * m   per-pcb mutex
  * c   owned (read & modified) by a single CPU
  */
 struct dt_pcb {
-   SMR_SLIST_ENTRY(dt_pcb)  dp_pnext;  /* [k,s] next PCB per probe */
-   TAILQ_ENTRY(dt_pcb)  dp_snext;  /* [k] next PCB per softc */
+   SMR_SLIST_ENTRY(dt_pcb)  dp_pnext;  /* [K,S] next PCB per probe */
+   TAILQ_ENTRY(dt_pcb)  dp_snext;  /* [K] next PCB per softc */
 
/* Event states ring */
unsigned int dp_prod;   /* [m] read index */
@@ -203,18 +203,18 @@ void   dt_pcb_ring_consume(struct dt_pcb
  *
  *  Locks used to protect struct members in this file:
  * I   immutable after creation
- * k   kernel lock
- * d   dt_lock
- * d,s dt_lock for writting and SMR for reading
+ * K   kernel lock
+ * D   dt_lock
+ * D,S dt_lock for writting and SMR for reading
  */
 struct dt_probe {
-   SIMPLEQ_ENTRY(dt_probe)  dtp_next;  /* [k] global list of probes */
-   SMR_SLIST_HEAD(, dt_pcb) dtp_pcbs;  /* [d,s] list of enabled PCBs */
+   SIMPLEQ_ENTRY(dt_probe)  dtp_next;  /* [K] global list of probes */
+   SMR_SLIST_HEAD(, dt_pcb) dtp_pcbs;  /* [D,S] list of enabled PCBs */
struct dt_provider  *dtp_prov;  /* [I] its to provider */
const char  *dtp_func;  /* [I] probe function */
const char  *dtp_name;  /* [I] probe name */
uint32_t dtp_pbn;   /* [I] unique ID */
-   volatile uint32_tdtp_recording; /* [d] is it recording? */
+   volatile uint32_tdtp_recording; /* [D] is it recording? */
uint8_t  

Re: pipe: reduce number of allocations

2020-06-23 Thread Anton Lindqvist
On Tue, Jun 16, 2020 at 09:10:54PM +0200, Anton Lindqvist wrote:
> Hi,
> Instead of performing three distinct allocations per created pipe,
> reduce it to a single one. Not only should this be more performant, it
> also solves a kqueue related issue found by visa@ who also requested
> this change:
> 
> > If you attach an EVFILT_WRITE filter to a pipe fd, the knote gets added
> > to the peer's klist. This is a problem for kqueue  because if you close
> > the peer's fd, the knote is left in the list whose head is about to be
> > freed. knote_fdclose() is not able to clear the knote because it is not
> > registered with the peer's fd.
> 
> FreeBSD also takes a similar approach to pipe allocations.
> 
> Comments? OK?

This was backed out since it contained bug. Updated diff below in which
pipe_buffer_free() is called while holding the pipe lock in
pipe_destroy(). This prevents another thread from freeing the pipe_pair
structure while another thread is sleeping inside pipe_buffer_free().

Note that pipe_buffer_free() is also called from pipe_buffer_realloc().
The pipe is then either being created, i.e. we have exclusive access at
this point or the pipe lock is already acquired.

Comments? OK?

Index: kern/sys_pipe.c
===
RCS file: /cvs/src/sys/kern/sys_pipe.c,v
retrieving revision 1.122
diff -u -p -r1.122 sys_pipe.c
--- kern/sys_pipe.c 19 Jun 2020 02:08:48 -  1.122
+++ kern/sys_pipe.c 22 Jun 2020 19:23:56 -
@@ -49,6 +49,12 @@
 
 #include 
 
+struct pipe_pair {
+   struct pipe pp_wpipe;
+   struct pipe pp_rpipe;
+   struct rwlock pp_lock;
+};
+
 /*
  * interfaces to the outside world
  */
@@ -103,13 +109,12 @@ const struct filterops pipe_wfiltops = {
 unsigned int nbigpipe;
 static unsigned int amountpipekva;
 
-struct pool pipe_pool;
-struct pool pipe_lock_pool;
+struct pool pipe_pair_pool;
 
 intdopipe(struct proc *, int *, int);
 void   pipeselwakeup(struct pipe *);
 
-struct pipe *pipe_create(void);
+intpipe_create(struct pipe *);
 void   pipe_destroy(struct pipe *);
 intpipe_rundown(struct pipe *);
 struct pipe *pipe_peer(struct pipe *);
@@ -120,6 +125,8 @@ int pipe_iolock(struct pipe *);
 void   pipe_iounlock(struct pipe *);
 intpipe_iosleep(struct pipe *, const char *);
 
+struct pipe_pair *pipe_pair_create(void);
+
 /*
  * The pipe system call for the DTYPE_PIPE type of pipes
  */
@@ -153,33 +160,17 @@ dopipe(struct proc *p, int *ufds, int fl
 {
struct filedesc *fdp = p->p_fd;
struct file *rf, *wf;
+   struct pipe_pair *pp;
struct pipe *rpipe, *wpipe = NULL;
-   struct rwlock *lock;
int fds[2], cloexec, error;
 
cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
 
-   if ((rpipe = pipe_create()) == NULL) {
-   error = ENOMEM;
-   goto free1;
-   }
-
-   /*
-* One lock is used per pipe pair in order to obtain exclusive access to
-* the pipe pair.
-*/
-   lock = pool_get(_lock_pool, PR_WAITOK);
-   rw_init(lock, "pipelk");
-   rpipe->pipe_lock = lock;
-
-   if ((wpipe = pipe_create()) == NULL) {
-   error = ENOMEM;
-   goto free1;
-   }
-   wpipe->pipe_lock = lock;
-
-   rpipe->pipe_peer = wpipe;
-   wpipe->pipe_peer = rpipe;
+   pp = pipe_pair_create();
+   if (pp == NULL)
+   return (ENOMEM);
+   wpipe = >pp_wpipe;
+   rpipe = >pp_rpipe;
 
fdplock(fdp);
 
@@ -226,7 +217,6 @@ free3:
rpipe = NULL;
 free2:
fdpunlock(fdp);
-free1:
pipe_destroy(wpipe);
pipe_destroy(rpipe);
return (error);
@@ -272,19 +262,14 @@ pipe_buffer_realloc(struct pipe *cpipe, 
 /*
  * initialize and allocate VM and memory for pipe
  */
-struct pipe *
-pipe_create(void)
+int
+pipe_create(struct pipe *cpipe)
 {
-   struct pipe *cpipe;
int error;
 
-   cpipe = pool_get(_pool, PR_WAITOK | PR_ZERO);
-
error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
-   if (error != 0) {
-   pool_put(_pool, cpipe);
-   return (NULL);
-   }
+   if (error != 0)
+   return (error);
 
sigio_init(>pipe_sigio);
 
@@ -292,7 +277,7 @@ pipe_create(void)
cpipe->pipe_atime = cpipe->pipe_ctime;
cpipe->pipe_mtime = cpipe->pipe_ctime;
 
-   return (cpipe);
+   return (0);
 }
 
 struct pipe *
@@ -834,7 +819,6 @@ void
 pipe_destroy(struct pipe *cpipe)
 {
struct pipe *ppipe;
-   struct rwlock *lock = NULL;
 
if (cpipe == NULL)
return;
@@ -862,20 +846,14 @@ pipe_destroy(struct pipe *cpipe)
ppipe->pipe_state |= PIPE_EOF;
wakeup(ppipe);
ppipe->pipe_peer = NULL;
-   } else {
-   /*
-

pipe: reduce number of allocations

2020-06-16 Thread Anton Lindqvist
Hi,
Instead of performing three distinct allocations per created pipe,
reduce it to a single one. Not only should this be more performant, it
also solves a kqueue related issue found by visa@ who also requested
this change:

> If you attach an EVFILT_WRITE filter to a pipe fd, the knote gets added
> to the peer's klist. This is a problem for kqueue  because if you close
> the peer's fd, the knote is left in the list whose head is about to be
> freed. knote_fdclose() is not able to clear the knote because it is not
> registered with the peer's fd.

FreeBSD also takes a similar approach to pipe allocations.

Comments? OK?

Index: kern/sys_pipe.c
===
RCS file: /cvs/src/sys/kern/sys_pipe.c,v
retrieving revision 1.120
diff -u -p -r1.120 sys_pipe.c
--- kern/sys_pipe.c 15 Jun 2020 15:29:40 -  1.120
+++ kern/sys_pipe.c 16 Jun 2020 19:07:49 -
@@ -49,6 +49,12 @@
 
 #include 
 
+struct pipe_pair {
+   struct pipe pp_wpipe;
+   struct pipe pp_rpipe;
+   struct rwlock pp_lock;
+};
+
 /*
  * interfaces to the outside world
  */
@@ -103,13 +109,12 @@ const struct filterops pipe_wfiltops = {
 unsigned int nbigpipe;
 static unsigned int amountpipekva;
 
-struct pool pipe_pool;
-struct pool pipe_lock_pool;
+struct pool pipe_pair_pool;
 
 intdopipe(struct proc *, int *, int);
 void   pipeselwakeup(struct pipe *);
 
-struct pipe *pipe_create(void);
+intpipe_create(struct pipe *);
 void   pipe_destroy(struct pipe *);
 intpipe_rundown(struct pipe *);
 struct pipe *pipe_peer(struct pipe *);
@@ -120,6 +125,8 @@ int pipe_iolock(struct pipe *);
 void   pipe_iounlock(struct pipe *);
 intpipe_iosleep(struct pipe *, const char *);
 
+struct pipe_pair *pipe_pair_create(void);
+
 /*
  * The pipe system call for the DTYPE_PIPE type of pipes
  */
@@ -153,33 +160,17 @@ dopipe(struct proc *p, int *ufds, int fl
 {
struct filedesc *fdp = p->p_fd;
struct file *rf, *wf;
+   struct pipe_pair *pp;
struct pipe *rpipe, *wpipe = NULL;
-   struct rwlock *lock;
int fds[2], cloexec, error;
 
cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
 
-   if ((rpipe = pipe_create()) == NULL) {
-   error = ENOMEM;
-   goto free1;
-   }
-
-   /*
-* One lock is used per pipe pair in order to obtain exclusive access to
-* the pipe pair.
-*/
-   lock = pool_get(_lock_pool, PR_WAITOK);
-   rw_init(lock, "pipelk");
-   rpipe->pipe_lock = lock;
-
-   if ((wpipe = pipe_create()) == NULL) {
-   error = ENOMEM;
-   goto free1;
-   }
-   wpipe->pipe_lock = lock;
-
-   rpipe->pipe_peer = wpipe;
-   wpipe->pipe_peer = rpipe;
+   pp = pipe_pair_create();
+   if (pp == NULL)
+   return (ENOMEM);
+   wpipe = >pp_wpipe;
+   rpipe = >pp_rpipe;
 
fdplock(fdp);
 
@@ -226,7 +217,6 @@ free3:
rpipe = NULL;
 free2:
fdpunlock(fdp);
-free1:
pipe_destroy(wpipe);
pipe_destroy(rpipe);
return (error);
@@ -272,19 +262,14 @@ pipe_buffer_realloc(struct pipe *cpipe, 
 /*
  * initialize and allocate VM and memory for pipe
  */
-struct pipe *
-pipe_create(void)
+int
+pipe_create(struct pipe *cpipe)
 {
-   struct pipe *cpipe;
int error;
 
-   cpipe = pool_get(_pool, PR_WAITOK | PR_ZERO);
-
error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
-   if (error != 0) {
-   pool_put(_pool, cpipe);
-   return (NULL);
-   }
+   if (error != 0)
+   return (error);
 
sigio_init(>pipe_sigio);
 
@@ -292,7 +277,7 @@ pipe_create(void)
cpipe->pipe_atime = cpipe->pipe_ctime;
cpipe->pipe_mtime = cpipe->pipe_ctime;
 
-   return (cpipe);
+   return (0);
 }
 
 struct pipe *
@@ -834,7 +819,6 @@ void
 pipe_destroy(struct pipe *cpipe)
 {
struct pipe *ppipe;
-   struct rwlock *lock = NULL;
 
if (cpipe == NULL)
return;
@@ -862,20 +846,13 @@ pipe_destroy(struct pipe *cpipe)
ppipe->pipe_state |= PIPE_EOF;
wakeup(ppipe);
ppipe->pipe_peer = NULL;
-   } else {
-   /*
-* Peer already gone. This is last reference to the pipe lock
-* and it must therefore be freed below.
-*/
-   lock = cpipe->pipe_lock;
}
 
rw_exit_write(cpipe->pipe_lock);
 
pipe_buffer_free(cpipe);
-   if (lock != NULL)
-   pool_put(_lock_pool, lock);
-   pool_put(_pool, cpipe);
+   if (ppipe == NULL)
+   pool_put(_pair_pool, cpipe->pipe_pair);
 }
 
 /*
@@ -1008,8 +985,33 @@ filt_pipewrite(struct knote *kn, long hi
 void
 pipe_init(void)
 {
-   pool_init(_pool, sizeof(struct pipe), 0, IPL_MPFLOOR, PR_WAITOK,
-   "pipepl", NULL);
-   pool_init(_lock_pool, sizeof(struct rwlock), 0, IPL_MPFLOOR,

Re: Call FRELE() without fdplock in dup*(2)

2020-06-10 Thread Anton Lindqvist
On Wed, Jun 10, 2020 at 02:34:00PM +, Visa Hankala wrote:
> A while ago, finishdup() was changed to release fdplock before calling
> closef() to avoid potential lock order problems in the file close path.
> However, the dup* code can still invoke that path with fdplock held
> through FRELE(). That is fixed by the diff below.
> 
> (In principle, the FRELE(fp, p) could be eliminated by getting fp while
> the file descriptor table is locked. As long as the lock is held, other
> threads are unable to close the file descriptor and hence the temporary
> file reference is unnecessary.)
> 
> OK?

ok anton@



ddb(4): missing tags

2020-05-17 Thread Anton Lindqvist
Hi,
The ddb(4) manual documents a couple of commands which can be
abbreviated. The diff below adds explicit tags for such commands which
in turn makes it possible to jump to for instance `examine' from within
your $PAGER.

Comments? OK?

Index: ddb.4
===
RCS file: /cvs/src/share/man/man4/ddb.4,v
retrieving revision 1.96
diff -u -p -r1.96 ddb.4
--- ddb.4   14 May 2020 06:58:54 -  1.96
+++ ddb.4   17 May 2020 08:54:18 -
@@ -202,6 +202,7 @@ are displayed and no other action is per
 .It Ic help
 List the available commands.
 .\" 
+.Tg examine
 .It Xo
 .Oo Ic e Oc Ns
 .Ic x Ns Op Ic amine
@@ -275,6 +276,7 @@ is set to the
 .Ar addr
 plus the size of the data examined.
 .\" 
+.Tg print
 .It Xo
 .Ic p Ns Op Ic rint
 .Op Cm /axzodurc
@@ -301,6 +303,7 @@ will print something like this:
 xx
 .Ed
 .\" 
+.Tg pprint
 .It Xo
 .Ic pp Ns Op Ic rint
 .Op Ar addr
@@ -316,6 +319,7 @@ as part of building a new kernel.
 .\" .Op Ar addr
 .\" .Ar expr Op expr ...
 .\" .Xc
+.Tg write
 .It Xo
 .Ic w Ns Op Ic rite
 .Op Cm /bhl
@@ -394,6 +398,7 @@ allows the breakpoint to be silently hit
 times before stopping at the
 break point.
 .\" 
+.Tg delete
 .It Xo
 .Ic d Ns Op Ic elete
 .Op Ar addr
@@ -405,6 +410,7 @@ command.
 .\" .It Xo Ic s Ns Op Cm /p
 .\" .Op Ic \&, Ns Ar count
 .\" .Xc
+.Tg step
 .It Xo
 .Ic s Ns Op Ic tep
 .Op Cm /p
@@ -437,6 +443,7 @@ Parentheses may be omitted if the functi
 The number of arguments is currently limited to 10.
 .\" 
 .\" .It Ic c Ns Op Cm /c
+.Tg continue
 .It Xo
 .Ic c Ns Op Ic ontinue
 .Op Cm /c
@@ -949,6 +956,7 @@ command.
 A synonym for
 .Ic show all procs .
 .\" 
+.Tg machine
 .It Xo
 .Ic mac Ns Op Ic hine
 .Ar subcommand Op Ar args ...



flock unlock

2020-03-13 Thread Anton Lindqvist
Hi,
After reading the flock(2) code I cannot spot anything preventing it
from being unlocked. This is all made possible by the recent MP-safe
effort including atomic file flags and posix file locks.

I've been running with this for a week by now. However, testing is much
appreciated. Remember to run `make -C sys/kern syscalls' after applying
the diff.

I'm not rushing this one as fcntl(2) was recently unlocked. Will commit
later next week unless I hear objections.

Comments? OK?

Index: kern/syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.206
diff -u -p -r1.206 syscalls.master
--- kern/syscalls.master10 Mar 2020 19:20:14 -  1.206
+++ kern/syscalls.master13 Mar 2020 10:13:03 -
@@ -260,7 +260,7 @@
 128STD { int sys_rename(const char *from, const char *to); }
 129OBSOL   otruncate
 130OBSOL   oftruncate
-131STD { int sys_flock(int fd, int how); }
+131STD NOLOCK  { int sys_flock(int fd, int how); }
 132STD { int sys_mkfifo(const char *path, mode_t mode); }
 133STD NOLOCK  { ssize_t sys_sendto(int s, const void *buf, \
size_t len, int flags, const struct sockaddr *to, \



atomic f_iflags

2020-03-10 Thread Anton Lindqvist
Hi,
In order to unlock flock(2), make writes to the f_iflags field of struct
file atomic; similar to recent changes to both struct file and process.
This also gets rid of the last kernel lock protected field in the scope
of struct file.

Comments? OK?

Index: kern/kern_descrip.c
===
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.200
diff -u -p -r1.200 kern_descrip.c
--- kern/kern_descrip.c 26 Feb 2020 13:54:52 -  1.200
+++ kern/kern_descrip.c 5 Mar 2020 11:51:28 -
@@ -707,7 +707,7 @@ fdinsert(struct filedesc *fdp, int fd, i
 
mtx_enter();
if ((fp->f_iflags & FIF_INSERTED) == 0) {
-   fp->f_iflags |= FIF_INSERTED;
+   atomic_setbits_int(>f_iflags, FIF_INSERTED);
if ((fq = fdp->fd_ofiles[0]) != NULL) {
LIST_INSERT_AFTER(fq, fp, f_list);
} else {
@@ -1317,7 +1317,7 @@ sys_flock(struct proc *p, void *v, regis
lf.l_len = 0;
if (how & LOCK_UN) {
lf.l_type = F_UNLCK;
-   fp->f_iflags &= ~FIF_HASLOCK;
+   atomic_clearbits_int(>f_iflags, FIF_HASLOCK);
error = VOP_ADVLOCK(vp, (caddr_t)fp, F_UNLCK, , F_FLOCK);
goto out;
}
@@ -1329,7 +1329,7 @@ sys_flock(struct proc *p, void *v, regis
error = EINVAL;
goto out;
}
-   fp->f_iflags |= FIF_HASLOCK;
+   atomic_setbits_int(>f_iflags, FIF_HASLOCK);
if (how & LOCK_NB)
error = VOP_ADVLOCK(vp, (caddr_t)fp, F_SETLK, , F_FLOCK);
else
Index: kern/vfs_syscalls.c
===
RCS file: /cvs/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.342
diff -u -p -r1.342 vfs_syscalls.c
--- kern/vfs_syscalls.c 30 Jan 2020 15:36:11 -  1.342
+++ kern/vfs_syscalls.c 5 Mar 2020 11:51:28 -
@@ -1188,7 +1188,7 @@ doopenat(struct proc *p, int fd, const c
goto out;
}
vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
-   fp->f_iflags |= FIF_HASLOCK;
+   atomic_setbits_int(>f_iflags, FIF_HASLOCK);
}
if (localtrunc) {
if ((fp->f_flag & FWRITE) == 0)
@@ -1457,7 +1457,7 @@ sys_fhopen(struct proc *p, void *v, regi
goto bad;
}
vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
-   fp->f_iflags |= FIF_HASLOCK;
+   atomic_setbits_int(>f_iflags, FIF_HASLOCK);
}
VOP_UNLOCK(vp);
*retval = indx;
Index: sys/file.h
===
RCS file: /cvs/src/sys/sys/file.h,v
retrieving revision 1.60
diff -u -p -r1.60 file.h
--- sys/file.h  1 Feb 2020 08:57:27 -   1.60
+++ sys/file.h  5 Mar 2020 11:51:28 -
@@ -75,7 +75,6 @@ structfileops {
  * a   atomic operations
  * f   per file `f_mtx'
  * v   vnode lock
- * k   kernel lock
  */
 struct file {
LIST_ENTRY(file) f_list;/* [F] list of active files */
@@ -86,7 +85,7 @@ struct file {
 #defineDTYPE_PIPE  3   /* pipe */
 #defineDTYPE_KQUEUE4   /* event queue */
 #defineDTYPE_DMABUF5   /* DMA buffer (for DRM) */
-   int f_iflags;   /* [k] internal flags */
+   u_int   f_iflags;   /* [a] internal flags */
int f_type; /* [I] descriptor type */
u_int   f_count;/* [a] reference count */
struct  ucred *f_cred;  /* [I] credentials associated with descriptor */
Index: sys/sysctl.h
===
RCS file: /cvs/src/sys/sys/sysctl.h,v
retrieving revision 1.204
diff -u -p -r1.204 sysctl.h
--- sys/sysctl.h16 Feb 2020 07:55:30 -  1.204
+++ sys/sysctl.h5 Mar 2020 11:51:28 -
@@ -735,7 +735,7 @@ do {
\
 struct kinfo_file {
uint64_tf_fileaddr; /* PTR: address of struct file */
uint32_tf_flag; /* UINT: flags (see fcntl.h) */
-   uint32_tf_iflags;   /* INT: internal flags */
+   uint32_tf_iflags;   /* UINT: internal flags */
uint32_tf_type; /* INT: descriptor type */
uint32_tf_count;/* UINT: reference count */
uint32_tf_msgcount; /* UINT: references from msg queue */



fcntl unlock

2020-03-05 Thread Anton Lindqvist
Hi,
After reading the fcntl(2) code I cannot spot anything obvious
preventing it from being unlocked[1]. This is all made possible by the
recent MP-safe effort including resource limits, atomic file flags,
ioctl unlock and posix file locks.

I've been running with this for a week by now. However, testing is much
appreciated. Remember to run `make -C sys/kern syscalls' after applying
the diff.

Comments? OK?

[1] The same is also true for flock(2) but one step at a time.

Index: kern/syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.205
diff -u -p -r1.205 syscalls.master
--- kern/syscalls.master22 Feb 2020 12:00:24 -  1.205
+++ kern/syscalls.master4 Mar 2020 20:05:30 -
@@ -203,7 +203,7 @@
 90 STD NOLOCK  { int sys_dup2(int from, int to); }
 91 STD NOLOCK  { int sys_nanosleep(const struct timespec *rqtp, \
struct timespec *rmtp); }
-92 STD { int sys_fcntl(int fd, int cmd, ... void *arg); }
+92 STD NOLOCK  { int sys_fcntl(int fd, int cmd, ... void *arg); }
 93 STD { int sys_accept4(int s, struct sockaddr *name, \
socklen_t *anamelen, int flags); }
 94 STD NOLOCK  { int sys___thrsleep(const volatile void *ident, \



rwlock(9): xr rwsleep(9)

2019-11-04 Thread Anton Lindqvist
ok?

Index: rwlock.9
===
RCS file: /cvs/src/share/man/man9/rwlock.9,v
retrieving revision 1.24
diff -u -p -r1.24 rwlock.9
--- rwlock.925 Feb 2019 22:03:56 -  1.24
+++ rwlock.94 Nov 2019 14:25:19 -
@@ -238,6 +238,7 @@ Lock is not locked.
 .Sh SEE ALSO
 .Xr witness 4 ,
 .Xr mutex 9 ,
+.Xr rwsleep 9 ,
 .Xr spl 9
 .Sh HISTORY
 The



Re: vmctl: invalid parent template error

2019-08-12 Thread Anton Lindqvist
On Mon, Aug 12, 2019 at 02:52:46PM +0200, Klemens Nanni wrote:
> On Mon, Aug 12, 2019 at 02:14:42PM +0200, Anton Lindqvist wrote:
> > Hi,
> > I recently fat fingered the vm template passed to vmctl and was greeted
> > with the following error:
> > 
> >   vmctl: start vm command failed: Operation not permitted
> > 
> > I think we can be more specific in order to improve usability:
> I agree, but your wording implies the template might be there but vmd
> failed to find it.  Also, "parent" and "template" seems too much;  we
> have no other templates, so may I suggest one of the simpler
> 
>   invalid template
>   template does not exist
> 

Updated diff changing the error message to invalid template.

Index: vmctl/vmctl.c
===
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.c,v
retrieving revision 1.69
diff -u -p -r1.69 vmctl.c
--- vmctl/vmctl.c   22 May 2019 16:19:21 -  1.69
+++ vmctl/vmctl.c   12 Aug 2019 16:18:22 -
@@ -249,6 +249,10 @@ vm_start_complete(struct imsg *imsg, int
"file");
*ret = ENOENT;
break;
+   case VMD_PARENT_INVALID:
+   warnx("invalid template");
+   *ret = EINVAL;
+   break;
default:
errno = res;
warn("start vm command failed");
Index: vmd/vmd.c
===
RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
retrieving revision 1.114
diff -u -p -r1.114 vmd.c
--- vmd/vmd.c   28 Jun 2019 13:32:51 -  1.114
+++ vmd/vmd.c   12 Aug 2019 16:18:22 -
@@ -1373,8 +1373,13 @@ vm_instance(struct privsep *ps, struct v
 
/* return without error if the parent is NULL (nothing to inherit) */
if ((vmc->vmc_flags & VMOP_CREATE_INSTANCE) == 0 ||
-   (*vm_parent = vm_getbyname(vmc->vmc_instance)) == NULL)
+   vmc->vmc_instance[0] == '\0')
return (0);
+
+   if ((*vm_parent = vm_getbyname(vmc->vmc_instance)) == NULL) {
+   errno = VMD_PARENT_INVALID;
+   return (-1);
+   }
 
errno = 0;
vmcp = &(*vm_parent)->vm_params;
Index: vmd/vmd.h
===
RCS file: /cvs/src/usr.sbin/vmd/vmd.h,v
retrieving revision 1.95
diff -u -p -r1.95 vmd.h
--- vmd/vmd.h   17 Jul 2019 05:51:07 -  1.95
+++ vmd/vmd.h   12 Aug 2019 16:18:22 -
@@ -72,6 +72,7 @@
 #define VMD_VM_STOP_INVALID1004
 #define VMD_CDROM_MISSING  1005
 #define VMD_CDROM_INVALID  1006
+#define VMD_PARENT_INVALID 1007
 
 /* Image file signatures */
 #define VM_MAGIC_QCOW  "QFI\xfb"



vmctl: invalid parent template error

2019-08-12 Thread Anton Lindqvist
Hi,
I recently fat fingered the vm template passed to vmctl and was greeted
with the following error:

  vmctl: start vm command failed: Operation not permitted

I think we can be more specific in order to improve usability:

  vmctl: could not find specified parent template

Comments? OK?

Index: vmctl/vmctl.c
===
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.c,v
retrieving revision 1.69
diff -u -p -r1.69 vmctl.c
--- vmctl/vmctl.c   22 May 2019 16:19:21 -  1.69
+++ vmctl/vmctl.c   5 Aug 2019 15:11:38 -
@@ -249,6 +249,10 @@ vm_start_complete(struct imsg *imsg, int
"file");
*ret = ENOENT;
break;
+   case VMD_PARENT_INVALID:
+   warnx("could not find specified parent 
template");
+   *ret = EINVAL;
+   break;
default:
errno = res;
warn("start vm command failed");
Index: vmd/vmd.c
===
RCS file: /cvs/src/usr.sbin/vmd/vmd.c,v
retrieving revision 1.114
diff -u -p -r1.114 vmd.c
--- vmd/vmd.c   28 Jun 2019 13:32:51 -  1.114
+++ vmd/vmd.c   5 Aug 2019 15:11:38 -
@@ -1373,8 +1373,13 @@ vm_instance(struct privsep *ps, struct v
 
/* return without error if the parent is NULL (nothing to inherit) */
if ((vmc->vmc_flags & VMOP_CREATE_INSTANCE) == 0 ||
-   (*vm_parent = vm_getbyname(vmc->vmc_instance)) == NULL)
+   vmc->vmc_instance[0] == '\0')
return (0);
+
+   if ((*vm_parent = vm_getbyname(vmc->vmc_instance)) == NULL) {
+   errno = VMD_PARENT_INVALID;
+   return (-1);
+   }
 
errno = 0;
vmcp = &(*vm_parent)->vm_params;
Index: vmd/vmd.h
===
RCS file: /cvs/src/usr.sbin/vmd/vmd.h,v
retrieving revision 1.95
diff -u -p -r1.95 vmd.h
--- vmd/vmd.h   17 Jul 2019 05:51:07 -  1.95
+++ vmd/vmd.h   5 Aug 2019 15:11:38 -
@@ -72,6 +72,7 @@
 #define VMD_VM_STOP_INVALID1004
 #define VMD_CDROM_MISSING  1005
 #define VMD_CDROM_INVALID  1006
+#define VMD_PARENT_INVALID 1007
 
 /* Image file signatures */
 #define VM_MAGIC_QCOW  "QFI\xfb"



Re: vm.conf: boot-device

2019-05-12 Thread Anton Lindqvist
On Wed, May 08, 2019 at 01:02:10PM -0700, Mike Larkin wrote:
> On Wed, May 08, 2019 at 09:41:42PM +0200, Anton Lindqvist wrote:
> > On Wed, May 08, 2019 at 07:19:45PM +0200, Reyk Floeter wrote:
> > > On Wed, May 08, 2019 at 06:47:53PM +0200, Anton Lindqvist wrote:
> > > > Hi,
> > > > A first stab at adding support for option `-B device' to vm.conf(5).
> > > > With the diff below, I'm able to add a dedicated VM to be used with
> > > > autoinstall(5):
> > > > 
> > > >   vm "amd64-install" {
> > > >   disable
> > > >   boot $snapshots "bsd.rd"
> > > >   disk $home "amd64.qcow2"
> > > >   boot-device net
> > > >   local interface
> > > >   }
> > > > 
> > > > The name `boot-device' is of course up for debate. Also, should allow 
> > > > support
> > > > also be considered?
> > > > 
> > > > Comments? OK?
> > > > 
> > > 
> > > Nice, thanks for adding this.  When I did vm.conf vs vmctl, I always
> > > liked to have vm.conf feature-complete or even to have it as a
> > > superset of vmctl - it is just nicer to add complex options with a
> > > grammar than command line flags (yay getsubopt(3)!).
> > > 
> > > Could you adjust the manpage based on vmctl(8)?  It is much more
> > > verbose and why should vm.conf(5) lack the details?
> > > 
> > > For the grammar, I think it should be done without the "-"...
> > > - boot "bsd.foo"
> > > - boot device net
> > > 
> > > ...as the parser should be able to handle this.  We should have made
> > > it more flexible but I guess we don't want to change the grammar now?
> > > - boot image "bsd.foo"
> > > - boot device net
> > 
> > Thanks for the feedback, new diff:
> > 
> > * Grammar changed to `boot device' according to feedback
> > 
> > * Manual tweaks, thanks jmc@ and sthen@
> > 
> > * I also took a stab at incorporating more of details from the vmctl
> >   manual. This part could probably be improved...
> > 
> 
> I like the diff, but there is a larger issue lurking ... :)
> 
> When I shuffled the man page bits last week, I just moved wording around, and
> actually didn't notice we call out PXE in the documentation. The problem here
> is that we don't actually support PXE in -current. I have a diff in my tree
> that fixes this but it's not committed yet. It breaks Linux guests, and every
> time I think I've nailed it, something else breaks. I had intended to commit
> it after AsisBSDcon last month but then I ran into that bug.
> 
> When claudio committed the "net boot" changes last year, he was pretty clear
> what it was intended to do:
> 
> 
> revision 1.7
> date: 2018/12/06 09:20:06;  author: claudio;  state: Exp;  lines: +7 -6;  
> commitid: 3sAzaMHdn0pTPnpR;
> Make it possible to define the bootdevice in vmd. This information is used
> currently only when booting a OpenBSD kernel. If VMBOOTDEV_NET is used the
> internal dhcp server will pass "auto_install" as boot file to the client and
> the boot loader passes the MAC of the first interface to the kernel to 
> indicate
> PXE booting. Adding boot order support to SeaBIOS is not yet implemented.
> Ok ccardenas@
> 
> So what this does is sets up the bootarg page to tell the kernel that it
> had been booted from PXE (even though it wasn't). That's used in conjunction
> with the vmd-dhcp server to provide auto_install options automatically to
> the VM. The fact that the term "PXE" was included in the commit may have
> misled some people to think this actually means PXE is supported now, which
> it is not.
> 
> So, while the diff is good, we should probably clean up the documentation
> to reflect what we really can and cannot do. Once I clean up iPXE and
> (really) get it working, we can go back and rev the documentation again.
> 
> Claudio, please correct me if I am wrong in the interpretation.

Good call. Here's an attempt to document the current implementation. If
we can agree upon something, vmctl(8) should also be updated
accordingly.

Index: parse.y
===
RCS file: /cvs/src/usr.sbin/vmd/parse.y,v
retrieving revision 1.51
diff -u -p -r1.51 parse.y
--- parse.y 11 May 2019 19:55:14 -  1.51
+++ parse.y 12 May 2019 18:46:56 -
@@ -120,12 +120,13 @@ typedef struct {
 
 
 %token INCLUDE ERROR
-%token ADD ALLOW BOOT CDROM DISABLE 

Re: vm.conf: boot-device

2019-05-08 Thread Anton Lindqvist
On Wed, May 08, 2019 at 07:19:45PM +0200, Reyk Floeter wrote:
> On Wed, May 08, 2019 at 06:47:53PM +0200, Anton Lindqvist wrote:
> > Hi,
> > A first stab at adding support for option `-B device' to vm.conf(5).
> > With the diff below, I'm able to add a dedicated VM to be used with
> > autoinstall(5):
> > 
> >   vm "amd64-install" {
> >   disable
> >   boot $snapshots "bsd.rd"
> >   disk $home "amd64.qcow2"
> >   boot-device net
> >   local interface
> >   }
> > 
> > The name `boot-device' is of course up for debate. Also, should allow 
> > support
> > also be considered?
> > 
> > Comments? OK?
> > 
> 
> Nice, thanks for adding this.  When I did vm.conf vs vmctl, I always
> liked to have vm.conf feature-complete or even to have it as a
> superset of vmctl - it is just nicer to add complex options with a
> grammar than command line flags (yay getsubopt(3)!).
> 
> Could you adjust the manpage based on vmctl(8)?  It is much more
> verbose and why should vm.conf(5) lack the details?
> 
> For the grammar, I think it should be done without the "-"...
> - boot "bsd.foo"
> - boot device net
> 
> ...as the parser should be able to handle this.  We should have made
> it more flexible but I guess we don't want to change the grammar now?
> - boot image "bsd.foo"
> - boot device net

Thanks for the feedback, new diff:

* Grammar changed to `boot device' according to feedback

* Manual tweaks, thanks jmc@ and sthen@

* I also took a stab at incorporating more of details from the vmctl
  manual. This part could probably be improved...

Index: parse.y
===
RCS file: /cvs/src/usr.sbin/vmd/parse.y,v
retrieving revision 1.50
diff -u -p -r1.50 parse.y
--- parse.y 13 Feb 2019 22:57:08 -  1.50
+++ parse.y 8 May 2019 19:36:04 -
@@ -120,12 +120,13 @@ typedef struct {
 
 
 %token INCLUDE ERROR
-%token ADD ALLOW BOOT CDROM DISABLE DISK DOWN ENABLE FORMAT GROUP INET6
-%token INSTANCE INTERFACE LLADDR LOCAL LOCKED MEMORY NIFS OWNER PATH PREFIX
-%token RDOMAIN SIZE SOCKET SWITCH UP VM VMID
+%token ADD ALLOW BOOT CDROM DEVICE DISABLE DISK DOWN ENABLE FORMAT GROUP
+%token INET6 INSTANCE INTERFACE LLADDR LOCAL LOCKED MEMORY NET NIFS OWNER
+%token PATH PREFIX RDOMAIN SIZE SOCKET SWITCH UP VM VMID
 %token   NUMBER
 %token   STRING
 %typelladdr
+%typebootdevice
 %typedisable
 %typeimage_format
 %typelocal
@@ -456,6 +457,9 @@ vm_opts : disable   {
}
vmc.vmc_flags |= VMOP_CREATE_KERNEL;
}
+   | BOOT DEVICE bootdevice{
+   vmc.vmc_bootdevice = $3;
+   }
| CDROM string  {
if (vcp->vcp_cdrom[0] != '\0') {
yyerror("cdrom specified more than once");
@@ -703,6 +707,11 @@ disable: ENABLE{ $$ = 
0; }
| DISABLE   { $$ = 1; }
;
 
+bootdevice : CDROM { $$ = VMBOOTDEV_CDROM; }
+   | DISK  { $$ = VMBOOTDEV_DISK; }
+   | NET   { $$ = VMBOOTDEV_NET; }
+   ;
+
 optcomma   : ','
|
;
@@ -756,6 +765,7 @@ lookup(char *s)
{ "allow",  ALLOW },
{ "boot",   BOOT },
{ "cdrom",  CDROM },
+   { "device", DEVICE },
{ "disable",DISABLE },
{ "disk",   DISK },
{ "down",   DOWN },
@@ -772,6 +782,7 @@ lookup(char *s)
{ "local",  LOCAL },
{ "locked", LOCKED },
{ "memory", MEMORY },
+   { "net",NET },
{ "owner",  OWNER },
{ "prefix", PREFIX },
{ "rdomain",RDOMAIN },
Index: vm.conf.5
===
RCS file: /cvs/src/usr.sbin/vmd/vm.conf.5,v
retrieving revision 1.42
diff -u -p -r1.42 vm.conf.5
--- vm.conf.5   7 Mar 2019 18:54:06 -   1.42
+++ vm.conf.5   8 May 2019 19:36:04 -
@@ -144,6 +144,34 @@ See
 Kernel or BIOS image to load when booting the VM.
 If not specified, the default is to boot using the BIOS image in
 .Pa /etc

vm.conf: boot-device

2019-05-08 Thread Anton Lindqvist
Hi,
A first stab at adding support for option `-B device' to vm.conf(5).
With the diff below, I'm able to add a dedicated VM to be used with
autoinstall(5):

  vm "amd64-install" {
  disable
  boot $snapshots "bsd.rd"
  disk $home "amd64.qcow2"
  boot-device net
  local interface
  }

The name `boot-device' is of course up for debate. Also, should allow support
also be considered?

Comments? OK?

Index: parse.y
===
RCS file: /cvs/src/usr.sbin/vmd/parse.y,v
retrieving revision 1.50
diff -u -p -r1.50 parse.y
--- parse.y 13 Feb 2019 22:57:08 -  1.50
+++ parse.y 28 Apr 2019 13:54:50 -
@@ -120,12 +120,13 @@ typedef struct {
 
 
 %token INCLUDE ERROR
-%token ADD ALLOW BOOT CDROM DISABLE DISK DOWN ENABLE FORMAT GROUP INET6
-%token INSTANCE INTERFACE LLADDR LOCAL LOCKED MEMORY NIFS OWNER PATH PREFIX
-%token RDOMAIN SIZE SOCKET SWITCH UP VM VMID
+%token ADD ALLOW BOOT BOOTDEVICE CDROM DISABLE DISK DOWN ENABLE FORMAT GROUP
+%token INET6 INSTANCE INTERFACE LLADDR LOCAL LOCKED MEMORY NET NIFS OWNER
+%token PATH PREFIX RDOMAIN SIZE SOCKET SWITCH UP VM VMID
 %token   NUMBER
 %token   STRING
 %typelladdr
+%typebootdevice
 %typedisable
 %typeimage_format
 %typelocal
@@ -456,6 +457,9 @@ vm_opts : disable   {
}
vmc.vmc_flags |= VMOP_CREATE_KERNEL;
}
+   | BOOTDEVICE bootdevice {
+   vmc.vmc_bootdevice = $2;
+   }
| CDROM string  {
if (vcp->vcp_cdrom[0] != '\0') {
yyerror("cdrom specified more than once");
@@ -703,6 +707,11 @@ disable: ENABLE{ $$ = 
0; }
| DISABLE   { $$ = 1; }
;
 
+bootdevice : CDROM { $$ = VMBOOTDEV_CDROM; }
+   | DISK  { $$ = VMBOOTDEV_DISK; }
+   | NET   { $$ = VMBOOTDEV_NET; }
+   ;
+
 optcomma   : ','
|
;
@@ -755,6 +764,7 @@ lookup(char *s)
{ "add",ADD },
{ "allow",  ALLOW },
{ "boot",   BOOT },
+   { "boot-device",BOOTDEVICE },
{ "cdrom",  CDROM },
{ "disable",DISABLE },
{ "disk",   DISK },
@@ -772,6 +782,7 @@ lookup(char *s)
{ "local",  LOCAL },
{ "locked", LOCKED },
{ "memory", MEMORY },
+   { "net",NET },
{ "owner",  OWNER },
{ "prefix", PREFIX },
{ "rdomain",RDOMAIN },
Index: vm.conf.5
===
RCS file: /cvs/src/usr.sbin/vmd/vm.conf.5,v
retrieving revision 1.42
diff -u -p -r1.42 vm.conf.5
--- vm.conf.5   7 Mar 2019 18:54:06 -   1.42
+++ vm.conf.5   28 Apr 2019 13:54:50 -
@@ -144,6 +144,13 @@ See
 Kernel or BIOS image to load when booting the VM.
 If not specified, the default is to boot using the BIOS image in
 .Pa /etc/firmware/vmm-bios .
+.It Cm boot-device Ar device
+Force VM to boot of
+.Ar device
+which may be either
+.Ar cdrom , disk
+or
+.Ar net .
 .It Cm cdrom Ar path
 ISO image file.
 .It Cm enable



chflagsat(2): fix function argument

2019-03-25 Thread Anton Lindqvist
Hi,
My guess is that flag actually refers to atflags.

Comments? OK?

Index: chflags.2
===
RCS file: /cvs/src/lib/libc/sys/chflags.2,v
retrieving revision 1.27
diff -u -p -r1.27 chflags.2
--- chflags.2   19 Jan 2015 15:54:11 -  1.27
+++ chflags.2   25 Mar 2019 20:38:23 -
@@ -113,12 +113,12 @@ in the
 .Fa fd
 parameter, the current working directory is used.
 If
-.Fa flag
+.Fa atflags
 is also zero, the behavior is identical to a call to
 .Fn chflags .
 .Pp
 The
-.Fa flag
+.Fa atflags
 argument is the bitwise OR of zero or more of the following values:
 .Pp
 .Bl -tag -width AT_SYMLINK_NOFOLLOW -offset indent -compact



lockf: initialization order

2019-02-21 Thread Anton Lindqvist
Hi,
Initialize the fields of `struct lockf' in the same order as the struct
definition, with the ambition of making easier to inspect that all
fields are properly initialized. Similar work has already been done in
lf_split().

Comments? OK?

Index: kern/vfs_lockf.c
===
RCS file: /cvs/src/sys/kern/vfs_lockf.c,v
retrieving revision 1.33
diff -u -p -r1.33 vfs_lockf.c
--- kern/vfs_lockf.c30 Jan 2019 17:04:04 -  1.33
+++ kern/vfs_lockf.c22 Feb 2019 06:44:46 -
@@ -243,15 +243,15 @@ lf_advlock(struct lockf_state **state, o
ls_rele(ls);
return (ENOLCK);
}
+   lock->lf_flags = flags;
+   lock->lf_type = fl->l_type;
lock->lf_start = start;
lock->lf_end = end;
lock->lf_id = id;
lock->lf_state = ls;
-   lock->lf_type = fl->l_type;
lock->lf_blk = NULL;
-   TAILQ_INIT(>lf_blkhd);
-   lock->lf_flags = flags;
lock->lf_pid = (flags & F_POSIX) ? p->p_p->ps_pid : -1;
+   TAILQ_INIT(>lf_blkhd);
 
switch (op) {
case F_SETLK:



wscons: struct wssrcops initializers

2019-02-18 Thread Anton Lindqvist
Hi,
Favor C99 initializers for struct wssrcops; improves grepability.

Comments? OK?

Index: dev/wscons/wskbd.c
===
RCS file: /cvs/src/sys/dev/wscons/wskbd.c,v
retrieving revision 1.95
diff -u -p -r1.95 wskbd.c
--- dev/wscons/wskbd.c  1 Feb 2019 07:02:31 -   1.95
+++ dev/wscons/wskbd.c  18 Feb 2019 17:48:38 -
@@ -279,13 +279,15 @@ struct wskbd_keyrepeat_data wskbd_defaul
 
 #if NWSMUX > 0 || NWSDISPLAY > 0
 struct wssrcops wskbd_srcops = {
-   WSMUX_KBD,
-   wskbd_mux_open, wskbd_mux_close, wskbd_do_ioctl,
-   wskbd_displayioctl,
+   .type   = WSMUX_KBD,
+   .dopen  = wskbd_mux_open,
+   .dclose = wskbd_mux_close,
+   .dioctl = wskbd_do_ioctl,
+   .ddispioctl = wskbd_displayioctl,
 #if NWSDISPLAY > 0
-   wskbd_set_display
+   .dsetdisplay= wskbd_set_display,
 #else
-   NULL
+   .dsetdisplay= NULL,
 #endif
 };
 #endif
Index: dev/wscons/wsmouse.c
===
RCS file: /cvs/src/sys/dev/wscons/wsmouse.c,v
retrieving revision 1.50
diff -u -p -r1.50 wsmouse.c
--- dev/wscons/wsmouse.c20 Nov 2018 19:33:44 -  1.50
+++ dev/wscons/wsmouse.c18 Feb 2019 17:48:38 -
@@ -168,8 +168,12 @@ struct cfattach wsmouse_ca = {
 
 #if NWSMUX > 0
 struct wssrcops wsmouse_srcops = {
-   WSMUX_MOUSE,
-   wsmouse_mux_open, wsmouse_mux_close, wsmousedoioctl, NULL, NULL
+   .type   = WSMUX_MOUSE,
+   .dopen  = wsmouse_mux_open,
+   .dclose = wsmouse_mux_close,
+   .dioctl = wsmousedoioctl,
+   .ddispioctl = NULL,
+   .dsetdisplay= NULL,
 };
 #endif
 
Index: dev/wscons/wsmux.c
===
RCS file: /cvs/src/sys/dev/wscons/wsmux.c,v
retrieving revision 1.39
diff -u -p -r1.39 wsmux.c
--- dev/wscons/wsmux.c  18 Feb 2019 17:39:14 -  1.39
+++ dev/wscons/wsmux.c  18 Feb 2019 17:48:38 -
@@ -108,9 +108,12 @@ void   wsmuxattach(int);
 void   wsmux_detach_sc_locked(struct wsmux_softc *, struct wsevsrc *);
 
 struct wssrcops wsmux_srcops = {
-   WSMUX_MUX,
-   wsmux_mux_open, wsmux_mux_close, wsmux_do_ioctl, wsmux_do_displayioctl,
-   wsmux_evsrc_set_display
+   .type   = WSMUX_MUX,
+   .dopen  = wsmux_mux_open,
+   .dclose = wsmux_mux_close,
+   .dioctl = wsmux_do_ioctl,
+   .ddispioctl = wsmux_do_displayioctl,
+   .dsetdisplay= wsmux_evsrc_set_display,
 };
 
 /* From upper level */



wscons: free size

2019-01-31 Thread Anton Lindqvist
Comments? OK?

Index: dev/wscons/wsevent.c
===
RCS file: /cvs/src/sys/dev/wscons/wsevent.c,v
retrieving revision 1.18
diff -u -p -r1.18 wsevent.c
--- dev/wscons/wsevent.c19 Nov 2018 19:19:24 -  1.18
+++ dev/wscons/wsevent.c1 Feb 2019 07:16:03 -
@@ -127,7 +127,7 @@ wsevent_fini(struct wseventvar *ev)
 #endif
return;
}
-   free(ev->q, M_DEVBUF, 0);
+   free(ev->q, M_DEVBUF, WSEVENT_QSIZE * sizeof(struct wscons_event));
ev->q = NULL;
 
sigio_free(>sigio);



cd9660: unused field in iso_node

2019-01-19 Thread Anton Lindqvist
Hi,
The i_lockf field in `struct iso_node' looks unused since the initial
import. Also, vop_advlock is mapped to eopnotsupp() in cd9660_vops.

Comments? OK?

Index: isofs/cd9660/cd9660_node.h
===
RCS file: /cvs/src/sys/isofs/cd9660/cd9660_node.h,v
retrieving revision 1.20
diff -u -p -r1.20 cd9660_node.h
--- isofs/cd9660/cd9660_node.h  19 Jun 2016 11:54:33 -  1.20
+++ isofs/cd9660/cd9660_node.h  18 Jan 2019 14:09:58 -
@@ -61,7 +61,6 @@ struct iso_node {
cdino_t i_number;   /* the identity of the inode */
/* we use the actual starting block of the file 
*/
struct  iso_mnt *i_mnt; /* filesystem associated with this inode */
-   struct  lockf *i_lockf; /* head of byte-level lock list */
doff_t  i_endoff;   /* end of useful stuff in directory */
doff_t  i_diroff;   /* offset in dir, where we found last entry */
doff_t  i_offset;   /* offset of free space in directory */



Re: kcov: trace cmp

2019-01-18 Thread Anton Lindqvist
On Fri, Jan 18, 2019 at 04:58:16PM +0100, Alexander Bluhm wrote:
> On Fri, Jan 18, 2019 at 04:38:55PM +0100, Anton Lindqvist wrote:
> > > - You allocate nmemb * sizeof(uintptr_t) bytes.
> > > - kd_buf[nmemb - 1] is the largest valid index.
> > > - kd_nmemb is nmemb - 1.
> > > - kd_buf[idx * 4 + 4] is the access at the largest position. 
> > > - So return if (idx * 4 + 4 > kd_nmemb).
> > 
> > You're right, I forgot about the `kd_nmemb = nmemb - 1' initialization.
> > I went ahead and matched the existing style in
> > __sanitizer_cov_trace_pc() but inverting the conditional could be done
> > later.
> 
> As long as the syle is the same, it is fine.  Then I have to think
> about it only once.
> 
> > +   idx = kd->kd_buf[0];
> > +   if (idx * 4 + 4 < kd->kd_nmemb) {
> > +   kd->kd_buf[idx * 4 + 1] = type;
> > +   kd->kd_buf[idx * 4 + 2] = arg1;
> > +   kd->kd_buf[idx * 4 + 3] = arg2;
> > +   kd->kd_buf[idx * 4 + 4] = pc;
> > +   kd->kd_buf[0] = idx + 1;
> > +   }
> > +}
> 
> You changed an off by two to an off by one.  Compare trace_cmp()
> with __sanitizer_cov_trace_pc().
> 
> idx = kd->kd_buf[0];
> if (idx < kd->kd_nmemb) {
> kd->kd_buf[idx + 1] = (uintptr_t)__builtin_return_address(0);
> kd->kd_buf[0] = idx + 1;
> }
> 
> One of them must be wrong as the largest index accessed and the
> index in the check differ.  I would prefer "if (idx * 4 + 4 <=
> kd->kd_nmemb)" and "if (idx + 1 <= kd->kd_nmemb)".  The latter is
> equivalent to the existing check.

You're right, updated diff.

Index: share/man/man4/kcov.4
===
RCS file: /cvs/src/share/man/man4/kcov.4,v
retrieving revision 1.6
diff -u -p -r1.6 kcov.4
--- share/man/man4/kcov.4   27 Dec 2018 19:33:08 -  1.6
+++ share/man/man4/kcov.4   18 Jan 2019 18:26:17 -
@@ -62,9 +62,35 @@ Enable code coverage tracing for the cur
 The
 .Fa mode
 must be one of the following:
-.Bl -tag -width KCOV_MODE_TRACE_PC
+.Bl -ohang
 .It Dv KCOV_MODE_TRACE_PC
 Trace the kernel program counter.
+.It Dv KCOV_MODE_TRACE_CMP
+Trace comparison instructions and switch statements.
+For switch statements, the number of traced comparison instructions is equal to
+the number of switch cases.
+Each traced comparison instruction is represented by 4 entries in the coverage
+buffer:
+.Bl -enum
+.It
+A mask where the least significant bit is set if one of the comparison operands
+is a compile-time constant, which is always true for switch statements.
+The remaining bits represents the log2 size of the operands, ranging from 0 to
+3.
+.It
+First comparison operand.
+For switch statements, this operand corresponds to the case value.
+.It
+Second comparison operand.
+For switch statements, this operand corresponds to the value passed to switch.
+.It
+Kernel program counter where the comparison instruction took place.
+.El
+.Pp
+In this mode, the first entry in the coverage buffer reflects the number of
+traced comparison instructions.
+Thus, the effective number of entries in the coverage buffer is given by
+multiplying the first entry by 4.
 .El
 .It Dv KIODISABLE Fa void
 Disable code coverage tracing for the current thread.
Index: sys/arch/amd64/conf/Makefile.amd64
===
RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
retrieving revision 1.108
diff -u -p -r1.108 Makefile.amd64
--- sys/arch/amd64/conf/Makefile.amd64  7 Jan 2019 20:24:59 -   1.108
+++ sys/arch/amd64/conf/Makefile.amd64  18 Jan 2019 18:26:17 -
@@ -95,7 +95,7 @@ LINKFLAGS+=   -S
 .endif
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
-PROF=  -fsanitize-coverage=trace-pc
+PROF=  -fsanitize-coverage=trace-pc,trace-cmp
 .endif
 
 %LOAD
@@ -152,7 +152,7 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
 kcov.o: $S/dev/kcov.c
-   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc,trace-cmp
 .endif
 
 clean:
Index: sys/arch/i386/conf/Makefile.i386
===
RCS file: /cvs/src/sys/arch/i386/conf/Makefile.i386,v
retrieving revision 1.130
diff -u -p -r1.130 Makefile.i386
--- sys/arch/i386/conf/Makefile.i38630 Oct 2018 11:08:30 -  1.130
+++ sys/arch/i386/conf/Makefile.i38618 Jan 2019 18:26:17 -
@@ -97,7 +97,7 @@ LINKFLAGS+=   -S
 .endif
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
-PROF=  -fsanitize-coverage=trace-pc
+PROF=  -fsanitize-coverage=trace-pc,trace-cmp
 .endif
 
 %LOAD

Re: kcov: trace cmp

2019-01-18 Thread Anton Lindqvist
On Wed, Jan 16, 2019 at 10:36:29PM +0100, Alexander Bluhm wrote:
> On Wed, Jan 16, 2019 at 09:17:33PM +0100, Anton Lindqvist wrote:
> > +   idx = kd->kd_buf[0];
> > +   if (idx * 4 + 4 + 1 >= kd->kd_nmemb)
> > +   return;
> > +
> > +   kd->kd_buf[idx * 4 + 1] = type;
> > +   kd->kd_buf[idx * 4 + 2] = arg1;
> > +   kd->kd_buf[idx * 4 + 3] = arg2;
> > +   kd->kd_buf[idx * 4 + 4] = pc;
> > +   kd->kd_buf[0] = idx + 1;
> 
> I think this boundary calculation is not correct.  My reasoning is:
> 
> - You allocate nmemb * sizeof(uintptr_t) bytes.
> - kd_buf[nmemb - 1] is the largest valid index.
> - kd_nmemb is nmemb - 1.
> - kd_buf[idx * 4 + 4] is the access at the largest position. 
> - So return if (idx * 4 + 4 > kd_nmemb).

You're right, I forgot about the `kd_nmemb = nmemb - 1' initialization.
I went ahead and matched the existing style in
__sanitizer_cov_trace_pc() but inverting the conditional could be done
later.

Nits from jmc@ are also fixed, see diff below.

> In __sanitizer_cov_trace_pc() you have the logic the other way around.
>   idx = kd->kd_buf[0];
>   if (idx < kd->kd_nmemb) {
>   kd->kd_buf[idx + 1] = (uintptr_t)__builtin_return_address(0);
>   kd->kd_buf[0] = idx + 1;
>   }
> And it is not equivalent to the calculation in trace_cmp().
> 
> Could you use the same style in both functions?  I would prefer
> this if you think my suggestion is correct.
> 
> trace_cmp():
>   if (idx * 4 + 4 > kd->kd_nmemb)
>   return;
> 
> __sanitizer_cov_trace_pc():
>   if (idx + 1 > kd->kd_nmemb)
>   return;
> 
> bluhm

Index: share/man/man4/kcov.4
===
RCS file: /cvs/src/share/man/man4/kcov.4,v
retrieving revision 1.6
diff -u -p -r1.6 kcov.4
--- share/man/man4/kcov.4   27 Dec 2018 19:33:08 -  1.6
+++ share/man/man4/kcov.4   18 Jan 2019 15:27:21 -
@@ -62,9 +62,35 @@ Enable code coverage tracing for the cur
 The
 .Fa mode
 must be one of the following:
-.Bl -tag -width KCOV_MODE_TRACE_PC
+.Bl -ohang
 .It Dv KCOV_MODE_TRACE_PC
 Trace the kernel program counter.
+.It Dv KCOV_MODE_TRACE_CMP
+Trace comparison instructions and switch statements.
+For switch statements, the number of traced comparison instructions is equal to
+the number of switch cases.
+Each traced comparison instruction is represented by 4 entries in the coverage
+buffer:
+.Bl -enum
+.It
+A mask where the least significant bit is set if one of the comparison operands
+is a compile-time constant, which is always true for switch statements.
+The remaining bits represents the log2 size of the operands, ranging from 0 to
+3.
+.It
+First comparison operand.
+For switch statements, this operand corresponds to the case value.
+.It
+Second comparison operand.
+For switch statements, this operand corresponds to the value passed to switch.
+.It
+Kernel program counter where the comparison instruction took place.
+.El
+.Pp
+In this mode, the first entry in the coverage buffer reflects the number of
+traced comparison instructions.
+Thus, the effective number of entries in the coverage buffer is given by
+multiplying the first entry by 4.
 .El
 .It Dv KIODISABLE Fa void
 Disable code coverage tracing for the current thread.
Index: sys/arch/amd64/conf/Makefile.amd64
===
RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
retrieving revision 1.108
diff -u -p -r1.108 Makefile.amd64
--- sys/arch/amd64/conf/Makefile.amd64  7 Jan 2019 20:24:59 -   1.108
+++ sys/arch/amd64/conf/Makefile.amd64  18 Jan 2019 15:27:21 -
@@ -95,7 +95,7 @@ LINKFLAGS+=   -S
 .endif
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
-PROF=  -fsanitize-coverage=trace-pc
+PROF=  -fsanitize-coverage=trace-pc,trace-cmp
 .endif
 
 %LOAD
@@ -152,7 +152,7 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
 kcov.o: $S/dev/kcov.c
-   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc,trace-cmp
 .endif
 
 clean:
Index: sys/arch/i386/conf/Makefile.i386
===
RCS file: /cvs/src/sys/arch/i386/conf/Makefile.i386,v
retrieving revision 1.130
diff -u -p -r1.130 Makefile.i386
--- sys/arch/i386/conf/Makefile.i38630 Oct 2018 11:08:30 -  1.130
+++ sys/arch/i386/conf/Makefile.i38618 Jan 2019 15:27:21 -
@@ -97,7 +97,7 @@ LINKFLAGS+=   -S
 .endif
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
-PROF=  -fsanitize-coverage=trace-pc
+PROF=  -fsanitize-coverage=trace-pc,trace-cmp
 .endif
 
 %LOAD
@@ -148,7 +148,7 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
 
 .if ${

kcov: trace cmp

2019-01-16 Thread Anton Lindqvist
Hi,
This diff add supports for an additional trace mode to kcov where
comparison instructions and switch statements can be traced. The actual
comparison operands ends up in the coverage buffer, with some additional
data. See the manual bits for further info. This mode will generate even
more useful coverage during fuzzing.

The implementation matches both Linux and FreeBSD.

Comments? OK?

Index: share/man/man4/kcov.4
===
RCS file: /cvs/src/share/man/man4/kcov.4,v
retrieving revision 1.6
diff -u -p -r1.6 kcov.4
--- share/man/man4/kcov.4   27 Dec 2018 19:33:08 -  1.6
+++ share/man/man4/kcov.4   16 Jan 2019 20:14:31 -
@@ -62,9 +62,35 @@ Enable code coverage tracing for the cur
 The
 .Fa mode
 must be one of the following:
-.Bl -tag -width KCOV_MODE_TRACE_PC
+.Bl -ohang
 .It Dv KCOV_MODE_TRACE_PC
 Trace the kernel program counter.
+.It Dv KCOV_MODE_TRACE_CMP
+Trace comparison instructions and switch statements.
+For switch statements, the number of traced comparison instructions is equal to
+number of switch cases.
+Each traced comparison instruction is represented by 4 entries in the coverage
+buffer:
+.Bl -enum
+.It
+A mask where the least significant bit is set if one of the comparison operands
+is a compile-time constant, which is always true for switch statements.
+The remaining bits represents the log2 size of the operands, ranging from 0 to
+3.
+.It
+First comparison operand.
+For switch statements, this operand corresponds to the case value.
+.It
+Second comparison operand.
+For switch statements, this operand corresponds to the value passed to switch.
+.It
+Kernel program counter where the comparison instruction took place.
+.El
+.Pp
+In this mode, the first entry in coverage buffer reflects the number of traced
+comparison instructions.
+Thus, the effective number of entries in the coverage buffer is given by
+multiplying the first entry by 4.
 .El
 .It Dv KIODISABLE Fa void
 Disable code coverage tracing for the current thread.
Index: sys/arch/amd64/conf/Makefile.amd64
===
RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
retrieving revision 1.108
diff -u -p -r1.108 Makefile.amd64
--- sys/arch/amd64/conf/Makefile.amd64  7 Jan 2019 20:24:59 -   1.108
+++ sys/arch/amd64/conf/Makefile.amd64  16 Jan 2019 20:14:31 -
@@ -95,7 +95,7 @@ LINKFLAGS+=   -S
 .endif
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
-PROF=  -fsanitize-coverage=trace-pc
+PROF=  -fsanitize-coverage=trace-pc,trace-cmp
 .endif
 
 %LOAD
@@ -152,7 +152,7 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
 kcov.o: $S/dev/kcov.c
-   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc,trace-cmp
 .endif
 
 clean:
Index: sys/arch/i386/conf/Makefile.i386
===
RCS file: /cvs/src/sys/arch/i386/conf/Makefile.i386,v
retrieving revision 1.130
diff -u -p -r1.130 Makefile.i386
--- sys/arch/i386/conf/Makefile.i38630 Oct 2018 11:08:30 -  1.130
+++ sys/arch/i386/conf/Makefile.i38616 Jan 2019 20:14:31 -
@@ -97,7 +97,7 @@ LINKFLAGS+=   -S
 .endif
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
-PROF=  -fsanitize-coverage=trace-pc
+PROF=  -fsanitize-coverage=trace-pc,trace-cmp
 .endif
 
 %LOAD
@@ -148,7 +148,7 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
 kcov.o: $S/dev/kcov.c
-   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc,trace-cmp
 .endif
 
 clean:
Index: sys/dev/kcov.c
===
RCS file: /cvs/src/sys/dev/kcov.c,v
retrieving revision 1.10
diff -u -p -r1.10 kcov.c
--- sys/dev/kcov.c  16 Jan 2019 19:27:07 -  1.10
+++ sys/dev/kcov.c  16 Jan 2019 20:14:31 -
@@ -26,6 +26,9 @@
 
 #include 
 
+#define KCOV_CMP_CONST 0x1
+#define KCOV_CMP_SIZE(x)   ((x) << 1)
+
 /* #define KCOV_DEBUG */
 #ifdef KCOV_DEBUG
 #define DPRINTF(x...) do { if (kcov_debug) printf(x); } while (0)
@@ -105,6 +108,139 @@ __sanitizer_cov_trace_pc(void)
}
 }
 
+/*
+ * Compiling the kernel with the `-fsanitize-coverage=trace-cmp' option will
+ * cause the following function to be called upon integer comparisons and 
switch
+ * statements.
+ *
+ * If kcov is enabled for the current thread, the comparison will be stored in
+ * its corresponding coverage buffer.
+ */
+void
+trace_cmp(uint64_t type, uint64_t arg1, uint64_t arg2, uintptr_t pc)
+{
+   struct kcov_dev *kd;
+   uint64_t idx;
+
+   /*
+* Do not trace before kcovopen() has been called at least once.
+* At this point, all secondary CPUs have booted and accessing curcpu()
+* is safe.
+*/
+   if (kcov_cold)
+ 

kcov: deny mmap() while enabled

2019-01-16 Thread Anton Lindqvist
Hi,
When a kcov fd is in an enabled state, disallow mmap() calls using the
same fd. Sometimes, syzkaller manages to create a device node using
mknod() with major=22 and minor=X where X maps to an already open kcov
fd in the current process; it then mmap() the kcov coverage buffer which
is used as input to other syscalls causing the coverage buffer to be
corrupted:

  kd = open("/dev/kcov")
  # kd = 3
  ioctl(kd, KIOSETBUFSIZE, ...)
  ioctl(kd, KIOENABLE, ...)
  mknod("./nod", major=22, minor=3)
  fd = open("./nod")
  # fd = dup(kd)
  p = mmap(fd, ...)
  lstat("./nod", p)
  # lstat() will corrupt the coverage buffer

A similiar check is also presents in Linux.

Comments? OK?

Index: dev/kcov.c
===
RCS file: /cvs/src/sys/dev/kcov.c,v
retrieving revision 1.9
diff -u -p -r1.9 kcov.c
--- dev/kcov.c  3 Jan 2019 08:56:53 -   1.9
+++ dev/kcov.c  14 Jan 2019 16:52:12 -
@@ -209,7 +209,7 @@ kcovmmap(dev_t dev, off_t offset, int pr
vaddr_t va;
 
kd = kd_lookup(minor(dev));
-   if (kd == NULL)
+   if (kd == NULL || kd->kd_state != KCOV_STATE_READY)
return (paddr_t)(-1);
 
if (offset < 0 || offset >= kd->kd_nmemb * sizeof(uintptr_t))



kcov: enable retguard

2019-01-07 Thread Anton Lindqvist
Hi,
The problems caused by enabling both kcov and retguard was due to the
increased kernel size. Now that NKL2_KIMG_ENTRIES has been bumped on
amd64, it's no longer a problem.

Comments? OK?

Index: arch/amd64/conf/Makefile.amd64
===
RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
retrieving revision 1.107
diff -u -p -r1.107 Makefile.amd64
--- arch/amd64/conf/Makefile.amd64  30 Dec 2018 23:08:05 -  1.107
+++ arch/amd64/conf/Makefile.amd64  7 Jan 2019 19:25:07 -
@@ -95,7 +95,6 @@ LINKFLAGS+=   -S
 .endif
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
-CMACHFLAGS+=   -fno-ret-protector
 PROF=  -fsanitize-coverage=trace-pc
 .endif



kcov: prepare for trace cmp [2/N]

2018-12-27 Thread Anton Lindqvist
Hi,
As a next step, let KIOENABLE accept the desired tracing mode. The API
matches the kcov implementation found both in Linux and FreeBSD. As far
as I know, syzkaller is the only kcov consumer in the wild and I will
make sure to adapt the new API there.

Comments? OK?

Index: share/man/man4/kcov.4
===
RCS file: /cvs/src/share/man/man4/kcov.4,v
retrieving revision 1.5
diff -u -p -r1.5 kcov.4
--- share/man/man4/kcov.4   30 Sep 2018 09:14:43 -  1.5
+++ share/man/man4/kcov.4   27 Dec 2018 10:35:13 -
@@ -57,8 +57,16 @@ whereas the returned pointer must be int
 entries.
 The first entry contains the number of entries in the array,
 excluding the first entry.
-.It Dv KIOENABLE Fa void
+.It Dv KIOENABLE Fa int *mode
 Enable code coverage tracing for the current thread.
+The
+.Fa mode
+must be one of the following:
+.Pp
+.Bl -tag -width KCOV_MODE_TRACE_PC -compact
+.It KCOV_MODE_TRACE_PC
+Trace the kernel program counter.
+.El
 .It Dv KIODISABLE Fa void
 Disable code coverage tracing for the current thread.
 .El
@@ -91,7 +99,7 @@ main(void)
 {
unsigned long *cover, i;
unsigned long size = 1024;
-   int fd;
+   int fd, mode;
 
fd = open("/dev/kcov", O_RDWR);
if (fd == -1)
@@ -104,7 +112,8 @@ main(void)
if (cover == MAP_FAILED)
err(1, "mmap");
 
-   if (ioctl(fd, KIOENABLE) == -1)
+   mode = KCOV_MODE_TRACE_PC;
+   if (ioctl(fd, KIOENABLE, ) == -1)
err(1, "ioctl: KIOENABLE");
read(-1, NULL, 0);
if (ioctl(fd, KIODISABLE) == -1)
Index: sys/dev/kcov.c
===
RCS file: /cvs/src/sys/dev/kcov.c,v
retrieving revision 1.7
diff -u -p -r1.7 kcov.c
--- sys/dev/kcov.c  27 Dec 2018 10:04:16 -  1.7
+++ sys/dev/kcov.c  27 Dec 2018 10:35:13 -
@@ -155,6 +155,7 @@ int
 kcovioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
 {
struct kcov_dev *kd;
+   int mode;
int error = 0;
 
kd = kd_lookup(minor(dev));
@@ -171,8 +172,13 @@ kcovioctl(dev_t dev, u_long cmd, caddr_t
error = EBUSY;
break;
}
+   mode = *((int *)data);
+   if (mode != KCOV_MODE_TRACE_PC) {
+   error = EINVAL;
+   break;
+   }
kd->kd_state = KCOV_STATE_TRACE;
-   kd->kd_mode = KCOV_MODE_TRACE_PC;
+   kd->kd_mode = mode;
p->p_kd = kd;
break;
case KIODISABLE:
Index: sys/sys/kcov.h
===
RCS file: /cvs/src/sys/sys/kcov.h,v
retrieving revision 1.2
diff -u -p -r1.2 kcov.h
--- sys/sys/kcov.h  27 Dec 2018 10:04:16 -  1.2
+++ sys/sys/kcov.h  27 Dec 2018 10:35:13 -
@@ -22,7 +22,7 @@
 #include 
 
 #define KIOSETBUFSIZE  _IOW('K', 1, unsigned long)
-#define KIOENABLE  _IO('K', 2)
+#define KIOENABLE  _IOW('K', 2, int)
 #define KIODISABLE _IO('K', 3)
 
 #define KCOV_MODE_NONE 0



kcov: prepare for trace cmp

2018-12-26 Thread Anton Lindqvist
Hi,
The end goal here is teach kcov to support tracing of comparison
instructions and switch statements in order to improve coverage during
fuzzing. It's achieved by compiling the kernel with the `
-fsanitize-coverage=trace-cmp' option.

But first, some refactoring: the diff below separates the kcov
descriptor state and trace mode as a first step. While here, try to
improve the name of states.

Comments? OK?

Index: dev/kcov.c
===
RCS file: /cvs/src/sys/dev/kcov.c,v
retrieving revision 1.6
diff -u -p -r1.6 kcov.c
--- dev/kcov.c  25 Dec 2018 21:56:53 -  1.6
+++ dev/kcov.c  26 Dec 2018 10:50:01 -
@@ -35,11 +35,12 @@
 
 struct kcov_dev {
enum {
-   KCOV_MODE_DISABLED,
-   KCOV_MODE_INIT,
-   KCOV_MODE_TRACE_PC,
-   KCOV_MODE_DYING,
-   }kd_mode;
+   KCOV_STATE_NONE,
+   KCOV_STATE_READY,
+   KCOV_STATE_TRACE,
+   KCOV_STATE_DYING,
+   }kd_state;
+   int  kd_mode;
int  kd_unit;   /* device minor */
uintptr_t   *kd_buf;/* traced coverage */
size_t   kd_nmemb;
@@ -137,12 +138,15 @@ kcovclose(dev_t dev, int flag, int mode,
if (kd == NULL)
return (EINVAL);
 
-   DPRINTF("%s: unit=%d\n", __func__, minor(dev));
+   DPRINTF("%s: unit=%d, state=%d, mode=%d\n",
+   __func__, kd->kd_unit, kd->kd_state, kd->kd_mode);
 
-   if (kd->kd_mode == KCOV_MODE_TRACE_PC)
-   kd->kd_mode = KCOV_MODE_DYING;
-   else
+   if (kd->kd_state == KCOV_STATE_TRACE) {
+   kd->kd_state = KCOV_STATE_DYING;
+   kd->kd_mode = KCOV_MODE_NONE;
+   } else {
kd_free(kd);
+   }
 
return (0);
 }
@@ -163,20 +167,22 @@ kcovioctl(dev_t dev, u_long cmd, caddr_t
break;
case KIOENABLE:
/* Only one kcov descriptor can be enabled per thread. */
-   if (p->p_kd != NULL || kd->kd_mode != KCOV_MODE_INIT) {
+   if (p->p_kd != NULL || kd->kd_state != KCOV_STATE_READY) {
error = EBUSY;
break;
}
+   kd->kd_state = KCOV_STATE_TRACE;
kd->kd_mode = KCOV_MODE_TRACE_PC;
p->p_kd = kd;
break;
case KIODISABLE:
/* Only the enabled thread may disable itself. */
-   if (p->p_kd != kd || kd->kd_mode != KCOV_MODE_TRACE_PC) {
+   if (p->p_kd != kd || kd->kd_state != KCOV_STATE_TRACE) {
error = EBUSY;
break;
}
-   kd->kd_mode = KCOV_MODE_INIT;
+   kd->kd_state = KCOV_STATE_READY;
+   kd->kd_mode = KCOV_MODE_NONE;
p->p_kd = NULL;
break;
default:
@@ -184,8 +190,8 @@ kcovioctl(dev_t dev, u_long cmd, caddr_t
DPRINTF("%s: %lu: unknown command\n", __func__, cmd);
}
 
-   DPRINTF("%s: unit=%d, mode=%d, error=%d\n",
-   __func__, kd->kd_unit, kd->kd_mode, error);
+   DPRINTF("%s: unit=%d, state=%d, mode=%d, error=%d\n",
+   __func__, kd->kd_unit, kd->kd_state, kd->kd_mode, error);
 
return (error);
 }
@@ -219,12 +225,15 @@ kcov_exit(struct proc *p)
if (kd == NULL)
return;
 
-   DPRINTF("%s: unit=%d\n", __func__, kd->kd_unit);
+   DPRINTF("%s: unit=%d, state=%d, mode=%d\n",
+   __func__, kd->kd_unit, kd->kd_state, kd->kd_mode);
 
-   if (kd->kd_mode == KCOV_MODE_DYING)
+   if (kd->kd_state == KCOV_STATE_DYING) {
kd_free(kd);
-   else
-   kd->kd_mode = KCOV_MODE_INIT;
+   } else {
+   kd->kd_state = KCOV_STATE_READY;
+   kd->kd_mode = KCOV_MODE_NONE;
+   }
p->p_kd = NULL;
 }
 
@@ -248,7 +257,7 @@ kd_init(struct kcov_dev *kd, unsigned lo
 
KASSERT(kd->kd_buf == NULL);
 
-   if (kd->kd_mode != KCOV_MODE_DISABLED)
+   if (kd->kd_state != KCOV_STATE_NONE)
return (EBUSY);
 
if (nmemb == 0 || nmemb > KCOV_BUF_MAX_NMEMB)
@@ -257,7 +266,7 @@ kd_init(struct kcov_dev *kd, unsigned lo
size = roundup(nmemb * sizeof(uintptr_t), PAGE_SIZE);
buf = malloc(size, M_SUBPROC, M_WAITOK | M_ZERO);
/* malloc() can sleep, ensure the race was won. */
-   if (kd->kd_mode != KCOV_MODE_DISABLED) {
+   if (kd->kd_state != KCOV_STATE_NONE) {
free(buf, M_SUBPROC, size);
return (EBUSY);
}
@@ -265,14 +274,15 @@ kd_init(struct kcov_dev *kd, unsigned lo
/* The first element is reserved to hold the number of used elements. */
kd->kd_nmemb = nmemb - 1;
kd->kd_size = size;
-   kd->kd_mode = KCOV_MODE_INIT;
+   kd->kd_state = 

Re: vmd: set dhcp hostname option during netboot

2018-12-10 Thread Anton Lindqvist
On Mon, Dec 10, 2018 at 09:40:41PM -0800, Mike Larkin wrote:
> On Mon, Dec 10, 2018 at 07:27:40PM -0800, Carlos Cardenas wrote:
> > On Sat, Dec 08, 2018 at 10:13:47AM +0100, Anton Lindqvist wrote:
> > > Hi,
> > > I've been trying out the new fake netboot feature in vmd. Overall, a
> > > great addition that removed the need for me to run dhcpd/rebound locally
> > > to achieve auto install. It would be convenient if the DHCP lease
> > > included a hostname inferred from the VM name in order to use dedicated
> > > response files for different VMs. Maybe this is a behavior that
> > > shouldn't be limited to just netboot? The res_hnok() validation is
> > > borrowed from dhclient.
> > > 
> > > Comments? OK?
> > 
> > This is a cool idea.
> > 
> > ok ccardenas@
> > 
> > +--+
> > Carlos
> > 
> 
> I am not opposed to this, but doesn't this assume that the vm name is
> the same as the desired hostname? (maybe that's ok?)

Not necessarily. The DHCP hostname is used to construct one of the URLs
used to probe for install.conf during autoinstall. The same hostname
will also be used as the default answer to the "System hostname"
question. Thus, the desired hostname can be overwritten in install.conf.
Also worth noting is that the final hostname written to /etc/myname is
joined together with the answer to the "DNS domain name" question.

> 
> reyk@, what do you think?
> 
> -ml
> 
> > > 
> > > Index: dhcp.c
> > > ===
> > > RCS file: /cvs/src/usr.sbin/vmd/dhcp.c,v
> > > retrieving revision 1.7
> > > diff -u -p -r1.7 dhcp.c
> > > --- dhcp.c6 Dec 2018 09:20:06 -   1.7
> > > +++ dhcp.c8 Dec 2018 09:04:33 -
> > > @@ -24,6 +24,7 @@
> > >  #include 
> > >  #include 
> > >  
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -44,8 +45,10 @@ dhcp_request(struct vionet_dev *dev, cha
> > >   struct packet_ctxpc;
> > >   struct dhcp_packet   req, resp;
> > >   struct in_addr   server_addr, mask, client_addr, requested_addr;
> > > - size_t   resplen, o;
> > > + size_t   len, resplen, o;
> > >   uint32_t ltime;
> > > + struct vmd_vm   *vm;
> > > + const char  *hostname = NULL;
> > >  
> > >   if (buflen < (ssize_t)(BOOTP_MIN_LEN + sizeof(struct ether_header)))
> > >   return (-1);
> > > @@ -108,8 +111,12 @@ dhcp_request(struct vionet_dev *dev, cha
> > >   resp.hlen = req.hlen;
> > >   resp.xid = req.xid;
> > >  
> > > - if (dev->pxeboot)
> > > + if (dev->pxeboot) {
> > >   strlcpy(resp.file, "auto_install", sizeof resp.file);
> > > + vm = vm_getbyvmid(dev->vm_vmid);
> > > + if (vm && res_hnok(vm->vm_params.vmc_params.vcp_name))
> > > + hostname = vm->vm_params.vmc_params.vcp_name;
> > > + }
> > >  
> > >   if ((client_addr.s_addr =
> > >   vm_priv_addr(>vmd_cfg,
> > > @@ -205,6 +212,14 @@ dhcp_request(struct vionet_dev *dev, cha
> > >   resp.options[o++] = sizeof(server_addr);
> > >   memcpy([o], _addr, sizeof(server_addr));
> > >   o += sizeof(server_addr);
> > > +
> > > + if (hostname != NULL) {
> > > + len = strlen(hostname);
> > > + resp.options[o++] = DHO_HOST_NAME;
> > > + resp.options[o++] = len;
> > > + memcpy([o], hostname, len);
> > > + o += len;
> > > + }
> > >  
> > >   resp.options[o++] = DHO_END;
> > >  
> > > 
> > 



Re: make build as root fails when SUDO=doas

2018-12-10 Thread Anton Lindqvist
On Mon, Dec 10, 2018 at 01:33:49PM +0100, Solene Rapenne wrote:
> hi
> 
> I have SUDO=doas in /etc/mk.conf for ports, this is preventing a `make build`
> in /usr/src as root if /etc/doas.conf doesn't have a line "permit nopass root
> as root". This fails when using "doas" in regress/usr/bin/ssh/
> 
> doas: Operation not permitted
> *** Error 1 in regress/usr.bin/ssh (Makefile:212 'clean')
> *** Error 1 in regress/usr.bin (:48 'cleandir')
> *** Error 1 in regress (:48 'cleandir')
> *** Error 1 in . (:48 'cleandir')
> *** Error 1 in . (Makefile:86 'do-build')
> *** Error 1 in /usr/src (Makefile:74 'build')
> 
> 
> the issue comes from the 3rd line of that extract from Makefile:212
> 
> clean: ${CLEAN_SUBDIR}
> rm -f ${CLEANFILES}
> test -z "${SUDO}" || ${SUDO} rm -f ${SUDO_CLEAN}
> rm -rf .putty
> 
> Not sure how to fix it. Maybe people shouldn't try to compile as root when
> having SUDO=doas set and then, it's not an issue anymore?

I have the following line in my /etc/mk.conf:

SUDO!!=[ `id -u` -ne 0 ] && echo /usr/bin/doas; true



Re: option kcov + GENERIC.MP -> silent crash

2018-12-09 Thread Anton Lindqvist
On Wed, Dec 05, 2018 at 10:03:38PM +0100, Anton Lindqvist wrote:
> On Sat, Dec 01, 2018 at 04:34:57PM +0100, Anton Lindqvist wrote:
> > On Tue, Nov 27, 2018 at 05:52:15PM -0800, Greg Steuck wrote:
> > > I booted the patched kernel and it seems to have gone farther and I 
> > > believe
> > > reached init before crashing.
> > 
> > By performing a semi-automated bisect I was able to identify the source
> > files that are incompatible with tracing. Common for all source files
> > seems to be that they define routines called early on in the boot
> > process before curcpu() is usable.
> > 
> > I do not have any plans on committing the diff below but please give it
> > a try. Instead, I'm working on extending the files.conf(5) grammar in
> > order to infer a different make target for the files.
> > 
> > Index: arch/amd64/conf/Makefile.amd64
> > ===
> > RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
> > retrieving revision 1.106
> > diff -u -p -r1.106 Makefile.amd64
> > --- arch/amd64/conf/Makefile.amd64  30 Oct 2018 11:08:30 -  1.106
> > +++ arch/amd64/conf/Makefile.amd64  1 Dec 2018 15:32:58 -
> > @@ -151,7 +151,31 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
> > ${CC} ${CFLAGS} ${CPPFLAGS} ${PROF} -c vers.c
> >  
> >  .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
> > +amd64_mem.o: $S/arch/amd64/amd64/amd64_mem.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +cpu.o: $S/arch/amd64/amd64/cpu.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +fpu.o: $S/arch/amd64/amd64/fpu.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +gdt.o: $S/arch/amd64/amd64/gdt.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +intr.o: $S/arch/amd64/amd64/intr.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +lapic.o: $S/arch/amd64/amd64/lapic.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +machdep.o: $S/arch/amd64/amd64/machdep.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +tsc.o: $S/arch/amd64/amd64/tsc.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> >  kcov.o: $S/dev/kcov.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +pvbus.o: $S/dev/pv/pvbus.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +kern_lock.o: $S/kern/kern_lock.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +kern_sched.o: $S/kern/kern_sched.c
> > +   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> > +kern_tc.o: $S/kern/kern_tc.c
> > ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> >  .endif
> >  
> 
> Here's a new diff taking a different approach. Keeping tracing off until
> all secondary CPUs have booted solves the issue of accessing curcpu()
> too early. Another issue was then discovered, curproc can be NULL before
> the idle thread tied the current CPU has started. Currently running with
> this diff applied on my laptop (MP) and positive results from Greg. The
> diff will be further exercised in the actual syzkaller setup before
> committing.

Yet another iteration and hopefully the last one. Idea from visa@ to
delay tracing until /dev/kcov has been successfully opened at least
once. At this point, accessing curcpu() is safe. Sort of a hack but it
removes the need to hook into init_main.c which is favorable.

Comments? OK?

Index: dev/kcov.c
===
RCS file: /cvs/src/sys/dev/kcov.c,v
retrieving revision 1.4
diff -u -p -r1.4 kcov.c
--- dev/kcov.c  27 Aug 2018 15:57:39 -  1.4
+++ dev/kcov.c  8 Dec 2018 16:40:41 -
@@ -58,6 +58,8 @@ static inline int inintr(void);
 
 TAILQ_HEAD(, kcov_dev) kd_list = TAILQ_HEAD_INITIALIZER(kd_list);
 
+int kcov_cold = 1;
+
 #ifdef KCOV_DEBUG
 int kcov_debug = 1;
 #endif
@@ -76,12 +78,15 @@ int kcov_debug = 1;
 void
 __sanitizer_cov_trace_pc(void)
 {
-   extern int cold;
struct kcov_dev *kd;
uint64_t idx;
 
-   /* Do not trace during boot. */
-   if (cold)
+   /*
+* Do not trace before kcovopen() has been called at least once.
+* At this point, all secondary CPUs have booted and accessing curcpu()
+* is safe.
+*/
+   if (kcov_cold)
return;
 
/* Do not trace in interrupts to prevent noisy coverage. */
@@ -111,6 +116,9 @@ kcovopen(dev_t dev, int flag, int mode, 
 
if (kd_lookup(minor(dev)) != NULL)
return (EBUSY);
+
+   if (kcov_cold)
+   kcov_cold = 0;
 
DPRINTF("%s: unit=%d\n", __func__, minor(dev));
 



vmd: set dhcp hostname option during netboot

2018-12-08 Thread Anton Lindqvist
Hi,
I've been trying out the new fake netboot feature in vmd. Overall, a
great addition that removed the need for me to run dhcpd/rebound locally
to achieve auto install. It would be convenient if the DHCP lease
included a hostname inferred from the VM name in order to use dedicated
response files for different VMs. Maybe this is a behavior that
shouldn't be limited to just netboot? The res_hnok() validation is
borrowed from dhclient.

Comments? OK?

Index: dhcp.c
===
RCS file: /cvs/src/usr.sbin/vmd/dhcp.c,v
retrieving revision 1.7
diff -u -p -r1.7 dhcp.c
--- dhcp.c  6 Dec 2018 09:20:06 -   1.7
+++ dhcp.c  8 Dec 2018 09:04:33 -
@@ -24,6 +24,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -44,8 +45,10 @@ dhcp_request(struct vionet_dev *dev, cha
struct packet_ctxpc;
struct dhcp_packet   req, resp;
struct in_addr   server_addr, mask, client_addr, requested_addr;
-   size_t   resplen, o;
+   size_t   len, resplen, o;
uint32_t ltime;
+   struct vmd_vm   *vm;
+   const char  *hostname = NULL;
 
if (buflen < (ssize_t)(BOOTP_MIN_LEN + sizeof(struct ether_header)))
return (-1);
@@ -108,8 +111,12 @@ dhcp_request(struct vionet_dev *dev, cha
resp.hlen = req.hlen;
resp.xid = req.xid;
 
-   if (dev->pxeboot)
+   if (dev->pxeboot) {
strlcpy(resp.file, "auto_install", sizeof resp.file);
+   vm = vm_getbyvmid(dev->vm_vmid);
+   if (vm && res_hnok(vm->vm_params.vmc_params.vcp_name))
+   hostname = vm->vm_params.vmc_params.vcp_name;
+   }
 
if ((client_addr.s_addr =
vm_priv_addr(>vmd_cfg,
@@ -205,6 +212,14 @@ dhcp_request(struct vionet_dev *dev, cha
resp.options[o++] = sizeof(server_addr);
memcpy([o], _addr, sizeof(server_addr));
o += sizeof(server_addr);
+
+   if (hostname != NULL) {
+   len = strlen(hostname);
+   resp.options[o++] = DHO_HOST_NAME;
+   resp.options[o++] = len;
+   memcpy([o], hostname, len);
+   o += len;
+   }
 
resp.options[o++] = DHO_END;
 



Re: option kcov + GENERIC.MP -> silent crash

2018-12-05 Thread Anton Lindqvist
On Sat, Dec 01, 2018 at 04:34:57PM +0100, Anton Lindqvist wrote:
> On Tue, Nov 27, 2018 at 05:52:15PM -0800, Greg Steuck wrote:
> > I booted the patched kernel and it seems to have gone farther and I believe
> > reached init before crashing.
> 
> By performing a semi-automated bisect I was able to identify the source
> files that are incompatible with tracing. Common for all source files
> seems to be that they define routines called early on in the boot
> process before curcpu() is usable.
> 
> I do not have any plans on committing the diff below but please give it
> a try. Instead, I'm working on extending the files.conf(5) grammar in
> order to infer a different make target for the files.
> 
> Index: arch/amd64/conf/Makefile.amd64
> ===
> RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
> retrieving revision 1.106
> diff -u -p -r1.106 Makefile.amd64
> --- arch/amd64/conf/Makefile.amd6430 Oct 2018 11:08:30 -  1.106
> +++ arch/amd64/conf/Makefile.amd641 Dec 2018 15:32:58 -
> @@ -151,7 +151,31 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
>   ${CC} ${CFLAGS} ${CPPFLAGS} ${PROF} -c vers.c
>  
>  .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
> +amd64_mem.o: $S/arch/amd64/amd64/amd64_mem.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +cpu.o: $S/arch/amd64/amd64/cpu.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +fpu.o: $S/arch/amd64/amd64/fpu.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +gdt.o: $S/arch/amd64/amd64/gdt.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +intr.o: $S/arch/amd64/amd64/intr.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +lapic.o: $S/arch/amd64/amd64/lapic.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +machdep.o: $S/arch/amd64/amd64/machdep.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +tsc.o: $S/arch/amd64/amd64/tsc.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
>  kcov.o: $S/dev/kcov.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +pvbus.o: $S/dev/pv/pvbus.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +kern_lock.o: $S/kern/kern_lock.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +kern_sched.o: $S/kern/kern_sched.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +kern_tc.o: $S/kern/kern_tc.c
>   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
>  .endif
>  

Here's a new diff taking a different approach. Keeping tracing off until
all secondary CPUs have booted solves the issue of accessing curcpu()
too early. Another issue was then discovered, curproc can be NULL before
the idle thread tied the current CPU has started. Currently running with
this diff applied on my laptop (MP) and positive results from Greg. The
diff will be further exercised in the actual syzkaller setup before
committing.

Comments? OK?

diff --git sys/dev/kcov.c sys/dev/kcov.c
index 8e36bc8b8ef..c97aae4ed5d 100644
--- sys/dev/kcov.c
+++ sys/dev/kcov.c
@@ -58,6 +58,8 @@ static inline int inintr(void);
 
 TAILQ_HEAD(, kcov_dev) kd_list = TAILQ_HEAD_INITIALIZER(kd_list);
 
+int kcov_cold = 1;
+
 #ifdef KCOV_DEBUG
 int kcov_debug = 1;
 #endif
@@ -76,19 +78,31 @@ int kcov_debug = 1;
 void
 __sanitizer_cov_trace_pc(void)
 {
-   extern int cold;
struct kcov_dev *kd;
+   struct proc *p;
uint64_t idx;
 
-   /* Do not trace during boot. */
-   if (cold)
+   /*
+* Do not trace before all secondary CPUs have booted.
+* Accessing the current CPU during boot causes a subtle crash since its
+* GSBASE register has not yet been written.
+*/
+   if (kcov_cold)
return;
 
/* Do not trace in interrupts to prevent noisy coverage. */
if (inintr())
return;
 
-   kd = curproc->p_kd;
+   /*
+* Protect against when the idle thread for the current CPU has not yet
+* started and curproc is absent.
+*/
+   p = curproc;
+   if (p == NULL)
+   return;
+
+   kd = p->p_kd;
if (kd == NULL || kd->kd_mode != KCOV_MODE_TRACE_PC)
return;
 
@@ -226,6 +240,12 @@ kcov_exit(struct proc *p)
p->p_kd = NULL;
 }
 
+void
+kcov_init(void)
+{
+   kcov_cold = 0;
+}
+
 struct kcov_dev *
 kd_lookup(int unit)
 {
diff --git sys/kern/init_main.c sys/kern/init_main.c
index 91070090bb1..25b71fd89ce 100644
--- sys/kern/init_main.c
+++ sys/kern/init_main.c
@@ -103,6 +103,11 @@ extern void nfs_init(void);
 #include "vscsi.h"
 #include "softraid.h"
 
+#include "kcov.h"
+#if NKCOV > 0
+#include 
+#endif
+
 const char copyright[] =
 "Copyright (c) 1982, 1986, 1989, 1991, 1993\n"
 "\tThe Regents of the University of California.  All rig

Re: option kcov + GENERIC.MP -> silent crash

2018-12-01 Thread Anton Lindqvist
On Tue, Nov 27, 2018 at 05:52:15PM -0800, Greg Steuck wrote:
> I booted the patched kernel and it seems to have gone farther and I believe
> reached init before crashing.

By performing a semi-automated bisect I was able to identify the source
files that are incompatible with tracing. Common for all source files
seems to be that they define routines called early on in the boot
process before curcpu() is usable.

I do not have any plans on committing the diff below but please give it
a try. Instead, I'm working on extending the files.conf(5) grammar in
order to infer a different make target for the files.

Index: arch/amd64/conf/Makefile.amd64
===
RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
retrieving revision 1.106
diff -u -p -r1.106 Makefile.amd64
--- arch/amd64/conf/Makefile.amd64  30 Oct 2018 11:08:30 -  1.106
+++ arch/amd64/conf/Makefile.amd64  1 Dec 2018 15:32:58 -
@@ -151,7 +151,31 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
${CC} ${CFLAGS} ${CPPFLAGS} ${PROF} -c vers.c
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
+amd64_mem.o: $S/arch/amd64/amd64/amd64_mem.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+cpu.o: $S/arch/amd64/amd64/cpu.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+fpu.o: $S/arch/amd64/amd64/fpu.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+gdt.o: $S/arch/amd64/amd64/gdt.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+intr.o: $S/arch/amd64/amd64/intr.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+lapic.o: $S/arch/amd64/amd64/lapic.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+machdep.o: $S/arch/amd64/amd64/machdep.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+tsc.o: $S/arch/amd64/amd64/tsc.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
 kcov.o: $S/dev/kcov.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+pvbus.o: $S/dev/pv/pvbus.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+kern_lock.o: $S/kern/kern_lock.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+kern_sched.o: $S/kern/kern_sched.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+kern_tc.o: $S/kern/kern_tc.c
${NORMAL_C} -fno-sanitize-coverage=trace-pc
 .endif
 



Re: option kcov + GENERIC.MP -> silent crash

2018-11-26 Thread Anton Lindqvist
On Mon, Nov 26, 2018 at 08:43:12AM -0800, Greg Steuck wrote:
> Thanks for the patch, I'll give it a go. Should I make up a patch reporting
> #error when trying to build kcov with MP in the meantime? The next person
> won't have to find it the hard way...

Please try out the diff first. I'd rather try coming up with a proper
fix before adding any #error directives.

> 
> On Sun, Nov 25, 2018 at 11:21 PM Anton Lindqvist  wrote:
> 
> > Hi Greg,
> >
> > On Sun, Nov 25, 2018 at 10:13:52AM -0800, Greg Steuck wrote:
> > > Hi Anton,
> > >
> > > I tried to boot a kernel with kcov based on GENERIC.MP and the machine
> > > reboots without a peep immediately after
> > >
> > > vmm0 at mainbus0: VMX (using slow L1TF mitigation)
> > >
> > > Switching off either of kcov or MP results in normally working kernels.
> > I'm
> > > attaching two concatenated dmesgs. The effect is reproducible on real HW
> > > and on GCE VM. Broken config is just:
> > > $ cat /sys/arch/amd64/conf/SYZKALLER
> > > include "arch/amd64/conf/GENERIC.MP"
> > > pseudo-device kcov 1
> > >
> > > Disabling either vmm or kcov in broken kernel UKC doesn't prevent
> > crashes.
> >
> > Known limitation, I haven't spent much time on making kcov MP-safe.
> > Especially since it's primarily used inside a VM through vmm which
> > currently is limited to a single CPU.
> >
> > However, I did some investigation before and concluded that the problem
> > resides in the trace routine which is called from
> > cpu_boot_secondary_processors() before the secondary CPU is accessible
> > through curcpu(). I came up with a hackish solution to this problem (see
> > diff below) that got rejected; kettenis@ mentioned that we instead
> > should set MSR_GSBASE earlier in cpu_hatch() but I never managed to get
> > the right people involved with knowledge in this area. I might take a
> > look myself.
> >
> > In the meantime, you could give the diff a try. It might be the case
> > that more functions are not eligible for tracing. OpenBSD as no method
> > of turning of tracing for a given source file like Linux does. This
> > might become necessary since I fear many more functions will not cope
> > with tracing.
> >
> > Index: dev/kcov.c
> > ===
> > RCS file: /cvs/src/sys/dev/kcov.c,v
> > retrieving revision 1.4
> > diff -u -p -r1.4 kcov.c
> > --- dev/kcov.c  27 Aug 2018 15:57:39 -  1.4
> > +++ dev/kcov.c  8 Sep 2018 21:51:20 -
> > @@ -49,6 +49,7 @@ struct kcov_dev {
> >  };
> >
> >  void kcovattach(int);
> > +void kcov_attachhook(struct device *);
> >
> >  int kd_alloc(struct kcov_dev *, unsigned long);
> >  void kd_free(struct kcov_dev *);
> > @@ -57,6 +58,7 @@ struct kcov_dev *kd_lookup(int);
> >  static inline int inintr(void);
> >
> >  TAILQ_HEAD(, kcov_dev) kd_list = TAILQ_HEAD_INITIALIZER(kd_list);
> > +int kcov_attached = 0;
> >
> >  #ifdef KCOV_DEBUG
> >  int kcov_debug = 1;
> > @@ -76,12 +78,11 @@ int kcov_debug = 1;
> >  void
> >  __sanitizer_cov_trace_pc(void)
> >  {
> > -   extern int cold;
> > struct kcov_dev *kd;
> > uint64_t idx;
> >
> > -   /* Do not trace during boot. */
> > -   if (cold)
> > +   /* Do not trace before the root file system is mounted. */
> > +   if (!kcov_attached)
> > return;
> >
> > /* Do not trace in interrupts to prevent noisy coverage. */
> > @@ -102,6 +103,13 @@ __sanitizer_cov_trace_pc(void)
> >  void
> >  kcovattach(int count)
> >  {
> > +   config_mountroot(NULL, kcov_attachhook);
> > +}
> > +
> > +void
> > +kcov_attachhook(struct device *dev)
> > +{
> > +   kcov_attached = 1;
> >  }
> >
> >  int
> >
> 
> 
> -- 
> nest.cx is Gmail hosted, use PGP for anything private. Key:
> http://goo.gl/6dMsr
> Fingerprint: 5E2B 2D0E 1E03 2046 BEC3  4D50 0B15 42BD 8DF5 A1B0



Re: option kcov + GENERIC.MP -> silent crash

2018-11-25 Thread Anton Lindqvist
Hi Greg,

On Sun, Nov 25, 2018 at 10:13:52AM -0800, Greg Steuck wrote:
> Hi Anton,
> 
> I tried to boot a kernel with kcov based on GENERIC.MP and the machine
> reboots without a peep immediately after
> 
> vmm0 at mainbus0: VMX (using slow L1TF mitigation)
> 
> Switching off either of kcov or MP results in normally working kernels. I'm
> attaching two concatenated dmesgs. The effect is reproducible on real HW
> and on GCE VM. Broken config is just:
> $ cat /sys/arch/amd64/conf/SYZKALLER
> include "arch/amd64/conf/GENERIC.MP"
> pseudo-device kcov 1
> 
> Disabling either vmm or kcov in broken kernel UKC doesn't prevent crashes.

Known limitation, I haven't spent much time on making kcov MP-safe.
Especially since it's primarily used inside a VM through vmm which
currently is limited to a single CPU.

However, I did some investigation before and concluded that the problem
resides in the trace routine which is called from
cpu_boot_secondary_processors() before the secondary CPU is accessible
through curcpu(). I came up with a hackish solution to this problem (see
diff below) that got rejected; kettenis@ mentioned that we instead
should set MSR_GSBASE earlier in cpu_hatch() but I never managed to get
the right people involved with knowledge in this area. I might take a
look myself.

In the meantime, you could give the diff a try. It might be the case
that more functions are not eligible for tracing. OpenBSD as no method
of turning of tracing for a given source file like Linux does. This
might become necessary since I fear many more functions will not cope
with tracing.

Index: dev/kcov.c
===
RCS file: /cvs/src/sys/dev/kcov.c,v
retrieving revision 1.4
diff -u -p -r1.4 kcov.c
--- dev/kcov.c  27 Aug 2018 15:57:39 -  1.4
+++ dev/kcov.c  8 Sep 2018 21:51:20 -
@@ -49,6 +49,7 @@ struct kcov_dev {
 };
 
 void kcovattach(int);
+void kcov_attachhook(struct device *);
 
 int kd_alloc(struct kcov_dev *, unsigned long);
 void kd_free(struct kcov_dev *);
@@ -57,6 +58,7 @@ struct kcov_dev *kd_lookup(int);
 static inline int inintr(void);
 
 TAILQ_HEAD(, kcov_dev) kd_list = TAILQ_HEAD_INITIALIZER(kd_list);
+int kcov_attached = 0;
 
 #ifdef KCOV_DEBUG
 int kcov_debug = 1;
@@ -76,12 +78,11 @@ int kcov_debug = 1;
 void
 __sanitizer_cov_trace_pc(void)
 {
-   extern int cold;
struct kcov_dev *kd;
uint64_t idx;
 
-   /* Do not trace during boot. */
-   if (cold)
+   /* Do not trace before the root file system is mounted. */
+   if (!kcov_attached)
return;
 
/* Do not trace in interrupts to prevent noisy coverage. */
@@ -102,6 +103,13 @@ __sanitizer_cov_trace_pc(void)
 void
 kcovattach(int count)
 {
+   config_mountroot(NULL, kcov_attachhook);
+}
+
+void
+kcov_attachhook(struct device *dev)
+{
+   kcov_attached = 1;
 }
 
 int



wscons: FIOGETOWN

2018-11-19 Thread Anton Lindqvist
Hi,
Now that wscons is using sigio, adding support for FIOSETOWN/TIOCGPGRP
is trivial.

Also, visa@ pointed out that FIOSETOWN gets translated into TIOCSPGRP by
sys_ioctl(), handling of FIOSETOWN is therefore redundant.

Comments? OK?

Index: wskbd.c
===
RCS file: /cvs/src/sys/dev/wscons/wskbd.c,v
retrieving revision 1.92
diff -u -p -r1.92 wskbd.c
--- wskbd.c 19 Nov 2018 19:19:24 -  1.92
+++ wskbd.c 19 Nov 2018 20:11:52 -
@@ -957,12 +957,13 @@ wskbd_do_ioctl_sc(struct wskbd_softc *sc
sc->sc_base.me_evp->async = *(int *)data != 0;
return (0);
 
-   case FIOSETOWN:
+   case TIOCGPGRP:
evar = sc->sc_base.me_evp;
if (evar == NULL)
return (EINVAL);
-   return (sigio_setown(>sigio, *(int *)data));
-  
+   *(int *)data = -sigio_getown(>sigio);
+   return (0);
+
case TIOCSPGRP:
if (*(int *)data < 0)
return (EINVAL);
Index: wsmouse.c
===
RCS file: /cvs/src/sys/dev/wscons/wsmouse.c,v
retrieving revision 1.48
diff -u -p -r1.48 wsmouse.c
--- wsmouse.c   19 Nov 2018 19:19:24 -  1.48
+++ wsmouse.c   19 Nov 2018 20:11:53 -
@@ -482,7 +482,6 @@ wsmouse_do_ioctl(struct wsmouse_softc *s
 
switch (cmd) {
case FIOASYNC:
-   case FIOSETOWN:
case TIOCSPGRP:
if ((flag & FWRITE) == 0)
return (EACCES);
@@ -498,11 +497,12 @@ wsmouse_do_ioctl(struct wsmouse_softc *s
sc->sc_base.me_evp->async = *(int *)data != 0;
return (0);
 
-   case FIOSETOWN:
+   case TIOCGPGRP:
evar = sc->sc_base.me_evp;
if (evar == NULL)
return (EINVAL);
-   return (sigio_setown(>sigio, *(int *)data));
+   *(int *)data = -sigio_getown(>sigio);
+   return (0);
 
case TIOCSPGRP:
if (*(int *)data < 0)
Index: wsmux.c
===
RCS file: /cvs/src/sys/dev/wscons/wsmux.c,v
retrieving revision 1.33
diff -u -p -r1.33 wsmux.c
--- wsmux.c 19 Nov 2018 19:19:24 -  1.33
+++ wsmux.c 19 Nov 2018 20:11:53 -
@@ -470,12 +470,12 @@ wsmux_do_ioctl(struct device *dv, u_long
return (EINVAL);
evar->async = *(int *)data != 0;
return (0);
-   case FIOSETOWN:
-   DPRINTF(("%s: FIOSETOWN\n", sc->sc_base.me_dv.dv_xname));
+   case TIOCGPGRP:
evar = sc->sc_base.me_evp;
if (evar == NULL)
return (EINVAL);
-   return (sigio_setown(>sigio, *(int *)data));
+   *(int *)data = -sigio_getown(>sigio);
+   return (0);
case TIOCSPGRP:
DPRINTF(("%s: TIOCSPGRP\n", sc->sc_base.me_dv.dv_xname));
if (*(int *)data < 0)



lockf: consistent use of new debug macro

2018-11-10 Thread Anton Lindqvist
Hi,
Make use of the new LFPRINT() debug macro.

Comments? OK?

Index: kern/vfs_lockf.c
===
RCS file: /cvs/src/sys/kern/vfs_lockf.c,v
retrieving revision 1.30
diff -u -p -r1.30 vfs_lockf.c
--- kern/vfs_lockf.c10 Nov 2018 11:54:03 -  1.30
+++ kern/vfs_lockf.c10 Nov 2018 17:41:01 -
@@ -302,10 +302,7 @@ lf_setlock(struct lockf *lock)
static char lockstr[] = "lockf";
int ovcase, priority, needtolink, error;
 
-#ifdef LOCKF_DEBUG
-   if (lockf_debug & DEBUG_SETLOCK)
-   lf_print("lf_setlock", lock);
-#endif /* LOCKF_DEBUG */
+   LFPRINT(("lf_setlock", lock), DEBUG_SETLOCK);
 
priority = PLOCK;
if (lock->lf_type == F_WRLCK)
@@ -369,12 +366,8 @@ lf_setlock(struct lockf *lock)
 * lf_next field refer to the blocking lock.
 */
lock->lf_next = block;
-#ifdef LOCKF_DEBUG
-   if (lockf_debug & DEBUG_SETLOCK) {
-   lf_print("lf_setlock", lock);
-   lf_print("lf_setlock: blocking on", block);
-   }
-#endif /* LOCKF_DEBUG */
+   LFPRINT(("lf_setlock", lock), DEBUG_SETLOCK);
+   LFPRINT(("lf_setlock: blocking on", block), DEBUG_SETLOCK);
TAILQ_INSERT_TAIL(>lf_blkhd, lock, lf_block);
error = tsleep(lock, priority, lockstr, 0);
if (lock->lf_next != NULL) {
@@ -502,11 +495,7 @@ lf_setlock(struct lockf *lock)
}
break;
}
-#ifdef LOCKF_DEBUG
-   if (lockf_debug & DEBUG_SETLOCK) {
-   lf_print("lf_setlock: got the lock", lock);
-   }
-#endif /* LOCKF_DEBUG */
+   LFPRINT(("lf_setlock: got the lock", lock), DEBUG_SETLOCK);
return (0);
 }
 
@@ -526,10 +515,7 @@ lf_clearlock(struct lockf *lock)
 
if (lf == NULL)
return (0);
-#ifdef LOCKF_DEBUG
-   if (lockf_debug & DEBUG_CLEARLOCK)
-   lf_print("lf_clearlock", lock);
-#endif /* LOCKF_DEBUG */
+   LFPRINT(("lf_clearlock", lock), DEBUG_CLEARLOCK);
while ((ovcase = lf_findoverlap(lf, lock, SELF, , ))) {
lf_wakelock(overlap);
 
@@ -570,10 +556,7 @@ lf_getlock(struct lockf *lock, struct fl
 {
struct lockf *block;
 
-#ifdef LOCKF_DEBUG
-   if (lockf_debug & DEBUG_CLEARLOCK)
-   lf_print("lf_getlock", lock);
-#endif /* LOCKF_DEBUG */
+   LFPRINT(("lf_getlock", lock), DEBUG_CLEARLOCK);
 
if ((block = lf_getblock(lock)) != NULL) {
fl->l_type = block->lf_type;
@@ -628,10 +611,7 @@ lf_findoverlap(struct lockf *lf, struct 
 {
off_t start, end;
 
-#ifdef LOCKF_DEBUG
-   if (lf && lockf_debug & DEBUG_FINDOVR)
-   lf_print("lf_findoverlap: looking for overlap in", lock);
-#endif /* LOCKF_DEBUG */
+   LFPRINT(("lf_findoverlap: looking for overlap in", lock), 
DEBUG_FINDOVR);
 
*overlap = lf;
start = lock->lf_start;
@@ -643,10 +623,7 @@ lf_findoverlap(struct lockf *lf, struct 
*overlap = lf = lf->lf_next;
continue;
}
-#ifdef LOCKF_DEBUG
-   if (lockf_debug & DEBUG_FINDOVR)
-   lf_print("\tchecking", lf);
-#endif /* LOCKF_DEBUG */
+   LFPRINT(("\tchecking", lf), DEBUG_FINDOVR);
/*
 * OK, check for overlap
 *
@@ -713,12 +690,8 @@ lf_split(struct lockf *lock1, struct loc
 {
struct lockf *splitlock;
 
-#ifdef LOCKF_DEBUG
-   if (lockf_debug & DEBUG_SPLIT) {
-   lf_print("lf_split", lock1);
-   lf_print("splitting from", lock2);
-   }
-#endif /* LOCKF_DEBUG */
+   LFPRINT(("lf_split", lock1), DEBUG_SPLIT);
+   LFPRINT(("splitting from", lock2), DEBUG_SPLIT);
 
/*
 * Check to see if splitting into only two pieces.



ktrace: struct flock

2018-11-04 Thread Anton Lindqvist
Hi,
Start tracing struct flock. I've been using this diff during lockf
development.

Comments? OK?

Index: sys/kern/kern_descrip.c
===
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.182
diff -u -p -r1.182 kern_descrip.c
--- sys/kern/kern_descrip.c 24 Aug 2018 12:45:27 -  1.182
+++ sys/kern/kern_descrip.c 5 Nov 2018 07:10:39 -
@@ -527,6 +527,10 @@ restart:
sizeof (fl));
if (error)
break;
+#ifdef KTRACE
+   if (KTRPOINT(p, KTR_STRUCT))
+   ktrflock(p, );
+#endif
if (fl.l_whence == SEEK_CUR) {
if (fl.l_start == 0 && fl.l_len < 0) {
/* lockf(3) compliance hack */
@@ -615,6 +619,10 @@ restart:
error = VOP_ADVLOCK(vp, fdp, F_GETLK, , F_POSIX);
if (error)
break;
+#ifdef KTRACE
+   if (KTRPOINT(p, KTR_STRUCT))
+   ktrflock(p, );
+#endif
error = (copyout((caddr_t), (caddr_t)SCARG(uap, arg),
sizeof (fl)));
break;
Index: sys/sys/ktrace.h
===
RCS file: /cvs/src/sys/sys/ktrace.h,v
retrieving revision 1.36
diff -u -p -r1.36 ktrace.h
--- sys/sys/ktrace.h28 Nov 2017 16:05:13 -  1.36
+++ sys/sys/ktrace.h5 Nov 2018 07:10:39 -
@@ -248,5 +248,7 @@ voidktrstruct(struct proc *, const c
ktrstruct(p, "pollfd", pfd, (count) * sizeof(struct pollfd))
 #define ktrfds(p, fds, count) \
ktrstruct(p, "fds", fds, (count) * sizeof(int))
+#define ktrflock(p, fl) \
+   ktrstruct(p, "flock", (fl), sizeof(struct flock))
 
 #endif /* !_KERNEL */
Index: usr.bin/kdump/kdump_subr.h
===
RCS file: /cvs/src/usr.bin/kdump/kdump_subr.h,v
retrieving revision 1.21
diff -u -p -r1.21 kdump_subr.h
--- usr.bin/kdump/kdump_subr.h  28 Apr 2017 13:53:05 -  1.21
+++ usr.bin/kdump/kdump_subr.h  5 Nov 2018 07:10:39 -
@@ -98,5 +98,6 @@ void evfflagsname(int, int);
 void pollfdeventname(int);
 void syslogflagname(int);
 void futexflagname(int);
+void flocktypename(int);
 
 extern int decimal, fancy, basecol, arg1;
Index: usr.bin/kdump/ktrstruct.c
===
RCS file: /cvs/src/usr.bin/kdump/ktrstruct.c,v
retrieving revision 1.25
diff -u -p -r1.25 ktrstruct.c
--- usr.bin/kdump/ktrstruct.c   13 Jul 2018 09:25:23 -  1.25
+++ usr.bin/kdump/ktrstruct.c   5 Nov 2018 07:10:39 -
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -517,6 +518,17 @@ ktrcmsghdr(char *data, socklen_t len)
printf("\n");
 }
 
+static void
+ktrflock(const struct flock *fl)
+{
+   printf("struct flock { start=%lld, len=%lld, pid=%d, type=",
+   fl->l_start, fl->l_len, fl->l_pid);
+   flocktypename(fl->l_type);
+   printf(", whence=");
+   whencename(fl->l_whence);
+   printf(" } \n");
+}
+
 void
 ktrstruct(char *buf, size_t buflen)
 {
@@ -658,6 +670,13 @@ ktrstruct(char *buf, size_t buflen)
printf("flags=");
showbufc(basecol + sizeof("flags=") - 1,
(unsigned char *)data, datalen, VIS_DQ | VIS_TAB | VIS_NL);
+   } else if (strcmp(name, "flock") == 0) {
+   struct flock fl;
+
+   if (datalen != sizeof(fl))
+   goto invalid;
+   memcpy(, data, datalen);
+   ktrflock();
} else {
printf("unknown structure %s\n", name);
}
Index: usr.bin/kdump/mksubr
===
RCS file: /cvs/src/usr.bin/kdump/mksubr,v
retrieving revision 1.35
diff -u -p -r1.35 mksubr
--- usr.bin/kdump/mksubr14 Feb 2018 17:26:56 -  1.35
+++ usr.bin/kdump/mksubr5 Nov 2018 07:10:39 -
@@ -365,6 +365,7 @@ auto_orz_type "pollfdeventname" "POLL[^_
 auto_orz_type "evflagsname" "EV_[^S][A-Z]+[[:space:]]+0x" "sys/event.h"
 auto_orz_type "syslogflagname" "LOG_[A-Z]+[[:space:]]+0x0*[1248]0*[[:space:]]" 
"sys/syslog.h"
 auto_orz_type "futexflagname" "FUTEX_[A-Z_]+[[:space:]]+[0-9]+" "sys/futex.h"
+auto_switch_type "flocktypename" "F_[A-Z]+LCK" "sys/fcntl.h"
 
 cat <<_EOF_
 /*



Re: ksh: fc -s and SIGINT

2018-10-29 Thread Anton Lindqvist
On Mon, Oct 29, 2018 at 09:55:38AM +0100, Martijn van Duren wrote:
> On 10/28/18 8:13 PM, Anton Lindqvist wrote:
> > Hi,
> > Bug reported by miod@, how to reproduce:
> > 
> >   $ command -v r
> >   alias r='fc -s'
> >   $ sleep 5
> >   $ r sleep
> >   ^C # abort sleep before finishing
> >   $ r sleep
> >   ksh: fc: history function called recursively
> > 
> > The c_fc() function has some internal state used to prevent recursive
> > invocations that gets out of sync when the re-executed command is
> > aborted by SIGINT. My proposal is to temporarily register a SIGINT
> > handler before re-executing the command in order to reset the call depth
> > counter upon signal delivery.
> > 
> > Comments? OK?
> > 
> You diff always unsets the shtrap pointer, which most likely unsets
> custom SIGINT handlers installed by the user (untested). Next to that
> this feels like special-casing and I'm afraid that there are some cases
> we might be overlooking here.

Thanks for the feedback. A few drive by thoughts, will look more closely
this evening: a user supplied SIGINT handler is never registered as a
one shot handler so it should never be overwritten. The hist_sigint()
one shot handler is registered before executing the command, if the
command does register a SIGINT handler it takes higher precedence and
hist_execute() will return under such circumstances implying that
fc_depth will be correctly decremented. The one shot handler is only
used when the command does not register a SIGINT handler.

> 
> The source with this issue is that a SIGINT triggers a list of
> siglongjmps, which don't allow us to return to c_fc and decrement the
> pointer. The diff below detects if we're on the lowest level of the
> unwinding by abusing the interactive variable. This works because
> calling fc from a non-interactive session is prohibited.
> 
> I'm not 100% convinced that the placement of fc_depth = 0 should be
> placed in shell(), or that we maybe should place it in unwind(). The
> reason I choose for shell() is because interactive is guaranteed to
> be setup for checking, because it's already there.
> 
> This diff is only lightly tested and the code here is very hard to
> untangle, so extreme scrutiny is desirable.
> 
> martijn@
> 
> Index: history.c
> ===
> RCS file: /cvs/src/bin/ksh/history.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 history.c
> --- history.c 15 Jan 2018 22:30:38 -  1.80
> +++ history.c 29 Oct 2018 08:19:12 -
> @@ -48,6 +48,8 @@ static uint32_t line_co;
>  
>  static struct stat last_sb;
>  
> +volatile sig_atomic_tfc_depth;
> +
>  int
>  c_fc(char **wp)
>  {
> @@ -58,9 +60,8 @@ c_fc(char **wp)
>   int optc, ret;
>   char *first = NULL, *last = NULL;
>   char **hfirst, **hlast, **hp;
> - static int depth;
>  
> - if (depth != 0) {
> + if (fc_depth != 0) {
>   bi_errorf("history function called recursively");
>   return 1;
>   }
> @@ -146,9 +147,9 @@ c_fc(char **wp)
>   hist_get_newest(false);
>   if (!hp)
>   return 1;
> - depth++;
> + fc_depth++;
>   ret = hist_replace(hp, pat, rep, gflag);
> - depth--;
> + fc_depth--;
>   return ret;
>   }
>  
> @@ -268,9 +269,9 @@ c_fc(char **wp)
>   shf_close(shf);
>   *xp = '\0';
>   strip_nuls(Xstring(xs, xp), Xlength(xs, xp));
> - depth++;
> + fc_depth++;
>   ret = hist_execute(Xstring(xs, xp));
> - depth--;
> + fc_depth--;
>   return ret;
>   }
>  }
> Index: main.c
> ===
> RCS file: /cvs/src/bin/ksh/main.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 main.c
> --- main.c29 Sep 2018 14:13:19 -  1.93
> +++ main.c29 Oct 2018 08:19:12 -
> @@ -549,6 +549,7 @@ shell(Source *volatile s, volatile int t
>   case LERROR:
>   case LSHELL:
>   if (interactive) {
> + fc_depth = 0;
>   if (i == LINTR)
>   shellf("\n");
>   /* Reset any eof that was read as part of a
> Index: sh.h
> ===
> RCS file: /cvs/src/bin/ksh/sh.h,v
> retrieving revision 1.73
> diff -u -p -r1.73 sh.h
&g

ksh: fc -s and SIGINT

2018-10-28 Thread Anton Lindqvist
Hi,
Bug reported by miod@, how to reproduce:

  $ command -v r
  alias r='fc -s'
  $ sleep 5
  $ r sleep
  ^C # abort sleep before finishing
  $ r sleep
  ksh: fc: history function called recursively

The c_fc() function has some internal state used to prevent recursive
invocations that gets out of sync when the re-executed command is
aborted by SIGINT. My proposal is to temporarily register a SIGINT
handler before re-executing the command in order to reset the call depth
counter upon signal delivery.

Comments? OK?

Index: history.c
===
RCS file: /cvs/src/bin/ksh/history.c,v
retrieving revision 1.80
diff -u -p -r1.80 history.c
--- history.c   15 Jan 2018 22:30:38 -  1.80
+++ history.c   28 Oct 2018 19:10:54 -
@@ -36,6 +36,8 @@ static char   **hist_get(const char *, i
 static char   **hist_get_oldest(void);
 static voidhistbackup(void);
 
+static voidhist_sigint(int);
+
 static FILE*histfh;
 static char   **histbase;  /* actual start of the history[] allocation */
 static char   **current;   /* current position in history[] */
@@ -46,6 +48,8 @@ static intignorespace;/* ditch lines s
 static Source  *hist_source;
 static uint32_tline_co;
 
+static volatile sig_atomic_t   fc_depth;
+
 static struct stat last_sb;
 
 int
@@ -58,9 +62,8 @@ c_fc(char **wp)
int optc, ret;
char *first = NULL, *last = NULL;
char **hfirst, **hlast, **hp;
-   static int depth;
 
-   if (depth != 0) {
+   if (fc_depth != 0) {
bi_errorf("history function called recursively");
return 1;
}
@@ -70,6 +73,8 @@ c_fc(char **wp)
return 1;
}
 
+   setsig([SIGINT], hist_sigint, SS_ONESHOT);
+
while ((optc = ksh_getopt(wp, _opt,
"e:glnrs0,1,2,3,4,5,6,7,8,9,")) != -1)
switch (optc) {
@@ -146,9 +151,9 @@ c_fc(char **wp)
hist_get_newest(false);
if (!hp)
return 1;
-   depth++;
+   fc_depth++;
ret = hist_replace(hp, pat, rep, gflag);
-   depth--;
+   fc_depth--;
return ret;
}
 
@@ -268,9 +273,9 @@ c_fc(char **wp)
shf_close(shf);
*xp = '\0';
strip_nuls(Xstring(xs, xp), Xlength(xs, xp));
-   depth++;
+   fc_depth++;
ret = hist_execute(Xstring(xs, xp));
-   depth--;
+   fc_depth--;
return ret;
}
 }
@@ -852,4 +857,10 @@ void
 hist_finish(void)
 {
history_close();
+}
+
+static void
+hist_sigint(int signo)
+{
+   fc_depth = 0;
 }
Index: sh.h
===
RCS file: /cvs/src/bin/ksh/sh.h,v
retrieving revision 1.73
diff -u -p -r1.73 sh.h
--- sh.h18 May 2018 13:25:20 -  1.73
+++ sh.h28 Oct 2018 19:10:54 -
@@ -230,6 +230,7 @@ typedef struct trap {
 #define TF_TTY_INTRBIT(7)  /* tty generated signal (see j_waitj) */
 #define TF_CHANGED BIT(8)  /* used by runtrap() to detect trap changes */
 #define TF_FATAL   BIT(9)  /* causes termination if not trapped */
+#define TF_ONESHOT BIT(10) /* temporary trap handler registered */
 
 /* values for setsig()/setexecsig() flags argument */
 #define SS_RESTORE_MASK0x3 /* how to restore a signal before an 
exec() */
@@ -240,6 +241,7 @@ typedef struct trap {
 #define SS_FORCE   BIT(3)  /* set signal even if original signal ignored */
 #define SS_USERBIT(4)  /* user is doing the set (ie, trap 
command) */
 #define SS_SHTRAP  BIT(5)  /* trap for internal use (CHLD,ALRM,WINCH) */
+#define SS_ONESHOT BIT(6)  /* register temporary trap handler */
 
 #define SIGEXIT_   0   /* for trap EXIT */
 #define SIGERR_NSIG/* for trap ERR */
Index: trap.c
===
RCS file: /cvs/src/bin/ksh/trap.c,v
retrieving revision 1.32
diff -u -p -r1.32 trap.c
--- trap.c  15 Mar 2018 16:51:29 -  1.32
+++ trap.c  28 Oct 2018 19:10:54 -
@@ -13,6 +13,8 @@
 
 Trap sigtraps[NSIG + 1];
 
+static sig_t   traphandler(Trap *);
+
 static struct sigaction Sigact_ign, Sigact_trap;
 
 void
@@ -117,6 +119,7 @@ void
 trapsig(int i)
 {
Trap *p = [i];
+   sig_t f;
int errno_ = errno;
 
trap = p->set = 1;
@@ -126,8 +129,9 @@ trapsig(int i)
fatal_trap = 1;
intrsig = 1;
}
-   if (p->shtrap)
-   (*p->shtrap)(i);
+   f = traphandler(p);
+   if (f)
+   (*f)(i);
errno = errno_;
 }
 
@@ -200,10 +204,13 @@ runtraps(int flag)
intrsig = 0;
if (flag & TF_FATAL)
fatal_trap = 0;
-   for (p = sigtraps, i = NSIG+1; --i >= 0; p++)
+   for (p = 

Re: Regression in "Add support to create and convert disk images from existing images"

2018-10-27 Thread Anton Lindqvist
On Wed, Oct 24, 2018 at 05:51:08PM +0200, Anton Lindqvist wrote:
> On Mon, Oct 22, 2018 at 11:05:13AM -0700, Greg Steuck wrote:
> > Hi Reyk & Anton,
> > 
> > I upgraded the syzkaller machine from Oct 11 to Oct 21 snapshot and started
> > seeing:
> > Oct 22 10:00:21 ci-openbsd vmd[15707]: qc2_open: missing base image
> > /syzkaller/managers/main/current/image
> > Oct 22 10:00:21 ci-openbsd vmd[15707]:
> > /syzkaller/managers/main/current/image: could not open qc2_open: No such
> > file or directory
> > Oct 22 10:00:21 ci-openbsd vmd[13268]: qc2_open: missing base image
> > /syzkaller/managers/main/current/image
> > Oct 22 10:00:21 ci-openbsd vmd[13268]:
> > /syzkaller/managers/main/current/image: could not open qc2_open: No such
> > file or directory
> > Oct 22 10:00:21 ci-openbsd vmd[51186]: qc2_open: missing base image
> > /syzkaller/managers/main/current/image
> > Oct 22 10:00:21 ci-openbsd vmd[51186]:
> > /syzkaller/managers/main/current/image: could not open qc2_open: No such
> > file or directory
> > 
> > Reverting the change in subject and rebuilding vmd stops the problem.
> > 
> > The file in question exists:
> > ci-openbsd$ file /syzkaller/managers/main/current/image
> > /syzkaller/managers/main/current/image: QEMU QCOW Image (v3), 1073741824
> > bytes
> > ci-openbsd$ ls -l /syzkaller/managers/main/current/image
> > -rw-r-  2 syzkaller  syzkaller  545783808 Oct 22 10:34
> > /syzkaller/managers/main/current/image
> > 
> > Under the hood this command line is executed:
> > vmctl start ci-openbsd-main-0 -b /syzkaller/managers/main/current/kernel -d
> > /syzkaller/managers/main/workdir/instance-0/disk.qcow2 -m 512M -L -c -t
> > syzkaller
> > 
> > Anton, what does your code is syzkaller do to create this file?
> > -rw---  1 syzkaller  syzkaller  262144 Oct 22 10:59
> > /syzkaller/managers/main/workdir/instance-0/disk.qcow2
> > 
> > 
> > The file does reference the base image:
> > ci-openbsd$ strings /syzkaller/managers/main/workdir/instance-0/disk.qcow2
> > 
> > h/syzkaller/managers/main/current/image
> > 
> > Thanks
> > Greg
> > -- 
> > nest.cx is Gmail hosted, use PGP for anything private. Key:
> > http://goo.gl/6dMsr
> > Fingerprint: 5E2B 2D0E 1E03 2046 BEC3  4D50 0B15 42BD 8DF5 A1B0
> 
> I'm also not able to start a VM using a derived disk after this commit.
> It looks to me like the code is trying to resolve the base disk for the
> derived disk over and over instead of walking down the chain of derived
> disks. This causes file descriptors to be opened only for the derived
> disk whereas the base disk is expected. With the diff below, I'm able to
> boot using a derived disk again.
> 
> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/vmd/config.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 config.c
> --- config.c  19 Oct 2018 10:12:39 -  1.53
> +++ config.c  24 Oct 2018 15:48:00 -
> @@ -364,6 +364,10 @@ config_setvm(struct privsep *ps, struct 
>   base, vcp->vcp_disks[i]);
>   goto fail;
>   }
> + if (strlcpy(path, base, sizeof(path)) >= sizeof(path)) {
> + log_warnx("%s, base path too long", __func__);
> + goto fail;
> + }
>   }
>   }
>  

For the record, reyk@ committed a similar fix yesterday so you should
be able to use a recent snapshot at this point.



Re: Regression in "Add support to create and convert disk images from existing images"

2018-10-24 Thread Anton Lindqvist
On Mon, Oct 22, 2018 at 11:05:13AM -0700, Greg Steuck wrote:
> Hi Reyk & Anton,
> 
> I upgraded the syzkaller machine from Oct 11 to Oct 21 snapshot and started
> seeing:
> Oct 22 10:00:21 ci-openbsd vmd[15707]: qc2_open: missing base image
> /syzkaller/managers/main/current/image
> Oct 22 10:00:21 ci-openbsd vmd[15707]:
> /syzkaller/managers/main/current/image: could not open qc2_open: No such
> file or directory
> Oct 22 10:00:21 ci-openbsd vmd[13268]: qc2_open: missing base image
> /syzkaller/managers/main/current/image
> Oct 22 10:00:21 ci-openbsd vmd[13268]:
> /syzkaller/managers/main/current/image: could not open qc2_open: No such
> file or directory
> Oct 22 10:00:21 ci-openbsd vmd[51186]: qc2_open: missing base image
> /syzkaller/managers/main/current/image
> Oct 22 10:00:21 ci-openbsd vmd[51186]:
> /syzkaller/managers/main/current/image: could not open qc2_open: No such
> file or directory
> 
> Reverting the change in subject and rebuilding vmd stops the problem.
> 
> The file in question exists:
> ci-openbsd$ file /syzkaller/managers/main/current/image
> /syzkaller/managers/main/current/image: QEMU QCOW Image (v3), 1073741824
> bytes
> ci-openbsd$ ls -l /syzkaller/managers/main/current/image
> -rw-r-  2 syzkaller  syzkaller  545783808 Oct 22 10:34
> /syzkaller/managers/main/current/image
> 
> Under the hood this command line is executed:
> vmctl start ci-openbsd-main-0 -b /syzkaller/managers/main/current/kernel -d
> /syzkaller/managers/main/workdir/instance-0/disk.qcow2 -m 512M -L -c -t
> syzkaller
> 
> Anton, what does your code is syzkaller do to create this file?
> -rw---  1 syzkaller  syzkaller  262144 Oct 22 10:59
> /syzkaller/managers/main/workdir/instance-0/disk.qcow2
> 
> 
> The file does reference the base image:
> ci-openbsd$ strings /syzkaller/managers/main/workdir/instance-0/disk.qcow2
> 
> h/syzkaller/managers/main/current/image
> 
> Thanks
> Greg
> -- 
> nest.cx is Gmail hosted, use PGP for anything private. Key:
> http://goo.gl/6dMsr
> Fingerprint: 5E2B 2D0E 1E03 2046 BEC3  4D50 0B15 42BD 8DF5 A1B0

I'm also not able to start a VM using a derived disk after this commit.
It looks to me like the code is trying to resolve the base disk for the
derived disk over and over instead of walking down the chain of derived
disks. This causes file descriptors to be opened only for the derived
disk whereas the base disk is expected. With the diff below, I'm able to
boot using a derived disk again.

Index: config.c
===
RCS file: /cvs/src/usr.sbin/vmd/config.c,v
retrieving revision 1.53
diff -u -p -r1.53 config.c
--- config.c19 Oct 2018 10:12:39 -  1.53
+++ config.c24 Oct 2018 15:48:00 -
@@ -364,6 +364,10 @@ config_setvm(struct privsep *ps, struct 
base, vcp->vcp_disks[i]);
goto fail;
}
+   if (strlcpy(path, base, sizeof(path)) >= sizeof(path)) {
+   log_warnx("%s, base path too long", __func__);
+   goto fail;
+   }
}
}
 



Re: Add bufferevent_setwatermark(3) to manual

2018-09-23 Thread Anton Lindqvist
On Sat, Sep 22, 2018 at 11:41:05AM -0700, Geoff Hill wrote:
> New patch included.
> 
> On Sat, Sep 22, 2018 at 07:09:44AM +0100, Jason McIntyre wrote:
> > does the "ok" request mean you have commit access? (sorry, find it hard
> > to keep track)
> 
> Nope, no commit access, just a user sending up a documentation fix.
> 
> > regarding the markup: there is no need to quote your argument. actually
> > it would be inconsistent to do so. if the intent was to actually display
> > quotes in the formatted page (which would be wrong too), you would have
> > to use a macro such as Dq.
> 
> Fixed these. Changed to end up leaving the previous paragraph as-is.
> 
> > you need a space between the arg and punctuation
> 
> Fixed.
> 
> > you can find trivial errors like that if you run your changes through
> > mandoc:
> > 
> > $ mandoc -Tlint event.3
> 
> Thanks, that is good to know. Should have kept mdoc(7) up while writing.
> 
> Geoff

Thanks! Committed with some additional tweaks.

> 
> 
> Index: event.3
> ===
> RCS file: /cvs/src/lib/libevent/event.3,v
> retrieving revision 1.54
> diff -u -p -u -r1.54 event.3
> --- event.3   26 Jul 2018 12:50:04 -  1.54
> +++ event.3   22 Sep 2018 17:16:18 -
> @@ -68,6 +68,7 @@
>  .Nm bufferevent_enable ,
>  .Nm bufferevent_disable ,
>  .Nm bufferevent_settimeout ,
> +.Nm bufferevent_setwatermark ,
>  .Nm EVBUFFER_INPUT ,
>  .Nm EVBUFFER_OUTPUT
>  .Nd execute a function when a specific event occurs
> @@ -156,6 +157,8 @@
>  .Fn "bufferevent_disable" "struct bufferevent *bufev" "short event"
>  .Ft void
>  .Fn "bufferevent_settimeout" "struct bufferevent *bufev" "int timeout_read" 
> "int timeout_write"
> +.Ft void
> +.Fn "bufferevent_setwatermark" "struct bufferevent *bufev" "short events" 
> "size_t lowmark" "size_t highmark"
>  .Ft "struct evbuffer *"
>  .Fn "EVBUFFER_INPUT" "struct bufferevent *bufev"
>  .Ft "struct evbuffer *"
> @@ -496,6 +499,30 @@ whenever the output buffer is drained be
>  which is
>  .Va 0
>  by default.
> +.Pp
> +The
> +.Fn bufferevent_setwatermark
> +function can set the low and high watermarks
> +for read and write events.
> +.Fa "events"
> +can be
> +.Va EV_READ ,
> +.Va EV_WRITE
> +or both via bitwise-OR.
> +When used with
> +.Va EV_READ ,
> +a bufferevent does not invoke the user read callback
> +unless there is at least
> +.Fa lowmark
> +data in the buffer.
> +If the read buffer is beyond
> +.Fa highmark ,
> +the bufferevent stops reading from the file descriptor.
> +When used with
> +.Va EV_WRITE ,
> +the user write callback is invoked whenever the buffered data
> +falls below
> +.Fa lowmark .
>  .Pp
>  The
>  .Fn bufferevent_write



Re: Add bufferevent_setwatermark(3) to manual

2018-09-22 Thread Anton Lindqvist
On Fri, Sep 21, 2018 at 06:36:54PM -0700, Geoff Hill wrote:
> Hello tech,
> 
> I noticed the event(3) manual pages don't mention the
> bufferevent_setwatermark(3) function and glosses over the details of
> watermarks, even though there's a few programs in userland that set both
> read and write watermarks. Looks like there was an effort in 2017
> to add some documentation but it stalled.
> 
> Here's a patch that adds the function synopsis and a brief description
> of how watermarks work separately for read and write. Mostly copied from
> the function declaration comments in event.h.
> 
> ok?

Few nits below, otherwise it looks good. Anyone else willing to ok?

> 
> Geoff Hill
> 
> 
> Index: event.3
> ===
> RCS file: /cvs/src/lib/libevent/event.3,v
> retrieving revision 1.54
> diff -u -p -u -r1.54 event.3
> --- event.3   26 Jul 2018 12:50:04 -  1.54
> +++ event.3   22 Sep 2018 01:26:56 -
> @@ -68,6 +68,7 @@
>  .Nm bufferevent_enable ,
>  .Nm bufferevent_disable ,
>  .Nm bufferevent_settimeout ,
> +.Nm bufferevent_setwatermark ,
>  .Nm EVBUFFER_INPUT ,
>  .Nm EVBUFFER_OUTPUT
>  .Nd execute a function when a specific event occurs
> @@ -156,6 +157,8 @@
>  .Fn "bufferevent_disable" "struct bufferevent *bufev" "short event"
>  .Ft void
>  .Fn "bufferevent_settimeout" "struct bufferevent *bufev" "int timeout_read" 
> "int timeout_write"
> +.Ft void
> +.Fn "bufferevent_setwatermark" "struct bufferevent *bufev" "short events" 
> "size_t lowmark" "size_t highmark"
>  .Ft "struct evbuffer *"
>  .Fn "EVBUFFER_INPUT" "struct bufferevent *bufev"
>  .Ft "struct evbuffer *"
> @@ -492,10 +495,35 @@ and
>  When read enabled the bufferevent will try to read from the file
>  descriptor and call the read callback.
>  The write callback is executed
> -whenever the output buffer is drained below the write low watermark,
> +whenever the output buffer is drained below the write
> +.Fa "lowmark" ,
>  which is
>  .Va 0
>  by default.
> +.Pp
> +The
> +.Fn bufferevent_setwatermark
> +function can set the the low and high watermarks

One `the' is enough.

> +for read and write events.
> +.Fa "events"
> +can be
> +.Va EV_READ ,
> +.Va EV_WRITE
> +or both.
> +When used with
> +.Va EV_READ ,
> +a bufferevent does not invoke the user read callback
> +unless there is at least
> +.Fa "lowmark"
> +data in the buffer.
> +If the read buffer is beyond
> +.Fa "highmark" ,
> +the bufferevent stops reading from the file descriptor.
> +When used with
> +.Va EV_WRITE,

Missing space between EV_WRITE and comma.

> +the user write callback is invoked whenever the buffered data
> +falls below
> +.Fa "lowmark" .
>  .Pp
>  The
>  .Fn bufferevent_write



Re: [patch] rebound: add NULL pointer checks

2018-09-08 Thread Anton Lindqvist
On Sat, Sep 08, 2018 at 11:48:35AM +0200, Clemens Goessnitzer wrote:
> 
> 
> On 08/09/18 11:40, Theo Buehler wrote:
> > On Sat, Sep 08, 2018 at 11:07:30AM +0200, Anton Lindqvist wrote:
> > > On Fri, Sep 07, 2018 at 09:22:33PM +0200, Theo Buehler wrote:
> > > > On Fri, Sep 07, 2018 at 09:15:30PM +0200, Clemens Goessnitzer wrote:
> > > > > This patch adds 2 missing NULL pointer checks to rebound.c after 
> > > > > malloc().
> > > > 
> > > > The same function also contains an unchecked calloc.
> > > 
> > > Since it's a daemon I guess it makes sense to continue execution instead
> > > of dying. However, we should make sure to not leak memory along the
> > > error paths. Also, log something when preloading the cache fails.
> > > 
> > > Regress passed; ok?
> > 
> > You may want to consider returning -1 instead of 1 on failure for
> > consistency with the rest of the daemon. Apart from that this looks
> > good.
> > 
> > ok
> > 
> 
> Or maybe something with goto error?

Committed, I settled on using goto. Thanks!

> 
> Clemens

> Index: rebound.c
> ===
> RCS file: /cvs/src/usr.sbin/rebound/rebound.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 rebound.c
> --- rebound.c 1 May 2018 15:14:43 -   1.98
> +++ rebound.c 8 Sep 2018 09:45:23 -
> @@ -630,12 +630,12 @@ encodename(const char *name, unsigned ch
>   return totlen;
>  }
>  
> -static void
> +static int
>  preloadcache(const char *name, uint16_t type, void *rdata, uint16_t rdatalen)
>  {
> - struct dnspacket *req, *resp;
> + struct dnspacket *req = NULL, *resp = NULL;
>   size_t reqlen, resplen;
> - struct dnscache *ent;
> + struct dnscache *ent = NULL;
>   unsigned char *p;
>   uint16_t len, class;
>   uint32_t ttl;
> @@ -643,6 +643,8 @@ preloadcache(const char *name, uint16_t 
>   /* header + len + name + type + class */
>   reqlen = sizeof(*req) + 1 + strlen(name) + 2 + 2;
>   req = malloc(reqlen);
> + if (req == NULL)
> + goto error;
>  
>   req->id = 0;
>   req->flags = htons(0x100);
> @@ -661,6 +663,9 @@ preloadcache(const char *name, uint16_t 
>   /* req + name (compressed) + type + class + ttl + len + data */
>   resplen = reqlen + 2 + 2 + 2 + 4 + 2 + rdatalen;
>   resp = malloc(resplen);
> + if (resp == NULL)
> + goto error;
> +
>   memcpy(resp, req, reqlen);
>   resp->flags = htons(0x100 | 0x8000);/* response */
>   resp->ancount = htons(1);
> @@ -682,6 +687,9 @@ preloadcache(const char *name, uint16_t 
>   memcpy(p, rdata, rdatalen);
>  
>   ent = calloc(1, sizeof(*ent));
> + if (ent == NULL)
> + goto error;
> +
>   ent->req = req;
>   ent->reqlen = reqlen;
>   ent->resp = resp;
> @@ -690,6 +698,13 @@ preloadcache(const char *name, uint16_t 
>  
>   RB_INSERT(cachetree, , ent);
>  
> + return 0;
> +
> +error:
> + free(req);
> + free(resp);
> + free(ent);
> + return -1;
>  }
>  
>  static void
> @@ -699,7 +714,8 @@ preloadA(const char *name, const char *i
>  
>   inet_aton(ip, );
>  
> - preloadcache(name, htons(1), , 4);
> + if (preloadcache(name, htons(1), , 4) == -1)
> + logerr("failed to add cahce entry for %s", name);
>  }
>  
>  static void
> @@ -715,7 +731,8 @@ preloadPTR(const char *ip, const char *n
>  
>   encodename(name, namebuf);
>  
> - preloadcache(ipbuf, htons(12), namebuf, 1 + strlen(namebuf));
> + if (preloadcache(ipbuf, htons(12), namebuf, 1 + strlen(namebuf)) == -1)
> + logerr("failed to load cache entry for %s", ip);
>  }
>  
>  static int



Re: [patch] rebound: add NULL pointer checks

2018-09-08 Thread Anton Lindqvist
On Fri, Sep 07, 2018 at 09:22:33PM +0200, Theo Buehler wrote:
> On Fri, Sep 07, 2018 at 09:15:30PM +0200, Clemens Goessnitzer wrote:
> > This patch adds 2 missing NULL pointer checks to rebound.c after malloc().
> 
> The same function also contains an unchecked calloc.

Since it's a daemon I guess it makes sense to continue execution instead
of dying. However, we should make sure to not leak memory along the
error paths. Also, log something when preloading the cache fails.

Regress passed; ok?

Index: rebound.c
===
RCS file: /cvs/src/usr.sbin/rebound/rebound.c,v
retrieving revision 1.98
diff -u -p -r1.98 rebound.c
--- rebound.c   1 May 2018 15:14:43 -   1.98
+++ rebound.c   8 Sep 2018 09:05:28 -
@@ -630,7 +630,7 @@ encodename(const char *name, unsigned ch
return totlen;
 }
 
-static void
+static int
 preloadcache(const char *name, uint16_t type, void *rdata, uint16_t rdatalen)
 {
struct dnspacket *req, *resp;
@@ -643,6 +643,8 @@ preloadcache(const char *name, uint16_t 
/* header + len + name + type + class */
reqlen = sizeof(*req) + 1 + strlen(name) + 2 + 2;
req = malloc(reqlen);
+   if (req == NULL)
+   return 1;
 
req->id = 0;
req->flags = htons(0x100);
@@ -661,6 +663,10 @@ preloadcache(const char *name, uint16_t 
/* req + name (compressed) + type + class + ttl + len + data */
resplen = reqlen + 2 + 2 + 2 + 4 + 2 + rdatalen;
resp = malloc(resplen);
+   if (resp == NULL) {
+   free(req);
+   return 1;
+   }
memcpy(resp, req, reqlen);
resp->flags = htons(0x100 | 0x8000);/* response */
resp->ancount = htons(1);
@@ -682,6 +688,11 @@ preloadcache(const char *name, uint16_t 
memcpy(p, rdata, rdatalen);
 
ent = calloc(1, sizeof(*ent));
+   if (ent == NULL) {
+   free(req);
+   free(resp);
+   return 1;
+   }
ent->req = req;
ent->reqlen = reqlen;
ent->resp = resp;
@@ -689,7 +700,7 @@ preloadcache(const char *name, uint16_t 
ent->permanent = 1;
 
RB_INSERT(cachetree, , ent);
-
+   return 0;
 }
 
 static void
@@ -699,7 +710,8 @@ preloadA(const char *name, const char *i
 
inet_aton(ip, );
 
-   preloadcache(name, htons(1), , 4);
+   if (preloadcache(name, htons(1), , 4))
+   logerr("failed to add cache entry for %s", name);
 }
 
 static void
@@ -715,7 +727,8 @@ preloadPTR(const char *ip, const char *n
 
encodename(name, namebuf);
 
-   preloadcache(ipbuf, htons(12), namebuf, 1 + strlen(namebuf));
+   if (preloadcache(ipbuf, htons(12), namebuf, 1 + strlen(namebuf)))
+   logerr("failed to add cache entry for %s", ip);
 }
 
 static int



kdump: recognize kcov ioctl commands

2018-08-29 Thread Anton Lindqvist
Hi,
Note that you must be running a fairly recent snapshot including the
sys/kcov.h header in order to compile.

Comments? OK?

diff --git Makefile Makefile
index e1918ae6a70..9c1416f12ef 100644
--- Makefile
+++ Makefile
@@ -39,6 +39,7 @@ ioctl.c: ${.CURDIR}/Makefile ${.CURDIR}/mkioctls
${SYS_DIR}/sys/dkio.h \
${SYS_DIR}/sys/filio.h \
${SYS_DIR}/sys/gpio.h \
+   ${SYS_DIR}/sys/kcov.h \
${SYS_DIR}/sys/memrange.h \
${SYS_DIR}/sys/mtio.h \
${SYS_DIR}/sys/pciio.h \
diff --git mkioctls mkioctls
index 21c66e6b334..1131f856c69 100644
--- mkioctls
+++ mkioctls
@@ -68,6 +68,7 @@ BEGIN {
print "#include "
print "#include "
print "#include "
+   print "#include "
print "#include "
print "#include "
print "#include "
@@ -98,7 +99,7 @@ BEGIN {
print ""
 }
 
-/^[]*#[]*define[   
]+(ATAIO|AUDIO_|BIOC|CDIO|CHIO|DIOC|DRM|GPIO|IPMICTL|TIO|FIO|MEMRANGE|MTIOC|LIOC|SIO|OSIO|SESIOC_|SIOC|PCIOC|PIPEX|PPPIOC|PPPOE|PVBUSIOC|RIOC|RND|STRIOC|SCIOC|OSCIOC|TUN|UDLIO|USB|VIDIOC|VNDIOC|VSCSI|WSKBD|WSMOUSE|WSDISPLAY|WSMUX|PTM)[A-Z_0-9]*[
   ]+(DRM)?_IO/ {
+/^[]*#[]*define[   
]+(ATAIO|AUDIO_|BIOC|CDIO|CHIO|DIOC|DRM|GPIO|IPMICTL|TIO|FIO|KIO|MEMRANGE|MTIOC|LIOC|SIO|OSIO|SESIOC_|SIOC|PCIOC|PIPEX|PPPIOC|PPPOE|PVBUSIOC|RIOC|RND|STRIOC|SCIOC|OSCIOC|TUN|UDLIO|USB|VIDIOC|VNDIOC|VSCSI|WSKBD|WSMOUSE|WSDISPLAY|WSMUX|PTM)[A-Z_0-9]*[
   ]+(DRM)?_IO/ {
 
# find where the name starts
for (i = 1; i <= NF; i++)



kcov: rename struct kd -> kcov_dev

2018-08-26 Thread Anton Lindqvist
Hi,
The terse named struct kd was initially fine since it wasn't used outside of
dev/kcov.c. Since struct proc now includes a pointer to such struct it
definitely deserves a more descriptive name, as reminded by visa@.

Comments? OK?

Index: dev/kcov.c
===
RCS file: /cvs/src/sys/dev/kcov.c,v
retrieving revision 1.3
diff -u -p -r1.3 kcov.c
--- dev/kcov.c  25 Aug 2018 15:38:07 -  1.3
+++ dev/kcov.c  25 Aug 2018 15:52:30 -
@@ -33,8 +33,7 @@
 #define DPRINTF(x...)
 #endif
 
-/* kcov descriptor */
-struct kd {
+struct kcov_dev {
enum {
KCOV_MODE_DISABLED,
KCOV_MODE_INIT,
@@ -46,18 +45,18 @@ struct kd {
size_t   kd_nmemb;
size_t   kd_size;
 
-   TAILQ_ENTRY(kd)  kd_entry;
+   TAILQ_ENTRY(kcov_dev)   kd_entry;
 };
 
 void kcovattach(int);
 
-int kd_alloc(struct kd *, unsigned long);
-void kd_free(struct kd *);
-struct kd *kd_lookup(int);
+int kd_alloc(struct kcov_dev *, unsigned long);
+void kd_free(struct kcov_dev *);
+struct kcov_dev *kd_lookup(int);
 
 static inline int inintr(void);
 
-TAILQ_HEAD(, kd) kd_list = TAILQ_HEAD_INITIALIZER(kd_list);
+TAILQ_HEAD(, kcov_dev) kd_list = TAILQ_HEAD_INITIALIZER(kd_list);
 
 #ifdef KCOV_DEBUG
 int kcov_debug = 1;
@@ -78,7 +77,7 @@ void
 __sanitizer_cov_trace_pc(void)
 {
extern int cold;
-   struct kd *kd;
+   struct kcov_dev *kd;
uint64_t idx;
 
/* Do not trace during boot. */
@@ -108,7 +107,7 @@ kcovattach(int count)
 int
 kcovopen(dev_t dev, int flag, int mode, struct proc *p)
 {
-   struct kd *kd;
+   struct kcov_dev *kd;
 
if (kd_lookup(minor(dev)) != NULL)
return (EBUSY);
@@ -124,7 +123,7 @@ kcovopen(dev_t dev, int flag, int mode, 
 int
 kcovclose(dev_t dev, int flag, int mode, struct proc *p)
 {
-   struct kd *kd;
+   struct kcov_dev *kd;
 
kd = kd_lookup(minor(dev));
if (kd == NULL)
@@ -143,7 +142,7 @@ kcovclose(dev_t dev, int flag, int mode,
 int
 kcovioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
 {
-   struct kd *kd;
+   struct kcov_dev *kd;
int error = 0;
 
kd = kd_lookup(minor(dev));
@@ -192,7 +191,7 @@ kcovioctl(dev_t dev, u_long cmd, caddr_t
 paddr_t
 kcovmmap(dev_t dev, off_t offset, int prot)
 {
-   struct kd *kd;
+   struct kcov_dev *kd;
paddr_t pa;
vaddr_t va;
 
@@ -212,7 +211,7 @@ kcovmmap(dev_t dev, off_t offset, int pr
 void
 kcov_exit(struct proc *p)
 {
-   struct kd *kd;
+   struct kcov_dev *kd;
 
kd = p->p_kd;
if (kd == NULL)
@@ -227,10 +226,10 @@ kcov_exit(struct proc *p)
p->p_kd = NULL;
 }
 
-struct kd *
+struct kcov_dev *
 kd_lookup(int unit)
 {
-   struct kd *kd;
+   struct kcov_dev *kd;
 
TAILQ_FOREACH(kd, _list, kd_entry) {
if (kd->kd_unit == unit)
@@ -240,7 +239,7 @@ kd_lookup(int unit)
 }
 
 int
-kd_alloc(struct kd *kd, unsigned long nmemb)
+kd_alloc(struct kcov_dev *kd, unsigned long nmemb)
 {
size_t size;
 
@@ -258,13 +257,13 @@ kd_alloc(struct kd *kd, unsigned long nm
 }
 
 void
-kd_free(struct kd *kd)
+kd_free(struct kcov_dev *kd)
 {
DPRINTF("%s: unit=%d mode=%d\n", __func__, kd->kd_unit, kd->kd_mode);
 
TAILQ_REMOVE(_list, kd, kd_entry);
free(kd->kd_buf, M_SUBPROC, kd->kd_size);
-   free(kd, M_SUBPROC, sizeof(struct kd));
+   free(kd, M_SUBPROC, sizeof(*kd));
 }
 
 static inline int
Index: sys/proc.h
===
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.257
diff -u -p -r1.257 proc.h
--- sys/proc.h  25 Aug 2018 15:38:07 -  1.257
+++ sys/proc.h  25 Aug 2018 15:52:30 -
@@ -288,7 +288,7 @@ struct process {
  "\024NOBROADCASTKILL" "\025PLEDGE" "\026WXNEEDED" "\027EXECPLEDGE" )
 
 
-struct kd;
+struct kcov_dev;
 struct lock_list_entry;
 
 struct proc {
@@ -376,7 +376,7 @@ struct proc {
 
struct  lock_list_entry *p_sleeplocks;
 
-   struct  kd *p_kd;   /* kcov descriptor */
+   struct  kcov_dev *p_kd;
 };
 
 /* Status values. */



Re: kcov: trace threads

2018-08-24 Thread Anton Lindqvist
On Thu, Aug 23, 2018 at 08:16:06AM +0200, Anton Lindqvist wrote:
> Hi.
> Currently kcov is enabled on a per process (pid) basis. A process with
> multiple threads therefore share the same coverage buffer which leads to
> non-deterministic results. Instead, kcov should be enabled on a per
> thread basis; just like how kcov behaves on Linux and FreeBSD. The
> decision to trace processes made development easier initially but I'd
> like to get this right now.
> 
> visa@ gave some helpful pointers since he did similar work related to
> witness and the p_sleeplocks member in struct proc. He also pointed out
> that conditionally (#ifdef) adding a member to struct proc is a bad idea
> since it's part of userland-visible ABI.
> 
> Another pleasant side-effect of this change is the removed linear search
> for the corresponding kcov descriptor inside __sanitizer_cov_trace_pc().
> 
> Survived a make release on amd64.
> 
> Comments? OK?

Both mpi@ and visa@ spotted a problem in kcovclose(): if one thread is
currently being traced and another one decides to close the /dev/kcov
file descriptor. Here's an updated diff in which this problem is solved
by delaying the freeing until kcov_exit() is called upon exit.

Index: dev/kcov.c
===
RCS file: /cvs/src/sys/dev/kcov.c,v
retrieving revision 1.2
diff -u -p -r1.2 kcov.c
--- dev/kcov.c  21 Aug 2018 18:06:12 -  1.2
+++ dev/kcov.c  24 Aug 2018 06:22:44 -
@@ -39,9 +39,9 @@ struct kd {
KCOV_MODE_DISABLED,
KCOV_MODE_INIT,
KCOV_MODE_TRACE_PC,
+   KCOV_MODE_DYING,
}kd_mode;
int  kd_unit;   /* device minor */
-   pid_tkd_pid;/* process being traced */
uintptr_t   *kd_buf;/* traced coverage */
size_t   kd_nmemb;
size_t   kd_size;
@@ -52,9 +52,9 @@ struct kd {
 void kcovattach(int);
 
 int kd_alloc(struct kd *, unsigned long);
+void kd_free(struct kd *);
 struct kd *kd_lookup(int);
 
-static inline struct kd *kd_lookup_pid(pid_t);
 static inline int inintr(void);
 
 TAILQ_HEAD(, kd) kd_list = TAILQ_HEAD_INITIALIZER(kd_list);
@@ -69,8 +69,8 @@ int kcov_debug = 1;
  * each block instructions that maps to a single line in the original source
  * code.
  *
- * If kcov is enabled for the current process, the executed address will be
- * stored in the corresponding coverage buffer.
+ * If kcov is enabled for the current thread, the kernel program counter will
+ * be stored in its corresponding coverage buffer.
  * The first element in the coverage buffer holds the index of next available
  * element.
  */
@@ -89,8 +89,8 @@ __sanitizer_cov_trace_pc(void)
if (inintr())
return;
 
-   kd = kd_lookup_pid(curproc->p_p->ps_pid);
-   if (kd == NULL)
+   kd = curproc->p_kd;
+   if (kd == NULL || kd->kd_mode != KCOV_MODE_TRACE_PC)
return;
 
idx = kd->kd_buf[0];
@@ -132,9 +132,11 @@ kcovclose(dev_t dev, int flag, int mode,
 
DPRINTF("%s: unit=%d\n", __func__, minor(dev));
 
-   TAILQ_REMOVE(_list, kd, kd_entry);
-   free(kd->kd_buf, M_SUBPROC, kd->kd_size);
-   free(kd, M_SUBPROC, sizeof(struct kd));
+   if (kd->kd_mode == KCOV_MODE_TRACE_PC)
+   kd->kd_mode = KCOV_MODE_DYING;
+   else
+   kd_free(kd);
+
return (0);
 }
 
@@ -159,30 +161,30 @@ kcovioctl(dev_t dev, u_long cmd, caddr_t
kd->kd_mode = KCOV_MODE_INIT;
break;
case KIOENABLE:
-   if (kd->kd_mode != KCOV_MODE_INIT) {
+   /* Only one kcov descriptor can be enabled per thread. */
+   if (p->p_kd != NULL || kd->kd_mode != KCOV_MODE_INIT) {
error = EBUSY;
break;
}
kd->kd_mode = KCOV_MODE_TRACE_PC;
-   kd->kd_pid = p->p_p->ps_pid;
+   p->p_kd = kd;
break;
case KIODISABLE:
-   /* Only the enabled process may disable itself. */
-   if (kd->kd_pid != p->p_p->ps_pid ||
-   kd->kd_mode != KCOV_MODE_TRACE_PC) {
+   /* Only the enabled thread may disable itself. */
+   if (p->p_kd != kd || kd->kd_mode != KCOV_MODE_TRACE_PC) {
error = EBUSY;
break;
}
kd->kd_mode = KCOV_MODE_INIT;
-   kd->kd_pid = 0;
+   p->p_kd = NULL;
break;
default:
error = EINVAL;
DPRINTF("%s: %lu: unknown command\n", __func__, cmd);
}
 
-   DPRINTF("%s: unit=%d, mode=%d, pid=%d, error=%d\n",
-

kcov: trace threads

2018-08-23 Thread Anton Lindqvist
Hi.
Currently kcov is enabled on a per process (pid) basis. A process with
multiple threads therefore share the same coverage buffer which leads to
non-deterministic results. Instead, kcov should be enabled on a per
thread basis; just like how kcov behaves on Linux and FreeBSD. The
decision to trace processes made development easier initially but I'd
like to get this right now.

visa@ gave some helpful pointers since he did similar work related to
witness and the p_sleeplocks member in struct proc. He also pointed out
that conditionally (#ifdef) adding a member to struct proc is a bad idea
since it's part of userland-visible ABI.

Another pleasant side-effect of this change is the removed linear search
for the corresponding kcov descriptor inside __sanitizer_cov_trace_pc().

Survived a make release on amd64.

Comments? OK?

Index: dev/kcov.c
===
RCS file: /cvs/src/sys/dev/kcov.c,v
retrieving revision 1.2
diff -u -p -r1.2 kcov.c
--- dev/kcov.c  21 Aug 2018 18:06:12 -  1.2
+++ dev/kcov.c  23 Aug 2018 06:05:58 -
@@ -41,7 +41,6 @@ struct kd {
KCOV_MODE_TRACE_PC,
}kd_mode;
int  kd_unit;   /* device minor */
-   pid_tkd_pid;/* process being traced */
uintptr_t   *kd_buf;/* traced coverage */
size_t   kd_nmemb;
size_t   kd_size;
@@ -54,7 +53,6 @@ void kcovattach(int);
 int kd_alloc(struct kd *, unsigned long);
 struct kd *kd_lookup(int);
 
-static inline struct kd *kd_lookup_pid(pid_t);
 static inline int inintr(void);
 
 TAILQ_HEAD(, kd) kd_list = TAILQ_HEAD_INITIALIZER(kd_list);
@@ -69,8 +67,8 @@ int kcov_debug = 1;
  * each block instructions that maps to a single line in the original source
  * code.
  *
- * If kcov is enabled for the current process, the executed address will be
- * stored in the corresponding coverage buffer.
+ * If kcov is enabled for the current thread, the kernel program counter will
+ * be stored in its corresponding coverage buffer.
  * The first element in the coverage buffer holds the index of next available
  * element.
  */
@@ -89,8 +87,8 @@ __sanitizer_cov_trace_pc(void)
if (inintr())
return;
 
-   kd = kd_lookup_pid(curproc->p_p->ps_pid);
-   if (kd == NULL)
+   kd = curproc->p_kd;
+   if (kd == NULL || kd->kd_mode != KCOV_MODE_TRACE_PC)
return;
 
idx = kd->kd_buf[0];
@@ -132,6 +130,9 @@ kcovclose(dev_t dev, int flag, int mode,
 
DPRINTF("%s: unit=%d\n", __func__, minor(dev));
 
+   if (kd == p->p_kd)
+   p->p_kd = NULL;
+
TAILQ_REMOVE(_list, kd, kd_entry);
free(kd->kd_buf, M_SUBPROC, kd->kd_size);
free(kd, M_SUBPROC, sizeof(struct kd));
@@ -159,30 +160,30 @@ kcovioctl(dev_t dev, u_long cmd, caddr_t
kd->kd_mode = KCOV_MODE_INIT;
break;
case KIOENABLE:
-   if (kd->kd_mode != KCOV_MODE_INIT) {
+   /* Only one kcov descriptor can be enabled per thread. */
+   if (p->p_kd != NULL || kd->kd_mode != KCOV_MODE_INIT) {
error = EBUSY;
break;
}
kd->kd_mode = KCOV_MODE_TRACE_PC;
-   kd->kd_pid = p->p_p->ps_pid;
+   p->p_kd = kd;
break;
case KIODISABLE:
-   /* Only the enabled process may disable itself. */
-   if (kd->kd_pid != p->p_p->ps_pid ||
-   kd->kd_mode != KCOV_MODE_TRACE_PC) {
+   /* Only the enabled thread may disable itself. */
+   if (p->p_kd != kd || kd->kd_mode != KCOV_MODE_TRACE_PC) {
error = EBUSY;
break;
}
kd->kd_mode = KCOV_MODE_INIT;
-   kd->kd_pid = 0;
+   p->p_kd = NULL;
break;
default:
error = EINVAL;
DPRINTF("%s: %lu: unknown command\n", __func__, cmd);
}
 
-   DPRINTF("%s: unit=%d, mode=%d, pid=%d, error=%d\n",
-   __func__, kd->kd_unit, kd->kd_mode, kd->kd_pid, error);
+   DPRINTF("%s: unit=%d, mode=%d, error=%d\n",
+   __func__, kd->kd_unit, kd->kd_mode, error);
 
return (error);
 }
@@ -212,12 +213,14 @@ kcov_exit(struct proc *p)
 {
struct kd *kd;
 
-   kd = kd_lookup_pid(p->p_p->ps_pid);
+   kd = p->p_kd;
if (kd == NULL)
return;
 
+   DPRINTF("%s: unit=%d\n", __func__, kd->kd_unit);
+
kd->kd_mode = KCOV_MODE_INIT;
-   kd->kd_pid = 0;
+   p->p_kd = NULL;
 }
 
 struct kd *
@@ -248,18 +251,6 @@ kd_alloc(struct kd *kd, unsigned long nm
kd->kd_nmemb = nmemb - 1;
kd->kd_size = size;
return (0);
-}
-
-static inline struct kd *
-kd_lookup_pid(pid_t pid)
-{
-   struct kd *kd;
-
-   

ffs_write: include vnode type in panic message

2018-07-21 Thread Anton Lindqvist
Hi,
A minor correction, include the vnode type just like ffs_read() does
when it encounters an unsupported type and panics.

Comments? OK?

Index: ffs_vnops.c
===
RCS file: /cvs/src/sys/ufs/ffs/ffs_vnops.c,v
retrieving revision 1.91
diff -u -p -r1.91 ffs_vnops.c
--- ffs_vnops.c 2 Jul 2018 20:56:22 -   1.91
+++ ffs_vnops.c 11 Jul 2018 05:20:27 -
@@ -315,7 +315,7 @@ ffs_write(void *v)
panic("ffs_write: nonsync dir write");
break;
default:
-   panic("ffs_write: type");
+   panic("ffs_write: type %d", vp->v_type);
}
 
fs = ip->i_fs;



vmctl: improve error message

2018-07-03 Thread Anton Lindqvist
Hi,
Stopping a VM owned by root as a non-root user fails with the following
error message:

  $ vmctl stop test
  vmctl: terminate vm command failed: Invalid argument

I think favoring warnc() with the appropriate errno number passed along
improves things. This of course is under the assumption that error codes
internal to vmd and errno:s are disjoint.

  $ ./obj/vmctl stop test 
  vmctl: terminate vm command failed: Operation not permitted

Comments? OK?

Index: vmctl.c
===
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.c,v
retrieving revision 1.49
diff -u -p -r1.49 vmctl.c
--- vmctl.c 19 Jun 2018 17:13:50 -  1.49
+++ vmctl.c 3 Jul 2018 08:46:25 -
@@ -465,7 +465,7 @@ terminate_vm_complete(struct imsg *imsg,
*ret = EIO;
break;
default:
-   warn("terminate vm command failed");
+   warnc(res, "terminate vm command failed");
*ret = EIO;
}
} else {



Re: kqueue: move members from filedesc to kqueue

2018-06-15 Thread Anton Lindqvist
On Fri, Jun 15, 2018 at 10:22:42AM +0200, Mark Kettenis wrote:
> > Date: Fri, 15 Jun 2018 09:59:47 +0200
> > From: Anton Lindqvist 
> > 
> > On Tue, Jun 12, 2018 at 09:00:14PM +0200, Anton Lindqvist wrote:
> > > Hi,
> > > This diff moves the kqueue related members from struct filedesc to
> > > struct kqueue with the prime motivation of fixing a panic in
> > > knote_processexit() that can occur when the filedesc is gone.
> > > Since filedesc is no longer shared between kqueues belong to the same
> > > process, I added a list of the current kqueues to struct process. This
> > > list is used in knote_closefd() in order to remove all knotes using the
> > > given fd as its ident from all kqueues belonging to the given process.
> > > 
> > > Similiar work has been done in both FreeBSD[1] and DragonFlyBSD[2].
> > > 
> > > Testing would be much appreciated. Not asking for anything particular,
> > > just apply the diff and exercise the kernel with your daily tasks.
> > > 
> > > [1] 
> > > https://github.com/freebsd/freebsd/commit/bc1805c6e871c178d0b6516c3baa774ffd77224a
> > > [2] 
> > > http://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/ccafe911a3aa55fd5262850ecfc5765cd31a56a2
> > 
> > Me and tb@ have been running this diff without problems. It also
> > survived a `make release' and upgrade. Already got an ok from visa@,
> > anyone else that can comment on the code and hopefully give another ok?
> 
> The kqueue regression tests still pass isn't it?

Correct

> 
> ok kettenis@
> 
> > > Index: kern/kern_descrip.c
> > > ===
> > > RCS file: /cvs/src/sys/kern/kern_descrip.c,v
> > > retrieving revision 1.164
> > > diff -u -p -r1.164 kern_descrip.c
> > > --- kern/kern_descrip.c   5 Jun 2018 09:29:05 -   1.164
> > > +++ kern/kern_descrip.c   12 Jun 2018 17:11:44 -
> > > @@ -650,8 +650,7 @@ finishdup(struct proc *p, struct file *f
> > >   *retval = new;
> > >  
> > >   if (oldfp != NULL) {
> > > - if (new < fdp->fd_knlistsize)
> > > - knote_fdclose(p, new);
> > > + knote_fdclose(p, new);
> > >   closef(oldfp, p);
> > >   }
> > >  
> > > @@ -686,8 +685,7 @@ fdrelease(struct proc *p, int fd)
> > >   FREF(fp);
> > >   *fpp = NULL;
> > >   fd_unused(fdp, fd);
> > > - if (fd < fdp->fd_knlistsize)
> > > - knote_fdclose(p, fd);
> > > + knote_fdclose(p, fd);
> > >   return (closef(fp, p));
> > >  }
> > >  
> > > @@ -999,7 +997,6 @@ fdinit(void)
> > >   newfdp->fd_fd.fd_nfiles = NDFILE;
> > >   newfdp->fd_fd.fd_himap = newfdp->fd_dhimap;
> > >   newfdp->fd_fd.fd_lomap = newfdp->fd_dlomap;
> > > - newfdp->fd_fd.fd_knlistsize = -1;
> > >  
> > >   newfdp->fd_fd.fd_freefile = 0;
> > >   newfdp->fd_fd.fd_lastfile = 0;
> > > @@ -1129,8 +1126,6 @@ fdfree(struct proc *p)
> > >   vrele(fdp->fd_cdir);
> > >   if (fdp->fd_rdir)
> > >   vrele(fdp->fd_rdir);
> > > - free(fdp->fd_knlist, M_TEMP, fdp->fd_knlistsize * sizeof(struct klist));
> > > - hashfree(fdp->fd_knhash, KN_HASHSIZE, M_TEMP);
> > >   pool_put(_pool, fdp);
> > >  }
> > >  
> > > Index: kern/kern_event.c
> > > ===
> > > RCS file: /cvs/src/sys/kern/kern_event.c,v
> > > retrieving revision 1.91
> > > diff -u -p -r1.91 kern_event.c
> > > --- kern/kern_event.c 5 Jun 2018 09:29:05 -   1.91
> > > +++ kern/kern_event.c 12 Jun 2018 17:11:44 -
> > > @@ -80,8 +80,8 @@ struct fileops kqueueops = {
> > >   .fo_close   = kqueue_close
> > >  };
> > >  
> > > -void knote_attach(struct knote *kn, struct filedesc *fdp);
> > > -void knote_drop(struct knote *kn, struct proc *p, struct filedesc 
> > > *fdp);
> > > +void knote_attach(struct knote *kn);
> > > +void knote_drop(struct knote *kn, struct proc *p);
> > >  void knote_enqueue(struct knote *kn);
> > >  void knote_dequeue(struct knote *kn);
> > >  #define knote_alloc() ((struct knote *)pool_get(_pool, PR_WAITOK))
> > > @@ -152,9 +152,13 @@ KQREF(struct kqueue *kq)
> > >  void
> > >  KQRELE(struct kqueue *kq)
> > >  {
> >

Re: kqueue: move members from filedesc to kqueue

2018-06-15 Thread Anton Lindqvist
On Tue, Jun 12, 2018 at 09:00:14PM +0200, Anton Lindqvist wrote:
> Hi,
> This diff moves the kqueue related members from struct filedesc to
> struct kqueue with the prime motivation of fixing a panic in
> knote_processexit() that can occur when the filedesc is gone.
> Since filedesc is no longer shared between kqueues belong to the same
> process, I added a list of the current kqueues to struct process. This
> list is used in knote_closefd() in order to remove all knotes using the
> given fd as its ident from all kqueues belonging to the given process.
> 
> Similiar work has been done in both FreeBSD[1] and DragonFlyBSD[2].
> 
> Testing would be much appreciated. Not asking for anything particular,
> just apply the diff and exercise the kernel with your daily tasks.
> 
> [1] 
> https://github.com/freebsd/freebsd/commit/bc1805c6e871c178d0b6516c3baa774ffd77224a
> [2] 
> http://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/ccafe911a3aa55fd5262850ecfc5765cd31a56a2

Me and tb@ have been running this diff without problems. It also
survived a `make release' and upgrade. Already got an ok from visa@,
anyone else that can comment on the code and hopefully give another ok?

> 
> Index: kern/kern_descrip.c
> ===
> RCS file: /cvs/src/sys/kern/kern_descrip.c,v
> retrieving revision 1.164
> diff -u -p -r1.164 kern_descrip.c
> --- kern/kern_descrip.c   5 Jun 2018 09:29:05 -   1.164
> +++ kern/kern_descrip.c   12 Jun 2018 17:11:44 -
> @@ -650,8 +650,7 @@ finishdup(struct proc *p, struct file *f
>   *retval = new;
>  
>   if (oldfp != NULL) {
> - if (new < fdp->fd_knlistsize)
> - knote_fdclose(p, new);
> + knote_fdclose(p, new);
>   closef(oldfp, p);
>   }
>  
> @@ -686,8 +685,7 @@ fdrelease(struct proc *p, int fd)
>   FREF(fp);
>   *fpp = NULL;
>   fd_unused(fdp, fd);
> - if (fd < fdp->fd_knlistsize)
> - knote_fdclose(p, fd);
> + knote_fdclose(p, fd);
>   return (closef(fp, p));
>  }
>  
> @@ -999,7 +997,6 @@ fdinit(void)
>   newfdp->fd_fd.fd_nfiles = NDFILE;
>   newfdp->fd_fd.fd_himap = newfdp->fd_dhimap;
>   newfdp->fd_fd.fd_lomap = newfdp->fd_dlomap;
> - newfdp->fd_fd.fd_knlistsize = -1;
>  
>   newfdp->fd_fd.fd_freefile = 0;
>   newfdp->fd_fd.fd_lastfile = 0;
> @@ -1129,8 +1126,6 @@ fdfree(struct proc *p)
>   vrele(fdp->fd_cdir);
>   if (fdp->fd_rdir)
>   vrele(fdp->fd_rdir);
> - free(fdp->fd_knlist, M_TEMP, fdp->fd_knlistsize * sizeof(struct klist));
> - hashfree(fdp->fd_knhash, KN_HASHSIZE, M_TEMP);
>   pool_put(_pool, fdp);
>  }
>  
> Index: kern/kern_event.c
> ===
> RCS file: /cvs/src/sys/kern/kern_event.c,v
> retrieving revision 1.91
> diff -u -p -r1.91 kern_event.c
> --- kern/kern_event.c 5 Jun 2018 09:29:05 -   1.91
> +++ kern/kern_event.c 12 Jun 2018 17:11:44 -
> @@ -80,8 +80,8 @@ struct fileops kqueueops = {
>   .fo_close   = kqueue_close
>  };
>  
> -void knote_attach(struct knote *kn, struct filedesc *fdp);
> -void knote_drop(struct knote *kn, struct proc *p, struct filedesc *fdp);
> +void knote_attach(struct knote *kn);
> +void knote_drop(struct knote *kn, struct proc *p);
>  void knote_enqueue(struct knote *kn);
>  void knote_dequeue(struct knote *kn);
>  #define knote_alloc() ((struct knote *)pool_get(_pool, PR_WAITOK))
> @@ -152,9 +152,13 @@ KQREF(struct kqueue *kq)
>  void
>  KQRELE(struct kqueue *kq)
>  {
> - if (--kq->kq_refs == 0) {
> - pool_put(_pool, kq);
> - }
> + if (--kq->kq_refs > 0)
> + return;
> +
> + LIST_REMOVE(kq, kq_next);
> + free(kq->kq_knlist, M_TEMP, kq->kq_knlistsize * sizeof(struct klist));
> + hashfree(kq->kq_knhash, KN_HASHSIZE, M_TEMP);
> + pool_put(_pool, kq);
>  }
>  
>  void kqueue_init(void);
> @@ -453,10 +457,9 @@ sys_kqueue(struct proc *p, void *v, regi
>   fp->f_data = kq;
>   KQREF(kq);
>   *retval = fd;
> - if (fdp->fd_knlistsize < 0)
> - fdp->fd_knlistsize = 0; /* this process has a kq */
>   kq->kq_fdp = fdp;
>   FILE_SET_MATURE(fp, p);
> + LIST_INSERT_HEAD(>p_p->ps_kqlist, kq, kq_next);
>   return (0);
>  }
>  
> @@ -583,19 +586,19 @@ kqueue_register(struct kqueue *kq, struc
>   if ((fp = fd_getfile(fdp, kev->ident)) == NULL)
>   return (EBADF);
&

kqueue: move members from filedesc to kqueue

2018-06-12 Thread Anton Lindqvist
Hi,
This diff moves the kqueue related members from struct filedesc to
struct kqueue with the prime motivation of fixing a panic in
knote_processexit() that can occur when the filedesc is gone.
Since filedesc is no longer shared between kqueues belong to the same
process, I added a list of the current kqueues to struct process. This
list is used in knote_closefd() in order to remove all knotes using the
given fd as its ident from all kqueues belonging to the given process.

Similiar work has been done in both FreeBSD[1] and DragonFlyBSD[2].

Testing would be much appreciated. Not asking for anything particular,
just apply the diff and exercise the kernel with your daily tasks.

[1] 
https://github.com/freebsd/freebsd/commit/bc1805c6e871c178d0b6516c3baa774ffd77224a
[2] 
http://gitweb.dragonflybsd.org/dragonfly.git/commitdiff/ccafe911a3aa55fd5262850ecfc5765cd31a56a2

Index: kern/kern_descrip.c
===
RCS file: /cvs/src/sys/kern/kern_descrip.c,v
retrieving revision 1.164
diff -u -p -r1.164 kern_descrip.c
--- kern/kern_descrip.c 5 Jun 2018 09:29:05 -   1.164
+++ kern/kern_descrip.c 12 Jun 2018 17:11:44 -
@@ -650,8 +650,7 @@ finishdup(struct proc *p, struct file *f
*retval = new;
 
if (oldfp != NULL) {
-   if (new < fdp->fd_knlistsize)
-   knote_fdclose(p, new);
+   knote_fdclose(p, new);
closef(oldfp, p);
}
 
@@ -686,8 +685,7 @@ fdrelease(struct proc *p, int fd)
FREF(fp);
*fpp = NULL;
fd_unused(fdp, fd);
-   if (fd < fdp->fd_knlistsize)
-   knote_fdclose(p, fd);
+   knote_fdclose(p, fd);
return (closef(fp, p));
 }
 
@@ -999,7 +997,6 @@ fdinit(void)
newfdp->fd_fd.fd_nfiles = NDFILE;
newfdp->fd_fd.fd_himap = newfdp->fd_dhimap;
newfdp->fd_fd.fd_lomap = newfdp->fd_dlomap;
-   newfdp->fd_fd.fd_knlistsize = -1;
 
newfdp->fd_fd.fd_freefile = 0;
newfdp->fd_fd.fd_lastfile = 0;
@@ -1129,8 +1126,6 @@ fdfree(struct proc *p)
vrele(fdp->fd_cdir);
if (fdp->fd_rdir)
vrele(fdp->fd_rdir);
-   free(fdp->fd_knlist, M_TEMP, fdp->fd_knlistsize * sizeof(struct klist));
-   hashfree(fdp->fd_knhash, KN_HASHSIZE, M_TEMP);
pool_put(_pool, fdp);
 }
 
Index: kern/kern_event.c
===
RCS file: /cvs/src/sys/kern/kern_event.c,v
retrieving revision 1.91
diff -u -p -r1.91 kern_event.c
--- kern/kern_event.c   5 Jun 2018 09:29:05 -   1.91
+++ kern/kern_event.c   12 Jun 2018 17:11:44 -
@@ -80,8 +80,8 @@ struct fileops kqueueops = {
.fo_close   = kqueue_close
 };
 
-void   knote_attach(struct knote *kn, struct filedesc *fdp);
-void   knote_drop(struct knote *kn, struct proc *p, struct filedesc *fdp);
+void   knote_attach(struct knote *kn);
+void   knote_drop(struct knote *kn, struct proc *p);
 void   knote_enqueue(struct knote *kn);
 void   knote_dequeue(struct knote *kn);
 #define knote_alloc() ((struct knote *)pool_get(_pool, PR_WAITOK))
@@ -152,9 +152,13 @@ KQREF(struct kqueue *kq)
 void
 KQRELE(struct kqueue *kq)
 {
-   if (--kq->kq_refs == 0) {
-   pool_put(_pool, kq);
-   }
+   if (--kq->kq_refs > 0)
+   return;
+
+   LIST_REMOVE(kq, kq_next);
+   free(kq->kq_knlist, M_TEMP, kq->kq_knlistsize * sizeof(struct klist));
+   hashfree(kq->kq_knhash, KN_HASHSIZE, M_TEMP);
+   pool_put(_pool, kq);
 }
 
 void kqueue_init(void);
@@ -453,10 +457,9 @@ sys_kqueue(struct proc *p, void *v, regi
fp->f_data = kq;
KQREF(kq);
*retval = fd;
-   if (fdp->fd_knlistsize < 0)
-   fdp->fd_knlistsize = 0; /* this process has a kq */
kq->kq_fdp = fdp;
FILE_SET_MATURE(fp, p);
+   LIST_INSERT_HEAD(>p_p->ps_kqlist, kq, kq_next);
return (0);
 }
 
@@ -583,19 +586,19 @@ kqueue_register(struct kqueue *kq, struc
if ((fp = fd_getfile(fdp, kev->ident)) == NULL)
return (EBADF);
 
-   if (kev->ident < fdp->fd_knlistsize) {
-   SLIST_FOREACH(kn, >fd_knlist[kev->ident], kn_link) 
{
+   if (kev->ident < kq->kq_knlistsize) {
+   SLIST_FOREACH(kn, >kq_knlist[kev->ident], kn_link) {
if (kq == kn->kn_kq &&
kev->filter == kn->kn_filter)
break;
}
}
} else {
-   if (fdp->fd_knhashmask != 0) {
+   if (kq->kq_knhashmask != 0) {
struct klist *list;
 
-   list = >fd_knhash[
-   KN_HASH((u_long)kev->ident, fdp->fd_knhashmask)];
+   list = >kq_knhash[
+   KN_HASH((u_long)kev->ident, 

Re: hashinit(): power of 2 fast path

2018-04-29 Thread Anton Lindqvist
On Sun, Apr 29, 2018 at 10:52:27AM +0200, Mathieu - wrote:
> Disregard that, brainfart on my side.
>
> Mathieu - wrote:
> > Anton Lindqvist wrote:
> > > Hi,
> > > If the elements argument passed to hashinit() is a power of 2 there's no
> > > need to find the closest power of 2 that can fit all elements since
> > > elements == hashsize will always be true. During boot of a stock amd64
> > > kernel running inside vmd 80% of the calls to hashinit() includes a
> > > power of 2 size.
> > > 
> > > Comments? OK?
> > 
> > 
> > Hi,
> > 
> > Dunno how much a win it is. But anyhow this will blow in hashfree as
> > hashsize won't be the same.
> > 
> > 
> > Mathieu.
> > 
> > 
> > 
> > > 
> > > Index: kern/kern_subr.c
> > > ===
> > > RCS file: /cvs/src/sys/kern/kern_subr.c,v
> > > retrieving revision 1.49
> > > diff -u -p -r1.49 kern_subr.c
> > > --- kern/kern_subr.c  14 Feb 2017 10:31:15 -  1.49
> > > +++ kern/kern_subr.c  28 Apr 2018 20:21:40 -
> > > @@ -163,8 +163,11 @@ hashinit(int elements, int type, int fla
> > >  
> > >   if (elements <= 0)
> > >   panic("hashinit: bad cnt");
> > > - for (hashsize = 1; hashsize < elements; hashsize <<= 1)
> > > - continue;
> > > + if ((elements & (elements - 1)) == 0)
> > > + hashsize = elements;
> > > + else
> > > + for (hashsize = 1; hashsize < elements; hashsize <<= 1)
> > > + continue;
> > >   hashtbl = mallocarray(hashsize, sizeof(*hashtbl), type, flags);
> > >   if (hashtbl == NULL)
> > >   return NULL;
> > > 
> > 

But the same logic could also be applied in hashfree() with the same
reasoning as in hashinit() still being valid.

OK?

Index: kern/kern_subr.c
===
RCS file: /cvs/src/sys/kern/kern_subr.c,v
retrieving revision 1.49
diff -u -p -r1.49 kern_subr.c
--- kern/kern_subr.c14 Feb 2017 10:31:15 -  1.49
+++ kern/kern_subr.c29 Apr 2018 09:21:26 -
@@ -163,8 +163,11 @@ hashinit(int elements, int type, int fla
 
if (elements <= 0)
panic("hashinit: bad cnt");
-   for (hashsize = 1; hashsize < elements; hashsize <<= 1)
-   continue;
+   if ((elements & (elements - 1)) == 0)
+   hashsize = elements;
+   else
+   for (hashsize = 1; hashsize < elements; hashsize <<= 1)
+   continue;
hashtbl = mallocarray(hashsize, sizeof(*hashtbl), type, flags);
if (hashtbl == NULL)
return NULL;
@@ -182,8 +185,11 @@ hashfree(void *hash, int elements, int t
 
if (elements <= 0)
panic("hashfree: bad cnt");
-   for (hashsize = 1; hashsize < elements; hashsize <<= 1)
-   continue;
+   if ((elements & (elements - 1)) == 0)
+   hashsize = elements;
+   else
+   for (hashsize = 1; hashsize < elements; hashsize <<= 1)
+   continue;
 
free(hashtbl, type, sizeof(*hashtbl) * hashsize);
 }



hashinit(): power of 2 fast path

2018-04-28 Thread Anton Lindqvist
Hi,
If the elements argument passed to hashinit() is a power of 2 there's no
need to find the closest power of 2 that can fit all elements since
elements == hashsize will always be true. During boot of a stock amd64
kernel running inside vmd 80% of the calls to hashinit() includes a
power of 2 size.

Comments? OK?

Index: kern/kern_subr.c
===
RCS file: /cvs/src/sys/kern/kern_subr.c,v
retrieving revision 1.49
diff -u -p -r1.49 kern_subr.c
--- kern/kern_subr.c14 Feb 2017 10:31:15 -  1.49
+++ kern/kern_subr.c28 Apr 2018 20:21:40 -
@@ -163,8 +163,11 @@ hashinit(int elements, int type, int fla
 
if (elements <= 0)
panic("hashinit: bad cnt");
-   for (hashsize = 1; hashsize < elements; hashsize <<= 1)
-   continue;
+   if ((elements & (elements - 1)) == 0)
+   hashsize = elements;
+   else
+   for (hashsize = 1; hashsize < elements; hashsize <<= 1)
+   continue;
hashtbl = mallocarray(hashsize, sizeof(*hashtbl), type, flags);
if (hashtbl == NULL)
return NULL;



patch(1): remove unused header

2018-04-07 Thread Anton Lindqvist
Hi,
Since patch(1) no longer invokes ed(1), pathnames.h can be removed.
_PATH_TMP is still used inside patch.c but including paths.h is
sufficient.

Comments? OK?

diff --git usr.bin/patch/patch.c usr.bin/patch/patch.c
index fef7df6466e..af142599340 100644
--- usr.bin/patch/patch.c
+++ usr.bin/patch/patch.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -42,7 +43,6 @@
 #include "pch.h"
 #include "inp.h"
 #include "backupfile.h"
-#include "pathnames.h"
 #include "ed.h"
 
 mode_t filemode = 0644;
diff --git usr.bin/patch/pathnames.h usr.bin/patch/pathnames.h
deleted file mode 100644
index 397e3fabe37..000
--- usr.bin/patch/pathnames.h
+++ /dev/null
@@ -1,11 +0,0 @@
-/* $OpenBSD: pathnames.h,v 1.1 2003/07/29 20:10:17 millert Exp $   */
-
-/*
- * Placed in the public domain by Todd C. Miller 
- * on July 29, 2003.
- */
-
-#include 
-
-#define_PATH_ED"/bin/ed"
-#define_PATH_MKDIR "/bin/mkdir"
diff --git usr.bin/patch/pch.c usr.bin/patch/pch.c
index 539754d814b..ac0645fe434 100644
--- usr.bin/patch/pch.c
+++ usr.bin/patch/pch.c
@@ -41,7 +41,6 @@
 #include "common.h"
 #include "util.h"
 #include "pch.h"
-#include "pathnames.h"
 
 /* Patch (diff listing) abstract type. */
 
diff --git usr.bin/patch/util.c usr.bin/patch/util.c
index f9d47906300..2ab5f1f0e28 100644
--- usr.bin/patch/util.c
+++ usr.bin/patch/util.c
@@ -43,7 +43,6 @@
 #include "common.h"
 #include "util.h"
 #include "backupfile.h"
-#include "pathnames.h"
 
 /* Rename a file, copying it if necessary. */



Re: What does the struct xen_softc represent?

2018-03-25 Thread Anton Lindqvist
On Mon, Mar 26, 2018 at 12:17:22AM +0530, Pratyush Yadav wrote:
> Hi everyone,
> 
> I was looking at the Xen drivers in sys/dev/pv/xen.c and I noticed a struct
> very frequently used, the xen_softc struct. I am new to Xen and kernel
> programming in general and couldn't understand the use of this and what it
> represents. I looked at its declaration in sys/dev/pv/xenvar.h, but it does
> not contain any comments explaining what it represents.
> 
> Can someone explain what it means and what it is used for?

I would start with excellent presentation by stsp@ entitled "Getting
started with OpenBSD device driver development" and especially page 22.

https://www.openbsd.org/papers/eurobsdcon2017-device-drivers.pdf



Re: ksh: __func__ in warnings

2018-03-15 Thread Anton Lindqvist
On Thu, Mar 15, 2018 at 08:37:06AM +0100, Anton Lindqvist wrote:
> On Tue, Mar 13, 2018 at 04:39:16PM +0800, Michael W. Bombardieri wrote:
> > Hello,
> > 
> > Some errors and warnings printed by ksh have the function name
> > prefixed. __func__ could be used here instead of hard-coding
> > the name. The names are wrong for tty_init(), j_set_async(),
> > j_change(), x_file_glob() and c_ulimit() afaics.
> > 
> > - Michael
> 
> Looks good, just some minor cosmetic nits where the format string in
> some cases now fits on the previous line.
> 
> Anyone else willing to OK?

This has now been committed, thanks!

> > Index: c_ksh.c
> > ===
> > RCS file: /cvs/src/bin/ksh/c_ksh.c,v
> > retrieving revision 1.58
> > diff -u -p -u -r1.58 c_ksh.c
> > --- c_ksh.c 16 Jan 2018 22:52:32 -  1.58
> > +++ c_ksh.c 13 Mar 2018 08:16:37 -
> > @@ -1273,7 +1273,7 @@ c_getopts(char **wp)
> > }
> >  
> > if (genv->loc->next == NULL) {
> > -   internal_warningf("c_getopts: no argv");
> > +   internal_warningf("%s: no argv", __func__);
> > return 1;
> > }
> > /* Which arguments are we parsing... */
> > Index: c_ulimit.c
> > ===
> > RCS file: /cvs/src/bin/ksh/c_ulimit.c,v
> > retrieving revision 1.26
> > diff -u -p -u -r1.26 c_ulimit.c
> > --- c_ulimit.c  16 Jan 2018 22:52:32 -  1.26
> > +++ c_ulimit.c  13 Mar 2018 08:16:37 -
> > @@ -97,7 +97,7 @@ c_ulimit(char **wp)
> > for (l = limits; l->name && l->option != optc; l++)
> > ;
> > if (!l->name) {
> > -   internal_warningf("ulimit: %c", optc);
> > +   internal_warningf("%s: %c", __func__, optc);
> > return 1;
> > }
> > if (builtin_opt.optarg) {
> > Index: edit.c
> > ===
> > RCS file: /cvs/src/bin/ksh/edit.c,v
> > retrieving revision 1.63
> > diff -u -p -u -r1.63 edit.c
> > --- edit.c  16 Jan 2018 22:52:32 -  1.63
> > +++ edit.c  13 Mar 2018 08:16:37 -
> > @@ -372,7 +372,7 @@ x_file_glob(int flags, const char *str, 
> > source = s;
> > if (yylex(ONEWORD|UNESCAPE) != LWORD) {
> > source = sold;
> > -   internal_warningf("fileglob: substitute error");
> > +   internal_warningf("%s: substitute error", __func__);
> > return 0;
> > }
> > source = sold;
> > Index: exec.c
> > ===
> > RCS file: /cvs/src/bin/ksh/exec.c,v
> > retrieving revision 1.72
> > diff -u -p -u -r1.72 exec.c
> > --- exec.c  16 Jan 2018 22:52:32 -  1.72
> > +++ exec.c  13 Mar 2018 08:16:37 -
> > @@ -727,7 +727,7 @@ shcomexec(char **wp)
> >  
> > tp = ktsearch(, *wp, hash(*wp));
> > if (tp == NULL)
> > -   internal_errorf("shcomexec: %s", *wp);
> > +   internal_errorf("%s: %s", __func__, *wp);
> > return call_builtin(tp, wp);
> >  }
> >  
> > @@ -1221,7 +1221,7 @@ herein(const char *content, int sub)
> > s->start = s->str = content;
> > source = s;
> > if (yylex(ONEWORD|HEREDOC) != LWORD)
> > -   internal_errorf("herein: yylex");
> > +   internal_errorf("%s: yylex", __func__);
> > source = osource;
> > shf_puts(evalstr(yylval.cp, 0), shf);
> > } else
> > @@ -1446,5 +1446,5 @@ static void
> >  dbteste_error(Test_env *te, int offset, const char *msg)
> >  {
> > te->flags |= TEF_ERROR;
> > -   internal_warningf("dbteste_error: %s (offset %d)", msg, offset);
> > +   internal_warningf("%s: %s (offset %d)", __func__, msg, offset);
> >  }
> > Index: jobs.c
> > ===
> > RCS file: /cvs/src/bin/ksh/jobs.c,v
> > retrieving revision 1.59
> > diff -u -p -u -r1.59 jobs.c
> > --- jobs.c  16 Jan 2018 22:52:32 -  1.59
> > +++ jobs.c  13 Mar 2018 08:16:38 -
> > @@ -200,13 +200,13 @@ j_suspend(void)
> >

Re: ksh: __func__ in warnings

2018-03-15 Thread Anton Lindqvist
On Tue, Mar 13, 2018 at 04:39:16PM +0800, Michael W. Bombardieri wrote:
> Hello,
> 
> Some errors and warnings printed by ksh have the function name
> prefixed. __func__ could be used here instead of hard-coding
> the name. The names are wrong for tty_init(), j_set_async(),
> j_change(), x_file_glob() and c_ulimit() afaics.
> 
> - Michael

Looks good, just some minor cosmetic nits where the format string in
some cases now fits on the previous line.

Anyone else willing to OK?

> 
> 
> Index: c_ksh.c
> ===
> RCS file: /cvs/src/bin/ksh/c_ksh.c,v
> retrieving revision 1.58
> diff -u -p -u -r1.58 c_ksh.c
> --- c_ksh.c   16 Jan 2018 22:52:32 -  1.58
> +++ c_ksh.c   13 Mar 2018 08:16:37 -
> @@ -1273,7 +1273,7 @@ c_getopts(char **wp)
>   }
>  
>   if (genv->loc->next == NULL) {
> - internal_warningf("c_getopts: no argv");
> + internal_warningf("%s: no argv", __func__);
>   return 1;
>   }
>   /* Which arguments are we parsing... */
> Index: c_ulimit.c
> ===
> RCS file: /cvs/src/bin/ksh/c_ulimit.c,v
> retrieving revision 1.26
> diff -u -p -u -r1.26 c_ulimit.c
> --- c_ulimit.c16 Jan 2018 22:52:32 -  1.26
> +++ c_ulimit.c13 Mar 2018 08:16:37 -
> @@ -97,7 +97,7 @@ c_ulimit(char **wp)
>   for (l = limits; l->name && l->option != optc; l++)
>   ;
>   if (!l->name) {
> - internal_warningf("ulimit: %c", optc);
> + internal_warningf("%s: %c", __func__, optc);
>   return 1;
>   }
>   if (builtin_opt.optarg) {
> Index: edit.c
> ===
> RCS file: /cvs/src/bin/ksh/edit.c,v
> retrieving revision 1.63
> diff -u -p -u -r1.63 edit.c
> --- edit.c16 Jan 2018 22:52:32 -  1.63
> +++ edit.c13 Mar 2018 08:16:37 -
> @@ -372,7 +372,7 @@ x_file_glob(int flags, const char *str, 
>   source = s;
>   if (yylex(ONEWORD|UNESCAPE) != LWORD) {
>   source = sold;
> - internal_warningf("fileglob: substitute error");
> + internal_warningf("%s: substitute error", __func__);
>   return 0;
>   }
>   source = sold;
> Index: exec.c
> ===
> RCS file: /cvs/src/bin/ksh/exec.c,v
> retrieving revision 1.72
> diff -u -p -u -r1.72 exec.c
> --- exec.c16 Jan 2018 22:52:32 -  1.72
> +++ exec.c13 Mar 2018 08:16:37 -
> @@ -727,7 +727,7 @@ shcomexec(char **wp)
>  
>   tp = ktsearch(, *wp, hash(*wp));
>   if (tp == NULL)
> - internal_errorf("shcomexec: %s", *wp);
> + internal_errorf("%s: %s", __func__, *wp);
>   return call_builtin(tp, wp);
>  }
>  
> @@ -1221,7 +1221,7 @@ herein(const char *content, int sub)
>   s->start = s->str = content;
>   source = s;
>   if (yylex(ONEWORD|HEREDOC) != LWORD)
> - internal_errorf("herein: yylex");
> + internal_errorf("%s: yylex", __func__);
>   source = osource;
>   shf_puts(evalstr(yylval.cp, 0), shf);
>   } else
> @@ -1446,5 +1446,5 @@ static void
>  dbteste_error(Test_env *te, int offset, const char *msg)
>  {
>   te->flags |= TEF_ERROR;
> - internal_warningf("dbteste_error: %s (offset %d)", msg, offset);
> + internal_warningf("%s: %s (offset %d)", __func__, msg, offset);
>  }
> Index: jobs.c
> ===
> RCS file: /cvs/src/bin/ksh/jobs.c,v
> retrieving revision 1.59
> diff -u -p -u -r1.59 jobs.c
> --- jobs.c16 Jan 2018 22:52:32 -  1.59
> +++ jobs.c13 Mar 2018 08:16:38 -
> @@ -200,13 +200,13 @@ j_suspend(void)
>   if (restore_ttypgrp >= 0) {
>   if (tcsetpgrp(tty_fd, restore_ttypgrp) < 0) {
>   warningf(false,
> - "j_suspend: tcsetpgrp() failed: %s",
> - strerror(errno));
> + "%s: tcsetpgrp() failed: %s",
> + __func__, strerror(errno));
>   } else {
>   if (setpgid(0, restore_ttypgrp) < 0) {
>   warningf(false,
> - "j_suspend: setpgid() failed: %s",
> - strerror(errno));
> + "%s: setpgid() failed: %s",
> + __func__, strerror(errno));
>   }
>   }
>   }
> @@ -225,14 +225,14 @@ 

Re: ksh: bug with quoted parameter expansion

2018-01-25 Thread Anton Lindqvist
On Wed, Jan 24, 2018 at 04:15:24PM +0100, m...@josuah.net wrote:
> I found a bug in ksh's parameter expansion on an edge case:
> 
>   true $(true "${USER#'"'}")
> 
>   /home/sh[4]: no closing quote
> 
> The problem seems to occurs when all of these conditions are present:
> 
> 1. On ${var#pattern} or ${var%pattern} parameter expansion
> 2. When the pattern contains a singly quoted double quote: '"'
> 3. While expansion occurs withing $(...), (but not `...`)
> 4. While the expansion is quoted: "${var#pattern}".
> 
> true can be replaced by other commands or var=...
> if '"' gets replaced by \", the issue disappear
> 
> The bin/ksh/lex.c seems to use a big switch+goto table with one label
> per grammar context.  Maybe by jumping from context to context in a
> specific pattern like above is producing the issue.
> 
> I did try to figure out where, but I lack time for now.
> 
> I hope the problem does not come from my way to test it and that it
> have not been fixed.
> 
> $ uname -a  # also tested with /bin/ksh
> OpenBSD t470s 6.2 GENERIC.MP#134 amd64
> 
> $ cvs co bin/ksh/; cd bin/ksh; make
> [...]
> 
> $ ./ksh ~/sh
> /home/sh[4]: no closing quote

For the record, probably related to this:

  $ pwd
  /usr/src/bin/ksh
  $ sed -n 474,479p lex.c
  case SCSPAREN: /* $( .. ) */
/* todo: deal with $(...) quoting properly
 * kludge to partly fake quoting inside $(..): doesn't
 * really work because nested $(..) or ${..} inside
 * double quotes aren't dealt with.
 */



Re: disabled code in ksh tree.c

2018-01-20 Thread Anton Lindqvist
On Tue, Jan 16, 2018 at 04:29:59PM +0100, Jeremie Courreges-Anglas wrote:
> On Mon, Jan 15 2018, "Michael W. Bombardieri" <m...@ii.net> wrote:
> > On Sun, Jan 14, 2018 at 05:47:43PM +0100, Jeremie Courreges-Anglas wrote:
> >> On Sun, Jan 14 2018, Anton Lindqvist <an...@openbsd.org> wrote:
> >> > On Thu, Jan 11, 2018 at 03:25:15PM +0800, Michael W. Bombardieri wrote:
> >> >> Hello,
> >> >> 
> >> >> Revision 1.9 of tree.c (from 1999) added the disabled code and it
> >> >> is still disabled. Would it be better to remove it?
> >> >
> >> > Fine with me. Anyone else willing OK?
> >> 
> >> I'd rather understand exactly what this code does and what the comment
> >> actually means.  The TEXEC case can definitely be reached.
> >
> > The TEXEC case in ptree() happens if fptreef() or snptreef() are called with
> > format specifier %T.
> > From what I see it is called like this:
> >
> > exchild() -> snptreef() -> vfptreef() -> ptree().
> >
> > Here snptreef() is used to copy the command string to be executed into 
> > a new Proc structure before exchild() forks and calls execute().
> >
> > When calling ptree() by running shell command "ls -l", t->type is first 
> > TEXEC,
> > then after the goto t->type is TCOM.
> > So the switch statement appears to happen twice.
> >
> > The comment "print original vars" does happen, but after the goto in TCOM 
> > case.
> > The if(t->vars) code in TCOM case is the same as the disabled 
> > if(t->left->vars)
> > code in TEXEC case.
> > Also, t->vars appears to be NULL in TEXEC case, but t->vars is set in TCOM 
> > case.
> > In terms of how the code is structured, vars belongs to TCOM not TEXEC.
> > So afaics the disabled code doesn't do anything useful.
> 
> Thanks for the explanation, ok jca@

This has now been committed, thanks!



Re: disabled code in ksh tree.c

2018-01-14 Thread Anton Lindqvist
On Thu, Jan 11, 2018 at 03:25:15PM +0800, Michael W. Bombardieri wrote:
> Hello,
> 
> Revision 1.9 of tree.c (from 1999) added the disabled code and it
> is still disabled. Would it be better to remove it?

Fine with me. Anyone else willing OK?

> - Michael
> 
> 
> Index: tree.c
> ===
> RCS file: /cvs/src/bin/ksh/tree.c,v
> retrieving revision 1.30
> diff -u -p -u -r1.30 tree.c
> --- tree.c6 Jan 2018 16:28:58 -   1.30
> +++ tree.c11 Jan 2018 07:16:55 -
> @@ -47,25 +47,8 @@ ptree(struct op *t, int indent, struct s
>   fptreef(shf, indent, "#no-args# ");
>   break;
>   case TEXEC:
> -#if 0 /* ?not useful - can't be called? */
> - /* Print original vars */
> - if (t->left->vars)
> - for (w = t->left->vars; *w != NULL; )
> - fptreef(shf, indent, "%S ", *w++);
> - else
> - fptreef(shf, indent, "#no-vars# ");
> - /* Print expanded vars */
> - if (t->args)
> - for (w = t->args; *w != NULL; )
> - fptreef(shf, indent, "%s ", *w++);
> - else
> - fptreef(shf, indent, "#no-args# ");
> - /* Print original io */
> - t = t->left;
> -#else
>   t = t->left;
>   goto Chain;
> -#endif
>   case TPAREN:
>   fptreef(shf, indent + 2, "( %T) ", t->left);
>   break;
> 



ksh: ulimit optstring

2018-01-14 Thread Anton Lindqvist
Hi,
Stop constructing the optstring passed to ksh_getopt() for ulimit at
runtime. While here remove ifdef RLIMIT_VMEM block since it's not
defined.

Comments? OK?

Index: c_ulimit.c
===
RCS file: /cvs/src/bin/ksh/c_ulimit.c,v
retrieving revision 1.24
diff -u -p -r1.24 c_ulimit.c
--- c_ulimit.c  14 Dec 2015 13:59:42 -  1.24
+++ c_ulimit.c  14 Jan 2018 16:12:33 -
@@ -53,27 +53,14 @@ c_ulimit(char **wp)
{ "memory(kbytes)", RLIMIT_RSS, 1024, 'm' },
{ "nofiles(descriptors)", RLIMIT_NOFILE, 1, 'n' },
{ "processes", RLIMIT_NPROC, 1, 'p' },
-#ifdef RLIMIT_VMEM
-   { "vmemory(kbytes)", RLIMIT_VMEM, 1024, 'v' },
-#endif /* RLIMIT_VMEM */
{ NULL }
};
-   static char options[4 + NELEM(limits) * 2];
+   static const char   *options = "HSat#f#c#d#s#l#m#n#p#";
int how = SOFT | HARD;
const struct limits *l;
int optc, all = 0;
 
-   if (!options[0]) {
-   /* build options string on first call - yuck */
-   char *p = options;
 
-   *p++ = 'H'; *p++ = 'S'; *p++ = 'a';
-   for (l = limits; l->name; l++) {
-   *p++ = l->option;
-   *p++ = '#';
-   }
-   *p = '\0';
-   }
/* First check for -a, -H and -S. */
while ((optc = ksh_getopt(wp, _opt, options)) != -1)
switch (optc) {



Re: httpd response mimetype bug

2018-01-13 Thread Anton Lindqvist
On Sat, Jan 13, 2018 at 01:08:38PM +0100, Hiltjo Posthuma wrote:
> On Sat, Jan 13, 2018 at 09:39:44AM +0100, Anton Lindqvist wrote:
> > On Tue, Jan 09, 2018 at 05:38:57PM +0100, Hidvégi Gábor wrote:
> > > >Synopsis: httpd reports wrong mimetype when item is in the browser cache
> > > >Category: httpd
> > > >Environment:
> > > System  : OpenBSD 6.2
> > > Details : OpenBSD 6.2 (GENERIC) #91: Wed Oct  4 00:35:21 MDT
> > > 2017
> > > 
> > > dera...@armv7.openbsd.org:/usr/src/sys/arch/armv7/compile/GENERIC
> > > 
> > > Architecture: OpenBSD.armv7
> > > Machine : armv7
> > > >Description:
> > > 
> > > httpd serves static files (eg. images) with Last-Modified http header. 
> > > When
> > > a browser next time asks whether this file changed (sends 
> > > If-Modified-Since
> > > http header) httpd responds with wrong mimetype, 'text/html' when the
> > > resource is in the browser cache (304 Not Modified status code).
> > > 
> > > >How-To-Repeat:
> > > 
> > > This bug is common, not arm only. When for example you open this image:
> > > https://man.openbsd.org/openbsd.gif
> > > 
> > > in a browser with developer tools (F12) open, on the network tab you can
> > > take a look at the response headers, mimetype is correct (image/gif). 
> > > After
> > > opening press refresh (F5) and look at the response headers again, and you
> > > get the incorrect mimetype, 'text/html'.
> > > 
> > > >Fix:
> > > 
> > > check httpd source
> > 
> > Please try out this diff, it makes sure to set the correct MIME-type and
> > not respond with a body if the resource has not changed. Sending this to
> > tech@ as well.
> > 
> > Index: server_file.c
> > ===
> > RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
> > retrieving revision 1.65
> > diff -u -p -r1.65 server_file.c
> > --- server_file.c   2 Feb 2017 22:19:59 -   1.65
> > +++ server_file.c   12 Jan 2018 19:10:20 -
> > @@ -230,8 +230,15 @@ server_file_request(struct httpd *env, s
> > goto abort;
> > }
> >  
> > -   if ((ret = server_file_modified_since(clt->clt_descreq, st)) != -1)
> > -   return (ret);
> > +   if (server_file_modified_since(clt->clt_descreq, st) == 0) {
> > +   media = media_find_config(env, srv_conf, path);
> > +   ret = server_response_http(clt, 304, media, 0,
> > +   st->st_mtim.tv_sec);
> > +   if (ret != -1)
> > +   goto done;
> > +   else
> > +   goto fail;
> > +   }
> >  
> > /* Now open the file, should be readable or we have another problem */
> > if ((fd = open(path, O_RDONLY)) == -1)
> > @@ -663,10 +670,10 @@ server_file_modified_since(struct http_d
> > if (strptime(since->kv_value,
> > "%a, %d %h %Y %T %Z", ) != NULL &&
> > timegm() >= st->st_mtim.tv_sec)
> > -   return (304);
> > +   return (0);
> > }
> >  
> > -   return (-1);
> > +   return (1);
> >  }
> >  
> >  int
> > 
> 
> Hey,
> 
> I've tested your patch.
> 
> When requesting a non-modified CSS file:
> 
>   #!/bin/sh
>   host="127.0.0.1"
>   port="6970"
>   printf 'GET /style.css HTTP/1.1\r\nHost: %s:%s\r\nIf-Modified-Since: 
> Sat, 16 Dec 2017 13:07:53 GMT\r\nConnection: close\r\n\r\n' "$host" "$port" | 
> \
>   nc "$host" "$port"
> 
> Full HTTP response:
> 
>   HTTP/1.1 304 Not Modified
>   Connection: close
>   Content-Length: 0
>   Content-Type: text/css
>   Date: Sat, 13 Jan 2018 11:54:13 GMT
>   Last-Modified: Sun, 05 Mar 2017 12:22:05 GMT
>   Server: OpenBSD httpd
> 
> I wonder if httpd should just omit the response header Content-Length and
> Content-Type entirely for this statuscode. Some httpd such as Nginx just
> omit them aswell.
> 
> At the moment responses with redirects (statuscode 301 and 302) also
> output a body response. Maybe it is better to handle it in server_http.c
> in the function server_abort_http() and not output the body there for some
> response status codes? I'm not sure.

Quoting the RFC[1]:

> A server MAY send a Content-Length he

ksh: remove BRACE_EXPAND

2018-01-13 Thread Anton Lindqvist
Hi,
Looks like all variants of ksh in the tree is compiled with BRACE_EXPAND
defined, therefore remove it. No binary change. While here, fix an endif
typo.

Comments? OK?

Index: config.h
===
RCS file: /cvs/src/bin/ksh/config.h,v
retrieving revision 1.17
diff -u -p -r1.17 config.h
--- config.h5 Jan 2018 15:44:31 -   1.17
+++ config.h11 Jan 2018 22:10:58 -
@@ -11,9 +11,6 @@
 #ifndef CONFIG_H
 #define CONFIG_H
 
-/* Include brace-expansion? */
-#define BRACE_EXPAND 1
-
 /* Include any history? */
 #define HISTORY 1
 
Index: emacs.c
===
RCS file: /cvs/src/bin/ksh/emacs.c,v
retrieving revision 1.82
diff -u -p -r1.82 emacs.c
--- emacs.c 8 Jan 2018 13:01:31 -   1.82
+++ emacs.c 11 Jan 2018 22:10:58 -
@@ -2132,4 +2132,4 @@ x_lastcp(void)
return (xlp);
 }
 
-#endif /* EDIT */
+#endif /* EMACS */
Index: eval.c
===
RCS file: /cvs/src/bin/ksh/eval.c,v
retrieving revision 1.57
diff -u -p -r1.57 eval.c
--- eval.c  6 Jan 2018 16:28:58 -   1.57
+++ eval.c  11 Jan 2018 22:10:58 -
@@ -56,9 +56,7 @@ staticvoidglobit(XString *, char **, c
 static char*maybe_expand_tilde(char *, XString *, char **, int);
 static char   *tilde(char *);
 static char   *homedir(char *);
-#ifdef BRACE_EXPAND
 static voidalt_expand(XPtrV *, char *, char *, char *, int);
-#endif
 
 /* compile and expand word */
 char *
@@ -180,10 +178,8 @@ expand(char *cp,   /* input word */
f &= ~DOGLOB;
if (Flag(FMARKDIRS))
f |= DOMARKDIRS;
-#ifdef BRACE_EXPAND
if (Flag(FBRACEEXPAND) && (f & DOGLOB))
f |= DOBRACE_;
-#endif /* BRACE_EXPAND */
 
Xinit(ds, dp, 128, ATEMP);  /* init dest. string */
type = XBASE;
@@ -563,15 +559,12 @@ expand(char *cp,  /* input word */
 
*dp++ = '\0';
p = Xclose(ds, dp);
-#ifdef BRACE_EXPAND
if (fdo & DOBRACE_)
/* also does globbing */
alt_expand(wp, p, p,
p + Xlength(ds, (dp - 1)),
fdo | (f & DOMARKDIRS));
-   else
-#endif /* BRACE_EXPAND */
-   if (fdo & DOGLOB)
+   else if (fdo & DOGLOB)
glob(p, wp, f & DOMARKDIRS);
else if ((f & DOPAT) || !(fdo & DOMAGIC_))
XPput(*wp, p);
@@ -628,7 +621,6 @@ expand(char *cp,/* input word */
*dp++ = MAGIC;
}
break;
-#ifdef BRACE_EXPAND
case OBRACE:
case ',':
case CBRACE:
@@ -638,7 +630,6 @@ expand(char *cp,/* input word */
*dp++ = MAGIC;
}
break;
-#endif /* BRACE_EXPAND */
case '=':
/* Note first unquoted = for ~ */
if (!(f & DOTEMP_) && !saw_eq) {
@@ -1221,7 +1212,6 @@ homedir(char *name)
return ap->val.s;
 }
 
-#ifdef BRACE_EXPAND
 static void
 alt_expand(XPtrV *wp, char *start, char *exp_start, char *end, int fdo)
 {
@@ -1296,4 +1286,3 @@ alt_expand(XPtrV *wp, char *start, char 
}
return;
 }
-#endif /* BRACE_EXPAND */
Index: main.c
===
RCS file: /cvs/src/bin/ksh/main.c,v
retrieving revision 1.86
diff -u -p -r1.86 main.c
--- main.c  5 Jan 2018 15:44:31 -   1.86
+++ main.c  11 Jan 2018 22:10:58 -
@@ -226,9 +226,7 @@ main(int argc, char *argv[])
 * brace expansion, so set this before setting up FPOSIX
 * (change_flag() clears FBRACEEXPAND when FPOSIX is set).
 */
-#ifdef BRACE_EXPAND
Flag(FBRACEEXPAND) = 1;
-#endif /* BRACE_EXPAND */
 
/* set posix flag just before environment so that it will have
 * exactly the same effect as the POSIXLY_CORRECT environment
Index: misc.c
===
RCS file: /cvs/src/bin/ksh/misc.c,v
retrieving revision 1.65
diff -u -p -r1.65 misc.c
--- misc.c  6 Jan 2018 16:28:58 -   1.65
+++ misc.c  11 Jan 2018 22:10:58 -
@@ -121,9 +121,7 @@ const struct option sh_options[] = {
 * entries MUST match the order of sh_flag F* enumerations in sh.h.
 */
{ 

Re: httpd response mimetype bug

2018-01-13 Thread Anton Lindqvist
On Tue, Jan 09, 2018 at 05:38:57PM +0100, Hidvégi Gábor wrote:
> >Synopsis: httpd reports wrong mimetype when item is in the browser cache
> >Category: httpd
> >Environment:
> System  : OpenBSD 6.2
> Details : OpenBSD 6.2 (GENERIC) #91: Wed Oct  4 00:35:21 MDT
> 2017
> 
> dera...@armv7.openbsd.org:/usr/src/sys/arch/armv7/compile/GENERIC
> 
> Architecture: OpenBSD.armv7
> Machine : armv7
> >Description:
> 
> httpd serves static files (eg. images) with Last-Modified http header. When
> a browser next time asks whether this file changed (sends If-Modified-Since
> http header) httpd responds with wrong mimetype, 'text/html' when the
> resource is in the browser cache (304 Not Modified status code).
> 
> >How-To-Repeat:
> 
> This bug is common, not arm only. When for example you open this image:
> https://man.openbsd.org/openbsd.gif
> 
> in a browser with developer tools (F12) open, on the network tab you can
> take a look at the response headers, mimetype is correct (image/gif). After
> opening press refresh (F5) and look at the response headers again, and you
> get the incorrect mimetype, 'text/html'.
> 
> >Fix:
> 
> check httpd source

Please try out this diff, it makes sure to set the correct MIME-type and
not respond with a body if the resource has not changed. Sending this to
tech@ as well.

Index: server_file.c
===
RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v
retrieving revision 1.65
diff -u -p -r1.65 server_file.c
--- server_file.c   2 Feb 2017 22:19:59 -   1.65
+++ server_file.c   12 Jan 2018 19:10:20 -
@@ -230,8 +230,15 @@ server_file_request(struct httpd *env, s
goto abort;
}
 
-   if ((ret = server_file_modified_since(clt->clt_descreq, st)) != -1)
-   return (ret);
+   if (server_file_modified_since(clt->clt_descreq, st) == 0) {
+   media = media_find_config(env, srv_conf, path);
+   ret = server_response_http(clt, 304, media, 0,
+   st->st_mtim.tv_sec);
+   if (ret != -1)
+   goto done;
+   else
+   goto fail;
+   }
 
/* Now open the file, should be readable or we have another problem */
if ((fd = open(path, O_RDONLY)) == -1)
@@ -663,10 +670,10 @@ server_file_modified_since(struct http_d
if (strptime(since->kv_value,
"%a, %d %h %Y %T %Z", ) != NULL &&
timegm() >= st->st_mtim.tv_sec)
-   return (304);
+   return (0);
}
 
-   return (-1);
+   return (1);
 }
 
 int



Re: ksh: remove unused param from kb_add()

2018-01-08 Thread Anton Lindqvist
On Sun, Jan 07, 2018 at 12:26:22PM -0700, Todd C. Miller wrote:
> In kb_add(), the args parameter is always NULL.  Noticed by anton@

ok anton@



wsconsctl: minor cleanup

2017-12-30 Thread Anton Lindqvist
Hi,
Get rid of a unused variable and define the noinput lex option in order
to suppress the following warning:

  map_scan.c:1235:16: warning: function 'input' is not needed and will not be 
emitted
  static int input  (void)

Comments? OK?

Index: map_scan.l
===
RCS file: /cvs/src/sbin/wsconsctl/map_scan.l,v
retrieving revision 1.7
diff -u -p -r1.7 map_scan.l
--- map_scan.l  9 Jul 2017 14:04:50 -   1.7
+++ map_scan.l  30 Dec 2017 16:50:02 -
@@ -30,7 +30,7 @@
  * POSSIBILITY OF SUCH DAMAGE.
  */
 
-%option noyywrap
+%option noinput noyywrap
 
 %{
 
Index: mousecfg.c
===
RCS file: /cvs/src/sbin/wsconsctl/mousecfg.c,v
retrieving revision 1.1
diff -u -p -r1.1 mousecfg.c
--- mousecfg.c  21 Jul 2017 20:38:20 -  1.1
+++ mousecfg.c  30 Dec 2017 16:50:02 -
@@ -303,7 +303,6 @@ mousecfg_pr_field(struct wsmouse_paramet
 void
 mousecfg_rd_field(struct wsmouse_parameters *field, char *val)
 {
-   enum wsmousecfg first = field->params[0].key;
int i, n;
const char *s;
float f;



Re: release(8): join exports

2017-12-23 Thread Anton Lindqvist
On Fri, Dec 22, 2017 at 10:39:20PM +0100, Mark Kettenis wrote:
> > Date: Fri, 22 Dec 2017 22:12:19 +0100
> > From: Anton Lindqvist <an...@openbsd.org>
> > 
> > Hi,
> > Since export accepts several variables, put them on a single line.
> > 
> > Comments? OK?
> 
> Feels like unnecessary chrun to me.  The exports are already on a
> single line, so there is no benefit for copy and pasting them into a
> shell.

Sorry, this went in a bit too quick but with an OK of course. The export
invocations are by now at least consistent throughout the manual.
However, feel free to retract or change. I have no personal preference
other than keeping them consistent.



release(8): join exports

2017-12-22 Thread Anton Lindqvist
Hi,
Since export accepts several variables, put them on a single line.

Comments? OK?

Index: release.8
===
RCS file: /cvs/src/share/man/man8/release.8,v
retrieving revision 1.89
diff -u -p -r1.89 release.8
--- release.8   5 Jun 2017 22:27:58 -   1.89
+++ release.8   22 Dec 2017 21:07:34 -
@@ -202,7 +202,7 @@ is also used and must not be configured.
 .Pp
 Make the release and check the contents of the release tarballs:
 .Bd -literal -offset indent
-# export DESTDIR=your-destdir; export RELEASEDIR=your-releasedir
+# export DESTDIR=your-destdir RELEASEDIR=your-releasedir
 # cd /usr/src/etc && make release
 # cd /usr/src/distrib/sets && sh checkflist
 # unset RELEASEDIR DESTDIR
@@ -240,7 +240,7 @@ will be removed.
 .Pp
 The steps to build and validate the Xenocara release are:
 .Bd -literal -offset indent
-# export DESTDIR=your-destdir; export RELEASEDIR=your-releasedir
+# export DESTDIR=your-destdir RELEASEDIR=your-releasedir
 # make release
 # make checkdist
 # unset RELEASEDIR DESTDIR
@@ -263,8 +263,7 @@ and
 are suitable for installs without network connectivity.
 They contain the tarballs and ports built in the previous steps.
 .Bd -literal -offset indent
-# export RELDIR=your-releasedir
-# export RELXDIR=your-xenocara-releasedir
+# export RELDIR=your-releasedir RELXDIR=your-xenocara-releasedir
 # cd /usr/src/distrib/$(machine)/iso && make
 # make install
 .Ed



Re: one XXX in ksh(1)

2017-12-18 Thread Anton Lindqvist
On Mon, Dec 18, 2017 at 08:37:17PM +0100, Anton Lindqvist wrote:
> On Mon, Dec 18, 2017 at 03:18:37PM +0800, Michael W. Bombardieri wrote:
> > Hello,
> > 
> > In my understanding the reason why texec had to be static was
> > because the struct member ioact was never initialised.
> > The call tree is execute() -> comexec() -> exchild() -> execute()
> > and a fork happens in exchild().
> > The second execute() dereferences t->ioact on line 115 which causes
> > SEGV. Setting ioact to NULL explicitly seems better than
> > doing this via "static".
> > Does this work for you?
> > 
> > - Michael
> > 
> > 
> > Index: exec.c
> > ===
> > RCS file: /cvs/src/bin/ksh/exec.c,v
> > retrieving revision 1.68
> > diff -u -p -u -r1.68 exec.c
> > --- exec.c  11 Dec 2016 17:49:19 -  1.68
> > +++ exec.c  18 Dec 2017 07:05:47 -
> > @@ -409,7 +409,7 @@ comexec(struct op *t, struct tbl *volati
> > volatile int rv = 0;
> > char *cp;
> > char **lastp;
> > -   static struct op texec; /* Must be static (XXX but why?) */
> > +   struct op texec;
> > int type_flags;
> > int keepasn_ok;
> > int fcflags = FC_BI|FC_FUNC|FC_PATH;
> > @@ -688,6 +688,7 @@ comexec(struct op *t, struct tbl *volati
> > texec.left = t; /* for tprint */
> > texec.str = tp->val.s;
> > texec.args = ap;
> > +   texec.ioact = NULL;
> > rv = exchild(, flags, xerrok, -1);
> > break;
> > }
> > 
> 
> Good analysis. I would go even further and zero out texec completely.
> 
> Comments? OK?
> 
> Index: exec.c
> ===
> RCS file: /cvs/src/bin/ksh/exec.c,v
> retrieving revision 1.68
> diff -u -p -r1.68 exec.c
> --- exec.c11 Dec 2016 17:49:19 -  1.68
> +++ exec.c18 Dec 2017 19:35:16 -
> @@ -409,7 +409,7 @@ comexec(struct op *t, struct tbl *volati
>   volatile int rv = 0;
>   char *cp;
>   char **lastp;
> - static struct op texec; /* Must be static (XXX but why?) */
> + struct op texec;
>   int type_flags;
>   int keepasn_ok;
>   int fcflags = FC_BI|FC_FUNC|FC_PATH;
> @@ -684,6 +684,7 @@ comexec(struct op *t, struct tbl *volati
>   }
>  
>   /* to fork we set up a TEXEC node and call execute */
> + memset(, 0, sizeof(texec));
>   texec.type = TEXEC;
>   texec.left = t; /* for tprint */
>   texec.str = tp->val.s;

Just committed the diff above, thanks!



Re: one XXX in ksh(1)

2017-12-18 Thread Anton Lindqvist
On Mon, Dec 18, 2017 at 03:18:37PM +0800, Michael W. Bombardieri wrote:
> Hello,
> 
> In my understanding the reason why texec had to be static was
> because the struct member ioact was never initialised.
> The call tree is execute() -> comexec() -> exchild() -> execute()
> and a fork happens in exchild().
> The second execute() dereferences t->ioact on line 115 which causes
> SEGV. Setting ioact to NULL explicitly seems better than
> doing this via "static".
> Does this work for you?
> 
> - Michael
> 
> 
> Index: exec.c
> ===
> RCS file: /cvs/src/bin/ksh/exec.c,v
> retrieving revision 1.68
> diff -u -p -u -r1.68 exec.c
> --- exec.c11 Dec 2016 17:49:19 -  1.68
> +++ exec.c18 Dec 2017 07:05:47 -
> @@ -409,7 +409,7 @@ comexec(struct op *t, struct tbl *volati
>   volatile int rv = 0;
>   char *cp;
>   char **lastp;
> - static struct op texec; /* Must be static (XXX but why?) */
> + struct op texec;
>   int type_flags;
>   int keepasn_ok;
>   int fcflags = FC_BI|FC_FUNC|FC_PATH;
> @@ -688,6 +688,7 @@ comexec(struct op *t, struct tbl *volati
>   texec.left = t; /* for tprint */
>   texec.str = tp->val.s;
>   texec.args = ap;
> + texec.ioact = NULL;
>   rv = exchild(, flags, xerrok, -1);
>   break;
>   }
> 

Good analysis. I would go even further and zero out texec completely.

Comments? OK?

Index: exec.c
===
RCS file: /cvs/src/bin/ksh/exec.c,v
retrieving revision 1.68
diff -u -p -r1.68 exec.c
--- exec.c  11 Dec 2016 17:49:19 -  1.68
+++ exec.c  18 Dec 2017 19:35:16 -
@@ -409,7 +409,7 @@ comexec(struct op *t, struct tbl *volati
volatile int rv = 0;
char *cp;
char **lastp;
-   static struct op texec; /* Must be static (XXX but why?) */
+   struct op texec;
int type_flags;
int keepasn_ok;
int fcflags = FC_BI|FC_FUNC|FC_PATH;
@@ -684,6 +684,7 @@ comexec(struct op *t, struct tbl *volati
}
 
/* to fork we set up a TEXEC node and call execute */
+   memset(, 0, sizeof(texec));
texec.type = TEXEC;
texec.left = t; /* for tprint */
texec.str = tp->val.s;



Re: csh dounsetenv()

2017-12-18 Thread Anton Lindqvist
On Sun, Dec 17, 2017 at 05:54:23PM +0800, Michael W. Bombardieri wrote:
> Hello,
> 
> The free() at the top of dounsetenv() in csh(1) isn't needed
> because name is always freed before returning at bottom of function.
> Also, name itself is never returned so it doesn't need to be static.
> 
> ./csh
> setenv HEY YU
> unsetenv HEY
> printenv
> 
> I ran the above and it seems to work the same as before.
> 
> - Michael
> 
> 
> Index: func.c
> ===
> RCS file: /cvs/src/bin/csh/func.c,v
> retrieving revision 1.36
> diff -u -p -u -r1.36 func.c
> --- func.c16 Dec 2017 10:27:21 -  1.36
> +++ func.c17 Dec 2017 09:41:01 -
> @@ -924,11 +924,9 @@ void
>  /*ARGSUSED*/
>  dounsetenv(Char **v, struct command *t)
>  {
> -Char  **ep, *p, *n;
> +Char  **ep, *p, *n, *name;
>  int i, maxi;
> -static Char *name = NULL;
>  
> -free(name);
>  /*
>   * Find the longest environment variable
>   */
> 

Committed, thanks!



Re: ksh: make home & end keys work out-of-the-box in the console

2017-12-18 Thread Anton Lindqvist
On Mon, Dec 18, 2017 at 06:29:46PM +0100, Anton Lindqvist wrote:
> On Sat, Dec 16, 2017 at 01:58:49PM +0200, Lari Rasku wrote:
> > Tested with vt220 and wsvt25.  Hopefully we're close to exhausting
> > all the different ways to encode these keys by now.
> > 
> > diff --git bin/ksh/emacs.c bin/ksh/emacs.c
> > index 07bfbd914..4291ce656 100644
> > --- bin/ksh/emacs.c
> > +++ bin/ksh/emacs.c
> > @@ -1536,6 +1536,8 @@ x_init_emacs(void)
> > kb_add(x_mv_end,NULL, CTRL('['), 'O', 'F', 0); /* end */
> > kb_add(x_mv_begin,  NULL, CTRL('['), '[', '1', '~', 0); /* 
> > home */
> > kb_add(x_mv_end,NULL, CTRL('['), '[', '4', '~', 0); /* 
> > end */
> > +   kb_add(x_mv_begin,  NULL, CTRL('['), '[', '7', '~', 0); /* 
> > home */
> > +   kb_add(x_mv_end,NULL, CTRL('['), '[', '8', '~', 0); /* 
> > end */
> >  
> > /* can't be bound */
> > kb_add(x_set_arg,   NULL, CTRL('['), '0', 0);
> > 
> 
> Looks correct to me, judging by:
> 
>   $ for f in /usr/share/terminfo/*/*; do infocmp -1 ${f##*/} | grep khome | 
> grep '\[7~'; done
>   $ for f in /usr/share/terminfo/*/*; do infocmp -1 ${f##*/} | grep kend | 
> grep '\[8~'; done
> 
> Any other willing to ok?

Committed, thanks!



Re: ksh: make home & end keys work out-of-the-box in the console

2017-12-18 Thread Anton Lindqvist
On Sat, Dec 16, 2017 at 01:58:49PM +0200, Lari Rasku wrote:
> Tested with vt220 and wsvt25.  Hopefully we're close to exhausting
> all the different ways to encode these keys by now.
> 
> diff --git bin/ksh/emacs.c bin/ksh/emacs.c
> index 07bfbd914..4291ce656 100644
> --- bin/ksh/emacs.c
> +++ bin/ksh/emacs.c
> @@ -1536,6 +1536,8 @@ x_init_emacs(void)
>   kb_add(x_mv_end,NULL, CTRL('['), 'O', 'F', 0); /* end */
>   kb_add(x_mv_begin,  NULL, CTRL('['), '[', '1', '~', 0); /* 
> home */
>   kb_add(x_mv_end,NULL, CTRL('['), '[', '4', '~', 0); /* 
> end */
> + kb_add(x_mv_begin,  NULL, CTRL('['), '[', '7', '~', 0); /* 
> home */
> + kb_add(x_mv_end,NULL, CTRL('['), '[', '8', '~', 0); /* 
> end */
>  
>   /* can't be bound */
>   kb_add(x_set_arg,   NULL, CTRL('['), '0', 0);
> 

Looks correct to me, judging by:

  $ for f in /usr/share/terminfo/*/*; do infocmp -1 ${f##*/} | grep khome | 
grep '\[7~'; done
  $ for f in /usr/share/terminfo/*/*; do infocmp -1 ${f##*/} | grep kend | grep 
'\[8~'; done

Any other willing to ok?



Re: csh: NULL checks before free()

2017-12-16 Thread Anton Lindqvist
On Fri, Dec 15, 2017 at 05:03:40PM +0800, Michael W. Bombardieri wrote:
> Hello,
> 
> Previously in csh(1) the xfree() function was removed in favour
> of regular free(). This patch removes a couple of NULL checks
> before free().

Committed, thanks!



ksh: stop emitting carriage return twice

2017-12-12 Thread Anton Lindqvist
Hi,
Here's a little nit that has been bugging me while writing tests for
Emacs editing mode in ksh. Since the map-CR-to-NL (ONLCR) output mode is
enabled by default on the tty, emitting a newline gets translated into a
carriage return (CR) followed by newline (NL). Parts of the Emacs
related code emits a explicit CR prior to NL causing two CR characters
to be emitted. The diff below disables ONLCR and explicitly ensures a CR
is present prior to every emitted NL. Vi mode already does this. I find
this approach more favorable and explicit as opposed of keeping ONLCR
enabled and removing the double CRs.

Comments? OK?

Index: edit.c
===
RCS file: /cvs/src/bin/ksh/edit.c,v
retrieving revision 1.57
diff -u -p -r1.57 edit.c
--- edit.c  8 Sep 2016 12:12:40 -   1.57
+++ edit.c  11 Dec 2017 20:10:00 -
@@ -178,6 +178,7 @@ x_mode(bool onoff)
edchars.werase = cb.c_cc[VWERASE];
cb.c_iflag &= ~(INLCR|ICRNL);
cb.c_lflag &= ~(ISIG|ICANON|ECHO);
+   cb.c_oflag &= ~(ONLCR);
/* osf/1 processes lnext when ~icanon */
cb.c_cc[VLNEXT] = _POSIX_VDISABLE;
/* sunos 4.1.x & osf/1 processes discard(flush) when ~icanon */
Index: emacs.c
===
RCS file: /cvs/src/bin/ksh/emacs.c,v
retrieving revision 1.75
diff -u -p -r1.75 emacs.c
--- emacs.c 26 Nov 2017 20:34:15 -  1.75
+++ emacs.c 11 Dec 2017 20:10:00 -
@@ -881,7 +881,7 @@ x_search_hist(int c)
*p = '\0';
while (1) {
if (offset < 0) {
-   x_e_puts("\nI-search: ");
+   x_e_puts("\r\nI-search: ");
x_e_puts(pat);
}
x_flush();
@@ -943,8 +943,10 @@ x_search(char *pat, int sameline, int of
for (hp = x_histp - (sameline ? 0 : 1) ; hp >= history; --hp) {
i = x_match(*hp, pat);
if (i >= 0) {
-   if (offset < 0)
+   if (offset < 0) {
+   x_e_putc('\r');
x_e_putc('\n');
+   }
x_load_hist(hp);
x_goto(xbuf + i + strlen(pat) - (*pat == '^'));
return i;
@@ -1018,10 +1020,9 @@ x_redraw(int limit)
char*cp;
 
x_adj_ok = 0;
+   x_e_putc('\r');
if (limit == -1)
x_e_putc('\n');
-   else
-   x_e_putc('\r');
x_flush();
if (xbp == xbuf) {
x_col = promptlen(prompt, NULL);
@@ -1171,7 +1172,7 @@ x_yank(int c)
killtp = killsp;
killtp --;
if (killstack[killtp] == 0) {
-   x_e_puts("\nnothing to yank");
+   x_e_puts("\r\nnothing to yank");
x_redraw(-1);
return KSTD;
}
@@ -1187,7 +1188,7 @@ x_meta_yank(int c)
if ((x_last_command != x_yank && x_last_command != x_meta_yank) ||
killstack[killtp] == 0) {
killtp = killsp;
-   x_e_puts("\nyank something first");
+   x_e_puts("\r\nyank something first");
x_redraw(-1);
return KSTD;
}



Re: ksh(1): kill the "version" function

2017-11-26 Thread Anton Lindqvist
On Sun, Nov 26, 2017 at 07:27:37PM +0100, Jeremie Courreges-Anglas wrote:
> On Tue, Nov 21 2017, Anton Lindqvist <an...@openbsd.org> wrote:
> > Hi,
> > While writing tests for the Emacs editing mode in ksh I discovered some
> > potential errors in the manual:
> >
> > - Sync the order of key bindings in emacs.c with the manual
> >
> > - ^W is bound to delete-word-backward and not kill-region which is
> >   mentioned in emacs.c. I don't know the full history here...
> >
> > - Add missing '^' in backward-char binding
> >
> > - The WERASE control character is also bound to delete-word-backward,
> >   see x_emacs_keys().
> >
> > - Rephrase delete-word-forward to match the other bindings
> >
> > - Move kill-region to a separate list of commands without default
> >   bindings
> 
> The last points brings the following question: what should we do with
> other undocumented functions?
> 
> --8<--
> --- a Wed Nov 22 02:47:00 2017
> +++ b Wed Nov 22 02:47:07 2017
> @@ -1,4 +1,5 @@
>  abort
> +auto-insert
>  backward-char
>  backward-word
>  beginning-of-history
> @@ -19,12 +20,14 @@
>  end-of-line
>  eot
>  eot-or-delete
> +error
>  exchange-point-and-mark
>  expand-file
>  forward-char
>  forward-word
>  goto-history
>  kill-line
> +kill-region
>  kill-to-eol
>  list
>  list-command
> @@ -42,5 +45,6 @@
>  transpose-chars
>  up-history
>  upcase-word
> +version
>  yank
>  yank-pop
> -->8--
> 
> I assume that "auto-insert" and "error" should be left out as
> implementation details, but what about "version"?
> 
> Here's a diff to remove that function.  If people want to keep and
> document it I would not object, but I don't really see the point.
> Quick way to see for yourself: bind ^[v=version then type ESC v
> 
> ok?

ok anton@

While at it, another gem: in Vi normal mode typing ^V inserts the
version string. Does not seem to be documented either.



ksh(1): manual tweaks

2017-11-21 Thread Anton Lindqvist
Hi,
While writing tests for the Emacs editing mode in ksh I discovered some
potential errors in the manual:

- Sync the order of key bindings in emacs.c with the manual

- ^W is bound to delete-word-backward and not kill-region which is
  mentioned in emacs.c. I don't know the full history here...

- Add missing '^' in backward-char binding

- The WERASE control character is also bound to delete-word-backward,
  see x_emacs_keys().

- Rephrase delete-word-forward to match the other bindings

- Move kill-region to a separate list of commands without default
  bindings

Comments? OK?

Index: emacs.c
===
RCS file: /cvs/src/bin/ksh/emacs.c,v
retrieving revision 1.73
diff -u -p -r1.73 emacs.c
--- emacs.c 30 Aug 2017 17:02:53 -  1.73
+++ emacs.c 21 Nov 2017 19:28:44 -
@@ -1472,6 +1472,7 @@ x_init_emacs(void)
kb_add(x_del_back,  NULL, CTRL('?'), 0);
kb_add(x_del_back,  NULL, CTRL('H'), 0);
kb_add(x_del_char,  NULL, CTRL('['), '[', '3', '~', 0); /* 
delete */
+   kb_add(x_del_bword, NULL, CTRL('W'), 0);
kb_add(x_del_bword, NULL, CTRL('['), CTRL('?'), 0);
kb_add(x_del_bword, NULL, CTRL('['), CTRL('H'), 0);
kb_add(x_del_bword, NULL, CTRL('['), 'h', 0);
@@ -1493,7 +1494,6 @@ x_init_emacs(void)
kb_add(x_mv_fword,  NULL, CTRL('['), 'f', 0);
kb_add(x_goto_hist, NULL, CTRL('['), 'g', 0);
/* kill-line */
-   kb_add(x_del_bword, NULL, CTRL('W'), 0); /* not what man 
says */
kb_add(x_kill,  NULL, CTRL('K'), 0);
kb_add(x_enumerate, NULL, CTRL('['), '?', 0);
kb_add(x_list_comm, NULL, CTRL('X'), '?', 0);
@@ -1505,6 +1505,7 @@ x_init_emacs(void)
kb_add(x_prev_histword, NULL, CTRL('['), '.', 0);
kb_add(x_prev_histword, NULL, CTRL('['), '_', 0);
/* how to handle: quote: ^^ */
+   kb_add(x_literal,   NULL, CTRL('^'), 0);
kb_add(x_draw_line, NULL, CTRL('L'), 0);
kb_add(x_search_char_back,  NULL, CTRL('['), CTRL(']'), 0);
kb_add(x_search_char_forw,  NULL, CTRL(']'), 0);
@@ -1516,7 +1517,6 @@ x_init_emacs(void)
kb_add(x_fold_upper,NULL, CTRL('['), 'U', 0);
kb_add(x_fold_upper,NULL, CTRL('['), 'u', 0);
kb_add(x_literal,   NULL, CTRL('V'), 0);
-   kb_add(x_literal,   NULL, CTRL('^'), 0);
kb_add(x_yank,  NULL, CTRL('Y'), 0);
kb_add(x_meta_yank, NULL, CTRL('['), 'y', 0);
/* man page ends here */
Index: ksh.1
===
RCS file: /cvs/src/bin/ksh/ksh.1,v
retrieving revision 1.195
diff -u -p -r1.195 ksh.1
--- ksh.1   30 Aug 2017 17:08:45 -  1.195
+++ ksh.1   21 Nov 2017 19:28:45 -
@@ -4656,7 +4656,7 @@ Simply causes the character to appear as
 Most ordinary characters are bound to this.
 .It Xo backward-char:
 .Op Ar n
-.No ^B , ^XD
+.No ^B , ^X^D
 .Xc
 Moves the cursor backward
 .Ar n
@@ -4749,7 +4749,7 @@ Deletes
 characters after the cursor.
 .It Xo delete-word-backward:
 .Op Ar n
-.No ERASE , ^[^? , ^[^H , ^[h
+.No WERASE , ^[ERASE , ^W, ^[^? , ^[^H , ^[h
 .Xc
 Deletes
 .Ar n
@@ -4758,9 +4758,9 @@ words before the cursor.
 .Op Ar n
 .No ^[d
 .Xc
-Deletes characters after the cursor up to the end of
+Deletes
 .Ar n
-words.
+words after the cursor.
 .It Xo down-history:
 .Op Ar n
 .No ^N , ^XB
@@ -4830,8 +4830,6 @@ Goes to history number
 .Ar n .
 .It kill-line: KILL
 Deletes the entire input line.
-.It kill-region: ^W
-Deletes the input between the cursor and the mark.
 .It Xo kill-to-eol:
 .Op Ar n
 .No ^K
@@ -4937,6 +4935,14 @@ Inserts the most recently killed text st
 Immediately after a
 .Ic yank ,
 replaces the inserted text string with the next previously killed text string.
+.El
+.Pp
+The following editing commands lack default bindings but can be used with the
+.Ic bind
+command:
+.Bl -tag -width Ds
+.It kill-region
+Deletes the input between the cursor and the mark.
 .El
 .Ss Vi editing mode
 The vi command-line editor in



Re: wsconscfg(8): ioctl request macros

2017-11-15 Thread Anton Lindqvist
On Tue, Oct 31, 2017 at 12:24:20PM +0100, Anton Lindqvist wrote:
> Hi,
> Use the actual ioctl request macros instead of relying on compatibility
> indirection.
> 
> Looks like wsconscfg(8) is the last consumer of the compat macros
> defined in dev/wscons/wsconsio.h. Assuming there's no usage in ports,
> now might be the time to zap them?

sthen@ helped me grep for the compat macros in ports and found nothing.
OK to remove?

Index: wsconsio.h
===
RCS file: /cvs/src/sys/dev/wscons/wsconsio.h,v
retrieving revision 1.86
diff -u -p -r1.86 wsconsio.h
--- wsconsio.h  24 Oct 2017 09:36:13 -  1.86
+++ wsconsio.h  15 Nov 2017 10:47:35 -
@@ -612,7 +612,6 @@ struct wsdisplay_emultype {
  */
 
 #define WSMUXIO_INJECTEVENT_IOW('W', 96, struct wscons_event)
-#defineWSMUX_INJECTEVENT   WSMUXIO_INJECTEVENT /* XXX compat */
 
 struct wsmux_device {
int type;
@@ -622,9 +621,7 @@ struct wsmux_device {
int idx;
 };
 #define WSMUXIO_ADD_DEVICE _IOW('W', 97, struct wsmux_device)
-#defineWSMUX_ADD_DEVICEWSMUXIO_ADD_DEVICE  /* XXX compat */
 #define WSMUXIO_REMOVE_DEVICE  _IOW('W', 98, struct wsmux_device)
-#defineWSMUX_REMOVE_DEVICE WSMUXIO_REMOVE_DEVICE   /* XXX compat */
 
 #define WSMUX_MAXDEV 32
 struct wsmux_device_list {
@@ -632,6 +629,5 @@ struct wsmux_device_list {
struct wsmux_device devices[WSMUX_MAXDEV];
 };
 #define WSMUXIO_LIST_DEVICES   _IOWR('W', 99, struct wsmux_device_list)
-#defineWSMUX_LIST_DEVICES  WSMUXIO_LIST_DEVICES/* XXX compat */
 
 #endif /* _DEV_WSCONS_WSCONSIO_H_ */



csh(1): minor tweaks

2017-11-14 Thread Anton Lindqvist
Hi,
In revision 1.35 of file.c tenex() was modified to respect the
inputline_size argument. I did miss one conditional which depends on the
size of the input buffer. This change does not cause any functional
change since tenex() only has one call site where
`inputline_size == sizeof(buf)` but for correctness...

While here, zap a redundant cast. Will go in a separate commit.

Comments? OK?

Index: file.c
===
RCS file: /cvs/src/bin/csh/file.c,v
retrieving revision 1.35
diff -u -p -r1.35 file.c
--- file.c  30 Aug 2017 06:57:48 -  1.35
+++ file.c  15 Nov 2017 07:47:01 -
@@ -673,8 +673,7 @@ again:  /* search for matches */
return (numitems);
 }
 else { /* LIST */
-   qsort(items, numitems, sizeof(*items),
-   (int (*)(const void *, const void *)) sortscmp);
+   qsort(items, numitems, sizeof(*items), sortscmp);
print_by_column(looking_for_lognames ? NULL : tilded_dir,
items, numitems);
if (items != NULL)
@@ -810,7 +809,7 @@ tenex(Char *inputline, int inputline_siz
 * buffer is full since more characters must be read in order to form a
 * complete command.
 */
-   if (i < sizeof(buf))
+   if (i < cl.size)
inputline[i] = '\0';
 
return cl.len;



Re: libfuse: signal handler doesn't cater for "Device busy" and other errors

2017-11-10 Thread Anton Lindqvist
On Fri, Nov 10, 2017 at 09:36:25AM +0100, Martin Pieuchot wrote:
> On 09/11/17(Thu) 09:02, Helg Bredow wrote:
> > The current libfuse signal handling assumes that the file system will 
> > always be unmounted by the child. One obvious case where this is not true 
> > is if the file system is busy. To replicate:
> > 
> > 1. mount a fuse file system
> > 2. cd anywhere on the file system
> > 3. pkill -INT 
> > 
> > The result is a zombie child process and no more response to signals even 
> > if the file system is no longer busy.
> > 
> > This patch ensures that the child always exits and that an error is printed 
> > to stdout if the file system cannot be unmounted. Tested with fuse-exfat 
> > and ntfs_3g. Suggestions for improvement are welcome.
> 
> Nice to see that you're fixing a bug.  However I'd suggest you to go
> much further in your improvement.
> 
> Signal handlers are hard and instead of doing work inside the signal
> handler you should toggle a global variable/flag and do this work
> inside the main loop (fuse_loop()?).
> 
> For example your code below calls fprintf(3) in the signal handler.  This
> is incorrect, this functions is not signal handler safe.  What about 
> fuse_unmount()?  Are you sure it can be called from a signal handler?

Some more info on making signal handlers asynchronous-safe:

https://www.securecoding.cert.org/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers



wsconscfg(8): ioctl request macros

2017-10-31 Thread Anton Lindqvist
Hi,
Use the actual ioctl request macros instead of relying on compatibility
indirection.

Looks like wsconscfg(8) is the last consumer of the compat macros
defined in dev/wscons/wsconsio.h. Assuming there's no usage in ports,
now might be the time to zap them?

Comments? OK?

Index: wsconscfg.c
===
RCS file: /cvs/src/usr.sbin/wsconscfg/wsconscfg.c,v
retrieving revision 1.15
diff -u -p -r1.15 wsconscfg.c
--- wsconscfg.c 23 Aug 2017 09:15:33 -  1.15
+++ wsconscfg.c 31 Oct 2017 11:16:47 -
@@ -125,13 +125,13 @@ main(int argc, char *argv[])
wmd.type = WSMUX_KBD;
wmd.idx = idx;
if (delete) {
-   res = ioctl(wsfd, WSMUX_REMOVE_DEVICE, );
+   res = ioctl(wsfd, WSMUXIO_REMOVE_DEVICE, );
if (res < 0)
-   err(3, "WSMUX_REMOVE_DEVICE");
+   err(3, "WSMUXIO_REMOVE_DEVICE");
} else {
-   res = ioctl(wsfd, WSMUX_ADD_DEVICE, );
+   res = ioctl(wsfd, WSMUXIO_ADD_DEVICE, );
if (res < 0)
-   err(3, "WSMUX_ADD_DEVICE");
+   err(3, "WSMUXIO_ADD_DEVICE");
}
} else if (delete) {
dsd.idx = idx;



pms: reset announcement

2017-10-29 Thread Anton Lindqvist
Hi,
Every now and then, my Synaptics touchpad stops working and no further
interrupts are received for the device. The following message appears in
the log:

  pms0: not in sync yet, discard input (state = 0)

Examining the bytes received prior to the halt reveals the sequence
[0xaa, 0x00] which denotes a Synaptics reset announcement[1]. This
sequence is sent after the touchpad experienced a loss of power and the
device is reset. Since any altered setting prior to the reset is lost
the device must be re-enabled in order to continue functioning. The
simplest(?) way is to disable and then enable the device (many thanks to
bru@ for helping out with this part).

Testing this diff on hardware equipped with a Synaptics touchpad would be much
appreciated. Even if you never experienced a reset in order make sure no
regression is introduced.

In addition, I'm looking for feedback and OKs.

[1] 
https://www.aquaphoenix.com/hardware/ledlamp/reference/synaptics_touchpad_interfacing_guide.pdf#page=32

Index: pms.c
===
RCS file: /cvs/src/sys/dev/pckbc/pms.c,v
retrieving revision 1.81
diff -u -p -r1.81 pms.c
--- pms.c   28 Oct 2017 14:31:29 -  1.81
+++ pms.c   28 Oct 2017 16:55:22 -
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -88,6 +89,8 @@ struct synaptics_softc {
 
int mode;
 
+   int annstate;   /* announcement state */
+
int mask;
 #define SYNAPTICS_MASK_NEWABS_STRICT   0xc8
 #define SYNAPTICS_MASK_NEWABS_RELAXED  0xc0
@@ -158,6 +161,8 @@ struct pms_softc {  /* driver status inf
 #define PMS_DEV_PRIMARY0x01
 #define PMS_DEV_SECONDARY  0x02
 
+   struct task sc_reset_task;
+
int poll;
int inputstate;
 
@@ -255,6 +260,7 @@ int pms_reset(struct pms_softc *);
 intpms_dev_enable(struct pms_softc *);
 intpms_dev_disable(struct pms_softc *);
 void   pms_protocol_lookup(struct pms_softc *);
+void   pms_reset_task(void *);
 
 intpms_enable_intelli(struct pms_softc *);
 
@@ -293,6 +299,7 @@ int synaptics_set_mode(struct pms_softc 
 intsynaptics_query(struct pms_softc *, int, int *);
 intsynaptics_get_hwinfo(struct pms_softc *);
 void   synaptics_sec_proc(struct pms_softc *);
+intsynaptics_is_reset(struct pms_softc *, int);
 
 intalps_sec_proc(struct pms_softc *);
 intalps_get_hwinfo(struct pms_softc *);
@@ -542,6 +549,32 @@ pms_protocol_lookup(struct pms_softc *sc
DPRINTF("%s: protocol type %d\n", DEVNAME(sc), sc->protocol->type);
 }
 
+void
+pms_reset_task(void *v)
+{
+   struct pms_softc *sc = v;
+
+   rw_enter_write(>sc_state_lock);
+
+   /* Do nothing if the device already is disabled. */
+   if (sc->sc_state == PMS_STATE_DISABLED)
+   goto done;
+
+#ifdef DIAGNOSTIC
+   printf("%s: device reset\n", DEVNAME(sc));
+#endif
+   if (sc->sc_sec_wsmousedev != NULL)
+   pms_change_state(sc, PMS_STATE_DISABLED, PMS_DEV_SECONDARY);
+   pms_change_state(sc, PMS_STATE_DISABLED, PMS_DEV_PRIMARY);
+
+   pms_change_state(sc, PMS_STATE_ENABLED, PMS_DEV_PRIMARY);
+   if (sc->sc_sec_wsmousedev != NULL)
+   pms_change_state(sc, PMS_STATE_ENABLED, PMS_DEV_SECONDARY);
+
+done:
+   rw_exit_write(>sc_state_lock);
+}
+
 int
 pms_enable_intelli(struct pms_softc *sc)
 {
@@ -679,6 +712,8 @@ pmsattach(struct device *parent, struct 
 */
sc->sc_wsmousedev = config_found(self, , wsmousedevprint);
 
+   task_set(>sc_reset_task, pms_reset_task, sc);
+
sc->poll = 1;
sc->sc_dev_enable = 0;
 
@@ -850,9 +885,11 @@ pmsinput(void *vsc, int data)
sc->packet[sc->inputstate] = data;
if (sc->protocol->sync(sc, data)) {
 #ifdef DIAGNOSTIC
-   printf("%s: not in sync yet, discard input (state %d)\n",
-   DEVNAME(sc), sc->inputstate);
+   printf("%s: not in sync yet, discard input "
+   "(state = %d, data = %#x)\n",
+   DEVNAME(sc), sc->inputstate, data);
 #endif
+
sc->inputstate = 0;
return;
}
@@ -1021,6 +1058,35 @@ synaptics_knock(struct pms_softc *sc)
return (0);
 }
 
+/*
+ * Recognize reset announcement ([0xaa, 0x0]).
+ * The sequence will be sent as input on rare occasions when the touchpad was
+ * reset due to a power failure.
+ */
+int
+synaptics_is_reset(struct pms_softc *sc, int data)
+{
+   struct synaptics_softc *syn = sc->synaptics;
+
+   switch (syn->annstate) {
+   case 0:
+   if (data == PMS_RSTDONE) {
+   syn->annstate++;
+   return (0);
+   }
+   break;
+   case 1:
+   if (data == 0) {
+   syn->annstate = 0;
+   return (1);
+   }
+   break;
+   }
+   syn->annstate = 0;
+
+   return (0);
+}
+
 int
 

Re: apmd: manual tweak

2017-10-16 Thread Anton Lindqvist
On Mon, Oct 16, 2017 at 06:10:02PM +0100, Jason McIntyre wrote:
> On Mon, Oct 16, 2017 at 06:38:31PM +0200, Anton Lindqvist wrote:
> > 
> > Good call. Without knowing too much about the apmd internals, here's a
> > first stab. As I read the code, the period polling is also used by
> > auto{suspend,hibernate}. Is this worth mentioning? Since a more frequent
> > interval will increase the accuracy of the chosen action to perform?
> > 
> 
> hmm. if it does more than what is documented, maybe we could get away
> with being vaguer, without losing anything important? i'm not sure.

Input from someone with better knowledge of the implementation would
help. tedu@ perhaps?

> > Index: apmd.8
> > ===
> > RCS file: /cvs/src/usr.sbin/apmd/apmd.8,v
> > retrieving revision 1.49
> > diff -u -p -r1.49 apmd.8
> > --- apmd.8  29 Aug 2017 06:13:20 -  1.49
> > +++ apmd.8  16 Oct 2017 16:29:20 -
> > @@ -57,13 +57,17 @@ When resuming after suspend or standby,
> >  .Nm
> >  runs the appropriate configuration program (if one exists).
> >  .Pp
> > -When the power status changes
> > -(battery is connected or disconnected)
> > +Upon receiving an event from
> > +.Xr apm 4 ,
> 
> have you changed this text because apm can warn about other things than
> battery connect or just you think it reads better? i ask because what
> was there was easier to understand.

Reads better to me, but that's just my opinion and I will revert the
change.

> 
> >  .Nm
> >  fetches the current status and reports it via
> >  .Xr syslog 3
> >  with logging facility
> >  .Dv LOG_DAEMON .
> > +In addition,
> > +.Nm
> > +periodically polls the power status and reports significant changes to the
> > +battery charge level.
> >  .Pp
> >  The options are as follows:
> >  .Bl -tag -width Ds
> > @@ -107,14 +111,9 @@ and
> >  .Nm
> >  exits without monitoring the APM status.
> >  .It Fl t Ar seconds
> > -.Nm
> > -periodically polls the APM driver for the current power state.
> > -If the battery charge level changes substantially or the external power
> > -status changes, the new status is logged.
> > -The polling rate defaults to
> > -once per 10 minutes, but may be specified using the
> > -.Fl t
> > -command-line flag.
> > +Set the APM driver poll interval to
> > +.Ar seconds .
> > +Defaults to 600 (10 minutes).
> 
> yes, now -t is nice and simple. just have to get the top bit right...
> 
> >  .It Fl Z Ar percent
> >  Automatically hibernate the system if no AC is connected and the
> >  estimated battery life is equal or below
> > 
> 
> jmc
> 



Re: apmd: manual tweak

2017-10-16 Thread Anton Lindqvist
On Mon, Oct 16, 2017 at 03:57:22PM +0100, Jason McIntyre wrote:
> On Mon, Oct 16, 2017 at 04:06:59PM +0200, Anton Lindqvist wrote:
> > Hi,
> > A proposal on improving the `-t` option documentation. Most importantly,
> > remove the recursive reference to the option being documented.
> > 
> > Comments? OK?
> > 
> > Index: apmd.8
> > ===
> > RCS file: /cvs/src/usr.sbin/apmd/apmd.8,v
> > retrieving revision 1.49
> > diff -u -p -r1.49 apmd.8
> > --- apmd.8  29 Aug 2017 06:13:20 -  1.49
> > +++ apmd.8  16 Oct 2017 14:05:02 -
> > @@ -107,14 +107,12 @@ and
> >  .Nm
> >  exits without monitoring the APM status.
> >  .It Fl t Ar seconds
> > -.Nm
> > -periodically polls the APM driver for the current power state.
> > -If the battery charge level changes substantially or the external power
> > -status changes, the new status is logged.
> > -The polling rate defaults to
> > -once per 10 minutes, but may be specified using the
> > -.Fl t
> > -command-line flag.
> > +Set the APM driver poll interval to
> > +.Ar seconds .
> > +Defaults to 10 minutes.
> > +After polling,
> > +the battery status is logged if the charge level changed substantially or 
> > the
> > +external power status changed.
> >  .It Fl Z Ar percent
> >  Automatically hibernate the system if no AC is connected and the
> >  estimated battery life is equal or below
> > 
> 
> hi.
> 
> i also dislike when options are documented this way, so i'm happy to see
> this change.
> 
> having said that, have you seen this paragraph near page top:
> 
>When the power status changes (battery is connected or
>disconnected) apmd fetches the current status and reports it
>via syslog(3) with logging facility LOG_DAEMON.
> 
> it looks a bit like we are describing logging in two places instead
> of one (and on top of that, the descriptions of when logging happens
> differ). i wonder whether it would make sense to more fully describe
> when logging happens (and that it happens after polling) in the
> paragraph i quoted, and that would allow for a very brief description
> of -t.
> 
> oh, i also dislike that the argument to -t is in seconds, but the
> default is given in minutes. minutes makes sense, but having the default
> in both might make it easier to both read and calculate a change.
> something like:
> 
>   The default is 600 (10 minutes).

Good call. Without knowing too much about the apmd internals, here's a
first stab. As I read the code, the period polling is also used by
auto{suspend,hibernate}. Is this worth mentioning? Since a more frequent
interval will increase the accuracy of the chosen action to perform?

Index: apmd.8
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.8,v
retrieving revision 1.49
diff -u -p -r1.49 apmd.8
--- apmd.8  29 Aug 2017 06:13:20 -  1.49
+++ apmd.8  16 Oct 2017 16:29:20 -
@@ -57,13 +57,17 @@ When resuming after suspend or standby,
 .Nm
 runs the appropriate configuration program (if one exists).
 .Pp
-When the power status changes
-(battery is connected or disconnected)
+Upon receiving an event from
+.Xr apm 4 ,
 .Nm
 fetches the current status and reports it via
 .Xr syslog 3
 with logging facility
 .Dv LOG_DAEMON .
+In addition,
+.Nm
+periodically polls the power status and reports significant changes to the
+battery charge level.
 .Pp
 The options are as follows:
 .Bl -tag -width Ds
@@ -107,14 +111,9 @@ and
 .Nm
 exits without monitoring the APM status.
 .It Fl t Ar seconds
-.Nm
-periodically polls the APM driver for the current power state.
-If the battery charge level changes substantially or the external power
-status changes, the new status is logged.
-The polling rate defaults to
-once per 10 minutes, but may be specified using the
-.Fl t
-command-line flag.
+Set the APM driver poll interval to
+.Ar seconds .
+Defaults to 600 (10 minutes).
 .It Fl Z Ar percent
 Automatically hibernate the system if no AC is connected and the
 estimated battery life is equal or below



apmd: manual tweak

2017-10-16 Thread Anton Lindqvist
Hi,
A proposal on improving the `-t` option documentation. Most importantly,
remove the recursive reference to the option being documented.

Comments? OK?

Index: apmd.8
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.8,v
retrieving revision 1.49
diff -u -p -r1.49 apmd.8
--- apmd.8  29 Aug 2017 06:13:20 -  1.49
+++ apmd.8  16 Oct 2017 14:05:02 -
@@ -107,14 +107,12 @@ and
 .Nm
 exits without monitoring the APM status.
 .It Fl t Ar seconds
-.Nm
-periodically polls the APM driver for the current power state.
-If the battery charge level changes substantially or the external power
-status changes, the new status is logged.
-The polling rate defaults to
-once per 10 minutes, but may be specified using the
-.Fl t
-command-line flag.
+Set the APM driver poll interval to
+.Ar seconds .
+Defaults to 10 minutes.
+After polling,
+the battery status is logged if the charge level changed substantially or the
+external power status changed.
 .It Fl Z Ar percent
 Automatically hibernate the system if no AC is connected and the
 estimated battery life is equal or below



Re: dirty1, for those w/vanishing memory to keep up w/multiple cu's

2017-10-16 Thread Anton Lindqvist
On Sun, Oct 15, 2017 at 10:45:53AM +0300, Artturi Alm wrote:
> Hi,
> 
> i've been saving time+frustration for having had this, but i'm not proposing
> anything, i'm aware how this what most likely exists only because of how badly
> scripts stick w/me across reinstalls/need of backups, compared to anything i
> push into my /usr/src.
> 
> for the example below i used boardnames, while i usually use the active branch
> name/window group it's on, so this can help locating them too xD
> 
> imagine making any use of the list below w/o "-t tag2remind" when in hurry to
> check what you've got connected/powered on, i think i've never chosen the 
> right
> cu process to kill if i've lost an xterm/tmux and started guessing one by 
> one..
> those cuaU*s are all usb adapters behind a couple of uhub*s, so they keep
> changing portnumbers almost once a week or so, reboot or accidentally
> unplugged uhub etc..

Not a complete solution, but for deterministic port numbers; you could
look into remote(5). Here's mine:

$ cat /etc/remote
apu:dv=/dev/cuaU0:br#115200
$ cu apu



acpi: free() size

2017-10-11 Thread Anton Lindqvist
Hi,
Add missing size to free(), tested on amd64.

Comments? OK?

Index: dsdt.c
===
RCS file: /cvs/src/sys/dev/acpi/dsdt.c,v
retrieving revision 1.234
diff -u -p -r1.234 dsdt.c
--- dsdt.c  28 May 2017 15:36:45 -  1.234
+++ dsdt.c  11 Oct 2017 07:51:57 -
@@ -452,7 +452,7 @@ _acpi_os_free(void *ptr, const char *fn,
 #endif
 
dnprintf(99, "free: %p %s:%d\n", sptr, fn, line);
-   free(sptr, M_ACPI, 0);
+   free(sptr, M_ACPI, sptr->size + sizeof(*sptr));
}
 }



Re: ctags(1): missing space between tag and line number

2017-10-10 Thread Anton Lindqvist
Ping

On Thu, Oct 05, 2017 at 10:41:09AM +0200, Anton Lindqvist wrote:
> Hi,
> Running `ctags -x` on a file including a tag which satisfies strlen(tag)
> >= 16 and line number >= 1000 corrupts the output since there's no space
> between the tag and line number. Therefore, add a space between them
> just like ectags and uctags in ports does.
> 
>   $ ctags -x /sys/dev/usb/umass.c | grep dump # before
>   umass_bbb_dump_cbw1830 /sys/dev/usb/umass.c umass_bbb_dump_cbw(struct 
> umass_softc *sc, struct umass_bbb_cbw *cbw)
>   umass_bbb_dump_csw1850 /sys/dev/usb/umass.c umass_bbb_dump_csw(struct 
> umass_softc *sc, struct umass_bbb_csw *csw)
>   umass_dump_buffer1867 /sys/dev/usb/umass.c umass_dump_buffer(struct 
> umass_softc *sc, u_int8_t *buffer, int buflen,
>   $ ./obj/ctags -x /sys/dev/usb/umass.c | grep dump # after
>   umass_bbb_dump_cbw 1830 /sys/dev/usb/umass.c umass_bbb_dump_cbw(struct 
> umass_softc *sc, struct umass_bbb_cbw *cbw)
>   umass_bbb_dump_csw 1850 /sys/dev/usb/umass.c umass_bbb_dump_csw(struct 
> umass_softc *sc, struct umass_bbb_csw *csw)
>   umass_dump_buffer 1867 /sys/dev/usb/umass.c umass_dump_buffer(struct 
> umass_softc *sc, u_int8_t *buffer, int buflen,
> 
> Comments? OK?
> 
> Index: print.c
> ===
> RCS file: /cvs/src/usr.bin/ctags/print.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 print.c
> --- print.c   4 Mar 2012 04:05:15 -   1.7
> +++ print.c   5 Oct 2017 08:39:54 -
> @@ -99,7 +99,7 @@ put_entries(NODE *node)
>   printf("%s %s %d\n",
>   node->entry, node->file, (node->lno + 63) / 64);
>   else if (xflag)
> - printf("%-16s%4d %-16s %s\n",
> + printf("%-16s %4d %-16s %s\n",
>   node->entry, node->lno, node->file, node->pat);
>   else
>   fprintf(outf, "%s\t%s\t%c^%s%c\n",



ctags(1): missing space between tag and line number

2017-10-05 Thread Anton Lindqvist
Hi,
Running `ctags -x` on a file including a tag which satisfies strlen(tag)
>= 16 and line number >= 1000 corrupts the output since there's no space
between the tag and line number. Therefore, add a space between them
just like ectags and uctags in ports does.

  $ ctags -x /sys/dev/usb/umass.c | grep dump # before
  umass_bbb_dump_cbw1830 /sys/dev/usb/umass.c umass_bbb_dump_cbw(struct 
umass_softc *sc, struct umass_bbb_cbw *cbw)
  umass_bbb_dump_csw1850 /sys/dev/usb/umass.c umass_bbb_dump_csw(struct 
umass_softc *sc, struct umass_bbb_csw *csw)
  umass_dump_buffer1867 /sys/dev/usb/umass.c umass_dump_buffer(struct 
umass_softc *sc, u_int8_t *buffer, int buflen,
  $ ./obj/ctags -x /sys/dev/usb/umass.c | grep dump # after
  umass_bbb_dump_cbw 1830 /sys/dev/usb/umass.c umass_bbb_dump_cbw(struct 
umass_softc *sc, struct umass_bbb_cbw *cbw)
  umass_bbb_dump_csw 1850 /sys/dev/usb/umass.c umass_bbb_dump_csw(struct 
umass_softc *sc, struct umass_bbb_csw *csw)
  umass_dump_buffer 1867 /sys/dev/usb/umass.c umass_dump_buffer(struct 
umass_softc *sc, u_int8_t *buffer, int buflen,

Comments? OK?

Index: print.c
===
RCS file: /cvs/src/usr.bin/ctags/print.c,v
retrieving revision 1.7
diff -u -p -r1.7 print.c
--- print.c 4 Mar 2012 04:05:15 -   1.7
+++ print.c 5 Oct 2017 08:39:54 -
@@ -99,7 +99,7 @@ put_entries(NODE *node)
printf("%s %s %d\n",
node->entry, node->file, (node->lno + 63) / 64);
else if (xflag)
-   printf("%-16s%4d %-16s %s\n",
+   printf("%-16s %4d %-16s %s\n",
node->entry, node->lno, node->file, node->pat);
else
fprintf(outf, "%s\t%s\t%c^%s%c\n",



Re: [patch] ftp(1): change mtime for http/https links

2017-09-23 Thread Anton Lindqvist
On Fri, Sep 22, 2017 at 11:56:51PM +0100, Raf Czlonka wrote:
> On Fri, Sep 22, 2017 at 11:01:57PM BST, Jesper Wallin wrote:
> > Hi all,
> > 
> > My morning routine consists of downloading the latest snapshot files and
> > running the upgrade.  To avoid wasting bandwidth and time, I check the
> > local modification time of INSTALL.amd64, fetch the remote one and check
> > if the modification time has changed.
> 
> Hi Jesper,
> 
> This is unrelated to your diff but what I do instead is to check
> the BUILDINFO file - it's tiny and all the information you need,
> is already there.

Continuing OT: I also rely on the contents of BUILDINFO to determine if
a new snapshot is available:

  $ head `which snapshot`
  #!/bin/sh

  url="$(head -1 /etc/installurl)/snapshots/$(uname -m)"

  [ -e BUILDINFO ] && ftp -V -o- "${url}/BUILDINFO" | cmp -s BUILDINFO - && 
exit 1



clang-local(1): typo

2017-09-13 Thread Anton Lindqvist
undesriable -> undesirable; ok?

Index: clang-local.1
===
RCS file: /cvs/src/share/man/man1/clang-local.1,v
retrieving revision 1.9
diff -u -p -r1.9 clang-local.1
--- clang-local.1   29 Jul 2017 21:01:13 -  1.9
+++ clang-local.1   13 Sep 2017 12:11:17 -
@@ -93,7 +93,7 @@ The
 .Xr valloc 3
 and
 .Xr free 3
-builtins are disabled to prevent undesriable optimizations of calls to
+builtins are disabled to prevent undesirable optimizations of calls to
 these functions.
 .El
 .Sh SEE ALSO



csh(1): zap redundant assignment

2017-08-30 Thread Anton Lindqvist
Comments? OK?

Index: lex.c
===
RCS file: /cvs/src/bin/csh/lex.c,v
retrieving revision 1.24
diff -u -p -r1.24 lex.c
--- lex.c   23 Jan 2017 04:53:15 -  1.24
+++ lex.c   30 Aug 2017 07:09:50 -
@@ -1426,7 +1426,6 @@ again:
off = (int) feobp % BUFSIZ;
roomleft = BUFSIZ - off;
 
-   roomleft = BUFSIZ - off;
for (;;) {
if (filec && intty) {
c = numleft ? numleft : tenex(ttyline, BUFSIZ);



csh(1): paste long lines

2017-08-28 Thread Anton Lindqvist
Hi,
deraadt@ discovered that pasting long lines in csh with filec enabled is
currently broken. NUL-terminating the input buffer from tenex()
instructs csh that the buffer contains a complete command. This is wrong
and should only happen when buffer is not full, otherwise more data has
to be read in order form a complete command.

While here, I did also discover that the prompt is printed again when
the input exceeds the input buffer. This is also true when inputting a
line continuation (i.e. backslash). In order to prevent printing the
prompt again, only print the prompt in tenex() when `needprompt == 1`.

Comments? OK?

Index: csh.c
===
RCS file: /cvs/src/bin/csh/csh.c,v
retrieving revision 1.40
diff -u -p -r1.40 csh.c
--- csh.c   26 Jul 2017 19:15:09 -  1.40
+++ csh.c   28 Aug 2017 07:10:31 -
@@ -1008,7 +1008,8 @@ process(bool catch)
 * read fresh stuff. Otherwise, we are rereading input and don't
 * need or want to prompt.
 */
-   if (!filec && aret == F_SEEK && fseekp == feobp)
+   needprompt = aret == F_SEEK && fseekp == feobp;
+   if (!filec && needprompt)
printprompt();
(void) fflush(cshout);
}
Index: csh.h
===
RCS file: /cvs/src/bin/csh/csh.h,v
retrieving revision 1.29
diff -u -p -r1.29 csh.h
--- csh.h   22 Jul 2017 09:37:21 -  1.29
+++ csh.h   28 Aug 2017 07:10:31 -
@@ -108,6 +108,7 @@ booltimflg; /* Time the next waite
 boolhavhash;   /* path hashing is available */
 
 boolfilec; /* doing filename expansion */
+boolneedprompt;/* print prompt, used by filec */
 
 /*
  * Global i/o info
Index: file.c
===
RCS file: /cvs/src/bin/csh/file.c,v
retrieving revision 1.33
diff -u -p -r1.33 file.c
--- file.c  26 Jul 2017 19:20:51 -  1.33
+++ file.c  28 Aug 2017 07:10:31 -
@@ -779,8 +779,11 @@ tenex(Char *inputline, int inputline_siz
cl.size = sizeof(buf);
if (tio->c_lflag & ALTWERASE)
cl.flags |= CL_ALTWERASE;
-   cl.flags |= CL_PROMPT;
-   cl_flush();  /* print prompt */
+   if (needprompt) {
+   needprompt = 0;
+   cl.flags |= CL_PROMPT;
+   cl_flush();
+   }
 
for (;;) {
if ((c = cl_getc()) == 0)
@@ -799,7 +802,14 @@ tenex(Char *inputline, int inputline_siz
 
for (i = 0; i < cl.len; i++)
inputline[i] = cl.buf[i];
-   inputline[i] = '\0';
+   /*
+* NUL-terminating the buffer implies that it contains a complete
+* command ready to be executed. Therefore, don't terminate if the
+* buffer is full since more characters must be read in order to form a
+* complete command.
+*/
+   if (i < sizeof(buf))
+   inputline[i] = '\0';
 
return cl.len;
 }
Index: func.c
===
RCS file: /cvs/src/bin/csh/func.c,v
retrieving revision 1.34
diff -u -p -r1.34 func.c
--- func.c  26 Jul 2017 19:15:09 -  1.34
+++ func.c  28 Aug 2017 07:10:32 -
@@ -589,7 +589,8 @@ search(int type, int level, Char *goal)
bseek();
 }
 do {
-   if (intty && !filec && fseekp == feobp && aret == F_SEEK)
+   needprompt = intty && fseekp == feobp && aret == F_SEEK;
+   if (!filec && needprompt)
(void) fprintf(cshout, "? "), (void) fflush(cshout);
aword[0] = 0;
(void) getword(aword);



uplcom(4): add adapter to manual

2017-08-07 Thread Anton Lindqvist
Hi,
I recently got an Aten UC232A (Prolific chipset) adapter which is
supported by uplcom.

Comments? OK?

Index: uplcom.4
===
RCS file: /cvs/src/share/man/man4/uplcom.4,v
retrieving revision 1.26
diff -u -p -r1.26 uplcom.4
--- uplcom.427 Apr 2009 20:06:27 -  1.26
+++ uplcom.47 Aug 2017 18:33:11 -
@@ -53,6 +53,7 @@ driver supports the following adapters:
 .Pp
 .Bl -tag -width Ds -offset indent -compact
 .It Alcatel One Touch 535/735
+.It Aten UC232A
 .It Belkin F5U257
 .It Digitus DA-70145
 .It ELECOM UC-SGT



  1   2   >