Re: Skylake m5/m7 Xorg to black screen after Jul 10 snapshot

[Bryan Vyhmeister]

On Mon, Jul 24, 2017 at 10:20:22PM +0200, Mark Kettenis wrote:
> > Date: Mon, 24 Jul 2017 12:55:25 -0700
> > From: Bryan Vyhmeister 
> > 
> > On Mon, Jul 24, 2017 at 09:42:41PM +0200, Mark Kettenis wrote:
> > > > From: Bryan Vyhmeister 
> > > > 
> > > > Running wsconsctl display.brightness also hangs. I have machdep.kbdreset
> > > > enabled in sysctl.conf so a Ctrl+Alt+Del does restart the machine.
> > > 
> > > That is worrying.  When that happens, can you log in remotely and send
> > > me the output of "ps AHuxl"?  If not, can you send me that same output
> > > after the machine has been up for a bit?
> > 
> > I can't kill the process without doing a reboot. I can still use the
> > machine but wherever I ran wsconsctl stays hung. Here is the output of
> > ps AHuxl.
> 
> So here is your problem:
> 
> > bcv  42682  0.0  0.0   272   220 p5  D+12:51PM0:00.00 wsconsctl 
> > displa  1000 40999   0  10   0 acpilk 
> 
> Something else is holding on to the acpi lock and blocking wsconsctl
> from reading the backlight state.  Bring this to the attentian of the
> acpi hackers on tech@.
> 
Thanks for your help. Those of you that are familiar with ACPI in
OpenBSD have any idea what the issue might be? This seems to be an ACPI
on both the HP EliteBook Folio G1 and Dell Latitude 13 7370 which both
have Skylake Core m processors. The HP has a Core m5-6Y54 and the Dell
has a Core m7-6Y75. If I run wsconsctl on either machine, it hangs as
can be seen in the above output from ps where wsconsctl is stuck in
acpilk.

My acpidump and so forth is in the bug report about the issues on bugs@
with the same title. Is there other information I can provide to help
figure this out?

Bryan

Re: kill: Use __dead, declare functions static

[Klemens Nanni]

On Mon, Jul 24, 2017 at 04:17:46PM -0600, Theo de Raadt wrote:
> > I know, this sounds silly. Isn't bringing the code up to modern standards 
> > part
> > of maintaining it?
> 
> I suppose that's my question:
> 
> What is it about __dead that makes it part of "modern standards", when it
> isn't dead, and an actual keyword.  __no_return isn't a standards mandated
> keyword either.
> 
> What makes this modern, if adoption if this is still MAYBE in the future?
> 
> In tiny programs like this, what's the gain?
> 
> I say zero, I think it is a waste of time, because this is all the
> lipstick.
Just consistency with other programs, style(9) conformance and
"lipstick" indeed.

> I do weird things like whitespace removals all the time, but the
> process is different.  I audit an entire program manually, cleaning it
> up and making annotations and notes as I go through, and then some of
> the changes are commited and many others are discarded.
> 
> Just saying that what I'm seeing here doesn't feel like the same
> valuable process.
That's where I still thought of these clean ups as worth to be
committed, I guess.

Thanks.

Re: kill: Use __dead, declare functions static

[Klemens Nanni]

On Mon, Jul 24, 2017 at 06:11:32PM -0400, Ted Unangst wrote:
> Klemens Nanni wrote:
> > usage() never returns, all functions are to be used within this unit
> > only.
> > 
> > Since changes are conflicting, I'll wait for this diff first, but I'd
> > like to remove the void casts for fprintf and use getprogname(3) over
> > __progname as well.
> > 
> > Feedback? Comments?
> 
> I enjoy doing these little cleanups myself, but usually (or always even) it's
> because I'm reading and studying the code for some other reason. There's not
> much benefit to going through all the code and making such changes
> mechanically. In the worst case, I think it makes old code look maintained
> when it's not. Having old code look old can be useful.
> 
> I know, this sounds silly. Isn't bringing the code up to modern standards part
> of maintaining it? Yes, that's true. I'm not sure there's a good explanation.
> But a diff to add __dead everywhere in usr.bin feels more like a just a fresh
> coat of paint than actual improvement.
This pretty much nails it, I'm convinced. Thank you.

Re: test: Add "<" and ">" to grammar comment, adjust alignment

[Jeremie Courreges-Anglas]

On Mon, Jul 24 2017, Klemens Nanni  wrote:
> Add missing operators and make the grammar more readable while using
> spaces and tabs consistently.
>
> Feedback? Comments?

I have added < and > but didn't touch anything else.  I believe that the
existing comment is readable ebnf (where ';' is meaningful).  If you want
to make more improvements, please leave whitespace issues aside.

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

Re: kill: Use __dead, declare functions static

[Theo de Raadt]

> > What is it about __dead that makes it part of "modern standards", when it
> > isn't dead, and an actual keyword.  __no_return isn't a standards mandated
> > keyword either.
> 
> The standard mandated spelling is _Noreturn  or noreturn with
> stdnoreturn.h. Just saying.

You are wrong.

OpenBSD compiles with older compilers also.

Why don't you go back to your oasis.

Re: kill: Use __dead, declare functions static

[Theo de Raadt]

> I know, this sounds silly. Isn't bringing the code up to modern standards part
> of maintaining it?

I suppose that's my question:

What is it about __dead that makes it part of "modern standards", when it
isn't dead, and an actual keyword.  __no_return isn't a standards mandated
keyword either.

What makes this modern, if adoption if this is still MAYBE in the future?

In tiny programs like this, what's the gain?

I say zero, I think it is a waste of time, because this is all the
lipstick.

I do weird things like whitespace removals all the time, but the
process is different.  I audit an entire program manually, cleaning it
up and making annotations and notes as I go through, and then some of
the changes are commited and many others are discarded.

Just saying that what I'm seeing here doesn't feel like the same
valuable process.

Re: kill: Use __dead, declare functions static

[Ted Unangst]

Klemens Nanni wrote:
> usage() never returns, all functions are to be used within this unit
> only.
> 
> Since changes are conflicting, I'll wait for this diff first, but I'd
> like to remove the void casts for fprintf and use getprogname(3) over
> __progname as well.
> 
> Feedback? Comments?

I enjoy doing these little cleanups myself, but usually (or always even) it's
because I'm reading and studying the code for some other reason. There's not
much benefit to going through all the code and making such changes
mechanically. In the worst case, I think it makes old code look maintained
when it's not. Having old code look old can be useful.

I know, this sounds silly. Isn't bringing the code up to modern standards part
of maintaining it? Yes, that's true. I'm not sure there's a good explanation.
But a diff to add __dead everywhere in usr.bin feels more like a just a fresh
coat of paint than actual improvement.

Re: kill: Use __dead, declare functions static

[Theo de Raadt]

No, I actually don't see the point.

It isn't a natural of C.

It isn't fixing any bugs.

What is it helping?  I don't see any help.  It looks like meaningless
churn.

> On Mon, Jul 24, 2017 at 03:27:05PM -0600, Theo de Raadt wrote:
> > this addiction to static is entirely pointless.
> Consider it a matter of taste and leave it out, then. I assume you're
> fine with __dead, though?
> 
> Index: kill.c
> ===
> RCS file: /cvs/src/bin/kill/kill.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 kill.c
> --- kill.c29 Mar 2017 22:40:15 -  1.14
> +++ kill.c24 Jul 2017 21:46:24 -
> @@ -42,10 +42,10 @@
>  
>  extern   char *__progname;
>  
> -void nosig(char *);
> +void __dead nosig(char *);
>  void printsignals(FILE *);
>  int signame_to_signum(char *);
> -void usage(void);
> +void __dead usage(void);
>  
>  int
>  main(int argc, char *argv[])
> @@ -148,7 +148,7 @@ signame_to_signum(char *sig)
>   return (-1);
>  }
>  
> -void
> +void __dead
>  nosig(char *name)
>  {
>  
> @@ -171,7 +171,7 @@ printsignals(FILE *fp)
>   }
>  }
>  
> -void
> +void __dead
>  usage(void)
>  {
>   (void)fprintf(stderr, "usage: %s [-s signal_name] pid ...\n",
> 

Re: kill: Use __dead, declare functions static

[Klemens Nanni]

On Mon, Jul 24, 2017 at 03:27:05PM -0600, Theo de Raadt wrote:
> this addiction to static is entirely pointless.
Consider it a matter of taste and leave it out, then. I assume you're
fine with __dead, though?

Index: kill.c
===
RCS file: /cvs/src/bin/kill/kill.c,v
retrieving revision 1.14
diff -u -p -r1.14 kill.c
--- kill.c  29 Mar 2017 22:40:15 -  1.14
+++ kill.c  24 Jul 2017 21:46:24 -
@@ -42,10 +42,10 @@
 
 extern char *__progname;
 
-void nosig(char *);
+void __dead nosig(char *);
 void printsignals(FILE *);
 int signame_to_signum(char *);
-void usage(void);
+void __dead usage(void);
 
 int
 main(int argc, char *argv[])
@@ -148,7 +148,7 @@ signame_to_signum(char *sig)
return (-1);
 }
 
-void
+void __dead
 nosig(char *name)
 {
 
@@ -171,7 +171,7 @@ printsignals(FILE *fp)
}
 }
 
-void
+void __dead
 usage(void)
 {
(void)fprintf(stderr, "usage: %s [-s signal_name] pid ...\n",

Re: kill: Use __dead, declare functions static

[Theo de Raadt]

this addiction to static is entirely pointless.

kill: Use __dead, declare functions static

[Klemens Nanni]

usage() never returns, all functions are to be used within this unit
only.

Since changes are conflicting, I'll wait for this diff first, but I'd
like to remove the void casts for fprintf and use getprogname(3) over
__progname as well.

Feedback? Comments?

Index: kill.c
===
RCS file: /cvs/src/bin/kill/kill.c,v
retrieving revision 1.14
diff -u -p -r1.14 kill.c
--- kill.c  29 Mar 2017 22:40:15 -  1.14
+++ kill.c  24 Jul 2017 21:03:03 -
@@ -42,10 +42,10 @@
 
 extern char *__progname;
 
-void nosig(char *);
-void printsignals(FILE *);
-int signame_to_signum(char *);
-void usage(void);
+static void __dead nosig(char *);
+static voidprintsignals(FILE *);
+static int signame_to_signum(char *);
+static void __dead usage(void);
 
 int
 main(int argc, char *argv[])
@@ -148,7 +148,7 @@ signame_to_signum(char *sig)
return (-1);
 }
 
-void
+static void __dead
 nosig(char *name)
 {
 
@@ -171,7 +171,7 @@ printsignals(FILE *fp)
}
 }
 
