Re: xidle: launching program on timeout without active-area

[Rob Pierce]

On Tue, Sep 04, 2018 at 01:54:10PM +0200, Claudio Jeker wrote:
> On Mon, Sep 03, 2018 at 03:49:46PM +0200, Sebastien Marie wrote:
> > ping
> 
> I like it, OK claudio@ but I'm not really a X person.

I also like it and it works well for me. ok rob@ with the same caveat.

Rob

> > On Tue, Aug 14, 2018 at 06:15:08AM +0200, Sebastien Marie wrote:
> > > ping
> > > 
> > > On Wed, Jul 25, 2018 at 02:13:49PM +0200, Sebastien Marie wrote:
> > > > On Wed, Jul 25, 2018 at 12:55:48PM +0200, Claudio Jeker wrote:
> > > > > On Wed, Jul 25, 2018 at 12:27:29PM +0200, Sebastien Marie wrote:
> > > > > > On Mon, Jul 16, 2018 at 11:37:41AM +0200, Sebastien Marie wrote:
> > > > > > 
> > > > > > > xidle(1) seems great for such purpose. But I didn't found a way 
> > > > > > > to just
> > > > > > > use timeout and not also an active area (a corner where the 
> > > > > > > program is
> > > > > > > launched if pointer stays inside few seconds).
> > > > > > > 
> > > > > > > The following diff tries to implement a way to disable the active 
> > > > > > > area
> > > > > > > without being too intrusive.
> > > > > > > 
> > > > > > > For that, I used the `delay' parameter ("Specify the number of 
> > > > > > > seconds
> > > > > > > the pointer has to be in the given position before running the
> > > > > > > program."), to allow value -1, and make it to discard the event.
> > > > > > > 
> > > > > > > Does it make sens ? Or any proposition to more straighfull 
> > > > > > > approch ?
> > > > > > > 
> > > > > 
> > > > > I would love to be able to use xidle without active area but I have 
> > > > > to say
> > > > > that your approach with -1 as delay seems a bit like a hack. Wouldn't 
> > > > > it
> > > > > be better to add a -no option to disable the corners all together? 
> > > > > Maybe
> > > > > even make that the default instead of -nw?
> > > > > 
> > > > 
> > > > the "-delay -1" approch was taken to avoid too instrusive change that
> > > > would clash with upstream (but are we upstream ? I didn't found xidle 
> > > > under
> > > > www.x.org). Anyway, the approch with -no option seems to not be too
> > > > intrusive neither and it is better for user point of vue.
> > > > 
> > > > So below a new diff with -no option.
> > > > 
> > > > When used, the -no flag that sets `position' variable to `none'. The
> > > > active area window is still created (its avoid to manage a new case
> > > > where `xi->win' could be NULL), but the window isn't mapper and no
> > > > event asked for EnterWindow.
> > > > 
> > > > Sending USR1 still work as intented.
> > > > 
> > > > I didn't change the default value for the position, but it could be
> > > > easily done (one line change in xidle.c and xidle.1).
> > > > 
> > > > -- 
> > > > Sebastien Marie
> > > > 
> > > > Index: xidle.1
> > > > ===
> > > > RCS file: /cvs/xenocara/app/xidle/xidle.1,v
> > > > retrieving revision 1.4
> > > > diff -u -p -r1.4 xidle.1
> > > > --- xidle.1 9 Nov 2017 19:13:03 -   1.4
> > > > +++ xidle.1 25 Jul 2018 11:54:13 -
> > > > @@ -35,7 +35,7 @@
> > > >  .Op Fl area Ar pixels
> > > >  .Op Fl delay Ar secs
> > > >  .Op Fl display Ar display
> > > > -.Op Fl nw | ne | sw | se
> > > > +.Op Fl no | nw | ne | sw | se
> > > >  .Op Fl program Ar path
> > > >  .Op Fl timeout Ar secs
> > > >  .Ek
> > > > @@ -66,8 +66,8 @@ The default is 2 seconds.
> > > >  .It Fl display Ar display
> > > >  This argument allows you to specify the server to connect to; see
> > > >  .Xr X 7 .
> > > > -.It Fl nw | ne | sw | se
> > > > -Set the position to one of northwest, northeast, southwest, or 
> > > > southeast,
> > > > +.It Fl no | nw | ne | sw | se
> > > > +Set the position to one of none, northwest, northeast, southwest, or 
> > > > southeast,
> > > >  respectively.
> > > >  If no position is specified,
> > > >  the default is northwest.
> > > > @@ -100,7 +100,9 @@ Specify the number of seconds to wait be
> > > >  .Fl delay
> > > >  option.
> > > >  .It Sy position No (class Sy Position )
> > > > -Set the position to one of: "nw", "ne", "sw", or "se"; see 
> > > > descriptions of the
> > > > +Set the position to one of: "no", "nw", "ne", "sw", or "se"; see 
> > > > descriptions
> > > > +of the
> > > > +.Fl no ,
> > > >  .Fl nw ,
> > > >  .Fl ne ,
> > > >  .Fl sw ,
> > > > Index: xidle.c
> > > > ===
> > > > RCS file: /cvs/xenocara/app/xidle/xidle.c,v
> > > > retrieving revision 1.5
> > > > diff -u -p -r1.5 xidle.c
> > > > --- xidle.c 20 Aug 2017 16:43:25 -  1.5
> > > > +++ xidle.c 25 Jul 2018 11:59:40 -
> > > > @@ -53,7 +53,8 @@ enum {
> > > > north = 0x01,
> > > > south = 0x02,
> > > > east  = 0x04,
> > > > -   west  = 0x08
> > > > +   west  = 0x08,
> > > > +   none  = 0x10,
> > > >  };
> > > >  
> > > >  enum { XIDLE_LOCK = 1, XIDLE_DIE = 2 };
> > > > @@ -84,6 +85,7 @@ static XrmOptionDescRec opts[] = {

Re: Remove unused variable in usr.bin/openssl/apps.c

[Rob Pierce]

On Thu, Aug 16, 2018 at 06:14:06PM +0800, Nan Xiao wrote:
> Hi tech@,
> 
> The `free_out' variable seems redundant, so this patch removes it:
> 
> Index: apps.c
> ===
> RCS file: /cvs/src/usr.bin/openssl/apps.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 apps.c
> --- apps.c7 Feb 2018 08:57:25 -   1.47
> +++ apps.c16 Aug 2018 09:18:43 -
> @@ -2050,11 +2050,9 @@ policies_print(BIO *out, X509_STORE_CTX
>  {
>   X509_POLICY_TREE *tree;
>   int explicit_policy;
> - int free_out = 0;
> 
>   if (out == NULL) {
>   out = BIO_new_fp(stderr, BIO_NOCLOSE);
> - free_out = 1;
>   }
>   tree = X509_STORE_CTX_get0_policy_tree(ctx);
>   explicit_policy = X509_STORE_CTX_get_explicit_policy(ctx);
> 
> -- 
> Best Regards
> Nan Xiao

Committed. Thanks!

Rob

remove some end-of-content code from ber api

[Rob Pierce]

As per X.690, "the end-of-contents octets shall be present if the length is
encoded as specified in 8.1.3.6, otherwise they shall not be present", i.e.
only used with indefinite length encoding. Since we do not support indefinite
length encoding, I thought it may make sense to remove some corresponding code
from the ber api.

The end-of-content type under the universal class has a special meaning; if
applications wish to make use of a similar, non-universal content value they
are free to do so without assistance from the ber api. It would appear that
ldap still wants to use the "eoc" functions, but I believe simply checking for
a ber element length of zero should suffice in these cases (i.e. aldap.c).

I think the RFC 2411 comment referenced in usr.sbin/ldapd/modify.c below should
be referring to RFC 4511, but I will send a separate diff proposal for that.

The ldapd and snmpd regression tests pass for me with these changes.

I am only looking for feedback and testing at this time. I may be wrong with
this approach, so feel free to correct me if you feel this is a misguided
change.

All such (DER like) restrictions enforced by the ber api will be clarified in
an upcoming ber.3 diff.

Regards,

Rob

Index: usr.bin/ldap/aldap.c
===
RCS file: /cvs/src/usr.bin/ldap/aldap.c,v
retrieving revision 1.4
diff -u -p -r1.4 aldap.c
--- usr.bin/ldap/aldap.c31 Jul 2018 11:37:18 -  1.4
+++ usr.bin/ldap/aldap.c12 Aug 2018 17:39:52 -
@@ -568,7 +568,7 @@ aldap_count_attrs(struct aldap_message *
return (-1);
 
for (i = 0, a = msg->body.search.attrs;
-   a != NULL && ber_get_eoc(a) != 0;
+   a != NULL && a->be_len != 0;
i++, a = a->be_next)
;
 
@@ -616,7 +616,7 @@ aldap_next_attr(struct aldap_message *ms
 
LDAP_DEBUG("attr", msg->body.search.iter);
 
-   if (ber_get_eoc(msg->body.search.iter) == 0)
+   if (msg->body.search.iter->be_len == 0)
goto notfound;
 
if (ber_scanf_elements(msg->body.search.iter, "{s(e)}e", , , )
@@ -654,7 +654,7 @@ aldap_match_attr(struct aldap_message *m
for (a = msg->body.search.attrs;;) {
if (a == NULL)
goto notfound;
-   if (ber_get_eoc(a) == 0)
+   if (a->be_len == 0)
goto notfound;
if (ber_scanf_elements(a, "{s(e", , ) != 0)
goto fail;
Index: usr.bin/ldap/ber.c
===
RCS file: /cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.18
diff -u -p -r1.18 ber.c
--- usr.bin/ldap/ber.c  3 Aug 2018 01:51:28 -   1.18
+++ usr.bin/ldap/ber.c  12 Aug 2018 17:39:52 -
@@ -359,28 +359,6 @@ ber_get_null(struct ber_element *elm)
return 0;
 }
 
-struct ber_element *
-ber_add_eoc(struct ber_element *prev)
-{
-   struct ber_element *elm;
-
-   if ((elm = ber_get_element(BER_TYPE_EOC)) == NULL)
-   return NULL;
-
-   ber_link_elements(prev, elm);
-
-   return elm;
-}
-
-int
-ber_get_eoc(struct ber_element *elm)
-{
-   if (elm->be_encoding != BER_TYPE_EOC)
-   return -1;
-
-   return 0;
-}
-
 size_t
 ber_oid2ber(struct ber_oid *o, u_int8_t *buf, size_t len)
 {
@@ -629,11 +607,6 @@ ber_printf_elements(struct ber_element *
case ')':
ber = sub;
break;
-   case '.':
-   if ((e = ber_add_eoc(ber)) == NULL)
-   goto fail;
-   ber = e;
-   break;
default:
break;
}
@@ -737,11 +710,6 @@ ber_scanf_elements(struct ber_element *b
goto fail;
ret++;
break;
-   case '.':
-   if (ber->be_encoding != BER_TYPE_EOC)
-   goto fail;
-   ret++;
-   break;
case 'p':
pos = va_arg(ap, off_t *);
*pos = ber_getpos(ber);
@@ -990,7 +958,10 @@ ber_dump_element(struct ber *ber, struct
ber_write(ber, root->be_val, root->be_len);
break;
case BER_TYPE_NULL: /* no payload */
+   break;
case BER_TYPE_EOC:
+   if (root->be_class == BER_CLASS_UNIVERSAL)
+   return -1;
break;
case BER_TYPE_SEQUENCE:
case BER_TYPE_SET:
@@ -1202,6 +1173,8 @@ ber_read_element(struct ber *ber, struct
 
switch (elm->be_encoding) {
case BER_TYPE_EOC:  /* End-Of-Content */
+   if (class == BER_CLASS_UNIVERSAL)
+   return -1;
break;
case BER_TYPE_BOOLEAN:
case 

change ber_write_elements to return ssize_t

[Rob Pierce]

In aldap.c, tls_write(2) and write(2) also return ssize_t, so both error and
wrote have been changed accordingly.

ok?

Index: usr.bin/ldap/aldap.c
===
RCS file: /cvs/src/usr.bin/ldap/aldap.c,v
retrieving revision 1.4
diff -u -p -r1.4 aldap.c
--- usr.bin/ldap/aldap.c31 Jul 2018 11:37:18 -  1.4
+++ usr.bin/ldap/aldap.c12 Aug 2018 01:53:10 -
@@ -126,10 +126,10 @@ aldap_tls(struct aldap *ldap, struct tls
 int
 aldap_send(struct aldap *ldap, struct ber_element *root)
 {
-   int error, wrote;
void *ptr;
char *data;
size_t len, done;
+   ssize_t error, wrote;
 
len = ber_calc_len(root);
error = ber_write_elements(>ber, root);
@@ -311,7 +311,7 @@ int
 aldap_create_page_control(struct ber_element *elm, int size,
 struct aldap_page_control *page)
 {
-   int len;
+   ssize_t len;
struct ber c;
struct ber_element *ber = NULL;
 
Index: usr.bin/ldap/ber.c
===
RCS file: /cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.18
diff -u -p -r1.18 ber.c
--- usr.bin/ldap/ber.c  3 Aug 2018 01:51:28 -   1.18
+++ usr.bin/ldap/ber.c  12 Aug 2018 01:53:10 -
@@ -802,7 +802,7 @@ ber_get_writebuf(struct ber *b, void **b
  * >=0 number of bytes written
  * -1  on failure and sets errno
  */
-int
+ssize_t
 ber_write_elements(struct ber *ber, struct ber_element *root)
 {
size_t len;
Index: usr.bin/ldap/ber.h
===
RCS file: /cvs/src/usr.bin/ldap/ber.h,v
retrieving revision 1.6
diff -u -p -r1.6 ber.h
--- usr.bin/ldap/ber.h  3 Aug 2018 01:51:28 -   1.6
+++ usr.bin/ldap/ber.h  12 Aug 2018 01:53:10 -
@@ -124,7 +124,7 @@ int  ber_string2oid(const char *, stru
 struct ber_element *ber_printf_elements(struct ber_element *, char *, ...);
 int ber_scanf_elements(struct ber_element *, char *, ...);
 ssize_t ber_get_writebuf(struct ber *, void **);
-int ber_write_elements(struct ber *, struct ber_element *);
+ssize_t ber_write_elements(struct ber *, struct 
ber_element *);
 voidber_set_readbuf(struct ber *, void *, size_t);
 struct ber_element *ber_read_elements(struct ber *, struct ber_element *);
 off_t   ber_getpos(struct ber_element *);
cvs server: Diffing usr.sbin
cvs server: Diffing usr.sbin/ldapd
Index: usr.sbin/ldapd/ber.c
===
RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
retrieving revision 1.28
diff -u -p -r1.28 ber.c
--- usr.sbin/ldapd/ber.c3 Aug 2018 01:51:28 -   1.28
+++ usr.sbin/ldapd/ber.c12 Aug 2018 01:53:10 -
@@ -802,7 +802,7 @@ ber_get_writebuf(struct ber *b, void **b
  * >=0 number of bytes written
  * -1  on failure and sets errno
  */
-int
+ssize_t
 ber_write_elements(struct ber *ber, struct ber_element *root)
 {
size_t len;
Index: usr.sbin/ldapd/ber.h
===
RCS file: /cvs/src/usr.sbin/ldapd/ber.h,v
retrieving revision 1.7
diff -u -p -r1.7 ber.h
--- usr.sbin/ldapd/ber.h3 Aug 2018 01:51:28 -   1.7
+++ usr.sbin/ldapd/ber.h12 Aug 2018 01:53:10 -
@@ -124,7 +124,7 @@ int  ber_string2oid(const char *, stru
 struct ber_element *ber_printf_elements(struct ber_element *, char *, ...);
 int ber_scanf_elements(struct ber_element *, char *, ...);
 ssize_t ber_get_writebuf(struct ber *, void **);
-int ber_write_elements(struct ber *, struct ber_element *);
+ssize_t ber_write_elements(struct ber *, struct 
ber_element *);
 voidber_set_readbuf(struct ber *, void *, size_t);
 struct ber_element *ber_read_elements(struct ber *, struct ber_element *);
 off_t   ber_getpos(struct ber_element *);
Index: usr.sbin/ldapd/ldape.c
===
RCS file: /cvs/src/usr.sbin/ldapd/ldape.c,v
retrieving revision 1.29
diff -u -p -r1.29 ldape.c
--- usr.sbin/ldapd/ldape.c  31 Jul 2018 11:01:00 -  1.29
+++ usr.sbin/ldapd/ldape.c  12 Aug 2018 01:53:10 -
@@ -72,7 +72,7 @@ void
 send_ldap_extended_response(struct conn *conn, int msgid, unsigned int type,
 long long result_code, const char *extended_oid)
 {
-   int  rc;
+   ssize_t  rc;
struct ber_element  *root, *elm;
void*buf;
 
@@ -117,7 +117,7 @@ ldap_refer(struct request *req, const ch
struct referral *ref;
long longresult_code = LDAP_REFERRAL;
unsigned int type;
- 

avoid overflow in snmp message id

[Rob Pierce]

Prevent server side (snmpd) overflow for message id in the snmp header.

ok?

Index: snmpclient.c
===
RCS file: /cvs/src/usr.sbin/snmpctl/snmpclient.c,v
retrieving revision 1.16
diff -u -p -r1.16 snmpclient.c
--- snmpclient.c8 Aug 2018 18:50:38 -   1.16
+++ snmpclient.c10 Aug 2018 14:21:23 -
@@ -407,7 +407,7 @@ snmpc_sendreq(struct snmpc *sc, unsigned
erroridx = SNMPC_MAXREPETITIONS;
 
/* SNMP header */
-   sc->sc_msgid = arc4random();
+   sc->sc_msgid = arc4random() & 0x7fff;
if ((root = ber_add_sequence(NULL)) == NULL)
return (-1);
if ((b = ber_printf_elements(root, "ds{tddd{{O0}}",

Re: please test: unveil for ifconfig

[Rob Pierce]

- Original Message -
> From: "Rob Pierce" 
> To: "Bryan Steele" 
> Cc: "tech" 
> Sent: Thursday, August 2, 2018 1:30:15 PM
> Subject: Re: please test: unveil for ifconfig

> - Original Message -
> > From: "Bryan Steele" 
> > To: "tech" 
> > Sent: Thursday, August 2, 2018 1:24:48 PM
> > Subject: Re: please test: unveil for ifconfig

> > On Thu, Aug 02, 2018 at 07:04:31PM +0200, Florian Obser wrote:
> > > I have been told that this is going to fall into snaps soon. If you
> > > are doing weird (or normal) things with ifconfig, please test.

> > > In particular if you use rulefile.

> > > Thanks!

> > > diff --git ifconfig.c ifconfig.c
> > > index 9bfb1751aab..873aed5bcc7 100644
> > > --- ifconfig.c
> > > +++ ifconfig.c
> > > @@ -676,10 +676,13 @@ main(int argc, char *argv[])
> > > int create = 0;
> > > int Cflag = 0;
> > > int gflag = 0;
> > > + int found_rulefile = 0;
> > > int i;

> > > /* If no args at all, print all interfaces. */
> > > if (argc < 2) {
> > > + if (unveil(NULL, NULL) == -1)
> > > + err(1, "unveil");
> > > aflag = 1;
> > > printif(NULL, 0);
> > > return (0);
> > > @@ -721,6 +724,18 @@ main(int argc, char *argv[])
> > > } else if (strlcpy(name, *argv, sizeof(name)) >= IFNAMSIZ)
> > > errx(1, "interface name '%s' too long", *argv);
> > > argc--, argv++;
> > > +
> > > + for (i = 0; i < argc; i++) {
> > > + if (strcmp(argv[0], "rulefile") == 0) {
> > > + found_rulefile = 1;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + if (!found_rulefile)
> > > + if (unveil(NULL, NULL) == -1)
> > > + err(1, "unveil");
> > > +
> > > if (argc > 0) {
> > > for (afp = rafp = afs; rafp->af_name; rafp++)
> > > if (strcmp(rafp->af_name, *argv) == 0) {

> > Are these the only two unveil(2) calls in ifconfig(8)? If not mistaken,
> > unless you unveil something before the call to disable unveil, it never
> > actually restricts access to the filesystem, instead you need:

> > if (unveil("/", "") == -1)
> > err(1, "unveil");
> > if (unveil(NULL, NULL) == -1)
> > err(1, "unveil");

> > -Bryan.

> Ah, yes. unveil(NULL, NULL) does not activate unveil. A recent man page update
> captures this a little better.

> I will test again. with your addition.

> Rob

ifstated regress passes with modified unveil calls.

They are pretty basic use cases though.

Rob

Re: tweaks to namei.9

[Rob Pierce]

On Thu, Aug 02, 2018 at 03:15:14PM +0100, Jason McIntyre wrote:
> On Thu, Aug 02, 2018 at 01:58:38PM +0000, Rob Pierce wrote:
> > A little less wordy when introducing the namieidata structure.
> > 
> > Ok?
> > 
> > Index: namei.9
> > ===
> > RCS file: /cvs/src/share/man/man9/namei.9,v
> > retrieving revision 1.18
> > diff -u -p -r1.18 namei.9
> > --- namei.9 23 Nov 2015 17:53:57 -  1.18
> > +++ namei.9 2 Aug 2018 13:51:43 -
> > @@ -67,10 +67,9 @@ for name-to-inode conversion, in the day
> >  .Xr vfs 9
> >  interface was implemented.
> >  .Pp
> > -The arguments passed to the functions are encapsulated in the
> > +Arguments passed to these functions are encapsulated in the following
> >  .Em nameidata
> > -structure.
> > -It has the following structure:
> > +structure:
> >  .Bd -literal
> >  struct nameidata {
> >  /*
> > 
> 
> hi.
> 
> i'm not sure it's a big win - it's just another way of saying the same
> thing. but now it can be interpreted to mean that there are more than one
> type of namei structure.
> 
> the use of "structure" twice isn;t ideal though, i agree.
> 
> jmc

I agree, this is not a big win, but when I read four instances of "the" and two
instances of "structure" in two sentence where one would do, I start to lose
focus. I don't see how this could be misinterpreted, but maybe I am missing
something.

Rob

tweaks to namei.9

[Rob Pierce]

A little less wordy when introducing the namieidata structure.

Ok?

Index: namei.9
===
RCS file: /cvs/src/share/man/man9/namei.9,v
retrieving revision 1.18
diff -u -p -r1.18 namei.9
--- namei.9 23 Nov 2015 17:53:57 -  1.18
+++ namei.9 2 Aug 2018 13:51:43 -
@@ -67,10 +67,9 @@ for name-to-inode conversion, in the day
 .Xr vfs 9
 interface was implemented.
 .Pp
-The arguments passed to the functions are encapsulated in the
+Arguments passed to these functions are encapsulated in the following
 .Em nameidata
-structure.
-It has the following structure:
+structure:
 .Bd -literal
 struct nameidata {
 /*

relocate some public ber functions

[Rob Pierce]

Some public ber functions sneaked in below the internal functions comment. Move
them up so the comment regains its former truthiness.

Ok?

Index: usr.bin/ldap/ber.c
===
RCS file: /cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.14
diff -u -p -r1.14 ber.c
--- usr.bin/ldap/ber.c  13 Jul 2018 08:50:38 -  1.14
+++ usr.bin/ldap/ber.c  30 Jul 2018 20:20:40 -
@@ -430,6 +430,32 @@ ber_string2oid(const char *oidstr, struc
return (0);
 }
 
+int
+ber_oid_cmp(struct ber_oid *a, struct ber_oid *b)
+{
+   size_t   i;
+   for (i = 0; i < BER_MAX_OID_LEN; i++) {
+   if (a->bo_id[i] != 0) {
+   if (a->bo_id[i] == b->bo_id[i])
+   continue;
+   else if (a->bo_id[i] < b->bo_id[i]) {
+   /* b is a successor of a */
+   return (1);
+   } else {
+   /* b is a predecessor of a */
+   return (-1);
+   }
+   } else if (b->bo_id[i] != 0) {
+   /* b is larger, but a child of a */
+   return (2);
+   } else
+   break;
+   }
+
+   /* b and a are identical */
+   return (0);
+}
+
 struct ber_element *
 ber_add_oid(struct ber_element *prev, struct ber_oid *o)
 {
@@ -756,6 +782,15 @@ ber_scanf_elements(struct ber_element *b
 
 }
 
+ssize_t
+ber_get_writebuf(struct ber *b, void **buf)
+{
+   if (b->br_wbuf == NULL)
+   return -1;
+   *buf = b->br_wbuf;
+   return (b->br_wend - b->br_wbuf);
+}
+
 /*
  * write ber elements to the write buffer
  *
@@ -795,6 +830,13 @@ ber_write_elements(struct ber *ber, stru
return (len);
 }
 
+void
+ber_set_readbuf(struct ber *b, void *buf, size_t len)
+{
+   b->br_rbuf = b->br_rptr = buf;
+   b->br_rend = (u_int8_t *)buf + len;
+}
+
 /*
  * read ber elements from the read buffer
  *
@@ -896,6 +938,26 @@ ber_calc_len(struct ber_element *root)
return (root->be_len + size);
 }
 
+void
+ber_set_application(struct ber *b, unsigned long (*cb)(struct ber_element *))
+{
+   b->br_application = cb;
+}
+
+void
+ber_set_writecallback(struct ber_element *elm, void (*cb)(void *, size_t),
+void *arg)
+{
+   elm->be_cb = cb;
+   elm->be_cbarg = arg;
+}
+
+void
+ber_free(struct ber *b)
+{
+   free(b->br_wbuf);
+}
+
 /*
  * internal functions
  */
@@ -1204,42 +1266,6 @@ ber_read_element(struct ber *ber, struct
return totlen;
 }
 
-void
-ber_set_readbuf(struct ber *b, void *buf, size_t len)
-{
-   b->br_rbuf = b->br_rptr = buf;
-   b->br_rend = (u_int8_t *)buf + len;
-}
-
-ssize_t
-ber_get_writebuf(struct ber *b, void **buf)
-{
-   if (b->br_wbuf == NULL)
-   return -1;
-   *buf = b->br_wbuf;
-   return (b->br_wend - b->br_wbuf);
-}
-
-void
-ber_set_application(struct ber *b, unsigned long (*cb)(struct ber_element *))
-{
-   b->br_application = cb;
-}
-
-void
-ber_set_writecallback(struct ber_element *elm, void (*cb)(void *, size_t),
-void *arg)
-{
-   elm->be_cb = cb;
-   elm->be_cbarg = arg;
-}
-
-void
-ber_free(struct ber *b)
-{
-   free(b->br_wbuf);
-}
-
 static ssize_t
 ber_getc(struct ber *b, u_char *c)
 {
@@ -1265,30 +1291,4 @@ ber_read(struct ber *ber, void *buf, siz
ber->br_offs += len;
 
return len;
-}
-
-int
-ber_oid_cmp(struct ber_oid *a, struct ber_oid *b)
-{
-   size_t   i;
-   for (i = 0; i < BER_MAX_OID_LEN; i++) {
-   if (a->bo_id[i] != 0) {
-   if (a->bo_id[i] == b->bo_id[i])
-   continue;
-   else if (a->bo_id[i] < b->bo_id[i]) {
-   /* b is a successor of a */
-   return (1);
-   } else {
-   /* b is a predecessor of a */
-   return (-1);
-   }
-   } else if (b->bo_id[i] != 0) {
-   /* b is larger, but a child of a */
-   return (2);
-   } else
-   break;
-   }
-
-   /* b and a are identical */
-   return (0);
 }
Index: usr.sbin/ldapd/ber.c
===
RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
retrieving revision 1.24
diff -u -p -r1.24 ber.c
--- usr.sbin/ldapd/ber.c13 Jul 2018 08:50:38 -  1.24
+++ usr.sbin/ldapd/ber.c30 Jul 2018 20:21:58 -
@@ -430,6 +430,32 @@ ber_string2oid(const char *oidstr, struc
return (0);
 }
 
+int
+ber_oid_cmp(struct ber_oid *a, struct ber_oid *b)
+{
+   size_t   i;
+   for (i = 0; i < BER_MAX_OID_LEN; i++) {
+   if (a->bo_id[i] != 0) {
+   if (a->bo_id[i] == 

cleanup defunct prototype in snmpe.c

[Rob Pierce]

It looks like some code was shuffled around in revision 1.34 in which
snmpe_application was renamed to smi_application and this prototype was missed.

ok?

Index: snmpe.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.52
diff -u -p -r1.52 snmpe.c
--- snmpe.c 15 Apr 2018 11:57:29 -  1.52
+++ snmpe.c 23 Jul 2018 19:32:40 -
@@ -45,8 +45,6 @@ intsnmpe_parse(struct snmp_message *);
 voidsnmpe_tryparse(int, struct snmp_message *);
 int snmpe_parsevarbinds(struct snmp_message *);
 voidsnmpe_response(struct snmp_message *);
-unsigned long
-snmpe_application(struct ber_element *);
 voidsnmpe_sig_handler(int sig, short, void *);
 int snmpe_dispatch_parent(int, struct privsep_proc *, struct imsg *);
 int snmpe_bind(struct address *);

avoid vfprintf NULL errors in ldape.c log_debug()

[Rob Pierce]

Running the current ldapd regression tests result in the following (repeated)
errors in my /var/log/messages:

... ldapd: vfprintf %s NULL in "current bind dn = %s "

This is because regress/usr.sbin/ldapd/run-tests.pl is performing
unnecessary unbinds in END { }.

Though the regression test should probably be fixed, the following diff
ensures that log_debug is not called with a NULL argument.

Does this make sense?

Index: ldape.c
===
RCS file: /cvs/src/usr.sbin/ldapd/ldape.c,v
retrieving revision 1.27
diff -u -p -r1.27 ldape.c
--- ldape.c 15 May 2018 11:19:21 -  1.27
+++ ldape.c 3 Jul 2018 23:32:27 -
@@ -229,7 +229,8 @@ ldap_abandon(struct request *req)
 int
 ldap_unbind(struct request *req)
 {
-   log_debug("current bind dn = %s", req->conn->binddn);
+   log_debug("current bind dn = %s",
+   req->conn->binddn == NULL ? "" : req->conn->binddn);
conn_disconnect(req->conn);
request_free(req);
return -1;  /* don't send any response */

Re: call ber_read() from ber_getc() in ldap, ldapd, and ypldap

[Rob Pierce]

On Tue, Jul 03, 2018 at 09:25:06PM +0100, Stuart Henderson wrote:
> On 2018/07/03 22:17, Claudio Jeker wrote:
> > I have a hard time to understand why this is needed in snmpd.
> > For single char reads ber_readbuf(b, c, 1) and ber_read(b, c, 1) should do
> > exaclty the same. At least in the old code. I see that snmpd added br_offs
> > in a way that causes this breakage. The update of br_offs is handled in
> > the wrong place in my opinion.
> > 
> > I would prefer something like the appended diff.
> > This also removes the r == 0 check in ber_read since ber_readbuf cannot
> > return 0.  Can someone check against the snmpd usecase that failed?
> 
> Yes this fixes it, thanks. The use case that fails is "configure SNMPv3,
> try to use it".
> e.g.
> 
> user "username" authkey "authkey" enc aes enckey "privkey"
> 
> $ snmpwalk -v3 -l authPriv -a SHA -A authkey -u username -x AES -X privkey 
> hostname
> 
> > -- 
> > :wq Claudio
> > 
> > Index: ber.c
> > ===
> > RCS file: /cvs/src/usr.sbin/snmpd/ber.c,v
> > retrieving revision 1.37
> > diff -u -p -r1.37 ber.c
> > --- ber.c   1 Jul 2018 20:03:48 -   1.37
> > +++ ber.c   3 Jul 2018 20:16:13 -
> > @@ -1212,8 +1212,7 @@ ber_read_element(struct ber *ber, struct
> >  static ssize_t
> >  ber_readbuf(struct ber *b, void *buf, size_t nbytes)
> >  {
> > -   size_t   sz;
> > -   size_t   len;
> > +   size_t   sz, len;
> >  
> > if (b->br_rbuf == NULL)
> > return -1;
> > @@ -1227,6 +1226,7 @@ ber_readbuf(struct ber *b, void *buf, si
> >  
> > bcopy(b->br_rptr, buf, len);
> > b->br_rptr += len;
> > +   b->br_offs += len;
> >  
> > return (len);
> >  }
> > @@ -1271,7 +1271,7 @@ ber_free(struct ber *b)
> >  static ssize_t
> >  ber_getc(struct ber *b, u_char *c)
> >  {
> > -   return ber_read(b, c, 1);
> > +   return ber_readbuf(b, c, 1);
> >  }
> >  
> >  static ssize_t
> > @@ -1284,14 +1284,10 @@ ber_read(struct ber *ber, void *buf, siz
> > r = ber_readbuf(ber, b, remain);
> > if (r == -1)
> > return -1;
> > -   if (r == 0)
> > -   return (b - (u_char *)buf);
> > b += r;
> > remain -= r;
> > }
> > -   r = b - (u_char *)buf;
> > -   ber->br_offs += r;
> > -   return r;
> > +   return (b - (u_char *)buf);
> >  }
> >  
> >  int
> > 

Your diff also passes the snmpd regression tests here, including that specific
use case. LDAP also appears happy with this change. 

How about this larger diff (based on yours) for all ber.c instances?

Index: usr.bin/ldap/ber.c
===
RCS file: /cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.8
diff -u -p -r1.8 ber.c
--- usr.bin/ldap/ber.c  3 Jul 2018 18:49:10 -   1.8
+++ usr.bin/ldap/ber.c  3 Jul 2018 21:33:26 -
@@ -1212,8 +1212,7 @@ ber_read_element(struct ber *ber, struct
 static ssize_t
 ber_readbuf(struct ber *b, void *buf, size_t nbytes)
 {
-   size_t   sz;
-   size_t   len;
+   size_t   sz, len;
 
if (b->br_rbuf == NULL)
return -1;
@@ -1227,6 +1226,7 @@ ber_readbuf(struct ber *b, void *buf, si
 
bcopy(b->br_rptr, buf, len);
b->br_rptr += len;
+   b->br_offs += len;
 
return (len);
 }
@@ -1284,14 +1284,10 @@ ber_read(struct ber *ber, void *buf, siz
r = ber_readbuf(ber, b, remain);
if (r == -1)
return -1;
-   if (r == 0)
-   return (b - (u_char *)buf);
b += r;
remain -= r;
}
-   r = b - (u_char *)buf;
-   ber->br_offs += r;
-   return r;
+   return (b - (u_char *)buf);
 }
 
 int
Index: usr.sbin/ldapd/ber.c
===
RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
retrieving revision 1.18
diff -u -p -r1.18 ber.c
--- usr.sbin/ldapd/ber.c3 Jul 2018 18:49:10 -   1.18
+++ usr.sbin/ldapd/ber.c3 Jul 2018 21:33:31 -
@@ -1212,8 +1212,7 @@ ber_read_element(struct ber *ber, struct
 static ssize_t
 ber_readbuf(struct ber *b, void *buf, size_t nbytes)
 {
-   size_t   sz;
-   size_t   len;
+   size_t   sz, len;
 
if (b->br_rbuf == NULL)
return -1;
@@ -1227,6 +1226,7 @@ ber_readbuf(struct ber *b, void *buf, si
 
bcopy(b->br_rptr, buf, len);
b->br_rptr += len;
+   b->br_offs += len;
 
return (len);
 }
@@ -1284,14 +1284,10 @@ ber_read(struct ber *ber, void *buf, siz
r = ber_readbuf(ber, b, remain);
if (r == -1)
return -1;
-   if (r == 0)
-   return (b - (u_char *)buf);
b += r;
remain -= r;
}
-   r = b - (u_char *)buf;
-   ber->br_offs += r;
-   return r;
+   return (b - (u_char *)buf);
 }
 
 int
Index: 

Re: call ber_read() from ber_getc() in ldap, ldapd, and ypldap

[Rob Pierce]

On Sat, Jun 30, 2018 at 02:04:16PM -0400, Rob Pierce wrote:
> I recently committed a piece of BER code synchronizing in the wrong direction
> (i.e. from the ldap instances to the snmpd instance). sthen@ noticed a break
> in SNMPv3 authentication and reverted that part of the change. Thanks Stuart!
> 
> I just fixed some typos in the snmpd regression test which prevented me from
> noticing the problem. I am also the author of those typos... arg.
> 
> I believe the diff below synchronizes this piece of BER code in the right
> direction, from the snmpd instance to the ldap instances. I have done some
> testing against ldap and ldapd, but not ypldap. If anyone out there could
> perform further testing on and/or review the change to make sure it doesn't
> break anything that would be much appreciated.
> 
> Thanks!
> 
> Rob
> 
> Index: usr.bin/ldap/ber.c
> ===
> RCS file: /cvs/src/usr.bin/ldap/ber.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 ber.c
> --- usr.bin/ldap/ber.c29 Jun 2018 18:28:41 -  1.6
> +++ usr.bin/ldap/ber.c30 Jun 2018 17:50:06 -
> @@ -1240,7 +1240,7 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t
> 
> Index: usr.sbin/ldapd/ber.c
> ===
> RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 ber.c
> --- usr.sbin/ldapd/ber.c  29 Jun 2018 18:28:42 -  1.16
> +++ usr.sbin/ldapd/ber.c  30 Jun 2018 17:50:06 -
> @@ -1240,7 +1240,7 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t
> 
> Index: usr.sbin/ypldap/ber.c
> ===
> RCS file: /cvs/src/usr.sbin/ypldap/ber.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 ber.c
> --- usr.sbin/ypldap/ber.c 29 Jun 2018 18:28:42 -  1.18
> +++ usr.sbin/ypldap/ber.c 30 Jun 2018 17:50:06 -
> @@ -1240,7 +1240,7 @@ ber_free(struct ber *b)
>  static ssize_t
>  ber_getc(struct ber *b, u_char *c)
>  {
> - return ber_readbuf(b, c, 1);
> + return ber_read(b, c, 1);
>  }
>  
>  static ssize_t

After further review and testing I am now looking for ok's on this.

Ok?

Re: [PATCH] fix typo in if_aue.c

[Rob Pierce]

Ok rob@ 

> From: "Kevin Lo" 
> To: "tech" 
> Sent: Monday, July 2, 2018 10:23:39 AM
> Subject: [PATCH] fix typo in if_aue.c

> Hi,

> I've just noticed a little typo in the if_aue.c (s/read/write).
> The diff is below.

> Index: sys/dev/usb/if_aue.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_aue.c,v
> retrieving revision 1.106
> diff -u -p -u -p -r1.106 if_aue.c
> --- sys/dev/usb/if_aue.c 22 Jan 2017 10:17:39 - 1.106
> +++ sys/dev/usb/if_aue.c 2 Jul 2018 14:19:57 -
> @@ -509,7 +509,7 @@ aue_miibus_writereg(struct device *dev,
> }

> if (i == AUE_TIMEOUT) {
> - printf("%s: MII read timed out\n",
> + printf("%s: MII write timed out\n",
> sc->aue_dev.dv_xname);
> }
> aue_unlock_mii(sc);

Re: [PATCH] mos: nuke unused variable

[Rob Pierce]

Ok rob@ 

> From: "Kevin Lo" 
> To: "tech" 
> Sent: Monday, July 2, 2018 10:22:58 AM
> Subject: [PATCH] mos: nuke unused variable

> Ok ?

> Index: sys/dev/usb/if_mos.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_mos.c,v
> retrieving revision 1.38
> diff -u -p -u -p -r1.38 if_mos.c
> --- sys/dev/usb/if_mos.c 22 Jan 2017 10:17:39 - 1.38
> +++ sys/dev/usb/if_mos.c 2 Jul 2018 14:09:39 -
> @@ -376,15 +376,12 @@ int
> mos_miibus_readreg(struct device *dev, int phy, int reg)
> {
> struct mos_softc *sc = (void *)dev;
> - uWord val;
> int i,res;

> if (usbd_is_dying(sc->mos_udev)) {
> DPRINTF(("mos: dying\n"));
> return (0);
> }
> -
> - USETW(val, 0);

> mos_lock_mii(sc);

call ber_read() from ber_getc() in ldap, ldapd, and ypldap

[Rob Pierce]

I recently committed a piece of BER code synchronizing in the wrong direction
(i.e. from the ldap instances to the snmpd instance). sthen@ noticed a break
in SNMPv3 authentication and reverted that part of the change. Thanks Stuart!

I just fixed some typos in the snmpd regression test which prevented me from
noticing the problem. I am also the author of those typos... arg.

I believe the diff below synchronizes this piece of BER code in the right
direction, from the snmpd instance to the ldap instances. I have done some
testing against ldap and ldapd, but not ypldap. If anyone out there could
perform further testing on and/or review the change to make sure it doesn't
break anything that would be much appreciated.

Thanks!

Rob

Index: usr.bin/ldap/ber.c
===
RCS file: /cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.6
diff -u -p -r1.6 ber.c
--- usr.bin/ldap/ber.c  29 Jun 2018 18:28:41 -  1.6
+++ usr.bin/ldap/ber.c  30 Jun 2018 17:50:06 -
@@ -1240,7 +1240,7 @@ ber_free(struct ber *b)
 static ssize_t
 ber_getc(struct ber *b, u_char *c)
 {
-   return ber_readbuf(b, c, 1);
+   return ber_read(b, c, 1);
 }
 
 static ssize_t

Index: usr.sbin/ldapd/ber.c
===
RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
retrieving revision 1.16
diff -u -p -r1.16 ber.c
--- usr.sbin/ldapd/ber.c29 Jun 2018 18:28:42 -  1.16
+++ usr.sbin/ldapd/ber.c30 Jun 2018 17:50:06 -
@@ -1240,7 +1240,7 @@ ber_free(struct ber *b)
 static ssize_t
 ber_getc(struct ber *b, u_char *c)
 {
-   return ber_readbuf(b, c, 1);
+   return ber_read(b, c, 1);
 }
 
 static ssize_t

Index: usr.sbin/ypldap/ber.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ber.c,v
retrieving revision 1.18
diff -u -p -r1.18 ber.c
--- usr.sbin/ypldap/ber.c   29 Jun 2018 18:28:42 -  1.18
+++ usr.sbin/ypldap/ber.c   30 Jun 2018 17:50:06 -
@@ -1240,7 +1240,7 @@ ber_free(struct ber *b)
 static ssize_t
 ber_getc(struct ber *b, u_char *c)
 {
-   return ber_readbuf(b, c, 1);
+   return ber_read(b, c, 1);
 }
 
 static ssize_t

synchronize ber.c and ber.h across four consumers

[Rob Pierce]

This diff is a final synchronization of ber.c and ber.h.

It basically takes (2012) ber additions to snmpd and adds them back to ldap,
ldapd, and ypldap instances. See usr.sbin/snmpd/ber.c revision 1.24 commit
log for a summary of those changes (e.g. SNMPv2 traps, User-based Security
Model, callback for the USM HMAC calculations).

The ber code in this diff is not used by ldap, ldapd, or ypldap, but these
changes will make maintenance easier going forward.

The current (six) consumers of this ber implementation are as follows:
 - usr.bin/ldap
 - usr.sbin/ldapctl
 - usr.sbin/ldapd
 - usr.sbin/snmpctl
 - usr.sbin/snmpd
 - usr.sbin/ypldap

Ok?

Index: usr.bin/ldap/ber.c
===
RCS file: /cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.6
diff -u -p -r1.6 ber.c
--- usr.bin/ldap/ber.c  29 Jun 2018 18:28:41 -  1.6
+++ usr.bin/ldap/ber.c  29 Jun 2018 19:41:59 -
@@ -631,10 +631,11 @@ ber_scanf_elements(struct ber_element *b
va_list  ap;
int *d, level = -1;
unsigned long   *t;
-   long long   *i;
+   long long   *i, l;
void**ptr;
size_t  *len, ret = 0, n = strlen(fmt);
char**s;
+   off_t   *pos;
struct ber_oid  *o;
struct ber_element  *parent[_MAX_SEQ], **e;
 
@@ -656,6 +657,13 @@ ber_scanf_elements(struct ber_element *b
goto fail;
ret++;
break;
+   case 'd':
+   d = va_arg(ap, int *);
+   if (ber_get_integer(ber, ) == -1)
+   goto fail;
+   *d = l;
+   ret++;
+   break;
case 'e':
e = va_arg(ap, struct ber_element **);
*e = ber;
@@ -712,6 +720,11 @@ ber_scanf_elements(struct ber_element *b
goto fail;
ret++;
break;
+   case 'p':
+   pos = va_arg(ap, off_t *);
+   *pos = ber_getpos(ber);
+   ret++;
+   continue;
case '{':
case '(':
if (ber->be_encoding != BER_TYPE_SEQUENCE &&
@@ -821,6 +834,12 @@ ber_read_elements(struct ber *ber, struc
return root;
 }
 
+off_t
+ber_getpos(struct ber_element *elm)
+{
+   return elm->be_offs;
+}
+
 void
 ber_free_element(struct ber_element *root)
 {
@@ -893,6 +912,8 @@ ber_dump_element(struct ber *ber, struct
uint8_t u;
 
ber_dump_header(ber, root);
+   if (root->be_cb)
+   root->be_cb(root->be_cbarg, ber->br_wptr - ber->br_wbuf);
 
switch (root->be_encoding) {
case BER_TYPE_BOOLEAN:
@@ -1101,6 +1122,7 @@ ber_read_element(struct ber *ber, struct
 
elm->be_type = type;
elm->be_len = len;
+   elm->be_offs = ber->br_offs;/* element position within stream */
elm->be_class = class;
 
if (elm->be_encoding == 0) {
@@ -1232,6 +1254,15 @@ ber_set_application(struct ber *b, unsig
 }
 
 void
+ber_set_writecallback(struct ber_element *elm, void (*cb)(void *, size_t),
+void *arg)
+{
+   elm->be_cb = cb;
+   elm->be_cbarg = arg;
+}
+
+
+void
 ber_free(struct ber *b)
 {
free(b->br_wbuf);
@@ -1258,5 +1289,33 @@ ber_read(struct ber *ber, void *buf, siz
b += r;
remain -= r;
}
-   return (b - (u_char *)buf);
+   r = b - (u_char *)buf;
+   ber->br_offs += r;
+   return r;
+}
+
+int
+ber_oid_cmp(struct ber_oid *a, struct ber_oid *b)
+{
+   size_t   i;
+   for (i = 0; i < BER_MAX_OID_LEN; i++) {
+   if (a->bo_id[i] != 0) {
+   if (a->bo_id[i] == b->bo_id[i])
+   continue;
+   else if (a->bo_id[i] < b->bo_id[i]) {
+   /* b is a successor of a */
+   return (1);
+   } else {
+   /* b is a predecessor of a */
+   return (-1);
+   }   
+   } else if (b->bo_id[i] != 0) {
+   /* b is larger, but a child of a */
+   return (2);
+   } else
+   break;
+   }
+
+   /* b and a are identical */
+   return (0);
 }

Index: usr.bin/ldap/ber.h
===
RCS file: /cvs/src/usr.bin/ldap/ber.h,v
retrieving revision 1.3
diff -u -p -r1.3 ber.h
--- usr.bin/ldap/ber.h  29 Jun 2018 18:28:41 -  1.3
+++ usr.bin/ldap/ber.h  29 Jun 2018 19:41:59 -
@@ 

next step in synchronizing ber.c and ber.h

[Rob Pierce]

The following diff makes ber.c and ber.h identical across ldap, ldapd, and
ypldap, and slightly reduces the diff with snmpd.

It covers the evolution of a few scattered enhancements, including:

 - sync proscription of indefinite length BER encoding
 - sync consistent presence of ber_free_element()
 - sync correct parsing of '}' and ')' in ber_scanf_elements()
   (i.e. a "break" instead of a "continue")
 - sync use of ber_readbuf in ber_getc()

Ok?

Index: usr.bin/ldap/ber.c
===
RCS file: /cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.4
diff -u -p -r1.4 ber.c
--- usr.bin/ldap/ber.c  27 Jun 2018 20:38:10 -  1.4
+++ usr.bin/ldap/ber.c  27 Jun 2018 22:15:55 -
@@ -729,7 +729,7 @@ ber_scanf_elements(struct ber_element *b
goto fail;
ber = parent[level--];
ret++;
-   continue;
+   break;
default:
goto fail;
}
@@ -822,6 +822,19 @@ ber_read_elements(struct ber *ber, struc
 }
 
 void
+ber_free_element(struct ber_element *root)
+{
+   if (root->be_sub && (root->be_encoding == BER_TYPE_SEQUENCE ||
+   root->be_encoding == BER_TYPE_SET))
+   ber_free_elements(root->be_sub);
+   if (root->be_free && (root->be_encoding == BER_TYPE_OCTETSTRING ||
+   root->be_encoding == BER_TYPE_BITSTRING ||
+   root->be_encoding == BER_TYPE_OBJECT))
+   free(root->be_val);
+   free(root);
+}
+
+void
 ber_free_elements(struct ber_element *root)
 {
if (root->be_sub && (root->be_encoding == BER_TYPE_SEQUENCE ||
@@ -1030,6 +1043,12 @@ get_len(struct ber *b, ssize_t *len)
return 1;
}
 
+   if (u == 0x80) {
+   /* Indefinite length not supported. */
+   errno = EINVAL;
+   return -1;
+   }
+
n = u & ~BER_TAG_MORE;
if (sizeof(ssize_t) < n) {
errno = ERANGE;
@@ -1046,12 +1065,6 @@ get_len(struct ber *b, ssize_t *len)
if (s < 0) {
/* overflow */
errno = ERANGE;
-   return -1;
-   }
-
-   if (s == 0) {
-   /* invalid encoding */
-   errno = EINVAL;
return -1;
}
 
Index: usr.bin/ldap/ber.h
===
RCS file: /cvs/src/usr.bin/ldap/ber.h,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 ber.h
--- usr.bin/ldap/ber.h  13 Jun 2018 15:45:57 -  1.1.1.1
+++ usr.bin/ldap/ber.h  27 Jun 2018 22:15:55 -
@@ -119,6 +119,7 @@ ssize_t  ber_get_writebuf(struct ber *
 int ber_write_elements(struct ber *, struct ber_element *);
 voidber_set_readbuf(struct ber *, void *, size_t);
 struct ber_element *ber_read_elements(struct ber *, struct ber_element *);
+voidber_free_element(struct ber_element *);
 voidber_free_elements(struct ber_element *);
 size_t  ber_calc_len(struct ber_element *);
 voidber_set_application(struct ber *,

Index: usr.sbin/ldapd/ber.c
===
RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
retrieving revision 1.14
diff -u -p -r1.14 ber.c
--- usr.sbin/ldapd/ber.c27 Jun 2018 13:22:17 -  1.14
+++ usr.sbin/ldapd/ber.c27 Jun 2018 22:15:55 -
@@ -729,7 +729,7 @@ ber_scanf_elements(struct ber_element *b
goto fail;
ber = parent[level--];
ret++;
-   continue;
+   break;
default:
goto fail;
}

Index: usr.sbin/ypldap/ber.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ber.c,v
retrieving revision 1.16
diff -u -p -r1.16 ber.c
--- usr.sbin/ypldap/ber.c   27 Jun 2018 20:38:10 -  1.16
+++ usr.sbin/ypldap/ber.c   27 Jun 2018 22:15:56 -
@@ -729,7 +729,7 @@ ber_scanf_elements(struct ber_element *b
goto fail;
ber = parent[level--];
ret++;
-   continue;
+   break;
default:
goto fail;
}
@@ -822,6 +822,19 @@ ber_read_elements(struct ber *ber, struc
 }
 
 void
+ber_free_element(struct ber_element *root)
+{
+   if (root->be_sub && (root->be_encoding == BER_TYPE_SEQUENCE ||
+   root->be_encoding == BER_TYPE_SET))
+   ber_free_elements(root->be_sub);
+   if (root->be_free && (root->be_encoding == BER_TYPE_OCTETSTRING ||
+   root->be_encoding == BER_TYPE_BITSTRING ||
+   root->be_encoding == 

sync calloc call in ber.c

[Rob Pierce]

This ber.c change has been in ldapd since rev 1.1 and was applied to snmpd back
in 2012. The following diff applies the change to the ldap client and ypldap.

Ok?

Index: usr.bin/ldap/ber.c
===
RCS file: /cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.2
diff -u -p -r1.2 ber.c
--- usr.bin/ldap/ber.c  27 Jun 2018 13:22:17 -  1.2
+++ usr.bin/ldap/ber.c  27 Jun 2018 13:55:51 -
@@ -269,7 +269,7 @@ ber_add_nstring(struct ber_element *prev
struct ber_element *elm;
char *string;
 
-   if ((string = calloc(1, len)) == NULL)
+   if ((string = calloc(1, len + 1)) == NULL)
return NULL;
if ((elm = ber_get_element(BER_TYPE_OCTETSTRING)) == NULL) {
free(string);

Index: usr.sbin/ypldap/ber.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ber.c,v
retrieving revision 1.14
diff -u -p -r1.14 ber.c
--- usr.sbin/ypldap/ber.c   27 Jun 2018 13:22:17 -  1.14
+++ usr.sbin/ypldap/ber.c   27 Jun 2018 13:55:51 -
@@ -269,7 +269,7 @@ ber_add_nstring(struct ber_element *prev
struct ber_element *elm;
char *string;
 
-   if ((string = calloc(1, len)) == NULL)
+   if ((string = calloc(1, len + 1)) == NULL)
return NULL;
if ((elm = ber_get_element(BER_TYPE_OCTETSTRING)) == NULL) {
free(string);

ber.c fix for length calculations

[Rob Pierce]

It looks like a BER problem found while testing the new ldap client (with an
empty password) was already addressed in snmpd back in 2010 by martinh.

In LDAP under a CONTEXT class, 0 corresponds to LDAP_AUTH_SIMPLE. This is
currently interpreted as an EOC (end-of-content) and causes a
miscalculation of the length which results in unexpected behaviour.

Reyk pointed me in the right direction, and after much messing around I came
up with a solution which was close to the following snmpd commit.

>From the src/usr.sbin/snmpd/ber.c commit log:


revision 1.23
date: 2010/09/20 08:30:13;  author: martinh;  state: Exp;  lines: +3 -2;
Allow output of null values with a context class. This is used in SNMPv2 to
return an error exception value for a varbind result ("noSuchObject[0]
IMPLICIT NULL" in rfc1905).


However, BER_TYPE_EOC only applies to the UNIVERSAL class, so I think an
explicit check against BER_CLASS_UNIVERSAL is a better solution and works
across both ldap and snmp.

The following diff applies this change to ldap, ldapd, and ypldap, and brings
snmpd in line with this approach.

Thoughts?

Ok?

Index: usr.bin/ldap/ber.c
===
RCS file: /cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 ber.c
--- usr.bin/ldap/ber.c  13 Jun 2018 15:45:57 -  1.1.1.1
+++ usr.bin/ldap/ber.c  24 Jun 2018 21:37:39 -
@@ -861,7 +861,8 @@ ber_calc_len(struct ber_element *root)
size += ber_calc_len(root->be_next);
 
/* This is an empty element, do not use a minimal size */
-   if (root->be_type == BER_TYPE_EOC && root->be_len == 0)
+   if (root->be_class == BER_CLASS_UNIVERSAL &&
+   root->be_type == BER_TYPE_EOC && root->be_len == 0)
return (0);
 
return (root->be_len + size);

Index: usr.sbin/ldapd/ber.c
===
RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
retrieving revision 1.13
diff -u -p -r1.13 ber.c
--- usr.sbin/ldapd/ber.c8 Feb 2018 18:02:06 -   1.13
+++ usr.sbin/ldapd/ber.c24 Jun 2018 21:37:39 -
@@ -874,7 +874,8 @@ ber_calc_len(struct ber_element *root)
size += ber_calc_len(root->be_next);
 
/* This is an empty element, do not use a minimal size */
-   if (root->be_type == BER_TYPE_EOC && root->be_len == 0)
+   if (root->be_class == BER_CLASS_UNIVERSAL &&
+   root->be_type == BER_TYPE_EOC && root->be_len == 0)
return (0);
 
return (root->be_len + size);

Index: usr.sbin/ypldap/ber.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ber.c,v
retrieving revision 1.13
diff -u -p -r1.13 ber.c
--- usr.sbin/ypldap/ber.c   8 Feb 2018 18:02:06 -   1.13
+++ usr.sbin/ypldap/ber.c   24 Jun 2018 21:37:39 -
@@ -861,7 +861,8 @@ ber_calc_len(struct ber_element *root)
size += ber_calc_len(root->be_next);
 
/* This is an empty element, do not use a minimal size */
-   if (root->be_type == BER_TYPE_EOC && root->be_len == 0)
+   if (root->be_class == BER_CLASS_UNIVERSAL &&
+   root->be_type == BER_TYPE_EOC && root->be_len == 0)
return (0);
 
return (root->be_len + size);

Index: usr.sbin/snmpd/ber.c
===
RCS file: /cvs/src/usr.sbin/snmpd/ber.c,v
retrieving revision 1.32
diff -u -p -r1.32 ber.c
--- usr.sbin/snmpd/ber.c8 Feb 2018 18:02:06 -   1.32
+++ usr.sbin/snmpd/ber.c24 Jun 2018 21:37:39 -
@@ -880,7 +880,7 @@ ber_calc_len(struct ber_element *root)
size += ber_calc_len(root->be_next);
 
/* This is an empty element, do not use a minimal size */
-   if (root->be_class != BER_CLASS_CONTEXT &&
+   if (root->be_class == BER_CLASS_UNIVERSAL &&
root->be_type == BER_TYPE_EOC && root->be_len == 0)
return (0);
 

use __func__ in iked util.c log_debug

[Rob Pierce]

ok?

Index: util.c
===
RCS file: /cvs/src/sbin/iked/util.c,v
retrieving revision 1.35
diff -u -p -r1.35 util.c
--- util.c  13 Dec 2017 08:27:06 -  1.35
+++ util.c  22 Jun 2018 12:52:09 -
@@ -703,7 +703,7 @@ expand_string(char *label, size_t len, c
char *p, *q;
 
if ((tmp = calloc(1, len)) == NULL) {
-   log_debug("expand_string: calloc");
+   log_debug("%s: calloc", __func__);
return (-1);
}
p = q = label;
@@ -711,7 +711,7 @@ expand_string(char *label, size_t len, c
*q = '\0';
if ((strlcat(tmp, p, len) >= len) ||
(strlcat(tmp, repl, len) >= len)) {
-   log_debug("expand_string: string too long");
+   log_debug("%s: string too long", __func__);
free(tmp);
return (-1);
}
@@ -719,7 +719,7 @@ expand_string(char *label, size_t len, c
p = q;
}
if (strlcat(tmp, p, len) >= len) {
-   log_debug("expand_string: string too long");
+   log_debug("%s: string too long", __func__);
free(tmp);
return (-1);
}

diff for snmpd agentx.c

[Rob Pierce]

This brings snmpd agentx.c closer to relayd agentx.c.

In the remaining delta, I am not sure if the pdu context code should be removed
from the snmpd version or added to the relayd version.

Anyway, this is one step closer.

Ok?

Rob

Index: agentx.c
===
RCS file: /cvs/src/usr.sbin/snmpd/agentx.c,v
retrieving revision 1.12
diff -u -p -r1.12 agentx.c
--- agentx.c14 Feb 2018 12:43:07 -  1.12
+++ agentx.c10 Jun 2018 16:59:01 -
@@ -18,12 +18,8 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
-#include 
-
-#include 
 #include 
 #include 
 #include 
@@ -217,9 +213,14 @@ snmp_agentx_response(struct agentx_handl
 {
struct agentx_response_data resp;
 
-   if (snmp_agentx_read_response(pdu, ) == -1)
+   if (snmp_agentx_read_raw(pdu, , sizeof(resp)) == -1)
return (-1);
 
+   if (!snmp_agentx_byteorder_native(pdu->hdr)) {
+   resp.error = snmp_agentx_int16_byteswap(resp.error);
+   resp.index = snmp_agentx_int16_byteswap(resp.index);
+   }
+
h->error = resp.error;
if (resp.error != AGENTX_ERR_NONE)
return (-1);
@@ -227,20 +228,6 @@ snmp_agentx_response(struct agentx_handl
return (0);
 }
 
-int
-snmp_agentx_read_response(struct agentx_pdu *pdu, struct agentx_response_data 
*resp)
-{
-   if (snmp_agentx_read_raw(pdu, resp, sizeof(*resp)) == -1)
-   return (-1);
-
-   if (!snmp_agentx_byteorder_native(pdu->hdr)) {
-   resp->error = snmp_agentx_int16_byteswap(resp->error);
-   resp->index = snmp_agentx_int16_byteswap(resp->index);
-   }
-
-   return (0);
-}
-
 /*
  * Read the response PDU for an open operation.
  */
@@ -341,7 +328,7 @@ snmp_agentx_recv(struct agentx_handle *h
if (h->r == NULL) {
if ((h->r = snmp_agentx_pdu_alloc()) == NULL)
return (NULL);
-   h->r->datalen = 0;  /* XXX -- force this for receive 
buffers */
+   h->r->datalen = 0;  /* XXX force this for receive buffers */
}
pdu = h->r;
 
@@ -1038,7 +1025,7 @@ snmp_oid2string(struct snmp_oid *o, char
bzero(buf, len);
 
for (i = 0; i < o->o_n; i++) {
-   snprintf(str, sizeof(str), "%d", o->o_id[i]);
+   snprintf(str, sizeof(str), "%u", o->o_id[i]);
strlcat(buf, str, len);
if (i < (o->o_n - 1))
strlcat(buf, ".", len);
Index: control.c
===
RCS file: /cvs/src/usr.sbin/snmpd/control.c,v
retrieving revision 1.42
diff -u -p -r1.42 control.c
--- control.c   21 Apr 2017 13:50:23 -  1.42
+++ control.c   10 Jun 2018 16:59:01 -
@@ -537,7 +537,7 @@ control_dispatch_agentx(int fd, short ev
struct agentx_varbind_hdrvbhdr;
struct ber_element  **elm, **iter;
 
-   if (snmp_agentx_read_response(pdu, ) == -1) {
+   if (snmp_agentx_read_raw(pdu, , sizeof(resp)) == 
-1) {
msg->sm_error = SNMP_ERROR_GENERR;
goto dispatch;
}
Index: snmp.h
===
RCS file: /cvs/src/usr.sbin/snmpd/snmp.h,v
retrieving revision 1.14
diff -u -p -r1.14 snmp.h
--- snmp.h  11 Jun 2015 18:49:09 -  1.14
+++ snmp.h  10 Jun 2018 16:59:01 -
@@ -369,7 +369,6 @@ struct agentx_handle *
 struct agentx_handle *
snmp_agentx_fdopen(int, char *, struct snmp_oid *);
 intsnmp_agentx_response(struct agentx_handle *, struct agentx_pdu *);
-intsnmp_agentx_read_response(struct agentx_pdu *, struct 
agentx_response_data *);
 intsnmp_agentx_open_response(struct agentx_handle *, struct agentx_pdu *);
 struct agentx_pdu *
snmp_agentx_open_pdu(struct agentx_handle *, char *descr,

Re: update ifstated parser

[Rob Pierce]

On Mon, Feb 26, 2018 at 05:10:43PM -0600, Michael Graves wrote:
> Hello
> 
> I use ifstated(8) to track the state of the the external interface that is
> configured via dhcp and based upon the state, (re)configure a VXLAN
> interface.
> The ifstated.conf currently looks like
> 
> ===
> exif="em0"
> vxif="vxlan0"
> 
> init-state state_down
> 
> state state_up {
>   init {
> run "ifconfig vlxna0 up"
>   }
>   if ( "ifconfig em0 | grep -q inet" every 60 )
> run "sleep 30 && ifconfig vxlan0 tunnel `ifconfig em0 | \
>  sed -nre 's/.*inet ([^ ]+).*/\1/p'` \
> `dig +short name-of-remote-device`"
>   if $exif.link.down
> set-state state_down
> }
> 
> state state_down {
>   init {
> run "ifconfig vxlan0 down"
>   }
>   if $exif.link.up
> set-state state_up
> }
> ===
> 
> The problem I ran into is that when I tried to substitute the vxlan0 and em0
> entries with exif and vxif in the 'run' statements no macro expansion
> occurred.
> This patch allows macro expansion within the 'run' and 'if' statements.
> 
> I appreciate any feedback.
> Regards

> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/ifstated/parse.y,v
> retrieving revision 1.47
> diff -u -p -r1.47 parse.y
> --- parse.y   21 Aug 2017 17:38:55 -  1.47
> +++ parse.y   26 Feb 2018 22:47:11 -
> @@ -509,9 +509,10 @@ int
>  yylex(void)
>  {
>   u_char   buf[8096];
> - u_char  *p, *val;
> + u_char  *p, *p1, *val;
>   int  quotec, next, c;
>   int  token;
> + size_t  x;
>  
>  top:
>   p = buf;
> @@ -575,6 +576,35 @@ top:
>   } else if (c == '\0') {
>   yyerror("syntax error");
>   return (findeol());
> + } else if (c == '$') {
> + p1 = p;
> + while (1) {
> + if ((c = lgetc(0)) == EOF)
> + return (0);
> + if (p1 + 1 >= buf + sizeof(buf) - 1) {
> + yyerror("string too long");
> + return (findeol());
> + }
> + if (isalnum(c) || c == '_') {
> + *p1++ = c;
> + continue;
> + }
> + *p1 = '\0';
> + lungetc(c);
> + break;
> + }
> + val = symget(p);
> + if (val == NULL) {
> + yyerror("macro '%s' not defined", buf);
> + return (findeol());
> + }
> + x = strlcpy(p,val,(buf-p));
> + if (x >= (buf-p)) {
> + yyerror("string too long");
> + return (findeol());
> + }
> + p += x;
> + continue;
>   }
>   if (p + 1 >= buf + sizeof(buf) - 1) {
>   yyerror("string too long");

Hey Michael,

Thank you for your email. I have been playing with your diff and will send you
some comments shortly. This might be worth future consideration.

For now, I think we should update the man page to clearly state that macro
expansion does not take place inside quotes as is currently done in the other
man pages.

Ok?

Index: ifstated.conf.5
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.conf.5,v
retrieving revision 1.11
diff -u -p -r1.11 ifstated.conf.5
--- ifstated.conf.5 3 Mar 2018 02:57:17 -   1.11
+++ ifstated.conf.5 5 Mar 2018 18:39:06 -
@@ -55,6 +55,8 @@ instead of using the first state defined
 Macros can be defined that will later be expanded in context.
 Macro names must start with a letter, digit, or underscore,
 and may contain any of those characters.
+Macros are not expanded inside quotes.
+.Pp
 Macro names may not be reserved words like, for example,
 .Ar state
 or

Re: snmpd agentx.c cleanup

[Rob Pierce]

On Mon, Feb 12, 2018 at 03:03:07PM -0500, Rob Pierce wrote:
> This reduces the diff with relayd/agentx.c.

A little bit closer now.

ok?

Index: agentx.c
===
RCS file: /cvs/src/usr.sbin/snmpd/agentx.c,v
retrieving revision 1.11
diff -u -p -r1.11 agentx.c
--- agentx.c5 Jan 2018 08:13:32 -   1.11
+++ agentx.c12 Feb 2018 22:08:34 -
@@ -18,12 +18,8 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
-#include 
-
-#include 
 #include 
 #include 
 #include 
@@ -217,9 +213,14 @@ snmp_agentx_response(struct agentx_handl
 {
struct agentx_response_data resp;
 
-   if (snmp_agentx_read_response(pdu, ) == -1)
+   if (snmp_agentx_read_raw(pdu, , sizeof(resp)) == -1)
return (-1);
 
+   if (!snmp_agentx_byteorder_native(pdu->hdr)) {
+   resp.error = snmp_agentx_int16_byteswap(resp.error);
+   resp.index = snmp_agentx_int16_byteswap(resp.index);
+   }
+
h->error = resp.error;
if (resp.error != AGENTX_ERR_NONE)
return (-1);
@@ -227,20 +228,6 @@ snmp_agentx_response(struct agentx_handl
return (0);
 }
 
-int
-snmp_agentx_read_response(struct agentx_pdu *pdu, struct agentx_response_data 
*resp)
-{
-   if (snmp_agentx_read_raw(pdu, resp, sizeof(*resp)) == -1)
-   return (-1);
-
-   if (!snmp_agentx_byteorder_native(pdu->hdr)) {
-   resp->error = snmp_agentx_int16_byteswap(resp->error);
-   resp->index = snmp_agentx_int16_byteswap(resp->index);
-   }
-
-   return (0);
-}
-
 /*
  * Read the response PDU for an open operation.
  */
@@ -341,7 +328,7 @@ snmp_agentx_recv(struct agentx_handle *h
if (h->r == NULL) {
if ((h->r = snmp_agentx_pdu_alloc()) == NULL)
return (NULL);
-   h->r->datalen = 0;  /* XXX -- force this for receive 
buffers */
+   h->r->datalen = 0;  /* XXX force this for receive buffers */
}
pdu = h->r;
 
@@ -962,7 +949,8 @@ snmp_agentx_do_read_oid(struct agentx_pd
 }
 
 int
-snmp_agentx_read_searchrange(struct agentx_pdu *pdu, struct 
agentx_search_range *sr)
+snmp_agentx_read_searchrange(struct agentx_pdu *pdu,
+struct agentx_search_range *sr)
 {
if (snmp_agentx_do_read_oid(pdu, >start, >include) == -1 ||
snmp_agentx_read_oid(pdu, >end) == -1)
@@ -1037,7 +1025,7 @@ snmp_oid2string(struct snmp_oid *o, char
bzero(buf, len);
 
for (i = 0; i < o->o_n; i++) {
-   snprintf(str, sizeof(str), "%d", o->o_id[i]);
+   snprintf(str, sizeof(str), "%u", o->o_id[i]);
strlcat(buf, str, len);
if (i < (o->o_n - 1))
strlcat(buf, ".", len);
@@ -1124,7 +1112,7 @@ snmp_agentx_dump_hdr(struct agentx_hdr *
return;
}
 
-   fprintf(stderr, 
+   fprintf(stderr,
"agentx: version %d type %s flags %d reserved %d"
" sessionid %d transactid %d packetid %d length %d",
hdr->version, snmp_agentx_type2name(hdr->type), hdr->flags,

Index: control.c
===
RCS file: /cvs/src/usr.sbin/snmpd/control.c,v
retrieving revision 1.42
diff -u -p -r1.42 control.c
--- control.c   21 Apr 2017 13:50:23 -  1.42
+++ control.c   12 Feb 2018 22:08:34 -
@@ -537,7 +537,7 @@ control_dispatch_agentx(int fd, short ev
struct agentx_varbind_hdrvbhdr;
struct ber_element  **elm, **iter;
 
-   if (snmp_agentx_read_response(pdu, ) == -1) {
+   if (snmp_agentx_read_raw(pdu, , sizeof(resp)) == 
-1) {
msg->sm_error = SNMP_ERROR_GENERR;
goto dispatch;
}

Index: snmp.h
===
RCS file: /cvs/src/usr.sbin/snmpd/snmp.h,v
retrieving revision 1.14
diff -u -p -r1.14 snmp.h
--- snmp.h  11 Jun 2015 18:49:09 -  1.14
+++ snmp.h  12 Feb 2018 22:08:34 -
@@ -369,7 +369,6 @@ struct agentx_handle *
 struct agentx_handle *
snmp_agentx_fdopen(int, char *, struct snmp_oid *);
 intsnmp_agentx_response(struct agentx_handle *, struct agentx_pdu *);
-intsnmp_agentx_read_response(struct agentx_pdu *, struct 
agentx_response_data *);
 intsnmp_agentx_open_response(struct agentx_handle *, struct agentx_pdu *);
 struct agentx_pdu *
snmp_agentx_open_pdu(struct agentx_handle *, char *descr,

snmpd agentx.c cleanup

[Rob Pierce]

This reduces the diff with relayd/agentx.c.

Ok?

Index: agentx.c
===
RCS file: /cvs/src/usr.sbin/snmpd/agentx.c,v
retrieving revision 1.11
diff -u -p -r1.11 agentx.c
--- agentx.c5 Jan 2018 08:13:32 -   1.11
+++ agentx.c12 Feb 2018 19:59:57 -
@@ -18,12 +18,8 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
-#include 
-
-#include 
 #include 
 #include 
 #include 
@@ -341,7 +337,7 @@ snmp_agentx_recv(struct agentx_handle *h
if (h->r == NULL) {
if ((h->r = snmp_agentx_pdu_alloc()) == NULL)
return (NULL);
-   h->r->datalen = 0;  /* XXX -- force this for receive 
buffers */
+   h->r->datalen = 0;  /* XXX force this for receive buffers */
}
pdu = h->r;
 
@@ -962,7 +958,8 @@ snmp_agentx_do_read_oid(struct agentx_pd
 }
 
 int
-snmp_agentx_read_searchrange(struct agentx_pdu *pdu, struct 
agentx_search_range *sr)
+snmp_agentx_read_searchrange(struct agentx_pdu *pdu,
+struct agentx_search_range *sr)
 {
if (snmp_agentx_do_read_oid(pdu, >start, >include) == -1 ||
snmp_agentx_read_oid(pdu, >end) == -1)
@@ -1037,7 +1034,7 @@ snmp_oid2string(struct snmp_oid *o, char
bzero(buf, len);
 
for (i = 0; i < o->o_n; i++) {
-   snprintf(str, sizeof(str), "%d", o->o_id[i]);
+   snprintf(str, sizeof(str), "%u", o->o_id[i]);
strlcat(buf, str, len);
if (i < (o->o_n - 1))
strlcat(buf, ".", len);
@@ -1124,7 +1121,7 @@ snmp_agentx_dump_hdr(struct agentx_hdr *
return;
}
 
-   fprintf(stderr, 
+   fprintf(stderr,
"agentx: version %d type %s flags %d reserved %d"
" sessionid %d transactid %d packetid %d length %d",
hdr->version, snmp_agentx_type2name(hdr->type), hdr->flags,

snmpd trap.c uninitialized variable

[Rob Pierce]

The pid_t confused me, but I believe this is correct - i.e. referring to the
packet id as oppose to a process id.

Comments?  Ok?

Index: trap.c
===
RCS file: /cvs/src/usr.sbin/snmpd/trap.c,v
retrieving revision 1.29
diff -u -p -r1.29 trap.c
--- trap.c  21 Apr 2017 13:46:15 -  1.29
+++ trap.c  20 Jan 2018 16:41:25 -
@@ -65,7 +65,6 @@ trap_agentx(struct agentx_handle *h, str
int  ret = AGENTX_ERR_NONE;
int  seensysuptime, seentrapoid;
size_t   len = 0;
-   pid_tpid = -1;
char*v = NULL;
 
*varcpy = NULL;
@@ -125,8 +124,8 @@ trap_agentx(struct agentx_handle *h, str
 
if (varbind != NULL)
len = ber_calc_len(varbind);
-   log_debug("trap_agentx: from pid %u len %zd elements %d",
-   pid, len, x);
+   log_debug("trap_agentx: from packetid %d len %zd elements %d",
+   pdu->hdr->packetid, len, x);
 
trap_send(, varbind);
 

Re: delaying the start of ifstated in /etc/rc

[Rob Pierce]

On Wed, Aug 30, 2017 at 08:30:52PM -0400, Rob Pierce wrote:
> Depending on the use case for ifstated, dependencies may exist with other
> daemons for performing interface checks and/or external tests. For example,
> one might use ifstated to check a dhcpd enabled interface, or connectivity to
> a vmd virtual machine.
> 
> Does anyone have any objections with delaying the start of ifstated?
> 
> Comments? Ok?

For the record, this is a bad diff; ifstated should start prior to disabling
the carp interlock. Starting ifstated after dhcpd might make sense for the
common use case, but maybe it is best for this to remain as is with local
modifications applied based on local use cases.

Rob

> Index: src/etc/rc
> ===
> RCS file: /cvs/src/etc/rc,v
> retrieving revision 1.517
> diff -u -p -r1.517 rc
> --- src/etc/rc29 Aug 2017 16:56:13 -  1.517
> +++ src/etc/rc31 Aug 2017 00:17:06 -
> @@ -558,7 +558,7 @@ echo 'preserving editor files.'; /usr/li
>  run_upgrade_script sysmerge
>  
>  echo -n 'starting network daemons:'
> -start_daemon ldomd sshd switchd snmpd ldpd ripd ospfd ospf6d bgpd ifstated
> +start_daemon ldomd sshd switchd snmpd ldpd ripd ospfd ospf6d bgpd
>  start_daemon relayd dhcpd dhcrelay mrouted dvmrpd radiusd eigrpd
>  
>  if ifconfig lo0 inet6 >/dev/null 2>&1; then
> @@ -569,7 +569,7 @@ fi
>  
>  start_daemon hostapd lpd smtpd slowcgi httpd ftpd
>  start_daemon ftpproxy ftpproxy6 tftpd tftpproxy identd inetd rarpd bootparamd
> -start_daemon rbootd mopd vmd spamd spamlogd sndiod
> +start_daemon rbootd mopd vmd spamd spamlogd sndiod ifstated
>  echo '.'
>  
>  # If rc.firsttime exists, run it just once, and make sure it is deleted.
> 

delaying the start of ifstated in /etc/rc

[Rob Pierce]

Depending on the use case for ifstated, dependencies may exist with other
daemons for performing interface checks and/or external tests. For example,
one might use ifstated to check a dhcpd enabled interface, or connectivity to
a vmd virtual machine.

Does anyone have any objections with delaying the start of ifstated?

Comments? Ok?

Index: src/etc/rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.517
diff -u -p -r1.517 rc
--- src/etc/rc  29 Aug 2017 16:56:13 -  1.517
+++ src/etc/rc  31 Aug 2017 00:17:06 -
@@ -558,7 +558,7 @@ echo 'preserving editor files.'; /usr/li
 run_upgrade_script sysmerge
 
 echo -n 'starting network daemons:'
-start_daemon ldomd sshd switchd snmpd ldpd ripd ospfd ospf6d bgpd ifstated
+start_daemon ldomd sshd switchd snmpd ldpd ripd ospfd ospf6d bgpd
 start_daemon relayd dhcpd dhcrelay mrouted dvmrpd radiusd eigrpd
 
 if ifconfig lo0 inet6 >/dev/null 2>&1; then
@@ -569,7 +569,7 @@ fi
 
 start_daemon hostapd lpd smtpd slowcgi httpd ftpd
 start_daemon ftpproxy ftpproxy6 tftpd tftpproxy identd inetd rarpd bootparamd
-start_daemon rbootd mopd vmd spamd spamlogd sndiod
+start_daemon rbootd mopd vmd spamd spamlogd sndiod ifstated
 echo '.'
 
 # If rc.firsttime exists, run it just once, and make sure it is deleted.

Re: ksh(1) history lines allocation

[Rob Pierce]

On Tue, Aug 15, 2017 at 02:03:43PM -0400, Jeremie Courreges-Anglas wrote:
> On Tue, Aug 15 2017, Rob Pierce <r...@2keys.ca> wrote:
> 
> [...]
> 
> > I was able to reproduce the problem with a HISTSIZE of 10 which at 
> > 125000
> > entries rendered my system unusable. With the patch I am running fine with a
> > HISTSIZE of 12 and have come back several times after hitting the 1.25x
> > threshold.
> >
> > Regression tests pass.
> 
> Thanks for your feedback.  As I said privately, I did a bad job at
> analyzing how to fix the slowness of afree().  With latest commit to
> alloc.c, switching history lines allocation to strdup(3)/free(3) is not
> needed any more; also it is probably better for us to use the same
> allocation pattern everywhere.

Sounds good. Your latest commit passed my local tests with a large HISTSIZE.

Regards,

Rob

> Thanks,
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: ifstated: stop tracking interface indexes

[Rob Pierce]

On Tue, Aug 15, 2017 at 02:37:22PM -0400, Jeremie Courreges-Anglas wrote:
> On Tue, Aug 15 2017, Rob Pierce <r...@2keys.ca> wrote:
> > On Mon, Aug 14, 2017 at 11:26:46PM -0400, Jeremie Courreges-Anglas wrote:
> >> On Mon, Aug 14 2017, Rob Pierce <r...@2keys.ca> wrote:
> >> > ifstated currently tracks and maintains the index of each monitored 
> >> > interface
> >> > and does not maintain interface names. This means we need to re-index on
> >> > interface departure and arrival.
> >> >
> >> > The following diff moves away from indexes to names. Indexes are still 
> >> > required,
> >> > but easily obtained dynamically as needed. This helps simplify the next 
> >> > diff
> >> > that will provide support for interface departure and arrival.
> >> >
> >> > Suggested by deraadt.
> >> >
> >> > No intended functional change. Regress tests pass.
> >> >
> >> > Ok?
> >> 
> >> The idea looks sound to me, however I would keep the "interface" symbol
> >> in parse.y (your diff doesn't remove all "interface" references btw).
> >> 
> >> The current code checks the existence of the interface at startup.  If
> >> the interface doesn't exists, you get a syntax error.  This could happen
> >> because of a missing interface (an interesting case), or because of
> >> a typo.  Whether or not we're erroring out, it is nice to print
> >> a diagnostic message.
> >> 
> >> I'm not sure this change was intended, so here's a tentative diff that
> >> that keeps the existing behavior.  Regress tests pass.
> >
> > Yes, I see that now. That change was not intended. Thanks!
> >
> > Your diff looks good.
> >
> > Ok?
> 
> Please use four spaces, not three, for second level indents; see
> style(9).
> 
> There's a check that should be fixed, please see below.

Ok, will do. Not sure why I started using three spaces for 2nd level indents...

> With those items addressed, ok jca@
> 
> It feels a bit weird to reject unknown interface names at startup but to
> cope with departed/joining interfaces at runtime.  But I guess we'll
> have to live with this.

I don't disagree. I am most concerned about handling the departure condition
more appropriately, which is (in my opinion) currently broken.

As discussed, it is an option to do the right thing on departure (i.e. demote
with a state change to down) and then fail hard, which would mirror the
(current) startup behaviour more closely (i.e. fail on startup due to the
interfaces not being present).

I suppose we could also allow startup with unknown interfaces (present in the
configuration file) to occur (which would be a change in behaviour), and simply
mark those interfaces as down and immediately demote but keep running, like we
do for an interface that is present at startup but in a down state.

This diff does not yet introduce interface departure and arrival, and with
your updates will not change any behaviour, and I think this change makes
sense regardless of which approach is taken in the next step.

Thank you for your thorough review and comments. I will update my diff
accordingly and wait a few days for any further comments before committing.

Regards,

Rob

> >> Index: ifstated.c
> >> ===
> >> RCS file: /d/cvs/src/usr.sbin/ifstated/ifstated.c,v
> >> retrieving revision 1.59
> >> diff -u -p -r1.59 ifstated.c
> >> --- ifstated.c 14 Aug 2017 03:15:28 -  1.59
> >> +++ ifstated.c 15 Aug 2017 03:04:47 -
> >> @@ -61,8 +61,8 @@ void external_handler(int, short, void 
> >>  void  external_exec(struct ifsd_external *, int);
> >>  void  check_external_status(struct ifsd_state *);
> >>  void  external_evtimer_setup(struct ifsd_state *, int);
> >> -void  scan_ifstate(int, int, int);
> >> -int   scan_ifstate_single(int, int, struct ifsd_state *);
> >> +void  scan_ifstate(const char *, int, int);
> >> +int   scan_ifstate_single(const char *, int, struct 
> >> ifsd_state *);
> >>  void  fetch_ifstate(int);
> >>  __dead void   usage(void);
> >>  void  adjust_expressions(struct ifsd_expression_list *, int);
> >> @@ -233,6 +233,8 @@ rt_msg_handler(int fd, short event, void
> >>char msg[2048];
> >>struct rt_msghdr *rtm = (struct rt_msghdr *)
> >>struct if_msghdr ifm;
&

Re: ifstated: stop tracking interface indexes

[Rob Pierce]

On Mon, Aug 14, 2017 at 11:26:46PM -0400, Jeremie Courreges-Anglas wrote:
> On Mon, Aug 14 2017, Rob Pierce <r...@2keys.ca> wrote:
> > ifstated currently tracks and maintains the index of each monitored 
> > interface
> > and does not maintain interface names. This means we need to re-index on
> > interface departure and arrival.
> >
> > The following diff moves away from indexes to names. Indexes are still 
> > required,
> > but easily obtained dynamically as needed. This helps simplify the next diff
> > that will provide support for interface departure and arrival.
> >
> > Suggested by deraadt.
> >
> > No intended functional change. Regress tests pass.
> >
> > Ok?
> 
> The idea looks sound to me, however I would keep the "interface" symbol
> in parse.y (your diff doesn't remove all "interface" references btw).
> 
> The current code checks the existence of the interface at startup.  If
> the interface doesn't exists, you get a syntax error.  This could happen
> because of a missing interface (an interesting case), or because of
> a typo.  Whether or not we're erroring out, it is nice to print
> a diagnostic message.
> 
> I'm not sure this change was intended, so here's a tentative diff that
> that keeps the existing behavior.  Regress tests pass.

Yes, I see that now. That change was not intended. Thanks!

Your diff looks good.

Ok?

> Index: ifstated.c
> ===
> RCS file: /d/cvs/src/usr.sbin/ifstated/ifstated.c,v
> retrieving revision 1.59
> diff -u -p -r1.59 ifstated.c
> --- ifstated.c14 Aug 2017 03:15:28 -  1.59
> +++ ifstated.c15 Aug 2017 03:04:47 -
> @@ -61,8 +61,8 @@ voidexternal_handler(int, short, void 
>  void external_exec(struct ifsd_external *, int);
>  void check_external_status(struct ifsd_state *);
>  void external_evtimer_setup(struct ifsd_state *, int);
> -void scan_ifstate(int, int, int);
> -int  scan_ifstate_single(int, int, struct ifsd_state *);
> +void scan_ifstate(const char *, int, int);
> +int  scan_ifstate_single(const char *, int, struct ifsd_state *);
>  void fetch_ifstate(int);
>  __dead void  usage(void);
>  void adjust_expressions(struct ifsd_expression_list *, int);
> @@ -233,6 +233,8 @@ rt_msg_handler(int fd, short event, void
>   char msg[2048];
>   struct rt_msghdr *rtm = (struct rt_msghdr *)
>   struct if_msghdr ifm;
> + char ifnamebuf[IFNAMSIZ];
> + char *ifname;
>   ssize_t len;
>  
>   if ((len = read(fd, msg, sizeof(msg))) == -1) {
> @@ -250,7 +252,10 @@ rt_msg_handler(int fd, short event, void
>   switch (rtm->rtm_type) {
>   case RTM_IFINFO:
>   memcpy(, rtm, sizeof(ifm));
> - scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
> + ifname = if_indextoname(ifm.ifm_index, ifnamebuf);
> + /* ifname is NULL on interface departure */
> + if (ifname != NULL)
> + scan_ifstate(ifname, ifm.ifm_data.ifi_link_state, 1);
>   break;
>   case RTM_DESYNC:
>   fetch_ifstate(1);
> @@ -431,7 +436,7 @@ external_evtimer_setup(struct ifsd_state
>  #define  LINK_STATE_IS_DOWN(_s)  (!LINK_STATE_IS_UP((_s)))
>  
>  int
> -scan_ifstate_single(int ifindex, int s, struct ifsd_state *state)
> +scan_ifstate_single(const char *ifname, int s, struct ifsd_state *state)
>  {
>   struct ifsd_ifstate *ifstate;
>   struct ifsd_expression_list expressions;
> @@ -440,7 +445,7 @@ scan_ifstate_single(int ifindex, int s, 
>   TAILQ_INIT();
>  
>   TAILQ_FOREACH(ifstate, >interface_states, entries) {
> - if (ifstate->ifindex == ifindex) {
> + if (strcmp(ifstate->ifname, ifname) == 0) {
>   if (ifstate->prevstate != s &&
>   (ifstate->prevstate != -1 || !opt_inhibit)) {
>   struct ifsd_expression *expression;
> @@ -472,15 +477,15 @@ scan_ifstate_single(int ifindex, int s, 
>  }
>  
>  void
> -scan_ifstate(int ifindex, int s, int do_eval)
> +scan_ifstate(const char *ifname, int s, int do_eval)
>  {
>   struct ifsd_state *state;
>   int cur_eval = 0;
>  
> - if (scan_ifstate_single(ifindex, s, >initstate) && do_eval)
> + if (scan_ifstate_single(ifname, s, >initstate) && do_eval)
>   eval_state(>initstate);
>   TAILQ_FOREACH(state, >states, entries) {
> - if (scan_if

Re: ksh(1) history lines allocation

[Rob Pierce]

On Mon, Aug 14, 2017 at 10:26:48PM -0400, Jeremie Courreges-Anglas wrote:
> 
> So I tinkered with the way ksh(1) tracks memory allocation, trying to
> make it faster in the general case.  One approach used a RB tree,
> I wrote since a simple hash table implementation which seems to work
> rather well.
> 
> But the actual problem I'd first like to solve is a corner case.  I use
> HISTSIZE=2, and when the actual line count in my histfile approaches
> 25000 (1.25 * HISTSIZE), ksh(1) has a hard time handling it. The main
> reason is that it calls afree() ~5000 times in a loop, with afree()
> traversing the APERM freelist, which contains >2 elements.  This is
> expensive.
> 
> For history lines, we don't actually need to keep track of allocations
> using an area, history lines are private to history.c and no gc/whatever
> is needed there.  So here's a diff that just uses strdup(3)/free(3).
> 
> Comments?  ok?

I was able to reproduce the problem with a HISTSIZE of 10 which at 125000
entries rendered my system unusable. With the patch I am running fine with a
HISTSIZE of 12 and have come back several times after hitting the 1.25x
threshold.

Regression tests pass.

Rob

> Index: history.c
> ===
> RCS file: /d/cvs/src/bin/ksh/history.c,v
> retrieving revision 1.64
> diff -u -p -p -u -r1.64 history.c
> --- history.c 11 Aug 2017 19:37:58 -  1.64
> +++ history.c 15 Aug 2017 01:14:58 -
> @@ -428,7 +428,7 @@ histbackup(void)
>  
>   if (histptr >= history && last_line != hist_source->line) {
>   hist_source->line--;
> - afree(*histptr, APERM);
> + free(*histptr);
>   histptr--;
>   last_line = hist_source->line;
>   }
> @@ -613,14 +613,15 @@ histsave(int lno, const char *cmd, int d
>  #endif
>   }
>  
> - c = str_save(cmd, APERM);
> + if ((c = strdup(cmd)) == NULL)
> + internal_errorf(1, "unable to allocate memory");
>   if ((cp = strrchr(c, '\n')) != NULL)
>   *cp = '\0';
>  
>   if (histptr < history + histsize - 1)
>   histptr++;
>   else { /* remove oldest command */
> - afree(*history, APERM);
> + free(*history);
>   memmove(history, history + 1,
>   (histsize - 1) * sizeof(*history));
>   }
> 
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

ifstated: stop tracking interface indexes

[Rob Pierce]

ifstated currently tracks and maintains the index of each monitored interface
and does not maintain interface names. This means we need to re-index on
interface departure and arrival.

The following diff moves away from indexes to names. Indexes are still required,
but easily obtained dynamically as needed. This helps simplify the next diff
that will provide support for interface departure and arrival.

Suggested by deraadt.

No intended functional change. Regress tests pass.

Ok?

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.59
diff -u -p -r1.59 ifstated.c
--- ifstated.c  14 Aug 2017 03:15:28 -  1.59
+++ ifstated.c  15 Aug 2017 00:34:11 -
@@ -61,8 +61,8 @@ void  external_handler(int, short, void 
 void   external_exec(struct ifsd_external *, int);
 void   check_external_status(struct ifsd_state *);
 void   external_evtimer_setup(struct ifsd_state *, int);
-void   scan_ifstate(int, int, int);
-intscan_ifstate_single(int, int, struct ifsd_state *);
+void   scan_ifstate(const char *, int, int);
+intscan_ifstate_single(const char *, int, struct ifsd_state *);
 void   fetch_ifstate(int);
 __dead voidusage(void);
 void   adjust_expressions(struct ifsd_expression_list *, int);
@@ -233,6 +233,8 @@ rt_msg_handler(int fd, short event, void
char msg[2048];
struct rt_msghdr *rtm = (struct rt_msghdr *)
struct if_msghdr ifm;
+   char ifnamebuf[IFNAMSIZ];
+   char *ifname;
ssize_t len;
 
if ((len = read(fd, msg, sizeof(msg))) == -1) {
@@ -250,7 +252,10 @@ rt_msg_handler(int fd, short event, void
switch (rtm->rtm_type) {
case RTM_IFINFO:
memcpy(, rtm, sizeof(ifm));
-   scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
+   ifname = if_indextoname(ifm.ifm_index, ifnamebuf);
+   /* ifname is NULL on interface departure */
+   if (ifname != NULL)
+   scan_ifstate(ifname, ifm.ifm_data.ifi_link_state, 1);
break;
case RTM_DESYNC:
fetch_ifstate(1);
@@ -431,7 +436,7 @@ external_evtimer_setup(struct ifsd_state
 #defineLINK_STATE_IS_DOWN(_s)  (!LINK_STATE_IS_UP((_s)))
 
 int
-scan_ifstate_single(int ifindex, int s, struct ifsd_state *state)
+scan_ifstate_single(const char *ifname, int s, struct ifsd_state *state)
 {
struct ifsd_ifstate *ifstate;
struct ifsd_expression_list expressions;
@@ -440,7 +445,7 @@ scan_ifstate_single(int ifindex, int s, 
TAILQ_INIT();
 
TAILQ_FOREACH(ifstate, >interface_states, entries) {
-   if (ifstate->ifindex == ifindex) {
+   if (strcmp(ifstate->ifname, ifname) == 0) {
if (ifstate->prevstate != s &&
(ifstate->prevstate != -1 || !opt_inhibit)) {
struct ifsd_expression *expression;
@@ -472,15 +477,15 @@ scan_ifstate_single(int ifindex, int s, 
 }
 
 void
-scan_ifstate(int ifindex, int s, int do_eval)
+scan_ifstate(const char *ifname, int s, int do_eval)
 {
struct ifsd_state *state;
int cur_eval = 0;
 
-   if (scan_ifstate_single(ifindex, s, >initstate) && do_eval)
+   if (scan_ifstate_single(ifname, s, >initstate) && do_eval)
eval_state(>initstate);
TAILQ_FOREACH(state, >states, entries) {
-   if (scan_ifstate_single(ifindex, s, state) &&
+   if (scan_ifstate_single(ifname, s, state) &&
(do_eval && state == conf->curstate))
cur_eval = 1;
}
@@ -619,8 +624,8 @@ fetch_ifstate(int do_eval)
for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
if (ifa->ifa_addr->sa_family == AF_LINK) {
struct if_data *ifdata = ifa->ifa_data;
-   scan_ifstate(if_nametoindex(ifa->ifa_name),
-   ifdata->ifi_link_state, do_eval);
+   scan_ifstate(ifa->ifa_name, ifdata->ifi_link_state,
+  do_eval);
}
}
 
Index: ifstated.h
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
retrieving revision 1.18
diff -u -p -r1.18 ifstated.h
--- ifstated.h  14 Aug 2017 03:15:28 -  1.18
+++ ifstated.h  15 Aug 2017 00:34:11 -
@@ -41,7 +41,7 @@ struct ifsd_ifstate {
 #define IFSD_LINKUP2
int  prevstate;
u_int32_trefcount;
-   u_short  ifindex;
+   char ifname[IFNAMSIZ];
 };
 
 struct ifsd_external {
Index: parse.y
===
RCS file: 

diff: pledge snmpd

[Rob Pierce]

The following diff pledges two of three processes in snmpd: the parent snmpd
process and the trap handler. We cannot currently pledge snmpe as snmp requests
asking for privileged kernel info are disallowed by pledge.

I have included a commented pledge block in snmpe.c below (which will not be
committed) which I believe would be possible if we moved the code that
violates pledge to another unpledged process. If we think that is worth while
I could pursue it further.

In the mean time I am looking for comments and/or ok's for the snmpd.c and
traphandler.c diffs below.

This passes the newly committed snmpd regression tests.

Regards,

Rob

Index: snmpd.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpd.c,v
retrieving revision 1.36
diff -u -p -r1.36 snmpd.c
--- snmpd.c 4 Apr 2017 02:37:15 -   1.36
+++ snmpd.c 11 Aug 2017 20:10:50 -
@@ -255,6 +255,9 @@ main(int argc, char *argv[])
 
proc_connect(ps);
 
+   if (pledge("stdio rpath cpath sendfd dns proc exec id", NULL) == -1)
+   fatal("pledge");
+
event_dispatch();
 
log_debug("%d parent exiting", getpid());
Index: snmpe.c
===
RCS file: /cvs/src/usr.sbin/snmpd/snmpe.c,v
retrieving revision 1.48
diff -u -p -r1.48 snmpe.c
--- snmpe.c 27 Jul 2017 14:04:16 -  1.48
+++ snmpe.c 11 Aug 2017 20:10:50 -
@@ -105,6 +105,10 @@ snmpe_init(struct privsep *ps, struct pr
snmpe_recvmsg, env);
event_add(>s_ev, NULL);
}
+/*
+   if (pledge("stdio recvfd inet vminfo route", NULL) == -1)
+   fatal("pledge");
+ */
 }
 
 void
Index: traphandler.c
===
RCS file: /cvs/src/usr.sbin/snmpd/traphandler.c,v
retrieving revision 1.8
diff -u -p -r1.8 traphandler.c
--- traphandler.c   9 Jan 2017 14:49:22 -   1.8
+++ traphandler.c   11 Aug 2017 20:10:50 -
@@ -96,6 +96,9 @@ traphandler_init(struct privsep *ps, str
struct snmpd*env = ps->ps_env;
struct listen_sock  *so;
 
+   if (pledge("stdio recvfd proc exec id", NULL) == -1)
+   fatal("pledge");
+
if (!env->sc_traphandler)
return;
 

bsd.regress.mk.5 typo fix

[Rob Pierce]

I just ran across this. Ok?

Index: bsd.regress.mk.5
===
RCS file: /cvs/src/share/man/man5/bsd.regress.mk.5,v
retrieving revision 1.14
diff -u -p -r1.14 bsd.regress.mk.5
--- bsd.regress.mk.53 Jul 2017 18:19:55 -   1.14
+++ bsd.regress.mk.511 Aug 2017 16:39:12 -
@@ -155,7 +155,7 @@ to run
 before, this directory or symlink may not exist.
 Then the test should run anyway.
 .Pp
-Test are executed with
+Tests are executed with
 .Sy make Cm regress ,
 but running
 .Sy make Cm all

Re: ifstated: add handing of departed interfaces

[Rob Pierce]

On Tue, Aug 08, 2017 at 12:12:43AM +0200, Jeremie Courreges-Anglas wrote:
> On Sun, Aug 06 2017, Rob Pierce <r...@2keys.ca> wrote:
> > The following diff adds support for detecting the state change of a departed
> > interface. ifstated is not a very verbose daemon, so this diff quietly does
> > the right thing (i.e. there is no exttra warning about a departing 
> > interface).
> 
> But maybe there should be at least a big scary message.  This is not
> exactly a normal situation.

Yes, I agree. This is a special case.

> > The re-arrival of a departed interface involves re-indexing the interface 
> > and
> > possibly other complexities that require more consideration, but for now at
> > least this obvious condition is handled in what I believe is a more
> > appropriate manner.
> 
> I wonder what's the most useful behavior here: complain loudly and fail
> hard (exit) or just consider the link down and monitor the rest of the
> remaining interfaces, as in your diff.

I do like the idea of complaining loudly and failing hard. I had originally
considered a fatal (after forcing a demote of course).

> Destroying then reconfiguring a carp(4) interface won't give you a very
> user-friendly behavior.  Same thing with disconnecting/reconnecting
> interfaces like urtwn(4) or urndis(4).  ifstated(8) won't do anything
> about your new interface, even if you checked that said interface was
> in good shape.
> 
> Also if we keep running, the link is now considered down and we're
> likely to execute commands that refer to an interface that has left;
> that might be good or bad, I don't know what people put in their
> ifstated.conf.
> 
> > Updated regression tests pass, and the corresponding regression diff is also
> > attached.
> >
> > Ok?
> 
> I'm not sure yet which path we should follow.  Let's discuss this in
> Toronto, shal we?  In the meantime, please see the nits below.

Sounds like a plan. Looking forward to it!

Rob

> > Index: regress/usr.sbin/ifstated/ifstated
> > ===
> > RCS file: /cvs/src/regress/usr.sbin/ifstated/ifstated,v
> > retrieving revision 1.3
> > diff -u -p -r1.3 ifstated
> > --- regress/usr.sbin/ifstated/ifstated  31 Jul 2017 18:41:21 -  
> > 1.3
> > +++ regress/usr.sbin/ifstated/ifstated  6 Aug 2017 23:29:11 -
> > @@ -124,6 +124,7 @@ changing state to primary
> >  changing state to demoted
> >  changing state to primary
> >  changing state to primary
> > +changing state to demoted
> >  EOF
> >  
> >  (cd working && nohup ifstated -dvf ./ifstated.conf > ifstated.log 2>&1) &
> > @@ -148,6 +149,8 @@ ifconfig carp${VHIDB} inet ${PREFIX}.${V
> > ${PREFIX}.255 vhid ${VHIDB} carpdev ${NIC}
> >  sleep ${SLEEP}
> >  kill -HUP $(pgrep ifstated) >/dev/null 2>&1
> > +sleep ${SLEEP}
> > +ifconfig carp${VHIDA} destroy
> >  sleep ${SLEEP}
> >  
> >  grep ^changing working/ifstated.log > working/output.new
> > Index: usr.sbin/ifstated/ifstated.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
> > retrieving revision 1.57
> > diff -u -p -r1.57 ifstated.c
> > --- usr.sbin/ifstated/ifstated.c6 Aug 2017 19:27:54 -   1.57
> > +++ usr.sbin/ifstated/ifstated.c6 Aug 2017 23:29:12 -
> > @@ -1,4 +1,4 @@
> > -/* $OpenBSD: ifstated.c,v 1.57 2017/08/06 19:27:54 rob Exp $   */
> > +/* $OpenBSD: ifstated.c,v 1.56 2017/07/24 12:33:59 jca Exp $   */
> >  
> >  /*
> >   * Copyright (c) 2004 Marco Pfatschbacher <m...@openbsd.org>
> > @@ -61,6 +61,7 @@ void  rt_msg_handler(int, short, void *)
> >  void   external_handler(int, short, void *);
> >  void   external_exec(struct ifsd_external *, int);
> >  void   check_external_status(struct ifsd_state *);
> > +void   check_for_ifdeparture(void);
> 
> check_ifdeparture() would be shorter and just as descriptive.
> 
> >  void   external_evtimer_setup(struct ifsd_state *, int);
> >  void   scan_ifstate(int, int, int);
> >  intscan_ifstate_single(int, int, struct ifsd_state *);
> > @@ -150,7 +151,7 @@ main(int argc, char *argv[])
> > if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
> > err(1, "no routing socket");
> >  
> > -   rtfilter = ROUTE_FILTER(RTM_IFINFO);
> > +   rtfilter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_IFANNOUNCE);
> > if (setsockopt(rt_f

Re: ifstated: consistent use of log.c

[Rob Pierce]

On Sun, Aug 06, 2017 at 06:47:38PM +0200, Jeremie Courreges-Anglas wrote:
> On Thu, Aug 03 2017, Rob Pierce <r...@2keys.ca> wrote:
> > As a result ifstated.c no longer needs err.h.
> >
> > Index: ifstated.c
> > ===
> > RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
> > retrieving revision 1.56
> > diff -u -p -r1.56 ifstated.c
> > --- ifstated.c  24 Jul 2017 12:33:59 -  1.56
> > +++ ifstated.c  3 Aug 2017 23:59:13 -
> > @@ -37,7 +37,6 @@
> >  #include 
> >  #include 
> >  #include 
> > -#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -102,7 +101,7 @@ main(int argc, char *argv[])
> > break;
> > case 'D':
> > if (cmdline_symset(optarg) < 0)
> > -   errx(1, "could not parse macro definition %s",
> > +   fatalx("could not parse macro definition %s",
> > optarg);
> > break;
> > case 'f':
> > @@ -135,7 +134,7 @@ main(int argc, char *argv[])
> > if (opts & IFSD_OPT_NOACTION) {
> > if ((newconf = parse_config(configfile, opts)) == NULL)
> > exit(1);
> > -   warnx("configuration OK");
> > +   fprintf(stderr, "configuration OK\n");
> 
> This changes the output from
> 
>   ifstated: configuration OK
> 
> to
> 
>   configuration OK
> 
> which is a bit less helpful.  To keep the output the same, you could use
> warnx, fprintf + __progname/getprogname instead, or just
> 
>   errx(0, "configuration OK");
> 
> I don't really have a preference here.

Good catch. I honestly didn't notice the change in output that was introduced
by my diff. I was following the code in other networking daemons.

I would like to standardize on log.c and remove the err.h include, so maybe
the __progname approach is best if we want to keep the same output, or maybe
having a few lingering uses of err.h is ok.

Many network daemons (below) do not reference themselves when confirming the
validity of their configuration file with the -n option. Is it better for
ifstated to retain it's historical output on the configuration check, or to be
made more consistent with other network daemons?

Regards,

Rob

bgpd/bgpd.c:fprintf(stderr, "configuration OK\n");
dvmrpd/dvmrpd.c:fprintf(stderr, "configuration OK\n");
eigrpd/eigrpd.c:fprintf(stderr, "configuration OK\n");
httpd/httpd.c:  fprintf(stderr, "configuration OK\n");
ldapd/ldapd.c:  fprintf(stderr, "configuration ok\n");
ldpd/ldpd.c:fprintf(stderr, "configuration OK\n");
ntpd/ntpd.c:fprintf(stderr, "configuration OK\n");
ospf6d/ospf6d.c:fprintf(stderr, "configuration OK\n");
ospfd/ospfd.c:  fprintf(stderr, "configuration OK\n");
radiusd/radiusd.c:  fprintf(stderr, "configuration OK\n");
relayd/relayd.c:fprintf(stderr, "configuration OK\n");
ripd/ripd.c:fprintf(stderr, "configuration OK\n");
sasyncd/sasyncd.c:  fprintf(stderr, "configuration OK\n");
smtpd/smtpd.c:  fprintf(stderr, "configuration OK\n");
snmpd/snmpd.c:  fprintf(stderr, "configuration ok\n");
switchd/switchd.c:  fprintf(stderr, "configuration OK\n");
vmd/vmd.c:  fprintf(stderr, "configuration OK\n");
ypldap/ypldap.c:fprintf(stderr, "configuration OK\n");

> > exit(0);
> > }
> >  
> > @@ -147,7 +146,7 @@ main(int argc, char *argv[])
> > log_setverbose(opts & IFSD_OPT_VERBOSE);
> >  
> > if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
> > -   err(1, "no routing socket");
> > +   fatal("no routing socket");
> >  
> > rtfilter = ROUTE_FILTER(RTM_IFINFO);
> > if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
> > @@ -604,7 +603,7 @@ fetch_ifstate(void)
> > struct ifaddrs *ifap, *ifa;
> >  
> > if (getifaddrs() != 0)
> > -   err(1, "getifaddrs");
> > +   fatal("getifaddrs");
> >  
> > for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
> > if (ifa->ifa_addr->sa_family == AF_LINK) {
> >
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

ifstated: add handing of departed interfaces

[Rob Pierce]

The following diff adds support for detecting the state change of a departed
interface. ifstated is not a very verbose daemon, so this diff quietly does
the right thing (i.e. there is no exttra warning about a departing interface).

The re-arrival of a departed interface involves re-indexing the interface and
possibly other complexities that require more consideration, but for now at
least this obvious condition is handled in what I believe is a more
appropriate manner.

Updated regression tests pass, and the corresponding regression diff is also
attached.

Ok?

Index: regress/usr.sbin/ifstated/ifstated
===
RCS file: /cvs/src/regress/usr.sbin/ifstated/ifstated,v
retrieving revision 1.3
diff -u -p -r1.3 ifstated
--- regress/usr.sbin/ifstated/ifstated  31 Jul 2017 18:41:21 -  1.3
+++ regress/usr.sbin/ifstated/ifstated  6 Aug 2017 23:29:11 -
@@ -124,6 +124,7 @@ changing state to primary
 changing state to demoted
 changing state to primary
 changing state to primary
+changing state to demoted
 EOF
 
 (cd working && nohup ifstated -dvf ./ifstated.conf > ifstated.log 2>&1) &
@@ -148,6 +149,8 @@ ifconfig carp${VHIDB} inet ${PREFIX}.${V
${PREFIX}.255 vhid ${VHIDB} carpdev ${NIC}
 sleep ${SLEEP}
 kill -HUP $(pgrep ifstated) >/dev/null 2>&1
+sleep ${SLEEP}
+ifconfig carp${VHIDA} destroy
 sleep ${SLEEP}
 
 grep ^changing working/ifstated.log > working/output.new
Index: usr.sbin/ifstated/ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.57
diff -u -p -r1.57 ifstated.c
--- usr.sbin/ifstated/ifstated.c6 Aug 2017 19:27:54 -   1.57
+++ usr.sbin/ifstated/ifstated.c6 Aug 2017 23:29:12 -
@@ -1,4 +1,4 @@
-/* $OpenBSD: ifstated.c,v 1.57 2017/08/06 19:27:54 rob Exp $   */
+/* $OpenBSD: ifstated.c,v 1.56 2017/07/24 12:33:59 jca Exp $   */
 
 /*
  * Copyright (c) 2004 Marco Pfatschbacher 
@@ -61,6 +61,7 @@ void  rt_msg_handler(int, short, void *)
 void   external_handler(int, short, void *);
 void   external_exec(struct ifsd_external *, int);
 void   check_external_status(struct ifsd_state *);
+void   check_for_ifdeparture(void);
 void   external_evtimer_setup(struct ifsd_state *, int);
 void   scan_ifstate(int, int, int);
 intscan_ifstate_single(int, int, struct ifsd_state *);
@@ -150,7 +151,7 @@ main(int argc, char *argv[])
if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
err(1, "no routing socket");
 
-   rtfilter = ROUTE_FILTER(RTM_IFINFO);
+   rtfilter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_IFANNOUNCE);
if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
, sizeof(rtfilter)) == -1) /* not fatal */
log_warn("%s: setsockopt msgfilter", __func__);
@@ -234,6 +235,7 @@ rt_msg_handler(int fd, short event, void
char msg[2048];
struct rt_msghdr *rtm = (struct rt_msghdr *)
struct if_msghdr ifm;
+   struct if_announcemsghdr ifan;
ssize_t len;
 
if ((len = read(fd, msg, sizeof(msg))) == -1) {
@@ -253,8 +255,19 @@ rt_msg_handler(int fd, short event, void
memcpy(, rtm, sizeof(ifm));
scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
break;
+   case RTM_IFANNOUNCE:
+   memcpy(, rtm, sizeof(ifan));
+   switch (ifan.ifan_what) {
+   case IFAN_DEPARTURE:
+   scan_ifstate(ifan.ifan_index, LINK_STATE_DOWN, 1);
+   break;
+   default:
+   break;
+   }
+   break;
case RTM_DESYNC:
fetch_ifstate(1);
+   check_for_ifdeparture();
break;
default:
break;
@@ -626,6 +639,26 @@ fetch_ifstate(int do_eval)
}
 
freeifaddrs(ifap);
+}
+
+void
+check_for_ifdeparture(void)
+{
+   struct ifsd_state *state;
+   struct ifsd_ifstate *ifstate;
+   char ifnamebuf[IF_NAMESIZE];
+   char *if_name;
+
+   TAILQ_FOREACH(state, >states, entries) {
+   TAILQ_FOREACH(ifstate, >interface_states, entries) {
+   if_name = if_indextoname(ifstate->ifindex, ifnamebuf);
+   if (if_name == NULL) {
+   scan_ifstate(ifstate->ifindex,
+  LINK_STATE_DOWN, 1);
+   }
+   }
+   }
+   return;
 }
 
 void

ifstated: improve routing socket error handling

[Rob Pierce]

Improve error checking during processing of routing messages. Handling of
RTM_DESYNC encouraged by deraadt. Regression tests pass.

I have another diff ready to go that handles interface depature, but I thought
it best to separate them.

Ok?

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.56
diff -u -p -r1.56 ifstated.c
--- ifstated.c  24 Jul 2017 12:33:59 -  1.56
+++ ifstated.c  6 Aug 2017 03:49:54 -
@@ -38,6 +38,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -63,7 +64,7 @@ void  check_external_status(struct ifsd_
 void   external_evtimer_setup(struct ifsd_state *, int);
 void   scan_ifstate(int, int, int);
 intscan_ifstate_single(int, int, struct ifsd_state *);
-void   fetch_ifstate(void);
+void   fetch_ifstate(int);
 __dead voidusage(void);
 void   adjust_expressions(struct ifsd_expression_list *, int);
 void   adjust_external_expressions(struct ifsd_state *);
@@ -210,7 +211,7 @@ load_config(void)
clear_config(conf);
conf = newconf;
conf->initstate.entered = time(NULL);
-   fetch_ifstate();
+   fetch_ifstate(0);
external_evtimer_setup(>initstate, IFSD_EVTIMER_ADD);
adjust_external_expressions(>initstate);
eval_state(>initstate);
@@ -235,20 +236,30 @@ rt_msg_handler(int fd, short event, void
struct if_msghdr ifm;
ssize_t len;
 
-   len = read(fd, msg, sizeof(msg));
+   if ((len = read(fd, msg, sizeof(msg))) == -1) {
+   if (errno == EAGAIN || errno == EINTR)
+   return;
+   fatal("%s: routing socket read error", __func__);
+   }
 
-   /* XXX ignore errors? */
-   if (len < sizeof(struct rt_msghdr))
-   return;
+   if (len == 0)
+   fatal("%s: routing socket closed", __func__);
 
if (rtm->rtm_version != RTM_VERSION)
return;
 
-   if (rtm->rtm_type != RTM_IFINFO)
-   return;
-
-   memcpy(, rtm, sizeof(ifm));
-   scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
+   switch (rtm->rtm_type) {
+   case RTM_IFINFO:
+   memcpy(, rtm, sizeof(ifm));
+   scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
+   break;
+   case RTM_DESYNC:
+   fetch_ifstate(1);
+   break;
+   default:
+   break;
+   }
+   return;
 }
 
 void
@@ -599,7 +610,7 @@ do_action(struct ifsd_action *action)
  * Fetch the current link states.
  */
 void
-fetch_ifstate(void)
+fetch_ifstate(int do_eval)
 {
struct ifaddrs *ifap, *ifa;
 
@@ -610,7 +621,7 @@ fetch_ifstate(void)
if (ifa->ifa_addr->sa_family == AF_LINK) {
struct if_data *ifdata = ifa->ifa_data;
scan_ifstate(if_nametoindex(ifa->ifa_name),
-   ifdata->ifi_link_state, 0);
+   ifdata->ifi_link_state, do_eval);
}
}
 

switchd: no need to include err.h

[Rob Pierce]

switchd no longer requires err.h.

Index: packet.c
===
RCS file: /cvs/src/usr.sbin/switchd/packet.c,v
retrieving revision 1.4
diff -u -p -r1.4 packet.c
--- packet.c26 Sep 2016 08:55:43 -  1.4
+++ packet.c4 Aug 2017 00:03:59 -
@@ -29,7 +29,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/switchd/parse.y,v
retrieving revision 1.4
diff -u -p -r1.4 parse.y
--- parse.y 5 Jan 2017 12:42:19 -   1.4
+++ parse.y 4 Aug 2017 00:03:59 -
@@ -29,7 +29,6 @@
 #include 
 
 #include 
-#include 
 #include 
 #include 
 #include 

ifstated: consistent use of log.c

[Rob Pierce]

As a result ifstated.c no longer needs err.h.

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.56
diff -u -p -r1.56 ifstated.c
--- ifstated.c  24 Jul 2017 12:33:59 -  1.56
+++ ifstated.c  3 Aug 2017 23:59:13 -
@@ -37,7 +37,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -102,7 +101,7 @@ main(int argc, char *argv[])
break;
case 'D':
if (cmdline_symset(optarg) < 0)
-   errx(1, "could not parse macro definition %s",
+   fatalx("could not parse macro definition %s",
optarg);
break;
case 'f':
@@ -135,7 +134,7 @@ main(int argc, char *argv[])
if (opts & IFSD_OPT_NOACTION) {
if ((newconf = parse_config(configfile, opts)) == NULL)
exit(1);
-   warnx("configuration OK");
+   fprintf(stderr, "configuration OK\n");
exit(0);
}
 
@@ -147,7 +146,7 @@ main(int argc, char *argv[])
log_setverbose(opts & IFSD_OPT_VERBOSE);
 
if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
-   err(1, "no routing socket");
+   fatal("no routing socket");
 
rtfilter = ROUTE_FILTER(RTM_IFINFO);
if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
@@ -604,7 +603,7 @@ fetch_ifstate(void)
struct ifaddrs *ifap, *ifa;
 
if (getifaddrs() != 0)
-   err(1, "getifaddrs");
+   fatal("getifaddrs");
 
for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
if (ifa->ifa_addr->sa_family == AF_LINK) {

dhcpd: remove unused structs

[Rob Pierce]

I can confirm that the following structs have not been used since at least
OpenBSD 4.9. From Edgar Pettijohn.

Ok?

Index: dhcpd.h
===
RCS file: /cvs/src/usr.sbin/dhcpd/dhcpd.h,v
retrieving revision 1.65
diff -u -p -r1.65 dhcpd.h
--- dhcpd.h 31 Jul 2017 19:00:40 -  1.65
+++ dhcpd.h 1 Aug 2017 22:41:34 -
@@ -73,25 +73,6 @@ struct option_data {
u_int8_t *data;
 };
 
-struct string_list {
-   struct string_list *next;
-   char *string;
-};
-
-/* A name server, from /etc/resolv.conf. */
-struct name_server {
-   struct name_server *next;
-   struct sockaddr_in addr;
-   time_t rcdate;
-};
-
-/* A domain search list element. */
-struct domain_search_list {
-   struct domain_search_list *next;
-   char *domain;
-   time_t rcdate;
-};
-
 /* A dhcp packet and the pointers to its option values. */
 struct packet {
struct dhcp_packet *raw;
@@ -294,12 +275,6 @@ struct interface_info {
int is_udpsock;
ssize_t (*send_packet)(struct interface_info *, struct dhcp_packet *,
size_t, struct in_addr, struct sockaddr_in *, struct hardware *);
-};
-
-struct hardware_link {
-   struct hardware_link *next;
-   char name[IFNAMSIZ];
-   struct hardware address;
 };
 
 struct dhcpd_timeout {

Re: ifstated diff: handling interface depature/arrival

[Rob Pierce]

On Mon, Jul 31, 2017 at 05:59:46PM -0400, Rob Pierce wrote:
> Good evening all,
> 
> Currently, ifstated does not detect the removal of an IFT_CARP pseudo device.
> As such, you can delete a carp interface and have ifstated happily remain in
> the current state without detecting any interface change.
> 
> The reasons are two fold:
> 
> 1. The routing socket is only monitored for RTM_IFINFO messages and therefore
>misses interface departure and arrival events. On an interface departure,
>for example, there is only one RTM_IFINFO message, and that particular
>message identifies the departed (carp) interface as up and master.
> 
> 2. As I have recently discovered, LINK_STATE_UNKNOWN is equivalent to
>LINK_STATE_UP, so even if a removal was detected, the state change would go
>unnoticed (i.e. there would be no state change). I ran into this while
>trying to address (1).
> 
> The following diff resolves this problem by adding RTM_IFANNOUNCE to the
> routing socket filter, and the route message handler now handles interface
> departures and arrivals. Upon departure, a rescan is performed with a (forced)
> state of LINK_STATE_DOWN, and upon the arrival of a previously configured
> pseudo device the interface is re-indexed and a rescan is performed with the
> actual interface state.
> 
> I have also updated the regression test to cover carp interface departure and
> arrival (diff included for testing convenience).
> 
> At this stage I am only looking for general comments on the approach and/or
> some testing if you are an ifstated user.
> 
> For example, maybe it is unnecessary to save and check the interface type,
> assuming that interface departure and arrival will always be on a (supported)
> pseudo device (e.g. IFT_CARP), and actual physical interfaces will not behave
> in this manner. Also, ifi_type is not available via an ioctl under pledge, so
> obtaining this information dynamically through routing messages may be a bit
> of a hack.
> 
> Thanks and regards,
> 
> Rob

I need to rethink this approach. A departing interface must be handled
appropriately, but the arrival of a new interface is problematic as we really
need to confirm that the interface is configured exactly as expected (i.e. as
it was prior to departure).

Pledge currently prevents us from obtaining some of that information upon
arrival for comparison purposes (e.g. SIOCGVH). This will also be an issue on
reload under the current design, as interface parameters are not inherently
addressed - only their state is considered.

Maybe a simpler diff that only handles interface departure would be better,
with the assumption that the interface will not come back, and indeed is not
permitted to return after departure. This would at least ensure that a
fail-over was handled appropriately within a pair of CARPed firewealls on
interface departure. The departing interface would need to be re-configured
and ifstated restarted in order for that host to return to a normal operating
state.

Rob

> 
> Index: src/usr.sbin/ifstated/ifstated.c
> ===
> RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
> retrieving revision 1.56
> diff -u -p -r1.56 ifstated.c
> --- src/usr.sbin/ifstated/ifstated.c  24 Jul 2017 12:33:59 -  1.56
> +++ src/usr.sbin/ifstated/ifstated.c  31 Jul 2017 20:48:42 -
> @@ -28,6 +28,7 @@
>  #include 
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -73,6 +74,8 @@ voiddo_action(struct ifsd_action *);
>  void remove_action(struct ifsd_action *, struct ifsd_state *);
>  void remove_expression(struct ifsd_expression *,
>   struct ifsd_state *);
> +void set_iftype(int, u_char);
> +void reindex_if(int, char *);
>  
>  __dead void
>  usage(void)
> @@ -149,7 +152,7 @@ main(int argc, char *argv[])
>   if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
>   err(1, "no routing socket");
>  
> - rtfilter = ROUTE_FILTER(RTM_IFINFO);
> + rtfilter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_IFANNOUNCE);
>   if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
>   , sizeof(rtfilter)) == -1) /* not fatal */
>   log_warn("%s: setsockopt msgfilter", __func__);
> @@ -231,24 +234,79 @@ void
>  rt_msg_handler(int fd, short event, void *arg)
>  {
>   char msg[2048];
> + char ifname[IFNAMSIZ];
>   struct rt_msghdr *rtm = (struct rt_msghdr *)
>   struct if_msghdr ifm;
> + struct if_announcemsghdr ifan;
> + struct ifaddrs *ifap, *ifa;
>   ssize_t len;
>  
>   len = read(fd, msg, sizeof(msg));
>  
>   /* XXX ignore error

ifstated diff: handling interface depature/arrival

[Rob Pierce]

Good evening all,

Currently, ifstated does not detect the removal of an IFT_CARP pseudo device.
As such, you can delete a carp interface and have ifstated happily remain in
the current state without detecting any interface change.

The reasons are two fold:

1. The routing socket is only monitored for RTM_IFINFO messages and therefore
   misses interface departure and arrival events. On an interface departure,
   for example, there is only one RTM_IFINFO message, and that particular
   message identifies the departed (carp) interface as up and master.

2. As I have recently discovered, LINK_STATE_UNKNOWN is equivalent to
   LINK_STATE_UP, so even if a removal was detected, the state change would go
   unnoticed (i.e. there would be no state change). I ran into this while
   trying to address (1).

The following diff resolves this problem by adding RTM_IFANNOUNCE to the
routing socket filter, and the route message handler now handles interface
departures and arrivals. Upon departure, a rescan is performed with a (forced)
state of LINK_STATE_DOWN, and upon the arrival of a previously configured
pseudo device the interface is re-indexed and a rescan is performed with the
actual interface state.

I have also updated the regression test to cover carp interface departure and
arrival (diff included for testing convenience).

At this stage I am only looking for general comments on the approach and/or
some testing if you are an ifstated user.

For example, maybe it is unnecessary to save and check the interface type,
assuming that interface departure and arrival will always be on a (supported)
pseudo device (e.g. IFT_CARP), and actual physical interfaces will not behave
in this manner. Also, ifi_type is not available via an ioctl under pledge, so
obtaining this information dynamically through routing messages may be a bit
of a hack.

Thanks and regards,

Rob

Index: src/usr.sbin/ifstated/ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.56
diff -u -p -r1.56 ifstated.c
--- src/usr.sbin/ifstated/ifstated.c24 Jul 2017 12:33:59 -  1.56
+++ src/usr.sbin/ifstated/ifstated.c31 Jul 2017 20:48:42 -
@@ -28,6 +28,7 @@
 #include 
 
 #include 
+#include 
 #include 
 #include 
 
@@ -73,6 +74,8 @@ void  do_action(struct ifsd_action *);
 void   remove_action(struct ifsd_action *, struct ifsd_state *);
 void   remove_expression(struct ifsd_expression *,
struct ifsd_state *);
+void   set_iftype(int, u_char);
+void   reindex_if(int, char *);
 
 __dead void
 usage(void)
@@ -149,7 +152,7 @@ main(int argc, char *argv[])
if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
err(1, "no routing socket");
 
-   rtfilter = ROUTE_FILTER(RTM_IFINFO);
+   rtfilter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_IFANNOUNCE);
if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
, sizeof(rtfilter)) == -1) /* not fatal */
log_warn("%s: setsockopt msgfilter", __func__);
@@ -231,24 +234,79 @@ void
 rt_msg_handler(int fd, short event, void *arg)
 {
char msg[2048];
+   char ifname[IFNAMSIZ];
struct rt_msghdr *rtm = (struct rt_msghdr *)
struct if_msghdr ifm;
+   struct if_announcemsghdr ifan;
+   struct ifaddrs *ifap, *ifa;
ssize_t len;
 
len = read(fd, msg, sizeof(msg));
 
/* XXX ignore errors? */
-   if (len < sizeof(struct rt_msghdr))
+   if ((size_t)len < sizeof(rtm->rtm_msglen) || len < rtm->rtm_msglen)
return;
 
if (rtm->rtm_version != RTM_VERSION)
return;
 
-   if (rtm->rtm_type != RTM_IFINFO)
-   return;
-
-   memcpy(, rtm, sizeof(ifm));
-   scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
+   switch (rtm->rtm_type) {
+   /*
+* We can't access ifi_type via an ioctl under pledge, but we can get
+* it for free in an RTM_IFINFO and update ifstate accordingly. This is
+* (may be?) required for reindexing, and reindexing isn't necessary
+* until we have received an RTM_IFINFO message.
+*/
+   case RTM_IFINFO:
+   memcpy(, rtm, sizeof(ifm));
+   set_iftype(ifm.ifm_index, ifm.ifm_data.ifi_type);
+   scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
+   break;
+   case RTM_IFANNOUNCE:
+   memcpy(, rtm, sizeof(ifan));
+   switch (ifan.ifan_what) {
+   case IFAN_ARRIVAL:
+   /*
+* We only reindex IFT_CARP pseudo devices that have
+* already been configured, and then rescan with the
+* actual interface state.
+*/
+   log_warnx("interface #%d arrived", ifan.ifan_index);
+   

ioctl under route promise for pledging snmpd

[Rob Pierce]

snmpe calls kif_update on an interface change which performs an ioctl
with SIOCGIFDESCR, currently disallowed by pledge. No other network daemons do
this. The only other programs that make this call appear to be ifconfig and
systat.  ifnet.if_description simply contains an optional user defined
interface description.

vmd performs an ioctl with SIOCSIFDESCR to set ifnet.if_description, and this
is done in a privileged process that is not pledged.

The following diff proposal allows for an ioctl on SIOCGIFDESCR under a route
promise.

Thoughts?

Rob

Index: kern_pledge.c
===
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.216
diff -u -p -r1.216 kern_pledge.c
--- kern_pledge.c   29 Jun 2017 04:10:07 -  1.216
+++ kern_pledge.c   26 Jul 2017 18:14:04 -
@@ -1305,6 +1305,7 @@ pledge_ioctl(struct proc *p, long com, s
if ((p->p_p->ps_pledge & PLEDGE_ROUTE)) {
switch (com) {
case SIOCGIFADDR:
+   case SIOCGIFDESCR:
case SIOCGIFFLAGS:
case SIOCGIFMETRIC:
case SIOCGIFGMEMB:

pledge snmpctl

[Rob Pierce]

snmpclient pledges after calling chroot(2) and requires a dns promise for
sendto(2) with non-NULL destination.

Rob

Index: snmpclient.c
===
RCS file: /cvs/src/usr.sbin/snmpctl/snmpclient.c,v
retrieving revision 1.13
diff -u -p -r1.13 snmpclient.c
--- snmpclient.c16 Jan 2015 06:40:21 -  1.13
+++ snmpclient.c25 Jul 2017 19:05:37 -
@@ -160,6 +160,9 @@ snmpclient(struct parse_result *res)
 #endif
}
 
+   if (pledge("stdio dns", NULL) == -1)
+   fatal("pledge");
+
sc.sc_fd = s;
sc.sc_community = res->community;
sc.sc_version = res->version;
Index: snmpctl.c
===
RCS file: /cvs/src/usr.sbin/snmpctl/snmpctl.c,v
retrieving revision 1.22
diff -u -p -r1.22 snmpctl.c
--- snmpctl.c   28 Oct 2016 20:49:32 -  1.22
+++ snmpctl.c   25 Jul 2017 19:05:37 -
@@ -123,6 +123,8 @@ main(int argc, char *argv[])
usage();
break;
case SHOW_MIB:
+   if (pledge("stdio", NULL) == -1)
+   fatal("pledge");
show_mib();
break;
case WALK:
@@ -131,6 +133,8 @@ main(int argc, char *argv[])
snmpclient(res);
break;
default:
+   if (pledge("stdio unix", NULL) == -1)
+   fatal("pledge");
goto connect;
}
 
@@ -155,6 +159,9 @@ main(int argc, char *argv[])
}
err(1, "connect: %s", sock);
}
+
+   if (pledge("stdio", NULL) == -1)
+   fatal("pledge");
 
imsg_init(, ctl_sock);
done = 0;

Re: broken base build at src/usr/lib/libpcap?

[Rob Pierce]

> From: "Marc Espie" <es...@nerim.net>
> To: "Rob Pierce" <r...@2keys.ca>
> Cc: "tech" <tech@openbsd.org>
> Sent: Sunday, July 23, 2017 7:36:29 PM
> Subject: Re: broken base build at src/usr/lib/libpcap?

> On Sun, Jul 23, 2017 at 05:40:24PM -0400, Rob Pierce wrote:
> > yacc -ppcap_yy -d grammar.y
>> cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval
>> -DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON -DHAVE_STRERROR
>> -DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H -DINET6
> > -DHAVE_BSD_IEEE80211 -c -o grammar.o y.tab.c
> > rm -f y.tab.c
> > lex -Ppcap_yy scanner.l
>> cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval
>> -DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON -DHAVE_STRERROR
>> -DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H -DINET6
> > -DHAVE_BSD_IEEE80211 -c -o scanner.o lex.yy.c
> > cc: lex.yy.c: No such file or directory
> > cc: no input files

> Building part of source seldom goes well.
> Especially WITHOUT updating share/mk first.
Yes, sorry for the noise. All I needed to do was review the FAQ section on 
following -current: 

http://www.openbsd.org/faq/current.html 

Rob 

Re: broken base build at src/usr/lib/libpcap?

[Rob Pierce]

> From: "Rob Pierce" <r...@2keys.ca>
> To: "tech" <tech@openbsd.org>
> Sent: Sunday, July 23, 2017 5:40:24 PM
> Subject: broken base build at src/usr/lib/libpcap?

> My build breaks at usr/lib/libpcap:
Sorry, I jumped the gun. Works fine after another update. 

broken base build at src/usr/lib/libpcap?

[Rob Pierce]

My build breaks at usr/lib/libpcap:

cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c pcap.c -o pcap.o
cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c inet.c -o inet.o
cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c gencode.c -o gencode.o
cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c optimize.c -o optimize.o
cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c nametoaddr.c -o nametoaddr.o
cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c etherent.c -o etherent.o
cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c savefile.c -o savefile.o
cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c /usr/src/lib/libpcap/../../sys/net/bpf_filter.c -o 
bpf_filter.o
cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c bpf_image.c -o bpf_image.o
yacc -ppcap_yy -d grammar.y
cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c -o grammar.o y.tab.c
rm -f y.tab.c
lex -Ppcap_yy scanner.l
cc -O2 -pipe -g -I. -I/usr/src/lib/libpcap -Dyylval=pcap_yylval 
-DHAVE_SYS_IOCCOM_H -DHAVE_SYS_SOCKIO_H -DHAVE_ETHER_HOSTTON  -DHAVE_STRERROR 
-DHAVE_SOCKADDR_SA_LEN -DLBL_ALIGN -DHAVE_IFADDRS_H  -DINET6 
-DHAVE_BSD_IEEE80211   -c -o scanner.o lex.yy.c 
cc: lex.yy.c: No such file or directory
cc: no input files
*** Error 1 in /usr/src/lib/libpcap (:196 'scanner.o')

unused struct in dhcpd.h

[Rob Pierce]

This struct appears to be unused at the moment.

Regards,

Rob

Index: dhcpd.h
===
RCS file: /cvs/src/usr.sbin/dhcpd/dhcpd.h,v
retrieving revision 1.64
diff -u -p -r1.64 dhcpd.h
--- dhcpd.h 24 Apr 2017 14:58:36 -  1.64
+++ dhcpd.h 23 Jul 2017 15:19:18 -
@@ -54,11 +54,6 @@ struct iaddr {
unsigned char iabuf[16];
 };
 
-struct iaddrlist {
-   struct iaddrlist *next;
-   struct iaddr addr;
-};
-
 #define DEFAULT_HASH_SIZE  97
 
 struct hash_bucket {

simple ifstated pledge

[Rob Pierce]

With the most recent commit ifstated can now be pledged in a straight forward
manner. A better pledge is possible with more work.

Does it make sense to get this one in now?

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.52
diff -u -p -r1.52 ifstated.c
--- ifstated.c  21 Jul 2017 16:32:18 -  1.52
+++ ifstated.c  22 Jul 2017 03:58:23 -
@@ -160,6 +160,9 @@ main(int argc, char *argv[])
, sizeof(rtfilter)) == -1) /* not fatal */
log_warn("%s: setsockopt tablefilter", __func__);
 
+   if (pledge("stdio rpath inet proc exec", NULL) == -1)
+   fatal("pledge");
+
signal_set(_ev, SIGCHLD, sigchld_handler, NULL);
signal_add(_ev, NULL);
 

Re: pledge ifstated

[Rob Pierce]

On Sun, Jul 16, 2017 at 04:47:07PM -0400, Rob Pierce wrote:
> On Thu, Jul 13, 2017 at 09:16:14PM -0400, Rob Pierce wrote:
> > On Mon, Jul 10, 2017 at 01:21:58PM -0400, Rob Pierce wrote:
> > > The following diff is loosely based on the approach that was taken for
> > > pledging mountd. Other code/approaches leveraged from various networking
> > > daemons.
> > > 
> > > This first step moves the ioctl with SIOCGIFDATA call to a privileged
> > > child so we can at least pledge "stdio rpath dns inet proc exec" without
> > > (intentionally) changing existing behaviour.
> > > 
> > > Comments and suggestions welcome.
> > > 
> > > Thanks!
> > > 
> > > Rob
> > 
> > An unnecessary call to log_info snuck in. Here is a clean diff.
> > 
> > Rob
> 
> My original diff to initially pledge ifstated with "stdio rpath dns inet proc
> exec" was incorrectly polling from fetch_ifstate which resulted in the delayed
> completion of that function. This new diff resolves the problem.
> 
> I also changed one instance of fatal to fatalx in sigchld_handler.

Running setproctitle in the parent breaks rcctl.

This diff removes the parent call to setproctitle so rcctl works again.

Rob

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

Re: pledge ifstated

[Rob Pierce]

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

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

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

Regards,

Rob

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

ifstated diff cleanup before delegating proc/exec to privchild

[Rob Pierce]

This diff should probably be three separate diffs, but changes are colliding
(i.e. adjacent lines of code are being modified within different diffs).

The diff contains three modifications:

 - removal of unused logging code (see previous diff);
 - a cleanup of parse.y, converting it to use log.c; and
 - string to character array changes in preparation for the next pledge diff.

Regards,

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.50
diff -u -p -r1.50 ifstated.c
--- ifstated.c  4 Jul 2017 21:09:52 -   1.50
+++ ifstated.c  15 Jul 2017 17:56:31 -
@@ -639,7 +639,6 @@ clear_config(struct ifsd_config *oconf)
TAILQ_REMOVE(>states, state, entries);
remove_action(state->init, state);
remove_action(state->body, state);
-   free(state->name);
free(state);
}
remove_action(oconf->initstate.init, >initstate);
@@ -656,12 +655,7 @@ remove_action(struct ifsd_action *action
return;
 
switch (action->type) {
-   case IFSD_ACTION_LOG:
-   free(action->act.logmessage);
-   break;
case IFSD_ACTION_COMMAND:
-   free(action->act.command);
-   break;
case IFSD_ACTION_CHANGESTATE:
break;
case IFSD_ACTION_CONDITION:
@@ -697,7 +691,6 @@ remove_expression(struct ifsd_expression
if (--expression->u.external->refcount == 0) {
TAILQ_REMOVE(>external_tests,
expression->u.external, entries);
-   free(expression->u.external->command);
event_del(>u.external->ev);
free(expression->u.external);
}
Index: ifstated.h
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
retrieving revision 1.16
diff -u -p -r1.16 ifstated.h
--- ifstated.h  4 Jul 2017 21:04:14 -   1.16
+++ ifstated.h  15 Jul 2017 17:56:31 -
@@ -28,6 +28,7 @@
 
 #include 
 #include 
+#include 
 
 struct ifsd_expression;
 TAILQ_HEAD(ifsd_expression_list, ifsd_expression);
@@ -45,15 +46,15 @@ struct ifsd_ifstate {
 };
 
 struct ifsd_external {
-   TAILQ_ENTRY(ifsd_external)   entries;
-   struct event ev;
-   struct ifsd_expression_list  expressions;
-   char*command;
-   int  prevstatus;
-   u_int32_tfrequency;
-   u_int32_trefcount;
-   u_int32_tlastexec;
-   pid_tpid;
+   TAILQ_ENTRY(ifsd_external)  entries;
+   struct eventev;
+   struct ifsd_expression_list expressions;
+   charcommand[LINE_MAX];
+   int prevstatus;
+   u_int32_t   frequency;
+   u_int32_t   refcount;
+   u_int32_t   lastexec;
+   pid_t   pid;
 };
 
 struct ifsd_action;
@@ -63,17 +64,15 @@ struct ifsd_action {
TAILQ_ENTRY(ifsd_action) entries;
struct ifsd_action  *parent;
union {
-   char*logmessage;
-   char*command;
+   charcommand[LINE_MAX];
struct ifsd_state   *nextstate;
-   char*statename;
+   charstatename[NAME_MAX];
struct {
struct ifsd_action_list  actions;
struct ifsd_expression  *expression;
} c;
} act;
u_int32_ttype;
-#define IFSD_ACTION_LOG0
 #define IFSD_ACTION_COMMAND1
 #define IFSD_ACTION_CHANGESTATE2
 #define IFSD_ACTION_CONDITION  3
@@ -111,7 +110,7 @@ struct ifsd_state {
struct ifsd_action  *init;
struct ifsd_action  *body;
u_int32_tentered;
-   char*name;
+   charname[NAME_MAX];
 };
 
 TAILQ_HEAD(ifsd_state_list, ifsd_state);
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ifstated/parse.y,v
retrieving revision 1.44
diff -u -p -r1.44 parse.y
--- parse.y 4 Jul 2017 21:13:03 -   1.44
+++ parse.y 15 Jul 2017 17:56:31 -
@@ -190,11 +190,12 @@ action: RUN STRING{
struct ifsd_action *action;
 
if ((action = calloc(1, sizeof(*action))) == NULL)

ifstated regress statemachine whitespace diff

[Rob Pierce]

Remove some unwanted whitespace that creeped in.

Rob

Index: statemachine
===
RCS file: /cvs/src/regress/usr.sbin/ifstated/statemachine,v
retrieving revision 1.1
diff -u -p -r1.1 statemachine
--- statemachine14 Jul 2017 10:41:30 -  1.1
+++ statemachine14 Jul 2017 11:52:58 -
@@ -74,7 +74,7 @@ test1 = '( "test -f ${FILE1}" every $EVE
 test2 = '( "test -f ${FILE2}" every $EVERY )'
 state one {
init {
-   run "sleep $SLEEP  && ( test -f ${FILE2} || touch ${FILE1} )"
+   run "sleep $SLEEP && ( test -f ${FILE2} || touch ${FILE1} )"
}
if \$test1 && ! \$test2
set-state two
@@ -100,14 +100,14 @@ state five {
init {
run "sleep $SLEEP && touch ${FILE1}"
}
-   if ( \$false || \$true ) && ( ! \$true || \$test1  )
+   if ( \$false || \$true ) && ( ! \$true || \$test1 )
set-state six
 }
 state six {
init {
run "sleep $SLEEP && touch ${FILE1}"
}
-   if ( \$false || \$true ) && ( ! \$true || \$test1  )
+   if ( \$false || \$true ) && ( ! \$true || \$test1 )
if \$true
set-state seven
 }

Re: pledge ifstated

[Rob Pierce]

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

An unnecessary call to log_info snuck in. Here is a clean diff.

Rob

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

Re: add simple ifstated regression test script

[Rob Pierce]

Sure, no problem. Thank you. 

Rob 




From: "Sebastian Benoit" <be...@openbsd.org> 
To: "Rob Pierce" <r...@2keys.ca> 
Cc: "tech" <tech@openbsd.org> 
Sent: Thursday, July 13, 2017 6:12:14 PM 
Subject: Re: add simple ifstated regression test script 




BQ_BEGIN
Hi, 

i wanted to commit this, but saw that it does not have a licence yet. 

Can i add /usr/share/misc/license.template with your name and email-Adress? 

/Benno 

Rob Pierce(r...@2keys.ca) on 2017.07.06 13:12:26 -0400: 
> On Sun, Jul 02, 2017 at 06:29:07PM +0200, Sebastian Benoit wrote: 
> > Rob Pierce(r...@2keys.ca) on 2017.07.02 12:06:25 -0400: 
> > > I am currently using this regression script for basic ifstated sanity 
> > > testing. 
> > > 
> > > Still a work in progress. Requesting commit for safe keeping. 
> > 
> > 
> > Hi, 
> > 
> > this should go into /usr/src/regress/usr.sbin/ifstated 
> > (which does not esist yet). 
> > 
> > Also, it should hook into the regress framework (bsd.regress.mk(5)). 
> > 
> > As it needs some network configuration, maybe it should be similar to 
> > relayd regress tests. 
> > 
> > Happy to work with you on that. 
> > 
> > /B. 
> 
> I have updated the ifstated regression scripts based on your feedback. 
> 
> I also added a script to test drive the state machine. 
> 
> Both should be more systematic in coverage, but hopefully it is a good start. 
> 
> Regards, 
> 
> Rob 
> 
> Index: regress/usr.sbin/ifstated/Makefile 
> === 
> RCS file: regress/usr.sbin/ifstated/Makefile 
> diff -N regress/usr.sbin/ifstated/Makefile 
> --- /dev/null 1 Jan 1970 00:00:00 - 
> +++ regress/usr.sbin/ifstated/Makefile 6 Jul 2017 16:55:57 - 
> @@ -0,0 +1,13 @@ 
> +# $OpenBSD$ 
> + 
> +# Regress tests for ifstated 
> + 
> +REGRESS_TARGETS = run-regress-statemachine run-regress-ifstated 
> + 
> +run-regress-statemachine: 
> + sh ${.CURDIR}/statemachine 
> + 
> +run-regress-ifstated: 
> + sh ${.CURDIR}/ifstated 
> + 
> +.include  
> 
> Index: regress/usr.sbin/ifstated/ifstated 
> === 
> RCS file: regress/usr.sbin/ifstated/ifstated 
> diff -N regress/usr.sbin/ifstated/ifstated 
> --- /dev/null 1 Jan 1970 00:00:00 - 
> +++ regress/usr.sbin/ifstated/ifstated 6 Jul 2017 16:55:57 - 
> @@ -0,0 +1,148 @@ 
> +# $OpenBSD$ 
> + 
> +# Basic ifstated regression script to test interface changes. 
> + 
> +# Golbal variables 
> +VHIDA=252 
> +VHIDB=253 
> +PREFIX=172.16.0 
> +DEMOTE=ifconfig 
> +PROMOTE=ifconfig 
> +EVERY=5 
> +SLEEP=10 
> + 
> +cleanup() { 
> + ifconfig carp${VHIDA} destroy > /dev/null 2>&1 
> + ifconfig carp${VHIDB} destroy > /dev/null 2>&1 
> + rm working/ifstated.conf >/dev/null 2>&1 
> + rm working/ifstated.log >/dev/null 2>&1 
> + rm working/output.test >/dev/null 2>&1 
> + rm working/output.new >/dev/null 2>&1 
> + rm working/nohup.out >/dev/null 2>&1 
> + rmdir working >/dev/null 2>&1 
> +} 
> + 
> +fail() { 
> + echo FAILED 
> + cleanup 
> + exit 1 
> +} 
> + 
> +skip() { 
> + echo SKIPPED 
> + cleanup 
> + exit 0 
> +} 
> + 
> +trap 'skip' INT 
> + 
> +# look for a suitable physical interface for carp 
> +NIC="$(netstat -rn -finet | grep ^default | awk '{ print $8 }')" 
> +STATUS="$(ifconfig | grep -A5 ^${NIC} | grep status: | awk '{ print $2 }')" 
> + 
> +if [ "$STATUS" != "active" ] 
> +then 
> + echo "No suitable physical interface found." 
> + echo SKIPPED 
> + exit 0 
> +fi 
> + 
> +if [ "$(pgrep ifstated)" ] 
> +then 
> + echo "The ifstated daemon is already running." 
> + echo SKIPPED 
> + exit 0 
> +fi 
> + 
> +for interface in carp${VHIDA} carp${VHIDB} 
> +do 
> + ifconfig ${interface} > /dev/null 2>&1 
> + if [ $? -eq 0 ] 
> + then 
> + echo "Interface $interface already exists." 
> + echo SKIPPED 
> + exit 0 
> + fi 
> +done 
> + 
> +mkdir -p working 
> + 
> +cat > working/ifstated.conf < +# This is a config template for ifstated regression testing 
> +carp = "carp${VHIDA}.link.up" 
> +init-state primary 
> +net = '( "ping -q -c 1 -w 1 ${PREFIX}.${VHIDB} > /dev/null" every ${EVERY})' 
> +state primary { 
> + init { 
> + run "ifconfig" 
> + } 
> + if ! \$net 
> + set-state demoted 
> + if ! \$carp 
> + set-state demoted 
&g

pledge ifstated

[Rob Pierce]

The following diff is loosely based on the approach that was taken for
pledging mountd. Other code/approaches leveraged from various networking
daemons.

This first step moves the ioctl with SIOCGIFDATA call to a privileged
child so we can at least pledge "stdio rpath dns inet proc exec" without
(intentionally) changing existing behaviour.

Comments and suggestions welcome.

Thanks!

Rob

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

Re: add simple ifstated regression test script

[Rob Pierce]

On Sun, Jul 02, 2017 at 06:29:07PM +0200, Sebastian Benoit wrote:
> Rob Pierce(r...@2keys.ca) on 2017.07.02 12:06:25 -0400:
> > I am currently using this regression script for basic ifstated sanity 
> > testing.
> > 
> > Still a work in progress. Requesting commit for safe keeping.
> 
> 
> Hi,
> 
> this should go into /usr/src/regress/usr.sbin/ifstated
> (which does not esist yet).
> 
> Also, it should hook into the regress framework (bsd.regress.mk(5)).
> 
> As it needs some network configuration, maybe it should be similar to
> relayd regress tests.
> 
> Happy to work with you on that.
> 
> /B.

I have updated the ifstated regression scripts based on your feedback.

I also added a script to test drive the state machine.

Both should be more systematic in coverage, but hopefully it is a good start.

Regards,

Rob

Index: regress/usr.sbin/ifstated/Makefile
===
RCS file: regress/usr.sbin/ifstated/Makefile
diff -N regress/usr.sbin/ifstated/Makefile
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.sbin/ifstated/Makefile  6 Jul 2017 16:55:57 -
@@ -0,0 +1,13 @@
+# $OpenBSD$
+
+# Regress tests for ifstated
+
+REGRESS_TARGETS =  run-regress-statemachine run-regress-ifstated
+
+run-regress-statemachine:
+   sh ${.CURDIR}/statemachine
+
+run-regress-ifstated:
+   sh ${.CURDIR}/ifstated
+
+.include 

Index: regress/usr.sbin/ifstated/ifstated
===
RCS file: regress/usr.sbin/ifstated/ifstated
diff -N regress/usr.sbin/ifstated/ifstated
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.sbin/ifstated/ifstated  6 Jul 2017 16:55:57 -
@@ -0,0 +1,148 @@
+# $OpenBSD$
+
+# Basic ifstated regression script to test interface changes.
+
+# Golbal variables
+VHIDA=252
+VHIDB=253
+PREFIX=172.16.0
+DEMOTE=ifconfig
+PROMOTE=ifconfig
+EVERY=5
+SLEEP=10
+
+cleanup() {
+   ifconfig carp${VHIDA} destroy > /dev/null 2>&1
+   ifconfig carp${VHIDB} destroy > /dev/null 2>&1
+   rm working/ifstated.conf >/dev/null 2>&1
+   rm working/ifstated.log >/dev/null 2>&1
+   rm working/output.test >/dev/null 2>&1
+   rm working/output.new >/dev/null 2>&1
+   rm working/nohup.out >/dev/null 2>&1
+   rmdir working >/dev/null 2>&1
+}
+
+fail() {
+   echo FAILED
+   cleanup
+   exit 1
+}
+
+skip() {
+   echo SKIPPED
+   cleanup
+   exit 0
+}
+
+trap 'skip' INT
+
+# look for a suitable physical interface for carp
+NIC="$(netstat -rn -finet | grep ^default | awk '{ print $8 }')"
+STATUS="$(ifconfig | grep -A5 ^${NIC} | grep status: | awk '{ print $2 }')"
+
+if [ "$STATUS" != "active" ]
+then
+   echo "No suitable physical interface found."
+   echo SKIPPED
+   exit 0
+fi
+
+if [ "$(pgrep ifstated)" ]
+then
+   echo "The ifstated daemon is already running."
+   echo SKIPPED
+   exit 0
+fi
+
+for interface in carp${VHIDA} carp${VHIDB}
+do
+   ifconfig ${interface} > /dev/null 2>&1
+   if [ $? -eq 0 ]
+   then
+   echo "Interface $interface already exists."
+   echo SKIPPED
+   exit 0
+   fi
+done
+
+mkdir -p working
+
+cat > working/ifstated.conf < working/output.test < ifstated.log 2>&1) &
+
+sleep ${SLEEP}
+ifconfig carp${VHIDA} down
+sleep ${SLEEP}
+ifconfig carp${VHIDA} up
+sleep ${SLEEP}
+ifconfig carp${VHIDB} destroy
+sleep ${SLEEP}
+ifconfig carp${VHIDB} inet ${PREFIX}.${VHIDB} netmask 255.255.255.0 broadcast \
+   ${PREFIX}.255 vhid ${VHIDB} carpdev ${NIC}
+sleep ${SLEEP}
+ifconfig carp${VHIDA} down
+sleep ${SLEEP}
+ifconfig carp${VHIDB} destroy
+sleep ${SLEEP}
+ifconfig carp${VHIDA} up
+sleep ${SLEEP}
+ifconfig carp${VHIDB} inet ${PREFIX}.${VHIDB} netmask 255.255.255.0 broadcast \
+   ${PREFIX}.255 vhid ${VHIDB} carpdev ${NIC}
+sleep ${SLEEP}
+kill -HUP $(pgrep ifstated) >/dev/null 2>&1
+sleep ${SLEEP}
+
+grep ^changing working/ifstated.log > working/output.new
+
+kill $(pgrep ifstated) >/dev/null 2>&1
+
+diff working/output.test working/output.new
+case $? in
+0) echo PASSED
+   cleanup
+   exit 0
+   ;;
+1) fail
+   ;;
+esac

Index: regress/usr.sbin/ifstated/statemachine
===
RCS file: regress/usr.sbin/ifstated/statemachine
diff -N regress/usr.sbin/ifstated/statemachine
--- /dev/null   1 Jan 1970 00:00:00 -
+++ regress/usr.sbin/ifstated/statemachine  6 Jul 2017 16:55:57 -
@@ -0,0 +1,183 @@
+# $OpenBSD$
+
+# Basic ifstated regression script to test the finite state machine.
+
+#
+# NOTE: Increase LSLEEP as required when adding additional test states.
+#
+
+# Golbal variables

ifstated remove unused logging code

[Rob Pierce]

This code has been here since version 1.1/1.2, but never used.

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.50
diff -u -p -r1.50 ifstated.c
--- ifstated.c  4 Jul 2017 21:09:52 -   1.50
+++ ifstated.c  6 Jul 2017 01:04:40 -
@@ -656,9 +656,6 @@ remove_action(struct ifsd_action *action
return;
 
switch (action->type) {
-   case IFSD_ACTION_LOG:
-   free(action->act.logmessage);
-   break;
case IFSD_ACTION_COMMAND:
free(action->act.command);
break;

Index: ifstated.h
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
retrieving revision 1.16
diff -u -p -r1.16 ifstated.h
--- ifstated.h  4 Jul 2017 21:04:14 -   1.16
+++ ifstated.h  6 Jul 2017 01:04:40 -
@@ -63,7 +63,6 @@ struct ifsd_action {
TAILQ_ENTRY(ifsd_action) entries;
struct ifsd_action  *parent;
union {
-   char*logmessage;
char*command;
struct ifsd_state   *nextstate;
char*statename;
@@ -73,7 +72,6 @@ struct ifsd_action {
} c;
} act;
u_int32_ttype;
-#define IFSD_ACTION_LOG0
 #define IFSD_ACTION_COMMAND1
 #define IFSD_ACTION_CHANGESTATE2
 #define IFSD_ACTION_CONDITION  3

Re: ifstated diff rename variables to avoid state confusion

[Rob Pierce]

On Mon, Jul 03, 2017 at 04:24:30PM -0400, Rob Pierce wrote:
> ifstated monitors interface state and the return state of invoked commands,
> and takes action accordingly, all of which is managed with the help of a
> finite state machine. That makes for a lot of "state" references in the code.
> 
> The following diff renames variables to make a distinction between link status
> and command execution status (now referenced as "status"), and the various 
> states
> within the state machine (referred to as "state").
> 
> As an example of the current confusion, the link_states() function in parse.y
> actually links states within the finite state machine (as oppose to having
> anything to do with interface link states).
> 
> This diff helps to make my head hurt less.
> 
> For your consideration.
> 
> Regards,
> 
> Rob

Ok, that was overkill. I would be happy with the following diff instead.

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.49
diff -u -p -r1.49 ifstated.c
--- ifstated.c  3 Jul 2017 18:45:34 -   1.49
+++ ifstated.c  4 Jul 2017 11:31:08 -
@@ -64,7 +64,7 @@ void  check_external_status(struct ifsd_
 void   external_evtimer_setup(struct ifsd_state *, int);
 void   scan_ifstate(int, int, int);
 intscan_ifstate_single(int, int, struct ifsd_state *);
-void   fetch_state(void);
+void   fetch_ifstate(void);
 __dead voidusage(void);
 void   adjust_expressions(struct ifsd_expression_list *, int);
 void   adjust_external_expressions(struct ifsd_state *);
@@ -208,7 +208,7 @@ load_config(void)
clear_config(conf);
conf = newconf;
conf->initstate.entered = time(NULL);
-   fetch_state();
+   fetch_ifstate();
external_evtimer_setup(>initstate, IFSD_EVTIMER_ADD);
adjust_external_expressions(>initstate);
eval_state(>initstate);
@@ -597,7 +597,7 @@ do_action(struct ifsd_action *action)
  * Fetch the current link states.
  */
 void
-fetch_state(void)
+fetch_ifstate(void)
 {
struct ifaddrs *ifap, *ifa;
char *oname = NULL;

ifstated parse.y removed unused tokens

[Rob Pierce]

These tokens have existed since version 1.1 but have never been used.

Can we delete them?

Rob

Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ifstated/parse.y,v
retrieving revision 1.43
diff -u -p -r1.43 parse.y
--- parse.y 2 Jul 2017 15:28:26 -   1.43
+++ parse.y 4 Jul 2017 09:33:17 -
@@ -106,7 +106,7 @@ typedef struct {
 %}
 
 %token STATE INITSTATE
-%token LINK UP DOWN UNKNOWN ADDED REMOVED
+%token LINK UP DOWN UNKNOWN
 %token IF RUN SETSTATE EVERY INIT
 %left  AND OR
 %left  UNARY
@@ -390,14 +390,12 @@ lookup(char *s)
/* this has to be sorted always */
static const struct keywords keywords[] = {
{ "&&", AND},
-   { "added",  ADDED},
{ "down",   DOWN},
{ "every",  EVERY},
{ "if", IF},
{ "init",   INIT},
{ "init-state", INITSTATE},
{ "link",   LINK},
-   { "removed",REMOVED},
{ "run",RUN},
{ "set-state",  SETSTATE},
{ "state",  STATE},

ifstated diff rename variables to avoid state confusion

[Rob Pierce]

ifstated monitors interface state and the return state of invoked commands,
and takes action accordingly, all of which is managed with the help of a
finite state machine. That makes for a lot of "state" references in the code.

The following diff renames variables to make a distinction between link status
and command execution status (now referenced as "status"), and the various 
states
within the state machine (referred to as "state").

As an example of the current confusion, the link_states() function in parse.y
actually links states within the finite state machine (as oppose to having
anything to do with interface link states).

This diff helps to make my head hurt less.

For your consideration.

Regards,

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.49
diff -u -p -r1.49 ifstated.c
--- ifstated.c  3 Jul 2017 18:45:34 -   1.49
+++ ifstated.c  3 Jul 2017 19:59:43 -
@@ -18,7 +18,7 @@
  */
 
 /*
- * ifstated listens to link_state transitions on interfaces
+ * ifstated listens to link status transitions on interfaces
  * and executes predefined commands.
  */
 
@@ -62,9 +62,9 @@ void  external_handler(int, short, void 
 void   external_exec(struct ifsd_external *, int);
 void   check_external_status(struct ifsd_state *);
 void   external_evtimer_setup(struct ifsd_state *, int);
-void   scan_ifstate(int, int, int);
-intscan_ifstate_single(int, int, struct ifsd_state *);
-void   fetch_state(void);
+void   scan_ifstatus(int, int, int);
+intscan_ifstatus_single(int, int, struct ifsd_state *);
+void   fetch_ifstatus(void);
 __dead voidusage(void);
 void   adjust_expressions(struct ifsd_expression_list *, int);
 void   adjust_external_expressions(struct ifsd_state *);
@@ -208,7 +208,7 @@ load_config(void)
clear_config(conf);
conf = newconf;
conf->initstate.entered = time(NULL);
-   fetch_state();
+   fetch_ifstatus();
external_evtimer_setup(>initstate, IFSD_EVTIMER_ADD);
adjust_external_expressions(>initstate);
eval_state(>initstate);
@@ -246,7 +246,7 @@ rt_msg_handler(int fd, short event, void
return;
 
memcpy(, rtm, sizeof(ifm));
-   scan_ifstate(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
+   scan_ifstatus(ifm.ifm_index, ifm.ifm_data.ifi_link_state, 1);
 }
 
 void
@@ -419,37 +419,37 @@ external_evtimer_setup(struct ifsd_state
 #defineLINK_STATE_IS_DOWN(_s)  (!LINK_STATE_IS_UP((_s)))
 
 int
-scan_ifstate_single(int ifindex, int s, struct ifsd_state *state)
+scan_ifstatus_single(int ifindex, int s, struct ifsd_state *state)
 {
-   struct ifsd_ifstate *ifstate;
+   struct ifsd_ifstatus *ifstatus;
struct ifsd_expression_list expressions;
int changed = 0;
 
TAILQ_INIT();
 
-   TAILQ_FOREACH(ifstate, >interface_states, entries) {
-   if (ifstate->ifindex == ifindex) {
-   if (ifstate->prevstate != s &&
-   (ifstate->prevstate != -1 || !opt_inhibit)) {
+   TAILQ_FOREACH(ifstatus, >interfaces_status, entries) {
+   if (ifstatus->ifindex == ifindex) {
+   if (ifstatus->prevstatus != s &&
+   (ifstatus->prevstatus != -1 || !opt_inhibit)) {
struct ifsd_expression *expression;
int truth;
 
truth =
-   (ifstate->ifstate == IFSD_LINKUNKNOWN &&
+   (ifstatus->ifstatus == IFSD_LINKUNKNOWN &&
s == LINK_STATE_UNKNOWN) ||
-   (ifstate->ifstate == IFSD_LINKDOWN &&
+   (ifstatus->ifstatus == IFSD_LINKDOWN &&
LINK_STATE_IS_DOWN(s)) ||
-   (ifstate->ifstate == IFSD_LINKUP &&
+   (ifstatus->ifstatus == IFSD_LINKUP &&
LINK_STATE_IS_UP(s));
 
TAILQ_FOREACH(expression,
-   >expressions, entries) {
+   >expressions, entries) {
expression->truth = truth;
TAILQ_INSERT_TAIL(,
expression, eval);
changed = 1;
}
-   ifstate->prevstate = s;
+   ifstatus->prevstatus = s;
}
}
}
@@ -460,15 +460,15 @@ scan_ifstate_single(int ifindex, int s, 
 }
 
 void
-scan_ifstate(int 

ifstated whitespace diff

[Rob Pierce]

Fix some variable alignment whitespace.

Rob

Index: ifstated.h
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
retrieving revision 1.15
diff -u -p -r1.15 ifstated.h
--- ifstated.h  2 Jul 2017 15:28:26 -   1.15
+++ ifstated.h  3 Jul 2017 19:40:21 -
@@ -92,9 +92,9 @@ struct ifsd_expression {
} u;
int  depth;
u_int32_ttype;
-#define IFSD_OPER_AND  1
-#define IFSD_OPER_OR   2
-#define IFSD_OPER_NOT  3
+#define IFSD_OPER_AND  1
+#define IFSD_OPER_OR   2
+#define IFSD_OPER_NOT  3
 #define IFSD_OPER_EXTERNAL 4
 #define IFSD_OPER_IFSTATE  5
u_int8_t truth;

Re: ifstated readability diff

[Rob Pierce]

On Sun, Jul 02, 2017 at 11:50:56PM -0400, Rob Pierce wrote:
> Remove obvious clear_config() comments and misleading state_change() comments.
> 
> Also relocate do_action() calls for the init block from change_state() to
> occur with the corresponding do_action() calls for the body block within
> the calling function for improved readability.
> 
> No functional change.
> 
> Rob

That diff was broken. This one is correct.

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.48
diff -u -p -r1.48 ifstated.c
--- ifstated.c  2 Jul 2017 15:28:26 -   1.48
+++ ifstated.c  3 Jul 2017 13:42:13 -
@@ -217,8 +217,10 @@ load_config(void)
conf->curstate->entered = time(NULL);
conf->nextstate = conf->curstate;
conf->curstate = NULL;
-   while (state_change())
+   while (state_change()) {
+   do_action(conf->curstate->init);
do_action(conf->curstate->body);
+   }
}
return (0);
 }
@@ -533,14 +535,13 @@ eval_state(struct ifsd_state *state)
if (external == NULL || external->lastexec >= state->entered ||
external->lastexec == 0) {
do_action(state->body);
-   while (state_change())
+   while (state_change()) {
+   do_action(conf->curstate->init);
do_action(conf->curstate->body);
+   }
}
 }
 
-/*
- *If a previous action included a state change, process it.
- */
 int
 state_change(void)
 {
@@ -556,7 +557,6 @@ state_change(void)
conf->curstate->entered = time(NULL);
external_evtimer_setup(conf->curstate, IFSD_EVTIMER_ADD);
adjust_external_expressions(conf->curstate);
-   do_action(conf->curstate->init);
return (1);
}
return (0);
@@ -627,9 +627,6 @@ fetch_state(void)
close(sock);
 }
 
-/*
- * Clear the config.
- */
 void
 clear_config(struct ifsd_config *oconf)
 {

ifstated readability diff

[Rob Pierce]

Remove obvious clear_config() comments and misleading state_change() comments.

Also relocate do_action() calls for the init block from change_state() to
occur with the corresponding do_action() calls for the body block within
the calling function for improved readability.

No functional change.

Rob


Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.48
diff -u -p -r1.48 ifstated.c
--- ifstated.c  2 Jul 2017 15:28:26 -   1.48
+++ ifstated.c  3 Jul 2017 03:41:14 -
@@ -218,6 +218,7 @@ load_config(void)
conf->nextstate = conf->curstate;
conf->curstate = NULL;
while (state_change())
+   do_action(conf->curstate->init);
do_action(conf->curstate->body);
}
return (0);
@@ -534,13 +535,11 @@ eval_state(struct ifsd_state *state)
external->lastexec == 0) {
do_action(state->body);
while (state_change())
+   do_action(conf->curstate->init);
do_action(conf->curstate->body);
}
 }
 
-/*
- *If a previous action included a state change, process it.
- */
 int
 state_change(void)
 {
@@ -556,7 +555,6 @@ state_change(void)
conf->curstate->entered = time(NULL);
external_evtimer_setup(conf->curstate, IFSD_EVTIMER_ADD);
adjust_external_expressions(conf->curstate);
-   do_action(conf->curstate->init);
return (1);
}
return (0);
@@ -627,9 +625,6 @@ fetch_state(void)
close(sock);
 }
 
-/*
- * Clear the config.
- */
 void
 clear_config(struct ifsd_config *oconf)
 {

Re: add simple ifstated regression test script

[Rob Pierce]

> From: "Sebastian Benoit" <be...@openbsd.org>
> To: "Rob Pierce" <r...@2keys.ca>
> Cc: "tech" <tech@openbsd.org>
> Sent: Sunday, July 2, 2017 12:29:07 PM
> Subject: Re: add simple ifstated regression test script

> Rob Pierce(r...@2keys.ca) on 2017.07.02 12:06:25 -0400:
> > I am currently using this regression script for basic ifstated sanity 
> > testing.

> > Still a work in progress. Requesting commit for safe keeping.

> Hi,

> this should go into /usr/src/regress/usr.sbin/ifstated
> (which does not esist yet).

> Also, it should hook into the regress framework (bsd.regress.mk(5)).

> As it needs some network configuration, maybe it should be similar to
> relayd regress tests.

> Happy to work with you on that.

> /B.
I have it locally in src/usr.sbin/regress/ but did my cvs diff too high in the 
tree. :) 

I didn't want to hook it up yet, I just wanted to commit it so I can continue 
to work on it in the tree. 

I have been studying the other regression scripts, but took the same approach 
as disklabel (i.e. just a script for local use). 

The next most straight forward example I found was newfs. 

I can work on it a bit more and send a new diff for review and comments. 

Regards, 

Rob 

add simple ifstated regression test script

[Rob Pierce]

I am currently using this regression script for basic ifstated sanity testing.

Still a work in progress. Requesting commit for safe keeping.

Regards,

Rob

Index: usr.sbin/ifstated/ifstated.sh
===
RCS file: usr.sbin/ifstated/ifstated.sh
diff -N usr.sbin/ifstated/ifstated.sh
--- /dev/null   1 Jan 1970 00:00:00 -
+++ usr.sbin/ifstated/ifstated.sh   2 Jul 2017 16:01:29 -
@@ -0,0 +1,156 @@
+# $OpenBSD$
+
+# basic ifstated regression test script
+
+# Modify variables as required
+NIC=em0
+VHID=254
+PREFIX=172.16.0
+PING=8.8.8.8
+DEMOTE=ifconfig
+PROMOTE=ifconfig
+EVERY=3
+SLEEP=6
+
+if [ "$(pgrep ifstated)" ]
+then
+   echo "The ifstated daemon is already running."
+   echo "Please stop ifstated before commencing regression testing."
+   exit 1
+fi
+
+if [ -n "$(grep ifstated_test_host /etc/hosts)" ]
+then
+   echo "ifstated_test_host is already present in /etc/hosts."
+   echo "Please remove before commencing regression testing."
+   exit 1
+fi
+
+if [ -n "$(grep 1.2.3.4 /etc/hosts)" ]
+then
+   echo "1.2.3.4 is already present in /etc/hosts."
+   echo "Please remove before commencing regression testing."
+   exit 1
+fi
+
+if [ -f /etc/hostname.carp${VHID} ]
+then
+   echo "/etc/hostname.carp${VHID} already exists."
+   echo "Please remove before commencing regression testing."
+   exit 1
+else
+   echo "inet ${PREFIX}.${VHID} 255.255.255.0 ${PREFIX}.255 vhid ${VHID} 
carpdev ${NIC}" > hostname.carp${VHID}
+   cp hostname.carp${VHID} /etc
+   chmod 0640 /etc/hostname.carp${VHID}
+fi
+
+mkdir -p working
+cd working
+
+cat > output.test <> /etc/hosts < ifstated.conf < ifstated.log &
+echo
+
+echo -n "Performing ifstated regression testing..."
+sleep ${SLEEP}
+ifconfig ${NIC} down
+sleep ${SLEEP}
+sh /etc/netstart ${NIC} >/dev/null 2>&1
+sleep ${SLEEP}
+ifconfig carp${VHID} down
+sleep ${SLEEP}
+sh /etc/netstart carp${VHID}
+sleep ${SLEEP}
+ifconfig ${NIC} down
+ifconfig carp${VHID} down
+sleep ${SLEEP}
+sh /etc/netstart ${NIC} >/dev/null 2>&1
+sleep ${SLEEP}
+sh /etc/netstart carp${VHID}
+sleep ${SLEEP}
+kill -HUP $(pgrep ifstated)
+sleep ${SLEEP}
+cat /etc/hosts | sed "s/.* ifstated_test_host/1.2.3.4 ifstated_test_host/" > 
/etc/hosts
+sleep ${SLEEP}
+cat /etc/hosts | sed "s/.* ifstated_test_host/${PING} ifstated_test_host/" > 
/etc/hosts
+sleep ${SLEEP}
+echo
+
+grep ^changing ifstated.log > output.new
+
+echo -n "Stopping ifstated..."
+kill $(pgrep ifstated)
+echo
+
+diff output.test output.new
+case $? in
+0) echo Test Passed.
+   echo -n "Cleaning up..."
+   ifconfig carp${VHID} down
+   ifconfig carp${VHID} delete
+   ifconfig carp${VHID} destroy
+   rm /etc/hostname.carp${VHID}
+   cp ./hosts /etc
+   echo
+   echo Done.
+   exit 0
+   ;;
+1) echo Test Failed.
+   echo Please examine the interfaces and review files in the working 
directory.
+   echo You will need to manually delete the carp interface and remove 
residual files.
+   exit 1
+   ;;
+esac

ifstated ifsd_config variable name change

[Rob Pierce]

Second diff to rename additional "always" variable in struct ifsd_config.

Requested by stsp@.

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.47
diff -u -p -r1.47 ifstated.c
--- ifstated.c  2 Jul 2017 14:28:45 -   1.47
+++ ifstated.c  2 Jul 2017 14:47:49 -
@@ -207,11 +207,11 @@ load_config(void)
if (conf != NULL)
clear_config(conf);
conf = newconf;
-   conf->always.entered = time(NULL);
+   conf->initstate.entered = time(NULL);
fetch_state();
-   external_evtimer_setup(>always, IFSD_EVTIMER_ADD);
-   adjust_external_expressions(>always);
-   eval_state(>always);
+   external_evtimer_setup(>initstate, IFSD_EVTIMER_ADD);
+   adjust_external_expressions(>initstate);
+   eval_state(>initstate);
if (conf->curstate != NULL) {
log_info("initial state: %s", conf->curstate->name);
conf->curstate->entered = time(NULL);
@@ -250,7 +250,7 @@ rt_msg_handler(int fd, short event, void
 void
 sigchld_handler(int fd, short event, void *arg)
 {
-   check_external_status(>always);
+   check_external_status(>initstate);
if (conf->curstate != NULL)
check_external_status(conf->curstate);
 }
@@ -463,8 +463,8 @@ scan_ifstate(int ifindex, int s, int do_
struct ifsd_state *state;
int cur_eval = 0;
 
-   if (scan_ifstate_single(ifindex, s, >always) && do_eval)
-   eval_state(>always);
+   if (scan_ifstate_single(ifindex, s, >initstate) && do_eval)
+   eval_state(>initstate);
TAILQ_FOREACH(state, >states, entries) {
if (scan_ifstate_single(ifindex, s, state) &&
(do_eval && state == conf->curstate))
@@ -635,7 +635,7 @@ clear_config(struct ifsd_config *oconf)
 {
struct ifsd_state *state;
 
-   external_evtimer_setup(>always, IFSD_EVTIMER_DEL);
+   external_evtimer_setup(>initstate, IFSD_EVTIMER_DEL);
if (conf != NULL && conf->curstate != NULL)
external_evtimer_setup(conf->curstate, IFSD_EVTIMER_DEL);
while ((state = TAILQ_FIRST(>states)) != NULL) {
@@ -645,8 +645,8 @@ clear_config(struct ifsd_config *oconf)
free(state->name);
free(state);
}
-   remove_action(oconf->always.init, >always);
-   remove_action(oconf->always.body, >always);
+   remove_action(oconf->initstate.init, >initstate);
+   remove_action(oconf->initstate.body, >initstate);
free(oconf);
 }
 
Index: ifstated.h
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
retrieving revision 1.14
diff -u -p -r1.14 ifstated.h
--- ifstated.h  2 Jul 2017 14:30:35 -   1.14
+++ ifstated.h  2 Jul 2017 14:47:49 -
@@ -117,7 +117,7 @@ struct ifsd_state {
 TAILQ_HEAD(ifsd_state_list, ifsd_state);
 
 struct ifsd_config {
-   struct ifsd_statealways;
+   struct ifsd_stateinitstate;
struct ifsd_state_list   states;
struct ifsd_state   *curstate;
struct ifsd_state   *nextstate;

Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ifstated/parse.y,v
retrieving revision 1.42
diff -u -p -r1.42 parse.y
--- parse.y 2 Jul 2017 14:27:30 -   1.42
+++ parse.y 2 Jul 2017 14:47:49 -
@@ -242,12 +242,12 @@ init  : INIT {
if (curstate != NULL)
curaction = curstate->init;
else
-   curaction = conf->always.init;
+   curaction = conf->initstate.init;
} action_block {
if (curstate != NULL)
curaction = curstate->body;
else
-   curaction = conf->always.body;
+   curaction = conf->initstate.body;
}
;
 
@@ -343,7 +343,7 @@ state   : STATE string {
} optnl '{' optnl stateopts_l '}' {
TAILQ_INSERT_TAIL(>states, curstate, entries);
curstate = NULL;
-   curaction = conf->always.body;
+   curaction = conf->initstate.body;
}
;
 
@@ -746,8 +746,8 @@ parse_config(char *filename, int opts)
 
TAILQ_INIT(>states);
 
-   init_state(>always);
-   curaction = conf->always.body;
+   init_state(>initstate);
+   curaction = conf->initstate.body;
conf->opts = opts;
 
yyparse();
@@ -943,7 +943,7 @@ new_ifstate(u_short ifindex, int s)
if (curstate != NULL)
state = curstate;
else

Re: rename variable in ifstated

[Rob Pierce]

On Sun, Jul 02, 2017 at 02:56:06PM +0200, Stefan Sperling wrote:
> On Sat, Jul 01, 2017 at 05:53:54PM -0400, Rob Pierce wrote:
> > never.never say always.always.
> > 
> > Rename one of the "always" variables to "body" for improved readability.
> > 
> > No functional change.
> > 
> > >From ifstated.conf(5):
> > 
> > "Each state consistes of an init block and a body. The init block is used
> > to initialize the state and is executed each time the state is entered.
> > The body of a state is only executed when that state is the current state
> > and an event occurs."
> > 
> > Rob
> 
> I agree with this change.
> 
> Could you make the same change for struct ifsd_config in a follow-up diff?

Will do. I have made the change for struct ifsd_config locally, but wanted to
get this first change committed prior to sending the next diff to make it less
confusing.

It is still somewhat awkward because there is both an init state (i.e.
init-state) as well as an init block within each state. I have renamed the
second (struct ifsd_config) instance of "always" to "initstate".

Stay tuned.

Rob

ifstated unused variable

[Rob Pierce]

Remove unused variable from header file.

Index: ifstated.h
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
retrieving revision 1.12
diff -u -p -r1.12 ifstated.h
--- ifstated.h  28 Jun 2017 11:10:08 -  1.12
+++ ifstated.h  2 Jul 2017 04:30:01 -
@@ -70,7 +70,6 @@ struct ifsd_action {
struct {
struct ifsd_action_list  actions;
struct ifsd_expression  *expression;
-   u_int8_t ignore_init;
} c;
} act;
u_int32_ttype;

ifstated whitespace var assignment diff

[Rob Pierce]

Remove variable assignment in declaration and add whitespace to improve
readability. No functional change.

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.45
diff -u -p -r1.45 ifstated.c
--- ifstated.c  28 Jun 2017 11:10:08 -  1.45
+++ ifstated.c  2 Jul 2017 03:52:04 -
@@ -527,7 +527,9 @@ adjust_expressions(struct ifsd_expressio
 void
 eval_state(struct ifsd_state *state)
 {
-   struct ifsd_external *external = TAILQ_FIRST(>external_tests);
+   struct ifsd_external *external;
+
+   external = TAILQ_FIRST(>external_tests);
if (external == NULL || external->lastexec >= state->entered ||
external->lastexec == 0) {
do_action(state->always);

rename variable in ifstated

[Rob Pierce]

never.never say always.always.

Rename one of the "always" variables to "body" for improved readability.

No functional change.

>From ifstated.conf(5):

"Each state consistes of an init block and a body. The init block is used
to initialize the state and is executed each time the state is entered.
The body of a state is only executed when that state is the current state
and an event occurs."

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.45
diff -u -p -r1.45 ifstated.c
--- ifstated.c  28 Jun 2017 11:10:08 -  1.45
+++ ifstated.c  1 Jul 2017 21:49:47 -
@@ -218,7 +218,7 @@ load_config(void)
conf->nextstate = conf->curstate;
conf->curstate = NULL;
while (state_change())
-   do_action(conf->curstate->always);
+   do_action(conf->curstate->body);
}
return (0);
 }
@@ -530,9 +530,9 @@ eval_state(struct ifsd_state *state)
struct ifsd_external *external = TAILQ_FIRST(>external_tests);
if (external == NULL || external->lastexec >= state->entered ||
external->lastexec == 0) {
-   do_action(state->always);
+   do_action(state->body);
while (state_change())
-   do_action(conf->curstate->always);
+   do_action(conf->curstate->body);
}
 }
 
@@ -639,12 +639,12 @@ clear_config(struct ifsd_config *oconf)
while ((state = TAILQ_FIRST(>states)) != NULL) {
TAILQ_REMOVE(>states, state, entries);
remove_action(state->init, state);
-   remove_action(state->always, state);
+   remove_action(state->body, state);
free(state->name);
free(state);
}
remove_action(oconf->always.init, >always);
-   remove_action(oconf->always.always, >always);
+   remove_action(oconf->always.body, >always);
free(oconf);
 }
 
Index: ifstated.h
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
retrieving revision 1.12
diff -u -p -r1.12 ifstated.h
--- ifstated.h  28 Jun 2017 11:10:08 -  1.12
+++ ifstated.h  1 Jul 2017 21:49:47 -
@@ -110,7 +110,7 @@ struct ifsd_state {
struct ifsd_external_listexternal_tests;
TAILQ_ENTRY(ifsd_state)  entries;
struct ifsd_action  *init;
-   struct ifsd_action  *always;
+   struct ifsd_action  *body;
u_int32_tentered;
char*name;
 };
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ifstated/parse.y,v
retrieving revision 1.41
diff -u -p -r1.41 parse.y
--- parse.y 18 Jun 2017 12:03:47 -  1.41
+++ parse.y 1 Jul 2017 21:49:47 -
@@ -245,9 +245,9 @@ init: INIT {
curaction = conf->always.init;
} action_block {
if (curstate != NULL)
-   curaction = curstate->always;
+   curaction = curstate->body;
else
-   curaction = conf->always.always;
+   curaction = conf->always.body;
}
;
 
@@ -339,11 +339,11 @@ state : STATE string {
init_state(state);
state->name = $2;
curstate = state;
-   curaction = state->always;
+   curaction = state->body;
} optnl '{' optnl stateopts_l '}' {
TAILQ_INSERT_TAIL(>states, curstate, entries);
curstate = NULL;
-   curaction = conf->always.always;
+   curaction = conf->always.body;
}
;
 
@@ -747,7 +747,7 @@ parse_config(char *filename, int opts)
TAILQ_INIT(>states);
 
init_state(>always);
-   curaction = conf->always.always;
+   curaction = conf->always.body;
conf->opts = opts;
 
yyparse();
@@ -755,7 +755,7 @@ parse_config(char *filename, int opts)
/* Link states */
TAILQ_FOREACH(state, >states, entries) {
link_states(state->init);
-   link_states(state->always);
+   link_states(state->body);
}
 
errors = file->errors;
@@ -928,10 +928,10 @@ init_state(struct ifsd_state *state)
state->init->type = IFSD_ACTION_CONDITION;
TAILQ_INIT(>init->act.c.actions);
 
-   if ((state->always = calloc(1, sizeof(*state->always))) == NULL)
+   if ((state->body = calloc(1, sizeof(*state->body))) == NULL)
 

remove errant ifstated whitespace

[Rob Pierce]

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.43
diff -u -p -r1.43 ifstated.c
--- ifstated.c  27 Jun 2017 20:46:34 -  1.43
+++ ifstated.c  28 Jun 2017 01:30:02 -
@@ -151,12 +151,12 @@ main(int argc, char *argv[])
 
rtfilter = ROUTE_FILTER(RTM_IFINFO);
if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
-   , sizeof(rtfilter)) == -1) /* not fatal */
+   , sizeof(rtfilter)) == -1) /* not fatal */
log_warn("%s: setsockopt msgfilter", __func__);
 
rtfilter = RTABLE_ANY;
if (setsockopt(rt_fd, PF_ROUTE, ROUTE_TABLEFILTER,
-   , sizeof(rtfilter)) == -1) /* not fatal */
+   , sizeof(rtfilter)) == -1) /* not fatal */
log_warn("%s: setsockopt tablefilter", __func__);
 
signal_set(_ev, SIGCHLD, sigchld_handler, NULL);
@@ -605,7 +605,7 @@ fetch_state(void)
 
for (ifa = ifap; ifa; ifa = ifa->ifa_next) {
struct ifreq ifr;
-   struct if_data  ifrdat;
+   struct if_data ifrdat;
 
if (oname && !strcmp(oname, ifa->ifa_name))
continue;
@@ -623,8 +623,6 @@ fetch_state(void)
freeifaddrs(ifap);
close(sock);
 }
-
-
 
 /*
  * Clear the config.
Index: ifstated.h
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
retrieving revision 1.11
diff -u -p -r1.11 ifstated.h
--- ifstated.h  18 Jun 2017 12:03:47 -  1.11
+++ ifstated.h  28 Jun 2017 01:30:02 -
@@ -29,7 +29,6 @@
 #include 
 #include 
 
-
 struct ifsd_expression;
 TAILQ_HEAD(ifsd_expression_list, ifsd_expression);
 
@@ -80,7 +79,6 @@ struct ifsd_action {
 #define IFSD_ACTION_CHANGESTATE2
 #define IFSD_ACTION_CONDITION  3
 };
-
 
 struct ifsd_expression {
TAILQ_ENTRY(ifsd_expression) entries;

ifstated.c hoist code in prep for future work

[Rob Pierce]

Hoist some privileged code in preparation for future work.

Is this the correct use of intptr_t?

Based on an approach in vmd with mc146818/ns8250.

No intended functional change.

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.42
diff -u -p -r1.42 ifstated.c
--- ifstated.c  18 Jun 2017 12:03:47 -  1.42
+++ ifstated.c  27 Jun 2017 16:44:16 -
@@ -36,6 +36,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -87,8 +88,9 @@ int
 main(int argc, char *argv[])
 {
struct timeval tv;
-   int ch;
+   int ch, rt_fd;
int debug = 0;
+   unsigned int rtfilter;
 
log_init(1, LOG_DAEMON);/* log to stderr until daemonized */
log_setverbose(1);
@@ -144,12 +146,25 @@ main(int argc, char *argv[])
log_init(debug, LOG_DAEMON);
log_setverbose(opts & IFSD_OPT_VERBOSE);
 
+   if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
+   err(1, "no routing socket");
+
+   rtfilter = ROUTE_FILTER(RTM_IFINFO);
+   if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
+   , sizeof(rtfilter)) == -1) /* not fatal */
+   log_warn("%s: setsockopt msgfilter", __func__);
+
+   rtfilter = RTABLE_ANY;
+   if (setsockopt(rt_fd, PF_ROUTE, ROUTE_TABLEFILTER,
+   , sizeof(rtfilter)) == -1) /* not fatal */
+   log_warn("%s: setsockopt tablefilter", __func__);
+
signal_set(_ev, SIGCHLD, sigchld_handler, NULL);
signal_add(_ev, NULL);
 
/* Loading the config needs to happen in the event loop */
timerclear();
-   evtimer_set(_ev, startup_handler, NULL);
+   evtimer_set(_ev, startup_handler, (void *)(intptr_t)rt_fd);
evtimer_add(_ev, );
 
event_loop(0);
@@ -159,28 +174,14 @@ main(int argc, char *argv[])
 void
 startup_handler(int fd, short event, void *arg)
 {
-   int rt_fd;
-   unsigned int rtfilter;
-
-   if ((rt_fd = socket(PF_ROUTE, SOCK_RAW, 0)) < 0)
-   err(1, "no routing socket");
+   int rfd = (intptr_t)arg;
 
if (load_config() != 0) {
log_warnx("unable to load config");
exit(1);
}
-
-   rtfilter = ROUTE_FILTER(RTM_IFINFO);
-   if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
-   , sizeof(rtfilter)) == -1) /* not fatal */
-   log_warn("%s: setsockopt msgfilter", __func__);
-
-   rtfilter = RTABLE_ANY;
-   if (setsockopt(rt_fd, PF_ROUTE, ROUTE_TABLEFILTER,
-   , sizeof(rtfilter)) == -1) /* not fatal */
-   log_warn("%s: setsockopt tablefilter", __func__);

-   event_set(_msg_ev, rt_fd, EV_READ|EV_PERSIST, rt_msg_handler, NULL);
+   event_set(_msg_ev, rfd, EV_READ|EV_PERSIST, rt_msg_handler, NULL);
event_add(_msg_ev, NULL);
 
signal_set(_ev, SIGHUP, sighup_handler, NULL);

minor bgpd.c diff

[Rob Pierce]

rfd does not need to be global.

Rob

Index: bgpd.c
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
retrieving revision 1.189
diff -u -p -r1.189 bgpd.c
--- bgpd.c  28 May 2017 15:16:33 -  1.189
+++ bgpd.c  27 Jun 2017 15:52:02 -
@@ -48,7 +48,6 @@ int   dispatch_imsg(struct imsgbuf *, int
 intcontrol_setup(struct bgpd_config *);
 intimsg_send_sockets(struct imsgbuf *, struct imsgbuf *);
 
-int rfd = -1;
 int cflags;
 volatile sig_atomic_t   mrtdump;
 volatile sig_atomic_t   quit;
@@ -108,6 +107,7 @@ main(int argc, char *argv[])
char*saved_argv0;
int  debug = 0;
int  rflag = 0, sflag = 0;
+   int  rfd = -1;
int  ch, timeout, status;
int  pipe_m2s[2];
int  pipe_m2r[2];

Re: update logging in ifstated

[Rob Pierce]

On Tue, Jun 13, 2017 at 08:44:46AM +0200, Sebastian Benoit wrote:
> Rob Pierce(r...@2keys.ca) on 2017.06.11 18:04:31 -0400:
> > This minimizes differences with the latest log.c.
> > 
> > I was not sure how to handle verbosity, as the current implementation is
> > verbose by default in debug mode. The diff below requires actually
> > requesting (double) verbosity on the command line in order to retain the
> > same behaviour (in debug mode).
> 
> Thanks.
> 
> can you redo this with the log.c from bgpd/ospfd/... ?
> The difference is exactly in the handling of verbose, and that way we dont
> change the -v semantics right now.
> 
> I havent unified the verbose handling yet, but if you diff them you'll see
> its only a small difference now. My plan is to narrow all the log.c users
> down to this small difference and then fix that in all of them in one go.
> 
> Also please add a log.h with the declarations just like in bgpd/ospfd/...
> 
> /Benno

Ah yes, thank you for the pointer. How is this?

log.[ch] are now identical to bgpd and ospfd.

Rob

cvs server: Diffing .
Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.41
diff -u -p -r1.41 ifstated.c
--- ifstated.c  30 May 2013 19:22:48 -  1.41
+++ ifstated.c  13 Jun 2017 23:09:05 -
@@ -36,12 +36,14 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 
 #include "ifstated.h"
+#include "log.h"
 
 struct  ifsd_config *conf = NULL, *newconf = NULL;
 
@@ -88,7 +90,8 @@ main(int argc, char *argv[])
int ch;
int debug = 0;
 
-   log_init(1);
+   log_init(1, LOG_DAEMON);/* log to stderr until daemonized */
+   log_setverbose(1);
 
while ((ch = getopt(argc, argv, "dD:f:hniv")) != -1) {
switch (ch) {
@@ -138,7 +141,8 @@ main(int argc, char *argv[])
daemon(1, 0);
 
event_init();
-   log_init(debug);
+   log_init(debug, LOG_DAEMON);
+   log_setverbose(opts & IFSD_OPT_VERBOSE);
 
signal_set(_ev, SIGCHLD, sigchld_handler, NULL);
signal_add(_ev, NULL);
@@ -169,12 +173,12 @@ startup_handler(int fd, short event, voi
rtfilter = ROUTE_FILTER(RTM_IFINFO);
if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
, sizeof(rtfilter)) == -1) /* not fatal */
-   log_warn("startup_handler: setsockopt msgfilter");
+   log_warn("%s: setsockopt msgfilter", __func__);
 
rtfilter = RTABLE_ANY;
if (setsockopt(rt_fd, PF_ROUTE, ROUTE_TABLEFILTER,
, sizeof(rtfilter)) == -1) /* not fatal */
-   log_warn("startup_handler: setsockopt tablefilter");
+   log_warn("%s: setsockopt tablefilter", __func__);

event_set(_msg_ev, rt_fd, EV_READ|EV_PERSIST, rt_msg_handler, NULL);
event_add(_msg_ev, NULL);
@@ -580,7 +584,7 @@ do_action(struct ifsd_action *action)
}
break;
default:
-   log_debug("do_action: unknown action %d", action->type);
+   log_debug("%s: unknown action %d", __func__, action->type);
break;
}
 }
Index: ifstated.h
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
retrieving revision 1.10
diff -u -p -r1.10 ifstated.h
--- ifstated.h  19 Jul 2016 08:04:53 -  1.10
+++ ifstated.h  13 Jun 2017 23:09:05 -
@@ -135,20 +135,3 @@ enum   { IFSD_EVTIMER_ADD, IFSD_EVTIMER_DE
 struct ifsd_config *parse_config(char *, int);
 intcmdline_symset(char *);
 void   clear_config(struct ifsd_config *);
-
-/* log.c */
-void   log_init(int);
-void   log_warn(const char *, ...)
-   __attribute__((__format__ (printf, 1, 2)));
-void   log_warnx(const char *, ...)
-   __attribute__((__format__ (printf, 1, 2)));
-void   log_info(const char *, ...)
-   __attribute__((__format__ (printf, 1, 2)));
-void   log_debug(const char *, ...)
-   __attribute__((__format__ (printf, 1, 2)));
-void   vlog(int, const char *, va_list)
-   __attribute__((__format__ (printf, 2, 0)));
-void   logit(int, const char *, ...)
-   __attribute__((__format__ (printf, 2, 3)));
-void fatal(const char *) __dead;
-void fatalx(const char *) __dead;
Index: log.c
===
RCS file: /cvs/src/usr.sbin/ifstated/log.c,v
retrieving revision 1.4
diff -u -p -r1.4 log.c
--- log.c   21 Mar 2017 12:06:55 -  1.4
+++ log.c   13 Jun 2017 23:09:05 -
@@ -16,40 +16,55 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-#include 
-#include 
 #includ

update logging in ifstated

[Rob Pierce]

This minimizes differences with the latest log.c.

I was not sure how to handle verbosity, as the current implementation is
verbose by default in debug mode. The diff below requires actually
requesting (double) verbosity on the command line in order to retain the
same behaviour (in debug mode).

Rob

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.41
diff -u -p -r1.41 ifstated.c
--- ifstated.c  30 May 2013 19:22:48 -  1.41
+++ ifstated.c  11 Jun 2017 21:51:16 -
@@ -32,10 +32,11 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -86,14 +87,12 @@ main(int argc, char *argv[])
 {
struct timeval tv;
int ch;
-   int debug = 0;
-
-   log_init(1);
+   int debug = 0, verbose = 0;
 
while ((ch = getopt(argc, argv, "dD:f:hniv")) != -1) {
switch (ch) {
case 'd':
-   debug = 1;
+   debug = 2;
break;
case 'D':
if (cmdline_symset(optarg) < 0)
@@ -113,6 +112,7 @@ main(int argc, char *argv[])
opt_inhibit = 1;
break;
case 'v':
+   verbose++;
if (opts & IFSD_OPT_VERBOSE)
opts |= IFSD_OPT_VERBOSE2;
opts |= IFSD_OPT_VERBOSE;
@@ -122,6 +122,9 @@ main(int argc, char *argv[])
}
}
 
+   /* log to stderr until daemonized */
+   log_init(debug ? debug : 1, LOG_DAEMON);
+
argc -= optind;
argv += optind;
if (argc > 0)
@@ -138,7 +141,8 @@ main(int argc, char *argv[])
daemon(1, 0);
 
event_init();
-   log_init(debug);
+   log_init(debug, LOG_DAEMON);
+   log_setverbose(verbose);
 
signal_set(_ev, SIGCHLD, sigchld_handler, NULL);
signal_add(_ev, NULL);
@@ -169,12 +173,12 @@ startup_handler(int fd, short event, voi
rtfilter = ROUTE_FILTER(RTM_IFINFO);
if (setsockopt(rt_fd, PF_ROUTE, ROUTE_MSGFILTER,
, sizeof(rtfilter)) == -1) /* not fatal */
-   log_warn("startup_handler: setsockopt msgfilter");
+   log_warn("%s: setsockopt msgfilter", __func__);
 
rtfilter = RTABLE_ANY;
if (setsockopt(rt_fd, PF_ROUTE, ROUTE_TABLEFILTER,
, sizeof(rtfilter)) == -1) /* not fatal */
-   log_warn("startup_handler: setsockopt tablefilter");
+   log_warn("%s: setsockopt tablefilter", __func__);

event_set(_msg_ev, rt_fd, EV_READ|EV_PERSIST, rt_msg_handler, NULL);
event_add(_msg_ev, NULL);
@@ -580,7 +584,7 @@ do_action(struct ifsd_action *action)
}
break;
default:
-   log_debug("do_action: unknown action %d", action->type);
+   log_debug("%s: unknown action %d", __func__, action->type);
break;
}
 }
Index: ifstated.h
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.h,v
retrieving revision 1.10
diff -u -p -r1.10 ifstated.h
--- ifstated.h  19 Jul 2016 08:04:53 -  1.10
+++ ifstated.h  11 Jun 2017 21:51:16 -
@@ -137,7 +137,10 @@ intcmdline_symset(char *);
 void   clear_config(struct ifsd_config *);
 
 /* log.c */
-void   log_init(int);
+void   log_init(int, int);
+void   log_procinit(const char *);
+void   log_setverbose(int);
+intlog_getverbose(void);
 void   log_warn(const char *, ...)
__attribute__((__format__ (printf, 1, 2)));
 void   log_warnx(const char *, ...)
@@ -146,9 +149,11 @@ void   log_info(const char *, ...)
__attribute__((__format__ (printf, 1, 2)));
 void   log_debug(const char *, ...)
__attribute__((__format__ (printf, 1, 2)));
-void   vlog(int, const char *, va_list)
-   __attribute__((__format__ (printf, 2, 0)));
 void   logit(int, const char *, ...)
__attribute__((__format__ (printf, 2, 3)));
-void fatal(const char *) __dead;
-void fatalx(const char *) __dead;
+void   vlog(int, const char *, va_list)
+   __attribute__((__format__ (printf, 2, 0)));
+__dead void fatal(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
+__dead void fatalx(const char *, ...)
+   __attribute__((__format__ (printf, 1, 2)));
Index: log.c
===
RCS file: /cvs/src/usr.sbin/ifstated/log.c,v
retrieving revision 1.4
diff -u -p -r1.4 log.c
--- log.c   21 Mar 2017 12:06:55 -  1.4
+++ log.c   11 Jun 2017 21:51:16 -
@@ -16,40 +16,74 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-#include 

ifstated.c diff for __dead usage

[Rob Pierce]

Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.41
diff -u -p -r1.41 ifstated.c
--- ifstated.c  30 May 2013 19:22:48 -  1.41
+++ ifstated.c  20 Mar 2017 00:19:52 -
@@ -50,28 +50,28 @@ int  opt_inhibit = 0;
 char   *configfile = "/etc/ifstated.conf";
 struct event   rt_msg_ev, sighup_ev, startup_ev, sigchld_ev;
 
-void   startup_handler(int, short, void *);
-void   sighup_handler(int, short, void *);
-intload_config(void);
-void   sigchld_handler(int, short, void *);
-void   rt_msg_handler(int, short, void *);
-void   external_handler(int, short, void *);
-void   external_exec(struct ifsd_external *, int);
-void   check_external_status(struct ifsd_state *);
-void   external_evtimer_setup(struct ifsd_state *, int);
-void   scan_ifstate(int, int, int);
-intscan_ifstate_single(int, int, struct ifsd_state *);
-void   fetch_state(void);
-void   usage(void);
-void   adjust_expressions(struct ifsd_expression_list *, int);
-void   adjust_external_expressions(struct ifsd_state *);
-void   eval_state(struct ifsd_state *);
-intstate_change(void);
-void   do_action(struct ifsd_action *);
-void   remove_action(struct ifsd_action *, struct ifsd_state *);
-void   remove_expression(struct ifsd_expression *, struct ifsd_state *);
+void   startup_handler(int, short, void *);
+void   sighup_handler(int, short, void *);
+intload_config(void);
+void   sigchld_handler(int, short, void *);
+void   rt_msg_handler(int, short, void *);
+void   external_handler(int, short, void *);
+void   external_exec(struct ifsd_external *, int);
+void   check_external_status(struct ifsd_state *);
+void   external_evtimer_setup(struct ifsd_state *, int);
+void   scan_ifstate(int, int, int);
+intscan_ifstate_single(int, int, struct ifsd_state *);
+void   fetch_state(void);
+__dead voidusage(void);
+void   adjust_expressions(struct ifsd_expression_list *, int);
+void   adjust_external_expressions(struct ifsd_state *);
+void   eval_state(struct ifsd_state *);
+intstate_change(void);
+void   do_action(struct ifsd_action *);
+void   remove_action(struct ifsd_action *, struct ifsd_state *);
+void   remove_expression(struct ifsd_expression *, struct ifsd_state 
*);
 
-void
+__dead void
 usage(void)
 {
extern char *__progname;

minor diff for switchd.8

[Rob Pierce]

Index: switchd.8
===
RCS file: /cvs/src/usr.sbin/switchd/switchd.8,v
retrieving revision 1.3
diff -u -p -r1.3 switchd.8
--- switchd.8   20 Oct 2016 19:55:29 -  1.3
+++ switchd.8   29 Nov 2016 02:49:53 -
@@ -29,7 +29,7 @@
 .Op Fl t Ar timeout
 .Sh DESCRIPTION
 .Nm
-is an controller for software-defined networking (SDN) and is
+is a controller for software-defined networking (SDN) and is
 compatible with the OpenFlow protocol.
 .Pp
 The options are as follows:

minor typo in aldap.c comment

[Rob Pierce]

Index: aldap.c
===
RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
retrieving revision 1.32
diff -u -p -r1.32 aldap.c
--- aldap.c 27 Apr 2016 10:53:27 -  1.32
+++ aldap.c 22 Oct 2016 02:42:13 -
@@ -1198,7 +1198,7 @@ isu8cont(unsigned char c)
 /*
  * Parse a LDAP value
  * notes:
- * the argument u should be a NULL terminated sequence of ASCII bytes.
+ * the argument p should be a NULL terminated sequence of ASCII bytes.
  */
 char *
 parseval(char *p, size_t len)

minor diff for ldapd.conf.5

[Rob Pierce]

Fix a couple of grammar mistakes, remove a redundant word, and add a FILES
reference for the /etc/ldap/certs directory.

Rob

Index: ldapd.conf.5
===
RCS file: /cvs/src/usr.sbin/ldapd/ldapd.conf.5,v
retrieving revision 1.19
diff -u -p -r1.19 ldapd.conf.5
--- ldapd.conf.511 Jun 2014 18:00:40 -  1.19
+++ ldapd.conf.517 Oct 2016 00:43:43 -
@@ -108,7 +108,7 @@ the interface name with a .crt extension
 .Pa /etc/ldap/certs/fxp0.crt .
 .Pp
 If the certificate name is an absolute path, a .crt and .key extension
-is appended to form the certificate path and key path respectively.
+are appended to form the certificate path and key path respectively.
 .Pp
 Only secured connections accept plain text password authentication.
 Connections using TLS or unix domain sockets are always considered secured.
@@ -125,7 +125,7 @@ This option can be given multiple times,
 considered equal.
 Clients may choose to follow any of the referral URLs.
 .Pp
-The URL format has the following format:
+The URL has the following format:
 .Bd -literal -offset indent
 ldap://ldap.example.com
 ldaps://ldap.example.com:3890
@@ -148,7 +148,7 @@ below.
 .Sh NAMESPACES
 A namespace is a subtree of the global X.500 DIT (Directory Information Tree),
 also known as a naming context.
-All entries' distinguished names (DN) has the same suffix, which is used to
+All entries' distinguished names (DN) have the same suffix, which is used to
 identify the namespace.
 The suffix should consist of the domain components, in reverse order, of your
 domain name, as recommended by RFC 2247.
@@ -322,6 +322,8 @@ This would define MyOidAttributes as a s
 .El
 .Sh FILES
 .Bl -tag -width "/etc/ldap/ldapd.confXXX" -compact
+.It Pa /etc/ldap/certs/
+The directory where LDAP certificates are kept.
 .It Pa /etc/ldapd.conf
 Default
 .Xr ldapd 8

remove kern.arand from sysctl.8 and rnd.c (comment)

[Rob Pierce]

Index: sysctl.8
===
RCS file: /cvs/src/sbin/sysctl/sysctl.8,v
retrieving revision 1.205
diff -u -p -r1.205 sysctl.8
--- sysctl.87 Sep 2016 17:30:12 -   1.205
+++ sysctl.823 Sep 2016 02:16:14 -
@@ -144,7 +144,6 @@ and a few require a kernel compiled with
 .It kern.sysvmsg Ta integer Ta no
 .It kern.sysvsem Ta integer Ta no
 .It kern.sysvshm Ta integer Ta no
-.It kern.arandom Ta u_int Ta no
 .It kern.msgbufsize Ta integer Ta no
 .It kern.malloc.buckets Ta string Ta no
 .It kern.malloc.bucket. Ta string Ta no

Index: rnd.c
===
RCS file: /cvs/src/sys/dev/rnd.c,v
retrieving revision 1.186
diff -u -p -r1.186 rnd.c
--- rnd.c   22 Sep 2016 22:04:02 -  1.186
+++ rnd.c   23 Sep 2016 02:25:06 -
@@ -85,9 +85,8 @@
  * The output of this single ChaCha20 engine is then shared amongst many
  * consumers in the kernel and userland via a few interfaces:
  * arc4random_buf(), arc4random(), arc4random_uniform(), randomread()
- * for the set of /dev/random nodes, the sysctl kern.arandom, and the
- * system call getentropy(), which provides seeds for process-context
- * pseudorandom generators.
+ * for the set of /dev/random nodes, and the system call getentropy(),
+ * which provides seeds for process-context pseudorandom generators.
  *
  * Acknowledgements:
  * =

Re: fix regress in pstat.c

[Rob Pierce]

On Sat, Sep 17, 2016 at 09:20:31PM +0200, Theo Buehler wrote:
> While this patch avoids the bug, it isn't quite right: pstat -ft should
> be equivalent to "pstat -f && pstat -t" but...
> 
> $ pstat -ft
> pstat: kvm_openfiles: /dev/mem: Permission denied
> 
> The actual problem is in ttymode(): if kd != NULL (which is the case if
> -f is combined with -t), there are accesses of the kvm datastructures
> without them being properly initialized:
> 
> 879   if (kd != NULL)
> 880   KGET(TTY_NTTY, ntty);
> 
> and
> 
> 896   KGET(TTY_TTYLIST, tty_head);
> 897   for (tp = TAILQ_FIRST(_head); tp;
> 898   tp = TAILQ_NEXT(, tty_link)) {
> 899   KGET2(tp, , sizeof tty, "tty struct");
> 900   tty2itty(, );
> 901   ttyprt();
> 902   }
> 
> These code paths must not be hit unless need_nlist is true, so expose it
> globally and replace 'kd != NULL' with the proper condition need_nlist
> 
> The following patch works in all affected cases, pstat -t, pstat -f,
> pstat -tf, both as root and non-root as well as pstat -tv as root
> (combinations with -d aren't interesting since pstat exits before
> doing anything else but -d).
> 
> Index: pstat.c
> ===
> RCS file: /var/cvs/src/usr.sbin/pstat/pstat.c,v
> retrieving revision 1.108
> diff -u -p -r1.108 pstat.c
> --- pstat.c   14 Aug 2016 22:47:26 -  1.108
> +++ pstat.c   17 Sep 2016 17:57:38 -
> @@ -89,6 +89,7 @@ struct e_vnode {
>   struct vnode vnode;
>  };
>  
> +int  need_nlist;
>  int  usenumflag;
>  int  totalflag;
>  int  kflag;
> @@ -141,7 +142,7 @@ main(int argc, char *argv[])
>   const char *dformat = NULL;
>   extern char *optarg;
>   extern int optind;
> - int i, need_nlist;
> + int i;
>  
>   hideroot = getuid();
>  
> @@ -869,7 +870,7 @@ ttymode(void)
>   struct itty itty, *itp;
>   size_t nlen;
>  
> - if (kd == 0) {
> + if (!need_nlist) {
>   mib[0] = CTL_KERN;
>   mib[1] = KERN_TTYCOUNT;
>   nlen = sizeof(ntty);
> @@ -879,7 +880,7 @@ ttymode(void)
>   KGET(TTY_NTTY, ntty);
>   (void)printf("%d terminal device%s\n", ntty, ntty == 1 ? "" : "s");
>   (void)printf("%s", hdr);
> - if (kd == 0) {
> + if (!need_nlist) {
>   mib[0] = CTL_KERN;
>   mib[1] = KERN_TTY;
>   mib[2] = KERN_TTY_INFO;
> 

Thanks Theo.

Fixes the regress and works well with your pending pledge diff.

Rob

fix regress in pstat.c

[Rob Pierce]

ttymode() needs nlist, otherwise "pstat -tf" will fail since kd will not be
NULL and the calls from ttymode() to KGET will error as follows:

pstat: cannot read ntty: invalid address (0)
pstat: cannot read tty_head: invalid address (0)

KGET(TTY_NTTY, ntty) and KGET(TTY_TTYLIST, tty_head) both result in a
kvm_read() with an address (globalnl[TTY_NTTY].n_value) of zero.

The following diff resolves this issue.

Took the opportunity to reorder the flags to match the switch statement.

With help from tb@.

Rob

Index: pstat.c
===
RCS file: /cvs/src/usr.sbin/pstat/pstat.c,v
retrieving revision 1.108
diff -u -p -r1.108 pstat.c
--- pstat.c 14 Aug 2016 22:47:26 -  1.108
+++ pstat.c 17 Sep 2016 17:00:28 -
@@ -190,7 +190,7 @@ main(int argc, char *argv[])
if ((dformat == NULL && argc > 0) || (dformat && argc == 0))
usage();
 
-   need_nlist = vnodeflag || dformat;
+   need_nlist = dformat || (fileflag && ttyflag) || vnodeflag;
 
if (nlistf != NULL || memf != NULL) {
if (fileflag || totalflag)

minor diff for vmctl.8

[Rob Pierce]

"host" is not a command argument.

Rob

Index: vmctl.8
===
RCS file: /cvs/src/usr.sbin/vmctl/vmctl.8,v
retrieving revision 1.13
diff -u -p -r1.13 vmctl.8
--- vmctl.8 18 Aug 2016 16:12:05 -  1.13
+++ vmctl.8 12 Sep 2016 01:36:59 -
@@ -28,8 +28,7 @@
 The
 .Nm
 utility is used to control the virtual machine monitor (VMM) subsystem.
-A VMM manages virtual machines (VMs) on a
-.Ar host .
+A VMM manages virtual machines (VMs) on a host.
 The VMM subsystem is responsible for creating, destroying, and executing
 VMs.
 .Pp

whitespace in /etc/rc

[Rob Pierce]

Index: rc
===
RCS file: /cvs/src/etc/rc,v
retrieving revision 1.486
diff -u -p -r1.486 rc
--- rc  10 Jul 2016 09:08:18 -  1.486
+++ rc  5 Sep 2016 15:45:09 -
@@ -490,7 +490,6 @@ echo clearing /tmp
 # rc.securelevel did not specifically set -1 or 2, so select the default: 1.
 (($(sysctl -n kern.securelevel) == 0)) && sysctl kern.securelevel=1
 
-
 # Patch /etc/motd.
 if [[ ! -f /etc/motd ]]; then
install -c -o root -g wheel -m 664 /dev/null /etc/motd

mention relayd in faq6.html

[Rob Pierce]

Not sure if this is the right place or the right wording, but I think it
deserves a mention somewhere in the faq.

Regards,

Rob

Index: faq6.html
===
RCS file: /cvs/www/faq/faq6.html,v
retrieving revision 1.383
diff -u -p -r1.383 faq6.html
--- faq6.html   15 Aug 2016 02:22:13 -  1.383
+++ faq6.html   4 Sep 2016 01:05:49 -
@@ -602,6 +602,11 @@ providing bandwidth control and packet p
 to create powerful and flexible firewalls.
 It is described in the PF User's Guide.
 
+
+See also OpenBSD's http://man.openbsd.org/relayd;>relayd(8)
+for implementing load-balancers, application layer gateways, and
+transparent proxies.
+
 Dynamic Host Configuration Protocol
 
 Dynamic Host Configuration Protocol is a way to configure network

minor diff for faq15.html

[Rob Pierce]

There is only one result mentioned: ready-to-install binary packages.

Rob

Index: faq15.html
===
RCS file: /cvs/www/faq/faq15.html,v
retrieving revision 1.141
diff -u -p -r1.141 faq15.html
--- faq15.html  1 Sep 2016 12:05:14 -   1.141
+++ faq15.html  3 Sep 2016 18:23:13 -
@@ -110,7 +110,7 @@ to OpenBSD's shared library specificatio
 secure whenever possible, etc.
 
 
-The end result of the porting effort are ready-to-install binary
+The end result of the porting effort is ready-to-install binary
 packages.
 The aim of the package system is to keep track of which software gets
 installed, so that it may at any time be updated or removed very easily.

60.html on ntpd and pledge

[Rob Pierce]

ntpd was pledged in 5.9.

Rob

Index: 60.html
===
RCS file: /cvs/www/60.html,v
retrieving revision 1.70
diff -u -p -r1.70 60.html
--- 60.html 24 Aug 2016 20:47:30 -  1.70
+++ 60.html 25 Aug 2016 07:48:55 -
@@ -598,7 +598,7 @@ to 6.0.
 Moved the execution of constraints from the ntp process to the
 parent process, allowing for better privilege separation since the
 ntp process can be further restricted.
-Added
+Improved
 http://man.openbsd.org/pledge.2;>pledge(2)
 support.
 Fixed high CPU usage when the network is down.

Re: syslogd: accept space-deliminated fields

[Rob Pierce]

- Original Message -
> From: "Todd C. Miller" 
> To: "tech" 
> Sent: Friday, July 1, 2016 12:55:11 PM
> Subject: syslogd: accept space-deliminated fields

> Linux, Net and Free also support space-deliminated fields. Maybe
> we should too...

> - todd

> Index: usr.sbin/syslogd/syslog.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/syslogd/syslog.conf.5,v
> retrieving revision 1.33
> diff -u -p -u -r1.33 syslog.conf.5
> --- usr.sbin/syslogd/syslog.conf.5 10 Sep 2015 15:16:44 - 1.33
> +++ usr.sbin/syslogd/syslog.conf.5 1 Jul 2016 16:50:37 -
> @@ -55,7 +55,7 @@ The
> .Em selector
> field is separated from the
> .Em action
> -field by one or more tab characters.
> +field by one or more tab or space characters.
> .Pp
> The
> .Em selectors
> @@ -334,6 +334,10 @@ file appeared in
> .Bx 4.3 ,
> along with
> .Xr syslogd 8 .
> +.Pp
> +Historic versions of
> +.Xr syslogd 8
> +did not support space-delimited fields.
> .Sh BUGS
> The effects of multiple selectors are sometimes not intuitive.
> For example
> Index: usr.sbin/syslogd/syslogd.c
> ===
> RCS file: /cvs/src/usr.sbin/syslogd/syslogd.c,v
> retrieving revision 1.207
> diff -u -p -u -r1.207 syslogd.c
> --- usr.sbin/syslogd/syslogd.c 1 Jul 2016 15:47:15 - 1.207
> +++ usr.sbin/syslogd/syslogd.c 1 Jul 2016 16:50:37 -
> @@ -2454,19 +2454,19 @@ cfline(char *line, char *progblock, char
> f->f_hostname = strdup(hostblock);

> /* scan through the list of selectors */
> - for (p = line; *p && *p != '\t';) {
> + for (p = line; *p && *p != '\t' && *p != ' ';) {

> /* find the end of this facility name list */
> - for (q = p; *q && *q != '\t' && *q++ != '.'; )
> + for (q = p; *q && *q != '\t' && *q != ' ' && *q++ != '.'; )
> continue;

> /* collect priority name */
> - for (bp = buf; *q && !strchr("\t,;", *q); )
> + for (bp = buf; *q && !strchr("\t,; ", *q); )
> *bp++ = *q++;
> *bp = '\0';

> /* skip cruft */
> - while (*q && strchr(", ;", *q))
> + while (*q && strchr(",;", *q))
> q++;

> /* decode priority name */
> @@ -2489,8 +2489,8 @@ cfline(char *line, char *progblock, char
> }

> /* scan facilities */
> - while (*p && !strchr("\t.;", *p)) {
> - for (bp = buf; *p && !strchr("\t,;.", *p); )
> + while (*p && !strchr("\t.; ", *p)) {
> + for (bp = buf; *p && !strchr("\t,;. ", *p); )
> *bp++ = *p++;
> *bp = '\0';
> if (*buf == '*')
> @@ -2516,7 +2516,7 @@ cfline(char *line, char *progblock, char
> }

> /* skip to action part */
> - while (*p == '\t')
> + while (*p == '\t' || *p == ' ')
> p++;

> switch (*p) {

This passed some basic testing on my end. Thanks!

Rob

intro.2 mention of pledge(2)

[Rob Pierce]

Does this make sense?

Rob

Index: intro.2
===
RCS file: /cvs/src/lib/libc/sys/intro.2,v
retrieving revision 1.63
diff -u -p -r1.63 intro.2
--- intro.2 6 Mar 2016 22:32:09 -   1.63
+++ intro.2 15 Apr 2016 16:26:24 -
@@ -42,6 +42,9 @@
 .Sh DESCRIPTION
 The manual pages in section 2 provide an overview of the system calls,
 their error returns, and other common definitions and concepts.
+.Pp
+Programs may be restricted to a subset of system calls with
+.Xr pledge 2 .
 .\".Pp
 .\".Sy System call restart
 .\".Pp
@@ -725,6 +728,7 @@ Each socket has an address chosen from t
 socket was created.
 .El
 .Sh SEE ALSO
+.Xr pledge 2 ,
 .Xr intro 3 ,
 .Xr perror 3
 .Sh HISTORY

Re: pledge pstat

[Rob Pierce]

On Tue, Apr 12, 2016 at 04:25:01PM -0400, Rob Pierce wrote:
> Hoist sysct and kvm calls, and pledge stdio, rpath, vminfo.
> 
> Rob

Update diff based on comments from tb@:

Changes include the following:

 - kd is a pointer, so compare with NULL (not 0).
 - simply nlistf/memf condition statment in filemode() now that kd is global

Here is the revised diff:

Index: pstat.c
===
RCS file: /cvs/src/usr.sbin/pstat/pstat.c,v
retrieving revision 1.102
diff -u -p -r1.102 pstat.c
--- pstat.c 12 Apr 2016 16:53:42 -  1.102
+++ pstat.c 13 Apr 2016 15:06:48 -
@@ -82,6 +82,8 @@ struct nlist vnodenl[] = {
{ NULL }
 };
 
+struct itty *globalitp;
+struct kinfo_file *kf;
 struct nlist *globalnl;
 
 struct e_vnode {
@@ -89,6 +91,10 @@ struct e_vnode {
struct vnode vnode;
 };
 
+intnumvnodes;
+intmaxfile;
+intnfile;
+intntty;
 intusenumflag;
 inttotalflag;
 intkflag;
@@ -111,15 +117,17 @@ kvm_t *kd = NULL;
}
 
 void   filemode(void);
+void   filemodeprep(void);
 struct mount *
getmnt(struct mount *);
 struct e_vnode *
-   kinfo_vnodes(int *);
+   kinfo_vnodes(void);
 void   mount_print(struct mount *);
 void   nfs_header(void);
 intnfs_print(struct vnode *);
 void   swapmode(void);
 void   ttymode(void);
+void   ttymodeprep(void);
 void   ttyprt(struct itty *);
 void   tty2itty(struct tty *tp, struct itty *itp);
 void   ufs_header(void);
@@ -130,6 +138,7 @@ voidusage(void);
 void   vnode_header(void);
 void   vnode_print(struct vnode *, struct vnode *);
 void   vnodemode(void);
+void   vnodemodeprep(void);
 
 inthideroot;
 
@@ -202,6 +211,25 @@ main(int argc, char *argv[])
O_RDONLY | (need_nlist ? 0 : KVM_NO_FILES), buf)) == 0)
errx(1, "kvm_openfiles: %s", buf);
 
+   if (need_nlist)
+   if (kvm_nlist(kd, vnodenl) == -1)
+   errx(1, "kvm_nlist: %s", kvm_geterr(kd));
+
+   if (!(fileflag | vnodeflag | ttyflag | swapflag | totalflag || dformat))
+   usage();
+
+   if(!dformat) {
+   if (fileflag || totalflag)
+   filemodeprep();
+   if (vnodeflag || totalflag)
+   vnodemodeprep();
+   if (ttyflag)
+   ttymodeprep();
+   }
+
+   if (pledge("stdio rpath vminfo", NULL) == -1)
+   err(1, "pledge");
+
if (dformat) {
struct nlist *nl;
int longformat = 0, stringformat = 0, error = 0, n;
@@ -314,12 +342,6 @@ main(int argc, char *argv[])
exit(error);
}
 
-   if (need_nlist)
-   if (kvm_nlist(kd, vnodenl) == -1)
-   errx(1, "kvm_nlist: %s", kvm_geterr(kd));
-
-   if (!(fileflag | vnodeflag | ttyflag | swapflag | totalflag || dformat))
-   usage();
if (fileflag || totalflag)
filemode();
if (vnodeflag || totalflag)
@@ -337,11 +359,10 @@ vnodemode(void)
struct e_vnode *e_vnodebase, *endvnode, *evp;
struct vnode *vp;
struct mount *maddr, *mp = NULL;
-   int numvnodes;
 
globalnl = vnodenl;
 
-   e_vnodebase = kinfo_vnodes();
+   e_vnodebase = kinfo_vnodes();
if (totalflag) {
(void)printf("%7d vnodes\n", numvnodes);
return;
@@ -398,6 +419,21 @@ vnodemode(void)
 }
 
 void
+vnodemodeprep(void)
+{
+   int mib[2];
+   size_t num;
+
+   if (kd == NULL) {
+   mib[0] = CTL_KERN;
+   mib[1] = KERN_NUMVNODES;
+   num = sizeof(numvnodes);
+   if (sysctl(mib, 2, , , NULL, 0) < 0)
+   err(1, "sysctl(KERN_NUMVNODES) failed");
+   }
+}
+
+void
 vnode_header(void)
 {
(void)printf("%*s TYP VFLAG  USE HOLD", 2 * (int)sizeof(long), "ADDR");
@@ -790,24 +826,17 @@ mount_print(struct mount *mp)
  * simulate what a running kernel does in kinfo_vnode
  */
 struct e_vnode *
-kinfo_vnodes(int *avnodes)
+kinfo_vnodes(void)
 {
struct mntlist kvm_mountlist;
struct mount *mp, mount;
struct vnode *vp, vnode;
char *vbuf, *evbuf, *bp;
-   int mib[2], numvnodes;
+   int mib[2];
size_t num;
 
-   if (kd == 0) {
-   mib[0] = CTL_KERN;
-   mib[1] = KERN_NUMVNODES;
-   num = sizeof(numvnodes);
-   if (sysctl(mib, 2, , , NULL, 0) < 0)
-   err(1, "sysctl(KERN_NUMVNODES) failed");
-   } else
+   if (kd != NULL)
KGET(V_NUMV, numvnodes);
-   *avnodes = numvnodes;
if (totalflag)
return NULL;
if ((vbuf = calloc(numvnodes + 20,
@@ -835,7 +864,7 @@ kinfo_vnodes(int *avnodes)

Re: pledge pstat

[Rob Pierce]

From: "Mike Belopuhov" <m...@belopuhov.com> 
To: "Rob Pierce" <r...@2keys.ca> 
Cc: "tech" <tech@openbsd.org> 
Sent: Tuesday, April 12, 2016 6:05:19 PM 
Subject: Re: pledge pstat 




BQ_BEGIN
On 12 April 2016 at 22:25, Rob Pierce <r...@2keys.ca> wrote: 
> Hoist sysct and kvm calls, and pledge stdio, rpath, vminfo. 
> 
> Rob 
> 

Please make sure that "pstat -d" still works. It's about the most useful 
thing it can do. 
BQ_END

Yes, of course. I have received some feedback and will be submitting an 
updated diff shortly. 

pledge pstat

[Rob Pierce]

Hoist sysct and kvm calls, and pledge stdio, rpath, vminfo.

Rob

Index: pstat.c
===
RCS file: /cvs/src/usr.sbin/pstat/pstat.c,v
retrieving revision 1.102
diff -u -p -r1.102 pstat.c
--- pstat.c 12 Apr 2016 16:53:42 -  1.102
+++ pstat.c 12 Apr 2016 20:20:39 -
@@ -82,6 +82,8 @@ struct nlist vnodenl[] = {
{ NULL }
 };
 
+struct itty *globalitp;
+struct kinfo_file *kf;
 struct nlist *globalnl;
 
 struct e_vnode {
@@ -89,6 +91,10 @@ struct e_vnode {
struct vnode vnode;
 };
 
+intnumvnodes;
+intmaxfile;
+intnfile;
+intntty;
 intusenumflag;
 inttotalflag;
 intkflag;
@@ -111,15 +117,17 @@ kvm_t *kd = NULL;
}
 
 void   filemode(void);
+void   filemodeprep(void);
 struct mount *
getmnt(struct mount *);
 struct e_vnode *
-   kinfo_vnodes(int *);
+   kinfo_vnodes(void);
 void   mount_print(struct mount *);
 void   nfs_header(void);
 intnfs_print(struct vnode *);
 void   swapmode(void);
 void   ttymode(void);
+void   ttymodeprep(void);
 void   ttyprt(struct itty *);
 void   tty2itty(struct tty *tp, struct itty *itp);
 void   ufs_header(void);
@@ -130,6 +138,7 @@ voidusage(void);
 void   vnode_header(void);
 void   vnode_print(struct vnode *, struct vnode *);
 void   vnodemode(void);
+void   vnodemodeprep(void);
 
 inthideroot;
 
@@ -202,6 +211,25 @@ main(int argc, char *argv[])
O_RDONLY | (need_nlist ? 0 : KVM_NO_FILES), buf)) == 0)
errx(1, "kvm_openfiles: %s", buf);
 
+   if (need_nlist)
+   if (kvm_nlist(kd, vnodenl) == -1)
+   errx(1, "kvm_nlist: %s", kvm_geterr(kd));
+
+   if (!(fileflag | vnodeflag | ttyflag | swapflag | totalflag || dformat))
+   usage();
+
+   if(!dformat) {
+   if (fileflag || totalflag)
+   filemodeprep();
+   if (vnodeflag || totalflag)
+   vnodemodeprep();
+   if (ttyflag)
+   ttymodeprep();
+   }
+
+   if (pledge("stdio rpath vminfo", NULL) == -1)
+   err(1, "pledge");
+
if (dformat) {
struct nlist *nl;
int longformat = 0, stringformat = 0, error = 0, n;
@@ -314,12 +342,6 @@ main(int argc, char *argv[])
exit(error);
}
 
-   if (need_nlist)
-   if (kvm_nlist(kd, vnodenl) == -1)
-   errx(1, "kvm_nlist: %s", kvm_geterr(kd));
-
-   if (!(fileflag | vnodeflag | ttyflag | swapflag | totalflag || dformat))
-   usage();
if (fileflag || totalflag)
filemode();
if (vnodeflag || totalflag)
@@ -337,11 +359,10 @@ vnodemode(void)
struct e_vnode *e_vnodebase, *endvnode, *evp;
struct vnode *vp;
struct mount *maddr, *mp = NULL;
-   int numvnodes;
 
globalnl = vnodenl;
 
-   e_vnodebase = kinfo_vnodes();
+   e_vnodebase = kinfo_vnodes();
if (totalflag) {
(void)printf("%7d vnodes\n", numvnodes);
return;
@@ -398,6 +419,21 @@ vnodemode(void)
 }
 
 void
+vnodemodeprep(void)
+{
+   int mib[2];
+   size_t num;
+
+   if (kd == 0) {
+   mib[0] = CTL_KERN;
+   mib[1] = KERN_NUMVNODES;
+   num = sizeof(numvnodes);
+   if (sysctl(mib, 2, , , NULL, 0) < 0)
+   err(1, "sysctl(KERN_NUMVNODES) failed");
+   }
+}
+
+void
 vnode_header(void)
 {
(void)printf("%*s TYP VFLAG  USE HOLD", 2 * (int)sizeof(long), "ADDR");
@@ -790,24 +826,17 @@ mount_print(struct mount *mp)
  * simulate what a running kernel does in kinfo_vnode
  */
 struct e_vnode *
-kinfo_vnodes(int *avnodes)
+kinfo_vnodes(void)
 {
struct mntlist kvm_mountlist;
struct mount *mp, mount;
struct vnode *vp, vnode;
char *vbuf, *evbuf, *bp;
-   int mib[2], numvnodes;
+   int mib[2];
size_t num;
 
-   if (kd == 0) {
-   mib[0] = CTL_KERN;
-   mib[1] = KERN_NUMVNODES;
-   num = sizeof(numvnodes);
-   if (sysctl(mib, 2, , , NULL, 0) < 0)
-   err(1, "sysctl(KERN_NUMVNODES) failed");
-   } else
+   if (kd != 0)
KGET(V_NUMV, numvnodes);
-   *avnodes = numvnodes;
if (totalflag)
return NULL;
if ((vbuf = calloc(numvnodes + 20,
@@ -835,7 +864,7 @@ kinfo_vnodes(int *avnodes)
num++;
}
}
-   *avnodes = num;
+   numvnodes = num;
return ((struct e_vnode *)vbuf);
 }
 
@@ -864,32 +893,18 @@ ttymode(void)
 {
struct ttylist_head tty_head;
struct tty *tp, tty;
-   int mib[3], ntty, i;
-   struct itty itty, *itp;
+   int mib[3], i;
+   struct itty itty;
size_t nlen;
 
-   if (kd == 0) {
-   mib[0] 

pstat diff fixed segmentation fault

[Rob Pierce]

Running pstat with either [-M core] or [-N system] along with the -T option
results in a segmentation fault. Make sure kvm access is initialized prior
to running kvm_read() when -T is specified.

This issue appears to have been introduced when setgid kmem support was
removed in revision 1.99.

Rob

Index: pstat.c
===
RCS file: /cvs/src/usr.sbin/pstat/pstat.c,v
retrieving revision 1.101
diff -u -p -r1.101 pstat.c
--- pstat.c 11 Dec 2015 11:53:52 -  1.101
+++ pstat.c 11 Apr 2016 18:22:35 -
@@ -193,7 +193,7 @@ main(int argc, char *argv[])
need_nlist = vnodeflag || dformat;
 
if (nlistf != NULL || memf != NULL) {
-   if (fileflag)
+   if (fileflag || totalflag)
need_nlist = 1;
}
 

Re: rcctl ls faulty -> failed

[Rob Pierce]

> From: "Antoine Jacoutot" 
> To: "Ian Darwin" 
> Cc: "tech" 
> Sent: Tuesday, March 29, 2016 10:59:54 AM
> Subject: Re: rcctl ls faulty -> failed

> On Tue, Mar 29, 2016 at 10:48:17AM -0400, Ian Darwin wrote:
> > On Tue, Mar 29, 2016 at 03:29:27PM +0200, Antoine Jacoutot wrote:
> > > Hi.

> > > We'd like to rename the 'faulty' listing to 'failed'.
> > > i.e. rcctl ls failed

> > > Index: etc/daily
> > > ===
> > > RCS file: /cvs/src/etc/daily,v
> > > retrieving revision 1.85
> > > diff -u -p -u -p -r1.85 daily
> > > --- etc/daily 28 Jan 2016 15:45:34 - 1.85
> > > +++ etc/daily 29 Mar 2016 13:25:59 -
> > > @@ -127,7 +127,7 @@ while [ "X$ROOTBACKUP" = X1 ]; do
> > > done

> > > next_part "Services that should run but don't:"

> > While you're there, can you please change "should run but don't" to
> > "should be running but aren't" ? The current wording is awkward,
> > and also implies that they don't run (ie. they fail to start)
> > when in fact they may have been running but been shut down
> > manually, or failed. Language should be precise as well as concise.

> Sure.

> --
> Antoine
Contractions aren't necessary. 

http://courses.cs.vt.edu/cs3604/support/Writing/writing.caveats.html 

prefer memset() over bzero() for ber.c in snmpd and ypldap

[Rob Pierce]

A similar change was done to ber.c in ldapd by @mmcc.

I just copied those changes for snmpd and ypldap.

No binary change.

Regards,

Index: snmpd/ber.c
===
RCS file: /cvs/src/usr.sbin/snmpd/ber.c,v
retrieving revision 1.30
diff -u -p -r1.30 ber.c
--- snmpd/ber.c 5 Dec 2015 06:42:18 -   1.30
+++ snmpd/ber.c 4 Mar 2016 21:34:32 -
@@ -420,7 +420,7 @@ ber_string2oid(const char *oidstr, struc
 
if (strlcpy(str, oidstr, sizeof(str)) >= sizeof(str))
return (-1);
-   bzero(o, sizeof(*o));
+   memset(o, 0, sizeof(*o));
 
/* Parse OID strings in the common forms n.n.n, n_n_n_n, or n-n-n */
for (p = sp = str; p != NULL; sp = p) {
@@ -505,7 +505,7 @@ ber_get_oid(struct ber_element *elm, str
if (!buf[i])
return (-1);
 
-   bzero(o, sizeof(*o));
+   memset(o, 0, sizeof(*o));
o->bo_id[j++] = buf[i] / 40;
o->bo_id[j++] = buf[i++] % 40;
for (; i < len && j < BER_MAX_OID_LEN; i++) {
@@ -639,7 +639,7 @@ ber_scanf_elements(struct ber_element *b
struct ber_oid  *o;
struct ber_element  *parent[_MAX_SEQ], **e;
 
-   bzero(parent, sizeof(struct ber_element *) * _MAX_SEQ);
+   memset(parent, 0, sizeof(struct ber_element *) * _MAX_SEQ);
 
va_start(ap, fmt);
while (*fmt) {

Index: ypldap/ber.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ber.c,v
retrieving revision 1.10
diff -u -p -r1.10 ber.c
--- ypldap/ber.c5 Dec 2015 19:10:19 -   1.10
+++ ypldap/ber.c4 Mar 2016 21:34:33 -
@@ -420,7 +420,7 @@ ber_string2oid(const char *oidstr, struc
 
if (strlcpy(str, oidstr, sizeof(str)) >= sizeof(str))
return (-1);
-   bzero(o, sizeof(*o));
+   memset(o, 0, sizeof(*o));
 
/* Parse OID strings in the common forms n.n.n, n_n_n_n, or n-n-n */
for (p = sp = str; p != NULL; sp = p) {
@@ -505,7 +505,7 @@ ber_get_oid(struct ber_element *elm, str
if (!buf[i])
return (-1);
 
-   bzero(o, sizeof(*o));
+   memset(o, 0, sizeof(*o));
o->bo_id[j++] = buf[i] / 40;
o->bo_id[j++] = buf[i++] % 40;
for (; i < len && j < BER_MAX_OID_LEN; i++) {
@@ -638,7 +638,7 @@ ber_scanf_elements(struct ber_element *b
struct ber_oid  *o;
struct ber_element  *parent[_MAX_SEQ], **e;
 
-   bzero(parent, sizeof(struct ber_element *) * _MAX_SEQ);
+   memset(parent, 0, sizeof(struct ber_element *) * _MAX_SEQ);
 
va_start(ap, fmt);
while (*fmt) {

afterboot - ntpd now on by default

[Rob Pierce]

Ping.

Index: afterboot.8
===
RCS file: /cvs/src/share/man/man8/afterboot.8,v
retrieving revision 1.149
diff -u -p -r1.149 afterboot.8
--- afterboot.8 24 Sep 2015 15:07:55 -  1.149
+++ afterboot.8 26 Sep 2015 16:22:07 -
@@ -124,6 +124,14 @@ Furthermore, the superuser's
 should never contain the current directory
 .Pq Dq \&. .
 .Ss System date
+.Xr ntpd 8
+is used to automatically synchronize clocks with remote NTP servers.
+You can use
+.Xr ntpctl 8
+to check the status.
+To change the NTP server see
+.Xr ntpd.conf 5 .
+.Pp
 Check the system date with the
 .Xr date 1
 command.
@@ -132,14 +140,11 @@ If needed, change the date, and/or chang
 to the correct time zone in the
 .Pa /usr/share/zoneinfo
 directory.
-Alternatively,
-.Xr ntpd 8
-can be used to automatically synchronize clocks with remote NTP servers.
 .Pp
 Examples:
 .Pp
-Set the current date to January 27th, 1999 3:04pm:
-.Dl # date 199901271504
+Set the current date to January 27th, 2016 3:04pm:
+.Dl # date 201601271504
 .Pp
 Set the time zone to Atlantic Standard Time:
 .Dl # ln -fs /usr/share/zoneinfo/Canada/Atlantic /etc/localtime

missing period in pledge.2

[Rob Pierce]

Index: pledge.2
===
RCS file: /cvs/src/lib/libc/sys/pledge.2,v
retrieving revision 1.15
diff -u -p -r1.15 pledge.2
--- pledge.216 Nov 2015 19:26:21 -  1.15
+++ pledge.217 Nov 2015 21:14:08 -
@@ -472,7 +472,7 @@ process:
 .Xr setlogin 2 ,
 .Xr setrlimit 2 ,
 .Xr getpriority 2 ,
-.Xr setpriority 2
+.Xr setpriority 2 .
 .El
 .Pp
 A whitelist of permitted paths may be provided in