Re: mg: fgetln -> getline

[Scott Cheloha]

> On Oct 11, 2017, at 2:36 AM, Florian Obser  wrote:
> 
> On Sun, Sep 17, 2017 at 02:56:32AM +, Scott Cheloha wrote:
>> 
>> 
>>  if (buf[len - 1] == '\n')
>>  buf[len - 1] = '\0';
> 
> the diff reads fine to me, but code tends to get copied around,
> can you send a diff with those lines added please? That's the
> canonical way of using getline I believe and there is no good
> reason for mg to be different, thanks!

here you go

--
Scott Cheloha

Index: usr.bin/mg/cscope.c
===
RCS file: /cvs/src/usr.bin/mg/cscope.c,v
retrieving revision 1.16
diff -u -p -r1.16 cscope.c
--- usr.bin/mg/cscope.c 19 Jan 2016 14:51:00 -  1.16
+++ usr.bin/mg/cscope.c 12 Oct 2017 04:11:36 -
@@ -166,9 +166,13 @@ cscreatelist(int f, int n)
struct stat sb;
FILE *fpipe;
char dir[NFILEN], cmd[BUFSIZ], title[BUFSIZ], *line, *bufp;
-   size_t len;
+   size_t sz;
+   ssize_t len;
int clen;
 
+   line = NULL;
+   sz = 0;
+
if (getbufcwd(dir, sizeof(dir)) == FALSE)
dir[0] = '\0';
 
@@ -221,11 +225,14 @@ cscreatelist(int f, int n)
}
addline(bp, title);
addline(bp, "");
-   /* All lines are NUL terminated */
-   while ((line = fgetln(fpipe, )) != NULL) {
-   line[len - 1] = '\0';
+   while ((len = getline(, , fpipe)) != -1) {
+   if (line[len - 1] == '\n')
+   line[len - 1] = '\0';
addline(bp, line);
}
+   free(line);
+   if (ferror(fpipe))
+   ewprintf("Problem reading pipe");
pclose(fpipe);
return (popbuftop(bp, WNONE));
 }
@@ -397,7 +404,11 @@ do_cscope(int i)
char pattern[MAX_TOKEN], cmd[BUFSIZ], title[BUFSIZ];
char *p, *buf;
int clen, nores = 0;
-   size_t len;
+   size_t sz;
+   ssize_t len;
+
+   buf = NULL;
+   sz = 0;
 
