Re: document that execve(2) reset an ignored SIGCHLD to default

[Philip Guenther]

On Sun, 11 Oct 2015, Philip Guenther wrote:
> Oooh, I like that combo.  How's this this look then, overall?

...and a followup diff for sigaction(2) and signal(3)...

Index: gen/signal.3
===
RCS file: /cvs/src/lib/libc/gen/signal.3,v
retrieving revision 1.53
diff -u -p -r1.53 signal.3
--- gen/signal.312 May 2015 02:44:06 -  1.53
+++ gen/signal.311 Oct 2015 08:57:39 -
@@ -228,10 +228,13 @@ a specific signal.
 .Pp
 When a process which has installed signal handlers forks,
 the child process inherits the signals.
-All caught signals may be reset to their default action by a call
+All caught signals, as well as
+.Dv SIGCHLD ,
+are reset to their default action by a call
 to the
 .Xr execve 2
 function;
+other
 ignored signals remain ignored.
 .Pp
 The following functions are either reentrant or not interruptible
Index: sys/sigaction.2
===
RCS file: /cvs/src/lib/libc/sys/sigaction.2,v
retrieving revision 1.71
diff -u -p -r1.71 sigaction.2
--- sys/sigaction.2 11 Oct 2015 07:53:49 -  1.71
+++ sys/sigaction.2 11 Oct 2015 08:57:39 -
@@ -289,9 +289,10 @@ and the restart/interrupt flags are inhe
 .Pp
 .Xr execve 2
 reinstates the default
-action for all signals which were caught and
-resets all signals to be caught on the user stack.
-Ignored signals remain ignored;
+action for
+.Dv SIGCHLD
+and all signals which were caught; all other signals remain ignored.
+All signals are reset to be caught on the user stack and
 the signal mask remains the same;
 signals that restart pending system calls continue to do so.
 .Pp

[patch] tcpdump segfault on invalid DECnet packet

[Kevin Reay]

Fix a tcpdump segfault when attempting to print an invalid DECnet
packet.

DECnet packet printing code could cause a segfault on an impossibly
large packet from a specifically crafted packet.

The segfault would occur in tcpdump.c:default_print() called by
print-decnet.c:decnet_print().

Patch built using changes from upstream repo; Git commit:
f61639179282f8e796966b21e45f79db317d7bdb
https://github.com/the-tcpdump-group/tcpdump/commit/f61639179282f8e796
966b21e45f79db317d7bdb

Changes adapted to older version of tcpdump in tree. Patch adds
various length/size checks including TCHECK* throughout. Brings
print-decnet.c length validation in-line with other print-*.c files.
Additionally adds return values to internal print_* functions to
indicate failure of new checks.

Does not include the header no-copy optimization that was in the
upstream patch.

Feedback is greatly appreciated. Let me know if there are any
questions, or if troublesome pcaps and backtrack are wanted.

Tested with various pcaps. Behavior now matches newer upstream version
on Linux.


Index: print-decnet.c
===
RCS file: /cvs/src/usr.sbin/tcpdump/print-decnet.c,v
retrieving revision 1.14
diff -u -p -u -p -r1.14 print-decnet.c
--- print-decnet.c  21 Aug 2015 02:07:32 -  1.14
+++ print-decnet.c  11 Oct 2015 10:40:22 -
@@ -44,13 +44,13 @@ struct rtentry;
 #include "addrtoname.h"
 
 /* Forwards */
-static void print_decnet_ctlmsg(const union routehdr *, u_int);
+static int print_decnet_ctlmsg(const union routehdr *, u_int, u_int);
 static void print_t_info(int);
-static void print_l1_routes(const char *, u_int);
-static void print_l2_routes(const char *, u_int);
+static int print_l1_routes(const char *, u_int);
+static int print_l2_routes(const char *, u_int);
 static void print_i_info(int);
-static void print_elist(const char *, u_int);
-static void print_nsp(const u_char *, u_int);
+static int print_elist(const char *, u_int);
+static int print_nsp(const u_char *, u_int);
 static void print_reason(int);
 #ifdef PRINT_NSPDATA
 static void pdata(u_char *, int);
@@ -76,12 +76,23 @@ decnet_print(register const u_char *ap, 
return;
}
 
+   TCHECK2(*ap, sizeof(short));
pktlen = EXTRACT_LE_16BITS(ap);
+   if (pktlen < sizeof(struct shorthdr)) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   if (pktlen > length) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   length = pktlen;
 
rhlen = min(length, caplen);
rhlen = min(rhlen, sizeof(*rhp));
memcpy((char *)rhp, (char *)&(ap[sizeof(short)]), rhlen);
 
+   TCHECK(rhp->rh_short.sh_flags);
mflags = EXTRACT_LE_8BITS(rhp->rh_short.sh_flags);
 