-void
+static void __dead
 usage(void)
 {
(void)fprintf(stderr, "usage: %s [-s signal_name] pid ...\n",

armv7 more pliable boot? rootdev != bootdev

[Artturi Alm]

Hi,

i'm finding the boot as is too restricted, for no good reason, imo..
I'm not sure about what part of the bootstrap to blame this for, but
almost certain that making this better will require touching efiloader
too.

I was quite surprised to findout it didnt work when i tried to have my
cubie running so that root is on usb, even if booting does happen from
sd-card, which is somewhat required. I tried to set the "openbsd,bootduid"
in "/chosen"-node of the DT u-boot loaded for efi, but the efi does what
it does unconditionally not leaving very clean ways to work around.
Partly this could be solved via the bootargs, i guess, and i don't mind
even if installer wouldn't get fixed to support this better than it currently
does, as it does, in a way:)

so i'm wondering what is the right place to work this out? there's plenty
of reasons for ie. not running w/rootdev==bootdev.

-Artturi

Re: Skylake m5/m7 Xorg to black screen after Jul 10 snapshot

[Mark Kettenis]

> Date: Mon, 24 Jul 2017 12:55:25 -0700
> From: Bryan Vyhmeister 
> 
> On Mon, Jul 24, 2017 at 09:42:41PM +0200, Mark Kettenis wrote:
> > > From: Bryan Vyhmeister 
> > > 
> > > Running wsconsctl display.brightness also hangs. I have machdep.kbdreset
> > > enabled in sysctl.conf so a Ctrl+Alt+Del does restart the machine.
> > 
> > That is worrying.  When that happens, can you log in remotely and send
> > me the output of "ps AHuxl"?  If not, can you send me that same output
> > after the machine has been up for a bit?
> 
> I can't kill the process without doing a reboot. I can still use the
> machine but wherever I ran wsconsctl stays hung. Here is the output of
> ps AHuxl.

So here is your problem:

> bcv  42682  0.0  0.0   272   220 p5  D+12:51PM0:00.00 wsconsctl 
> displa  1000 40999   0  10   0 acpilk 

Something else is holding on to the acpi lock and blocking wsconsctl
from reading the backlight state.  Bring this to the attentian of the
acpi hackers on tech@.

pcxrtc(4)

[Mark Kettenis]

A driver for the NXP PCF8563 RTC.  It is similar to the pcfrtc(4)
driver, but the register layout and i2c protocol to read/write the
registers differ enough to warrant a separate driver.

ok?


Index: dev/i2c/files.i2c
===
RCS file: /cvs/src/sys/dev/i2c/files.i2c,v
retrieving revision 1.57
diff -u -p -r1.57 files.i2c
--- dev/i2c/files.i2c   1 Sep 2016 10:04:51 -   1.57
+++ dev/i2c/files.i2c   24 Jul 2017 19:20:38 -
@@ -133,6 +133,11 @@ device  pcfrtc
 attach pcfrtc at i2c
 file   dev/i2c/pcf8523.c   pcfrtc
 
+# NXP PCF8563 Real Time Clock
+device  pcxrtc
+attach pcxrtc at i2c
+file   dev/i2c/pcf8563.c   pcxrtc
+
 # RICOH RS5C372[AB] Real Time Clock
 device ricohrtc
 attach ricohrtc at i2c
Index: dev/i2c/pcf8563.c
===
RCS file: dev/i2c/pcf8563.c
diff -N dev/i2c/pcf8563.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ dev/i2c/pcf8563.c   24 Jul 2017 19:20:38 -
@@ -0,0 +1,286 @@
+/* $OpenBSD$   */
+
+/*
+ * Copyright (c) 2005 Kimihiro Nonaka
+ * Copyright (c) 2016, 2017 Mark Kettenis
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY WASABI SYSTEMS, INC. ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL WASABI SYSTEMS, INC
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+
+/*
+ * PCF8563 Real-Time Clock
+ */
+
+#define PCF8563_ADDR   0x51/* Fixed I2C Slave Address */
+
+#define PCF8563_CONTROL1   0x00
+#define PCF8563_CONTROL2   0x01
+#define PCF8563_SECONDS0x02
+#define PCF8563_MINUTES0x03
+#define PCF8563_HOURS  0x04
+#define PCF8563_DAY0x05
+#define PCF8563_WDAY   0x06
+#define PCF8563_MONTH  0x07
+#define PCF8563_YEAR   0x08
+
+#define PCF8563_NREGS  12
+#define PCF8563_NRTC_REGS  7
+
+/*
+ * Bit definitions.
+ */
+#define PCF8563_CONTROL1_TESTC (1 << 3)
+#define PCF8563_CONTROL1_STOP  (1 << 5)
+#define PCF8563_CONTROL1_TEST1 (1 << 1)
+#define PCF8563_SECONDS_MASK   0x7f
+#define PCF8563_SECONDS_VL (1 << 7)
+#define PCF8563_MINUTES_MASK   0x7f
+#define PCF8563_HOURS_MASK 0x3f
+#define PCF8563_DAY_MASK   0x3f
+#define PCF8563_WDAY_MASK  0x07
+#define PCF8563_MONTH_MASK 0x0f
+#define PCF8563_MONTH_C(1 << 7)
+
+struct pcxrtc_softc {
+   struct device sc_dev;
+   i2c_tag_t sc_tag;
+   int sc_address;
+   struct todr_chip_handle sc_todr;
+};
+
+int pcxrtc_match(struct device *, void *, void *);
+void pcxrtc_attach(struct device *, struct device *, void *);
+
+struct cfattach pcxrtc_ca = {
+   sizeof(struct pcxrtc_softc), pcxrtc_match, pcxrtc_attach
+};
+
+struct cfdriver pcxrtc_cd = {
+   NULL, "pcxrtc", DV_DULL
+};
+
+uint8_t pcxrtc_reg_read(struct pcxrtc_softc *, int);
+void pcxrtc_reg_write(struct pcxrtc_softc *, int, uint8_t);
+int pcxrtc_clock_read(struct pcxrtc_softc *, struct clock_ymdhms *);
+int pcxrtc_clock_write(struct pcxrtc_softc *, struct clock_ymdhms *);
+int pcxrtc_gettime(struct todr_chip_handle *, struct timeval *);
+int pcxrtc_settime(struct todr_chip_handle *, struct timeval *);
+int pcxrtc_getcal(struct todr_chip_handle *, int *);
+int pcxrtc_setcal(struct todr_chip_handle *, int);
+
+int
+pcxrtc_match(struct device *parent, void *v, void *arg)
+{
+   struct i2c_attach_args *ia = arg;
+
+   if (strcmp(ia->ia_name, "nxp,pcf8563") == 0 &&
+   ia->ia_addr == PCF8563_ADDR)
+   return (1);
+
+   return (0);
+}
+
+void
+pcxrtc_attach(struct device *parent, struct device *self, void *arg)
+{
+   struct pcxrtc_softc *sc = (struct pcxrtc_softc *)self;
+   struct i2c_attach_args 

Re: pflogd: cope with interface departure

[Sebastian Benoit]

Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2017.07.22 21:40:53 +0200:
> 
> If you destroy the interface pflogd(8) listens on, you get killed
> because socket(2) is denied by the current pledge(2) restrictions:
> 
>   pflogd(15868): syscall 97 "inet"
> 
> The ioctl(SIOCGIFDATA) call would be fatal too.
> 
> The diff below just uses if_nametoindex(3), which is always allowed.
> The if_exists() function is then so simple that it could be deleted.
> 
> # ./obj/pflogd -s 160 -D -i pflog1
> [priv]: msg PRIV_OPEN_LOG received
> interface pflog1 went away
> Exiting
> 
> Opinions / ok?

ok

> Index: pflogd.c
> ===
> RCS file: /d/cvs/src/sbin/pflogd/pflogd.c,v
> retrieving revision 1.53
> diff -u -p -p -u -r1.53 pflogd.c
> --- pflogd.c  16 Jan 2016 03:17:48 -  1.53
> +++ pflogd.c  22 Jul 2017 19:28:21 -
> @@ -194,23 +194,7 @@ set_pcap_filter(void)
>  int
>  if_exists(char *ifname)
>  {
> - int s, ret = 1;
> - struct ifreq ifr;
> - struct if_data ifrdat;
> -
> - if ((s = socket(AF_INET, SOCK_DGRAM, 0)) == -1)
> - err(1, "socket");
> - bzero(, sizeof(ifr));
> - if (strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name)) >=
> - sizeof(ifr.ifr_name))
> - errx(1, "main ifr_name: strlcpy");
> - ifr.ifr_data = (caddr_t)
> - if (ioctl(s, SIOCGIFDATA, (caddr_t)) == -1)
> - ret = 0;
> - if (close(s))
> - err(1, "close");
> -
> - return (ret);
> + return (if_nametoindex(ifname) != 0);
>  }
>  
>  int
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

Re: test: Add "<" and ">" to grammar comment, adjust alignment

[Klemens Nanni]

On Mon, Jul 24, 2017 at 08:31:26PM +0200, Klemens Nanni wrote:
> Add missing operators and make the grammar more readable while using
> spaces and tabs consistently.
Missing bits: operands can be more than "any legacy UNIX file name" of
course, correct that as well. One might just say  or 
but I think  nicely catches files and directories.

tech: use getprogname(3)

[Klemens Nanni]

As done in other places already, replace __progname with getprogname(3).

Feedback? Comments?

