Re: ifconfig: add -rdomain option

[Ayaka Koshibe]

On Thu, Feb 22, 2018 at 4:54 AM, David Gwynne  wrote:
>
>
>> On 22 Feb 2018, at 5:00 pm, Ayaka Koshibe  wrote:
>>
>> On Wed, Feb 21, 2018 at 03:42:58PM +0100, Sebastian Benoit wrote:
>>> Ayaka Koshibe(akosh...@openbsd.org) on 2018.02.20 21:20:20 -0800:
 On Tue, Feb 20, 2018 at 4:48 AM, Reyk Floeter  wrote:
>
>> Am 20.02.2018 um 11:15 schrieb Klemens Nanni :
>>
>>> On Mon, Feb 19, 2018 at 05:09:58PM -0800, Ayaka Koshibe wrote:
>>> This diff would allow saying 'ifconfig foo -rdomain' instead of 
>>> 'ifconfig foo rdomain 0'.
>> I can see where you're coming from but this breaks semantics: `-option'
>> clears an optional parameter or deconfigures functionality whereas
>> `rdomain' is mandatory (defaulting to 0), every interface is attached
>> to exactly one routing domain all the time.
>>
>
> I would rather say that -option resets it to the default non-specific 
> option. For example, -mode doesn???t remove all wireless modes.

 This was also my interpretation of -option -- a way to reset to the
 default, which happens to be a value of 0 for this case.

> I think -rdomain is a good addition and the diff looks fine.
>
> Reyk
>>>
>>> i'm ok with the diff as well, but i want to remind you that rdomain is
>>> special, because it also removes all ip configuration from the interface.
>>> I.e. -rdomain does more than the other -options.
>>
>> Ah, right. I hope I can just update the man page to explicitly mention
>> this. I've also made it a bit more concise as jmc had suggested.
>
> you should probably do a similar change for tunneldomain, just to be 
> consistent.

Ok, will take a look for a separate diff. I realized after the fact
that there's no mention of this for the rdomain option, either.

> cheers,
> dlg
>
>>
>>
>> Index: sbin/ifconfig/ifconfig.8
>> ===
>> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
>> retrieving revision 1.303
>> diff -u -p -u -p -r1.303 ifconfig.8
>> --- sbin/ifconfig/ifconfig.8  20 Feb 2018 07:34:28 -  1.303
>> +++ sbin/ifconfig/ifconfig.8  22 Feb 2018 06:06:00 -
>> @@ -439,6 +439,10 @@ domains.
>> If the specified rdomain does not yet exist it will be created, including
>> a routing table with the same id.
>> By default all interfaces belong to routing domain 0.
>> +.It Cm -rdomain
>> +Remove the interface from the routing domain and return it to routing
>> +domain 0.
>> +Any inet and inet6 addresses on the interface will also be removed.
>> .It Cm rtlabel Ar route-label
>> (inet)
>> Attach
>> Index: sbin/ifconfig/ifconfig.c
>> ===
>> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
>> retrieving revision 1.360
>> diff -u -p -u -p -r1.360 ifconfig.c
>> --- sbin/ifconfig/ifconfig.c  20 Feb 2018 15:33:16 -  1.360
>> +++ sbin/ifconfig/ifconfig.c  22 Feb 2018 06:06:01 -
>> @@ -237,6 +237,7 @@ void  unsetvlandev(const char *, int);
>> void  mpe_status(void);
>> void  mpw_status(void);
>> void  setrdomain(const char *, int);
>> +void unsetrdomain(const char *, int);
>> int   prefix(void *val, int);
>> void  getifgroups(void);
>> void  setifgroup(const char *, int);
>> @@ -421,6 +422,7 @@ const struct  cmd {
>>   { "rtlabel",NEXTARG,0,  setifrtlabel },
>>   { "-rtlabel",   -1, 0,  setifrtlabel },
>>   { "rdomain",NEXTARG,0,  setrdomain },
>> + { "-rdomain",   0,  0,  unsetrdomain },
>>   { "staticarp",  IFF_STATICARP,  0,  setifflags },
>>   { "-staticarp", -IFF_STATICARP, 0,  setifflags },
>>   { "mpls",   IFXF_MPLS,  0,  setifxflags },
>> @@ -5665,6 +5667,15 @@ setrdomain(const char *id, int param)
>>   strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
>>   ifr.ifr_rdomainid = rdomainid;
>>   if (ioctl(s, SIOCSIFRDOMAIN, (caddr_t)) < 0)
>> + warn("SIOCSIFRDOMAIN");
>> +}
>> +
>> +void
>> +unsetrdomain(const char *ignored, int alsoignored)
>> +{
>> + strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
>> + ifr.ifr_rdomainid = 0;
>> + if (ioctl(s, SIOCSIFRDOMAIN, (caddr_t)) < 0)
>>   warn("SIOCSIFRDOMAIN");
>> }
>> #endif
>>
>

Re: pf generic packet delay

[Henning Brauer]

* Martin Pieuchot  [2018-02-21 09:37]:
> On 21/02/18(Wed) 02:37, Henning Brauer wrote:
> I'd suggest moving the pool allocation and the function in net/pf*.c
> and only have a function call under #if NPF > 0.

worth discussing, but imo that part doesn't really have all that much
to do with pf.

> I'd suggest defining your own structure containing a timeout and a mbuf
> pointer instead of abusing ph_cookie.  Since you're already allocating
> something it doesn't matter much and you're code won't be broken by
> a future refactoring.

dlg pointed me to ph_cookie, I was about to use my own structure.
"ph_cookie is for exactly that"

> You're leaking a mbuf if the interface has been detached.

hah! true. fixed locally (the obvious: else m_freem(m);)

-- 
Henning Brauer, h...@bsws.de, henn...@openbsd.org
BS Web Services GmbH, http://bsws.de, Full-Service ISP
Secure Hosting, Mail and DNS. Virtual & Dedicated Servers, Root to Fully Managed
Henning Brauer Consulting, http://henningbrauer.com/

Re: armv7 really isn't a strict-alignment architecture

[Brandon Bergren]

No dice. Still seeing lockup / watchdog reset at some point before consinit(). 
I guess I'll try messing with the CCR from the bootloader instead to see what 
state it's in before transfer-of-control. I still suspect an alignment issue, 
as it happens reliably on the kernel images that it happens on, and never 
happens to the other kernel images.

On Thu, Feb 22, 2018, at 4:52 PM, Brandon Bergren wrote:
> FWIW I've been chasing a failure on beaglebone black where a random 
> subset of relinked kernels fail to get as far as console init and am 
> pretty flummoxed about it.
> 
> Turning off the strictness might fix whatever issue that was, and save 
> me the pain of soldering on a JTAG connector and learning OpenOCD just 
> because I don't have access to printf() early enough to debug it without 
> doing things the hard way. I will be testing this patch with interest.
> 
> An alternative to having the processor sweep unaligned accesses under 
> the rug would be to change the trap handler to do actual alignment 
> fixups, complaining about having to do alignment fixups, and then 
> continuing, instead of treating it in a fatal manner, I suppose.
> 
> Just my 2c.
> 
> On Thu, Feb 22, 2018, at 3:51 PM, Mark Kettenis wrote:
> > I hate to loose yet another strict-alignment canary, but the reality
> > is that the rest of the world assumes that armv7 supports unaligned
> > access which means that compilers generate code that assumes this
> > works when compiling code for armv7 and later (e.g. NEON code) and
> > that hand-written assembler code assumes this works as well.  I hit
> > yet another case of this while playing around with softfp.
> > 
> > The diff below stops the kernel from generating alignment faults as a
> > first step.
> > 
> > ok?
> > 
> > 
> > Index: arch/arm/arm/cpufunc.c
> > ===
> > RCS file: /cvs/src/sys/arch/arm/arm/cpufunc.c,v
> > retrieving revision 1.52
> > diff -u -p -r1.52 cpufunc.c
> > --- arch/arm/arm/cpufunc.c  8 Sep 2017 05:36:51 -   1.52
> > +++ arch/arm/arm/cpufunc.c  22 Feb 2018 21:42:35 -
> > @@ -395,7 +395,6 @@ armv7_setup()
> > | CPU_CONTROL_AFE;
> >  
> > cpuctrl = CPU_CONTROL_MMU_ENABLE
> > -   | CPU_CONTROL_AFLT_ENABLE
> > | CPU_CONTROL_DC_ENABLE
> > | CPU_CONTROL_BPRD_ENABLE
> > | CPU_CONTROL_IC_ENABLE
> > 
> 
> 
> -- 
>   Brandon Bergren
>   Technical Generalist
> 


-- 
  Brandon Bergren
  Technical Generalist

Re: armv7 really isn't a strict-alignment architecture

[Brandon Bergren]

FWIW I've been chasing a failure on beaglebone black where a random subset of 
relinked kernels fail to get as far as console init and am pretty flummoxed 
about it.

Turning off the strictness might fix whatever issue that was, and save me the 
pain of soldering on a JTAG connector and learning OpenOCD just because I don't 
have access to printf() early enough to debug it without doing things the hard 
way. I will be testing this patch with interest.

An alternative to having the processor sweep unaligned accesses under the rug 
would be to change the trap handler to do actual alignment fixups, complaining 
about having to do alignment fixups, and then continuing, instead of treating 
it in a fatal manner, I suppose.

Just my 2c.

On Thu, Feb 22, 2018, at 3:51 PM, Mark Kettenis wrote:
> I hate to loose yet another strict-alignment canary, but the reality
> is that the rest of the world assumes that armv7 supports unaligned
> access which means that compilers generate code that assumes this
> works when compiling code for armv7 and later (e.g. NEON code) and
> that hand-written assembler code assumes this works as well.  I hit
> yet another case of this while playing around with softfp.
> 
> The diff below stops the kernel from generating alignment faults as a
> first step.
> 
> ok?
> 
> 
> Index: arch/arm/arm/cpufunc.c
> ===
> RCS file: /cvs/src/sys/arch/arm/arm/cpufunc.c,v
> retrieving revision 1.52
> diff -u -p -r1.52 cpufunc.c
> --- arch/arm/arm/cpufunc.c8 Sep 2017 05:36:51 -   1.52
> +++ arch/arm/arm/cpufunc.c22 Feb 2018 21:42:35 -
> @@ -395,7 +395,6 @@ armv7_setup()
>   | CPU_CONTROL_AFE;
>  
>   cpuctrl = CPU_CONTROL_MMU_ENABLE
> - | CPU_CONTROL_AFLT_ENABLE
>   | CPU_CONTROL_DC_ENABLE
>   | CPU_CONTROL_BPRD_ENABLE
>   | CPU_CONTROL_IC_ENABLE
> 


-- 
  Brandon Bergren
  Technical Generalist

armv7 really isn't a strict-alignment architecture

[Mark Kettenis]

I hate to loose yet another strict-alignment canary, but the reality
is that the rest of the world assumes that armv7 supports unaligned
access which means that compilers generate code that assumes this
works when compiling code for armv7 and later (e.g. NEON code) and
that hand-written assembler code assumes this works as well.  I hit
yet another case of this while playing around with softfp.

The diff below stops the kernel from generating alignment faults as a
first step.

ok?


Index: arch/arm/arm/cpufunc.c
===
RCS file: /cvs/src/sys/arch/arm/arm/cpufunc.c,v
retrieving revision 1.52
diff -u -p -r1.52 cpufunc.c
--- arch/arm/arm/cpufunc.c  8 Sep 2017 05:36:51 -   1.52
+++ arch/arm/arm/cpufunc.c  22 Feb 2018 21:42:35 -
@@ -395,7 +395,6 @@ armv7_setup()
| CPU_CONTROL_AFE;
 
cpuctrl = CPU_CONTROL_MMU_ENABLE
-   | CPU_CONTROL_AFLT_ENABLE
| CPU_CONTROL_DC_ENABLE
| CPU_CONTROL_BPRD_ENABLE
| CPU_CONTROL_IC_ENABLE

Re: shutdown: fork+exec for wall(1) broadcasts

[Scott Cheloha]

On Thu, Feb 22, 2018 at 02:13:16PM -0700, Todd C. Miller wrote:
> On Thu, 22 Feb 2018 15:06:04 -0600, Scott Cheloha wrote:
> 
> > Could that difference effect the behavior of the program in practice?
> 
> I don't think so.
> 
> > [...]
> >
> > So unless you or someone else has concerns about breakage I'll stick
> > with dprintf.
> 
> That's fine with me.

Cool.

> > > The only thing that concerns me is the possibility of closing the
> > > wrong fd if stdin is not actually open (unlikely).  I prefer an
> > > idiom like the following:
> > > 
> > >   if (dup2(fd[0], STDIN_FILENO) == -1) {
> > >   syslog(LOG_ERR, "dup2: %m");
> > >   _exit(1);
> > >   }
> > >   if (fd[0] != STDIN_FILENO)
> > >   close(fd[0]);
> > >   ...
> >
> > [...]
> >
> > How would stdin not be open in this case?  Or is it a more general
> > good-to-do-just-in-case when you're piping to stdin?
> 
> There's usually no guarantee that stdin, stdout and stderr are
> actually open when you execute a program.  The caller could have
> closed them.  On OpenBSD, when running a setuid program, the kernel
> will open fds 0-2 if not already open (directing them to /dev/null).
> 
> Since shutdown is setuid, the kernel will do the right thing but I
> still think it is worth using the safer idiom.

Alright, that makes sense, thanks for the explanation.

--

Up-to-date diff attached.  Concerns or oks from anyone else?

--
Scott Cheloha

Index: sbin/shutdown/shutdown.c
===
RCS file: /cvs/src/sbin/shutdown/shutdown.c,v
retrieving revision 1.47
diff -u -p -r1.47 shutdown.c
--- sbin/shutdown/shutdown.c4 Feb 2018 04:28:41 -   1.47
+++ sbin/shutdown/shutdown.c22 Feb 2018 21:23:48 -
@@ -40,7 +40,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -86,7 +85,7 @@ struct interval {
 
 static time_t offset, shuttime;
 static int dofast, dohalt, doreboot, dopower, dodump, mbuflen, nosync;
-static sig_atomic_t killflg;
+static sig_atomic_t killflg, timed_out;
 static char *whom, mbuf[BUFSIZ];
 
 void badtime(void);
@@ -268,8 +267,6 @@ loop(void)
die_you_gravy_sucking_pig_dog();
 }
 
-static jmp_buf alarmbuf;
-
 static char *restricted_environ[] = {
"PATH=" _PATH_STDPATH,
NULL
@@ -279,59 +276,84 @@ void
 timewarn(int timeleft)
 {
static char hostname[HOST_NAME_MAX+1];
-   char wcmd[PATH_MAX + 4];
-   extern char **environ;
static int first;
-   FILE *pf;
+   int fd[2];
+   pid_t pid, wpid;
 
if (!first++)
(void)gethostname(hostname, sizeof(hostname));
 
-   /* undoc -n option to wall suppresses normal wall banner */
-   (void)snprintf(wcmd, sizeof(wcmd), "%s -n", _PATH_WALL);
-   environ = restricted_environ;
-   if (!(pf = popen(wcmd, "w"))) {
-   syslog(LOG_ERR, "shutdown: can't find %s: %m", _PATH_WALL);
+   if (pipe(fd) == -1) {
+   syslog(LOG_ERR, "pipe: %m");
+   return;
+   }
+   switch (pid = fork()) {
+   case -1:
+   syslog(LOG_ERR, "fork: %m");
+   close(fd[0]);
+   close(fd[1]);
return;
+   case 0:
+   if (dup2(fd[0], STDIN_FILENO) == -1) {
+   syslog(LOG_ERR, "dup2: %m");
+   _exit(1);
+   }
+   if (fd[0] != STDIN_FILENO)
+   close(fd[0]);
+   close(fd[1]);
+   /* wall(1)'s undocumented '-n' flag suppresses its banner. */
+   execle(_PATH_WALL, _PATH_WALL, "-n", (char *)NULL,
+   restricted_environ);
+   syslog(LOG_ERR, "%s: %m", _PATH_WALL);
+   _exit(1);
+   default:
+   close(fd[0]);
}
 
-   (void)fprintf(pf,
+   dprintf(fd[1],
"\007*** %sSystem shutdown message from %s@%s ***\007\n",
timeleft ? "": "FINAL ", whom, hostname);
 
if (timeleft > 10*60) {
struct tm *tm = localtime();
 
-   fprintf(pf, "System going down at %d:%02d\n\n",
+   dprintf(fd[1], "System going down at %d:%02d\n\n",
tm->tm_hour, tm->tm_min);
} else if (timeleft > 59)
-   (void)fprintf(pf, "System going down in %d minute%s\n\n",
+   dprintf(fd[1], "System going down in %d minute%s\n\n",
timeleft / 60, (timeleft > 60) ? "s" : "");
else if (timeleft)
-   (void)fprintf(pf, "System going down in 30 seconds\n\n");
+   dprintf(fd[1], "System going down in 30 seconds\n\n");
else
-   (void)fprintf(pf, "System going down IMMEDIATELY\n\n");
+   dprintf(fd[1], "System going down IMMEDIATELY\n\n");
 
if (mbuflen)
-   (void)fwrite(mbuf, sizeof(*mbuf), mbuflen, pf);
+   (void)write(fd[1], mbuf, mbuflen);
+   

Re: shutdown: fork+exec for wall(1) broadcasts

[Todd C. Miller]

On Thu, 22 Feb 2018 15:06:04 -0600, Scott Cheloha wrote:

> Could that difference effect the behavior of the program in practice?

I don't think so.

> Attached diff is using the file stream functions still, for comparison.
>
> But the dprintf diff feels more natural; keeping the stream functions
> means mucking with fdopen and the introduction of a label to handle the
> failure case, or a new if clause, which makes the diff even larger.
>
> So unless you or someone else has concerns about breakage I'll stick
> with dprintf.

That's fine with me.

> > The only thing that concerns me is the possibility of closing the
> > wrong fd if stdin is not actually open (unlikely).  I prefer an
> > idiom like the following:
> > 
> > if (dup2(fd[0], STDIN_FILENO) == -1) {
> > syslog(LOG_ERR, "dup2: %m");
> > _exit(1);
> > }
> > if (fd[0] != STDIN_FILENO)
> > close(fd[0]);
> > ...
>
> Ah, I was wondering why other programs around the tree did that.
> The attached diff now does this, I believe.
>
> How would stdin not be open in this case?  Or is it a more general
> good-to-do-just-in-case when you're piping to stdin?

There's usually no guarantee that stdin, stdout and stderr are
actually open when you execute a program.  The caller could have
closed them.  On OpenBSD, when running a setuid program, the kernel
will open fds 0-2 if not already open (directing them to /dev/null).

Since shutdown is setuid, the kernel will do the right thing but I
still think it is worth using the safer idiom.

 - todd

Re: shutdown: fork+exec for wall(1) broadcasts

[Scott Cheloha]

On Thu, Feb 22, 2018 at 01:09:02PM -0700, Todd C. Miller wrote:
> On Thu, 22 Feb 2018 13:50:13 -0600, Scott Cheloha wrote:
> 
> > I think setjmping from a signal handler to put a time limit on
> > pclose is too magical, especially when the alternative doesn't
> > require much more code.
> 
> Agreed.
> 
> > [...]
> >
> >   * fprintf -> dprintf, fwrite -> write, and
> >
> > [...]
> 
> You could use fdopen() and keep using stdio but I suppose it doesn't
> really matter--dprintf() will effectively get you line buffering.

Could that difference effect the behavior of the program in practice?

Attached diff is using the file stream functions still, for comparison.

But the dprintf diff feels more natural; keeping the stream functions
means mucking with fdopen and the introduction of a label to handle the
failure case, or a new if clause, which makes the diff even larger.

So unless you or someone else has concerns about breakage I'll stick
with dprintf.

> The only thing that concerns me is the possibility of closing the
> wrong fd if stdin is not actually open (unlikely).  I prefer an
> idiom like the following:
> 
>   if (dup2(fd[0], STDIN_FILENO) == -1) {
>   syslog(LOG_ERR, "dup2: %m");
>   _exit(1);
>   }
>   if (fd[0] != STDIN_FILENO)
>   close(fd[0]);
>   ...

Ah, I was wondering why other programs around the tree did that.
The attached diff now does this, I believe.

How would stdin not be open in this case?  Or is it a more general
good-to-do-just-in-case when you're piping to stdin?

--
Scott Cheloha

Index: sbin/shutdown/shutdown.c
===
RCS file: /cvs/src/sbin/shutdown/shutdown.c,v
retrieving revision 1.47
diff -u -p -r1.47 shutdown.c
--- sbin/shutdown/shutdown.c4 Feb 2018 04:28:41 -   1.47
+++ sbin/shutdown/shutdown.c22 Feb 2018 20:39:03 -
@@ -40,7 +40,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -86,7 +85,7 @@ struct interval {
 
 static time_t offset, shuttime;
 static int dofast, dohalt, doreboot, dopower, dodump, mbuflen, nosync;
-static sig_atomic_t killflg;
+static sig_atomic_t killflg, timed_out;
 static char *whom, mbuf[BUFSIZ];
 
 void badtime(void);
@@ -268,8 +267,6 @@ loop(void)
die_you_gravy_sucking_pig_dog();
 }
 
-static jmp_buf alarmbuf;
-
 static char *restricted_environ[] = {
"PATH=" _PATH_STDPATH,
NULL
@@ -279,20 +276,44 @@ void
 timewarn(int timeleft)
 {
static char hostname[HOST_NAME_MAX+1];
-   char wcmd[PATH_MAX + 4];
-   extern char **environ;
static int first;
FILE *pf;
+   int fd[2];
+   pid_t pid, wpid;
 
if (!first++)
(void)gethostname(hostname, sizeof(hostname));
 
-   /* undoc -n option to wall suppresses normal wall banner */
-   (void)snprintf(wcmd, sizeof(wcmd), "%s -n", _PATH_WALL);
-   environ = restricted_environ;
-   if (!(pf = popen(wcmd, "w"))) {
-   syslog(LOG_ERR, "shutdown: can't find %s: %m", _PATH_WALL);
+   if (pipe(fd) == -1) {
+   syslog(LOG_ERR, "pipe: %m");
+   return;
+   }
+   switch (pid = fork()) {
+   case -1:
+   syslog(LOG_ERR, "fork: %m");
+   close(fd[0]);
+   close(fd[1]);
return;
+   case 0:
+   if (dup2(fd[0], STDIN_FILENO) == -1) {
+   syslog(LOG_ERR, "dup2: %m");
+   _exit(1);
+   }
+   if (fd[0] != STDIN_FILENO)
+   close(fd[0]);
+   close(fd[1]);
+   /* wall(1)'s undocumented '-n' flag suppresses its banner. */
+   execle(_PATH_WALL, _PATH_WALL, "-n", (char *)NULL,
+   restricted_environ);
+   syslog(LOG_ERR, "%s: %m", _PATH_WALL);
+   _exit(1);
+   default:
+   close(fd[0]);
+   }
+   if ((pf = fdopen(fd[1], "w")) == NULL) {
+   syslog(LOG_ERR, "fdopen: %m");
+   close(fd[1]);
+   goto wait;
}
 
(void)fprintf(pf,
@@ -314,24 +335,31 @@ timewarn(int timeleft)
 
if (mbuflen)
(void)fwrite(mbuf, sizeof(*mbuf), mbuflen, pf);
-
+   fclose(pf);
+wait:
/*
-* play some games, just in case wall doesn't come back
-* probably unnecessary, given that wall is careful.
+* If we wait longer than 30 seconds for wall(1) to exit we'll
+* throw off our schedule.
 */
-   if (!setjmp(alarmbuf)) {
-   (void)signal(SIGALRM, timeout);
-   (void)alarm((u_int)30);
-   (void)pclose(pf);
-   (void)alarm((u_int)0);
-   (void)signal(SIGALRM, SIG_DFL);
+   signal(SIGALRM, timeout);
+   siginterrupt(SIGALRM, 1);
+   alarm(30);
+   while ((wpid = wait(NULL)) != pid && !timed_out)
+  

Re: shutdown: fork+exec for wall(1) broadcasts

[Todd C. Miller]

On Thu, 22 Feb 2018 13:50:13 -0600, Scott Cheloha wrote:

> I think setjmping from a signal handler to put a time limit on
> pclose is too magical, especially when the alternative doesn't
> require much more code.

Agreed.

> But I do think putting a time limit on our wait for wall to do its
> job is a reasonable precaution, though, so:
>
>   * Replace popen with pipe/fork/execle,
>
>   * fprintf -> dprintf, fwrite -> write, and
>
>   * Strip SA_RESTART from SIGALRM's sigaction so we EINTR out
> of our wait(2) after 30 seconds.
>
> We wait (instead of waitpid) to pick up any stragglers from prior
> calls to timewarn() that we had to leave behind.

You could use fdopen() and keep using stdio but I suppose it doesn't
really matter--dprintf() will effectively get you line buffering.

The only thing that concerns me is the possibility of closing the
wrong fd if stdin is not actually open (unlikely).  I prefer an
idiom like the following:

if (dup2(fd[0], STDIN_FILENO) == -1) {
syslog(LOG_ERR, "dup2: %m");
_exit(1);
}
if (fd[0] != STDIN_FILENO)
close(fd[0]);
...

Otherwise OK.

 - todd

shutdown: fork+exec for wall(1) broadcasts

[Scott Cheloha]

Hey,

I think setjmping from a signal handler to put a time limit on
pclose is too magical, especially when the alternative doesn't
require much more code.

But I do think putting a time limit on our wait for wall to do its
job is a reasonable precaution, though, so:

  * Replace popen with pipe/fork/execle,

  * fprintf -> dprintf, fwrite -> write, and

  * Strip SA_RESTART from SIGALRM's sigaction so we EINTR out
of our wait(2) after 30 seconds.

We wait (instead of waitpid) to pick up any stragglers from prior
calls to timewarn() that we had to leave behind.

ok?

--
Scott Cheloha

Index: sbin/shutdown/shutdown.c
===
RCS file: /cvs/src/sbin/shutdown/shutdown.c,v
retrieving revision 1.47
diff -u -p -r1.47 shutdown.c
--- sbin/shutdown/shutdown.c4 Feb 2018 04:28:41 -   1.47
+++ sbin/shutdown/shutdown.c22 Feb 2018 19:26:27 -
@@ -40,7 +40,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -86,7 +85,7 @@ struct interval {
 
 static time_t offset, shuttime;
 static int dofast, dohalt, doreboot, dopower, dodump, mbuflen, nosync;
-static sig_atomic_t killflg;
+static sig_atomic_t killflg, timed_out;
 static char *whom, mbuf[BUFSIZ];
 
 void badtime(void);
@@ -268,8 +267,6 @@ loop(void)
die_you_gravy_sucking_pig_dog();
 }
 
-static jmp_buf alarmbuf;
-
 static char *restricted_environ[] = {
"PATH=" _PATH_STDPATH,
NULL
@@ -279,59 +276,83 @@ void
 timewarn(int timeleft)
 {
static char hostname[HOST_NAME_MAX+1];
-   char wcmd[PATH_MAX + 4];
-   extern char **environ;
static int first;
-   FILE *pf;
+   int fd[2];
+   pid_t pid, wpid;
 
if (!first++)
(void)gethostname(hostname, sizeof(hostname));
 
-   /* undoc -n option to wall suppresses normal wall banner */
-   (void)snprintf(wcmd, sizeof(wcmd), "%s -n", _PATH_WALL);
-   environ = restricted_environ;
-   if (!(pf = popen(wcmd, "w"))) {
-   syslog(LOG_ERR, "shutdown: can't find %s: %m", _PATH_WALL);
+   if (pipe(fd) == -1) {
+   syslog(LOG_ERR, "pipe: %m");
+   return;
+   }
+   switch (pid = fork()) {
+   case -1:
+   syslog(LOG_ERR, "fork: %m");
+   close(fd[0]);
+   close(fd[1]);
return;
+   case 0:
+   if (dup2(fd[0], STDIN_FILENO) == -1) {
+   syslog(LOG_ERR, "dup2: %m");
+   _exit(1);
+   }
+   close(fd[0]);
+   close(fd[1]);
+   /* wall(1)'s undocumented '-n' flag suppresses its banner. */
+   execle(_PATH_WALL, _PATH_WALL, "-n", (char *)NULL,
+   restricted_environ);
+   syslog(LOG_ERR, "%s: %m", _PATH_WALL);
+   _exit(1);
+   default:
+   close(fd[0]);
}
 
-   (void)fprintf(pf,
+   dprintf(fd[1],
"\007*** %sSystem shutdown message from %s@%s ***\007\n",
timeleft ? "": "FINAL ", whom, hostname);
 
if (timeleft > 10*60) {
struct tm *tm = localtime();
 
-   fprintf(pf, "System going down at %d:%02d\n\n",
+   dprintf(fd[1], "System going down at %d:%02d\n\n",
tm->tm_hour, tm->tm_min);
} else if (timeleft > 59)
-   (void)fprintf(pf, "System going down in %d minute%s\n\n",
+   dprintf(fd[1], "System going down in %d minute%s\n\n",
timeleft / 60, (timeleft > 60) ? "s" : "");
else if (timeleft)
-   (void)fprintf(pf, "System going down in 30 seconds\n\n");
+   dprintf(fd[1], "System going down in 30 seconds\n\n");
else
-   (void)fprintf(pf, "System going down IMMEDIATELY\n\n");
+   dprintf(fd[1], "System going down IMMEDIATELY\n\n");
 
if (mbuflen)
-   (void)fwrite(mbuf, sizeof(*mbuf), mbuflen, pf);
+   (void)write(fd[1], mbuf, mbuflen);
+   close(fd[1]);
 
/*
-* play some games, just in case wall doesn't come back
-* probably unnecessary, given that wall is careful.
+* If we wait longer than 30 seconds for wall(1) to exit we'll
+* throw off our schedule.
 */
-   if (!setjmp(alarmbuf)) {
-   (void)signal(SIGALRM, timeout);
-   (void)alarm((u_int)30);
-   (void)pclose(pf);
-   (void)alarm((u_int)0);
-   (void)signal(SIGALRM, SIG_DFL);
+   signal(SIGALRM, timeout);
+   siginterrupt(SIGALRM, 1);
+   alarm(30);
+   while ((wpid = wait(NULL)) != pid && !timed_out)
+   continue;
+   alarm(0);
+   signal(SIGALRM, SIG_DFL);
+   if (timed_out) {
+   syslog(LOG_NOTICE,
+   "wall[%ld] is taking too long to exit; continuing",
+   

Re: ifconfig: add -rdomain option

[David Gwynne]

> On 22 Feb 2018, at 5:00 pm, Ayaka Koshibe  wrote:
> 
> On Wed, Feb 21, 2018 at 03:42:58PM +0100, Sebastian Benoit wrote:
>> Ayaka Koshibe(akosh...@openbsd.org) on 2018.02.20 21:20:20 -0800:
>>> On Tue, Feb 20, 2018 at 4:48 AM, Reyk Floeter  wrote:
 
> Am 20.02.2018 um 11:15 schrieb Klemens Nanni :
> 
>> On Mon, Feb 19, 2018 at 05:09:58PM -0800, Ayaka Koshibe wrote:
>> This diff would allow saying 'ifconfig foo -rdomain' instead of 
>> 'ifconfig foo rdomain 0'.
> I can see where you're coming from but this breaks semantics: `-option'
> clears an optional parameter or deconfigures functionality whereas
> `rdomain' is mandatory (defaulting to 0), every interface is attached
> to exactly one routing domain all the time.
> 
 
 I would rather say that -option resets it to the default non-specific 
 option. For example, -mode doesn???t remove all wireless modes.
>>> 
>>> This was also my interpretation of -option -- a way to reset to the
>>> default, which happens to be a value of 0 for this case.
>>> 
 I think -rdomain is a good addition and the diff looks fine.
 
 Reyk
>> 
>> i'm ok with the diff as well, but i want to remind you that rdomain is
>> special, because it also removes all ip configuration from the interface.
>> I.e. -rdomain does more than the other -options.
> 
> Ah, right. I hope I can just update the man page to explicitly mention
> this. I've also made it a bit more concise as jmc had suggested.

you should probably do a similar change for tunneldomain, just to be consistent.

cheers,
dlg

> 
> 
> Index: sbin/ifconfig/ifconfig.8
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.303
> diff -u -p -u -p -r1.303 ifconfig.8
> --- sbin/ifconfig/ifconfig.8  20 Feb 2018 07:34:28 -  1.303
> +++ sbin/ifconfig/ifconfig.8  22 Feb 2018 06:06:00 -
> @@ -439,6 +439,10 @@ domains.
> If the specified rdomain does not yet exist it will be created, including
> a routing table with the same id.
> By default all interfaces belong to routing domain 0.
> +.It Cm -rdomain
> +Remove the interface from the routing domain and return it to routing
> +domain 0.
> +Any inet and inet6 addresses on the interface will also be removed.
> .It Cm rtlabel Ar route-label
> (inet)
> Attach
> Index: sbin/ifconfig/ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.360
> diff -u -p -u -p -r1.360 ifconfig.c
> --- sbin/ifconfig/ifconfig.c  20 Feb 2018 15:33:16 -  1.360
> +++ sbin/ifconfig/ifconfig.c  22 Feb 2018 06:06:01 -
> @@ -237,6 +237,7 @@ void  unsetvlandev(const char *, int);
> void  mpe_status(void);
> void  mpw_status(void);
> void  setrdomain(const char *, int);
> +void unsetrdomain(const char *, int);
> int   prefix(void *val, int);
> void  getifgroups(void);
> void  setifgroup(const char *, int);
> @@ -421,6 +422,7 @@ const struct  cmd {
>   { "rtlabel",NEXTARG,0,  setifrtlabel },
>   { "-rtlabel",   -1, 0,  setifrtlabel },
>   { "rdomain",NEXTARG,0,  setrdomain },
> + { "-rdomain",   0,  0,  unsetrdomain },
>   { "staticarp",  IFF_STATICARP,  0,  setifflags },
>   { "-staticarp", -IFF_STATICARP, 0,  setifflags },
>   { "mpls",   IFXF_MPLS,  0,  setifxflags },
> @@ -5665,6 +5667,15 @@ setrdomain(const char *id, int param)
>   strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
>   ifr.ifr_rdomainid = rdomainid;
>   if (ioctl(s, SIOCSIFRDOMAIN, (caddr_t)) < 0)
> + warn("SIOCSIFRDOMAIN");
> +}
> +
> +void
> +unsetrdomain(const char *ignored, int alsoignored)
> +{
> + strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
> + ifr.ifr_rdomainid = 0;
> + if (ioctl(s, SIOCSIFRDOMAIN, (caddr_t)) < 0)
>   warn("SIOCSIFRDOMAIN");
> }
> #endif
> 

Re: ifconfig: add -rdomain option

[Sebastian Benoit]

Ayaka Koshibe(akosh...@openbsd.org) on 2018.02.21 23:00:22 -0800:
> On Wed, Feb 21, 2018 at 03:42:58PM +0100, Sebastian Benoit wrote:
> > Ayaka Koshibe(akosh...@openbsd.org) on 2018.02.20 21:20:20 -0800:
> > > On Tue, Feb 20, 2018 at 4:48 AM, Reyk Floeter  wrote:
> > > >
> > > >> Am 20.02.2018 um 11:15 schrieb Klemens Nanni :
> > > >>
> > > >>> On Mon, Feb 19, 2018 at 05:09:58PM -0800, Ayaka Koshibe wrote:
> > > >>> This diff would allow saying 'ifconfig foo -rdomain' instead of 
> > > >>> 'ifconfig foo rdomain 0'.
> > > >> I can see where you're coming from but this breaks semantics: `-option'
> > > >> clears an optional parameter or deconfigures functionality whereas
> > > >> `rdomain' is mandatory (defaulting to 0), every interface is attached
> > > >> to exactly one routing domain all the time.
> > > >>
> > > >
> > > > I would rather say that -option resets it to the default non-specific 
> > > > option. For example, -mode doesn???t remove all wireless modes.
> > > 
> > > This was also my interpretation of -option -- a way to reset to the
> > > default, which happens to be a value of 0 for this case.
> > > 
> > > > I think -rdomain is a good addition and the diff looks fine.
> > > >
> > > > Reyk
> > 
> > i'm ok with the diff as well, but i want to remind you that rdomain is
> > special, because it also removes all ip configuration from the interface.
> > I.e. -rdomain does more than the other -options.
> 
> Ah, right. I hope I can just update the man page to explicitly mention
> this. I've also made it a bit more concise as jmc had suggested.

ok benno@
 
>  
> Index: sbin/ifconfig/ifconfig.8
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.303
> diff -u -p -u -p -r1.303 ifconfig.8
> --- sbin/ifconfig/ifconfig.8  20 Feb 2018 07:34:28 -  1.303
> +++ sbin/ifconfig/ifconfig.8  22 Feb 2018 06:06:00 -
> @@ -439,6 +439,10 @@ domains.
>  If the specified rdomain does not yet exist it will be created, including
>  a routing table with the same id.
>  By default all interfaces belong to routing domain 0.
> +.It Cm -rdomain
> +Remove the interface from the routing domain and return it to routing
> +domain 0.
> +Any inet and inet6 addresses on the interface will also be removed.
>  .It Cm rtlabel Ar route-label
>  (inet)
>  Attach
> Index: sbin/ifconfig/ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.360
> diff -u -p -u -p -r1.360 ifconfig.c
> --- sbin/ifconfig/ifconfig.c  20 Feb 2018 15:33:16 -  1.360
> +++ sbin/ifconfig/ifconfig.c  22 Feb 2018 06:06:01 -
> @@ -237,6 +237,7 @@ void  unsetvlandev(const char *, int);
>  void mpe_status(void);
>  void mpw_status(void);
>  void setrdomain(const char *, int);
> +void unsetrdomain(const char *, int);
>  int  prefix(void *val, int);
>  void getifgroups(void);
>  void setifgroup(const char *, int);
> @@ -421,6 +422,7 @@ const struct  cmd {
>   { "rtlabel",NEXTARG,0,  setifrtlabel },
>   { "-rtlabel",   -1, 0,  setifrtlabel },
>   { "rdomain",NEXTARG,0,  setrdomain },
> + { "-rdomain",   0,  0,  unsetrdomain },
>   { "staticarp",  IFF_STATICARP,  0,  setifflags },
>   { "-staticarp", -IFF_STATICARP, 0,  setifflags },
>   { "mpls",   IFXF_MPLS,  0,  setifxflags },
> @@ -5665,6 +5667,15 @@ setrdomain(const char *id, int param)
>   strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
>   ifr.ifr_rdomainid = rdomainid;
>   if (ioctl(s, SIOCSIFRDOMAIN, (caddr_t)) < 0)
> + warn("SIOCSIFRDOMAIN");
> +}
> +
> +void
> +unsetrdomain(const char *ignored, int alsoignored)
> +{
> + strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
> + ifr.ifr_rdomainid = 0;
> + if (ioctl(s, SIOCSIFRDOMAIN, (caddr_t)) < 0)
>   warn("SIOCSIFRDOMAIN");
>  }
>  #endif
> 

Re: ifconfig: add -rdomain option

[Stefan Sperling]

On Wed, Feb 21, 2018 at 11:00:22PM -0800, Ayaka Koshibe wrote:
> Ah, right. I hope I can just update the man page to explicitly mention
> this. I've also made it a bit more concise as jmc had suggested.

Looks good and works as advertised in a quick test. ok stsp@

> Index: sbin/ifconfig/ifconfig.8
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> retrieving revision 1.303
> diff -u -p -u -p -r1.303 ifconfig.8
> --- sbin/ifconfig/ifconfig.8  20 Feb 2018 07:34:28 -  1.303
> +++ sbin/ifconfig/ifconfig.8  22 Feb 2018 06:06:00 -
> @@ -439,6 +439,10 @@ domains.
>  If the specified rdomain does not yet exist it will be created, including
>  a routing table with the same id.
>  By default all interfaces belong to routing domain 0.
> +.It Cm -rdomain
> +Remove the interface from the routing domain and return it to routing
> +domain 0.
> +Any inet and inet6 addresses on the interface will also be removed.
>  .It Cm rtlabel Ar route-label
>  (inet)
>  Attach
> Index: sbin/ifconfig/ifconfig.c
> ===
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.360
> diff -u -p -u -p -r1.360 ifconfig.c
> --- sbin/ifconfig/ifconfig.c  20 Feb 2018 15:33:16 -  1.360
> +++ sbin/ifconfig/ifconfig.c  22 Feb 2018 06:06:01 -
> @@ -237,6 +237,7 @@ void  unsetvlandev(const char *, int);
>  void mpe_status(void);
>  void mpw_status(void);
>  void setrdomain(const char *, int);
> +void unsetrdomain(const char *, int);
>  int  prefix(void *val, int);
>  void getifgroups(void);
>  void setifgroup(const char *, int);
> @@ -421,6 +422,7 @@ const struct  cmd {
>   { "rtlabel",NEXTARG,0,  setifrtlabel },
>   { "-rtlabel",   -1, 0,  setifrtlabel },
>   { "rdomain",NEXTARG,0,  setrdomain },
> + { "-rdomain",   0,  0,  unsetrdomain },
>   { "staticarp",  IFF_STATICARP,  0,  setifflags },
>   { "-staticarp", -IFF_STATICARP, 0,  setifflags },
>   { "mpls",   IFXF_MPLS,  0,  setifxflags },
> @@ -5665,6 +5667,15 @@ setrdomain(const char *id, int param)
>   strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
>   ifr.ifr_rdomainid = rdomainid;
>   if (ioctl(s, SIOCSIFRDOMAIN, (caddr_t)) < 0)
> + warn("SIOCSIFRDOMAIN");
> +}
> +
> +void
> +unsetrdomain(const char *ignored, int alsoignored)
> +{
> + strlcpy(ifr.ifr_name, name, sizeof(ifr.ifr_name));
> + ifr.ifr_rdomainid = 0;
> + if (ioctl(s, SIOCSIFRDOMAIN, (caddr_t)) < 0)
>   warn("SIOCSIFRDOMAIN");
>  }
>  #endif
>