/* If current buffer isn't a source file just return */
if (fnmatch("*.[chy]", curbp->b_fname, 0) != 0) {
@@ -447,13 +458,18 @@ do_cscope(int i)
addline(bp, title);
addline(bp, "");
addline(bp, 
"---");
-   /* All lines are NUL terminated */
-   while ((buf = fgetln(fpipe, )) != NULL) {
-   buf[len - 1] = '\0';
-   if (addentry(bp, buf) != TRUE)
+   while ((len = getline(, , fpipe)) != -1) {
+   if (buf[len - 1] == '\n')
+   buf[len - 1] = '\0';
+   if (addentry(bp, buf) != TRUE) {
+   free(buf);
return (FALSE);
+   }
nores = 1;
-   };
+   }
+   free(buf);
+   if (ferror(fpipe))
+   ewprintf("Problem reading pipe");
pclose(fpipe);
addline(bp, 
"---");
if (nores == 0)
Index: usr.bin/mg/grep.c
===
RCS file: /cvs/src/usr.bin/mg/grep.c,v
retrieving revision 1.44
diff -u -p -r1.44 grep.c
--- usr.bin/mg/grep.c   19 Mar 2015 21:48:05 -  1.44
+++ usr.bin/mg/grep.c   12 Oct 2017 04:11:36 -
@@ -178,12 +178,16 @@ compile_mode(const char *name, const cha
struct buffer   *bp;
FILE*fpipe;
char*buf;
-   size_t   len;
+   size_t   sz;
+   ssize_t  len;
int  ret, n;
char cwd[NFILEN], qcmd[NFILEN];
char timestr[NTIME];
time_t   t;
 
+   buf = NULL;
+   sz = 0;
+
n = snprintf(qcmd, sizeof(qcmd), "%s 2>&1", command);
if (n < 0 || n >= sizeof(qcmd))
return (NULL);
@@ -210,15 +214,14 @@ compile_mode(const char *name, const cha
ewprintf("Problem opening pipe");
return (NULL);
}
-   /*
-* We know that our commands are nice and the last line will end with
-* a \n, so we don't need to try to deal with the last line problem
-* in fgetln.
-*/
-   while ((buf = fgetln(fpipe, )) != NULL) {
-   buf[len - 1] = '\0';
+   while ((len = getline(, , fpipe)) != -1) {
+   if (buf[len - 1] == '\n')
+   buf[len - 1] = '\0';
addline(bp, buf);
}
+   free(buf);
+   if (ferror(fpipe))
+   ewprintf("Problem reading pipe");
ret = pclose(fpipe);
t = time(NULL);
strftime(timestr, sizeof(timestr), "%a %b %e %T %Y", localtime());

Re: [patch] hostname.if5 additional info on point to point addressing

[Tom Smyth]

Hello Stuart, all,
Thanks for the corrections Stuart,
I have corrected the patch to take into account your suggestions
and I hope this proposed patch is more correct and useful
Index: src/share/man/man5/hostname.if.5
===
RCS file: /cvs/src/share/man/man5/hostname.if.5,v
retrieving revision 1.65
diff -u -p -u -r1.65 hostname.if.5
--- src/share/man/man5/hostname.if.510 Mar 2017 18:28:11 -  1.65
+++ src/share/man/man5/hostname.if.512 Oct 2017 00:06:15 -
@@ -91,6 +91,16 @@ Regular IPv4 network setup:
 .Va dest_addr
 .Ed
 .Pp
+Point to Point IPv4 network setup:
+.Bd -ragged -offset indent
+.Li inet
+.Op Li alias
+.Va addr
+.Va netmask
+.Va network_addr
+.Va options
+.Ed
+.Pp
 Regular IPv6 network setup:
 .Bd -ragged -offset indent
 .Li inet6
@@ -122,6 +132,15 @@ inet6 alias fec0::1 64
 inet6 alias fec0::2 64 anycast
 !route add 65.65.65.65 10.0.1.13
 up
+.Ed
+.Pp
+Point to point IP addresses or IP unnumbered addresses
+can also be applied to an interface iff it is a tunnel or serial interface
+such as; gif(4), gre(4), pppoe(4), ppp(4), sppp(4).
+For example:
+.Bd -literal -offset 1n
+inet 10.64.100.2 0x 10.64.80.25
+#local_addr  /32_netmask remote_addr
 .Ed
 .Pp
 The above formats have the following field values:

On 2 October 2017 at 11:33, Stuart Henderson  wrote:
> On 2017/10/02 03:04, Tom Smyth wrote:
>> Hello,
>>
>> But the Ip configuration syntax in hostname.if is the same.
>
> For a /31 you just use e.g. "inet 192.0.2.100/31" (and it works properly
> in other parts of the system, e.g. ospfd).
>
>> Is there anything specifically wrong in the proposed patch ?
>
> This configuration only works on actual point-to-point interfaces (gif, gre,
> tun). Without further explanation people might expect it to work on ethernet
> like interfaces, and the "endpoint" address (10.64.80.25 in your example)
> doesn't do anything there.
>



-- 
Kindest regards,
Tom Smyth

Mobile: +353 87 6193172
The information contained in this E-mail is intended only for the
confidential use of the named recipient. If the reader of this message
is not the intended recipient or the person responsible for
delivering it to the recipient, you are hereby notified that you have
received this communication in error and that any review,
dissemination or copying of this communication is strictly prohibited.
If you have received this in error, please notify the sender
immediately by telephone at the number above and erase the message
You are requested to carry out your own virus check before
opening any attachment.

route warning

[Julien Dhaille]

Hi,

when a default gateway is not set :

# route get 4.4.4.4
route: writing to routing socket: No such process

this small patch uses oerrno translation :

# route get 4.4.4.4
get host 4.4.4.4: not in table

Index: route.c
===
RCS file: /cvs/src/sbin/route/route.c,v
retrieving revision 1.203
diff -u -p -r1.203 route.c
--- route.c 6 Sep 2017 20:21:22 -   1.203
+++ route.c 11 Oct 2017 22:05:22 -
@@ -676,8 +676,7 @@ newroute(int argc, char **argv)
}
if (*cmd == 'g') {
if (ret != 0 && qflag == 0)
-   warn("writing to routing socket");
-   exit(0);
+   oerrno = ESRCH;
}
oerrno = errno;
if (!qflag) {

Re: IPsec w/o KERNEL_LOCK()

[Alexander Bluhm]

On Wed, Oct 11, 2017 at 05:01:46PM +0200, Martin Pieuchot wrote:
> Tests and comments welcome.

All regress tests passed.

Code looks reasonable.

OK bluhm@

> Index: net/if_enc.c
> ===
> RCS file: /cvs/src/sys/net/if_enc.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 if_enc.c
> --- net/if_enc.c  11 Aug 2017 21:24:19 -  1.69
> +++ net/if_enc.c  11 Oct 2017 14:44:48 -
> @@ -214,6 +214,8 @@ enc_getif(u_int id, u_int unit)
>  {
>   struct ifnet*ifp;
>  
> + NET_ASSERT_LOCKED();
> +
>   /* Check if the caller wants to get a non-default enc interface */
>   if (unit > 0) {
>   if (unit > enc_max_unit)
> Index: net/pfkeyv2.c
> ===
> RCS file: /cvs/src/sys/net/pfkeyv2.c,v
> retrieving revision 1.167
> diff -u -p -r1.167 pfkeyv2.c
> --- net/pfkeyv2.c 9 Oct 2017 08:35:38 -   1.167
> +++ net/pfkeyv2.c 11 Oct 2017 14:44:49 -
> @@ -80,6 +80,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
>  #include 
>  #include 
>  #include 
> @@ -148,6 +150,8 @@ struct dump_state {
>  
>  /* Static globals */
>  static LIST_HEAD(, keycb) pfkeyv2_sockets = LIST_HEAD_INITIALIZER(keycb);
> +
> +struct mutex pfkeyv2_mtx = MUTEX_INITIALIZER(IPL_NONE);
>  static uint32_t pfkeyv2_seq = 1;
>  static int nregistered = 0;
>  static int npromisc = 0;
> @@ -260,11 +264,16 @@ pfkeyv2_detach(struct socket *so, struct
>  
>   LIST_REMOVE(kp, kcb_list);
>  
> - if (kp->flags & PFKEYV2_SOCKETFLAGS_REGISTERED)
> - nregistered--;
> -
> - if (kp->flags & PFKEYV2_SOCKETFLAGS_PROMISC)
> - npromisc--;
> + if (kp->flags &
> + (PFKEYV2_SOCKETFLAGS_REGISTERED|PFKEYV2_SOCKETFLAGS_PROMISC)) {
> + mtx_enter(_mtx);
> + if (kp->flags & PFKEYV2_SOCKETFLAGS_REGISTERED)
> + nregistered--;
> +
> + if (kp->flags & PFKEYV2_SOCKETFLAGS_PROMISC)
> + npromisc--;
> + mtx_leave(_mtx);
> + }
>  
>   raw_detach(>rcb);
>   return (0);
> @@ -940,23 +949,22 @@ pfkeyv2_send(struct socket *so, void *me
>   struct ipsec_acquire *ipa;
>   struct radix_node_head *rnh;
>   struct radix_node *rn = NULL;
> -
>   struct keycb *kp, *bkp;
> -
>   void *freeme = NULL, *bckptr = NULL;
>   void *headers[SADB_EXT_MAX + 1];
> -
>   union sockaddr_union *sunionp;
> -
>   struct tdb *sa1 = NULL, *sa2 = NULL;
> -
>   struct sadb_msg *smsg;
>   struct sadb_spirange *sprng;
>   struct sadb_sa *ssa;
>   struct sadb_supported *ssup;
>   struct sadb_ident *sid, *did;
> -
>   u_int rdomain;
> + int promisc;
> +
> + mtx_enter(_mtx);
> + promisc = npromisc;
> + mtx_leave(_mtx);
>  
>   NET_LOCK();
>  
> @@ -972,7 +980,7 @@ pfkeyv2_send(struct socket *so, void *me
>   rdomain = kp->rdomain;
>  
>   /* If we have any promiscuous listeners, send them a copy of the 
> message */
> - if (npromisc) {
> + if (promisc) {
>   struct mbuf *packet;
>  
>   if (!(freeme = malloc(sizeof(struct sadb_msg) + len, M_PFKEY,
> @@ -1392,7 +1400,9 @@ pfkeyv2_send(struct socket *so, void *me
>   case SADB_REGISTER:
>   if (!(kp->flags & PFKEYV2_SOCKETFLAGS_REGISTERED)) {
>   kp->flags |= PFKEYV2_SOCKETFLAGS_REGISTERED;
> + mtx_enter(_mtx);
>   nregistered++;
> + mtx_leave(_mtx);
>   }
>  
>   i = sizeof(struct sadb_supported) + sizeof(ealgs);
> @@ -1788,11 +1798,15 @@ pfkeyv2_send(struct socket *so, void *me
>   if (j) {
>   kp->flags |=
>   PFKEYV2_SOCKETFLAGS_PROMISC;
> + mtx_enter(_mtx);
>   npromisc++;
> + mtx_leave(_mtx);
>   } else {
>   kp->flags &=
>   ~PFKEYV2_SOCKETFLAGS_PROMISC;
> + mtx_enter(_mtx);
>   npromisc--;
> + mtx_leave(_mtx);
>   }
>   }
>   }
> @@ -1859,11 +1873,15 @@ pfkeyv2_acquire(struct ipsec_policy *ipo
>   struct sadb_prop *sa_prop;
>   struct sadb_msg *smsg;
>   int rval = 0;
> - int i, j;
> + int i, j, registered;
>  
> + mtx_enter(_mtx);
>   *seq = pfkeyv2_seq++;
>  
> - if (!nregistered) {
> + registered = nregistered;
> + mtx_leave(_mtx);
> +
> + if (!registered) {
>   rval = ESRCH;
>   goto ret;
>   }
> @@ -2100,7 +2118,10 @@ pfkeyv2_expire(struct tdb *sa, 

Re: rw locks vs memory barriers and adaptive spinning

[Mateusz Guzik]

On Wed, Oct 11, 2017 at 03:15:42PM +0200, Martin Pieuchot wrote:
> On 10/10/17(Tue) 20:19, Mateusz Guzik wrote:
> > On Tue, Oct 10, 2017 at 10:15:48AM +0200, Martin Pieuchot wrote:
> > > Hello Mateusz,
> > >
> > > On 09/10/17(Mon) 21:43, Mateusz Guzik wrote:
> > > > I was looking at rw lock code out of curiosity and noticed you always do
> > > > membar_enter which on MP-enabled amd64 kernel translates to mfence.
> > > > This makes the entire business a little bit slower.
> > > >
> > > > Interestingly you already have relevant macros for amd64:
> > > > #define membar_enter_after_atomic() __membar("")
> > > > #define membar_exit_before_atomic() __membar("")
> > >
> > > If you look at the history, you'll see that theses macro didn't exist
> > > when membar_* where added to rw locks.
> > >
> > 
> > I know. Addition of such a macro sounds like an accelent opportunity to
> > examine existing users. I figured rw locks were ommitted by accident
> > as the original posting explicitly mentions mutexes.
> > 
> > https://marc.info/?l=openbsd-tech=149555411827749=2
> > 
> > > > And there is even a default variant for archs which don't provide one.
> > > > I guess the switch should be easy.
> > >
> > > Then please do it :)  At lot of stuff is easy on OpenBSD but we are not
> > > enough.
> > >
> > > Do it, test it, explain it, get oks and commit it 8)
> > >
> > 
> > As noted earlier the patch is trivial. I have no easy means to even
> > compile-test it though as I'm not using OpenBSD. Only had a look out of
> > curiosity.
> 
> Why don't you start using it?  Testing is what makes the difference.
> Many of us have ideas of how to improve the kernel but we're lacking
> the time to test & debug them all.
> 

I'm playing with concurrency as a hobby and smpifying single-threaded
code is above the pain threshold I'm willing to accept in my spare time.

> Sometimes a "correct" change exposes a bug and we try not to break
> -current, so we cannot afford to commit a non-tested diff.
> 

I definitely don't advocate for just committing stuff. I do state though
that there should be no binary change for archs other than i386 and
amd64 (== nothing to test there). Given the nature of the change for
these two it really seemed it just fell through the cracks earlier and I
just wanted to point it out as perhaps someone will find cycles to pick
it up and test on relevant platforms.

> > Nonetheless, here is the diff which can be split in 2. One part deals
> > with barriers, another one cleans up rw_status.
> >
> > [...]
> >
> > Read from the lock only once in rw_status. The lock word is marked as
> > volatile which forces re-read on each access. There is no correctness
> > issue here, but the assembly is worse than it needs to be and in case
> > of contention this adds avoidable cache traffic.
> 
> Here's the diff to cleans rw_status.  The difference on amd64 with
> clang 4.0  is the following:
> 
>   48 8b 0fmov(%rdi),%rcx
>   f6 c1 04test   $0x4,%cl
>   75 0c   jne738 
>   31 c0   xor%eax,%eax
> - 48 83 3f 00 cmpq   $0x0,(%rdi)
> + 48 85 c9test   %rcx,%rcx
> 
> ok?


There should be another change past rrw_status+0x18:
812557e8:   65 48 8b 04 25 08 00mov%gs:0x8,%rax
812557ef:   00 00
812557f1:   48 8b 80 90 02 00 00mov0x290(%rax),%rax
812557f8:   48 33 07xor(%rdi),%rax

> 
> Index: kern/kern_rwlock.c
> ===
> RCS file: /cvs/src/sys/kern/kern_rwlock.c,v
> retrieving revision 1.30
> diff -u -p -r1.30 kern_rwlock.c
> --- kern/kern_rwlock.c12 Aug 2017 23:27:44 -  1.30
> +++ kern/kern_rwlock.c11 Oct 2017 12:59:19 -
> @@ -312,13 +312,15 @@ _rw_exit(struct rwlock *rwl LOCK_FL_VARS
>  int
>  rw_status(struct rwlock *rwl)
>  {
> - if (rwl->rwl_owner & RWLOCK_WRLOCK) {
> - if (RW_PROC(curproc) == RW_PROC(rwl->rwl_owner))
> + unsigned long owner = rwl->rwl_owner;
> +
> + if (owner & RWLOCK_WRLOCK) {
> + if (RW_PROC(curproc) == RW_PROC(owner))
>   return RW_WRITE;
>   else
>   return RW_WRITE_OTHER;
>   }
> - if (rwl->rwl_owner)
> + if (owner)
>   return RW_READ;
>   return (0);
>  }

-- 
Mateusz Guzik

Re: perl 5.24.3 update (OK?)

[Alexander Bluhm]

On Fri, Sep 22, 2017 at 06:56:27PM -0700, Andrew Fresh wrote:
> Hoping this will be able to go in before the lock, I've heard good

Updating Perl after unlock is better.  Running with it for a while
without problems.  No library bump needed.

OK bluhm@ for Perl v5.24.3

IPsec w/o KERNEL_LOCK()

[Martin Pieuchot]

OpenBSD 6.2 includes nice performance and latency improvements due to
the work done in the Network Stack in the previous years.  However as
soon as IPsec is enabled, all network related processing are affected.
In other words you cannot profit from the last MP work in the Network
stack if you use IPsec.

During the last 6 months I hoped that somebody else would look at the
IPsec stack and tell us what needs to be done.  This didn't happen.

Now that 6.2 is released, we cannot afford to continue to parallelize
the Network Stack if some of our users and testers still run it under
KERNEL_LOCK(). 

So I did an audit of the IPsec stack and came with the small diff below.
This diff doesn't remove the KERNEL_LOCK() (yet), but add some asserts
to make sure that the global data structure are all accessed while
holding the NET_LOCK().

I introduced a mutex for protecting some globals in the PF_KEY sockets.
If you want to improve it and try to unlock the PF_KEY socket layer,
that'd make me really happy.

In the whole IPsec stack, a single timeout wasn't running in process
context, I changed that and made it grab the NET_LOCK().

I tried to document which data structures are protected by the
NET_LOCK(), to make it short it's "all of them".

Tests and comments welcome.

Index: net/if_enc.c
===
RCS file: /cvs/src/sys/net/if_enc.c,v
retrieving revision 1.69
diff -u -p -r1.69 if_enc.c
--- net/if_enc.c11 Aug 2017 21:24:19 -  1.69
+++ net/if_enc.c11 Oct 2017 14:44:48 -
@@ -214,6 +214,8 @@ enc_getif(u_int id, u_int unit)
 {
struct ifnet*ifp;
 
+   NET_ASSERT_LOCKED();
+
/* Check if the caller wants to get a non-default enc interface */
if (unit > 0) {
if (unit > enc_max_unit)
Index: net/pfkeyv2.c
===
RCS file: /cvs/src/sys/net/pfkeyv2.c,v
retrieving revision 1.167
diff -u -p -r1.167 pfkeyv2.c
--- net/pfkeyv2.c   9 Oct 2017 08:35:38 -   1.167
+++ net/pfkeyv2.c   11 Oct 2017 14:44:49 -
@@ -80,6 +80,8 @@
 #include 
 #include 
 #include 
+#include 
+
 #include 
 #include 
 #include 
@@ -148,6 +150,8 @@ struct dump_state {
 
 /* Static globals */
 static LIST_HEAD(, keycb) pfkeyv2_sockets = LIST_HEAD_INITIALIZER(keycb);
+
+struct mutex pfkeyv2_mtx = MUTEX_INITIALIZER(IPL_NONE);
 static uint32_t pfkeyv2_seq = 1;
 static int nregistered = 0;
 static int npromisc = 0;
@@ -260,11 +264,16 @@ pfkeyv2_detach(struct socket *so, struct
 
LIST_REMOVE(kp, kcb_list);
 
-   if (kp->flags & PFKEYV2_SOCKETFLAGS_REGISTERED)
-   nregistered--;
-
-   if (kp->flags & PFKEYV2_SOCKETFLAGS_PROMISC)
-   npromisc--;
+   if (kp->flags &
+   (PFKEYV2_SOCKETFLAGS_REGISTERED|PFKEYV2_SOCKETFLAGS_PROMISC)) {
+   mtx_enter(_mtx);
+   if (kp->flags & PFKEYV2_SOCKETFLAGS_REGISTERED)
+   nregistered--;
+
+   if (kp->flags & PFKEYV2_SOCKETFLAGS_PROMISC)
+   npromisc--;
+   mtx_leave(_mtx);
+   }
 
raw_detach(>rcb);
return (0);
@@ -940,23 +949,22 @@ pfkeyv2_send(struct socket *so, void *me
struct ipsec_acquire *ipa;
struct radix_node_head *rnh;
struct radix_node *rn = NULL;
-
struct keycb *kp, *bkp;
-
void *freeme = NULL, *bckptr = NULL;
void *headers[SADB_EXT_MAX + 1];
-
union sockaddr_union *sunionp;
-
struct tdb *sa1 = NULL, *sa2 = NULL;
-
struct sadb_msg *smsg;
struct sadb_spirange *sprng;
struct sadb_sa *ssa;
struct sadb_supported *ssup;
struct sadb_ident *sid, *did;
-
u_int rdomain;
+   int promisc;
+
+   mtx_enter(_mtx);
+   promisc = npromisc;
+   mtx_leave(_mtx);
 
NET_LOCK();
 
@@ -972,7 +980,7 @@ pfkeyv2_send(struct socket *so, void *me
rdomain = kp->rdomain;
 
/* If we have any promiscuous listeners, send them a copy of the 
message */
-   if (npromisc) {
+   if (promisc) {
struct mbuf *packet;
 
if (!(freeme = malloc(sizeof(struct sadb_msg) + len, M_PFKEY,
@@ -1392,7 +1400,9 @@ pfkeyv2_send(struct socket *so, void *me
case SADB_REGISTER:
if (!(kp->flags & PFKEYV2_SOCKETFLAGS_REGISTERED)) {
kp->flags |= PFKEYV2_SOCKETFLAGS_REGISTERED;
+   mtx_enter(_mtx);
nregistered++;
+   mtx_leave(_mtx);
}
 
i = sizeof(struct sadb_supported) + sizeof(ealgs);
@@ -1788,11 +1798,15 @@ pfkeyv2_send(struct socket *so, void *me
if (j) {
kp->flags |=
PFKEYV2_SOCKETFLAGS_PROMISC;
+   mtx_enter(_mtx);
   

Re: Send sysctl_mq() where it belongs

[Alexander Bluhm]

On Wed, Oct 11, 2017 at 02:58:21PM +0200, Martin Pieuchot wrote:
> sysctl_mq() messes with 'struct mbuf_queue' internals, so keep it in
> kern/uipc_mbuf.c with the rest of the mq* functions.

I wonder whether it should be called mq_sysctl() as it will be
located in the mq_ namespace.  A grep for _sysctl and sysctl_ shows
that we are rather inconsistent in naming, so commit it either way.

> ok?

OK bluhm@

> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.514
> diff -u -p -r1.514 if.c
> --- net/if.c  11 Oct 2017 07:57:27 -  1.514
> +++ net/if.c  11 Oct 2017 12:56:18 -
> @@ -82,7 +82,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -2654,38 +2653,6 @@ ifpromisc(struct ifnet *ifp, int pswitch
>   }
>   ifr.ifr_flags = ifp->if_flags;
>   return ((*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)));
> -}
> -
> -/* XXX move to kern/uipc_mbuf.c */
> -int
> -sysctl_mq(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> -void *newp, size_t newlen, struct mbuf_queue *mq)
> -{
> - unsigned int maxlen;
> - int error;
> -
> - /* All sysctl names at this level are terminal. */
> - if (namelen != 1)
> - return (ENOTDIR);
> -
> - switch (name[0]) {
> - case IFQCTL_LEN:
> - return (sysctl_rdint(oldp, oldlenp, newp, mq_len(mq)));
> - case IFQCTL_MAXLEN:
> - maxlen = mq->mq_maxlen;
> - error = sysctl_int(oldp, oldlenp, newp, newlen, );
> - if (!error && maxlen != mq->mq_maxlen) {
> - mtx_enter(>mq_mtx);
> - mq->mq_maxlen = maxlen;
> - mtx_leave(>mq_mtx);
> - }
> - return (error);
> - case IFQCTL_DROPS:
> - return (sysctl_rdint(oldp, oldlenp, newp, mq_drops(mq)));
> - default:
> - return (EOPNOTSUPP);
> - }
> - /* NOTREACHED */
>  }
>  
>  void
> Index: net/if_var.h
> ===
> RCS file: /cvs/src/sys/net/if_var.h,v
> retrieving revision 1.81
> diff -u -p -r1.81 if_var.h
> --- net/if_var.h  8 May 2017 08:46:39 -   1.81
> +++ net/if_var.h  11 Oct 2017 12:56:18 -
> @@ -324,9 +324,6 @@ int   if_clone_destroy(const char *);
>  struct if_clone *
>   if_clone_lookup(const char *, int *);
>  
> -int sysctl_mq(int *, u_int, void *, size_t *, void *, size_t,
> - struct mbuf_queue *);
> -
>  void ifa_add(struct ifnet *, struct ifaddr *);
>  void ifa_del(struct ifnet *, struct ifaddr *);
>  void ifa_update_broadaddr(struct ifnet *, struct ifaddr *,
> Index: kern/uipc_mbuf.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
> retrieving revision 1.249
> diff -u -p -r1.249 uipc_mbuf.c
> --- kern/uipc_mbuf.c  15 Sep 2017 18:13:05 -  1.249
> +++ kern/uipc_mbuf.c  11 Oct 2017 12:56:19 -
> @@ -84,6 +84,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -1648,4 +1649,35 @@ mq_purge(struct mbuf_queue *mq)
>   mq_delist(mq, );
>  
>   return (ml_purge());
> +}
> +
> +int
> +sysctl_mq(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> +void *newp, size_t newlen, struct mbuf_queue *mq)
> +{
> + unsigned int maxlen;
> + int error;
> +
> + /* All sysctl names at this level are terminal. */
> + if (namelen != 1)
> + return (ENOTDIR);
> +
> + switch (name[0]) {
> + case IFQCTL_LEN:
> + return (sysctl_rdint(oldp, oldlenp, newp, mq_len(mq)));
> + case IFQCTL_MAXLEN:
> + maxlen = mq->mq_maxlen;
> + error = sysctl_int(oldp, oldlenp, newp, newlen, );
> + if (!error && maxlen != mq->mq_maxlen) {
> + mtx_enter(>mq_mtx);
> + mq->mq_maxlen = maxlen;
> + mtx_leave(>mq_mtx);
> + }
> + return (error);
> + case IFQCTL_DROPS:
> + return (sysctl_rdint(oldp, oldlenp, newp, mq_drops(mq)));
> + default:
> + return (EOPNOTSUPP);
> + }
> + /* NOTREACHED */
>  }
> Index: sys/sysctl.h
> ===
> RCS file: /cvs/src/sys/sys/sysctl.h,v
> retrieving revision 1.174
> diff -u -p -r1.174 sysctl.h
> --- sys/sysctl.h  14 Jun 2017 03:00:40 -  1.174
> +++ sys/sysctl.h  11 Oct 2017 12:56:21 -
> @@ -936,6 +936,9 @@ int sysctl_rdstruct(void *, size_t *, vo
>  int sysctl_struct(void *, size_t *, void *, size_t, void *, size_t);
>  int sysctl_file(int *, u_int, char *, size_t *, struct proc *);
>  int sysctl_doproc(int *, u_int, char *, size_t *);
> +struct mbuf_queue;
> +int sysctl_mq(int *, u_int, void *, size_t *, void *, size_t,
> +struct mbuf_queue *);
>  struct rtentry;
>  

Re: ifioctl() cleanup, step 2

[Alexander Bluhm]

On Wed, Oct 11, 2017 at 10:44:22AM +0200, Martin Pieuchot wrote:
> Moar cleanup to be able to selectively take the NET_LOCK() around some
> ioctls.
> 
> This diff change many "return (error)" into "break".

It looks a bit inconsistent, where you replace the return and where
not.  But I am sure your next diff will resolve this.

> It adds error checks for SIOC{A,D}IFGROUP.  The only driver handling
> these ioctl(2)s is... carp(4)!

This (error == ENOTTY) is strange.  I hides something in the kernel
where the user land requests impossible things and cannot cope with
an error.  But as we do not want to break things, I think this
approach makes sense.

> ok?

OK bluhm@

> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.514
> diff -u -p -r1.514 if.c
> --- net/if.c  11 Oct 2017 07:57:27 -  1.514
> +++ net/if.c  11 Oct 2017 08:37:31 -
> @@ -1817,7 +1817,7 @@ ifioctl(struct socket *so, u_long cmd, c
>  {
>   struct ifnet *ifp;
>   struct ifreq *ifr = (struct ifreq *)data;
> - struct ifgroupreq *ifgr;
> + struct ifgroupreq *ifgr = (struct ifgroupreq *)data;
>   struct if_afreq *ifar = (struct if_afreq *)data;
>   char ifdescrbuf[IFDESCRSIZE];
>   char ifrtlabelbuf[RTLABEL_LEN];
> @@ -1858,7 +1858,7 @@ ifioctl(struct socket *so, u_long cmd, c
>   case SIOCIFAFATTACH:
>   case SIOCIFAFDETACH:
>   if ((error = suser(p, 0)) != 0)
> - return (error);
> + break;
>   switch (ifar->ifar_af) {
>   case AF_INET:
>   /* attach is a noop for AF_INET */
> @@ -1874,7 +1874,7 @@ ifioctl(struct socket *so, u_long cmd, c
>   break;
>  #endif /* INET6 */
>   default:
> - return (EAFNOSUPPORT);
> + error = EAFNOSUPPORT;
>   }
>   break;
>  
> @@ -1909,7 +1909,7 @@ ifioctl(struct socket *so, u_long cmd, c
>  
>   case SIOCSIFFLAGS:
>   if ((error = suser(p, 0)) != 0)
> - return (error);
> + break;
>  
>   ifp->if_flags = (ifp->if_flags & IFF_CANTCHANGE) |
>   (ifr->ifr_flags & ~IFF_CANTCHANGE);
> @@ -1934,13 +1934,13 @@ ifioctl(struct socket *so, u_long cmd, c
>  
>   case SIOCSIFXFLAGS:
>   if ((error = suser(p, 0)) != 0)
> - return (error);
> + break;
>  
>  #ifdef INET6
>   if (ISSET(ifr->ifr_flags, IFXF_AUTOCONF6)) {
>   error = in6_ifattach(ifp);
>   if (error != 0)
> - return (error);
> + break;
>   }
>  #endif   /* INET6 */
>  
> @@ -1972,7 +1972,7 @@ ifioctl(struct socket *so, u_long cmd, c
>   error = ifp->if_wol(ifp, 1);
>   splx(s);
>   if (error)
> - return (error);
> + break;
>   }
>   if (ISSET(ifp->if_xflags, IFXF_WOL) &&
>   !ISSET(ifr->ifr_flags, IFXF_WOL)) {
> @@ -1981,7 +1981,7 @@ ifioctl(struct socket *so, u_long cmd, c
>   error = ifp->if_wol(ifp, 0);
>   splx(s);
>   if (error)
> - return (error);
> + break;
>   }
>   } else if (ISSET(ifr->ifr_flags, IFXF_WOL)) {
>   ifr->ifr_flags &= ~IFXF_WOL;
> @@ -1995,13 +1995,13 @@ ifioctl(struct socket *so, u_long cmd, c
>  
>   case SIOCSIFMETRIC:
>   if ((error = suser(p, 0)) != 0)
> - return (error);
> + break;
>   ifp->if_metric = ifr->ifr_metric;
>   break;
>  
>   case SIOCSIFMTU:
>   if ((error = suser(p, 0)) != 0)
> - return (error);
> + break;
>   if (ifp->if_ioctl == NULL)
>   return (EOPNOTSUPP);
>   error = (*ifp->if_ioctl)(ifp, cmd, data);
> @@ -2049,7 +2049,7 @@ ifioctl(struct socket *so, u_long cmd, c
>  
>   case SIOCSIFDESCR:
>   if ((error = suser(p, 0)) != 0)
> - return (error);
> + break;
>   error = copyinstr(ifr->ifr_data, ifdescrbuf,
>   IFDESCRSIZE, );
>   if (error == 0) {
> @@ -2070,7 +2070,7 @@ ifioctl(struct socket *so, u_long cmd, c
>  
>   case SIOCSIFRTLABEL:
>   if ((error = suser(p, 0)) != 0)
> - return (error);
> + break;
>   error = copyinstr(ifr->ifr_data, ifrtlabelbuf,
>   

Re: rw locks vs memory barriers and adaptive spinning

[Martin Pieuchot]

On 10/10/17(Tue) 20:19, Mateusz Guzik wrote:
> On Tue, Oct 10, 2017 at 10:15:48AM +0200, Martin Pieuchot wrote:
> > Hello Mateusz,
> >
> > On 09/10/17(Mon) 21:43, Mateusz Guzik wrote:
> > > I was looking at rw lock code out of curiosity and noticed you always do
> > > membar_enter which on MP-enabled amd64 kernel translates to mfence.
> > > This makes the entire business a little bit slower.
> > >
> > > Interestingly you already have relevant macros for amd64:
> > > #define membar_enter_after_atomic() __membar("")
> > > #define membar_exit_before_atomic() __membar("")
> >
> > If you look at the history, you'll see that theses macro didn't exist
> > when membar_* where added to rw locks.
> >
> 
> I know. Addition of such a macro sounds like an accelent opportunity to
> examine existing users. I figured rw locks were ommitted by accident
> as the original posting explicitly mentions mutexes.
> 
> https://marc.info/?l=openbsd-tech=149555411827749=2
> 
> > > And there is even a default variant for archs which don't provide one.
> > > I guess the switch should be easy.
> >
> > Then please do it :)  At lot of stuff is easy on OpenBSD but we are not
> > enough.
> >
> > Do it, test it, explain it, get oks and commit it 8)
> >
> 
> As noted earlier the patch is trivial. I have no easy means to even
> compile-test it though as I'm not using OpenBSD. Only had a look out of
> curiosity.

Why don't you start using it?  Testing is what makes the difference.
Many of us have ideas of how to improve the kernel but we're lacking
the time to test & debug them all.

Sometimes a "correct" change exposes a bug and we try not to break
-current, so we cannot afford to commit a non-tested diff.

> Nonetheless, here is the diff which can be split in 2. One part deals
> with barriers, another one cleans up rw_status.
>
> [...]
>
> Read from the lock only once in rw_status. The lock word is marked as
> volatile which forces re-read on each access. There is no correctness
> issue here, but the assembly is worse than it needs to be and in case
> of contention this adds avoidable cache traffic.

Here's the diff to cleans rw_status.  The difference on amd64 with
clang 4.0  is the following:

48 8b 0fmov(%rdi),%rcx
f6 c1 04test   $0x4,%cl
75 0c   jne738 
31 c0   xor%eax,%eax
-   48 83 3f 00 cmpq   $0x0,(%rdi)
+   48 85 c9test   %rcx,%rcx

ok?

Index: kern/kern_rwlock.c
===
RCS file: /cvs/src/sys/kern/kern_rwlock.c,v
retrieving revision 1.30
diff -u -p -r1.30 kern_rwlock.c
--- kern/kern_rwlock.c  12 Aug 2017 23:27:44 -  1.30
+++ kern/kern_rwlock.c  11 Oct 2017 12:59:19 -
@@ -312,13 +312,15 @@ _rw_exit(struct rwlock *rwl LOCK_FL_VARS
 int
 rw_status(struct rwlock *rwl)
 {
-   if (rwl->rwl_owner & RWLOCK_WRLOCK) {
-   if (RW_PROC(curproc) == RW_PROC(rwl->rwl_owner))
+   unsigned long owner = rwl->rwl_owner;
+
+   if (owner & RWLOCK_WRLOCK) {
+   if (RW_PROC(curproc) == RW_PROC(owner))
return RW_WRITE;
else
return RW_WRITE_OTHER;
}
-   if (rwl->rwl_owner)
+   if (owner)
return RW_READ;
return (0);
 }

Send sysctl_mq() where it belongs

[Martin Pieuchot]

sysctl_mq() messes with 'struct mbuf_queue' internals, so keep it in
kern/uipc_mbuf.c with the rest of the mq* functions.

ok?

Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.514
diff -u -p -r1.514 if.c
--- net/if.c11 Oct 2017 07:57:27 -  1.514
+++ net/if.c11 Oct 2017 12:56:18 -
@@ -82,7 +82,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -2654,38 +2653,6 @@ ifpromisc(struct ifnet *ifp, int pswitch
}
ifr.ifr_flags = ifp->if_flags;
return ((*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)));
-}
-
-/* XXX move to kern/uipc_mbuf.c */
-int
-sysctl_mq(int *name, u_int namelen, void *oldp, size_t *oldlenp,
-void *newp, size_t newlen, struct mbuf_queue *mq)
-{
-   unsigned int maxlen;
-   int error;
-
-   /* All sysctl names at this level are terminal. */
-   if (namelen != 1)
-   return (ENOTDIR);
-
-   switch (name[0]) {
-   case IFQCTL_LEN:
-   return (sysctl_rdint(oldp, oldlenp, newp, mq_len(mq)));
-   case IFQCTL_MAXLEN:
-   maxlen = mq->mq_maxlen;
-   error = sysctl_int(oldp, oldlenp, newp, newlen, );
-   if (!error && maxlen != mq->mq_maxlen) {
-   mtx_enter(>mq_mtx);
-   mq->mq_maxlen = maxlen;
-   mtx_leave(>mq_mtx);
-   }
-   return (error);
-   case IFQCTL_DROPS:
-   return (sysctl_rdint(oldp, oldlenp, newp, mq_drops(mq)));
-   default:
-   return (EOPNOTSUPP);
-   }
-   /* NOTREACHED */
 }
 
 void
Index: net/if_var.h
===
RCS file: /cvs/src/sys/net/if_var.h,v
retrieving revision 1.81
diff -u -p -r1.81 if_var.h
--- net/if_var.h8 May 2017 08:46:39 -   1.81
+++ net/if_var.h11 Oct 2017 12:56:18 -
@@ -324,9 +324,6 @@ int if_clone_destroy(const char *);
 struct if_clone *
if_clone_lookup(const char *, int *);
 
-int sysctl_mq(int *, u_int, void *, size_t *, void *, size_t,
-   struct mbuf_queue *);
-
 void   ifa_add(struct ifnet *, struct ifaddr *);
 void   ifa_del(struct ifnet *, struct ifaddr *);
 void   ifa_update_broadaddr(struct ifnet *, struct ifaddr *,
Index: kern/uipc_mbuf.c
===
RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.249
diff -u -p -r1.249 uipc_mbuf.c
--- kern/uipc_mbuf.c15 Sep 2017 18:13:05 -  1.249
+++ kern/uipc_mbuf.c11 Oct 2017 12:56:19 -
@@ -84,6 +84,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -1648,4 +1649,35 @@ mq_purge(struct mbuf_queue *mq)
mq_delist(mq, );
 
return (ml_purge());
+}
+
+int
+sysctl_mq(int *name, u_int namelen, void *oldp, size_t *oldlenp,
+void *newp, size_t newlen, struct mbuf_queue *mq)
+{
+   unsigned int maxlen;
+   int error;
+
+   /* All sysctl names at this level are terminal. */
+   if (namelen != 1)
+   return (ENOTDIR);
+
+   switch (name[0]) {
+   case IFQCTL_LEN:
+   return (sysctl_rdint(oldp, oldlenp, newp, mq_len(mq)));
+   case IFQCTL_MAXLEN:
+   maxlen = mq->mq_maxlen;
+   error = sysctl_int(oldp, oldlenp, newp, newlen, );
+   if (!error && maxlen != mq->mq_maxlen) {
+   mtx_enter(>mq_mtx);
+   mq->mq_maxlen = maxlen;
+   mtx_leave(>mq_mtx);
+   }
+   return (error);
+   case IFQCTL_DROPS:
+   return (sysctl_rdint(oldp, oldlenp, newp, mq_drops(mq)));
+   default:
+   return (EOPNOTSUPP);
+   }
+   /* NOTREACHED */
 }
Index: sys/sysctl.h
===
RCS file: /cvs/src/sys/sys/sysctl.h,v
retrieving revision 1.174
diff -u -p -r1.174 sysctl.h
--- sys/sysctl.h14 Jun 2017 03:00:40 -  1.174
+++ sys/sysctl.h11 Oct 2017 12:56:21 -
@@ -936,6 +936,9 @@ int sysctl_rdstruct(void *, size_t *, vo
 int sysctl_struct(void *, size_t *, void *, size_t, void *, size_t);
 int sysctl_file(int *, u_int, char *, size_t *, struct proc *);
 int sysctl_doproc(int *, u_int, char *, size_t *);
+struct mbuf_queue;
+int sysctl_mq(int *, u_int, void *, size_t *, void *, size_t,
+struct mbuf_queue *);
 struct rtentry;
 struct walkarg;
 int sysctl_dumpentry(struct rtentry *, void *, unsigned int);

Re: LLVM 5.0 on armv7

[Mark Kettenis]

> Date: Wed, 11 Oct 2017 14:21:17 +0200
> From: Patrick Wildt 
> 
> On Wed, Oct 11, 2017 at 12:19:47PM +0200, Mark Kettenis wrote:
> > Diff below makes clang and lld build again on armv7.  Sorted the files
> > such that they match the order in the CMakeLists.txt file.
> 
> The lists are explicitly sorted alphabetically and not like in the
> CMakeLists.  I would actually like to keep it that way.
> 
> If there are unsorted lists, then that might be because I didn't have
> to touch them since the last update. :)

Makes it hard to compare the lists though.  We can fight over that
another time though.

> > Index: gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile
> > ===
> > RCS file: /cvs/src/gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile,v
> > retrieving revision 1.3
> > diff -u -p -r1.3 Makefile
> > --- gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile9 Jul 2017 15:28:34 
> > -   1.3
> > +++ gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile11 Oct 2017 10:17:13 
> > -
> > @@ -11,7 +11,6 @@ SRCS= A15SDOptimizer.cpp \
> > ARMAsmPrinter.cpp \
> > ARMBaseInstrInfo.cpp \
> > ARMBaseRegisterInfo.cpp \
> > -   ARMComputeBlockSize.cpp \
> > ARMConstantIslandPass.cpp \
> > ARMConstantPoolValue.cpp \
> > ARMExpandPseudoInsts.cpp \
> > @@ -24,6 +23,7 @@ SRCS= A15SDOptimizer.cpp \
> > ARMLoadStoreOptimizer.cpp \
> > ARMMCInstLower.cpp \
> > ARMMachineFunctionInfo.cpp \
> > +   ARMMacroFusion.cpp \
> > ARMRegisterInfo.cpp \
> > ARMOptimizeBarriersPass.cpp \
> > ARMSelectionDAGInfo.cpp \
> > @@ -37,7 +37,8 @@ SRCS= A15SDOptimizer.cpp \
> > ThumbRegisterInfo.cpp \
> > Thumb2ITBlockPass.cpp \
> > Thumb2InstrInfo.cpp \
> > -   Thumb2SizeReduction.cpp
> > +   Thumb2SizeReduction.cpp \
> > +   ARMComputeBlockSize.cpp
> >  
> >  .PATH: ${.CURDIR}/../../../llvm/lib/Target/ARM
> >  
> > 
> 

additions

[Martin Pieuchot]

To make our CTF tools (and any ELF-related tool) easier to port to other
OSes, I'd like to follow the Solaris/FreeBSD/OSX lead and use a 
header instead of our current mix of  & .

However devel/libelf will use  instead of its own, if it is
available.  But with the current content of our  two
ports fail to build: devel/libdwarf and devel/valgrind.

The diff below adds the necessary defines for these ports to build.
Then I'd like to add  and bump devel/libelf.  Once I've dealt
with runtime fallouts, if any, I'll convert our base tools to use
.

ok?

Index: sys/exec_elf.h
===
RCS file: /cvs/src/sys/sys/exec_elf.h,v
retrieving revision 1.75
diff -u -p -r1.75 exec_elf.h
--- sys/exec_elf.h  5 Sep 2017 06:35:19 -   1.75
+++ sys/exec_elf.h  11 Oct 2017 12:34:35 -
@@ -187,12 +187,14 @@ typedef struct {
 #define EM_PARISC  15  /* HPPA */
 #define EM_SPARC32PLUS 18  /* Enhanced instruction set SPARC */
 #define EM_PPC 20  /* PowerPC */
+#define EM_PPC64   21  /* PowerPC 64 */
 #define EM_ARM 40  /* Advanced RISC Machines ARM */
 #define EM_ALPHA   41  /* DEC ALPHA */
-#defineEM_SH   42  /* Hitachi/Renesas Super-H */
+#define EM_SH  42  /* Hitachi/Renesas Super-H */
 #define EM_SPARCV9 43  /* SPARC version 9 */
 #define EM_IA_64   50  /* Intel IA-64 Processor */
 #define EM_AMD64   62  /* AMD64 architecture */
+#define EM_X86_64  EM_AMD64
 #define EM_VAX 75  /* DEC VAX */
 #define EM_AARCH64 183 /* ARM 64-bit architecture (AArch64) */
 
@@ -288,10 +290,18 @@ typedef struct {
 
 
 /* Section Attribute Flags - sh_flags */
-#define SHF_WRITE  0x1 /* Writable */
-#define SHF_ALLOC  0x2 /* occupies memory */
-#define SHF_EXECINSTR  0x4 /* executable */
-#define SHF_TLS0x400   /* thread local storage */
+#define SHF_WRITE  0x1 /* Writable */
+#define SHF_ALLOC  0x2 /* occupies memory */
+#define SHF_EXECINSTR  0x4 /* executable */
+#define SHF_MERGE  0x10/* may be merged */
+#define SHF_STRINGS0x20/* contains strings */
+#define SHF_INFO_LINK  0x40/* sh_info holds section index */
+#define SHF_LINK_ORDER 0x80/* ordering requirements */
+#define SHF_OS_NONCONFORMING   0x100   /* OS-specific processing required */
+#define SHF_GROUP  0x200   /* member of section group */
+#define SHF_TLS0x400   /* thread local storage */
+#define SHF_COMPRESSED 0x800   /* contains compressed data */
+#define SHF_MASKOS 0x0ff0  /* OS-specific semantics */
 #define SHF_MASKPROC   0xf000  /* reserved bits for processor */
/*  specific section attributes */
 
@@ -557,6 +567,11 @@ typedef struct {
Elf64_Half descsz;
Elf64_Half type;
 } Elf64_Note;
+
+/* Values for n_type. */
+#define NT_PRSTATUS1   /* Process status. */
+#define NT_FPREGSET2   /* Floating point registers. */
+#define NT_PRPSINFO3   /* Process state info. */
 
 /*
  * OpenBSD-specific core file information.

Re: [PATCH] tests for vmd config parsing

[Alexander Bluhm]

On Tue, Oct 10, 2017 at 10:57:22PM -0700, Mike Larkin wrote:
> On Tue, Oct 10, 2017 at 07:54:20PM -0700, Carlos Cardenas wrote:
> > This patch adds a set of tests for vmd config parsing.
> > 
> > Comments? Ok?
> > 
> 
> ok by me. I think bluhm@ also looked at this.
> 
> bluhm, ok to commit?
> 
> -ml

Committed, thanks for the test.  Passes on amd64 and i386.
Added to my daily test list.

bluhm

Re: [PATCH] VMD: Remove switch on reload prior to re-creating

[Carlos Cardenas]

On 10/10/17 23:31, Claudio Jeker wrote:
> On Tue, Oct 10, 2017 at 10:52:42PM -0700, Mike Larkin wrote:
>> On Wed, Oct 11, 2017 at 06:46:22AM +0200, Claudio Jeker wrote:
>>> On Tue, Oct 10, 2017 at 03:57:25PM -0700, Carlos Cardenas wrote:
 Destroy switch on `vmctl reload` to allow SIOCBRDGADD to succeed
 when creating new bridge and attaching interfaces to it.

 Comments? Ok?
>>>
>>> I don't think it is a good idea to destroy and recreate bridge interfaces
>>> on every reload. This changes the underlying network and may result in
>>> broken connections and network hickups. I would suggest that vmctl is
>>> instead checking which interfaces are on the bridge and calls SIOCBRDDEL
>>> if needed and skips the SIOCBRDGADD if not needed.
>>>
>>
>> you mean vmd, right?
>>
>> vmctl has no special privilege to do this.
> 
> yeah, sorry, was not fully awake yet. vmd needs to be smarter on config
> reload IMO. Destroying interfaces for no reason is a bad practice.
>

We can definitely make things smarter.  For my sake, I'm going to go through a
couple current scenarios to make sure I'm picking up what you're putting down.

1)  vm.conf has the following switch definition
switch "a" {
add vether0
}

On vmd startup, a bridge(4) is created (bridge0 at first) and vether0 is added
to it.  On reset/reload, any running vms are stopped and the switch is removed
prior to re-reading the config file and creating new bridges/vms.  This means,
the new bridge is now bridge1 and vether0 will be added.

Today, this scenario fails since vether0 was never removed from bridge0 to be
added to bridge1. (what the patch addresses).

2) vm.conf has the following definition
switch "a" {
interface "bridge0"
add vether0
}

Just like 1) except this time after any reset/reload, the switch name is
always bridge0.  This seems to be way a switch(4) is used in vmd.

In scenario 1, are you proposing we keep bridge0 (and other predecessors around)
and only remove/add the bridge ports (vether0 in this case) to the current
bridge{n}?  If so, should we use the bridge ports defined in vm.conf or from
SIOCBRDGIFS since they could be different?

How should we handle scenario 2 since my patch is kinda heavy handed for it?

Another option would be to make "interface NAME" mandatory in switch definitions
which collapses our scenarios.

What are y'all's thoughts?


+--+
Carlos
  
>>>  
 diff --git usr.sbin/vmd/priv.c usr.sbin/vmd/priv.c
 index ef42549d105..0190c049837 100644
 --- usr.sbin/vmd/priv.c
 +++ usr.sbin/vmd/priv.c
 @@ -88,6 +88,7 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, 
 struct imsg *imsg)
switch (imsg->hdr.type) {
case IMSG_VMDOP_PRIV_IFDESCR:
case IMSG_VMDOP_PRIV_IFCREATE:
 +  case IMSG_VMDOP_PRIV_IFDESTROY:
case IMSG_VMDOP_PRIV_IFRDOMAIN:
case IMSG_VMDOP_PRIV_IFADD:
case IMSG_VMDOP_PRIV_IFUP:
 @@ -125,6 +126,13 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, 
 struct imsg *imsg)
errno != EEXIST)
log_warn("SIOCIFCREATE");
break;
 +  case IMSG_VMDOP_PRIV_IFDESTROY:
 +  /* Destroy the bridge */
 +  strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
 +  if (ioctl(env->vmd_fd, SIOCIFDESTROY, ) < 0 &&
 +  errno != EEXIST)
 +  log_warn("SIOCIFDESTROY");
 +  break;
case IMSG_VMDOP_PRIV_IFRDOMAIN:
strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
ifr.ifr_rdomainid = vfr.vfr_id;
 @@ -444,6 +452,23 @@ vm_priv_brconfig(struct privsep *ps, struct 
 vmd_switch *vsw)
return (0);
  }
  
 +int
 +vm_priv_brdestroy(struct privsep *ps, struct vmd_switch *vsw)
 +{
 +  struct vmop_ifreqvfr;
 +
 +  memset(, 0, sizeof(vfr));
 +
 +  if (strlcpy(vfr.vfr_name, vsw->sw_ifname,
 +  sizeof(vfr.vfr_name)) >= sizeof(vfr.vfr_name))
 +  return (-1);
 +
 +  proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFDESTROY,
 +  , sizeof(vfr));
 +
 +  return (0);
 +}
 +
  uint32_t
  vm_priv_addr(struct address *h, uint32_t vmid, int idx, int isvm)
  {
 diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
 index f1abc54d9a3..834654a76de 100644
 --- usr.sbin/vmd/vmd.c
 +++ usr.sbin/vmd/vmd.c
 @@ -852,7 +852,7 @@ int
  vmd_reload(unsigned int reset, const char *filename)
  {
struct vmd_vm   *vm, *next_vm;
 -  struct vmd_switch   *vsw;
 +  struct vmd_switch   *vsw, *next_vsw;
int  reload = 0;
  
/* Switch back to the default config file */
 @@ -885,6 +885,19 @@ vmd_reload(unsigned int reset, const char *filename)
}
}
  
 +  TAILQ_FOREACH_SAFE(vsw, 

Re: LLVM 5.0 on armv7

[Patrick Wildt]

On Wed, Oct 11, 2017 at 12:19:47PM +0200, Mark Kettenis wrote:
> Diff below makes clang and lld build again on armv7.  Sorted the files
> such that they match the order in the CMakeLists.txt file.

The lists are explicitly sorted alphabetically and not like in the
CMakeLists.  I would actually like to keep it that way.

If there are unsorted lists, then that might be because I didn't have
to touch them since the last update. :)

> ok?
> 
> 
> Index: gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile
> ===
> RCS file: /cvs/src/gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile,v
> retrieving revision 1.3
> diff -u -p -r1.3 Makefile
> --- gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile  9 Jul 2017 15:28:34 
> -   1.3
> +++ gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile  11 Oct 2017 10:17:13 
> -
> @@ -11,7 +11,6 @@ SRCS=   A15SDOptimizer.cpp \
>   ARMAsmPrinter.cpp \
>   ARMBaseInstrInfo.cpp \
>   ARMBaseRegisterInfo.cpp \
> - ARMComputeBlockSize.cpp \
>   ARMConstantIslandPass.cpp \
>   ARMConstantPoolValue.cpp \
>   ARMExpandPseudoInsts.cpp \
> @@ -24,6 +23,7 @@ SRCS=   A15SDOptimizer.cpp \
>   ARMLoadStoreOptimizer.cpp \
>   ARMMCInstLower.cpp \
>   ARMMachineFunctionInfo.cpp \
> + ARMMacroFusion.cpp \
>   ARMRegisterInfo.cpp \
>   ARMOptimizeBarriersPass.cpp \
>   ARMSelectionDAGInfo.cpp \
> @@ -37,7 +37,8 @@ SRCS=   A15SDOptimizer.cpp \
>   ThumbRegisterInfo.cpp \
>   Thumb2ITBlockPass.cpp \
>   Thumb2InstrInfo.cpp \
> - Thumb2SizeReduction.cpp
> + Thumb2SizeReduction.cpp \
> + ARMComputeBlockSize.cpp
>  
>  .PATH:   ${.CURDIR}/../../../llvm/lib/Target/ARM
>  
> 

Re: ifioctl() cleanup, step 2

[Alexandr Nedvedicky]

On Wed, Oct 11, 2017 at 12:43:45PM +0200, Martin Pieuchot wrote:
> On 11/10/17(Wed) 12:28, Alexandr Nedvedicky wrote:
> > Hello,
> > 
> > just for my curiosity:
> > 
> > why do you leave some returns untreated? is that intentional?
> 
> Yes it is intentional to make the diff shorter and easier to review.
> 

O.K. understood.

thanks and
regards
sasha

Re: ifioctl() cleanup, step 2

[Martin Pieuchot]

On 11/10/17(Wed) 12:28, Alexandr Nedvedicky wrote:
> Hello,
> 
> just for my curiosity:
> 
> why do you leave some returns untreated? is that intentional?

Yes it is intentional to make the diff shorter and easier to review.

> just like here:
> @@ -2048,8 +2048,11 @@ ifioctl(struct socket *so, u_long cmd, caddr_t 
> data, struct proc *p)
>   case SIOCGVNETID:
>   case SIOCGIFPAIR:
>   case SIOCGIFPARENT:
> -   if (ifp->if_ioctl == 0)
> -   return (EOPNOTSUPP);
> +   if (ifp->if_ioctl == 0) {
> +   /* ??? sashan@ */
> +   error = EOPNOTSUPP;
> +   break;
> +   }
>   error = (*ifp->if_ioctl)(ifp, cmd, data);
>   break;

This will be addressed in the next diff.  We can simply remove the if ()
block because all interface MUST have an if_ioctl handler.

Re: ifioctl() cleanup, step 2

[Alexandr Nedvedicky]

Hello,

just for my curiosity:

why do you leave some returns untreated? is that intentional?
just like here:
@@ -2048,8 +2048,11 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, 
struct proc *p)
case SIOCGVNETID:
case SIOCGIFPAIR:
case SIOCGIFPARENT:
-   if (ifp->if_ioctl == 0)
-   return (EOPNOTSUPP);
+   if (ifp->if_ioctl == 0) {
+   /* ??? sashan@ */
+   error = EOPNOTSUPP;
+   break;
+   }
error = (*ifp->if_ioctl)(ifp, cmd, data);
break;
 

below is patch, which makes switch in ifioctl() consistent by converting
all return (error) to breaks;

thanks and
regards
sasha

8<---8<---8<--8<
diff --git a/sys/net/if.c b/sys/net/if.c
index 60020606df5..40f6fe1b776 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -1876,7 +1876,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, 
struct proc *p)
break;
 #endif /* INET6 */
default:
-   return (EAFNOSUPPORT);
+   error = EAFNOSUPPORT;
}
if (oif_flags != ifp->if_flags || oif_xflags != ifp->if_xflags)
rtm_ifchg(ifp);
@@ -1920,7 +1920,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, 
struct proc *p)
 
case SIOCSIFFLAGS:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
 
ifp->if_flags = (ifp->if_flags & IFF_CANTCHANGE) |
(ifr->ifr_flags & ~IFF_CANTCHANGE);
@@ -1945,13 +1945,13 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, 
struct proc *p)
 
case SIOCSIFXFLAGS:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
 
 #ifdef INET6
if (ISSET(ifr->ifr_flags, IFXF_AUTOCONF6)) {
error = in6_ifattach(ifp);
if (error != 0)
-   return (error);
+   break;
}
 #endif /* INET6 */
 
@@ -1983,7 +1983,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, 
struct proc *p)
error = ifp->if_wol(ifp, 1);
splx(s);
if (error)
-   return (error);
+   break;
}
if (ISSET(ifp->if_xflags, IFXF_WOL) &&
!ISSET(ifr->ifr_flags, IFXF_WOL)) {
@@ -1992,7 +1992,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, 
struct proc *p)
error = ifp->if_wol(ifp, 0);
splx(s);
if (error)
-   return (error);
+   break;
}
} else if (ISSET(ifr->ifr_flags, IFXF_WOL)) {
ifr->ifr_flags &= ~IFXF_WOL;
@@ -2007,13 +2007,13 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, 
struct proc *p)
 
case SIOCSIFMETRIC:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
ifp->if_metric = ifr->ifr_metric;
break;
 
case SIOCSIFMTU:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
if (ifp->if_ioctl == NULL)
return (EOPNOTSUPP);
error = (*ifp->if_ioctl)(ifp, cmd, data);
@@ -2037,7 +2037,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, 
struct proc *p)
case SIOCSIFPARENT:
case SIOCDIFPARENT:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
/* FALLTHROUGH */
case SIOCGIFPSRCADDR:
case SIOCGIFPDSTADDR:
@@ -2048,8 +2048,10 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, 
struct proc *p)
case SIOCGVNETID:
case SIOCGIFPAIR:
case SIOCGIFPARENT:
-   if (ifp->if_ioctl == 0)
-   return (EOPNOTSUPP);
+   if (ifp->if_ioctl == 0) {
+   error = EOPNOTSUPP;
+   break;
+   }
error = (*ifp->if_ioctl)(ifp, cmd, data);
break;
 
@@ -2061,7 +2063,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, 
struct proc *p)
 
case SIOCSIFDESCR:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
error = 

LLVM 5.0 on armv7

[Mark Kettenis]

Diff below makes clang and lld build again on armv7.  Sorted the files
such that they match the order in the CMakeLists.txt file.

ok?


Index: gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile
===
RCS file: /cvs/src/gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile,v
retrieving revision 1.3
diff -u -p -r1.3 Makefile
--- gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile9 Jul 2017 15:28:34 
-   1.3
+++ gnu/usr.bin/clang/libLLVMARMCodeGen/Makefile11 Oct 2017 10:17:13 
-
@@ -11,7 +11,6 @@ SRCS= A15SDOptimizer.cpp \
ARMAsmPrinter.cpp \
ARMBaseInstrInfo.cpp \
ARMBaseRegisterInfo.cpp \
-   ARMComputeBlockSize.cpp \
ARMConstantIslandPass.cpp \
ARMConstantPoolValue.cpp \
ARMExpandPseudoInsts.cpp \
@@ -24,6 +23,7 @@ SRCS= A15SDOptimizer.cpp \
ARMLoadStoreOptimizer.cpp \
ARMMCInstLower.cpp \
ARMMachineFunctionInfo.cpp \
+   ARMMacroFusion.cpp \
ARMRegisterInfo.cpp \
ARMOptimizeBarriersPass.cpp \
ARMSelectionDAGInfo.cpp \
@@ -37,7 +37,8 @@ SRCS= A15SDOptimizer.cpp \
ThumbRegisterInfo.cpp \
Thumb2ITBlockPass.cpp \
Thumb2InstrInfo.cpp \
-   Thumb2SizeReduction.cpp
+   Thumb2SizeReduction.cpp \
+   ARMComputeBlockSize.cpp
 
 .PATH: ${.CURDIR}/../../../llvm/lib/Target/ARM
 

Re: inteldrm: reduce i2c busy-wait to 100us

[Mark Kettenis]

> Date: Tue, 10 Oct 2017 15:42:26 -0500
> From: joshua stein 
> 
> On my Kaby Lake laptop, running xbacklight or xrandr causes the
> audio and mouse cursor to pause briefly.  This is because those
> codepaths do DRM operations that have to probe all of the available
> connectors, even disconnected ones.  For HDMI, it does i2c
> operations that have to timeout.
> 
> Reducing the DELAY() calls to 100us avoids this, while still making
> HDMI output work when it's actually connected.
> 
> intel_gmbus_exec appears to be an OpenBSD-specific function.

This will need careful testing on older hardware, especially with
multi-screen desktop setups.

> Index: sys/dev/pci/drm/i915/intel_i2c.c
> ===
> RCS file: /cvs/src/sys/dev/pci/drm/i915/intel_i2c.c,v
> retrieving revision 1.12
> diff -u -p -u -p -r1.12 intel_i2c.c
> --- sys/dev/pci/drm/i915/intel_i2c.c  30 Sep 2017 07:36:56 -  1.12
> +++ sys/dev/pci/drm/i915/intel_i2c.c  10 Oct 2017 20:32:59 -
> @@ -231,7 +231,7 @@ intel_gmbus_exec(void *cookie, i2c_op_t 
>   st = I915_READ(GMBUS2);
>   if (st & (GMBUS_SATOER | GMBUS_HW_RDY))
>   break;
> - DELAY(1000);
> + DELAY(100);
>   }
>   if (st & GMBUS_SATOER) {
>   bus_err = 1;
> @@ -254,7 +254,7 @@ intel_gmbus_exec(void *cookie, i2c_op_t 
>   st = I915_READ(GMBUS2);
>   if (st & (GMBUS_SATOER | GMBUS_HW_RDY))
>   break;
> - DELAY(1000);
> + DELAY(100);
>   }
>   if (st & GMBUS_SATOER) {
>   bus_err = 1;
> @@ -279,7 +279,7 @@ out:
>   bus_err = 1;
>   if ((st & GMBUS_ACTIVE) == 0)
>   break;
> - DELAY(1000);
> + DELAY(100);
>   }
>   if (st & GMBUS_ACTIVE)
>   return (ETIMEDOUT);
> 
> 

ifioctl() cleanup, step 2

[Martin Pieuchot]

Moar cleanup to be able to selectively take the NET_LOCK() around some
ioctls.

This diff change many "return (error)" into "break".

It adds error checks for SIOC{A,D}IFGROUP.  The only driver handling
these ioctl(2)s is... carp(4)!

ok?

Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.514
diff -u -p -r1.514 if.c
--- net/if.c11 Oct 2017 07:57:27 -  1.514
+++ net/if.c11 Oct 2017 08:37:31 -
@@ -1817,7 +1817,7 @@ ifioctl(struct socket *so, u_long cmd, c
 {
struct ifnet *ifp;
struct ifreq *ifr = (struct ifreq *)data;
-   struct ifgroupreq *ifgr;
+   struct ifgroupreq *ifgr = (struct ifgroupreq *)data;
struct if_afreq *ifar = (struct if_afreq *)data;
char ifdescrbuf[IFDESCRSIZE];
char ifrtlabelbuf[RTLABEL_LEN];
@@ -1858,7 +1858,7 @@ ifioctl(struct socket *so, u_long cmd, c
case SIOCIFAFATTACH:
case SIOCIFAFDETACH:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
switch (ifar->ifar_af) {
case AF_INET:
/* attach is a noop for AF_INET */
@@ -1874,7 +1874,7 @@ ifioctl(struct socket *so, u_long cmd, c
break;
 #endif /* INET6 */
default:
-   return (EAFNOSUPPORT);
+   error = EAFNOSUPPORT;
}
break;
 
@@ -1909,7 +1909,7 @@ ifioctl(struct socket *so, u_long cmd, c
 
case SIOCSIFFLAGS:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
 
ifp->if_flags = (ifp->if_flags & IFF_CANTCHANGE) |
(ifr->ifr_flags & ~IFF_CANTCHANGE);
@@ -1934,13 +1934,13 @@ ifioctl(struct socket *so, u_long cmd, c
 
case SIOCSIFXFLAGS:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
 
 #ifdef INET6
if (ISSET(ifr->ifr_flags, IFXF_AUTOCONF6)) {
error = in6_ifattach(ifp);
if (error != 0)
-   return (error);
+   break;
}
 #endif /* INET6 */
 
@@ -1972,7 +1972,7 @@ ifioctl(struct socket *so, u_long cmd, c
error = ifp->if_wol(ifp, 1);
splx(s);
if (error)
-   return (error);
+   break;
}
if (ISSET(ifp->if_xflags, IFXF_WOL) &&
!ISSET(ifr->ifr_flags, IFXF_WOL)) {
@@ -1981,7 +1981,7 @@ ifioctl(struct socket *so, u_long cmd, c
error = ifp->if_wol(ifp, 0);
splx(s);
if (error)
-   return (error);
+   break;
}
} else if (ISSET(ifr->ifr_flags, IFXF_WOL)) {
ifr->ifr_flags &= ~IFXF_WOL;
@@ -1995,13 +1995,13 @@ ifioctl(struct socket *so, u_long cmd, c
 
case SIOCSIFMETRIC:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
ifp->if_metric = ifr->ifr_metric;
break;
 
case SIOCSIFMTU:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
if (ifp->if_ioctl == NULL)
return (EOPNOTSUPP);
error = (*ifp->if_ioctl)(ifp, cmd, data);
@@ -2049,7 +2049,7 @@ ifioctl(struct socket *so, u_long cmd, c
 
case SIOCSIFDESCR:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
error = copyinstr(ifr->ifr_data, ifdescrbuf,
IFDESCRSIZE, );
if (error == 0) {
@@ -2070,7 +2070,7 @@ ifioctl(struct socket *so, u_long cmd, c
 
case SIOCSIFRTLABEL:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
error = copyinstr(ifr->ifr_data, ifrtlabelbuf,
RTLABEL_LEN, );
if (error == 0) {
@@ -2085,7 +2085,7 @@ ifioctl(struct socket *so, u_long cmd, c
 
case SIOCSIFPRIORITY:
if ((error = suser(p, 0)) != 0)
-   return (error);
+   break;
if (ifr->ifr_metric < 0 || ifr->ifr_metric > 15)
return (EINVAL);
ifp->if_priority = ifr->ifr_metric;
@@ -2097,32 +2097,32 @@ ifioctl(struct socket *so, u_long cmd, c
 
  

acpi: free() size

[Anton Lindqvist]

Hi,
Add missing size to free(), tested on amd64.

Comments? OK?

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

Re: mg: fgetln -> getline

[Florian Obser]

On Sun, Sep 17, 2017 at 02:56:32AM +, Scott Cheloha wrote:
> Hi,
> 
> This will make mg(1) ever so slightly more portable: downstream
> will appreciate it.  And fgetln(3) was recently deprecated.
> 
> Misc. comments:
> 
> The comments about NUL and "the last line problem" aren't per se
> applicable with getline(3) so I removed them.
> 
> I will say that if ever a command does *not* emit a trailing
> newline that we will stub out the last byte, but I decided to take
> the comments' authors at their word and continued to assume the
> presence of a newline.  This can be rectified with:
> 
>   if (buf[len - 1] == '\n')
>   buf[len - 1] = '\0';

the diff reads fine to me, but code tends to get copied around,
can you send a diff with those lines added please? That's the
canonical way of using getline I believe and there is no good
reason for mg to be different, thanks!


> 
> The '};' in do_cscope() looked like a typo so I deleted the
> semicolon.
> 
> I also noticed that there were no error checks after the read loops
> so I added an echo print on ferror().  I don't know if this is
> sufficient, but we weren't doing anything before, so it's a start.
> 
> Thoughts?
> 
> --
> Scott Cheloha
> 
> Index: usr.bin/mg/cscope.c
> ===
> RCS file: /cvs/src/usr.bin/mg/cscope.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 cscope.c
> --- usr.bin/mg/cscope.c   19 Jan 2016 14:51:00 -  1.16
> +++ usr.bin/mg/cscope.c   16 Sep 2017 22:44:50 -
> @@ -166,9 +166,13 @@ cscreatelist(int f, int n)
>   struct stat sb;
>   FILE *fpipe;
>   char dir[NFILEN], cmd[BUFSIZ], title[BUFSIZ], *line, *bufp;
> - size_t len;
> + size_t sz;
> + ssize_t len;
>   int clen;
>  
> + line = NULL;
> + sz = 0;
> +
>   if (getbufcwd(dir, sizeof(dir)) == FALSE)
>   dir[0] = '\0';
>  
> @@ -221,11 +225,13 @@ cscreatelist(int f, int n)
>   }
>   addline(bp, title);
>   addline(bp, "");
> - /* All lines are NUL terminated */
> - while ((line = fgetln(fpipe, )) != NULL) {
> + while ((len = getline(, , fpipe)) != -1) {
>   line[len - 1] = '\0';
>   addline(bp, line);
>   }
> + free(line);
> + if (ferror(fpipe))
> + ewprintf("Problem reading pipe");
>   pclose(fpipe);
>   return (popbuftop(bp, WNONE));
>  }
> @@ -397,7 +403,11 @@ do_cscope(int i)
>   char pattern[MAX_TOKEN], cmd[BUFSIZ], title[BUFSIZ];
>   char *p, *buf;
>   int clen, nores = 0;
> - size_t len;
> + size_t sz;
> + ssize_t len;
> +
> + buf = NULL;
> + sz = 0;
>  
>   /* If current buffer isn't a source file just return */
>   if (fnmatch("*.[chy]", curbp->b_fname, 0) != 0) {
> @@ -447,13 +457,17 @@ do_cscope(int i)
>   addline(bp, title);
>   addline(bp, "");
>   addline(bp, 
> "---");
> - /* All lines are NUL terminated */
> - while ((buf = fgetln(fpipe, )) != NULL) {
> + while ((len = getline(, , fpipe)) != -1) {
>   buf[len - 1] = '\0';
> - if (addentry(bp, buf) != TRUE)
> + if (addentry(bp, buf) != TRUE) {
> + free(buf);
>   return (FALSE);
> + }
>   nores = 1;
> - };
> + }
> + free(buf);
> + if (ferror(fpipe))
> + ewprintf("Problem reading pipe");
>   pclose(fpipe);
>   addline(bp, 
> "---");
>   if (nores == 0)
> Index: usr.bin/mg/grep.c
> ===
> RCS file: /cvs/src/usr.bin/mg/grep.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 grep.c
> --- usr.bin/mg/grep.c 19 Mar 2015 21:48:05 -  1.44
> +++ usr.bin/mg/grep.c 16 Sep 2017 22:44:50 -
> @@ -178,12 +178,16 @@ compile_mode(const char *name, const cha
>   struct buffer   *bp;
>   FILE*fpipe;
>   char*buf;
> - size_t   len;
> + size_t   sz;
> + ssize_t  len;
>   int  ret, n;
>   char cwd[NFILEN], qcmd[NFILEN];
>   char timestr[NTIME];
>   time_t   t;
>  
> + buf = NULL;
> + sz = 0;
> +
>   n = snprintf(qcmd, sizeof(qcmd), "%s 2>&1", command);
>   if (n < 0 || n >= sizeof(qcmd))
>   return (NULL);
> @@ -210,15 +214,13 @@ compile_mode(const char *name, const cha
>   ewprintf("Problem opening pipe");
>   return (NULL);
>   }
> - /*
> -  * We know that our commands are nice and the last line will end with
> -  * a \n, so we don't need to try to deal with the last line problem
> -  * in fgetln.
> -  */
> - while ((buf = fgetln(fpipe, )) != NULL) {
> + while ((len = getline(, , fpipe)) != -1) {
>   buf[len - 1] = '\0';
>   

Re: [PATCH] VMD: Remove switch on reload prior to re-creating

[Claudio Jeker]

On Tue, Oct 10, 2017 at 10:52:42PM -0700, Mike Larkin wrote:
> On Wed, Oct 11, 2017 at 06:46:22AM +0200, Claudio Jeker wrote:
> > On Tue, Oct 10, 2017 at 03:57:25PM -0700, Carlos Cardenas wrote:
> > > Destroy switch on `vmctl reload` to allow SIOCBRDGADD to succeed
> > > when creating new bridge and attaching interfaces to it.
> > > 
> > > Comments? Ok?
> > 
> > I don't think it is a good idea to destroy and recreate bridge interfaces
> > on every reload. This changes the underlying network and may result in
> > broken connections and network hickups. I would suggest that vmctl is
> > instead checking which interfaces are on the bridge and calls SIOCBRDDEL
> > if needed and skips the SIOCBRDGADD if not needed.
> > 
> 
> you mean vmd, right?
> 
> vmctl has no special privilege to do this.

yeah, sorry, was not fully awake yet. vmd needs to be smarter on config
reload IMO. Destroying interfaces for no reason is a bad practice.
 
> >  
> > > diff --git usr.sbin/vmd/priv.c usr.sbin/vmd/priv.c
> > > index ef42549d105..0190c049837 100644
> > > --- usr.sbin/vmd/priv.c
> > > +++ usr.sbin/vmd/priv.c
> > > @@ -88,6 +88,7 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, 
> > > struct imsg *imsg)
> > >   switch (imsg->hdr.type) {
> > >   case IMSG_VMDOP_PRIV_IFDESCR:
> > >   case IMSG_VMDOP_PRIV_IFCREATE:
> > > + case IMSG_VMDOP_PRIV_IFDESTROY:
> > >   case IMSG_VMDOP_PRIV_IFRDOMAIN:
> > >   case IMSG_VMDOP_PRIV_IFADD:
> > >   case IMSG_VMDOP_PRIV_IFUP:
> > > @@ -125,6 +126,13 @@ priv_dispatch_parent(int fd, struct privsep_proc *p, 
> > > struct imsg *imsg)
> > >   errno != EEXIST)
> > >   log_warn("SIOCIFCREATE");
> > >   break;
> > > + case IMSG_VMDOP_PRIV_IFDESTROY:
> > > + /* Destroy the bridge */
> > > + strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
> > > + if (ioctl(env->vmd_fd, SIOCIFDESTROY, ) < 0 &&
> > > + errno != EEXIST)
> > > + log_warn("SIOCIFDESTROY");
> > > + break;
> > >   case IMSG_VMDOP_PRIV_IFRDOMAIN:
> > >   strlcpy(ifr.ifr_name, vfr.vfr_name, sizeof(ifr.ifr_name));
> > >   ifr.ifr_rdomainid = vfr.vfr_id;
> > > @@ -444,6 +452,23 @@ vm_priv_brconfig(struct privsep *ps, struct 
> > > vmd_switch *vsw)
> > >   return (0);
> > >  }
> > >  
> > > +int
> > > +vm_priv_brdestroy(struct privsep *ps, struct vmd_switch *vsw)
> > > +{
> > > + struct vmop_ifreqvfr;
> > > +
> > > + memset(, 0, sizeof(vfr));
> > > +
> > > + if (strlcpy(vfr.vfr_name, vsw->sw_ifname,
> > > + sizeof(vfr.vfr_name)) >= sizeof(vfr.vfr_name))
> > > + return (-1);
> > > +
> > > + proc_compose(ps, PROC_PRIV, IMSG_VMDOP_PRIV_IFDESTROY,
> > > + , sizeof(vfr));
> > > +
> > > + return (0);
> > > +}
> > > +
> > >  uint32_t
> > >  vm_priv_addr(struct address *h, uint32_t vmid, int idx, int isvm)
> > >  {
> > > diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
> > > index f1abc54d9a3..834654a76de 100644
> > > --- usr.sbin/vmd/vmd.c
> > > +++ usr.sbin/vmd/vmd.c
> > > @@ -852,7 +852,7 @@ int
> > >  vmd_reload(unsigned int reset, const char *filename)
> > >  {
> > >   struct vmd_vm   *vm, *next_vm;
> > > - struct vmd_switch   *vsw;
> > > + struct vmd_switch   *vsw, *next_vsw;
> > >   int  reload = 0;
> > >  
> > >   /* Switch back to the default config file */
> > > @@ -885,6 +885,19 @@ vmd_reload(unsigned int reset, const char *filename)
> > >   }
> > >   }
> > >  
> > > + TAILQ_FOREACH_SAFE(vsw, env->vmd_switches,
> > > + sw_entry, next_vsw) {
> > > + log_debug("%s: removing %s",
> > > + __func__, vsw->sw_ifname);
> > > + if (vm_priv_brdestroy(>vmd_ps,
> > > + vsw) == -1 ) {
> > > + log_warn("%s: failed to destroy switch",
> > > + __func__);
> > > + }
> > > + TAILQ_REMOVE(env->vmd_switches, vsw, sw_entry);
> > > + free(vsw);
> > > + }
> > > +
> > >   /* Update shared global configuration in all children */
> > >   if (config_setconfig(env) == -1)
> > >   return (-1);
> > > diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> > > index 4b7b5f70495..fce5fcaaa60 100644
> > > --- usr.sbin/vmd/vmd.h
> > > +++ usr.sbin/vmd/vmd.h
> > > @@ -100,6 +100,7 @@ enum imsg_type {
> > >   IMSG_VMDOP_PRIV_IFGROUP,
> > >   IMSG_VMDOP_PRIV_IFADDR,
> > >   IMSG_VMDOP_PRIV_IFRDOMAIN,
> > > + IMSG_VMDOP_PRIV_IFDESTROY,
> > >   IMSG_VMDOP_VM_SHUTDOWN,
> > >   IMSG_VMDOP_VM_REBOOT,
> > >   IMSG_VMDOP_CONFIG
> > > @@ -323,6 +324,7 @@ intpriv_findname(const char *, const char **);
> > >  int   priv_validgroup(const char *);
> > >  int   vm_priv_ifconfig(struct privsep *, struct vmd_vm *);

Re: [PATCH] tests for vmd config parsing

[Mike Larkin]

On Tue, Oct 10, 2017 at 07:54:20PM -0700, Carlos Cardenas wrote:
> This patch adds a set of tests for vmd config parsing.
> 
> Comments? Ok?
> 

ok by me. I think bluhm@ also looked at this.

bluhm, ok to commit?

-ml

> 
> diff --git regress/usr.sbin/Makefile regress/usr.sbin/Makefile
> index 3912e794d4d..f19a656d45e 100644
> --- regress/usr.sbin/Makefile
> +++ regress/usr.sbin/Makefile
> @@ -12,6 +12,7 @@ SUBDIR += relayd
>  SUBDIR += snmpd
>  SUBDIR += switchd
>  SUBDIR += syslogd
> +SUBDIR += vmd
>  
>  .if defined(REGRESS_FULL) || make(clean) || make(cleandir) || make(obj)
>  SUBDIR += pkg_add
> diff --git regress/usr.sbin/vmd/Makefile regress/usr.sbin/vmd/Makefile
> new file mode 100644
> index 000..6c6671ada3f
> --- /dev/null
> +++ regress/usr.sbin/vmd/Makefile
> @@ -0,0 +1,5 @@
> +#$OpenBSD$
> +
> +SUBDIR += config
> +
> +.include 
> diff --git regress/usr.sbin/vmd/config/Makefile 
> regress/usr.sbin/vmd/config/Makefile
> new file mode 100644
> index 000..f5f58658af6
> --- /dev/null
> +++ regress/usr.sbin/vmd/config/Makefile
> @@ -0,0 +1,32 @@
> +#$OpenBSD$
> +
> +VMD ?= /usr/sbin/vmd
> +
> +VMD_PASS=boot-keyword memory-round memory-just-enough
> +VMD_FAIL=kernel-keyword too-few-ram vm-name-too-long too-many-ifs \
> +  boot-name-too-long disk-path-too-long too-many-disks
> +
> +REGRESS_TARGETS=
> +
> +.for n in ${VMD_PASS}
> +REGRESS_TARGETS +=   vmd-pass-${n}
> +
> +vmd-pass-${n}:
> + @echo ' $@ '
> + ${VMD} -n -f ${.CURDIR}/vmd-pass-${n}.conf 2>&1 | \
> + diff -u ${.CURDIR}/vmd-pass-${n}.ok /dev/stdin
> +.endfor
> +
> +.for n in ${VMD_FAIL}
> +REGRESS_TARGETS +=   vmd-fail-${n}
> +
> +vmd-fail-${n}:
> + @echo ' $@ '
> + ${VMD} -n -f ${.CURDIR}/vmd-fail-${n}.conf 2>&1 | \
> + cut -d : -f 2,3,4 | \
> + diff -u ${.CURDIR}/vmd-fail-${n}.ok /dev/stdin
> +.endfor
> +
> +.PHONY: ${REGRESS_TARGETS}
> +
> +.include 
> diff --git regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.conf 
> regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.conf
> new file mode 100644
> index 000..bc569a9119e
> --- /dev/null
> +++ regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.conf
> @@ -0,0 +1,6 @@
> +#$OpenBSD$
> +# Fail on boot path (> 128)
> +ramdisk="/some/absolutepath/somewhere/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/bsd.rd"
> +vm "x" {
> +boot $ramdisk
> +}
> diff --git regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.ok 
> regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.ok
> new file mode 100644
> index 000..56cb73b98cf
> --- /dev/null
> +++ regress/usr.sbin/vmd/config/vmd-fail-boot-name-too-long.ok
> @@ -0,0 +1 @@
> +5: kernel name too long
> diff --git regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.conf 
> regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.conf
> new file mode 100644
> index 000..b70c3acf507
> --- /dev/null
> +++ regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.conf
> @@ -0,0 +1,6 @@
> +#$OpenBSD$
> +# Fail on disk path (> 128)
> +rdisk="/some/absolutepath/somewhere/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/bsd.img"
> +vm "x" {
> +disk $rdisk
> +}
> diff --git regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.ok 
> regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.ok
> new file mode 100644
> index 000..a384c812362
> --- /dev/null
> +++ regress/usr.sbin/vmd/config/vmd-fail-disk-path-too-long.ok
> @@ -0,0 +1,2 @@
> +disk path too long
> +5: failed to parse disks: 
> /some/absolutepath/somewhere/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/abcdefghijklmnopqrstuvwxyz0123456789/bsd.img
> diff --git regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.conf 
> regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.conf
> new file mode 100644
> index 000..3505def581b
> --- /dev/null
> +++ regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.conf
> @@ -0,0 +1,12 @@
> +#$OpenBSD$
> +# Fail on kernel keyword; has been replaced by boot.
> +ramdisk="/bsd.rd"
> +switch "sw" {
> +add vether0
> +}
> +vm "x" {
> +kernel $ramdisk
> +memory 1G
> +disable
> +interface { switch "sw" }
> +}
> diff --git regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.ok 
> regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.ok
> new file mode 100644
> index 000..348817b1477
> --- /dev/null
> +++ regress/usr.sbin/vmd/config/vmd-fail-kernel-keyword.ok
> @@ -0,0 +1 @@
> +8: syntax error
> diff --git regress/usr.sbin/vmd/config/vmd-fail-too-few-ram.conf 
> regress/usr.sbin/vmd/config/vmd-fail-too-few-ram.conf
> new file mode 100644
> index 000..f8b27056dea
> --- /dev/null
> +++ regress/usr.sbin/vmd/config/vmd-fail-too-few-ram.conf
> @@ -0,0 +1,5 @@
> +#$OpenBSD$
> +# Fail on memory (less than 1MB)
> +vm "x" {
> +