Index: test.c
===
RCS file: /cvs/src/bin/test/test.c,v
retrieving revision 1.17
diff -u -p -r1.17 test.c
--- test.c  21 Jan 2017 11:03:42 -  1.17
+++ test.c  24 Jul 2017 15:02:03 -
@@ -155,13 +154,12 @@ static __dead void syntax(const char *op
 int
 main(int argc, char *argv[])
 {
-   extern char *__progname;
-   int res;
+   int res;
 
if (pledge("stdio rpath", NULL) == -1)
err(2, "pledge");
 
-   if (strcmp(__progname, "[") == 0) {
+   if (strcmp(getprogname(), "[") == 0) {
if (strcmp(argv[--argc], "]"))
errx(2, "missing ]");
argv[argc] = NULL;

test: Add "<" and ">" to grammar comment, adjust alignment

[Klemens Nanni]

Add missing operators and make the grammar more readable while using
spaces and tabs consistently.

Feedback? Comments?

Index: test.c
===
RCS file: /cvs/src/bin/test/test.c,v
retrieving revision 1.17
diff -u -p -r1.17 test.c
--- test.c  21 Jan 2017 11:03:42 -  1.17
+++ test.c  24 Jul 2017 15:02:03 -
@@ -22,20 +23,18 @@
 #include 
 
 /* test(1) accepts the following grammar:
-   oexpr   ::= aexpr | aexpr "-o" oexpr ;
-   aexpr   ::= nexpr | nexpr "-a" aexpr ;
-   nexpr   ::= primary | "!" primary
-   primary ::= unary-operator operand
-   | operand binary-operator operand
-   | operand
-   | "(" oexpr ")"
-   ;
-   unary-operator ::= "-r"|"-w"|"-x"|"-f"|"-d"|"-c"|"-b"|"-p"|
-   "-u"|"-g"|"-k"|"-s"|"-t"|"-z"|"-n"|"-o"|"-O"|"-G"|"-L"|"-S";
-
-   binary-operator ::= "="|"!="|"-eq"|"-ne"|"-ge"|"-gt"|"-le"|"-lt"|
-   "-nt"|"-ot"|"-ef";
-   operand ::= 
+   oexpr   ::= aexpr | aexpr "-o" oexpr
+   aexpr   ::= nexpr | nexpr "-a" aexpr
+   nexpr   ::= primary | "!" primary
+   primary ::= unary-operator operand |
+   operand binary-operator operand |
+   operand |
+   "(" oexpr ")"
+   unary-operator  ::= "-r"|"-w"|"-x"|"-f"|"-d"|"-c"|"-b"|"-p"|"-u"|"-g"|
+   "-k"|"-s"|"-t"|"-z"|"-n"|"-o"|"-O"|"-G"|"-L"|"-S"
+   binary-operator ::= "="|"!="|"<"|">"|"-eq"|"-ne"|"-ge"|"-gt"|"-le"|
+   "-lt"|"-nt"|"-ot"|"-ef"
+   operand ::=  |  | 
 */
 
 enum token {

ksh: use RB trees as freelists

[Jeremie Courreges-Anglas]

This is not ripe yet, but I figured I'd share it.

Since some time I find ksh(1) slow at startup.  I use
HISTFILE=~/.ksh_history and HISTSIZE=2; currently my ksh_history is
24487 lines long.

Even if you're above HISTSIZE lines, ksh will avoid truncating your
HISTFILE until it has grown by ~25%.  The problem is that when you go
over HISTSIZE lines, the oldest lines in the history array are freed and
the whole array is moved.  Since those lines are oldest, afree() needs
to walk to the end of the freelist. history_load() calls histsave() and
thus afree() in a loop, which is kinda expensive.

Since afree() is the worst offender I thought about making it snappier
using a RB tree, which seems to solve the slowness here.  wip diff, use
at your own risk...


Index: alloc.c
===
RCS file: /d/cvs/src/bin/ksh/alloc.c,v
retrieving revision 1.16
diff -u -p -r1.16 alloc.c
--- alloc.c 29 May 2017 13:09:17 -  1.16
+++ alloc.c 24 Jul 2017 17:44:00 -
@@ -6,20 +6,35 @@
  * area-based allocation built on malloc/free
  */
 
+#include 
 #include 
 #include 
 
 #include "sh.h"
 
+/* The key is the address of the node */
 struct link {
-   struct link *prev;
-   struct link *next;
+   RB_ENTRY(link)  entry;
 };
 
+static int
+addrcmp(struct link *l1, struct link *l2)
+{
+   uintptr_t u1 = (uintptr_t)l1, u2 = (uintptr_t)l2;
+   return (u1 < u2 ? -1 : u1 > u2);
+}
+
+RB_HEAD(addrtree, link);
+RB_GENERATE_STATIC(addrtree, link, entry, addrcmp);
+
 Area *
 ainit(Area *ap)
 {
-   ap->freelist = NULL;
+   ap->tree = calloc(1, sizeof(*ap->tree));
+   if (ap->tree == NULL)
+   abort(); /* XXX */
+   RB_INIT(ap->tree);
+
return ap;
 }
 
@@ -28,11 +43,18 @@ afreeall(Area *ap)
 {
struct link *l, *l2;
 
-   for (l = ap->freelist; l != NULL; l = l2) {
-   l2 = l->next;
+   RB_FOREACH_SAFE(l, addrtree, ap->tree, l2) {
+   RB_REMOVE(addrtree, ap->tree, l);
free(l);
}
-   ap->freelist = NULL;
+}
+
+void
+adestroy(Area *ap)
+{
+   afreeall(ap);
+   free(ap->tree);
+   ap->tree = NULL;
 }
 
 #define L2P(l) ( (void *)(((char *)(l)) + sizeof(struct link)) )
@@ -50,11 +72,7 @@ alloc(size_t size, Area *ap)
l = calloc(1, sizeof(struct link) + size);
if (l == NULL)
internal_errorf(1, "unable to allocate memory");
-   l->next = ap->freelist;
-   l->prev = NULL;
-   if (ap->freelist)
-   ap->freelist->prev = l;
-   ap->freelist = l;
+   RB_INSERT(addrtree, ap->tree, l);
 
return L2P(l);
 }
@@ -82,7 +100,7 @@ areallocarray(void *ptr, size_t nmemb, s
 void *
 aresize(void *ptr, size_t size, Area *ap)
 {
-   struct link *l, *l2, *lprev, *lnext;
+   struct link *l, *l2;
 
if (ptr == NULL)
return alloc(size, ap);
@@ -92,18 +110,11 @@ aresize(void *ptr, size_t size, Area *ap
internal_errorf(1, "unable to allocate memory");
 
l = P2L(ptr);
-   lprev = l->prev;
-   lnext = l->next;
-
+   RB_REMOVE(addrtree, ap->tree, l);
l2 = realloc(l, sizeof(struct link) + size);
if (l2 == NULL)
internal_errorf(1, "unable to allocate memory");
-   if (lprev)
-   lprev->next = l2;
-   else
-   ap->freelist = l2;
-   if (lnext)
-   lnext->prev = l2;
+   RB_INSERT(addrtree, ap->tree, l2);
 
return L2P(l2);
 }
@@ -111,26 +122,12 @@ aresize(void *ptr, size_t size, Area *ap
 void
 afree(void *ptr, Area *ap)
 {
-   struct link *l, *l2;
+   struct link *l;
 
if (!ptr)
return;
 
l = P2L(ptr);
-
-   for (l2 = ap->freelist; l2 != NULL; l2 = l2->next) {
-   if (l == l2)
-   break;
-   }
-   if (l2 == NULL)
-   internal_errorf(1, "afree: %p not present in area %p", ptr, ap);
-
-   if (l->prev)
-   l->prev->next = l->next;
-   else
-   ap->freelist = l->next;
-   if (l->next)
-   l->next->prev = l->prev;
-
+   RB_REMOVE(addrtree, ap->tree, l);
free(l);
 }
Index: sh.h
===
RCS file: /d/cvs/src/bin/ksh/sh.h,v
retrieving revision 1.61
diff -u -p -r1.61 sh.h
--- sh.h4 Jul 2017 11:46:15 -   1.61
+++ sh.h24 Jul 2017 17:42:03 -
@@ -48,7 +48,7 @@ externcharusername[]; /* username for 
  * Area-based allocation built on malloc/free
  */
 typedef struct Area {
-   struct link *freelist;  /* free list */
+   struct addrtree *tree;  /* free list */
 } Area;
 
 extern Areaaperm;  /* permanent object space */
@@ -380,6 +380,7 @@ extern  int x_cols; /* tty columns */
 /* alloc.c */
 Area * ainit(Area *);
 void   afreeall(Area *);
+void   adestroy(Area *);
 void * 

test: Catch integer overflow, fail on trailing whitespaces

[Klemens Nanni]

test's internal getn() makes integers out of strings although boundaries
are checked for long which leads to wrong test results when operands are
greater than INT_MAX but smaller than LONG_MAX:

$ t() { /bin/test "$1" -lt 0 && : overflow; }
$ t 1
+ /bin/test 1 -lt 0
$ t $((1 << 31))# INT32_MAX + 1
+ /bin/test 2147483648 -lt 0
+ : overflow
n=$((0x7fff))
n=${n%7}8
$ t $n  # INT64_MAX + 1
+ /bin/test 9223372036854775808 -lt 0
test: 9223372036854775808: out of range

The INT64_MAX case is expected behaviour, just wanted to illustrate it.
Note how $n is crafted manually since $((1 << 63)) overflows in our KSH
already.

$ t "0 "
+ /bin/test 0  -lt 0


With this diff overflows are properly catched:

$ t $((1 << 31))
+ /usr/obj/bin/test 2147483648 -lt 0
test: 2147483648: too large

Another intended side effect is that trailing whitespaces in integer
operands will now cause test(1) to fail:

$ t "0 "
+ /usr/obj/bin/test/test 0  -lt 0
test: 0 : invalid

Expecting some people to oppose, I'd like to discuss this particular
change in behaviour. Here are my four cents:

- When used in shell scripts, integer operands should usually
  not require quoting, whether they're passed verbatim, through
  variables or subshells or the like. If they do contain
  whitespaces, the shell already takes care of them when
  omitting quotes since after variable substitution/expansion
  and before passing arguments to the executable or built-in.
  If quotes are used (as in "take this as is"), I generally
  prefer to be warned instead of letting unexpected characters
  slip through, especially when it comes to arithmetic
  operations.
- While properly returing integers, strtol(3) still considers
  trailing non-digits as error, stripping them in test(1) as of
  now seems more like a convenience hack (possibly problematic).
- Using the more strict strtonum(3) is not only safer but also
  way simpler and shorter. The manual pages tell us that leading
  whitespaces are ok but trailing ones are not.
- Behaviour across binaries and built-ins of various shells
  already differs so changing/improving our test(1) won't break
  consistency. This was tested with `test "0 " -lt 0':
  * test(1) from GNU coreutils 8.25 returns 0
  * bash, dash and ksh return 0
  * zsh returns 2 and warns "integer expression expected: 0 "
  * fish silently returns 1

Feedback and comments, please.

Index: test.c
===
RCS file: /cvs/src/bin/test/test.c,v
retrieving revision 1.17
diff -u -p -r1.17 test.c
--- test.c  21 Jan 2017 11:03:42 -  1.17
+++ test.c  24 Jul 2017 15:02:03 -
@@ -13,6 +13,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -451,26 +448,17 @@ t_lex(char *s)
return OPERAND;
 }
 
-/* atoi with error detection */
 static int
 getn(const char *s)
 {
-   char *p;
-   long r;
-
-   errno = 0;
-   r = strtol(s, , 10);
-
-   if (errno != 0)
-   errx(2, "%s: out of range", s);
-
-   while (isspace((unsigned char)*p))
-   p++;
+   const char *e;
+   int r;
 
-   if (*p)
-   errx(2, "%s: bad number", s);
+   r = strtonum(s, INT_MIN, INT_MAX, );
+   if (e)
+   errx(2, "%s: %s", s, e);
 
-   return (int) r;
+   return r;
 }
 
 static int

Re: apm(8): remove TRUE/FALSE defines

[Anton Lindqvist]

Hi,

On Sun, Jul 23, 2017 at 03:53:14PM +0200, Otto Moerbeek wrote:
> On Sun, Jul 23, 2017 at 03:04:54PM +0200, Anton Lindqvist wrote:
> 
> > Hi,
> > Fairly straightforward and no intended functional change. If changes
> > like this one is not encouraged I would advocate on at least using
> > stdbool.h instead.
> > 
> > Comments? OK?
> 
> why? It might be old school, but there's noting wrong with this code.

Just trying to make it more consistent with the rest of src. But it sure
does work despite being old school so lets leave it like that.

> 
>   -Otto
> 
> 
> > 
> > Index: apm.c
> > ===
> > RCS file: /cvs/src/usr.sbin/apm/apm.c,v
> > retrieving revision 1.33
> > diff -u -p -r1.33 apm.c
> > --- apm.c   23 Jul 2017 12:51:20 -  1.33
> > +++ apm.c   23 Jul 2017 13:02:38 -
> > @@ -45,9 +45,6 @@
> >  #include "pathnames.h"
> >  #include "apm-proto.h"
> >  
> > -#define FALSE 0
> > -#define TRUE 1
> > -
> >  extern char *__progname;
> >  
> >  static int do_zzz(int, enum apm_action);
> > @@ -145,12 +142,12 @@ int
> >  main(int argc, char *argv[])
> >  {
> > const char *sockname = _PATH_APM_SOCKET;
> > -   int doac = FALSE;
> > -   int dopct = FALSE;
> > -   int dobstate = FALSE;
> > -   int domin = FALSE;
> > -   int doperf = FALSE;
> > -   int verbose = FALSE;
> > +   int doac = 0;
> > +   int dopct = 0;
> > +   int dobstate = 0;
> > +   int domin = 0;
> > +   int doperf = 0;
> > +   int verbose = 0;
> > int ch, fd, rval;
> > enum apm_action action = NONE;
> > struct apm_command command;
> > @@ -164,7 +161,7 @@ main(int argc, char *argv[])
> > while ((ch = getopt(argc, argv, "ACHLlmbvaPSzZf:")) != -1) {
> > switch (ch) {
> > case 'v':
> > -   verbose = TRUE;
> > +   verbose = 1;
> > break;
> > case 'f':
> > sockname = optarg;
> > @@ -207,31 +204,31 @@ main(int argc, char *argv[])
> > case 'b':
> > if (action != NONE && action != GETSTATUS)
> > usage();
> > -   dobstate = TRUE;
> > +   dobstate = 1;
> > action = GETSTATUS;
> > break;
> > case 'l':
> > if (action != NONE && action != GETSTATUS)
> > usage();
> > -   dopct = TRUE;
> > +   dopct = 1;
> > action = GETSTATUS;
> > break;
> > case 'm':
> > if (action != NONE && action != GETSTATUS)
> > usage();
> > -   domin = TRUE;
> > +   domin = 1;
> > action = GETSTATUS;
> > break;
> > case 'a':
> > if (action != NONE && action != GETSTATUS)
> > usage();
> > -   doac = TRUE;
> > +   doac = 1;
> > action = GETSTATUS;
> > break;
> > case 'P':
> > if (action != NONE && action != GETSTATUS)
> > usage();
> > -   doperf = TRUE;
> > +   doperf = 1;
> > action = GETSTATUS;
> > break;
> > default:
> > @@ -280,7 +277,7 @@ main(int argc, char *argv[])
> > goto balony;
> > case NONE:
> > action = GETSTATUS;
> > -   verbose = doac = dopct = dobstate = domin = doperf = TRUE;
> > +   verbose = doac = dopct = dobstate = domin = doperf = 1;
> > /* FALLTHROUGH */
> > case GETSTATUS:
> > if (fd == -1) {
> 

Re: sorwakeup() missing KERNEL_LOCK()

[Alexander Bluhm]

On Mon, Jul 24, 2017 at 12:54:31PM +0200, Martin Pieuchot wrote:
> divert/divert6 might end up calling sorwakeup() w/o KERNEL_LOCK() since
> pf_test() is not always executed with it.  Diff below fixes that and
> put two asserts where selwakup() is called in the socket layer.

I think calling divert_packet() directly from pf_test() is a layer
violation.  It would be better to add a mbuf tag in pf and process
divert-packet at the protocol delivery loop.  Then divert_packet()
could be a regular pr_input function and work more like divert-to.
I will try to do something like this.  Then we can deal with
sorwakeup() in all pr_input funtions the same way.  We still have
to find a general solution.

> ok?

You diff is a quick fix for an actual problem.  Let's move forward.
OK bluhm@

> Index: kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.196
> diff -u -p -r1.196 uipc_socket.c
> --- kern/uipc_socket.c20 Jul 2017 09:49:45 -  1.196
> +++ kern/uipc_socket.c24 Jul 2017 10:33:56 -
> @@ -1926,6 +1926,7 @@ sogetopt(struct socket *so, int level, i
>  void
>  sohasoutofband(struct socket *so)
>  {
> + KERNEL_ASSERT_LOCKED();
>   csignal(so->so_pgid, SIGURG, so->so_siguid, so->so_sigeuid);
>   selwakeup(>so_rcv.sb_sel);
>  }
> Index: kern/uipc_socket2.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 uipc_socket2.c
> --- kern/uipc_socket2.c   18 Jul 2017 06:12:09 -  1.84
> +++ kern/uipc_socket2.c   24 Jul 2017 10:49:12 -
> @@ -382,6 +382,7 @@ sbunlock(struct sockbuf *sb)
>  void
>  sowakeup(struct socket *so, struct sockbuf *sb)
>  {
> + KERNEL_ASSERT_LOCKED();
>   soassertlocked(so);
>  
>   selwakeup(>sb_sel);
> Index: netinet/ip_divert.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_divert.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 ip_divert.c
> --- netinet/ip_divert.c   26 Jun 2017 09:32:32 -  1.48
> +++ netinet/ip_divert.c   24 Jul 2017 10:44:39 -
> @@ -226,8 +226,11 @@ divert_packet(struct mbuf *m, int dir, u
>   divstat_inc(divs_fullsock);
>   m_freem(m);
>   return (0);
> - } else
> + } else {
> + KERNEL_LOCK();
>   sorwakeup(inp->inp_socket);
> + KERNEL_UNLOCK();
> + }
>   }
>  
>   if (sa == NULL) {
> Index: netinet6/ip6_divert.c
> ===
> RCS file: /cvs/src/sys/netinet6/ip6_divert.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 ip6_divert.c
> --- netinet6/ip6_divert.c 26 Jun 2017 09:32:32 -  1.48
> +++ netinet6/ip6_divert.c 24 Jul 2017 10:44:36 -
> @@ -227,8 +227,11 @@ divert6_packet(struct mbuf *m, int dir, 
>   div6stat_inc(div6s_fullsock);
>   m_freem(m);
>   return (0);
> - } else
> + } else {
> + KERNEL_LOCK();
>   sorwakeup(inp->inp_socket);
> + KERNEL_UNLOCK();
> + }
>   }
>  
>   if (sa == NULL) {

Re: pfkeyv2 rename struct keycb pointer

[Alexander Bluhm]

On Sat, Jul 22, 2017 at 12:47:55PM +0200, Claudio Jeker wrote:
> Suggested by bluhm@, switch from struct keycb *pk to struct keycb *kp.
> 
> OK?

OK bluhm@, this is more consistent with other PCB code.

> @@ -942,7 +942,7 @@ pfkeyv2_send(struct socket *so, void *me
>   struct radix_node_head *rnh;
>   struct radix_node *rn = NULL;
>  
> - struct keycb *pk, *bpk = NULL;
> + struct keycb *kp, *bkp = NULL;
>  
>   void *freeme = NULL, *bckptr = NULL;
>   void *headers[SADB_EXT_MAX + 1];

And I recommended to not intialize bkp, it is a loop variable.

Re: make: clarify an error string

[Todd C. Miller]

On Mon, 24 Jul 2017 13:02:12 +0200, Marc Espie wrote:

> Here's a proper error message:

OK millert@

 - todd

Re: NET_LOCK() w/o SPL

[Alexander Bluhm]

On Mon, Jul 24, 2017 at 04:39:14PM +0200, Martin Pieuchot wrote:
> Here's a simpler diff.  I'd like to move forward with this since it's
> the first but blocking step to split the NET_LOCK() and socket locks.

OK bluhm@

> 
> Index: sys/systm.h
> ===
> RCS file: /cvs/src/sys/sys/systm.h,v
> retrieving revision 1.131
> diff -u -p -r1.131 systm.h
> --- sys/systm.h   29 May 2017 12:12:35 -  1.131
> +++ sys/systm.h   24 Jul 2017 13:22:37 -
> @@ -299,12 +299,12 @@ extern struct rwlock netlock;
>  #define  NET_LOCK(s) 
> \
>  do { \
>   rw_enter_write();   \
> - s = splsoftnet();   \
> + s = IPL_SOFTNET;\
>  } while (0)
>  
>  #define  NET_UNLOCK(s)   
> \
>  do { \
> - splx(s);\
> + (void)s;\
>   rw_exit_write();\
>  } while (0)
>  
> @@ -312,7 +312,6 @@ do {  
> \
>  do { \
>   if (rw_status() != RW_WRITE)\
>   splassert_fail(RW_WRITE, rw_status(), __func__);\
> - splsoftassert(IPL_SOFTNET); \
>  } while (0)
>  
>  #define  NET_ASSERT_UNLOCKED()   
> \

Re: ospfd: add IMSG_IFADDRADD to deal with "sh /etc/netstart if"

[Remi Locherer]

On Fri, Jul 21, 2017 at 06:24:06PM +0200, Remi Locherer wrote:
> On Fri, Jul 21, 2017 at 02:45:03PM +0200, Florian Riehm wrote:
> > On 06/25/17 23:47, Remi Locherer wrote:
> > > Hi,
> > > 
> > > ospfd does not react nicely when running "sh /etc/netstart if".
> > > 
> > > This is because adding the same address again do an interface results
> > > in RTM_DELADDR and RTM_NEWADDR. ospfd handles the former but the later.
> > > If this happens ospfd says "interface vether0:192.168.250.1 gone".
> > > Adjacencies on that interface are down and ospfd can not recover.
> > > 
> > > The below patch adds IMSG_IFADDRADD to deal with that. With it ospfd
> > > logs the following after "ifconfig vether0 192.168.250.1/24" (same address
> > > as active before):
> > > 
> > 
> > Hi Remi,
> > 
> > thanks for your report and your patch.
> > I think it is the right approach, but unfortunately it doesn't work in my 
> > setup.
> > If I run 'sh /etc/netstart vio1' vio1 goes down and stays down.
> > Please have a look to my config / logs. What is the differece between your 
> > and
> > my test?
> 
> I tested with an interface connected to stub network. Your output shows an
> interface connected to a transit network. In my test setup I did not get the
> error message: "if_join_group: error IP_ADD_MEMBERSHIP"
> 
> I'll look into it and try to fix this.

I could reproduce the behaviour you see with my patch when testing with
vmm and vio interfaces. It looks like in the IFADDRDEL case the interface
can not leave the multicast group.

I do not see this when testing with vether, pair or vlan (over ix)
interfaces. Could this be a bug with multicast handling in vio?

> > Beside that:
> > - I would rename 'struct ifaddrdel' to 'struct ifaddr' and extend it. So we
> >   can use it in if_newaddr() and if_deladdr() and avoid 'struct ifaddrnew'.
> 
> Makes sense.

The below patch introduces struct ifaddr and removes struct ifaddrdel. It
also introduces two additional debug messages in interface.c:

if_act_reset: interface vether0 left group 224.0.0.5
if_act_start: interface vether0 joined group 224.0.0.5

I added those to see a bit better what's going on. But I would not commit
them. They are now printed even if joining the group is not successful
(the message before would tell it).


Index: interface.c
===
RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v
retrieving revision 1.81
diff -u -p -r1.81 interface.c
--- interface.c 5 Dec 2015 12:20:13 -   1.81
+++ interface.c 24 Jul 2017 14:55:58 -
@@ -357,6 +357,8 @@ if_act_start(struct iface *iface)
inet_aton(AllSPFRouters, );
if (if_join_group(iface, ))
return (-1);
+   log_debug("if_act_start: interface %s joined group %s",
+   iface->name, AllSPFRouters);
iface->state = IF_STA_POINTTOPOINT;
break;
case IF_TYPE_VIRTUALLINK:
@@ -371,6 +373,8 @@ if_act_start(struct iface *iface)
inet_aton(AllSPFRouters, );
if (if_join_group(iface, ))
return (-1);
+   log_debug("if_act_start: interface %s joined group %s",
+   iface->name, AllSPFRouters);
if (iface->priority == 0) {
iface->state = IF_STA_DROTHER;
} else {
@@ -547,6 +551,8 @@ if_act_reset(struct iface *iface)
/* try to cleanup */
inet_aton(AllSPFRouters, );
if_leave_group(iface, );
+   log_debug("if_act_reset: interface %s left group %s",
+   iface->name, AllSPFRouters);
if (iface->state & IF_STA_DRORBDR) {
inet_aton(AllDRouters, );
if_leave_group(iface, );
Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v
retrieving revision 1.107
diff -u -p -r1.107 kroute.c
--- kroute.c27 Dec 2016 09:15:16 -  1.107
+++ kroute.c24 Jul 2017 14:55:58 -
@@ -1046,6 +1046,7 @@ if_newaddr(u_short ifindex, struct socka
 {
struct kif_node *kif;
struct kif_addr *ka;
+   struct ifaddrifn;
 
if (ifa == NULL || ifa->sin_family != AF_INET)
return;
@@ -1066,6 +1067,12 @@ if_newaddr(u_short ifindex, struct socka
ka->dstbrd.s_addr = INADDR_NONE;
 
TAILQ_INSERT_TAIL(>addrs, ka, entry);
+
+   ifn.addr = ka->addr;
+   ifn.mask = ka->mask;
+   ifn.dst = ka->dstbrd;
+   ifn.ifindex = ifindex;
+   main_imsg_compose_ospfe(IMSG_IFADDRADD, 0, , sizeof(ifn));
 }
 
 void
@@ -1074,7 +1081,7 @@ if_deladdr(u_short ifindex, struct socka
 {
struct kif_node *kif;
struct kif_addr *ka, *nka;
-   struct ifaddrdel ifc;
+   struct ifaddrifc;
 
if (ifa == NULL || ifa->sin_family != AF_INET)
   

Re: NET_LOCK() w/o SPL

[Martin Pieuchot]

On 03/07/17(Mon) 12:23, Martin Pieuchot wrote:
> All network processing contexts, with the exception of hardware
> interrupt handlers, are now process contexts.  That means the SPL
> protection is no longer needed inside the NET_LOCK().
> 
> So the diff below removes the splsofnet()/splx() dance from the
> NET_LOCK().  I'm not changing the NET_LOCK() macro in the same
> diff in case we find some unexpected bugs and need to revert this
> change.

Here's a simpler diff.  I'd like to move forward with this since it's
the first but blocking step to split the NET_LOCK() and socket locks.

Index: sys/systm.h
===
RCS file: /cvs/src/sys/sys/systm.h,v
retrieving revision 1.131
diff -u -p -r1.131 systm.h
--- sys/systm.h 29 May 2017 12:12:35 -  1.131
+++ sys/systm.h 24 Jul 2017 13:22:37 -
@@ -299,12 +299,12 @@ extern struct rwlock netlock;
 #defineNET_LOCK(s) 
\
 do {   \
rw_enter_write();   \
-   s = splsoftnet();   \
+   s = IPL_SOFTNET;\
 } while (0)
 
 #defineNET_UNLOCK(s)   
\
 do {   \
-   splx(s);\
+   (void)s;\
rw_exit_write();\
 } while (0)
 
@@ -312,7 +312,6 @@ do {
\
 do {   \
if (rw_status() != RW_WRITE)\
splassert_fail(RW_WRITE, rw_status(), __func__);\
-   splsoftassert(IPL_SOFTNET); \
 } while (0)
 
 #defineNET_ASSERT_UNLOCKED()   
\

Re: connect(2), KERNEL_LOCK() vs socket lock

[Alexander Bluhm]

On Mon, Jul 24, 2017 at 03:06:06PM +0200, Martin Pieuchot wrote:
> So here's a fixed diff.

OK bluhm@

> Index: kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.196
> diff -u -p -r1.196 uipc_socket.c
> --- kern/uipc_socket.c20 Jul 2017 09:49:45 -  1.196
> +++ kern/uipc_socket.c24 Jul 2017 12:58:09 -
> @@ -311,11 +311,12 @@ soaccept(struct socket *so, struct mbuf 
>  int
>  soconnect(struct socket *so, struct mbuf *nam)
>  {
> - int s, error;
> + int error;
> +
> + soassertlocked(so);
>  
>   if (so->so_options & SO_ACCEPTCONN)
>   return (EOPNOTSUPP);
> - s = solock(so);
>   /*
>* If protocol is connection-based, can only connect once.
>* Otherwise, if connected, try to disconnect first.
> @@ -329,19 +330,17 @@ soconnect(struct socket *so, struct mbuf
>   else
>   error = (*so->so_proto->pr_usrreq)(so, PRU_CONNECT,
>   NULL, nam, NULL, curproc);
> - sounlock(s);
>   return (error);
>  }
>  
>  int
>  soconnect2(struct socket *so1, struct socket *so2)
>  {
> - int s, error;
> + int error;
>  
> - s = solock(so1);
> + soassertlocked(so1);
>   error = (*so1->so_proto->pr_usrreq)(so1, PRU_CONNECT2, NULL,
>   (struct mbuf *)so2, NULL, curproc);
> - sounlock(s);
>   return (error);
>  }
>  
> Index: kern/uipc_syscalls.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.155
> diff -u -p -r1.155 uipc_syscalls.c
> --- kern/uipc_syscalls.c  20 Jul 2017 18:40:16 -  1.155
> +++ kern/uipc_syscalls.c  24 Jul 2017 12:56:42 -
> @@ -373,18 +373,19 @@ sys_connect(struct proc *p, void *v, reg
>   if ((error = getsock(p, SCARG(uap, s), )) != 0)
>   return (error);
>   so = fp->f_data;
> + s = solock(so);
>   if (so->so_state & SS_ISCONNECTING) {
> - FRELE(fp, p);
> - return (EALREADY);
> + error = EALREADY;
> + goto out;
>   }
>   error = sockargs(, SCARG(uap, name), SCARG(uap, namelen),
>   MT_SONAME);
>   if (error)
> - goto bad;
> + goto out;
>   error = pledge_socket(p, so->so_proto->pr_domain->dom_family,
>   so->so_state);
>   if (error)
> - goto bad;
> + goto out;
>  #ifdef KTRACE
>   if (KTRPOINT(p, KTR_STRUCT))
>   ktrsockaddr(p, mtod(nam, caddr_t), SCARG(uap, namelen));
> @@ -393,11 +394,8 @@ sys_connect(struct proc *p, void *v, reg
>   if (isdnssocket(so)) {
>   u_int namelen = nam->m_len;
>   error = dns_portcheck(p, so, mtod(nam, void *), );
> - if (error) {
> - FRELE(fp, p);
> - m_freem(nam);
> - return (error);
> - }
> + if (error)
> + goto out;
>   nam->m_len = namelen;
>   }
>  
> @@ -405,11 +403,9 @@ sys_connect(struct proc *p, void *v, reg
>   if (error)
>   goto bad;
>   if ((so->so_state & SS_NBIO) && (so->so_state & SS_ISCONNECTING)) {
> - FRELE(fp, p);
> - m_freem(nam);
> - return (EINPROGRESS);
> + error = EINPROGRESS;
> + goto out;
>   }
> - s = solock(so);
>   while ((so->so_state & SS_ISCONNECTING) && so->so_error == 0) {
>   error = sosleep(so, >so_timeo, PSOCK | PCATCH,
>   "netcon2", 0);
> @@ -423,10 +419,11 @@ sys_connect(struct proc *p, void *v, reg
>   error = so->so_error;
>   so->so_error = 0;
>   }
> - sounlock(s);
>  bad:
>   if (!interrupted)
>   so->so_state &= ~SS_ISCONNECTING;
> +out:
> + sounlock(s);
>   FRELE(fp, p);
>   m_freem(nam);
>   if (error == ERESTART)
> @@ -446,7 +443,7 @@ sys_socketpair(struct proc *p, void *v, 
>   struct filedesc *fdp = p->p_fd;
>   struct file *fp1, *fp2;
>   struct socket *so1, *so2;
> - int type, cloexec, nonblock, fflag, error, sv[2];
> + int s, type, cloexec, nonblock, fflag, error, sv[2];
>  
>   type  = SCARG(uap, type) & ~(SOCK_CLOEXEC | SOCK_NONBLOCK);
>   cloexec = (SCARG(uap, type) & SOCK_CLOEXEC) ? UF_EXCLOSE : 0;
> @@ -460,14 +457,20 @@ sys_socketpair(struct proc *p, void *v, 
>   if (error)
>   goto free1;
>  
> - if ((error = soconnect2(so1, so2)) != 0)
> + s = solock(so1);
> + error = soconnect2(so1, so2);
> + sounlock(s);
> + if (error != 0)
>   goto free2;
>  
>   if ((SCARG(uap, type) & SOCK_TYPE_MASK) == SOCK_DGRAM) {
>   /*
>* Datagram socket connection is asymmetric.
>*/
> -  if ((error = soconnect2(so2, so1)) != 0)
> 

Re: connect(2), KERNEL_LOCK() vs socket lock

[Martin Pieuchot]

On 24/07/17(Mon) 14:34, Alexander Bluhm wrote:
> On Mon, Jul 24, 2017 at 11:56:20AM +0200, Martin Pieuchot wrote:
> > Updated diff addressing your comments.
> 
> Yes, previous issues are fixed.  But static code analysis shows
> that you missed a lock in sys_socketpair() for soconnect2().
> 
> Analyzing locks for soconnect2
> No lock: [soconnect2, sys_socketpair]
> 
> I could convince Zaur Molotnikov to publish his tool:
> https://github.com/qutorial/lockfish

Nice.

So here's a fixed diff.

Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.196
diff -u -p -r1.196 uipc_socket.c
--- kern/uipc_socket.c  20 Jul 2017 09:49:45 -  1.196
+++ kern/uipc_socket.c  24 Jul 2017 12:58:09 -
@@ -311,11 +311,12 @@ soaccept(struct socket *so, struct mbuf 
 int
 soconnect(struct socket *so, struct mbuf *nam)
 {
-   int s, error;
+   int error;
+
+   soassertlocked(so);
 
if (so->so_options & SO_ACCEPTCONN)
return (EOPNOTSUPP);
-   s = solock(so);
/*
 * If protocol is connection-based, can only connect once.
 * Otherwise, if connected, try to disconnect first.
@@ -329,19 +330,17 @@ soconnect(struct socket *so, struct mbuf
else
error = (*so->so_proto->pr_usrreq)(so, PRU_CONNECT,
NULL, nam, NULL, curproc);
-   sounlock(s);
return (error);
 }
 
 int
 soconnect2(struct socket *so1, struct socket *so2)
 {
-   int s, error;
+   int error;
 
-   s = solock(so1);
+   soassertlocked(so1);
error = (*so1->so_proto->pr_usrreq)(so1, PRU_CONNECT2, NULL,
(struct mbuf *)so2, NULL, curproc);
-   sounlock(s);
return (error);
 }
 
Index: kern/uipc_syscalls.c
===
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.155
diff -u -p -r1.155 uipc_syscalls.c
--- kern/uipc_syscalls.c20 Jul 2017 18:40:16 -  1.155
+++ kern/uipc_syscalls.c24 Jul 2017 12:56:42 -
@@ -373,18 +373,19 @@ sys_connect(struct proc *p, void *v, reg
if ((error = getsock(p, SCARG(uap, s), )) != 0)
return (error);
so = fp->f_data;
+   s = solock(so);
if (so->so_state & SS_ISCONNECTING) {
-   FRELE(fp, p);
-   return (EALREADY);
+   error = EALREADY;
+   goto out;
}
error = sockargs(, SCARG(uap, name), SCARG(uap, namelen),
MT_SONAME);
if (error)
-   goto bad;
+   goto out;
error = pledge_socket(p, so->so_proto->pr_domain->dom_family,
so->so_state);
if (error)
-   goto bad;
+   goto out;
 #ifdef KTRACE
if (KTRPOINT(p, KTR_STRUCT))
ktrsockaddr(p, mtod(nam, caddr_t), SCARG(uap, namelen));
@@ -393,11 +394,8 @@ sys_connect(struct proc *p, void *v, reg
if (isdnssocket(so)) {
u_int namelen = nam->m_len;
error = dns_portcheck(p, so, mtod(nam, void *), );
-   if (error) {
-   FRELE(fp, p);
-   m_freem(nam);
-   return (error);
-   }
+   if (error)
+   goto out;
nam->m_len = namelen;
}
 
@@ -405,11 +403,9 @@ sys_connect(struct proc *p, void *v, reg
if (error)
goto bad;
if ((so->so_state & SS_NBIO) && (so->so_state & SS_ISCONNECTING)) {
-   FRELE(fp, p);
-   m_freem(nam);
-   return (EINPROGRESS);
+   error = EINPROGRESS;
+   goto out;
}
-   s = solock(so);
while ((so->so_state & SS_ISCONNECTING) && so->so_error == 0) {
error = sosleep(so, >so_timeo, PSOCK | PCATCH,
"netcon2", 0);
@@ -423,10 +419,11 @@ sys_connect(struct proc *p, void *v, reg
error = so->so_error;
so->so_error = 0;
}
-   sounlock(s);
 bad:
if (!interrupted)
so->so_state &= ~SS_ISCONNECTING;
+out:
+   sounlock(s);
FRELE(fp, p);
m_freem(nam);
if (error == ERESTART)
@@ -446,7 +443,7 @@ sys_socketpair(struct proc *p, void *v, 
struct filedesc *fdp = p->p_fd;
struct file *fp1, *fp2;
struct socket *so1, *so2;
-   int type, cloexec, nonblock, fflag, error, sv[2];
+   int s, type, cloexec, nonblock, fflag, error, sv[2];
 
type  = SCARG(uap, type) & ~(SOCK_CLOEXEC | SOCK_NONBLOCK);
cloexec = (SCARG(uap, type) & SOCK_CLOEXEC) ? UF_EXCLOSE : 0;
@@ -460,14 +457,20 @@ sys_socketpair(struct proc *p, void *v, 
if (error)
goto free1;
 
-   if ((error = soconnect2(so1, so2)) != 0)
+   s = solock(so1);
+   error = soconnect2(so1, 

Re: connect(2), KERNEL_LOCK() vs socket lock

[Alexander Bluhm]

On Mon, Jul 24, 2017 at 11:56:20AM +0200, Martin Pieuchot wrote:
> Updated diff addressing your comments.

Yes, previous issues are fixed.  But static code analysis shows
that you missed a lock in sys_socketpair() for soconnect2().

Analyzing locks for soconnect2
No lock: [soconnect2, sys_socketpair]

I could convince Zaur Molotnikov to publish his tool:
https://github.com/qutorial/lockfish

bluhm

> Index: kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.196
> diff -u -p -r1.196 uipc_socket.c
> --- kern/uipc_socket.c20 Jul 2017 09:49:45 -  1.196
> +++ kern/uipc_socket.c24 Jul 2017 09:52:01 -
> @@ -311,11 +311,12 @@ soaccept(struct socket *so, struct mbuf 
>  int
>  soconnect(struct socket *so, struct mbuf *nam)
>  {
> - int s, error;
> + int error;
> +
> + soassertlocked(so);
>  
>   if (so->so_options & SO_ACCEPTCONN)
>   return (EOPNOTSUPP);
> - s = solock(so);
>   /*
>* If protocol is connection-based, can only connect once.
>* Otherwise, if connected, try to disconnect first.
> @@ -329,19 +330,17 @@ soconnect(struct socket *so, struct mbuf
>   else
>   error = (*so->so_proto->pr_usrreq)(so, PRU_CONNECT,
>   NULL, nam, NULL, curproc);
> - sounlock(s);
>   return (error);
>  }
>  
>  int
>  soconnect2(struct socket *so1, struct socket *so2)
>  {
> - int s, error;
> + int error;
>  
> - s = solock(so1);
> + soassertlocked(so1);
>   error = (*so1->so_proto->pr_usrreq)(so1, PRU_CONNECT2, NULL,
>   (struct mbuf *)so2, NULL, curproc);
> - sounlock(s);
>   return (error);
>  }
>  
> Index: kern/uipc_syscalls.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
> retrieving revision 1.155
> diff -u -p -r1.155 uipc_syscalls.c
> --- kern/uipc_syscalls.c  20 Jul 2017 18:40:16 -  1.155
> +++ kern/uipc_syscalls.c  24 Jul 2017 09:53:13 -
> @@ -373,18 +373,19 @@ sys_connect(struct proc *p, void *v, reg
>   if ((error = getsock(p, SCARG(uap, s), )) != 0)
>   return (error);
>   so = fp->f_data;
> + s = solock(so);
>   if (so->so_state & SS_ISCONNECTING) {
> - FRELE(fp, p);
> - return (EALREADY);
> + error = EALREADY;
> + goto out;
>   }
>   error = sockargs(, SCARG(uap, name), SCARG(uap, namelen),
>   MT_SONAME);
>   if (error)
> - goto bad;
> + goto out;
>   error = pledge_socket(p, so->so_proto->pr_domain->dom_family,
>   so->so_state);
>   if (error)
> - goto bad;
> + goto out;
>  #ifdef KTRACE
>   if (KTRPOINT(p, KTR_STRUCT))
>   ktrsockaddr(p, mtod(nam, caddr_t), SCARG(uap, namelen));
> @@ -393,11 +394,8 @@ sys_connect(struct proc *p, void *v, reg
>   if (isdnssocket(so)) {
>   u_int namelen = nam->m_len;
>   error = dns_portcheck(p, so, mtod(nam, void *), );
> - if (error) {
> - FRELE(fp, p);
> - m_freem(nam);
> - return (error);
> - }
> + if (error)
> + goto out;
>   nam->m_len = namelen;
>   }
>  
> @@ -405,11 +403,9 @@ sys_connect(struct proc *p, void *v, reg
>   if (error)
>   goto bad;
>   if ((so->so_state & SS_NBIO) && (so->so_state & SS_ISCONNECTING)) {
> - FRELE(fp, p);
> - m_freem(nam);
> - return (EINPROGRESS);
> + error = EINPROGRESS;
> + goto out;
>   }
> - s = solock(so);
>   while ((so->so_state & SS_ISCONNECTING) && so->so_error == 0) {
>   error = sosleep(so, >so_timeo, PSOCK | PCATCH,
>   "netcon2", 0);
> @@ -423,10 +419,11 @@ sys_connect(struct proc *p, void *v, reg
>   error = so->so_error;
>   so->so_error = 0;
>   }
> - sounlock(s);
>  bad:
>   if (!interrupted)
>   so->so_state &= ~SS_ISCONNECTING;
> +out:
> + sounlock(s);
>   FRELE(fp, p);
>   m_freem(nam);
>   if (error == ERESTART)
> Index: miscfs/fifofs/fifo_vnops.c
> ===
> RCS file: /cvs/src/sys/miscfs/fifofs/fifo_vnops.c,v
> retrieving revision 1.57
> diff -u -p -r1.57 fifo_vnops.c
> --- miscfs/fifofs/fifo_vnops.c8 Jul 2017 09:19:02 -   1.57
> +++ miscfs/fifofs/fifo_vnops.c24 Jul 2017 09:54:45 -
> @@ -124,7 +124,7 @@ fifo_open(void *v)
>   struct fifoinfo *fip;
>   struct proc *p = ap->a_p;
>   struct socket *rso, *wso;
> - int error;
> + int s, error;
>  
>   if ((fip = vp->v_fifoinfo) == NULL) {
>   fip = malloc(sizeof(*fip), M_VNODE, M_WAITOK);
> 

Re: make netstart a variable

[Theo de Raadt]

No way, that doesn't serve anyone else's purposes.

You can maintain that as a diff in your own tree.

> Hello,
> personally, I prefer using my short script, over the stock /etc/netstart,
> so why not let everybody use what they prefer?
> 
> diff --git a/etc/rc b/etc/rc
> index 75e63a397e4..f56485a60f5 100644
> --- a/etc/rc
> +++ b/etc/rc
> @@ -447,7 +447,7 @@ echo 'starting network'
>   # Prevents carp from preempting until the system is booted.
>   ifconfig -g carp carpdemote 128
> 
> -sh /etc/netstart
> +sh $netstart
> 
>   # Any write triggers a rekey.
>   dmesg >/dev/random
> diff --git a/etc/rc.conf b/etc/rc.conf
> index 05146d58a4e..340c3de55df 100644
> --- a/etc/rc.conf
> +++ b/etc/rc.conf
> @@ -40,6 +40,7 @@ ldpd_flags=NO
>   lpd_flags=NO   # for normal use: "" (or "-l" for debugging)
>   mopd_flags=NO
>   mrouted_flags=NO   # be sure to enable multicast below
> +netstart=/etc/netstart
>   npppd_flags=NO
>   nsd_flags=NO
>   ntpd_flags=
> 
> 
> 
> 
> 
> 
> 

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

[Rob Pierce]

> From: "Marc Espie" 
> To: "Rob Pierce" 
> Cc: "tech" 
> 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: newsyslog: simplify mail sending

[Ingo Schwarze]

Hi Jeremie,

Jeremie Courreges-Anglas wrote on Sat, Jul 22, 2017 at 09:27:29PM +0200:

> I have nothing against asprintf(3), but the openmail function looks
> needlessly complicated.
> 
> ok?

Looks like a nice simplification, works for me, and looks good to
code inspection, so OK schwarze@.

Yours,
  Ingo


> Index: newsyslog.c
> ===
> RCS file: /d/cvs/src/usr.bin/newsyslog/newsyslog.c,v
> retrieving revision 1.107
> diff -u -p -p -u -r1.107 newsyslog.c
> --- newsyslog.c   22 Jul 2017 17:06:40 -  1.107
> +++ newsyslog.c   22 Jul 2017 19:21:23 -
> @@ -147,7 +147,6 @@ char  hostname[HOST_NAME_MAX+1]; /* Hostn
>  char daytime[33];/* timenow in human readable form */
>  char *arcdir;/* Dir to put archives in (if it exists) */
>  
> -FILE   *openmail(void);
>  char   *lstat_log(char *, size_t, int);
>  char   *missing_field(char *, char *, int);
>  char   *sob(char *);
> @@ -1072,13 +1071,12 @@ domonitor(struct conf_entry *ent)
>   fclose(fp);
>   goto cleanup;
>   }
> -
> - /* Send message. */
>   fclose(fp);
>  
> - fp = openmail();
> + /* Send message. */
> + fp = popen(SENDMAIL " -t", "w");
>   if (fp == NULL) {
> - warn("openmail");
> + warn("popen");
>   goto cleanup;
>   }
>   fprintf(fp, "Auto-Submitted: auto-generated\n");
> @@ -1103,20 +1101,6 @@ cleanup:
>   free(flog);
>   free(rb);
>   return (1);
> -}
> -
> -FILE *
> -openmail(void)
> -{
> - char *cmdbuf = NULL;
> - FILE *ret;
> -
> - if (asprintf(, "%s -t", SENDMAIL) != -1) {
> - ret = popen(cmdbuf, "w");
> - free(cmdbuf);
> - return (ret);
> - }
> - return (NULL);
>  }
>  
>  /* ARGSUSED */

Re: make netstart a variable

[Gregory Edigarov]

Hello, Martin, hello, Ingo.

ok, so here's more background as to why I propose this change.

my setup is even simpler then stock setup. I just use the script that 
usually reads:


#!/bin/sh

/bin/hostname 
/sbin/ifconfig lo0 127.0.0.1/8
/sbin/ifconfig 
/sbin/route add -net default ...
/sbin/dhclient ...

the reason I am doing this is both historical (BSD/OS had this setup)
and practical (prefer to have a single observable script) also aesthetic 
(just do not like to have many single line files).



On 24.07.17 13:33, Ingo Schwarze wrote:

Hi,

Gregory Edigarov wrote on Mon, Jul 24, 2017 at 01:22:55PM +0300:


personally, I prefer using my short script, over the stock
/etc/netstart, so why not let everybody use what they prefer?

Hell no.
This is directly contrary to project goals.

KISS.  No useless knobs.

That said, OpenBSD is free software, you can replace any component
you want on your personal computer, even /etc/netstart.
But that certainly isn't supported, and if it ever results in
bogus bug reports, you *will* get yelled at.

Yours,
   Ingo



diff --git a/etc/rc b/etc/rc
index 75e63a397e4..f56485a60f5 100644
--- a/etc/rc
+++ b/etc/rc
@@ -447,7 +447,7 @@ echo 'starting network'
   # Prevents carp from preempting until the system is booted.
   ifconfig -g carp carpdemote 128

-sh /etc/netstart
+sh $netstart

   # Any write triggers a rekey.
   dmesg >/dev/random
diff --git a/etc/rc.conf b/etc/rc.conf
index 05146d58a4e..340c3de55df 100644
--- a/etc/rc.conf
+++ b/etc/rc.conf
@@ -40,6 +40,7 @@ ldpd_flags=NO
   lpd_flags=NO   # for normal use: "" (or "-l" for debugging)
   mopd_flags=NO
   mrouted_flags=NO   # be sure to enable multicast below
+netstart=/etc/netstart
   npppd_flags=NO
   nsd_flags=NO
   ntpd_flags=

Re: make: clarify an error string

[Ingo Schwarze]

Hi Marc,

Marc Espie wrote on Mon, Jul 24, 2017 at 01:02:12PM +0200:
> On Mon, Jul 24, 2017 at 10:43:03AM +0200, Marc Espie wrote:
>> On Mon, Jul 24, 2017 at 03:15:32PM +0800, Michael W. Bombardieri wrote:

>>> In make(1), do_run_command() has a sanity check for a null command string.
>>> The error message passes the null string to printf(). Is this any better?

> Here's a proper error message:

Works for me, looks good to code inspection, and i don't see how it
could possibly break anything, so OK schwarze@.

Yours,
  Ingo


> Index: engine.c
> ===
> RCS file: /cvs/src/usr.bin/make/engine.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 engine.c
> --- engine.c  9 Jul 2017 15:28:00 -   1.53
> +++ engine.c  24 Jul 2017 10:56:38 -
> @@ -734,7 +734,7 @@ setup_engine(void)
>  }
>  
>  static bool
> -do_run_command(Job *job)
> +do_run_command(Job *job, const char *pre)
>  {
>   bool silent;/* Don't print command */
>   bool doExecute; /* Execute the command */
> @@ -752,7 +752,9 @@ do_run_command(Job *job)
>   /* How can we execute a null command ? we warn the user that the
>* command expanded to nothing (is this the right thing to do?).  */
>   if (*cmd == '\0') {
> - Error("%s expands to empty string", cmd);
> + Parse_Error(PARSE_WARNING, 
> + "'%s' expands to '' while building %s", 
> + pre, job->node->name);
>   return false;
>   }
>  
> @@ -833,7 +835,7 @@ job_run_next(Job *job)
>   job->next_cmd = Lst_Adv(job->next_cmd);
>   if (fatal_errors)
>   Punt(NULL);
> - started = do_run_command(job);
> + started = do_run_command(job, command->string);
>   if (started)
>   return false;
>   else
> 

Re: make: clarify an error string

[Marc Espie]

On Mon, Jul 24, 2017 at 10:43:03AM +0200, Marc Espie wrote:
> On Mon, Jul 24, 2017 at 03:15:32PM +0800, Michael W. Bombardieri wrote:
> > Hi,
> > 
> > In make(1), do_run_command() has a sanity check for a null command string.
> > The error message passes the null string to printf(). Is this any better?
> > 
> > - Michael
> >  

Here's a proper error message:

Index: engine.c
===
RCS file: /cvs/src/usr.bin/make/engine.c,v
retrieving revision 1.53
diff -u -p -r1.53 engine.c
--- engine.c9 Jul 2017 15:28:00 -   1.53
+++ engine.c24 Jul 2017 10:56:38 -
@@ -734,7 +734,7 @@ setup_engine(void)
 }
 
 static bool
-do_run_command(Job *job)
+do_run_command(Job *job, const char *pre)
 {
bool silent;/* Don't print command */
bool doExecute; /* Execute the command */
@@ -752,7 +752,9 @@ do_run_command(Job *job)
/* How can we execute a null command ? we warn the user that the
 * command expanded to nothing (is this the right thing to do?).  */
if (*cmd == '\0') {
-   Error("%s expands to empty string", cmd);
+   Parse_Error(PARSE_WARNING, 
+   "'%s' expands to '' while building %s", 
+   pre, job->node->name);
return false;
}
 
@@ -833,7 +835,7 @@ job_run_next(Job *job)
job->next_cmd = Lst_Adv(job->next_cmd);
if (fatal_errors)
Punt(NULL);
-   started = do_run_command(job);
+   started = do_run_command(job, command->string);
if (started)
return false;
else

sorwakeup() missing KERNEL_LOCK()

[Martin Pieuchot]

divert/divert6 might end up calling sorwakeup() w/o KERNEL_LOCK() since
pf_test() is not always executed with it.  Diff below fixes that and
put two asserts where selwakup() is called in the socket layer.

ok?

Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.196
diff -u -p -r1.196 uipc_socket.c
--- kern/uipc_socket.c  20 Jul 2017 09:49:45 -  1.196
+++ kern/uipc_socket.c  24 Jul 2017 10:33:56 -
@@ -1926,6 +1926,7 @@ sogetopt(struct socket *so, int level, i
 void
 sohasoutofband(struct socket *so)
 {
+   KERNEL_ASSERT_LOCKED();
csignal(so->so_pgid, SIGURG, so->so_siguid, so->so_sigeuid);
selwakeup(>so_rcv.sb_sel);
 }
Index: kern/uipc_socket2.c
===
RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
retrieving revision 1.84
diff -u -p -r1.84 uipc_socket2.c
--- kern/uipc_socket2.c 18 Jul 2017 06:12:09 -  1.84
+++ kern/uipc_socket2.c 24 Jul 2017 10:49:12 -
@@ -382,6 +382,7 @@ sbunlock(struct sockbuf *sb)
 void
 sowakeup(struct socket *so, struct sockbuf *sb)
 {
+   KERNEL_ASSERT_LOCKED();
soassertlocked(so);
 
selwakeup(>sb_sel);
Index: netinet/ip_divert.c
===
RCS file: /cvs/src/sys/netinet/ip_divert.c,v
retrieving revision 1.48
diff -u -p -r1.48 ip_divert.c
--- netinet/ip_divert.c 26 Jun 2017 09:32:32 -  1.48
+++ netinet/ip_divert.c 24 Jul 2017 10:44:39 -
@@ -226,8 +226,11 @@ divert_packet(struct mbuf *m, int dir, u
divstat_inc(divs_fullsock);
m_freem(m);
return (0);
-   } else
+   } else {
+   KERNEL_LOCK();
sorwakeup(inp->inp_socket);
+   KERNEL_UNLOCK();
+   }
}
 
if (sa == NULL) {
Index: netinet6/ip6_divert.c
===
RCS file: /cvs/src/sys/netinet6/ip6_divert.c,v
retrieving revision 1.48
diff -u -p -r1.48 ip6_divert.c
--- netinet6/ip6_divert.c   26 Jun 2017 09:32:32 -  1.48
+++ netinet6/ip6_divert.c   24 Jul 2017 10:44:36 -
@@ -227,8 +227,11 @@ divert6_packet(struct mbuf *m, int dir, 
div6stat_inc(div6s_fullsock);
m_freem(m);
return (0);
-   } else
+   } else {
+   KERNEL_LOCK();
sorwakeup(inp->inp_socket);
+   KERNEL_UNLOCK();
+   }
}
 
if (sa == NULL) {

Re: make netstart a variable

[Ingo Schwarze]

Hi,

Gregory Edigarov wrote on Mon, Jul 24, 2017 at 01:22:55PM +0300:

> personally, I prefer using my short script, over the stock
> /etc/netstart, so why not let everybody use what they prefer?

Hell no.
This is directly contrary to project goals.

KISS.  No useless knobs.

That said, OpenBSD is free software, you can replace any component
you want on your personal computer, even /etc/netstart.
But that certainly isn't supported, and if it ever results in
bogus bug reports, you *will* get yelled at.

Yours,
  Ingo


> diff --git a/etc/rc b/etc/rc
> index 75e63a397e4..f56485a60f5 100644
> --- a/etc/rc
> +++ b/etc/rc
> @@ -447,7 +447,7 @@ echo 'starting network'
>   # Prevents carp from preempting until the system is booted.
>   ifconfig -g carp carpdemote 128
> 
> -sh /etc/netstart
> +sh $netstart
> 
>   # Any write triggers a rekey.
>   dmesg >/dev/random
> diff --git a/etc/rc.conf b/etc/rc.conf
> index 05146d58a4e..340c3de55df 100644
> --- a/etc/rc.conf
> +++ b/etc/rc.conf
> @@ -40,6 +40,7 @@ ldpd_flags=NO
>   lpd_flags=NO   # for normal use: "" (or "-l" for debugging)
>   mopd_flags=NO
>   mrouted_flags=NO   # be sure to enable multicast below
> +netstart=/etc/netstart
>   npppd_flags=NO
>   nsd_flags=NO
>   ntpd_flags=

Re: [patch] Remove binc from vi(1)

[Ingo Schwarze]

Hi Martijn,

On 06/22/17 21:32, Martijn van Duren wrote:

> Attached a patch to remove the binc function from vi
> and replace it with recallocarray.

In principle, the memory handling in vi is very ugly, and getting it
into a more standard form seems desirable - but likely to cause quite
some work.

Coordinating with nvi might also make sense.  It is not obvious to
me whether they would want to adopt recallocarray(3) - which would
no doubt be desirable - or if not, whether we want to maintain yet
another difference.

> The functions effectively do the same thing since BINC_{GOTO,RET}
> already do the nlen > llen comparison. I've run this without any
> issues, but since recallocarray does extra checks and binc ALWAYS
> allocates A LOT more than requested there might be some bugs
> lurking.

Indeed, the existing code is highly inconsistent.

The binc() function increases the buffer size *by* the last argument,
not *to* the last argument.  So the last argument is misnamed: it
is called "min", but it ought to be called "inc".  All the same,
it considers the buffer large enough if the last argument is smaller
than the current buffer size.  That is grossly inconsistent.  You
can only use the function to at least double the buffer size, not
to grow it by, say, 50%.

Even though binc() is never called directly, the BINC_* macro wrappers
repeat the same size check.  That's just duplicate code, always
executed twice in a row.

In some cases, sometimes through additional wrappers, the code
calling BINC_* does things like

  BINC_GOTO(sp, lp, llen, llen + MAXIMUM(total, 64))

SIC: the "total" already includes "llen", yet it gets ADDED to llen,
and then passed as an INCREMENT, which results in at least three
times the required space being allocated.  Savour it: there are
at least three levels of wrappers, and at least two of these layers
treat their argument as a size increment, but get passed the desired
new size!

In other cases, you have code like the following:

  BINC_GOTO(sp, tp->lb, tp->lb_len, tp->len + 1);

Currently, contrary to appearance, that (usually, approximately)
*doubles* the buffer size.  With your patch, it increases the buffer
size by one byte.  In principle, such changes might result in
performance degradation.  You say you are running the patch without
regressions.  You are probably not running it on luna88k, so you
may not see such degradation even if it exists.  Worse, BINC_* is
used at so many different places, often indirectly, that a case
where performance degradation is relevant may simply not occur in
your usage patterns.  And yet worse, the consistently inconsistent
buffer size increasing can quite likely hide instances of buffer
size miscalculations, so your patch may possibly expose such
miscalculations and result in buffer overruns and segfaults.
Possibly in code paths that do not occur in your usage patterns.

Yes, this is a giant, terrible mess.

Reducing the amount of memory allocated in binc to better match
what is really needed may quite possibly result in performance
*improvements* because the functions are used for all kinds of
buffers, even temporary ones and line buffers stored on lists, in
which case wasting memory might possibly add up quite a bit.  But
i fear if we really want to do that, ALL USES need to be carefully
audited, both to make sure that tiny increases repeatedly done in
loops do not degrade performance, and to make sure that no buffer
overruns get exposed.  I'm not convinced such an audit can be
done in one go, and even less that it can be *checked* in one go,
to provide an OK.

So if we really want to do this, we should probably start from
the other end: change the dozens (or hundreds?) of places where
this is used, often indirectly, to directly use recallocarray(3),
one by one, auditing them for performance and security, one by one.
Then, at the very end, remove the layers and layers of then unused
wrappers.

It is not a small task.


In the current form, if feel unable to verify whether the patch
is safe or not.  It may well be, or it might not.

Yours,
  Ingo

Re: connect(2), KERNEL_LOCK() vs socket lock

[Martin Pieuchot]

On 18/07/17(Tue) 15:49, Alexander Bluhm wrote:
> On Tue, Jul 18, 2017 at 09:03:00AM +0200, Martin Pieuchot wrote:
> > Diff below extends the scope of the socket lock.  It has been previously
> > introduced in sys_connect(), first as NET_LOCK() then renamed, where old
> > splsofnet() were used.  But we now need to "move the line up" in order to
> > protect fields currently relying on the KERNEL_LOCK().
> 
> We were also discussing to move the lock down to the network stack.
> Eventually we need locks for the sockets and a lock for the stack.
> The first must move up and the latter down.  As we have only one
> lock at the moment, I am fine with the upward direction for now.
> 
> > Moving the line up means that soconnect() and soconnect2() will now
> > assert for the socket lock.
> 
> The soconnect2() is only used for local sockets that run with the
> kernel lock.  So we could not lock this function.  I guess your
> goal is per socket locks, so your change is fine.

Updated diff addressing your comments.

Index: kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.196
diff -u -p -r1.196 uipc_socket.c
--- kern/uipc_socket.c  20 Jul 2017 09:49:45 -  1.196
+++ kern/uipc_socket.c  24 Jul 2017 09:52:01 -
@@ -311,11 +311,12 @@ soaccept(struct socket *so, struct mbuf 
 int
 soconnect(struct socket *so, struct mbuf *nam)
 {
-   int s, error;
+   int error;
+
+   soassertlocked(so);
 
if (so->so_options & SO_ACCEPTCONN)
return (EOPNOTSUPP);
-   s = solock(so);
/*
 * If protocol is connection-based, can only connect once.
 * Otherwise, if connected, try to disconnect first.
@@ -329,19 +330,17 @@ soconnect(struct socket *so, struct mbuf
else
error = (*so->so_proto->pr_usrreq)(so, PRU_CONNECT,
NULL, nam, NULL, curproc);
-   sounlock(s);
return (error);
 }
 
 int
 soconnect2(struct socket *so1, struct socket *so2)
 {
-   int s, error;
+   int error;
 
-   s = solock(so1);
+   soassertlocked(so1);
error = (*so1->so_proto->pr_usrreq)(so1, PRU_CONNECT2, NULL,
(struct mbuf *)so2, NULL, curproc);
-   sounlock(s);
return (error);
 }
 
Index: kern/uipc_syscalls.c
===
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.155
diff -u -p -r1.155 uipc_syscalls.c
--- kern/uipc_syscalls.c20 Jul 2017 18:40:16 -  1.155
+++ kern/uipc_syscalls.c24 Jul 2017 09:53:13 -
@@ -373,18 +373,19 @@ sys_connect(struct proc *p, void *v, reg
if ((error = getsock(p, SCARG(uap, s), )) != 0)
return (error);
so = fp->f_data;
+   s = solock(so);
if (so->so_state & SS_ISCONNECTING) {
-   FRELE(fp, p);
-   return (EALREADY);
+   error = EALREADY;
+   goto out;
}
error = sockargs(, SCARG(uap, name), SCARG(uap, namelen),
MT_SONAME);
if (error)
-   goto bad;
+   goto out;
error = pledge_socket(p, so->so_proto->pr_domain->dom_family,
so->so_state);
if (error)
-   goto bad;
+   goto out;
 #ifdef KTRACE
if (KTRPOINT(p, KTR_STRUCT))
ktrsockaddr(p, mtod(nam, caddr_t), SCARG(uap, namelen));
@@ -393,11 +394,8 @@ sys_connect(struct proc *p, void *v, reg
if (isdnssocket(so)) {
u_int namelen = nam->m_len;
error = dns_portcheck(p, so, mtod(nam, void *), );
-   if (error) {
-   FRELE(fp, p);
-   m_freem(nam);
-   return (error);
-   }
+   if (error)
+   goto out;
nam->m_len = namelen;
}
 
@@ -405,11 +403,9 @@ sys_connect(struct proc *p, void *v, reg
if (error)
goto bad;
if ((so->so_state & SS_NBIO) && (so->so_state & SS_ISCONNECTING)) {
-   FRELE(fp, p);
-   m_freem(nam);
-   return (EINPROGRESS);
+   error = EINPROGRESS;
+   goto out;
}
-   s = solock(so);
while ((so->so_state & SS_ISCONNECTING) && so->so_error == 0) {
error = sosleep(so, >so_timeo, PSOCK | PCATCH,
"netcon2", 0);
@@ -423,10 +419,11 @@ sys_connect(struct proc *p, void *v, reg
error = so->so_error;
so->so_error = 0;
}
-   sounlock(s);
 bad:
if (!interrupted)
so->so_state &= ~SS_ISCONNECTING;
+out:
+   sounlock(s);
FRELE(fp, p);
m_freem(nam);
if (error == ERESTART)
Index: miscfs/fifofs/fifo_vnops.c
===
RCS file: 

Re: make: clarify an error string

[Marc Espie]

On Mon, Jul 24, 2017 at 03:15:32PM +0800, Michael W. Bombardieri wrote:
> Hi,
> 
> In make(1), do_run_command() has a sanity check for a null command string.
> The error message passes the null string to printf(). Is this any better?
> 
> - Michael
>  
> 
> Index: engine.c
> ===
> RCS file: /cvs/src/usr.bin/make/engine.c,v
> retrieving revision 1.53
> diff -u -p -u -r1.53 engine.c
> --- engine.c  9 Jul 2017 15:28:00 -   1.53
> +++ engine.c  24 Jul 2017 06:58:31 -
> @@ -752,7 +752,7 @@ do_run_command(Job *job)
>   /* How can we execute a null command ? we warn the user that the
>* command expanded to nothing (is this the right thing to do?).  */
>   if (*cmd == '\0') {
> - Error("%s expands to empty string", cmd);
> + Error("command expands to empty string");
>   return false;
>   }
>  
Good catch. I'll do something better, since I know where the command
comes from, so I can display a proper error message with line number,
and (more importantly) the string before substitution.

make: clarify an error string

[Michael W. Bombardieri]

Hi,

In make(1), do_run_command() has a sanity check for a null command string.
The error message passes the null string to printf(). Is this any better?

- Michael
 

Index: engine.c
===
RCS file: /cvs/src/usr.bin/make/engine.c,v
retrieving revision 1.53
diff -u -p -u -r1.53 engine.c
--- engine.c9 Jul 2017 15:28:00 -   1.53
+++ engine.c24 Jul 2017 06:58:31 -
@@ -752,7 +752,7 @@ do_run_command(Job *job)
/* How can we execute a null command ? we warn the user that the
 * command expanded to nothing (is this the right thing to do?).  */
if (*cmd == '\0') {
-   Error("%s expands to empty string", cmd);
+   Error("command expands to empty string");
return false;
}
 

efiboot: improve cd9660 support

[FUKAUMI Naoki]

Hi tech@,

With this patch,

- 'cd0' is used for the name of the disk for CD-ROM
- 'cd0a' is used for the 'device' variable when it's booted from CD-ROM


probing: pc0 com0 com1 mem[640K 3049M 16M 4M 64K 1024M]
disk: hd0* cd0
>> OpenBSD/amd64 BOOTX64 3.33
boot> set
>> OpenBSD/amd64 BOOTX64 3.33
addr 0x0
howto
device   cd0a
tty  pc0
image/bsd.rd
timeout  0
db_console   unset
boot> machine diskinfo
DiskBlkSiz  IoAlign SizeFlags   Checksum
hd0 512 0   32GB0x0 0x0 
cd0 20480   13MB0xa 0x0 Removable


# should efiboot version be bumped?

Index: sys/arch/amd64/stand/efiboot/efiboot.c
===
RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efiboot.c,v
retrieving revision 1.20
diff -u -p -r1.20 efiboot.c
--- sys/arch/amd64/stand/efiboot/efiboot.c  1 Jun 2017 11:32:15 -   
1.20
+++ sys/arch/amd64/stand/efiboot/efiboot.c  21 Jul 2017 05:57:44 -
@@ -91,9 +91,14 @@ efi_main(EFI_HANDLE image, EFI_SYSTEM_TA
if (status == EFI_SUCCESS) {
for (dp = dp0; !IsDevicePathEnd(dp);
dp = NextDevicePathNode(dp)) {
-   if (DevicePathType(dp) == MEDIA_DEVICE_PATH &&
-   DevicePathSubType(dp) == MEDIA_HARDDRIVE_DP) {
+   if (DevicePathType(dp) != MEDIA_DEVICE_PATH)
+   continue;
+   if (DevicePathSubType(dp) == MEDIA_HARDDRIVE_DP) {
bios_bootdev = 0x80;
+   efi_bootdp = dp0;
+   break;
+   } else if (DevicePathSubType(dp) == MEDIA_CDROM_DP) {
+   bios_bootdev = 0x100;
efi_bootdp = dp0;
break;
}
Index: sys/arch/amd64/stand/efiboot/efidev.c
===
RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efidev.c,v
retrieving revision 1.27
diff -u -p -r1.27 efidev.c
--- sys/arch/amd64/stand/efiboot/efidev.c   21 Jul 2017 01:21:42 -  
1.27
+++ sys/arch/amd64/stand/efiboot/efidev.c   21 Jul 2017 05:57:44 -
@@ -787,7 +787,8 @@ efi_dump_diskinfo(void)
sizu = "GB";
}
 
-   printf("hd%d\t%u\t%u\t%u%s\t0x%x\t0x%x\t%s\n",
+   printf("%cd%d\t%u\t%u\t%u%s\t0x%x\t0x%x\t%s\n",
+   (bdi->flags & BDI_EL_TORITO) ? 'c' : 'h',
(bdi->bios_number & 0x7f),
ed->blkio->Media->BlockSize,
ed->blkio->Media->IoAlign, (unsigned)siz, sizu,
Index: sys/arch/amd64/stand/libsa/diskprobe.c
===
RCS file: /cvs/src/sys/arch/amd64/stand/libsa/diskprobe.c,v
retrieving revision 1.20
diff -u -p -r1.20 diskprobe.c
--- sys/arch/amd64/stand/libsa/diskprobe.c  21 Jul 2017 01:21:42 -  
1.20
+++ sys/arch/amd64/stand/libsa/diskprobe.c  21 Jul 2017 05:57:44 -
@@ -186,22 +186,24 @@ hardprobe(void)
 static void
 efi_hardprobe(void)
 {
-   int  n;
struct diskinfo *dip, *dipt;
-   u_intbsdunit, type = 0;
-   u_intscsi= 0, ide = 0;
+   u_intbsdunit, type;
+   u_intscsi = 0, ide = 0, atapi = 0;
+   u_intn;
+   char c;
extern struct disklist_lh
 efi_disklist;
 
-   n = 0;
TAILQ_FOREACH_SAFE(dip, _disklist, list, dipt) {
TAILQ_REMOVE(_disklist, dip, list);
-   printf(" hd%u", n);
 
+   c = 'h';
+   n = scsi + ide;
dip->bios_info.bios_number = 0x80 | n;
/* Try to find the label, to figure out device type */
if ((efi_getdisklabel(dip->efi_info, >disklabel))) {
-   printf("*");
+   printf(" %cd%u*", c, n);
+   type = 0;   /* XXX let it be IDE */
bsdunit = ide++;
} else {
/* Best guess */
@@ -219,11 +221,23 @@ efi_hardprobe(void)
dip->bios_info.flags |= BDI_GOODLABEL;
break;
 
+   case DTYPE_ATAPI:
+   c = 'c';
+   n = atapi;
+   dip->bios_info.bios_number = n;
+   type = 6;
+   bsdunit = atapi++;
+   dip->bios_info.flags |=
+   BDI_GOODLABEL | BDI_EL_TORITO;
+   break;
+
default:
dip->bios_info.flags |= BDI_BADLABEL;
 

makefs(8): add support for UEFI

[FUKAUMI Naoki]

Hi tech@,

This adds support for UEFI.

>From NetBSD 
>http://mail-index.netbsd.org/source-changes/2017/01/24/msg081292.html


you can make BIOS/UEFI dual boot ISO image like this,

 mkdir -p efiboot/EFI/BOOT
 cp /usr/mdec/BOOT*.EFI efiboot/EFI/BOOT/
 makefs -M 1m -t msdos efiboot.img efiboot
 
 mkdir -p cd-dir/etc
 echo 'set image /bsd.rd' > cd-dir/etc/boot.conf
 cp /bsd.rd cd-dir/
 makefs -t cd9660 -o 
'rockridge,bootimage=i386;/usr/mdec/cdboot,no-emul-boot,allow-multidot,bootimage=i386;efiboot.img,platformid=efi,no-emul-boot'
 cd.iso cd-dir

Index: usr.sbin/makefs/cd9660.c
===
RCS file: /cvs/src/usr.sbin/makefs/cd9660.c,v
retrieving revision 1.20
diff -u -p -r1.20 cd9660.c
--- usr.sbin/makefs/cd9660.c6 Apr 2017 19:09:45 -   1.20
+++ usr.sbin/makefs/cd9660.c21 Jul 2017 05:57:45 -
@@ -1,5 +1,5 @@
 /* $OpenBSD: cd9660.c,v 1.20 2017/04/06 19:09:45 natano Exp $  */
-/* $NetBSD: cd9660.c,v 1.53 2016/11/25 23:02:44 christos Exp $ */
+/* $NetBSD: cd9660.c,v 1.54 2017/01/24 11:22:43 nonaka Exp $   */
 
 /*
  * Copyright (c) 2005 Daniel Watt, Walter Deignan, Ryan Gabrys, Alan
@@ -265,6 +265,7 @@ cd9660_prep_opts(fsinfo_t *fsopts)
OPT_STR("no-emul-boot"),
OPT_BOOL("no-trailing-padding", include_padding_areas),
OPT_BOOL("omit-trailing-period", omit_trailing_period),
+   OPT_STR("platformid"),
OPT_STR("preparer"),
OPT_STR("publisher"),
OPT_BOOL("rockridge", rock_ridge_enabled),
@@ -341,7 +342,8 @@ cd9660_parse_opts(const char *option, fs
if (strcmp(name, "applicationid") == 0) {
rv = cd9660_arguments_set_string(buf, name, 128, 'a',
diskStructure->primaryDescriptor.application_id);
-   } else if (strcmp(name, "boot-load-segment") == 0) {
+   } else if (strcmp(name, "boot-load-segment") == 0 ||
+   strcmp(name, "platformid") == 0) {
if (buf[0] == '\0') {
warnx("Option `%s' doesn't contain a value",
name);
Index: usr.sbin/makefs/makefs.8
===
RCS file: /cvs/src/usr.sbin/makefs/makefs.8,v
retrieving revision 1.18
diff -u -p -r1.18 makefs.8
--- usr.sbin/makefs/makefs.813 Nov 2016 10:22:21 -  1.18
+++ usr.sbin/makefs/makefs.821 Jul 2017 05:57:45 -
@@ -235,6 +235,8 @@ ElTorito image.
 Do not pad the image (apparently Linux needs the padding).
 .It Sy omit-trailing-period
 Omit trailing periods in filenames.
+.It Sy platformid
+Set platform ID of section header entry of the boot image.
 .It Sy preparer
 Preparer ID of the image.
 .It Sy publisher
Index: usr.sbin/makefs/cd9660/cd9660_eltorito.c
===
RCS file: /cvs/src/usr.sbin/makefs/cd9660/cd9660_eltorito.c,v
retrieving revision 1.9
diff -u -p -r1.9 cd9660_eltorito.c
--- usr.sbin/makefs/cd9660/cd9660_eltorito.c17 Dec 2016 16:22:04 -  
1.9
+++ usr.sbin/makefs/cd9660/cd9660_eltorito.c21 Jul 2017 05:57:45 -
@@ -1,5 +1,5 @@
 /* $OpenBSD: cd9660_eltorito.c,v 1.9 2016/12/17 16:22:04 krw Exp $ */
-/* $NetBSD: cd9660_eltorito.c,v 1.20 2013/01/28 21:03:28 christos Exp $
*/
+/* $NetBSD: cd9660_eltorito.c,v 1.21 2017/01/24 11:22:43 nonaka Exp $  
*/
 
 /*
  * Copyright (c) 2005 Daniel Watt, Walter Deignan, Ryan Gabrys, Alan
@@ -57,11 +57,12 @@ static struct boot_catalog_entry *cd9660
 static struct boot_catalog_entry *cd9660_boot_setup_default_entry(
 struct cd9660_boot_image *);
 static struct boot_catalog_entry *cd9660_boot_setup_section_head(char);
-static struct boot_catalog_entry *cd9660_boot_setup_validation_entry(char);
 #if 0
 static u_char cd9660_boot_get_system_type(struct cd9660_boot_image *);
 #endif
 
+static struct cd9660_boot_image *default_boot_image;
+
 int
 cd9660_add_boot_disk(iso9660_disk *diskStructure, const char *boot_info)
 {
@@ -163,9 +164,15 @@ cd9660_add_boot_disk(iso9660_disk *diskS
 
new_image->serialno = diskStructure->image_serialno++;
 
+   new_image->platform_id = new_image->system;
+
/* TODO : Need to do anything about the boot image in the tree? */
diskStructure->is_bootable = 1;
 
+   /* First boot image is initial/default entry. */
+   if (default_boot_image == NULL)
+   default_boot_image = new_image;
+
return 1;
 }
 
@@ -199,6 +206,13 @@ cd9660_eltorito_add_boot_option(iso9660_
warn("%s: strtoul", __func__);
return 0;
}
+   } else if (strcmp(option_string, "platformid") == 0) {
+   if (strcmp(value, "efi") == 0)
+   image->platform_id = ET_SYS_EFI;
+   else {
+   warn("%s: unknown platform: %s", __func__, value);
+