if (mflags & RMF_PAD) {
@@ -89,6 +100,11 @@ decnet_print(register const u_char *ap, 
u_int padlen = mflags & RMF_PADMASK;
if (vflag)
(void) printf("[pad:%d] ", padlen);
+   if (length < padlen + 2) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   TCHECK2(ap[sizeof(short)], padlen);
ap += padlen;
length -= padlen;
caplen -= padlen;
@@ -100,38 +116,43 @@ decnet_print(register const u_char *ap, 
 
if (mflags & RMF_FVER) {
(void) printf("future-version-decnet");
-   default_print(ap, length);
+   default_print(ap, min(length, caplen));
return;
}
 
/* is it a control message? */
if (mflags & RMF_CTLMSG) {
-   print_decnet_ctlmsg(rhp, min(length, caplen));
+   if(!print_decnet_ctlmsg(rhp, length, caplen))
+   goto trunc;
return;
}
 
switch (mflags & RMF_MASK) {
case RMF_LONG:
+   if (length < sizeof(struct longhdr)) {
+   (void)printf("[|decnet]");
+   return;
+   }
+   TCHECK(rhp->rh_long);
dst =
EXTRACT_LE_16BITS(rhp->rh_long.lg_dst.dne_remote.dne_nodeaddr);
src =
EXTRACT_LE_16BITS(rhp->rh_long.lg_src.dne_remote.dne_nodeaddr);
hops = EXTRACT_LE_8BITS(rhp->rh_long.lg_visits);
nspp = &(ap[sizeof(short) + sizeof(struct longhdr)]);
-   nsplen = min((length - sizeof(struct longhdr)),
-(caplen - sizeof(struct longhdr)));
+   nsplen = length - sizeof(struct longhdr);
break;
case RMF_SHORT:
+   TCHECK(rhp->rh_short);
dst = EXTRACT_LE_16BITS(rhp->rh_short.sh_dst);
src = EXTRACT_LE_16BITS(rhp->rh_short.sh_src);
hops = (EXTRACT_LE_8BITS(rhp->rh_short.sh_visits) & VIS_MASK)+1;
nspp = &(ap[sizeof(short) + sizeof(struct shorthdr)]);
-   nsplen = min((length - sizeof(struct shorthdr)),
-  

Re: npppd: simplify and lock down priv_open()

[YASUOKA Masahiko]

ok yasuoka, the diff is tested.

On Sat, 10 Oct 2015 21:58:02 -0700
Philip Guenther  wrote:
> On Sat, 10 Oct 2015, Theo de Raadt wrote:
>> I don't know the code either, but it is probably better if privsep's had 
>> more narrow task-specific operations.  Like open-specific-file-for-read, 
>> and open log-over-there.  Privsep operations should be tightly 
>> specified, not very generic.

I see.  I'll fix them.

>> Then the child can't open anything.  It feels like this should be 
>> PRIVSEP_GET_NEW_TUN (with the parent selecting which one)
> 
> Yeah.  It *does* currently check the path against a list...but I just 
> noticed that that check is mostly broken, permitted read-only open of any 
> path!
> 
> This needs to be broken into at least two privsep operations, with at 
> least the NPPPD_DIR part in its own call!

I think I could understand how it is broken.  I'll fix it as well.

--yasuoka

Re: document that execve(2) reset an ignored SIGCHLD to default

[jmc]

I think that reads fine.
jmc

  Original Message  
From: Philip Guenther
Sent: Sunday, 11 October 2015 09:49
To: Tech-OpenBSD
Subject: Re: document that execve(2) reset an ignored SIGCHLD to default

On Sun, 11 Oct 2015, Jason McIntyre wrote:
> if you can easily combine it, it probably makes sense to do so. i find
> the last example you've given to be easiest to read. wording it that way
> puts the emphasis on SIGCHLD though - not sure if that's your aim. if
> not you could use example two, but maybe rework it a little:
> 
> Signals set to be ignored in the calling process, with the
> exception of SIGCHLD, are set to be...
> 
> but really they all read fine.

Oooh, I like that combo. How's this this look then, overall?

(Note: signals have only three disposition: caught, ignored, and default, 
so saying that signals not ignored are set to default would be the same as 
saying signals caught are set to default. It's the special handling of 
SIGCHLD which makes one wording simpler than the other...)


Philip

Index: sys/execve.2
===
RCS file: /cvs/src/lib/libc/sys/execve.2,v
retrieving revision 1.46
diff -u -p -r1.46 execve.2
--- sys/execve.210 Sep 2015 17:55:21 -  1.46
+++ sys/execve.211 Oct 2015 08:42:08 -
@@ -121,10 +121,13 @@ some system file like
The intent is to ensure these descriptors are not unallocated, since
many libraries make assumptions about the use of these 3 file descriptors.
.Pp
-Signals set to be ignored in the calling process are set to be ignored in
+Signals set to be ignored in the calling process,
+with the exception of
+.Dv SIGCHLD ,
+are set to be ignored in
the
new process.
-Signals which are set to be caught in the calling process image
+Other signals
are set to default action in the new process image.
Blocked signals remain blocked regardless of changes to the signal action.
The signal stack is reset to be undefined (see
@@ -323,3 +326,12 @@ to a non-superuser, but is executed when
is
.Dq root ,
then the process has some of the powers of a superuser as well.
+.Pp
+.St -p1003.1-2008
+permits
+.Nm
+to leave
+.Dv SIGCHLD
+as ignored in the new process; portable programs cannot rely on
+.Nm
+resetting it to the default disposition.

document that execve(2) reset an ignored SIGCHLD to default

[Philip Guenther]

POSIX permits the Right Thing (resetting SIGCHLD, as we do), but doesn't 
require it.  Document both that we do it and that it's not portable to 
require it.


I'm moderately concerned that the first bit should be merged into the 
sentence at the start of the context.  Which is clearer, what I have in 
the diff:
 Signals set to be ignored in the calling process are set to be ignored in
 the new process.  Signals which are set to be caught in the calling
 process image are set to default action in the new process image.  As an
 exception, if SIGCHLD is ignored then it is reset to the default action.

or should the descriptions be merged:
 Signals other than SIGCHLD set to be ignored in the calling process are
 set to be ignored in the new process.  SIGCHLD and signals which are set
 to be caught in the calling process image are set to the default action
 in the new process image.

or maybe:
 Except for SIGCHLD, signals set to be ignored in the calling process are
 set to be ignored in the new process.  SIGCHLD and signals which are set
 to be caught in the calling process image are set to the default action
 in the new process image.

What phrasing is clearest?

Jason, HELP!



Philip Guenther

Index: sys/execve.2
===
RCS file: /cvs/src/lib/libc/sys/execve.2,v
retrieving revision 1.46
diff -u -p -U5 -r1.46 execve.2
--- sys/execve.210 Sep 2015 17:55:21 -  1.46
+++ sys/execve.211 Oct 2015 06:20:59 -
@@ -124,10 +124,13 @@ many libraries make assumptions about th
 Signals set to be ignored in the calling process are set to be ignored in
 the
 new process.
 Signals which are set to be caught in the calling process image
 are set to default action in the new process image.
+As an exception, if
+.Dv SIGCHLD
+is ignored then it is reset to the default action.
 Blocked signals remain blocked regardless of changes to the signal action.
 The signal stack is reset to be undefined (see
 .Xr sigaction 2
 for more information).
 .Pp
@@ -321,5 +324,14 @@ If a program is
 to a non-superuser, but is executed when the real
 .Em uid
 is
 .Dq root ,
 then the process has some of the powers of a superuser as well.
+.Pp
+.St -p1003.1-2008
+permits
+.Nm
+to leave
+.Dv SIGCHLD
+as ignored in the new process; portable programs cannot rely on
+.Nm
+resetting it to the default disposition.

patch: native ed support

[Tobias Stoeckmann]

Currently, patch calls /bin/ed to support ed-formatted diffs, which
means that patch needs to pledge proc and exec. As discussed, it would
be a huge benefit if we can remove that.

Our patch implementation and GNU patch are already very restrictive in
which patch lines are sent to ed, so don't expect a full-blown ed here:

- Address ranges must be supplied in absolute line numbers. No search
  regular expressions, no relative positioning etc.
- Allowed commands are a, c, d, i, and s.
  - s only works for one address, not a range.
  - s only works with /\.\././ (want to change this to /.//)
- No support for binary files, because our patch lacks that in general

If you ever happened to work with ed-formatted diffs, no news so far.
What's new is this:

- Invalid address ranges lead to fatal instead of silently accepting
  them. This behaviour is in sync with unified diff handling now. If
  garbage at the end of a diff file starts with a number, it might be
  affected.
- No ed output on your console, because... there is no ed running. :P
- The ed code also uses plan A or plan B now. The code benefits from
  this because it renders the needed scratch file of /bin/ed obsolete.

I decided to store the ed code in its own file to reduce complexity
in pch.c. If ed-diffs ever become obsolete, simply remove ed.{c,h}.

So... anyone with ed-diffs out there? This diff wants some tests and
reviews.


Tobias

PS: No, I won't send this in ed-format.
PPS: Yes, I tried and it applies just fine after "touch ed.{c,h}". ;)

Index: Makefile
===
RCS file: /cvs/src/usr.bin/patch/Makefile,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 Makefile
--- Makefile16 May 2005 15:22:46 -  1.4
+++ Makefile11 Oct 2015 09:43:59 -
@@ -1,6 +1,6 @@
 #  $OpenBSD: Makefile,v 1.4 2005/05/16 15:22:46 espie Exp $
 
 PROG=  patch
-SRCS=  patch.c pch.c inp.c util.c backupfile.c mkpath.c
+SRCS=  patch.c pch.c inp.c util.c backupfile.c mkpath.c ed.c
 
 .include 
Index: ed.c
===
RCS file: ed.c
diff -N ed.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ ed.c11 Oct 2015 09:43:59 -
@@ -0,0 +1,335 @@
+/* $OpenBSD$ */
+
+/*
+ * Copyright (c) 2015 Tobias Stoeckmann 
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include "common.h"
+#include "util.h"
+#include "pch.h"
+#include "inp.h"
+
+/* states of finit state machine */
+#define FSM_CMD1
+#define FSM_A  2
+#define FSM_C  3
+#define FSM_D  4
+#define FSM_I  5
+#define FSM_S  6
+
+#define SRC_INP1   /* line's origin is input file */
+#define SRC_PCH2   /* line's origin is patch file */
+
+static voidinit_lines(void);
+static voidfree_lines(void);
+static struct ed_line  *get_line(LINENUM);
+static struct ed_line  *create_line(off_t);
+static int valid_addr(LINENUM, LINENUM);
+static int get_command(void);
+static voidwrite_lines(char *);
+
+LIST_HEAD(ed_head, ed_line) head;
+struct ed_line {
+   LIST_ENTRY(ed_line) entries;
+   int src;
+   unsigned long   subst;
+   union {
+   LINENUM lineno;
+   off_t   seek;
+   } pos;
+};
+
+static LINENUM first_addr;
+static LINENUM second_addr;
+static LINENUM line_count;
+static struct ed_line  *cline; /* current line */
+
+void
+do_ed_script(void)
+{
+   off_t linepos;
+   struct ed_line *nline;
+   LINENUM i, range;
+   int fsm;
+
+   init_lines();
+   cline = NULL;
+   fsm = FSM_CMD;
+
+   for (;;) {
+   linepos = ftello(pfp);
+   if (pgets(buf, sizeof buf, pfp) == NULL)
+   break;
+   p_input_line++;
+
+   if (fsm == FSM_CMD) {
+   if ((fsm = get_command()) == -1)
+   break;
+
+   switch (fsm) {
+   case FSM_C:
+   case FSM_D:
+  

Re: document that execve(2) reset an ignored SIGCHLD to default

[Jason McIntyre]

On Sat, Oct 10, 2015 at 11:34:02PM -0700, Philip Guenther wrote:
> POSIX permits the Right Thing (resetting SIGCHLD, as we do), but doesn't 
> require it.  Document both that we do it and that it's not portable to 
> require it.
> 
> 
> I'm moderately concerned that the first bit should be merged into the 
> sentence at the start of the context.  Which is clearer, what I have in 
> the diff:
>  Signals set to be ignored in the calling process are set to be ignored in
>  the new process.  Signals which are set to be caught in the calling
>  process image are set to default action in the new process image.  As an
>  exception, if SIGCHLD is ignored then it is reset to the default action.
> 
> or should the descriptions be merged:
>  Signals other than SIGCHLD set to be ignored in the calling process are
>  set to be ignored in the new process.  SIGCHLD and signals which are set
>  to be caught in the calling process image are set to the default action
>  in the new process image.
> 
> or maybe:
>  Except for SIGCHLD, signals set to be ignored in the calling process are
>  set to be ignored in the new process.  SIGCHLD and signals which are set
>  to be caught in the calling process image are set to the default action
>  in the new process image.
> 
> What phrasing is clearest?
> 
> Jason, HELP!
> 
> 

if you can easily combine it, it probably makes sense to do so. i find
the last example you've given to be easiest to read. wording it that way
puts the emphasis on SIGCHLD though - not sure if that's your aim. if
not you could use example two, but maybe rework it a little:

Signals set to be ignored in the calling process, with the
exception of SIGCHLD, are set to be...

but really they all read fine.
jmc

> 
> Philip Guenther
> 
> Index: sys/execve.2
> ===
> RCS file: /cvs/src/lib/libc/sys/execve.2,v
> retrieving revision 1.46
> diff -u -p -U5 -r1.46 execve.2
> --- sys/execve.2  10 Sep 2015 17:55:21 -  1.46
> +++ sys/execve.2  11 Oct 2015 06:20:59 -
> @@ -124,10 +124,13 @@ many libraries make assumptions about th
>  Signals set to be ignored in the calling process are set to be ignored in
>  the
>  new process.
>  Signals which are set to be caught in the calling process image
>  are set to default action in the new process image.
> +As an exception, if
> +.Dv SIGCHLD
> +is ignored then it is reset to the default action.
>  Blocked signals remain blocked regardless of changes to the signal action.
>  The signal stack is reset to be undefined (see
>  .Xr sigaction 2
>  for more information).
>  .Pp
> @@ -321,5 +324,14 @@ If a program is
>  to a non-superuser, but is executed when the real
>  .Em uid
>  is
>  .Dq root ,
>  then the process has some of the powers of a superuser as well.
> +.Pp
> +.St -p1003.1-2008
> +permits
> +.Nm
> +to leave
> +.Dv SIGCHLD
> +as ignored in the new process; portable programs cannot rely on
> +.Nm
> +resetting it to the default disposition.
> 

Re: document that execve(2) reset an ignored SIGCHLD to default

[Philip Guenther]

On Sun, 11 Oct 2015, Jason McIntyre wrote:
> if you can easily combine it, it probably makes sense to do so. i find
> the last example you've given to be easiest to read. wording it that way
> puts the emphasis on SIGCHLD though - not sure if that's your aim. if
> not you could use example two, but maybe rework it a little:
> 
>   Signals set to be ignored in the calling process, with the
>   exception of SIGCHLD, are set to be...
> 
> but really they all read fine.

Oooh, I like that combo.  How's this this look then, overall?

(Note: signals have only three disposition: caught, ignored, and default, 
so saying that signals not ignored are set to default would be the same as 
saying signals caught are set to default.  It's the special handling of 
SIGCHLD which makes one wording simpler than the other...)


Philip

Index: sys/execve.2
===
RCS file: /cvs/src/lib/libc/sys/execve.2,v
retrieving revision 1.46
diff -u -p -r1.46 execve.2
--- sys/execve.210 Sep 2015 17:55:21 -  1.46
+++ sys/execve.211 Oct 2015 08:42:08 -
@@ -121,10 +121,13 @@ some system file like
 The intent is to ensure these descriptors are not unallocated, since
 many libraries make assumptions about the use of these 3 file descriptors.
 .Pp
-Signals set to be ignored in the calling process are set to be ignored in
+Signals set to be ignored in the calling process,
+with the exception of
+.Dv SIGCHLD ,
+are set to be ignored in
 the
 new process.
-Signals which are set to be caught in the calling process image
+Other signals
 are set to default action in the new process image.
 Blocked signals remain blocked regardless of changes to the signal action.
 The signal stack is reset to be undefined (see
@@ -323,3 +326,12 @@ to a non-superuser, but is executed when
 is
 .Dq root ,
 then the process has some of the powers of a superuser as well.
+.Pp
+.St -p1003.1-2008
+permits
+.Nm
+to leave
+.Dv SIGCHLD
+as ignored in the new process; portable programs cannot rely on
+.Nm
+resetting it to the default disposition.

Re: ftp: ctype interfaces need unsigned chars

[Joerg Jung]

> Am 11.10.2015 um 04:38 schrieb Philip Guenther :
> 
>> --- smtpd/rfc2822.c
>> +++ /tmp/cocci-output-29655-69b554-rfc2822.c
>> @@ -93,13 +93,13 @@ parser_feed_header(struct rfc2822_parser
>>char*pos;
>> 
>>/* new header */
>> -if (! isspace(*line) && *line != '\0') {
>> +if (! isspace((unsigned char)*line) && *line != '\0') {
> 
> Yep

I would remove the space between '!' and 'isspace()'.

>> -if (isspace(*(pos + 1)))
>> +if (isspace((unsigned char)*(pos + 1)))
> 
> Again, array indexing reads better here, IMO:
>if (isspace((unsigned char)pos[1])

Yes.

>> @@ -169,7 +169,7 @@ rfc2822_parser_feed(struct rfc2822_parse
>>charbuffer[RFC2822_MAX_LINE_SIZE+1];
>> 
>>/* in header and line is not a continuation, execute callback */
>> -if (rp->in_hdr && (*line == '\0' || !isspace(*line)))
>> +if (rp->in_hdr && (*line == '\0' || !isspace((unsigned char)*line)))
> 
> Yep.
> 
> ok?

ok jung@

Re: document that execve(2) reset an ignored SIGCHLD to default

[jmc]

Also reads fine.
jmc

  Original Message  
From: Philip Guenther
Sent: Sunday, 11 October 2015 10:04
To: Tech-OpenBSD
Subject: Re: document that execve(2) reset an ignored SIGCHLD to default

On Sun, 11 Oct 2015, Philip Guenther wrote:
> Oooh, I like that combo. How's this this look then, overall?

...and a followup diff for sigaction(2) and signal(3)...

Index: gen/signal.3
===
RCS file: /cvs/src/lib/libc/gen/signal.3,v
retrieving revision 1.53
diff -u -p -r1.53 signal.3
--- gen/signal.312 May 2015 02:44:06 -  1.53
+++ gen/signal.311 Oct 2015 08:57:39 -
@@ -228,10 +228,13 @@ a specific signal.
.Pp
When a process which has installed signal handlers forks,
the child process inherits the signals.
-All caught signals may be reset to their default action by a call
+All caught signals, as well as
+.Dv SIGCHLD ,
+are reset to their default action by a call
to the
.Xr execve 2
function;
+other
ignored signals remain ignored.
.Pp
The following functions are either reentrant or not interruptible
Index: sys/sigaction.2
===
RCS file: /cvs/src/lib/libc/sys/sigaction.2,v
retrieving revision 1.71
diff -u -p -r1.71 sigaction.2
--- sys/sigaction.2 11 Oct 2015 07:53:49 -  1.71
+++ sys/sigaction.2 11 Oct 2015 08:57:39 -
@@ -289,9 +289,10 @@ and the restart/interrupt flags are inhe
.Pp
.Xr execve 2
reinstates the default
-action for all signals which were caught and
-resets all signals to be caught on the user stack.
-Ignored signals remain ignored;
+action for
+.Dv SIGCHLD
+and all signals which were caught; all other signals remain ignored.
+All signals are reset to be caught on the user stack and
the signal mask remains the same;
signals that restart pending system calls continue to do so.
.Pp

tcpdump: simplify signal handling

[Philip Guenther]

I suspect I haven't gone far enough.

Simplify and correct signal handling in tcpdump:
 * RETSIGTYPE == void, so kill the define for it
 * simplify and strengthen setsignal():
   * we have sigaction(), so use it
   * in general, CLI tools shouldn't catch a signal that was ignored on 
 entry.  Enforce that here, which eliminates the need to return the
 previous signal disposition
   * for SIGCHLD, use SA_NOCLDSTOP to avoid superfluous signals if the
 child is stopped
 * with setsignal() Doing The Right Thing, set_slave_signals() is *really* 
   simple

As I understand it, we're not tracking upstream tcpdump, so I don't think 
this is a (new) hinderance for merges.

ok?

Philip Guenther


PS: on OpenBSD, execve(2) resets SIGCHLD to SIG_DFL if it was set to 
SIG_IGN.  Doing otherwise breaks lots of programs, so it's explicitly 
permitted by POSIX...but we don't seem to document that.  Expect a manpage 
diff after this...


Index: Makefile
===
RCS file: /data/src/openbsd/src/usr.sbin/tcpdump/Makefile,v
retrieving revision 1.57
diff -u -p -r1.57 Makefile
--- Makefile10 Oct 2015 07:52:30 -  1.57
+++ Makefile11 Oct 2015 05:21:58 -
@@ -28,7 +28,7 @@ CFLAGS+=-Wall -I${.CURDIR}/../../sbin/pf
 # for pcap-int.h
 CFLAGS+=-I${.CURDIR}/../../lib/libpcap
 
-CFLAGS+=-DCSLIP -DPPP -DHAVE_FDDI -DETHER_SERVICE -DRETSIGTYPE=void 
-DHAVE_NET_SLIP_H -DHAVE_ETHER_NTOHOST -DINET6
+CFLAGS+=-DCSLIP -DPPP -DHAVE_FDDI -DETHER_SERVICE -DHAVE_NET_SLIP_H 
-DHAVE_ETHER_NTOHOST -DINET6
 
 LDADD+=-lpcap -ll -lcrypto -lm
 DPADD+=${LIBL} ${LIBPCAP} ${LIBCRYPTO}
Index: setsignal.c
===
RCS file: /data/src/openbsd/src/usr.sbin/tcpdump/setsignal.c,v
retrieving revision 1.5
diff -u -p -r1.5 setsignal.c
--- setsignal.c 5 Apr 2015 17:02:57 -   1.5
+++ setsignal.c 11 Oct 2015 05:47:35 -
@@ -23,52 +23,23 @@
 
 #include 
 
-#ifdef HAVE_MEMORY_H
-#include 
-#endif
 #include 
-#ifdef HAVE_SIGACTION
 #include 
-#endif
-
-#ifdef HAVE_OS_PROTO_H
-#include "os-proto.h"
-#endif
 
 #include "setsignal.h"
 
-/*
- * An os independent signal() with BSD semantics, e.g. the signal
- * catcher is restored following service of the signal.
- *
- * When sigset() is available, signal() has SYSV semantics and sigset()
- * has BSD semantics and call interface. Unfortunately, Linux does not
- * have sigset() so we use the more complicated sigaction() interface
- * there.
- *
- * Did I mention that signals suck?
- */
-RETSIGTYPE
-(*setsignal (int sig, RETSIGTYPE (*func)(int)))(int)
+void
+setsignal(int sig, void (*func)(int))
 {
-#ifdef HAVE_SIGACTION
-   struct sigaction old, new;
-
-   memset(, 0, sizeof(new));
-   new.sa_handler = func;
-#ifdef SA_RESTART
-   new.sa_flags |= SA_RESTART;
-#endif
-   if (sigaction(sig, , ) < 0)
-   return (SIG_ERR);
-   return (old.sa_handler);
+   struct sigaction sa;
 
-#else
-#ifdef HAVE_SIGSET
-   return (sigset(sig, func));
-#else
-   return (signal(sig, func));
-#endif
-#endif
+   if (sigaction(sig, NULL, ) == 0 && sa.sa_handler != SIG_IGN) {
+   sa.sa_handler = func;
+   sa.sa_flags = SA_RESTART;
+   if (sig == SIGCHLD)
+   sa.sa_flags |= SA_NOCLDSTOP;
+   sigemptyset(_mask);
+   sigaction(sig, , NULL);
+   }
 }
 
Index: setsignal.h
===
RCS file: /data/src/openbsd/src/usr.sbin/tcpdump/setsignal.h,v
retrieving revision 1.3
diff -u -p -r1.3 setsignal.h
--- setsignal.h 7 Oct 2007 16:41:05 -   1.3
+++ setsignal.h 11 Oct 2015 05:21:02 -
@@ -25,5 +25,5 @@
 #ifndef setsignal_h
 #define setsignal_h
 
-RETSIGTYPE (*setsignal(int, RETSIGTYPE (*)(int)))(int);
+void   setsignal(int, void (*)(int));
 #endif
Index: tcpdump.c
===
RCS file: /data/src/openbsd/src/usr.sbin/tcpdump/tcpdump.c,v
retrieving revision 1.74
diff -u -p -r1.74 tcpdump.c
--- tcpdump.c   9 Oct 2015 01:37:09 -   1.74
+++ tcpdump.c   11 Oct 2015 05:48:13 -
@@ -92,8 +92,8 @@ extern void bpf_dump(struct bpf_program 
 extern int esp_init(char *);
 
 /* Forwards */
-RETSIGTYPE cleanup(int);
-RETSIGTYPE gotchld(int);
+void   cleanup(int);
+void   gotchld(int);
 extern __dead void usage(void);
 
 /* Length of saved portion of packet. */
@@ -503,8 +503,7 @@ main(int argc, char **argv)
 }
 
 /* make a clean exit on interrupts */
-/* ARGSUSED */
-RETSIGTYPE
+void
 cleanup(int signo)
 {
struct pcap_stat stat;
@@ -533,8 +532,7 @@ cleanup(int signo)
_exit(0);
 }
 
-/* ARGSUSED */
-RETSIGTYPE
+void
 gotchld(int signo)
 {
pid_t pid;
@@ -681,14 +679,10 @@ default_print(register const u_char *bp,
 void
 set_slave_signals(void)
 {
-   RETSIGTYPE (*oldhandler)(int);
-
setsignal(SIGTERM, 

iwm: fix handling of large firmware commands

[Stefan Sperling]

The iwm(4) driver pre-allocates fw command payload buffers of 320 bytes.

For some firmware commands, particularly those used when configuring
the PHY (iwm_send_phy_db_cmd) and running scans (iwm_mvm_scan_request),
the payload exceeds 320 bytes. I've seen somewhere between 2k and 3.5k
being used. Precisely these commands fail often while we're trying to
bring the interface up. You've probably seen "could not initiate scan".
The PHY failure case doesn't print anything unless IWM_DEBUG is set.

If the payload doesn't fit, the driver tries to use an mbuf instead
of the pre-allocated payload buffer. This seems to be based on the
approach taken in iwn(4).

The current code seems confused about sizes.
If a command requires 'sizeof(cmd->hdr) + paylen' bytes, the driver only
maps hcmd->len[0] bytes for DMA. But 'paylen' is the sum of hcmod->len[0]
and hcmd->len[1]. And the DMA map used to map large payloads only handles
mappings of up to MCLBYTES, which is only 2k...

Fix this by sizing the mbuf and its DMA map correctly.

Index: if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.55
diff -u -p -r1.55 if_iwm.c
--- if_iwm.c11 Oct 2015 10:22:28 -  1.55
+++ if_iwm.c11 Oct 2015 14:37:41 -
@@ -1108,14 +1108,21 @@ iwm_alloc_tx_ring(struct iwm_softc *sc, 
paddr = ring->cmd_dma.paddr;
for (i = 0; i < IWM_TX_RING_COUNT; i++) {
struct iwm_tx_data *data = >data[i];
+   size_t mapsize;
 
data->cmd_paddr = paddr;
data->scratch_paddr = paddr + sizeof(struct iwm_cmd_header)
+ offsetof(struct iwm_tx_cmd, scratch);
paddr += sizeof(struct iwm_device_cmd);
 
-   error = bus_dmamap_create(sc->sc_dmat, MCLBYTES,
-   IWM_NUM_OF_TBS - 2, MCLBYTES, 0, BUS_DMA_NOWAIT,
+   /* FW commands may require more mapped space than packets. */
+   if (qid == IWM_MVM_CMD_QUEUE)
+   mapsize = (sizeof(struct iwm_cmd_header) +
+   IWM_MAX_CMD_PAYLOAD_SIZE);
+   else
+   mapsize = MCLBYTES;
+   error = bus_dmamap_create(sc->sc_dmat, mapsize,
+   IWM_NUM_OF_TBS - 2, mapsize, 0, BUS_DMA_NOWAIT,
>map);
if (error != 0) {
printf("%s: could not create TX buf DMA map\n", 
DEVNAME(sc));
@@ -3393,30 +3400,31 @@ iwm_send_cmd(struct iwm_softc *sc, struc
data = >data[ring->cur];
 
if (paylen > sizeof(cmd->data)) {
-   /* Command is too large */
-   if (sizeof(cmd->hdr) + paylen > IWM_RBUF_SIZE) {
+   /* Command is too large to fit in pre-allocated space. */
+   size_t totlen = sizeof(cmd->hdr) + paylen;
+   if (paylen > IWM_MAX_CMD_PAYLOAD_SIZE) {
+   printf("%s: firmware command too long (%zd bytes)\n",
+   DEVNAME(sc), totlen);
error = EINVAL;
goto out;
}
-   m = m_gethdr(M_DONTWAIT, MT_DATA);
+   m = MCLGETI(NULL, M_DONTWAIT, NULL, totlen);
if (m == NULL) {
-   error = ENOMEM;
-   goto out;
-   }
-   MCLGETI(m, M_DONTWAIT, NULL, IWM_RBUF_SIZE);
-   if (!(m->m_flags & M_EXT)) {
-   m_freem(m);
+   printf("%s: could not get fw cmd mbuf (%zd bytes)\n",
+   DEVNAME(sc), totlen);
error = ENOMEM;
goto out;
}
cmd = mtod(m, struct iwm_device_cmd *);
error = bus_dmamap_load(sc->sc_dmat, data->map, cmd,
-   hcmd->len[0], NULL, BUS_DMA_NOWAIT | BUS_DMA_WRITE);
+   totlen, NULL, BUS_DMA_NOWAIT | BUS_DMA_WRITE);
if (error != 0) {
+   printf("%s: could not load fw cmd mbuf (%zd bytes)\n",
+   DEVNAME(sc), totlen);
m_freem(m);
goto out;
}
-   data->m = m;
+   data->m = m; /* mbuf will be freed in iwm_cmd_done() */
paddr = data->map->dm_segs[0].ds_addr;
} else {
cmd = >cmd[ring->cur];
@@ -3447,13 +3455,13 @@ iwm_send_cmd(struct iwm_softc *sc, struc
code, hcmd->len[0] + hcmd->len[1] + sizeof(cmd->hdr),
async ? " (async)" : ""));
 
-   if (hcmd->len[0] > sizeof(cmd->data)) {
-   bus_dmamap_sync(sc->sc_dmat, data->map, 0, hcmd->len[0],
-   BUS_DMASYNC_PREWRITE);
+   if (paylen > sizeof(cmd->data)) {
+   bus_dmamap_sync(sc->sc_dmat, data->map, 0,
+   sizeof(cmd->hdr) + paylen, BUS_DMASYNC_PREWRITE);
 

Re: sleep: don't return errno from main()

[Ted Unangst]

Philip Guenther wrote:
> 
> As a general rule, programs should not use errno values as an exit status.
> 
> Compare "sleep 10001" w/ and w/o this diff.
> 
> ok?

agreed, but why not return 1? i don't want to have to slap 2>/dev/null around
all my sleep calls now.

> 
> Index: sleep.c
> ===
> RCS file: /data/src/openbsd/src/bin/sleep/sleep.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 sleep.c
> --- sleep.c   9 Oct 2015 01:37:06 -   1.23
> +++ sleep.c   10 Oct 2015 23:19:03 -
> @@ -107,7 +107,7 @@ main(int argc, char *argv[])
>  
>   if ((secs > 0) || (nsecs > 0))
>   if (nanosleep(, NULL))
> - return (errno);
> + err(1, NULL);
>   return (0);
>  }
>  
> 

Re: iwm: fix handling of large firmware commands

[Ted Unangst]

Stefan Sperling wrote:
> The iwm(4) driver pre-allocates fw command payload buffers of 320 bytes.
> 
> For some firmware commands, particularly those used when configuring
> the PHY (iwm_send_phy_db_cmd) and running scans (iwm_mvm_scan_request),
> the payload exceeds 320 bytes. I've seen somewhere between 2k and 3.5k
> being used. Precisely these commands fail often while we're trying to
> bring the interface up. You've probably seen "could not initiate scan".
> The PHY failure case doesn't print anything unless IWM_DEBUG is set.
> 
> If the payload doesn't fit, the driver tries to use an mbuf instead
> of the pre-allocated payload buffer. This seems to be based on the
> approach taken in iwn(4).

huh? Why an mbuf? Is dma_alloc not a better choice?

Re: iwm: fix handling of large firmware commands

[Stefan Sperling]

On Sun, Oct 11, 2015 at 11:31:35AM -0400, Ted Unangst wrote:
> huh? Why an mbuf? Is dma_alloc not a better choice?

The mbuf pointer already exists to keep track of packets on the
otheor Tx queues. I guess that's why iwn (where this came from)
does it this way.

I don't mind changing to dma_alloc. I just wanted to fix this in
a non-intrusive way (in terms of lines of diff) and move on...

If we change this, I think we should consider moving firmware
commands off the Tx queues entirely. We're just sending one
command at a time anyway.

iwm: always initialize index into tx queue array

[Stefan Sperling]

The index into sc->txq used by iwm_tx() is only initialized for
management frames. For data frames, iwm picks a garbage index off
the stack which is not initialized unless a mgmt frame was processed
first in the same loop.

This could end rather badly. Fix it the easy way for now, because
overall qos handling needs more love than just this.

Index: if_iwm.c
===
RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v
retrieving revision 1.55
diff -u -p -r1.55 if_iwm.c
--- if_iwm.c11 Oct 2015 10:22:28 -  1.55
+++ if_iwm.c11 Oct 2015 12:15:25 -
@@ -5572,7 +5572,7 @@ iwm_start(struct ifnet *ifp)
struct ieee80211_node *ni;
struct ether_header *eh;
struct mbuf *m;
-   int ac;
+   int ac = EDCA_AC_BE; /* XXX */
 
if ((ifp->if_flags & (IFF_RUNNING | IFF_OACTIVE)) != IFF_RUNNING)
return;
@@ -5588,7 +5588,6 @@ iwm_start(struct ifnet *ifp)
IF_DEQUEUE(>ic_mgtq, m);
if (m) {
ni = m->m_pkthdr.ph_cookie;
-   ac = 0;
goto sendit;
}
if (ic->ic_state != IEEE80211_S_RUN) {

Re: iwm: fix handling of large firmware commands

[Ted Unangst]

Stefan Sperling wrote:
> On Sun, Oct 11, 2015 at 11:31:35AM -0400, Ted Unangst wrote:
> > huh? Why an mbuf? Is dma_alloc not a better choice?
> 
> The mbuf pointer already exists to keep track of packets on the
> otheor Tx queues. I guess that's why iwn (where this came from)
> does it this way.
> 
> I don't mind changing to dma_alloc. I just wanted to fix this in
> a non-intrusive way (in terms of lines of diff) and move on...
> 
> If we change this, I think we should consider moving firmware
> commands off the Tx queues entirely. We're just sending one
> command at a time anyway.


oh, ok. it look like the short command path wasn't using an mbuf, but i got
that impression only from looking at the diff. maybe wrong.

Re: iwm: fix handling of large firmware commands

[Stefan Sperling]

On Sun, Oct 11, 2015 at 12:11:22PM -0400, Ted Unangst wrote:
> oh, ok. it look like the short command path wasn't using an mbuf, but i got
> that impression only from looking at the diff. maybe wrong.

No, it's as bad as you think it is. Short commands stick payload data
into the Tx descriptor, which is mapped via a different DMA map.

So during transmit of normal packets, we always have a descriptor,
which points at an mbuf (mapped elsewhere) with a packet in it.
Note that the transmit operation is also a firmware command (IWM_TX_CMD).

For firmware commands not dealing with packets, there's always a
descriptor which either inlines the command payload (up to 320 bytes)
or points at other storage (currently an mbuf) for payload.

Perhaps this can be made simpler. However, the firmware expects commands
on Tx queue 9, so it invites drivers to treat the command queue in the
same way as other queues.

What I'm doing right now is fixing the code as it ought to be written
if an mbuf is used for large command payloads. Perhaps not perfect,
but better than broken.

Re: iwm: fix handling of large firmware commands

[Mark Kettenis]

> Date: Sun, 11 Oct 2015 17:45:47 +0200
> From: Stefan Sperling 
> 
> On Sun, Oct 11, 2015 at 11:31:35AM -0400, Ted Unangst wrote:
> > huh? Why an mbuf? Is dma_alloc not a better choice?
> 
> The mbuf pointer already exists to keep track of packets on the
> otheor Tx queues. I guess that's why iwn (where this came from)
> does it this way.
> 
> I don't mind changing to dma_alloc. I just wanted to fix this in
> a non-intrusive way (in terms of lines of diff) and move on...

Using dma_alloc() here would be wrong.  Drivers should use bus_dmamem_alloc().

Re: FreeType-2.6.1 !!header files layout changed again!!

[Matthieu Herrb]

On Wed, Oct 07, 2015 at 06:12:24AM -0600, David Coppa wrote:
> 
> Hi!
> 
> New freetype version, new header file layout :( :(
> 
> Now, all header files except 'ft2build.h' are (again) into
> /usr/X11R6/include/freetype2/freetype/.
> 
> Luckily, no ABI changes this time.
>
As far as I'm concerned (ie waiting for ports build results) ok
matthieu@.

Running this on a couple of machines without visible problems.
-- 
Matthieu Herrb


signature.asc
Description: PGP signature

Re: sleep: don't return errno from main()

[Philip Guenther]

On Sun, Oct 11, 2015 at 8:19 AM, Ted Unangst  wrote:
> agreed, but why not return 1? i don't want to have to slap 2>/dev/null around
> all my sleep calls now.

So don't.  Why would you need to?

sendbug(1): remove -V flag

[Michael Reed]

The version is stored in a constant string which is prone to
become outdated, as has happened here.  uname(3) could be
used to make sure it's always up to date, but I don't see
the point given uname(1)'s `r' flag already does this.


Index: sendbug.1
===
RCS file: /cvs/src/usr.bin/sendbug/sendbug.1,v
retrieving revision 1.25
diff -u -p -r1.25 sendbug.1
--- sendbug.1   9 Sep 2015 21:23:31 -   1.25
+++ sendbug.1   11 Oct 2015 21:11:00 -
@@ -11,7 +11,7 @@
 .Nd report a bug in OpenBSD
 .Sh SYNOPSIS
 .Nm
-.Op Fl DEPV
+.Op Fl DEP
 .Sh DESCRIPTION
 .Nm
 is used to submit problem reports (PRs) to the
@@ -80,8 +80,6 @@ to read a PR from the standard input, de
 and write them into the current directory.
 .It Fl P
 Generate and print the template with system information filled out.
-.It Fl V
-Print the version number.
 .El
 .Sh ENVIRONMENT
 .Bl -tag -width Ds
Index: sendbug.c
===
RCS file: /cvs/src/usr.bin/sendbug/sendbug.c,v
retrieving revision 1.71
diff -u -p -r1.71 sendbug.c
--- sendbug.c   10 Oct 2015 20:35:01 -  1.71
+++ sendbug.c   11 Oct 2015 21:11:00 -
@@ -45,7 +45,6 @@ void  usbdevs(FILE *);
 
 const char *categories = "system user library documentation kernel "
 "alpha amd64 arm hppa i386 m88k mips64 powerpc sh sparc sparc64 vax";
-char *version = "5.5";
 const char *comment[] = {
"",
"",
@@ -65,7 +64,7 @@ usage(void)
 {
extern char *__progname;
 
-   fprintf(stderr, "usage: %s [-DEPV]\n", __progname);
+   fprintf(stderr, "usage: %s [-DEP]\n", __progname);
exit(1);
 }
 
@@ -89,7 +88,7 @@ main(int argc, char *argv[])
if (pledge("stdio rpath wpath cpath tmppath getpw proc exec", NULL) == 
-1)
err(1, "pledge");
 
-   while ((ch = getopt(argc, argv, "DEPV")) != -1)
+   while ((ch = getopt(argc, argv, "DEP")) != -1)
switch (ch) {
case 'D':
Dflag = 1;
@@ -100,9 +99,6 @@ main(int argc, char *argv[])
case 'P':
Pflag = 1;
break;
-   case 'V':
-   printf("%s\n", version);
-   exit(0);
default:
usage();
}

Re: [PATCH] fix failover delay in relayd

[Brian S. Vangsgaard]

bump

Den 02-10-2015 kl. 12:09 skrev Brian S. Vangsgaard:

Hi

Patch for relayd that will enable quick removal of all host-related
entries in the related relayd anchor if the host fails the SLA check.

Without this patch, the anchor cleanup wil not be called before the next
SLA round (adding a minimum 10 sec delay on failover).

Tested on a dual gateway setup (using CARP on vlans with pfsync defer
on) with 3 pool members in the relayd config.


Index: pfe.c
===
RCS file: /cvs/src/usr.sbin/relayd/pfe.c,v
retrieving revision 1.79.2.1
diff -u -p -r1.79.2.1 pfe.c
--- pfe.c   20 Sep 2015 11:20:16 -  1.79.2.1
+++ pfe.c   2 Oct 2015 09:32:46 -
@@ -152,6 +152,8 @@ pfe_dispatch_hce(int fd, struct privsep_
 table->conf.flags |= F_CHANGED;
 host->flags |= F_DEL;
 host->flags &= ~(F_ADD);
+   host->up = st.up;
+   pfe_sync();
 }

 host->up = st.up;


--
bsv

Re: ftp: ctype interfaces need unsigned chars

[Alexander Bluhm]

On Sat, Oct 10, 2015 at 08:03:28PM -0700, Philip Guenther wrote:
> On Sat, 10 Oct 2015, Michael McConville wrote:
> > FWIW, this is a perfect use case for Coccinelle. Below is what I dredged 
> > up in src/usr.sbin (diff not yet carefully audited, but apparently 
> > sane).
> 
> These look good to me.  bluhm?

Commited, thanks.

> > --- syslogd/syslogd.c
> > +++ /tmp/cocci-output-17550-6b6404-syslogd.c
> > @@ -1126,7 +1126,7 @@ octet_counting(struct evbuffer *evbuf, c
> >  * It can be assumed that octet-counting framing is used if a syslog
> >  * frame starts with a digit.
> >  */
> > -   if (buf >= end || !isdigit(*buf))
> > +   if (buf >= end || !isdigit((unsigned char)*buf))
> > return (-1);
> > /*
> >  * SYSLOG-FRAME = MSG-LEN SP SYSLOG-MSG
> > @@ -1134,7 +1134,7 @@ octet_counting(struct evbuffer *evbuf, c
> >  * We support up to 5 digits in MSG-LEN, so the maximum is 9.
> >  */
> > for (p = buf; p < end && p < buf + 5; p++) {
> > -   if (!isdigit(*p))
> > +   if (!isdigit((unsigned char)*p))
> > break;
> > }
> > if (buf >= p || p >= end || *p != ' ')

Re: tame() error handling diff

[Ingo Schwarze]

Hi,

Nicholas Marriott wrote on Tue, Oct 06, 2015 at 11:53:48AM +0100:
> On Tue, Oct 06, 2015 at 12:25:57PM +0200, Benny Lofgren wrote:

>> In any case, I hope you at least agree with me that the documentation
>> should reflect actual behaviour? :-) I've updated my diff to tame.2 to
>> describe the error returns more accurately, minus the errno change.

Committed with minimal tweaks, thanks.

>> (And I hope I got it right, I suck at mdoc...)

> this is ok with me

Thanks for checking it.
  Ingo

>> Index: tame.2
>> ===
>> RCS file: /cvs/src/lib/libc/sys/tame.2,v
>> retrieving revision 1.31
>> diff -u -p -r1.31 tame.2
>> --- tame.2 4 Oct 2015 20:47:16 - 1.31
>> +++ tame.2 6 Oct 2015 10:13:13 -
>> @@ -424,17 +424,24 @@ will fail if:
>> .Bl -tag -width Er
>> .It Bq Er EFAULT
>> .Fa paths
>> +or one of its elements, or
>> +.Fa request
>> points outside the process's allocated address space.
>> .It Bq Er ENAMETOOLONG
>> An element of
>> .Fa paths
>> -is too large, or prepending
>> +is too large, prepending
>> .Fa cwd
>> to it would exceed
>> .Dv PATH_MAX
>> -bytes.
>> +bytes, or
>> +.Fa request
>> +is too long.
>> .It Bq Er EPERM
>> -This process is attempting to increase permissions.
>> +This process is attempting to increase its permissions.
>> +.It Bq Er EINVAL
>> +.Ar request
>> +is malformed or contains invalid keywords.
>> .It Bq Er E2BIG
>> The
>> .Ar paths

Re: bsd.port.mk.5: restructure `clean' section

[Ingo Schwarze]

Hi Vadim,

Vadim Zhukov wrote on Mon, Oct 05, 2015 at 09:07:41PM +0300:
> 2015-09-26 21:49 GMT+03:00 Michael Reed :

>> The `clean' target takes optional arguments, so denote that in
>> the item header.  Additionally, the subtargets are fixed strings,
>> not variables, so change the use of Va to Cm to reflect that.

> Well, technically "clean=" is not a target, but a hack that looks
> like a parametrized target.

I don't think it would be a good idea to explain that technicality.
Probably, the reader is best served by hiding it as well as we can.
The patch below does that by spelling out all the pseudotargets in
full in order to discuss the idiosyncratic syntax as little as
possible.

The possibility to combine becomes clear by example, as seen in
the explanations of "all" and "packages".

> As a result, the ordering in the following
> cases will differ, and that matters:
> 
> make package clean
> make package clean=work

Explaining that would seem even more confusing.  I feel that it was
a mistake to introduce this hack in the first place.  It probably
would have been better to manually define individual targets
clean-all, clean-bulk, clean-build, and so on, but that's a different
matter; now we have to document the existing stuff.

> Even worse, the wording is confusing, because the term "subtarget" is
> used in other parts of this manual page with a different semantics.

Sure, the word "subtarget" should be avoided.  The patch below gets
away with not introducing any term at all.

> Can't comment on mdoc macro choosing, sorry.

Michael is right that .Va is the wrong macro here and .Cm is the
best one available.

OK?
  Ingo


Index: bsd.port.mk.5
===
RCS file: /cvs/src/share/man/man5/bsd.port.mk.5,v
retrieving revision 1.426
diff -u -p -r1.426 bsd.port.mk.5
--- bsd.port.mk.5   23 Sep 2015 01:38:36 -  1.426
+++ bsd.port.mk.5   12 Oct 2015 02:24:48 -
@@ -179,52 +179,44 @@ main archive site in the case of a check
 .Ev NO_CHECKSUM
 can be used to avoid all checksumming steps.
 .It Cm clean
-Clean ports contents.
-By default, it will clean the work directory.
-It can be invoked as
-make clean='[depends build bulk work fake flavors dist install sub package
-packages plist]'.
-.Bl -tag -width packages
-.It Va work
-Clean work directory.
-.It Va bulk
+Shorthand for
+.Cm clean Ns = Ns Cm work .
+.It Cm clean Ns = Ns Cm all
+Shorthand for
+.Cm clean Ns = Ap Cm work flavors packages plist Ap .
+.It Cm clean Ns = Ns Cm bulk
 Clean bulk cookie.
-.It Va build
+.It Cm clean Ns = Ns Cm build
 Clean the
 .Va WRKBUILD
 directory (only useful if
 .Va SEPARATE_BUILD
 is set).
-.It Va depends
+.It Cm clean Ns = Ns Cm depends No or Cm clean-depends
 Recurse into dependencies.
-.It Va dist
+.It Cm clean Ns = Ns Cm dist
 Clean distribution files.
-.It Va fake
+.It Cm clean Ns = Ns Cm fake
 Clean fake installation directory.
-.It Va flavors
+.It Cm clean Ns = Ns Cm flavors
 Clean all work directories.
-.It Va install
+.It Cm clean Ns = Ns Cm install
 Uninstall package.
-.It Va package
-Remove all copies of package file.
-.It Va plist
+.It Cm clean Ns = Ns Cm package
+Remove all copies of package files.
+.It Cm clean Ns = Ns Cm packages
+Shorthand for
+.Cm clean Ns = Ap Cm sub package Ap .
+.It Cm clean Ns = Ns Cm plist
 Remove registered packing lists of all subpackages.
-.It Va sub
+.It Cm clean Ns = Ns Cm sub
 With
-.Va install
+.Cm install
 or
-.Va package ,
+.Cm package ,
 clean subpackages as well.
-.It Va packages
-Shorthand for
-.Sq sub package .
-.It Va all
-Shorthand for
-.Sq work flavors packages plist .
-.El
-.It Cm clean-depends
-Shorthand for
-.Ql make clean=depends .
+.It Cm clean Ns = Ns Cm work
+Clean work directory.
 .It Cm configure
 Configure the port.
 Might be a void operation.