Re: Patch for crypt(3) man page.

2021-01-27 Thread Ingo Schwarze
Hi,

this page is a mess.  It is full of unclear wordings, in some cases
verging incorrect statements.  At the same time, parts of it are wordy.

Here is an attempt to start fixing it.
I refrained from trying to explain $2a$ (as suggested by sthen@) or to
document the missing bcrypt_gensalt(3) in the same patch because this
patch is already quite large.

Rationale:
 * Rename the "salt" argument of bcrypt(3) to "setting" to match crypt(3).
   It is *not* the salt, it is exactly the same for both functions.
 * Avoid talking about "first" and "second" arguments, just use the
   argument names as in every other manual page.
 * Properly document ".Fa setting" using a syntax display.
 * Precisely explain the format of the encoded salt, and mention
   what happens with additional bytes.
 * Mention what happens to excessive bytes in the key.
 * Precisely describe the return value for Blowfish.
 * Below RETURN VALUES, avoid the misleading statement that it
   returns just "the encrypted value"; it actually returns much
   more than that.  What exactly depends on the algorithm, and
   those details were already covered in the algorithm-specific
   section(s) above.  So instead mention what this heap of bytes
   is typically used for.

I hope i got this right; OK?
  Ingo


Index: crypt.3
===
RCS file: /cvs/src/lib/libc/crypt/crypt.3,v
retrieving revision 1.45
diff -u -r1.45 crypt.3
--- crypt.3 6 Apr 2015 20:49:41 -   1.45
+++ crypt.3 27 Jan 2021 21:30:22 -
@@ -49,7 +49,7 @@
 .Ft char *
 .Fn bcrypt_gensalt "u_int8_t log_rounds"
 .Ft char *
-.Fn bcrypt "const char *key" "const char *salt"
+.Fn bcrypt "const char *key" "const char *setting"
 .Sh DESCRIPTION
 These functions are deprecated in favor of
 .Xr crypt_checkpass 3
@@ -62,58 +62,80 @@
 Additional code has been added to deter key search attempts and to use
 stronger hashing algorithms.
 .Pp
-The first argument to
-.Fn crypt
-is a NUL-terminated
-string
-.Fa key ,
+The
+.Fa key
+is a NUL-terminated string,
 typically a user's typed password.
-The second,
-.Fa setting ,
-currently supports a single form.
-If it begins
-with a string character
-.Pq Ql $
-and a number then a different algorithm is used depending on the number.
-At the moment
-.Ql $2
-chooses Blowfish hashing; see below for more information.
+The
+.Fa setting
+argument is required to begin with an algorithm identifier enclosed
+in dollar signs.
+The meaning of the rest of the
+.Fa setting
+argument is algorithm-specific.
+.Pp
+Currently, only
+.Sx Blowfish crypt
+is supported, so
+.Fa setting
+needs to begin with
+.Qq $2b$ .
 .Ss Blowfish crypt
-The Blowfish version of crypt has 128 bits of
-.Fa salt
+The Blowfish version of crypt has 128 bits of salt
 in order to make building dictionaries of common passwords space consuming.
+.Pp
+The format of
+.Fa setting
+is:
+.Pp
+.Sm off
+.D1 $ Cm 2b No $ Ar log_rounds No $ Ar encoded_salt
+.Sm on
+.Pp
+The binary logarithm of the number of rounds
+is specified as a decimal number containing exactly two digits
+in the range 04 to 31, inclusive.
+For example,
+.Qq $2b$08$
+requests 256 rounds.
+.Pp
+The
+.Ar encoded_salt
+is a byte array of at least 22 bytes, with the first 172 bits
+encoding the 128-bit salt in base64 form.
+Any additional bytes are ignored.
+.Pp
 The initial state of the
 Blowfish cipher is expanded using the
-.Fa salt
-and the
-.Fa password
-repeating the process a variable number of rounds, which is encoded in
-the password string.
-The maximum password length is 72.
+salt and the
+.Fa key
+repeating the process the specifieds number of rounds.
+.Pp
+The maximum
+.Fa key
+length is 72; excessive bytes are silently discarded.
 The final Blowfish password entry is created by encrypting the string
-.Pp
-.Dq OrpheanBeholderScryDoubt
-.Pp
+.Qq OrpheanBeholderScryDoubt
 with the Blowfish state 64 times.
 .Pp
-The version number, the logarithm of the number of rounds and
-the concatenation of salt and hashed password are separated by the
-.Ql $
-character.
-An encoded
-.Sq 8
-would specify 256 rounds.
-A valid Blowfish password looks like this:
-.Pp
-.Dq $2b$12$FPWWO2RJ3CK4FINTw0Hi8OiPKJcX653gzSS.jqltHFMxyDmmQ0Hqq .
+The return value contains a copy of
+.Ar setting
+at the beginning, followed by 31 bytes containing the encrypted password
+encoded in base64 form, so the return value is effectively of the type
+.Vt char[60] .
+For example, a valid return value for Blowfish looks like this:
 .Pp
-The whole Blowfish password string is passed as
-.Fa setting
-for interpretation.
+.Dl $2b$12$FPWWO2RJ3CK4FINTw0Hi8OiPKJcX653gzSS.jqltHFMxyDmmQ0Hqq
 .Sh RETURN VALUES
-The function
+The functions
 .Fn crypt
-returns a pointer to the encrypted value on success, and
+and
+.Fn bcrypt
+return a string usable for the
+.Ar password
+field in the
+.Xr master.passwd 5
+file on success or
 .Dv NULL
 on failure.
 .Sh SEE ALSO



Re: man: help pagers recognise HTML files as such

2021-01-26 Thread Ingo Schwarze
Hi Klemens,

Klemens Nanni wrote on Sat, Jan 16, 2021 at 10:31:49AM +0100:

> On rare occasions I'd like to use the following idiom to read manuals in
> browsers, mostly to better readability and navigation of long sections:
> 
>   MANPAGER=netsurf-gtk3 man -Thtml jq
> 
> (jq(1) has lots of examples and HTML is nicer to read when it comes to
> literal input and output for example.)

Indeed, on rare occasions, such idioms are useful for various purposes.

> My problem is that browsers fail to render HTML as such in case the
> temporary file generated by mandoc(1) lack a file extension, i.e.
> `$browser /tmp/man.abc123' will show literal HTML code whereas
> `$browser /tmp/man.abc123.html' will actually render it.

Software deriving the file type from the file name extension is
stupid and un-UNIXy.  Such behaviour also tends to be dangerous
for obvious reasons.  Admittedly, such behaviour is widespread
in the Web context - even httpd(8) derives file types from file
name extensions...

Not all browsers fall into that trap, and some explicitly allow
specifying the file type, for example

   $ MANPAGER="lynx --force_html" man -T html ls

But i agree that lack of such support is maybe not a sufficent reason
to refrain from using a browser one wants to use for other reasons,
so the feature you propose seems useful to me, and i agree with
your general direction.

> HTML is the only output type I've encountered problem with so far.
> PDF/mupdf(1) for example works fine with arbitrary file names:
> 
>   MANPAGER=mupdf man -Tpdf jq
> 
> Diff below makes mandoc produce temporary files with the ".html" suffix
> in case `-Thtml' was specified.

Sounds sound, i see no downside.

> It feels a bit off to do this for HTML only,

That part doesn't seem that bad to me.  Abusing filename extensions
is particularly widespread on the web, so it feels unsurprising
that HTML may need more handholding than other modes.

> but it greatly improves
> accessibility for me and is simple enough.  If need be, the suffix
> handling could be easily extended to cover more output types in the
> future.
> 
> Feedback? Objections? OK?

It's maybe just a bikeshed, but could you put the logic selecting
the filename extension (either "" or ".html") at the place where
term_tag_init() is called?  That (main.c) is the module where the OUTT_
constants are defined, so it's the natural place to make decisions based
on them.  Then just pass a third const char * into term_tag_init().

Or do you see any downside that approach might have?

*If* people ever request the same for PDF, it makes adding that even
easier.

Also, maybe put the new argument in the middle position because it is
only related to the first argument and not to the second.

Finally and KNFly, please refrain from using function calls as
variable initializers.

Yours,
  Ingo



Re: syspatch exit state

2020-12-07 Thread Ingo Schwarze
Hi Antoine,

Antoine Jacoutot wrote on Mon, Dec 07, 2020 at 01:39:30PM +0100:
> On Mon, Dec 07, 2020 at 01:30:55PM +0100, Ingo Schwarze wrote:
>> Antoine Jacoutot wrote on Mon, Dec 07, 2020 at 01:01:27PM +0100:

>>> I just tested this change and it seems to work:
[...]
>> I think a slightly more explicit wording might make such a
>> misunderstanding less likely, for example:
>> 
>>   .Sh EXIT STATUS
>>   .Ex -std syspatch
>>   In particular, 2 indicates that applying patches was requested
>>   but no additional patch was installed.
[...]

> Sure that's fine as well.
> I did change it to your initial proposal;

Thanks, but...
I initially documented what you originally implemented.

You changed what your implementation does, so the documentation
needed to change as well to match the second iteration of the
implementation.

> but careful, since you're a doc master
> I will put whatever you send my way ;-)

Sometimes, i am wrong, so i appreciate it when people read my
suggestions with a critical eye.  Authors usually know their code
better than i do, and the documentation being correct is even
more important than being nicely worded and properly formatted.  ;-)


> Index: syspatch.8
> ===
> RCS file: /cvs/src/usr.sbin/syspatch/syspatch.8,v
> retrieving revision 1.21
> diff -u -p -r1.21 syspatch.8
> --- syspatch.825 Jul 2020 15:45:44 -  1.21
> +++ syspatch.87 Dec 2020 12:39:07 -
> @@ -64,6 +64,8 @@ of installed patches.
>  .El
>  .Sh EXIT STATUS
>  .Ex -std syspatch
> +In particular, 2 indicates that applying patches was requested but no
> +additional patch was installed.
>  .Sh SEE ALSO
>  .Xr signify 1 ,
>  .Xr installurl 5 ,
> Index: syspatch.sh
> ===
> RCS file: /cvs/src/usr.sbin/syspatch/syspatch.sh,v
> retrieving revision 1.166
> diff -u -p -r1.166 syspatch.sh
> --- syspatch.sh   27 Oct 2020 17:42:05 -  1.166
> +++ syspatch.sh   7 Dec 2020 12:39:07 -
> @@ -320,6 +320,7 @@ if ((OPTIND == 1)); then
>   [[ -f ${_D}/rollback.tgz ]] || rm -r ${_D}
>   done
>   _PATCHES=$(ls_missing) # can't use errexit in a for loop
> + [[ -n ${_PATCHES} ]] || exit 2
>   for _PATCH in ${_PATCHES}; do
>   apply_patch ${_OSrev}-${_PATCH}
>   _PATCH_APPLIED=true



Re: syspatch exit state

2020-12-07 Thread Ingo Schwarze
Hello Antoine,

Antoine Jacoutot wrote on Mon, Dec 07, 2020 at 01:01:27PM +0100:

> I just tested this change and it seems to work:

I did not repeat my testing, but here is some quick feedback purely
from code inspection:

The proposed code change makes sense to me.

The proposed manual page text might allow the misconception that
the tool will always exit 2 if no additional patch was installed,
including when -c, -l, or -r was specified.

I think a slightly more explicit wording might make such a
misunderstanding less likely, for example:

  .Sh EXIT STATUS
  .Ex -std syspatch
  In particular, 2 indicates that applying patches was requested
  but no additional patch was installed.

With non-standard meanings of exit codes, i think it is worthwhile
to be as precise as possible.

Yours,
  Ingo


> Index: syspatch.8
> ===
> RCS file: /cvs/src/usr.sbin/syspatch/syspatch.8,v
> retrieving revision 1.21
> diff -u -p -r1.21 syspatch.8
> --- syspatch.825 Jul 2020 15:45:44 -  1.21
> +++ syspatch.87 Dec 2020 11:58:06 -
> @@ -64,6 +64,7 @@ of installed patches.
>  .El
>  .Sh EXIT STATUS
>  .Ex -std syspatch
> +2 indicates that no additional patch was installed.
>  .Sh SEE ALSO
>  .Xr signify 1 ,
>  .Xr installurl 5 ,
> Index: syspatch.sh
> ===
> RCS file: /cvs/src/usr.sbin/syspatch/syspatch.sh,v
> retrieving revision 1.166
> diff -u -p -r1.166 syspatch.sh
> --- syspatch.sh   27 Oct 2020 17:42:05 -  1.166
> +++ syspatch.sh   7 Dec 2020 11:58:06 -
> @@ -320,6 +320,7 @@ if ((OPTIND == 1)); then
>   [[ -f ${_D}/rollback.tgz ]] || rm -r ${_D}
>   done
>   _PATCHES=$(ls_missing) # can't use errexit in a for loop
> + [[ -n ${_PATCHES} ]] || exit 2
>   for _PATCH in ${_PATCHES}; do
>   apply_patch ${_OSrev}-${_PATCH}
>   _PATCH_APPLIED=true



Re: syspatch exit state

2020-12-07 Thread Ingo Schwarze
Hi Antoine,

Antoine Jacoutot wrote on Mon, Dec 07, 2020 at 09:48:36AM +0100:
> On Sun, Dec 06, 2020 at 10:52:37PM +0100, Alexander Hall wrote:
>> On December 6, 2020 8:13:26 PM GMT+01:00, Antoine Jacoutot wrote:
>>> On Sun, Dec 06, 2020 at 05:20:31PM +, Stuart Henderson wrote:
 On 2020/12/06 16:39, Otto Moerbeek wrote:
> On Sun, Dec 06, 2020 at 03:31:19PM +, SW wrote:
>> On 06/12/2020 14:32, Otto Moerbeek wrote:
>>> On Sun, Dec 06, 2020 at 02:19:05PM +, SW wrote:

 I've been looking to have syspatch give me a quick indication
 of whether a reboot is likely to be required. As a quick and
 dirty check, I've just been treating "Were patches applied?"
 as the indicator.
 The following diff will cause syspatch to exit when applying
 patches with status code 0 only if patches were actually applied.
 My biggest concern is that it does cause a change in behaviour,
 so perhaps this either needs making into an option or a different
 approach entirely?
[...]
>>> I don't this is correct since it maks syspatch exit 1 on "no
>>> patches applied".

>> That's precisely the idea- from previous discussion with a couple
>> of people there didn't seem to be an easy (programmatic) way to
>> figure out whether syspatch had done anything or not.

> exit code 1 normally used for error conditions. A system being
> up-to-date is not an error condition. 

>> Doing this would be a bit of a blunt way of handling things, and
>> perhaps it would be better gated behind a flag, but is there a
>> better way to make a scripted update work automatically
>> (including rebooting as necessary)?

 How about the same exit codes as acme-client? They seem fairly
 reasonable - 0=updated, 1=failure, 2=no change.

>>> I wouldn't object to that.

>> So that'd boil down to
>> $_PATCH_APPLIED || exit 4
>> or
>> $_PATCH_APPLIED && exit
>> exit 4
>> ...if the explicit exit feels better instead of just running to the
>> end of the script.
>> But maybe this script prefers some more verbosity... :-)

> Something like this?

I'm on the fence whether regarding an up-to-date system as an error
condition makes sense; i neither like the idea nor object to it.

But if you decide to head into that direction, i suggest to try
and make the documentation more precise.  "No patch is available"
sounds as if the OpenBSD project did not yet provide a single
patch for the release in question.

> Index: syspatch.8
> ===
> RCS file: /cvs/src/usr.sbin/syspatch/syspatch.8,v
> retrieving revision 1.21
> diff -u -p -r1.21 syspatch.8
> --- syspatch.825 Jul 2020 15:45:44 -  1.21
> +++ syspatch.87 Dec 2020 08:47:48 -
> @@ -64,6 +64,7 @@ of installed patches.
>  .El
>  .Sh EXIT STATUS
>  .Ex -std syspatch
> +2 indicates that no patch is available.

To describe your proposed implementation, i think you would have
to say something like:

  2 indicates that no additional patch was installed.

But i doubt that is what you actually intend to do.

>  .Sh SEE ALSO
>  .Xr signify 1 ,
>  .Xr installurl 5 ,
> Index: syspatch.sh
> ===
> RCS file: /cvs/src/usr.sbin/syspatch/syspatch.sh,v
> retrieving revision 1.166
> diff -u -p -r1.166 syspatch.sh
> --- syspatch.sh   27 Oct 2020 17:42:05 -  1.166
> +++ syspatch.sh   7 Dec 2020 08:47:48 -
> @@ -248,7 +248,8 @@ must be run manually to install the new 
>   fi
>   fi
>  
> - ${_PATCH_APPLIED} && echo "Errata can be reviewed under ${_PDIR}"
> + ${_PATCH_APPLIED} && echo "Errata can be reviewed under ${_PDIR}" ||
> + return 2

This doesn't appear to work unless i screwed up my testing.
Even if _PATCH_APPLIED==false, syspatch still exits 0 for me.
The return value of the trap function doesn't appear to affect
the exit status of the script.

If i change "return 2" to "exit 2", (and insert a debugging statement

  echo _PATCH_APPLIED = ${_PATCH_APPLIED}

right before the "${_PATCH_APPLIED} && ..." test), then i see
the following potentially unintended behaviour:

  # /usr/sbin/syspatch -r ; echo $? 
  Reverting patch 007_xmaplen
  _PATCH_APPLIED = false
  2

That means -r would never again succeed?  And now:

  # /usr/sbin/syspatch -c ; echo $? 
  007_xmaplen
  _PATCH_APPLIED = false
  2

  # /usr/sbin/syspatch -l ; echo $? 
  001_bgpd
  002_icmp6
  003_tmux
  004_wg
  005_unwind
  006_rpki
  _PATCH_APPLIED = false
  2

So listing patches is an error when new patches are available?

  # /usr/sbin/syspatch ; echo $?
  Get/Verify syspatch68-007_xmaplen... 100% |*|  4547 KB00:00
  Installing patch 007_xmaplen
  _PATCH_APPLIED = true
  Errata can be reviewed under /var/syspatch
  0

That is the only test result matching what i think you might intend.
But now, this still happens:


Re: sio_open.3: clarify what sio_start() does

2020-11-28 Thread Ingo Schwarze
Hi Alexandre,

Alexandre Ratchov wrote on Fri, Nov 27, 2020 at 07:05:27PM +0100:

> this wording is shorter and more precise and complete.

This looks good mdoc(7)-wise, so go ahead, but please consider
the two nits below when committing.

Yours,
  Ingo


> Index: sio_open.3
> ===
> RCS file: /cvs/src/lib/libsndio/sio_open.3,v
> retrieving revision 1.51
> diff -u -p -r1.51 sio_open.3
> --- sio_open.320 Nov 2020 12:09:45 -  1.51
> +++ sio_open.327 Nov 2020 18:02:16 -
> @@ -387,17 +387,17 @@ bitmasks should always be used.
>  .Ss Starting and stopping the device
>  The
>  .Fn sio_start
> -function puts the device in a waiting state:
> -the device will wait for playback data to be provided
> -(using the
> -.Fn sio_write
> -function).
> -Once enough data is queued to ensure that play buffers
> -will not underrun, actual playback is started automatically.
> -If record mode only is selected, then recording starts
> -immediately.
> +function prepares the devices to start.

As Erico already noticed, i think you need to go back to the singular
here, s/devices/device/.  Otherwise, if changing to the plural is
intentional, a clarification might make sense which other device(s)
apart from .Fa hdl the text intends to talk about.

> +Once the play buffer is full, i.e.

This line should be:

  Once the play buffer is full, i.e.\&

If a dot at the end of an input line is not a full stop,
it needs escaping to prevent excessive horizontal spacing.

> +.Fa sio_par.bufsz
> +samples are queued with
> +.Fn sio_write ,
> +playback starts automatically.
> +If record mode only is selected, then
> +.Fn sio_start
> +starts recording immediately.
>  In full-duplex mode, playback and recording will start
> -synchronously as soon as enough data to play is available.
> +synchronously as soon as the play buffer is full.
>  .Pp
>  The
>  .Fn sio_stop



Re: AUDIORECDEVICE environment variable in sndio lib

2020-11-17 Thread Ingo Schwarze
Hi Solene,

sorry if i misunderstand because i did not fully follow the thread...

Solene Rapenne wrote on Tue, Nov 17, 2020 at 07:36:38PM +0100:

> I added the new variable after MIDIDEVICE in the ENVIRONMENT section
> to keep order of appearance in the document.

Usually, we order the ENVIRONMENT section alphabetically.

Also, on first sight, the text of AUDIODEVICE and AUDIORECDEVICE
seems to cause a contradiction.  The earlier text below "Default
Audio and MIDI devices" appears to resolve the apparent contradiction,
but wouldn't it be better to give a complete definition at one
place?  For example similar to ths:

.It Ev AUDIODEVICE
Audio device descriptor to use for playback when no descriptor is
explicitly specified to a program.  It is also used for recording if
.Ev AUDIORECDEVICE
is unset.
.It Ev AUDIORECDEVICE
Audio device descriptor to use for recording when no descriptor is
explicitly specified to a program.
.It Ev MIDIDEVICE
[... unchanged ...]

And below "Default Audio and MIDI devices" something like:

When no audio device descriptor is provided to a program
or when the reserved word
.Cm default
is passed as the device descriptor,
the program uses the one specified in the
.Ev AUDIODEVICE
environment variable for playback and the one specified in
.Ev AUDIORECDEVICE
for recording, falling back to
.Ev AUDIODEVICE
if the latter is unset.
If the applicable variables are unset, the program first tries
to connect to [...]

Feel free to ignore me if i misunderstood or if you disagree,
or to tweak the wording according to your taste.

Yours,
  Ingo


> Index: sndio.7
> ===
> RCS file: /home/reposync/src/lib/libsndio/sndio.7,v
> retrieving revision 1.24
> diff -u -p -r1.24 sndio.7
> --- sndio.7   18 Jul 2020 05:01:14 -  1.24
> +++ sndio.7   17 Nov 2020 18:32:34 -
> @@ -157,6 +157,10 @@ If it is not set, the program first trie
>  .Li snd/0 .
>  If that fails, it then tries to use
>  .Li rsnd/0 .
> +The
> +.Ev AUDIORECDEVICE
> +environment variable
> +defines a device to use prior to the audio device when recording.
>  .Pp
>  Similarly, if no MIDI descriptor is provided to a program
>  or when the reserved word
> @@ -196,6 +200,9 @@ Audio device descriptor to use
>  when no descriptor is explicitly specified to a program.
>  .It Ev MIDIDEVICE
>  MIDI port descriptor to use
> +when no descriptor is explicitly specified to a program.
> +.It Ev AUDIORECDEVICE
> +Audio recording device descriptor to use
>  when no descriptor is explicitly specified to a program.
>  .El
>  .Pp
> 



Re: Import seq(1) from FreeBSD

2020-11-17 Thread Ingo Schwarze
Hi Todd,

in view of your arguments and sthen@'s OK, i'm also OK with this
going in.  I think a bit of code cleanup and copy editing in the
manual page may be useful afterwards, but that can be done in the
tree, no need for playing patch ping pong.

See below for answers to the individual points.  If you think any
of these can already be trivially handled by your initial commit,
feel free to do so.  Otherwise, we can reconsider after the commit.

Also note that this thing is UTF-8 neutral even though many other
utilities processing separators are not.  The -f, -s, and -t
arguments can contain UTF-8 and it just works.   No need to call
setlocale(3) or inspect LC_*.

Yours,
  Ingo


Todd C. Miller wrote on Mon, Nov 16, 2020 at 10:08:08AM -0700:
> On Mon, 16 Nov 2020 16:14:31 +0100, Ingo Schwarze wrote:
 
>> are you really sure this is a good idea?  The version you sent is
>> wildly incompatible with GNU sed.  So we add a non-standard utility
>> that exhibits different behaviour on different systems even though
>> a standard utility already exists for the purpose?

> I don't think we need to be bug-compatible with GNU seq

That's probably true.

> and characterizing jot as a "standard utility" is simply not accurate.

Oops.  You are right, it's a BSD utility, not a standard utility.
Seems like i got too used to having it around.

>>$ seq 3 -1 10 ; echo $?
>>   seq: needs positive increment
>>   1
>>$ gseq 3 -1 10 ; echo $?
>>   0

> This is not valid usage, you cannot get to 10 from 3 with a negative
> increment.  We could silently exit like GNU seq if that is desirable
> but is silently ignoring a usage error really what we want?

Granted that this can be subsumed under "bug-compatible" and that
the FreeBSD behaviour makes more sense.  Even though:

   $ /usr/local/plan9/bin/seq 3 -1 10 ; echo $?
  0

>>$ seq 3 0 10 ; echo $?   
>>   seq: zero increment
>>   1
>>$ gseq 3 0 10 ; echo $?
>>   gseq: Abort trap (core dumped) 
>>   134

> I get the following:
> gseq: invalid Zero increment value: ‘0’
> Try 'gseq --help' for more information.

How stupid of me.  I'm running the kernel+libc combo poisoning
printf(%n) in writeable memory, so i should really inspect
/var/log/messages when something hits abort(3), or running that
stuff is useless.  You are right, with gnulib inside coreutils
fixed, i see the some output as you do.  So no discrepancy here.

   $ /usr/local/plan9/bin/seq 3 0 10 ; echo $?  
  seq: zero increment
  1

>>$ seq 3 1 ; echo $?
>>   3
>>   2
>>   1
>>   0
>>$ gseq 3 1 ; echo $?
>>   0

> GNU seq uses a default increment of 1 even if first > last.
> Personally, I think using a default increment of -1 makes more sense
> in the above case, but we can easily make this match the GNU behavior
> if we desire.

I fully agree that the FreeBSD behaviour is more useful, even though

   $ /usr/local/plan9/bin/seq 3 1 ; echo $?
  0

If this goes in now, we are probably far enough away from a release
to notice if anything in ports builds depends on the GNU behaviour -
which does not seem very likely.  If the FreeBSD behaviour causes
problems, we can decide what to do later.

>>$ seq -f '%a' 3  
>>   0x1p+0
>>   0x1p+1
>>   0x1.8p+1
>>$ gseq -f '%a' 3
>>   0x8p-3
>>   0x8p-2
>>   0xcp-2

> The BSD seq output is consistent with printf(1), GNU seq is not.
> I'd classify this as a GNU bug.

Fair enough.  Besides, %a is documented as ambiguous.  Strangely,
the GNU form matches the description in our manual page (minimal
length of the mantissa) while our printf(3) output does not (it
seems to maximize the exponent instead, which is the same as
minimizing the *size*, not the *length* of the mantissa).

By the way, Plan 9 does not seem to support -f '%a' at all. 

>>$ seq -f '%i' 3  
>>   seq: invalid format string: `%i'
>>$ gseq -f '%i' 3 
>>   gseq: Abort trap (core dumped) 

> I get:
> gseq: format ‘%i’ has unknown %i directive

Yes, so there is no issue here.

>>$ seq -s / 3 ; echo $?
>>   1/2/3/0
>>$ gseq -s / 3 ; echo $?
>>   1/2/3
>>   0

> The missing newline appears to be a bug in the FreeBSD seq, NetBSD
> seq works correctly.

The more serious problem here is whether the -s argument should
appear after each number or merely between numbers.  That is not
a matter of bug compatibility but a serious difference in behaviour.

Both the FreeBSD and the GNU manual page say "separate", so
maybe the trailing separator in FreeBSD seq can be regarded as a bug.

The GNU behaviour also makes more sense because the FreeBSD behaviour
of "printing after" can simply be achieved by appending the -s argument
to the format string.  

Re: Import seq(1) from FreeBSD

2020-11-16 Thread Ingo Schwarze
Hi Todd,

are you really sure this is a good idea?  The version you sent is
wildly incompatible with GNU sed.  So we add a non-standard utility
that exhibits different behaviour on different systems even though
a standard utility already exists for the purpose?

Is this needed for porting work?  If so, which behavour is more
commonly expected - GNU compatible or FreeBSD compatible?

Would it be picked up by ./configure in ports, and to what effect?

Yours,
  Ingo


   $ seq 3 -1 10 ; echo $?
  seq: needs positive increment
  1
   $ gseq 3 -1 10 ; echo $?
  0

   $ seq 3 0 10 ; echo $?   
  seq: zero increment
  1
   $ gseq 3 0 10 ; echo $?
  gseq: Abort trap (core dumped) 
  134

   $ seq 3 1 ; echo $?
  3
  2
  1
  0
   $ gseq 3 1 ; echo $?
  0

   $ seq -f '%a' 3  
  0x1p+0
  0x1p+1
  0x1.8p+1
   $ gseq -f '%a' 3
  0x8p-3
  0x8p-2
  0xcp-2

   $ seq -f '%i' 3  
  seq: invalid format string: `%i'
   $ gseq -f '%i' 3 
  gseq: Abort trap (core dumped) 

   $ seq -s / 3 ; echo $?
  1/2/3/0
   $ gseq -s / 3 ; echo $?
  1/2/3
  0

   $ seq -s '.\n' 3 ; echo $?
  1.
  2.
  3.
  0
   $ gseq -s '.\n' 3 ; echo $?
  1.\n2.\n3
  0

   $ seq -s / -t '\n' 3 ; echo $? 
  1/2/3/
  0
   $ gseq -s / -t '\n' 3 ; echo $?
  gseq: unknown option -- t
  Try 'gseq --help' for more information.
  1
   $ seq --help  
  seq: unknown option -- help
  usage: seq [-w] [-f format] [-s string] [-t string] [first [incr]] last
   $ gseq --help
  [... prints a novel ...]

   $ seq 0 .5 1
  0
  0.5
  1
   $ gseq 0 .5 1
  0.0
  0.5
  1.0



Re: accton(8) requires a reboot after being enabled

2020-11-03 Thread Ingo Schwarze
Hi Jason,

Jason McIntyre wrote on Mon, Nov 02, 2020 at 05:29:37PM +:

> - adding EXIT STATUS makes sense. i agree.

So i added just the .Sh and .Ex lines.

All the rest (both regarding "file" and "install") seems controversial
and hardly worth have a long discussion, so i dropped all the rest.

Yours,
  Ingo



Re: accton(8) requires a reboot after being enabled

2020-11-02 Thread Ingo Schwarze
Hi Theo,

Theo de Raadt wrote on Fri, Oct 30, 2020 at 12:10:41PM -0600:

> Yes, that diff is a whole bunch of TOCTUO.
> 
> If this was going to be changed, it should be in the kernel.
> 
> But I don't know if it should be changed yet, which is why I asked
> a bunch of questions.
> 
> Stepping back to the man page change, we could decide that accton
> should continue to behave how it does, and this conversation started
> because the man page fell into the trap of documenting the rc scripts
> RATHER than documenting accton.

Given that nobody seems in a rush to patch the kernel, i suggest to
improve the accton(8) manual page for now.  In particular, that
manual page lacks the required EXIT STATUS section, which happens
to be a natural place for mentioning what happens if the file does
not exist.

As usual, an EXAMPLES section is not strictly required, but in this
case, it seems useful to me, to save people from having to inspect
/etc/mtree/special for the recommended file permissions.

OK?
  Ingo


Index: accton.8
===
RCS file: /cvs/src/usr.sbin/accton/accton.8,v
retrieving revision 1.12
diff -u -r1.12 accton.8
--- accton.82 Nov 2020 13:58:44 -   1.12
+++ accton.82 Nov 2020 14:19:43 -
@@ -64,6 +64,17 @@
 .It Pa /var/account/acct
 default accounting file
 .El
+.Sh EXIT STATUS
+.Ex -std
+For example, it is an error if the
+.Ar file
+does not exist.
+.Sh EXAMPLES
+The following commands enable accounting if it was never used before:
+.Bd -literal -offset 4n
+# install -o root -g wheel -m 0644 /dev/null /var/account/acct
+# accton /var/account/acct
+.Ed
 .Sh SEE ALSO
 .Xr lastcomm 1 ,
 .Xr acct 2 ,



Re: accton(8) requires a reboot after being enabled

2020-10-30 Thread Ingo Schwarze
Hi Solene,

Solene Rapenne wrote on Fri, Oct 30, 2020 at 06:34:09PM +0100:

> Following diff changes accton(8) behavior.
> 
> If the file given as parameter doesn't exists, accton will create it.
> Then it will check the ownership and will make it root:wheel if
> it's different.
> 
> I added a check to be sure it's run as root because it's has no use if
> not run as root.

If it naturally runs into privileged system calls anyway and the
error message is comprehensible, that is not necessarily needed.
Currently, the error message seems OK to me:

   $ accton
  accton: Operation not permitted

> I don't often write C, if the logic is good but the C implementation
> wrong I'm open to critic.
> 
> The -f test and touch in /etc/rc won't be needed anymore with this
> change.
> 
> 
> Index: accton.8
> ===
> RCS file: /home/reposync/src/usr.sbin/accton/accton.8,v
> retrieving revision 1.11
> diff -u -p -r1.11 accton.8
> --- accton.8  10 Nov 2019 20:51:53 -  1.11
> +++ accton.8  30 Oct 2020 17:27:36 -
> @@ -36,7 +36,7 @@
>  .Nm accton
>  .Op Ar file
>  .Sh DESCRIPTION
> -With an argument naming an existing
> +With an argument naming a
>  .Ar file ,
>  .Nm
>  causes system accounting information for every process executed

*If* we change accton(8) to create the file if it does not exist,
the manual should probably be more explicit and say that an
existing file is appended to, but that it is created if it does
not exist.

Maybe it should even say something like

  Unlike other implementations, this version of
  .Nm
  creates the
  .Ar file
  if it does not exist.

in order to not set a trap for experienced users.

> Index: accton.c
> ===
> RCS file: /home/reposync/src/usr.sbin/accton/accton.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 accton.c
> --- accton.c  27 Oct 2009 23:59:50 -  1.8
> +++ accton.c  30 Oct 2020 17:26:31 -
> @@ -27,6 +27,7 @@
>   * SUCH DAMAGE.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -47,6 +48,10 @@ int
>  main(int argc, char *argv[])
>  {
>   int ch;
> + struct stat info_file;
> +
> + if(getuid() != 0)
> + err(1, "need root privileges");
>  
>   while ((ch = getopt(argc, argv, "")) != -1)
>   switch(ch) {
> @@ -63,6 +68,15 @@ main(int argc, char *argv[])
>   err(1, NULL);
>   break;
>   case 1:
> + if(stat(*argv,_file) != 0)
> + if(fopen(*argv,"w"))

Uh oh, that looks like a TOCTOU race condition to me.

> + if(stat(*argv,_file))

And that looks like another TOCTOU race condition.
Checking the return value of fopen(3) might be better.

Leaking a file descriptor from open(2) is also unusual.
Arguably, it may not be a problem here because the program
promptly exits anyway, but is is not nice for auditors.

Finally, what do you need fopen(3) for?
Wouldn't open(2) be sufficient?

> + err(1, "file %s couldn't be created", 
> *argv);
> +
> + if (info_file.st_uid != 0 || info_file.st_gid != 0)
> + if(chown(*argv, 0, 0) != 0)
> + err(1, "Impossible to fix %s ownership", *argv);
> +

Isn't that yet another TOCTOU race condition?
I guess using fchown(2) might be more appropriate?

Also, fchmod(2) seems to be lacking, or even better, couldn't that
be covered by the third argument to open(2)?

>   if (acct(*argv))
>   err(1, "%s", *argv);

Arguably, there might also be a race condition between userland and
the kernel, but i'm not so sure about kernel land.  Given how system
calls like open(2) work - taking a path, returning an int to avoid
race conditions - wouldn't it be more natural to have the acct(2)
system call create the file on the kernel side if necessary?

Yours,
  Ingo



Re: accton(8) requires a reboot after being enabled

2020-10-30 Thread Ingo Schwarze
Hi Theo,

Theo de Raadt wrote on Fri, Oct 30, 2020 at 09:59:09AM -0600:

> With a careful reading of the current manual page, everything is there
> and it is accurate.
> 
> With an argument naming an existing file
>
> 
> Ok so let's create it with touch.  Probably has wrong permissions.
> But now accton to that file works.  Or enable it and reboot, and now
> disable it and reboot, and the file still exists, so now accton works
> because it is an existing file (with the right permissions I guess).
> 
> So it *IS* working as documented.  It is just a bit weird, because the
> accton command (and system call) do not create the file.
> 
> 
> My problem with these changes is this is the man page for the accton(8)
> command, it documents *the binary*.  The manpage has already been subverted
> talk about rcctl, and about how /etc/rc runs the command.  But the man
> page should first and foremost be about the command, not about /etc/rc
> and rcctl, am i not right?

You are right.

The paragraph about rc.conf.local(8) is short (one sentence, one
line and three half lines) and near the end of the DESCRIPTION,
which is appropriate IMHO.  It feels more prominent than it should
only because the accton(8) command itself is so simple and
straightforward that it can be described exhaustively in three
lines of text.

> For instance, the ntpd manual page has a tiny section about rc.conf.

That paragraph has more or less the same length as the one in accton(8).

> So in conclusion, I think both of you are writing text about the startup
> subsystem, into the wrong manual page.  I think both proposals are skewed.
>
> So questions.
>
> 1 - historically it requires a file to be pre-created.  In the rc scripts,
> this is a touch.  That grabs the umask and ownership of root's run of
> /etc/rc.
> 2 - could we do better, in some way?
> 3 - accton system call does not have a umask, is that where this design came
> from?
> 4 - could we improve upon this?
> 5 - could accton always (attempt to) create the file, without harming
> existing use cases, with proper owner and chmod flags?

Maybe.  It would change behaviour, though.

Somebody might rely on the fact that accton(8) is a NOOP when the
file given as an argument does not exist.  I'm not saying that is
necessarily a good idea, but someone might have done it, for
example because they have to maintain a herd of similar machines,
want accounting on some but not on others, deploy the same
/etc/rc.conf.local to all and create or delete the file as desired.

Having the kernel or accton(8) create the file if it does not exist
would make the tool slightly easier to use for most users.  Is that
worth asking users doing soemthing like the above to change their
deployment?  Maybe, but i'm not sure.  The advantages feel small
either way.

> 6 - or should that be tied to a flag, making it easier to document?

I somewhat dislike that.  Either we regard creating the file as
part of the job.  Then accton(8) ought to do it without a flag, and
people who don't want accounting should not call it with an argument
in the first place.  Or we regard creating and deleting the file
as a separate decision, like we do it now.  Then accton(8) should
not meddle with it, not even as a matter of a flag.  Besides,
creating a root:wheel 0644 file is not rocket science requiring an
additional option in some specialized program to be accomplished.
For that purpose, several adequate and flexible tools already exist,
including install(1).


Triggered by looking at this manual page: Let's trim the ridiculous
HISTORY section.  No, it didn't exist forever.  No, the manual page
isn't new.

A predecessor "gacct(8)" may have existed briefly in v4, but i'm
not wholly convinced.  There is only a skeleton manual page

  https://minnie.tuhs.org/cgi-bin/utree.pl?file=V4/man/manx/gacct.8

not even containing a DESCRIPTION.  I found no code, and it seems
it probably wasn't contained in v5 nor in v6.  So it seems best
to not mention it.

OK?
  Ingo


Index: accton.8
===
RCS file: /cvs/src/usr.sbin/accton/accton.8,v
retrieving revision 1.11
diff -u -p -r1.11 accton.8
--- accton.810 Nov 2019 20:51:53 -  1.11
+++ accton.830 Oct 2020 16:43:55 -
@@ -73,4 +73,5 @@ default accounting file
 .Sh HISTORY
 The
 .Nm
-command has existed nearly forever, but this man page is new.
+command first appeared in
+.At v7 .



Re: [PATCH] Fix link in Porting Guide

2020-10-29 Thread Ingo Schwarze
Hi Martin,

Martin Vahlensieck wrote on Wed, Oct 28, 2020 at 09:02:21PM +0100:

> This refers to the libc function.

Committed, thanks.

> P.S.: I noticed that e.g. sysmerge(8) is mentioned like this but not a
> link. Is this intentional?

Probably not, i added a few more links while there.

Yours,
  Ingo


> Index: faq/ports/guide.html
> ===
> RCS file: /cvs/www/faq/ports/guide.html,v
> retrieving revision 1.91
> diff -u -p -r1.91 guide.html
> --- faq/ports/guide.html  15 Jul 2020 21:52:04 -  1.91
> +++ faq/ports/guide.html  28 Oct 2020 20:01:21 -
> @@ -1320,7 +1320,7 @@ Another very common problem is the   Heed the warnings of the bsd linker about its uses.
>  These must be fixed.
>  This is not quite as simple as s/mktemp/mkstemp/g.
> -Refer to https://man.openbsd.org/mktemp;>mktemp(3) for more
> +Refer to https://man.openbsd.org/mktemp.3;>mktemp(3) for more
>  information.
>  Correct code using mkstemp includes the source to
>  ed or mail.



Re: [PATCH] Mention unsupported stacking in softraid(4)

2020-10-25 Thread Ingo Schwarze
Hi Jeremie and Filippo,

Jeremie Courreges-Anglas wrote on Sun, Oct 25, 2020 at 03:05:04PM +0100:
> On Sun, Oct 25 2020, "Filippo Valsorda"  wrote:

>> Based on the text in faq14.html, but using the manpage language.

> Makes sense.  I'm not sure .Em is useful here, though.

Indeed.  We use .Em very sparingly in manual pages because too much
markup tends to look ugly, because manual pages tend to have quite a
bit of markup from semantical macros already, and because .Em can
sometimes be mistaken for semantical markup rather than stress
emphasis.  So i removed the .Em before i committed this, see the
commit below.

> ok jca@ if someone wants to pick this up, else I'll just commit it in
> a few hours.

Thanks to jca@ for checking.

Yours,
  Ingo


CVSROOT:/cvs
Module name:src
Changes by: schwa...@cvs.openbsd.org2020/10/25 09:33:20

Modified files:
share/man/man4 : softraid.4 

Log message:
mention that stacking disciplines is not supported,
with wording similar to the FAQ;
suggested by Filippo Valsorda ;
tweak and OK jca@


Index: softraid.4
===
RCS file: /cvs/src/share/man/man4/softraid.4,v
retrieving revision 1.42
retrieving revision 1.43
diff -u -r1.42 -r1.43
--- softraid.4  27 Jun 2017 16:02:05 -  1.42
+++ softraid.4  25 Oct 2020 15:33:19 -  1.43
@@ -1,4 +1,4 @@
-.\"$OpenBSD: softraid.4,v 1.42 2017/06/27 16:02:05 tb Exp $
+.\"$OpenBSD: softraid.4,v 1.43 2020/10/25 15:33:19 schwarze Exp $
 .\"
 .\" Copyright (c) 2007 Todd T. Fries   
 .\" Copyright (c) 2007 Marco Peereboom 
@@ -15,7 +15,7 @@
 .\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 .\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 .\"
-.Dd $Mdocdate: June 27 2017 $
+.Dd $Mdocdate: October 25 2020 $
 .Dt SOFTRAID 4
 .Os
 .Sh NAME
@@ -200,6 +200,9 @@
 .Pp
 The RAID 5 discipline does not initialize parity upon creation, instead parity
 is only updated upon write.
+.Pp
+Stacking disciplines (CRYPTO on top of RAID 1, for example) is not
+supported at this time.
 .Pp
 Currently there is no automated mechanism to recover from failed disks.
 .Pp



Re: diff: fixing a few broken links on 68.html, and other minor things

2020-10-22 Thread Ingo Schwarze
Hi,

Andras Farkas wrote on Wed, Oct 21, 2020 at 09:19:43PM -0400:

> While reading 68.html I noticed some of the man page links pointed to
> the man pages in the wrong section of the manual. (at least, given the
> manual section numbers listed next to them in the 68.html text)
> I decided to fix these.

Thanks, i just committed these four fixes.

By the way, this is why i urge developers to get into the habit
of always writing manual page links in the form

  https://man.openbsd.org/name.section

The two additional keystrokes for the dot and the section number
won't wear out your fingertips, and experience shows that trying
to do it "only when needed" just doesn't work.  Practically everybody
(myself included) occasionally forgets a required number when trying
that, and a habit of checking every link would cause more work than
adding two bytes.

Besides, sometimes new pages appear later that cause a link to
change.  For example, we have editline(3) for a long time, and then
later on, editline(7) appeared, too.  At that point, a link to just
https://man.openbsd.org/editline would have changed target, even
for pages of *past* releases.

> While there, I also made the fixed links point at 6.8 man pages
> rather than -current man pages.

I didn't take these changes: sysctl(2), scsi(4), video(4), drm(4)
are not going away.

> This is important in case functionality mentioned in 68.html
> is changed or removed in a later version of OpenBSD.

No that isn't important, even if in rare cases, old links go dead.
In general, it is useful to have old release pages link to -current
documentation (because the reason for looking at old release
pages is often "when did this feature appear?", and then the new
manual page is more useful for users than an outdated version).
Besides, adding /OpenBSD-X.Y/ makes the links too long and ugly.

> I also turned two mentions of bettertls.com into hyperlinks.

Never send diffs mixing logically unrelated changes.

I committed that separately.

> Diff attached:
> SHA256 (68diff) =
> d16eb33d863866b004d75041e42c24100dd4200864a5b2243913f98ad5d8eaa9

Hashing diffs is useless.  The are scrutinized before commit anyway,
and what matters is whether they are correct, not whether they
contain what you intended.

More importantly, never send diffs as attachments; that's just a
waste of developer time.  Include them directly into the body of
the mail, ideally at the end.  And please don't waste more time
arguing that your mail client can't do that; in that case just get
a decent mail client *before* sending diffs.

> Also, some links on 68.html (like amlpwrc

Hopefully, somebody is going to document that driver eventually,
and then the link will come alive.

> and sftp-client)

That was just an error: sftp-client.c is the source file, but the
program and the manual page are called sftp(1).  Fixed.

[...]
> issues noticed by
> https://validator.w3.org/nu/

Done, too.

> There was also a mention of sshd(1) that should probably be ssh(1) but
> I wasn't sure.  Please check.

Right, that's sshconnect2.c rev. 1.322, order_hostkeyalgs(), called
from ssh_kex2(), called from ssh_login(), called from ssh.c main(),
which is the ssh(1) client, not sshd(8).  Fixed.

Yours,
  Ingo



Re: libexec/security: don't prune mount points

2020-10-11 Thread Ingo Schwarze
Hi Todd,

Todd C. Miller wrote on Wed, Oct 07, 2020 at 09:36:33AM -0600:

> The recent changes to the daily security script will result in it
> not traversing file systems where the parent mount point is mounted
> with options nodev,nosuid but the child is mounted with setuid
> enabled.
> 
> For example, if /var/www is a separate file system that allows
> setuid but /var is mounted with nodev and nosuid, security will not
> traverse /var/www.
> 
> 198976b83d9da70f.e /var ffs rw,nodev,nosuid 1 2
> 198976b83d9da70f.f /var/www ffs rw 1 2
> 
> The simplest solution is to pass the list of file systems to traverse
> to File::Find and use the equivalent of find's -xdev option.
>
> Anyone want to double-check my logic? :-)

OK schwarze@;
feel free to use the two nits below, inline.

The patch also works.
On one of my machines, i have:
/co ffs rw,nodev,nosuid 1 2
/co/destdir ffs rw,noexec,noperm 1 2
The destdir gets checked while the parent /co does not.
Also, i see no regressions, not even with
/usr/ports ffs rw,nodev,nosuid 1 2
/usr/ports/pobj ffs rw,nodev,nosuid,wxallowed 1 2

Yours,
  Ingo


> Index: libexec/security/security
> ===
> RCS file: /cvs/src/libexec/security/security,v
> retrieving revision 1.40
> diff -u -p -u -r1.40 security
> --- libexec/security/security 17 Sep 2020 06:51:06 -  1.40
> +++ libexec/security/security 7 Oct 2020 15:34:14 -
> @@ -530,6 +530,7 @@ sub strmode {
>  
>  sub find_special_files {
>   my %skip;
> + my @fs;

The rest of the file uses the compact notation:

my (%skip, @fs);

>  
>   %skip = map { $_ => 1 } split ' ', $ENV{SUIDSKIP}
>   if $ENV{SUIDSKIP};
> @@ -541,11 +542,11 @@ sub find_special_files {
>   and return;
>   while (<$fh>) {
>   my ($path, $opt) = /\son\s+(.*?)\s+type\s+\w+(.*)/;
> - $skip{$path} = 1 if $path &&
> - ($opt !~ /local/ ||
> -  ($opt =~ /nodev/ && $opt =~ /nosuid/));
> + push(@fs, $path) if $path && $opt =~ /local/ &&

The parentheses are redundant, just

push @fs, $path if ...

is sufficient.

> + !($opt =~ /nodev/ && $opt =~ /nosuid/);
>   }
>   close_or_nag $fh, "mount" or return;
> + return unless @fs;
>  
>   my $setuid_files = {};
>   my $device_files = {};
> @@ -554,14 +555,19 @@ sub find_special_files {
>   File::Find::find({no_chdir => 1, wanted => sub {
>  
>   if ($skip{$_}) {
> - no warnings 'once';
>   $File::Find::prune = 1;
>   return;
>   }
>  
>   my ($dev, $ino, $mode, $nlink, $uid, $gid, $rdev, $size,
>   $atime, $mtime, $ctime, $blksize, $blocks) = lstat;
> - unless (defined $dev) {
> + if (defined $dev) {
> + no warnings 'once';
> + if ($dev != $File::Find::topdev) {
> + $File::Find::prune = 1;
> + return;
> + }
> + } else {
>   nag !$!{ENOENT}, "stat: $_: $!";
>   return;
>   }
> @@ -592,7 +598,7 @@ sub find_special_files {
>   $file->{size}= $size;
>   @$file{qw(wday mon day time year)} =
>   split ' ', localtime $mtime;
> - }}, '/');
> + }}, @fs);
>  
>   nag $uudecode_is_setuid, 'Uudecode is setuid.';
>   return $setuid_files, $device_files;



Re: Make df output more human friendly in daily(8)

2020-10-03 Thread Ingo Schwarze
Hi Daniel,

my OK still stands, except that you went too far in one respect
in the manual page, see below.

Yours,
  Ingo


Daniel Jakots wrote on Fri, Oct 02, 2020 at 03:41:31PM -0400:

> Index: share/man/man8/daily.8
> ===
> RCS file: /cvs/src/share/man/man8/daily.8,v
> retrieving revision 1.28
> diff -u -p -r1.28 daily.8
> --- share/man/man8/daily.826 Jul 2020 13:27:24 -  1.28
> +++ share/man/man8/daily.82 Oct 2020 19:34:47 -
> @@ -114,15 +114,6 @@ Lists any daemons which are enabled in
>  .Xr rc.conf.local 8
>  but which are not actually running.
>  .It
> -Checks disk status.
> -Reports on the amount of disk used/available via
> -.Xr df 1 .

So far, so good.

But this needs to remain:

> -Reports on which file systems need to be dumped via
> -.Xr dump 8 .
> -.It

>From here on, deletion is OK again.

> -Reports networking statistics via
> -.Xr netstat 1 .
> -.It
>  Runs the
>  .Xr calendar 1
>  utility unless the environment variable
> @@ -205,15 +196,6 @@ If set to 1, run
>  with the no-write flag.
>  .It Ev ROOTBACKUP
>  If set to 1, make a backup of the root file system.
> -.It Ev VERBOSESTATUS
> -If set to 0,
> -.Xr df 1 ,
> -.Xr dump 8 ,
> -and
> -.Xr netstat 1
> -are skipped.
> -Consequently, if none of the other commands produce any output,
> -no mail will be sent to root.
>  .El
>  .Pp
>  The following variables can be set in
> @@ -250,9 +232,7 @@ Root
>  .Sh SEE ALSO
>  .Xr calendar 1 ,
>  .Xr crontab 1 ,
> -.Xr df 1 ,
>  .Xr locate 1 ,
> -.Xr netstat 1 ,
>  .Xr rdist 1 ,
>  .Xr whatis 1 ,
>  .Xr crontab 5 ,



Re: Make df output more human friendly in daily(8)

2020-10-02 Thread Ingo Schwarze
Hi Daniel,

Daniel Jakots wrote on Fri, Oct 02, 2020 at 02:16:57PM -0400:
> On Fri, 2 Oct 2020 19:55:53 +0200, Ingo Schwarze wrote:
>> Daniel Jakots wrote on Thu, Oct 01, 2020 at 10:32:31PM -0400:

>>> Currently daily(8) runs `df -ikl`.  

>> By default, it does not.  It only does that if you set VERBOSESTATUS.

> Are you sure? It looks like it does not, *if* you set VERBOSESTATUS to
> 0. (And that's what daily(8) says as well).

Sorry for misremebering this and for not checking it before i posted.
You are right.

>> I would prefer deleting the VERBOSESTATUS parts completely,
>> strictly enforcing the principle "daily(8) only produces output
>> when something unexpected happens", and tell people to use
>> daily.local(8) if they want to run df or netstat.  The code
>> for those two parts is totally trivial and riddled with
>> choices that look like personal preferences, like the one
>> you suggest to change.

> I agree, that would a better change indeed. I think I'll fix my
> problem by setting VERBOSESTATUS to 0 and add what I want to my
> daily.local.
 
>> I dimly remember that some developers wanted to keep VERBOSESTATUS,
>> though (i might misremember), so we'll probably keep it.  If we
>> keep it, i absolutely don't care what it does.  So i'll neither OK
>> this nor object to it.

> Anyone cares about this one way or the other? Here's a diff for it. (If
> we want to go this way, I'll craft a diff for current.html as well).

I certainly like this, and it works for me.

But i think a change like this would need more than one OK,
and you should wait some days such that developers can raise
objections.

Just in case you get sufficient OKs and there are no serious
objections, see below for two suggested tweaks.

Yours,
  Ingo


> Index: ./share/man/man8/daily.8
> ===
> RCS file: /cvs/src/share/man/man8/daily.8,v
> retrieving revision 1.28
> diff -u -p -r1.28 daily.8
> --- ./share/man/man8/daily.8  26 Jul 2020 13:27:24 -
> 1.28 +++ ./share/man/man8/daily.8 2 Oct 2020 18:12:39 -
> @@ -205,15 +205,6 @@ If set to 1, run
>  with the no-write flag.
>  .It Ev ROOTBACKUP
>  If set to 1, make a backup of the root file system.
> -.It Ev VERBOSESTATUS
> -If set to 0,
> -.Xr df 1 ,
> -.Xr dump 8 ,
> -and
> -.Xr netstat 1
> -are skipped.
> -Consequently, if none of the other commands produce any output,
> -no mail will be sent to root.
>  .El
>  .Pp
>  The following variables can be set in

The following would also have to go:

 * part of the list item "Checks disk status"
 * the list item "Reports networking statistics"
 * df(1) and netstat(1) in SEE ALSO

> Index: etc/daily
> ===
> RCS file: /cvs/src/etc/daily,v
> retrieving revision 1.93
> diff -u -p -r1.93 daily
> --- etc/daily 9 Sep 2019 20:02:26 -   1.93
> +++ etc/daily 2 Oct 2020 18:12:39 -
> @@ -137,20 +137,7 @@ next_part "Services that should be runni
>  rcctl ls failed
>  
>  next_part "Checking subsystem status:"

This output line should probably be updated, maybe something like:

   next_part "Backing up filesystems with dump:"

> -if [ "X$VERBOSESTATUS" != X0 ]; then
> - echo ""
> - echo "disks:"
> - df -ikl
> - echo ""
> - dump W
> -else
> - dump w | grep -vB1 ^Dump
> -fi
> -
> -next_part "network:"
> -if [ "X$VERBOSESTATUS" != X0 ]; then
> - netstat -ivn
> -fi
> +dump w | grep -vB1 ^Dump
>  
>  next_part "Running calendar in the background:"
>  if [ "X$CALENDAR" != X0 -a \



Re: Make df output more human friendly in daily(8)

2020-10-02 Thread Ingo Schwarze
Hi,

Daniel Jakots wrote on Thu, Oct 01, 2020 at 10:32:31PM -0400:

> Currently daily(8) runs `df -ikl`.

By default, it does not.  It only does that if you set VERBOSESTATUS.

I would prefer deleting the VERBOSESTATUS parts completely,
strictly enforcing the principle "daily(8) only produces output
when something unexpected happens", and tell people to use
daily.local(8) if they want to run df or netstat.  The code
for those two parts is totally trivial and riddled with
choices that look like personal preferences, like the one
you suggest to change.

I dimly remember that some developers wanted to keep VERBOSESTATUS,
though (i might misremember), so we'll probably keep it.  If we
keep it, i absolutely don't care what it does.  So i'll neither OK
this nor object to it.

Yours,
  Ingo



Re: drop support for afs, nnpfs, and procfs from security(8)

2020-09-17 Thread Ingo Schwarze
Hi Todd,

Todd C. Miller wrote on Wed, Sep 16, 2020 at 01:36:09PM -0600:
> On Wed, 16 Sep 2020 18:17:36 +0200, Ingo Schwarze wrote:

>> Does anyone think that explicitely excluding these file system
>> types might still be useful, or is the following simplification
>> OK?  No functional change intended.

> I think those bits can go.  OK millert@

Committed, thanks for checking.
  Ingo



drop support for afs, nnpfs, and procfs from security(8)

2020-09-16 Thread Ingo Schwarze
Hi,

by chance, i noticed that security(8) is careful to avoid scanning
filesystems of the types "afs", "nnpfs", and "procfs".  According
to "ls /sbin/mount*", no such file systems are supported, and the
only page "man -ak any=afs any=nnpfs any=procfs" brings up seems
to be sshd_config(5) talking about KerberosGetAFSToken, which seems
tangentially related at best.  Even pkg_locate(1) comes up empty-handed
with respect to mount_afs, mount_nnpfs, and mount_procfs.

Does anyone think that explicitely excluding these file system
types might still be useful, or is the following simplification
OK?  No functional change intended.

Yours,
  Ingo


Index: security
===
RCS file: /cvs/src/libexec/security/security,v
retrieving revision 1.39
diff -U4 -p -r1.39 security
--- security14 Sep 2020 14:43:13 -  1.39
+++ security14 Sep 2020 15:11:07 -
@@ -539,11 +539,11 @@ sub find_special_files {
nag !(open my $fh, '-|', 'mount'),
"cannot spawn mount: $!"
and return;
while (<$fh>) {
-   my ($path, $type, $opt) = /\son\s+(.*?)\s+type\s+(\w+)(.*)/;
+   my ($path, $opt) = /\son\s+(.*?)\s+type\s+\w+(.*)/;
$skip{$path} = 1 if $path &&
-   ($type =~ /^(?:a|nnp|proc)fs$/ || $opt !~ /local/ ||
+   ($opt !~ /local/ ||
 ($opt =~ /nodev/ && $opt =~ /nosuid/));
}
close_or_nag $fh, "mount" or return;



Re: route.8, remove unprinted text

2020-09-10 Thread Ingo Schwarze
Hi Denis,

Denis Fondras wrote on Thu, Sep 10, 2020 at 10:09:14PM +0200:

> I can't see where these two lines are printed.

Not OK, please do not commit that diff.

You are correct that these two macros produce no output to the
terminal, but they do produce output to the ctags(1) file and they
also produce output in -T html mode.


In a shell, please try the following command:

  $ man -O tag=destination route

For documentation about what .Tg does, type

  $ man -O tag=Tg mdoc

to get a brief one-line summry,
then press the key "t" once for a complete description.

Also see:

  $ man -O tag=MANPAGER man
  $ man -O tag=tag mandoc

I admit the documentation is somewhat scattered, but given that
many components must work in concert for this - mdoc(7) language,
mandoc(1) formatter, man(1) viewer, less(1) pager, ctags(1) file
format - i'm not sure how to document it better.  Fresh ideas are
certainly welcome.

I also admit the commit message is somewhat short,
contrary to my habits:

  revision 1.91
  date: 2020/01/19 18:22:31;  author: schwarze;  state: Exp;
lines: +27 -2;  commitid: 4187beoLSE9wGXJc;
  add some explicit tagging macros; OK kn@ on a previous version

Yours,
  Ingo


> Index: route.8
> ===
> RCS file: /cvs/src/sbin/route/route.8,v
> retrieving revision 1.91
> diff -u -p -r1.91 route.8
> --- route.8   19 Jan 2020 18:22:31 -  1.91
> +++ route.8   10 Sep 2020 20:06:52 -
> @@ -197,8 +197,6 @@ If the priority is negative, then routes
>  priority are shown.
>  .El
>  .Pp
> -.Tg destination
> -.Tg gateway
>  The other commands relating to adding, changing, or deleting routes
>  have the syntax:
>  .Pp



Re: exFAT support

2020-08-06 Thread Ingo Schwarze
Hi,

in addition to what Bryan said...

This message is wildly off-topic on tech@.
If you reply, please reply to misc@.

Quoting from https://www.openbsd.org/mail.html (please read that!):

  Developer lists:
  [...]
  tech@openbsd.org
  Discussion of technical topics for OpenBSD developers and advanced
  users.  This is _not_ a "tech support" forum - do not use it as such.


jo...@armadilloaerospace.com wrote on Thu, Aug 06, 2020 at 02:16:11PM -0700:

> I tried to mount a 12TB USB drive, and was getting an "Inappropriate
> file type or format" error.

Even on misc@, when asking questions, please state what you are
actually doing, showing the exact commands you type and the exact
output you get, together in the original order, for example like
this:

  isnote# mount -t msdos /dev/sd1a /mnt
  mount_msdos: /dev/sd1a on /mnt: Inappropriate file type or format

> It turned out to be due to exFAT formatting, but it took me some
> investigating to figure that out.  Would it be reasonable to have the
> kernel print

You didn't say where you saw the message, but i assume it was the
output of mount(8), not kernel output on the system console or in
/var/log/messages.  The kernel doesn't print such messages.  The
application program (e.g. mount(8)) prints them.  All the kernel
does is return an errno(2) to the mount(2) syscall.  So you can
find an exhaustive list of the messages that could be printed in
the errno(2) manual page.  Also, which errno(2) is returned from
syscalls in which error case is not arbitrary and cannot be changed
arbitrarily.

> a more informative warning like "exFAT filesystem not
> supported" when you try to mount it with mount_msdos, or are additional
> kernel prints considered bad form?

I bet if the kernel had printed anything, you wouldn't even have
noticed.  Besides, yes indeed, the kernel is absolutely not supposed
to print anything when users type wrong commands.

Yours,
  Ingo



Re: ksh [emacs.c] -- simplify isu8cont()

2020-07-25 Thread Ingo Schwarze
Hi,

Martijn van Duren wrote on Sat, Jul 25, 2020 at 09:54:53PM +0200:

> This function is used throughout the OpenBSD tree and I think it's
> fine as it is. This way it's clearer to me that it's about byte
> 7 and 8 and not have to do the math in my head to check if we
> might have botched it.
> 
> Also compilers should be smart enough to optimize this out at
> compile-time anyway.

Indeed, that's exactly why tedu@ designed it as it is.
  Ingo


> On Sat, 2020-07-25 at 17:40 +0100, ropers wrote:

>> Index: emacs.c
>> ===
>> RCS file: /cvs/src/bin/ksh/emacs.c,v
>> retrieving revision 1.87
>> diff -u -r1.87 emacs.c
>> --- emacs.c  8 May 2020 14:30:42 -   1.87
>> +++ emacs.c  25 Jul 2020 16:31:22 -
>> @@ -269,10 +269,11 @@
>>  { 0, 0, 0 },
>>  };
>> 
>> +/* is octet a UTF-8 continuation byte? */
>>  int
>>  isu8cont(unsigned char c)
>>  {
>> -return (c & (0x80 | 0x40)) == 0x80;
>> +return (c & 0xC0) == 0x80;
>>  }
>> 
>>  int



Re: join(1) remove redundant memory copy

2020-07-23 Thread Ingo Schwarze
Hi Martijn,

this is a nice simplification, OK schwarze@.

A few nits:

 * The MAXIMUM() macro is now unused, so i prefer that you delete
   the definition.
 * The second getline(3) argument should be size_t, not u_long,
   so change that in the struct declaration (it's not used anywhere
   else).  Not sure whether your type mismatch might cause trouble
   on any of our platforms, or if the two types are identical
   everywhere.  While there, make the comment more intelligible.
 * From one comment, delete "and copy line" which is no longer true.

I'm appending the version of your patch that i tested.

Todd C. Miller wrote on Wed, Jul 22, 2020 at 01:19:47PM -0600:
> On Wed, 22 Jul 2020 20:29:06 +0200, Martijn van Duren wrote:

>>  /* Remove trailing newline, if it exists, and copy line. */
>> -if (line[len - 1] == '\n')
>> +if (lp->line[len - 1] == '\n')
>>  len--;
>>  lp->line[len] = '\0';

> You could replace "len--" with "lp->line[len - 1] = '\0'" here (or
> "lp->line[--len] = '\0'" if you prefer but len is otherwise unused).
> Then there would be no need to NUL terminate here since getline(3)
> always NUL terminates.

Indeed.  I think this form is clearest, shortest, and safest:

/* Remove the trailing newline, if any. */
if (lp->line[len - 1] == '\n')
p->line[--len] = '\0';

Yours,
  Ingo


Index: join.c
===
RCS file: /cvs/src/usr.bin/join/join.c,v
retrieving revision 1.32
diff -u -p -r1.32 join.c
--- join.c  14 Nov 2018 15:16:09 -  1.32
+++ join.c  23 Jul 2020 14:47:06 -
@@ -43,8 +43,6 @@
 #include 
 #include 
 
-#define MAXIMUM(a, b)  (((a) > (b)) ? (a) : (b))
-
 /*
  * There's a structure per input file which encapsulates the state of the
  * file.  We repeatedly read lines from each file until we've read in all
@@ -53,7 +51,7 @@
  */
 typedef struct {
char *line; /* line */
-   u_long linealloc;   /* line allocated count */
+   size_t linealloc;   /* bytes allocated for line */
char **fields;  /* line field(s) */
u_long fieldcnt;/* line field(s) count */
u_long fieldalloc;  /* line field(s) allocated count */
@@ -271,9 +269,8 @@ slurp(INPUT *F)
 {
LINE *lp, *lastlp, tmp;
ssize_t len;
-   size_t linesize;
u_long cnt;
-   char *bp, *fieldp, *line;
+   char *bp, *fieldp;
 
/*
 * Read all of the lines from an input file that have the same
@@ -281,8 +278,6 @@ slurp(INPUT *F)
 */
 
F->setcnt = 0;
-   line = NULL;
-   linesize = 0;
for (lastlp = NULL; ; ++F->setcnt) {
/*
 * If we're out of space to hold line structures, allocate
@@ -320,23 +315,12 @@ slurp(INPUT *F)
F->pushbool = 0;
continue;
}
-   if ((len = getline(, , F->fp)) == -1)
+   if ((len = getline(&(lp->line), &(lp->linealloc), F->fp)) == -1)
break;
 
-   /* Remove trailing newline, if it exists, and copy line. */
-   if (line[len - 1] == '\n')
-   len--;
-   if (lp->linealloc <= len + 1) {
-   char *p;
-   u_long newsize = lp->linealloc +
-   MAXIMUM(100, len + 1 - lp->linealloc);
-   if ((p = realloc(lp->line, newsize)) == NULL)
-   err(1, NULL);
-   lp->line = p;
-   lp->linealloc = newsize;
-   }
-   memcpy(lp->line, line, len);
-   lp->line[len] = '\0';
+   /* Remove the trailing newline, if any. */
+   if (lp->line[len - 1] == '\n')
+   lp->line[--len] = '\0';
 
/* Split the line into fields, allocate space as necessary. */
lp->fieldcnt = 0;
@@ -363,7 +347,6 @@ slurp(INPUT *F)
break;
}
}
-   free(line);
 }
 
 char *



Re: switch default MANPAGER from more(1) to less(1)

2020-07-20 Thread Ingo Schwarze
Hi,

ropers wrote on Mon, Jul 20, 2020 at 05:54:46AM +0100:

> Ah, I see where you're coming from, Ingo.  You've dropped the idea of
> testing for less(1) in non-portable mandoc because we know less(1) is
> in base.[1]

Configuration testing is never needed in a base system.  It may
sometimes linger when importing third-party software into base,
but even then, it is often deleted.

> I'm not certain I understand why they're passing /dev/null as
> a tagsfile.  Maybe to test if any -T  throws any error?

Exactly, that's what i wrote the HAVE_LESS_T test in mandoc-portable
for: to test whether less(1) has (support for) -T.

> (I also don't actually understand the redirection to file
> descriptor 3.  Cluebats welcome, on- or off-list.)

   $ sed -n /Output/,/3:/p configure
  # Output file descriptor usage:
  # 1 (stdout): config.h, Makefile.local
  # 2 (stderr): original stderr, usually to the console
  # 3: config.log

Anyway, i committed the patch to OpenBSD, thanks to all who provided
feedback.

No need to discuss mandoc-portable on tech@ any further.

Yours,
  Ingo



Re: switch default MANPAGER from more(1) to less(1)

2020-07-19 Thread Ingo Schwarze
Hi Jason & Theo,

thanks for the feedback!

Jason McIntyre wrote on Sun, Jul 19, 2020 at 05:02:02PM +0100:

> i guess the argument in favour of more(1) would be that it is part of
> posix, even if optional, where less(1) is not. so it makes sense to
> choose a command most likely to work on most machines.
> 
> having said that, i've nothing against the switch. i imagine it'd be
> hard to find a system without less(1).

I was only talking about mandoc in OpenBSD.  I rarely ask questions
about mandoc-portable on , and i rarely decide
what to do in mandoc-portable before things are committed to
OpenBSD.

It does indeed seem hard to find an OpenBSD system without less(1).  :)

Not yet sure what i will do in -portable.  Maybe test for the
availability of less(1) in ./configure, which is quite easy to do
and which ./configure already does for many operating system features.
That would also be convenient because the mandoc ./configure is set
up in such a way that it is trivial for downstream package maintainers
(say in Void Linux, FreeBSD, or Illumos) to manually override in
their package any result that ./configure automatically detects.
But that does not need to bother anyone here.  None of the (very
small amount of) portability code is in our OpenBSD CVS tree.

> about -s: it's inclusion probably comes from a time when there was an
> annoying bug in nroff that made our man pages randomly display a number
> of blank lines in the middle of a page. -s mitigated that somewhat.

Ah.  Good to know.  Then that's definitely no longer needed.

Thanks,
  Ingo



switch default MANPAGER from more(1) to less(1)

2020-07-19 Thread Ingo Schwarze
Hi,

currently, if neither the MANPAGER nor the PAGER environment variable
is set, man(1) uses "more -s" as the manual page pager.  I am quite
sure that the only reason i did this is that i thought this behaviour
was required by POSIX.

But it is not:

  https://pubs.opengroup.org/onlinepubs/9699919799/utilities/man.html

  "If the PAGER variable is null or not set,
   the command shall be either "more" or another
   paginator utility documented in the system documentation."

Right now, i even failed to find any indication that POSIX ever
required more(1) as the default for this purpose.  I no longer
understand where i got that idea from in the first place.

That said, on OpenBSD, the pager we provide is less(1).  In effect,
it is less(1) even when called as more(1), albeit with minor
differences in the default behaviour.  On top of that, our man(1)
utility heavily relies on less(1) features that tradional more(1)
wouldn't even have, in particular tagging support (:t).

So, i would find it logical to use less(1) as the default pager
instead of more(1).

Same thing for the "-s" option: i thought it was required by POSIX,
but it isn't.  I also provides little benefit, if any, and it may
occasionally break output, in the rare case where two consecutive
blank lines are syntacically significant (for example in an EXAMPLES
section presenting a code sample).  Besides, if the author deliberately
chose to put two consecutive blank lines, i don't see why man(1)
should squeeze them and override the author's intent.

Is anybody concerned about the following patch, or would you agree
with it?

Yours,
  Ingo


Index: apropos.1
===
RCS file: /cvs/src/usr.bin/mandoc/apropos.1,v
retrieving revision 1.41
diff -u -p -r1.41 apropos.1
--- apropos.1   22 Nov 2018 12:32:10 -  1.41
+++ apropos.1   19 Jul 2020 15:18:30 -
@@ -340,7 +340,7 @@ types appearing in function arguments in
 Any non-empty value of the environment variable
 .Ev MANPAGER
 is used instead of the standard pagination program,
-.Xr more 1 ;
+.Xr less 1 ;
 see
 .Xr man 1
 for details.
@@ -363,7 +363,7 @@ Specifies the pagination program to use 
 .Ev MANPAGER
 is not defined.
 If neither PAGER nor MANPAGER is defined,
-.Xr more 1
+.Xr less 1
 .Fl s
 is used.
 Only used if
Index: main.c
===
RCS file: /cvs/src/usr.bin/mandoc/main.c,v
retrieving revision 1.253
diff -u -p -r1.253 main.c
--- main.c  15 Jun 2020 17:25:03 -  1.253
+++ main.c  19 Jul 2020 15:18:31 -
@@ -1194,7 +1194,7 @@ spawn_pager(struct outstate *outst, char
if (pager == NULL || *pager == '\0')
pager = getenv("PAGER");
if (pager == NULL || *pager == '\0')
-   pager = "more -s";
+   pager = "less";
cp = mandoc_strdup(pager);
 
/*
Index: man.1
===
RCS file: /cvs/src/usr.bin/mandoc/man.1,v
retrieving revision 1.37
diff -u -p -r1.37 man.1
--- man.1   17 Jun 2020 19:41:25 -  1.37
+++ man.1   19 Jul 2020 15:18:31 -
@@ -275,7 +275,7 @@ is case insensitive.
 Any non-empty value of the environment variable
 .Ev MANPAGER
 is used instead of the standard pagination program,
-.Xr more 1 .
+.Xr less 1 .
 If
 .Xr less 1
 is used, the interactive
@@ -329,7 +329,7 @@ Specifies the pagination program to use 
 .Ev MANPAGER
 is not defined.
 If neither PAGER nor MANPAGER is defined,
-.Xr more 1
+.Xr less 1
 .Fl s
 is used.
 .El
Index: mandoc.1
===
RCS file: /cvs/src/usr.bin/mandoc/mandoc.1,v
retrieving revision 1.168
diff -u -p -r1.168 mandoc.1
--- mandoc.115 Jun 2020 18:05:25 -  1.168
+++ mandoc.119 Jul 2020 15:18:31 -
@@ -650,7 +650,7 @@ It never affects the interpretation of i
 Any non-empty value of the environment variable
 .Ev MANPAGER
 is used instead of the standard pagination program,
-.Xr more 1 ;
+.Xr less 1 ;
 see
 .Xr man 1
 for details.
@@ -664,7 +664,7 @@ Specifies the pagination program to use 
 .Ev MANPAGER
 is not defined.
 If neither PAGER nor MANPAGER is defined,
-.Xr more 1
+.Xr less 1
 .Fl s
 is used.
 Only used if



Re: LC_MESSAGES in xargs(1)

2020-07-19 Thread Ingo Schwarze
Hi Martijn,

Martijn van Duren wrote on Thu, Jul 16, 2020 at 11:13:31PM +0200:

> I gave a quick look at replacing prompt with readpassphrase(3), but that
> would be more trouble than it's worth. (adjusting pledge, semantics
> change in where the "?..." would be printed).
> 
> Minor nits inline, but either way OK martijn@

Thanks to all three of you for checking, i just committed the patch.

> On Thu, 2020-07-16 at 21:49 +0200, Ingo Schwarze wrote:
[...]
>>  * There is no need to put a marker "return value ignored on
>>purpose" where ignoring the return value almost never indicates
>>a bug, like for fprintf, fflush, fclose.

> If you're going to change this, please also remove them from run()

Done.

>> -(void)fflush(stderr);
>> +fflush(stderr);

> According to setvbuf(3):
> The standard error stream stderr is initially unbuffered.
> And since setvbuf is not called in xargs this seems superfluous to me.
> If this is correct, there's a second fflush that can also be removed.

I didn't mix that into this diff because i didn't want to have two
unrelated, potentially non-trivial changes in the same commit.

But if you think this is worth cleaning up, by all means, send a
diff for it.  I'm not very experienced with stdio buffering, so i'd
hope you might get an OK from another developer.

Yours,
  Ingo



Re: LC_MESSAGES in xargs(1)

2020-07-16 Thread Ingo Schwarze
Hi Jan,

Jan Stary wrote on Thu, Jul 16, 2020 at 09:45:44AM +0200:

> Does xargs need to set LC_MESSAGES?

As it stands, your patch doesn't make much sense.  It is true that
it doesn't change behaviour, but it leaves more cruft behund than
it tidies up.

That said, i agree that we will never implement LC_MESSAGES.

That allows a nice cleanup, simplifying the code and getting rid
of several headers and several calls to complicated functions.

Remarks:
 * .Tn was deprecated years ago.
 * On OpenBSD, as the manual page says, nl_langinfo(3) is really
   only useful for CODESET.
 * There is no need to put a marker "return value ignored on
   purpose" where ignoring the return value almost never indicates
   a bug, like for fprintf, fflush, fclose.
 * Usually, we prefer getline(3) over fgetln(3), but none of the
   arguments making it better really applies here, so we can as
   well leave it.  The only downside is that ttyfp needs to remain
   open until the stdio input buffer has been inspected, so we need
   another automatic variable, "doit", but it's still simpler than
   the malloc(3) dance that getline(3) would require.

OK?
  Ingo


Index: xargs.1
===
RCS file: /cvs/src/usr.bin/xargs/xargs.1,v
retrieving revision 1.28
diff -u -p -r1.28 xargs.1
--- xargs.1 4 Jun 2014 06:48:33 -   1.28
+++ xargs.1 16 Jul 2020 19:29:52 -
@@ -213,11 +213,11 @@ at once.
 .It Fl p
 Echo each command to be executed and ask the user whether it should be
 executed.
-An affirmative response,
+If the answer starts with
 .Ql y
-in the POSIX locale,
-causes the command to be executed, any other response causes it to be
-skipped.
+or
+.Ql Y ,
+the command is executed; otherwise it is skipped.
 No commands are executed if the process is not attached to a terminal.
 .It Fl R Ar replacements
 Specify the maximum number of arguments that
@@ -330,8 +330,7 @@ The flags
 are extensions to
 .St -p1003.1-2008 .
 .Pp
-The meanings of the 123, 124, and 125 exit values were taken from
-.Tn GNU
+The meanings of the 123, 124, and 125 exit values were taken from GNU
 .Nm xargs .
 .Sh HISTORY
 The
Index: xargs.c
===
RCS file: /cvs/src/usr.bin/xargs/xargs.c,v
retrieving revision 1.34
diff -u -p -r1.34 xargs.c
--- xargs.c 12 Jun 2018 15:24:31 -  1.34
+++ xargs.c 16 Jul 2020 19:29:52 -
@@ -41,10 +41,7 @@
 #include 
 #include 
 #include 
-#include 
-#include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -86,8 +83,6 @@ main(int argc, char *argv[])
eofstr = "";
Jflag = nflag = 0;
 
-   (void)setlocale(LC_MESSAGES, "");
-
/*
 * POSIX.2 limits the exec line length to ARG_MAX - 2K.  Running that
 * caused some E2BIG errors, so it was changed to ARG_MAX - 4K.  Given
@@ -607,26 +602,19 @@ waitchildren(const char *name, int waita
 static int
 prompt(void)
 {
-   regex_t cre;
size_t rsize;
-   int match;
char *response;
FILE *ttyfp;
+   int doit = 0;
 
if ((ttyfp = fopen(_PATH_TTY, "r")) == NULL)
return (2); /* Indicate that the TTY failed to open. */
-   (void)fprintf(stderr, "?...");
-   (void)fflush(stderr);
-   if ((response = fgetln(ttyfp, )) == NULL ||
-   regcomp(, nl_langinfo(YESEXPR), REG_BASIC) != 0) {
-   (void)fclose(ttyfp);
-   return (0);
-   }
-   response[rsize - 1] = '\0';
-   match = regexec(, response, 0, NULL, 0);
-   (void)fclose(ttyfp);
-   regfree();
-   return (match == 0);
+   fprintf(stderr, "?...");
+   fflush(stderr);
+   response = fgetln(ttyfp, );
+   doit = response != NULL && (*response == 'y' || *response == 'Y');
+   fclose(ttyfp);
+   return (doit);
 }
 
 static void



Re: disable libc sys wrappers?

2020-07-10 Thread Ingo Schwarze
Hi Theo,

Theo de Raadt wrote on Fri, Jul 10, 2020 at 10:02:46AM -0600:

> I also don't see the point of the leading _
> 
> Where does that come from?
> 
> This isn't a function namespace.  What does it signify, considering
> no other environment variable uses _ prefixing.

   $ man -kO Ev Ev~^_ | sed 's/ [^_][^ ]*//g'  # chicken scratches!
  at, _
  ksh, _
  port-modules(5) _MODFOO_*
  sudoers(5) _RLD*

So, not absolutely accurate, but very close to the truth.  The
bsd.port.mk(5) implementation, which does make extensive use of
variable names with leading underscores in the sense intended by
Ted (private variable, keep your fingers away!), is probably quite
a special beast.

> I think you are mixing too much stuff in here...

Yours,
  Ingo



Re: disable libc sys wrappers?

2020-07-09 Thread Ingo Schwarze
Hi Mark,

Mark Kettenis wrote on Thu, Jul 09, 2020 at 07:59:25PM +0200:
> Ingo Schwrze wrote:

>> Now that i look at that, and at what you said previously, is it even
>> plausible that some user ever wants "-t c" ktracing but does
>> specifically *not* want to see clock_gettime(2) and friends?
[...]
> Yes.  If I ktrace something like chromium I really don't want to see
> the thousands of pointless clock_gettime(2) calls.

Fair enough.  Together with what deraadt@ explained,
it probably is -T, then.

>> Regarding the LD_ prefix, clock_gettime(2) and gettimeofday(2)
>> are in  and , respectively, but maybe a TIME_
>> prefix would be too specific.  Sure, it is the "time" sublibrary
>> of libc, but "TIME_" wouldn't really make it apparent that it
>> is related to libc at all.
>> 
>> So, should it instead be something like:
>> 
>>   LIBC_KTRACE_GETTIME   ?

> I wonder if it should start with an underscore instead to indicate
> that the variable is not for public consumption.  _KTRACE_TIME would
> do the job for me.  We can always rename it if we discover another use
> case apart from ktrace.

Sounds like user interface is almost finished; _KTRACE_TIME sounds
nicely concise, yet still properly specific.

Yours,
  Ingo



Re: disable libc sys wrappers?

2020-07-09 Thread Ingo Schwarze
Hi Theo,

Theo de Raadt wrote on Thu, Jul 09, 2020 at 11:08:38AM -0600:
> Ingo Schwarze  wrote:

>> So, what about
>>   LD_KTRACE_GETTIME
>> or a similar environment variable name?

> That naming seems logical.
> 
> If it is mostly hidden behind a ktrace flag (-T ?) then I think
> we're in good shape.

Technically, the implementation of the new ktrace(1) flag will be
totally different from the implementation of the existing ktrace -t
flags.  But from the user perspective, the purpose of the new flag
is just to "put some more information into the dump file", so
shouldn't it be just another -t letter?  Like,

c   trace system calls
T   trace clock_gettime(2) and gettimeofday(2)

or similar?

Now that i look at that, and at what you said previously, is it even
plausible that some user ever wants "-t c" ktracing but does
specifically *not* want to see clock_gettime(2) and friends?

If "i want system call ktracing" logically more or less implies "i
also want to see clock_gettime and friends", then maybe we do not
need a new command line flag at all, not even a sub-flag, and can
instead just let "-t c" enable that, too?

Keeping the user interface small is often good.  Needing only one
sub-flag rather than two to see all system calls doesn't seem that
bad, either.

Unless i'm mistaken, we don't provide a way either to say "i want
to see system calls, except that i don't want to see chdir(2),
chmod(2), getuid(2), and mprotect(2)."


Regarding the LD_ prefix, clock_gettime(2) and gettimeofday(2)
are in  and , respectively, but maybe a TIME_
prefix would be too specific.  Sure, it is the "time" sublibrary
of libc, but "TIME_" wouldn't really make it apparent that it
is related to libc at all.

So, should it instead be something like:

  LIBC_KTRACE_GETTIME   ?

Yours,
  Ingo



Re: disable libc sys wrappers?

2020-07-09 Thread Ingo Schwarze
Hi Theo,

Theo de Raadt wrote on Thu, Jul 09, 2020 at 09:47:26AM -0600:

> This time, they become invisible.
[...]
> There are many circumstances where ltrace is not suitable.
[...]
> We fiddle with programs all the time, to inspect them.

Fair enough, then.

The following variables already exist according to man -ak Ev~'\

Re: disable libc sys wrappers?

2020-07-09 Thread Ingo Schwarze
Hi,

i wonder whether there is a layering violation going on here.

>From the user perspective, whether an API call is a syscall or a
mere library function doesn't make any difference, it's an
implementation detail.  API calls have often moved from being library
functions to syscall and vice versa, witnessed by the blurry line
between sections 2 and 3 in the manual (which is not a problem at
all).

So you say the user wants to see whether their program calls
some time-related API functions, and in the past, ktrace was useful
for that.  On a machine where syscalls for the purpose are no longer
needed due to pirofti@'s optimization, does it really make sense to
force useless syscalls just to be able to see the API calls in ktrace?
Wouldn't it make more sense to instead simply use ltrace(1), which IIUC
is intended for just that purpose, tracing API calls?

Someone said setting up ltrace(1) is kind of harder, while ktrace(1)
is very easy to use (i can confirm the latter, i use ktrace regularly).
So maybe the real problem is to make sure ltrace(1) is about as easy
to use as ktrace(1)?  (Not sure what exactly the problem(s) with ltrace
usability are, if any, i didn't use it much so far.)

It feels odd that a user would fiddle with the internal way how API
calls are implemented, in particular using something as scary as
environment variables, just to *inspect* what their program is doing...
And that they would actually change what their program is doing just
for the purpose of that inspection...

Obviously, i don't object to any of this, and i may be totally wrong
trying to think about something so deep down in the system.  I'm just
wondering...

Yours,
  Ingo



rewrite printf(3) manual page

2020-07-08 Thread Ingo Schwarze
Hi,

the following is the execution of an idea brought up by deraadt@.

There are four problems with the printf(3) manual page:

 1. While some modifiers (like "argno$" and "width") do the same
for all conversion specifiers, the effects of some other
modifiers varies wildly.  For example, "precision" does rather
different things for %d, %s, %e, %f, and %g.  Some modifiers
do not apply to all specifiers in the first place.  Yet, all
modifiers are explained in one place, as if each one did only
one consistent job.  This leads to a jungle of confusing
forward references.

 2. All conversion specifiers have their own syntax, but it is
not clearly presented for any of them.

 3. It isn't properly explained what %lc and %ls do, and in particular
the fact that their operation is locale-dependent isn't made
explicit.

 4. Part of the RETURN VALUES section is outright wrong.  These
functions do not return numbers of characters printed but numbers
of *bytes* printed.

The following patch attempts to solve these issues by rewriting
most of the page in a substantially different style.  deraadt@ and
millert@ have already said that they like the general direction and
jmc@ is also fine with it.  But given that this API probably is
among the most widely used ones and that my changes are quite large
and radical, i consider it fair to show it to a wider audience
before commit.  Both general feedback and scrutiny of the details
are now welcome, to make sure that i didn't introduce any errors.

During my own checks of the accuracy of these descriptions, i already
committed a collection of new unit tests to
/usr/src/regress/lib/libc/printf/.

Note that our current version of this manual page is very close to
what is in the POSIX standard, and this patch moves the wording and
structure quite far away from it (but not the content, of course).
I think that is OK.  The audience of a standard is language lawyers,
it is optimized to support making money from disputes whether some
implementation is right or wrong.  It is not necessarily optimized
to be easy to understand, nor to avoid users getting confused and
consequently misusing the API.  For that reason, most of our manual
pages are worded quite differently than the standard.

Also note that there are a few other pages with similar content,
for example wprintf(3) and printf(1).  But this patch is large
enough as it stands, so i'm not going to change other pages in the
same commit.  Once this is in and the dust has settled, it becomes
much easier to decide what to do with the others.

Finally, note that the first few paragraphs probably also allow
improvements.  But this patch deliberately focusses on one topic,
the description of the syntax and semantics of conversion specifications.
Other polishing can be done another day.

OK?
  Ingo


Index: printf.3
===
RCS file: /cvs/src/lib/libc/stdio/printf.3,v
retrieving revision 1.85
diff -u -r1.85 printf.3
--- printf.36 Jul 2020 17:24:59 -   1.85
+++ printf.38 Jul 2020 18:49:15 -
@@ -159,487 +159,580 @@
 which are copied unchanged to the output stream,
 and conversion specifications, each of which results
 in fetching zero or more subsequent arguments.
-Each conversion specification is introduced by the character
-.Cm % .
 The arguments must correspond properly (after type promotion)
-with the conversion specifier.
-After the
-.Cm % ,
-the following appear in sequence:
-.Bl -bullet
-.It
-An optional field, consisting of a decimal digit string followed by a
-.Cm $
-specifying the next argument to access.
-If this field is not provided, the argument following the last
-argument accessed will be used.
-Arguments are numbered starting at
-.Cm 1 .
-.It
-Zero or more of the following flags:
-.Bl -hyphen
-.It
-A hash
-.Sq Cm #
-character
-specifying that the value should be converted to an
-.Dq alternate form .
-For
-.Cm o
-conversions, the precision of the number is increased to force the first
-character of the output string to a zero (except if a zero value is printed
-with an explicit precision of zero).
-For
-.Cm x
-and
-.Cm X
-conversions, a non-zero result has the string
-.Ql 0x
-(or
-.Ql 0X
-for
-.Cm X
-conversions) prepended to it.
-For
-.Cm a ,
-.Cm A ,
-.Cm e ,
-.Cm E ,
-.Cm f ,
-.Cm F ,
-.Cm g ,
-and
-.Cm G
-conversions, the result will always contain a decimal point, even if no
-digits follow it (normally, a decimal point appears in the results of
-those conversions only if a digit follows).
-For
-.Cm g
-and
-.Cm G
-conversions, trailing zeros are not removed from the result as they
-would otherwise be.
-For all other formats, behaviour is undefined.
-.It
-A zero
-.Sq Cm \&0
-character specifying zero padding.
-For all conversions except
-.Cm n ,
-the converted value is padded on the left with zeros rather than blanks.
-If a precision is given with a numeric conversion
-.Pf ( Cm d ,
-.Cm i ,
-.Cm o 

Re: snmp(1) initial UTF-8 support

2020-07-03 Thread Ingo Schwarze
Hi Martijn,

sorry for the delay, now i finally looked at the function
smi_displayhint_os() from the line "if (MB_CUR_MAX > 1) {" to the
end of the corresponding else clause.  IIUC, what that code is
trying to do is iterate the input buffer "const char *buf" of length
"size_t buflen".

Before starting, i will consider the sizing of rbuf.  It is octetlength
plus strlen("STRING: ") plus 1 byte for the terminating NUL.  Then
optionally, "STRING: " is copied into it, so octetlength plus one
byte for the terminating NUL remains.

However, if the "STRING: " is copied in, j is intialized to
strlen("STRING: ") and then later compared to octetlength which
remains unchanged.  That feels odd.  I would expect that if j starts
at strlen("STRING: "), octetlength should be increased accordingly,
or alternatively, if octetlength remains as it is, j should always
start at 0.  Otherwise, the last eight bytes allocated always seem
to remain unused.  Is the octetlength input parameter supposed to
include strlen("STRING: ") or not?  [1]

What the else clause does (i.e. when the user selected the C locale)
seems to be this:

 * j is unconditionally re-initialized to 0, so if "STRING: "
   was written earlier, that gets overwritten.  Is that
   intentional?  [2]
 * UTF-8 starting bytes followed by another UTF-8 starting byte
   are totally ignored.  Is that intentional?  If feels odd.  [3]
 * UTF-8 starting bytes followed by an ascii byte
   are represented as ".".  That seems intentional.
 * UTF-8 starting bytes followed by at least one UTF-8 cont byte
   are represented by "?".  Up to FOUR cont bytes are totally
   ignored, which seems quite odd because an UTF-8 sequence
   can have at most THREE cont bytes (four bytes total including
   the starting byte).  [4]  From the FIFTH following cont byte
   onwards, "." is written for each cont byte.
   Note that this is *not* trying to check whether the UTF-8 sequence
   is valid (which wouldn't be trivial at all), but probably that
   isn't required.  It merely tries to check whether the sequence
   is longer than the absolute maximum (and seems to allow one byte
   too much unless i misread the code).
 * A sequence "start + cont + ASCII + cont" prints "?a",
   then totally ignores the second cont byte as if it were
   a continuation of the initial start byte, despite the
   intervening ASCII byte.  [5]
   The state machine doesn't appear to be fully thought out.
   Did you prepare a list for yourself containing all the
   possible states and all the edges of the machine?
 * An initial cont byte, or one after start + ASCII, or one after
   start + at least 4 cont + ASCII is represented by ".";
   that seems intentional.
 * In the for loop in the else clause, the "i < octetlength" feels
   confusing to me.  It cannot cause an overflow because in the
   else clause, we always have j < i (caveat, this might only be
   due to the possibly unintentional re-initializing of j to 0).
   But assume an input buf containing lots of UTF-8 characters.  In
   that case, you abort processing long before the output rbuf is
   full.  Is that intentional?  Either way, wouldn't "j < octetlength"
   be both clearer and safer?  [6]
 * The input "UTF-8 start + ASCII, end of input" is represented
   as "x." rather than ".x".  [7]

So the idea probably is to print

 * "." for clearly invalid bytes including non-printable ASCII bytes
 * "?" for UTF-8 start bytes including following cont bytes that
   _might_ form a valid UTF-8 sequence, accepting invalid sequences
   that would be non-trivial to identify.

But it doesn't quite do that.

For the "if (MB_CUR_MAX > 1)" clause (i.e., when the user selected
a UTF-8 lcale), it seems to do this:

 * Embedded NUL bytes terminate processing, returning what was
   decoded so far.  Note that differs from what the ASCII clause
   does, which represents NUL as "." and continues processing.
   I don't think it can be intentional that processing stops
   at different places depending on the user's locale.  [8]
 * The "case -1" clause obviously lacks a call
   (void)mbtowc(NULL, NULL, MB_CUR_MAX);
   see the mbtowc(3) manual page,
   /usr/src/usr.bin/fmt/fmt.c for simple examples, or
   /usr/src/usr.bin/ssh/utf8.c for a full-blown example.  [9]
 * In contrast to the ASCII clause, the UTF-8 clause does detect
   all invalid UTF-8 sequences and writes a replacement character
   for each invalid byte (rather than a replacement character
   for groups of invalid bytes as the ASCII clause does).
   I think this difference in behaviour makes sense because
   the UTF-8 clause can easily detect such conditions while
   the ASCII clause should better not try.
 * For non-printable UTF-8 characters, it prints ".".
   This feels inconsistent.  We have:

   v-- input ASCII   UTF-8  <-  locale
   invalid byte  "." REPLACEMENT
   non-printable char"." "."
   UTF-8 printable char  "?" itself
   ASCII printable char  itself  itself

   So judging from 

Re: Add man parameter for HTML pager

2020-06-15 Thread Ingo Schwarze
Hi Abel,

i committed a tweaked version of your patch with these changes:

 - pass pointers to structs around, don't pass structs by value
 - no need for an additional automatic variable for the URI
 - no need for comments similar to:  i++; /* increment i */
 - only use file:// when -O tag was indeed given

I dropped changing the filename.  I consider that a bug in lynx(1).
Changing behaviour based on the filename "extension" is Microsoft
Windows style and a concept totally alien to Unix.  So, i don't
really like the idea to cater to that.  Besides, it's trivial to
work around the lynx bug with -force_html without putting weird,
pointless complication into mandoc.

Your patch was mangled in at least one way.  Most lines were prefixed
with a bogus space character; i did not check whether it was also
mangled in other ways.  In this case the mangling didn't hurt because
i had to apply it by hand anyway to check it and to perform the
above tweaks.  But when you send patches in the future, please try
to make sure they apply cleanly.  Mangled patches can sometimes
cause needless annoyance, and they often delay merging by OpenBSD
developers.

I have also documented the new feature in the "HTML Output" subsection
of the mandoc(1) manual page.

Thanks for the suggestion and for the patch!
I is very rare that users who are not BSD or Linux operating system
developers send such substantial mandoc patches.
The latest instance i found in my notes came from Franco Fichtner in 2013.

Yours,
  Ingo



Re: Add man parameter for HTML pager

2020-06-14 Thread Ingo Schwarze
Hi Abel,

Romero Perez, Abel wrote on Mon, Jun 15, 2020 at 03:06:26AM +0200:
> Romero Perez, Abel wrote: 

>> I tried to view the manuals in HTML format with lynx, but I couldn't

I assume what you mean is that you could, but the "-O tag" option had
no effect.

>> because lynx and links can't accept the /tmp/man.XX... file as
>> an URL...

In "-T html" output mode, the tag file isn't even written in the first
place.  Only the terminal formatter writes a tag file.

>> then I decided to add a new parameter (-U)

Absolutely not.  Adding new options to man(1) is completely out of
the question.  Option space is almost full and very messy already:

  https://mandoc.bsd.lv/man/man.options.1.html

It hardly matters that -U is already taken:

  https://mandoc.bsd.lv/man/man.options.1.html#U

You certainly can't take an option that is still free for this
purpose, either.

>> to pass the pager a formal file URL scheme as follows:
>> file:///tmp/man.XXX#tag
>> Then man spawns the pager (lynx for my case):
>> lynx -force_html file:///tmp/man.XXX#tag

That's an interesting idea.

It needs to be done by default when -T html -O tag is given, just
like tagging is enabled by default when the pager is less(1).

>> The testing command is:
>> PAGER="" MANPAGER="lynx -force_html" ./man -U -T html -O tag=k pfctl

The PAGER="" variable has no effect when MANPAGER is set,
see the man(1) manual.

So the goal is to make this work:

  MANPAGER="lynx -force_html" ./man -T html -O tag=k pfctl

> Sorry, I didn't reviewed the code well. The first diff has some bugs.
> As follows the stable diff (please, review):

I didn't review the diff closely, yet.

Obviously, using strncpy(3) or strncat(3) is not permitted,
but those should be easy to replace, perhaps with mandoc_asprintf(),
not sure yet.

It looks like you will have to pass outst into run_pager() and
spawn_pager() rather than outst.tag_files, such that you can check
for outst.outtype == OUTT_HTML inside spawn_pager().

In any case, thanks for the nice idea!

Yours,
  Ingo



Re: snmp(1) initial UTF-8 support

2020-05-31 Thread Ingo Schwarze
Hi Martijn,

Martijn van Duren wrote on Tue, May 19, 2020 at 10:25:37PM +0200:

> So according to RFC2579 an octetstring can contain UTF-8 characters if  
> so described in the DISPLAY-HINT. One of the main consumers of this is
> SnmpAdminString, which is used quite a lot.
> 
> Now that we trimmed a little fat from snmp's oid, I wanted to fill it
> up again and implemented the bare minimum DISPLAY-HINT parsing that is
> required for SnmpAdminString. Other parts of the syntax can be atter
> at a later state if desirable.
> 
> Now I decided to implement this a little different from net-snmp,
> because they throw incomplete sequences directly to the terminal,
> which I recon is not a sane approach.

No, definitely not.  Never throw invalid or incomplete UTF-8 at the
terminal unless the user unambiguously requests just that (like
with `printf "a\x80z\n"` or `cat /usr/bin/cc` or something like that).

> I choose to take the approach taken by the likes of wall(1) and replace
> invalid and incomplete sequences with a '?'.

If the specification says that the user is only allowed to put valid
UTF-8 into octetstrings (and not arbitrary bytes), replacing invalid
bytes at output time is both acceptable and feels like the only
sensible option, unless something like vis(3) is considered more
appropriate.

However, the reason why wall(1) uses '?' as the replacement character
is because wall(1) must not assume that the output device can handle
UTF-8 and has to work safely if all the output device can handle is
ASCII.  So, wall(1) has to live with the inconvenience that when you
see a question mark, you don't know whether it really is a question
mark or whether it originally used to be garbage input.

If i understand correctly, here we are talking about output that is
UTF-8 anyway.  So instead of (ab)using the question mark, you might
consider using the U+FFFD REPLACEMENT CHARACTER.  The intended
purpose of that one is to stand in for invalid and inconplete
byte sequences, and also for characters that cannot be displayed.

> This because DISPLAY-HINT already states that too long sequences
> strings should be cut short at the final multibyte character fitting
> inside the specified length, meaning that output can already get
> mangled.
> 
> This also brings me to the question how we should handle DISPLAY-HINT
> in the case of -Oa and -Ox. Net-snmp prioritizes DISPLAY-HINT over
> these, but it has the option to disable this by disabling MIB-loading
> via -m''; This implementation doesn't give us the that option (yet?).
> The current diff follows net-snmp, but there is something to say to
> prioritize -Ox over DISPLAY-HINT, so an admin can use it to debug
> snmp-output without it being mangled. Any feedback here is welcome.
> 
> Once it's clear if this is the right approach I'll do a thorough search
> through mib.h on which objects actually can use this new definition.

I can't really comment on what snmp(1) should do, or which settings
should override which other settings.

Do i understand correctly that you want to make -Oa print invalid
UTF-8 to the terminal?  If so, that would sound reasonable to me,
for the following reason.  Printing non-printable ASCII to the
terminal is often dangerous, it can screw up or reconfigure the
terminal, so -Oa is already an option that must be used with care,
just like `cat /usr/bin/cc`.  While printing invalid UTF-8 is not
a good idea in general, it is less dangerous than printing non-printable
ASCII, so just passing the bytes through for -Oa doesn't make
anything more dangerous than it already is.

Maybe -Oa should have a warning in the manual page about the dangers
of using it on untrusted data?  Not sure.

Oh, by the way, when -Oa is not specified and there is UTF-8
content, don't forget to still filter out non-printable bytes:
both non-printable ASCII bytes and non-printable UTF-8 characters.
Right?

> Index: smi.c
> ===
> RCS file: /cvs/src/usr.bin/snmp/smi.c,v
[...]
> @@ -598,6 +638,60 @@ smi_foreach(struct oid *oid)
>  }
>  
>  int
> +isu8cont(unsigned char c)
> +{
> + return (c & (0x80 | 0x40)) == 0x80;
> +}
> +
> +char *
> +smi_displayhint_os(struct textconv *tc, const char *buf, size_t buflen)
> +{
> + size_t octetlength, i, j, start;
> + char *displayformat;
> + char *rbuf;
> + mbstate_t mbs;
> +
> + errno = 0;
> + octetlength = (size_t) strtol(tc->tc_display_hint, , 10);
> + if (octetlength == 0 && errno != 0) {
> + errno = EINVAL;
> + return NULL;
> + }
> + if (displayformat[0] == 't') {
> + rbuf = malloc(octetlength + 1);

NULL pointer access ahead on ENOMEM.

> + if (strcmp(nl_langinfo(CODESET), "UTF-8") == 0) {

On OpenBSD, that is not the normal way of checking whether
the locale is UTF-8.  Also, it is not portable because the CODESET
values are not standardized.

We usually do

#include 

if (MB_CUR_MAX > 

Re: locale thread support

2020-05-29 Thread Ingo Schwarze
Hi Marc,

do i understand correctly that you intend to say:  there are
legitimate reasons to use these xlocale.h functions in portable
software, in particular in portable libraries, so we should probably
have them all, or at least most of them?

I see your point that using functions like isdigit(3) can (at least
in theory) be dangerous when running on systems that support
exceedingly weird single-byte 8-bit locales.  (As opposed to only
supporting one single single-byte locale, which is 7-bit, like we do.)

I'm not even sure the question matters whether xlocale.h is the best
possible way to avoid such issues.  To make support desirable, it
might be sufficient that xlocale.h is one reasonable way and that
some amounts of useful software actually use it.

Yours,
  Ingo


Marc Espie wrote on Thu, May 28, 2020 at 05:52:12PM +0200:
> On Tue, May 26, 2020 at 12:13:02AM +0200, Ingo Schwarze wrote:

> > my impression is there are two totally independent topics in this
> > thread.
> > 
> >  1. Whether and how to make things safer, ideally terminating the
> > program in a controlled manner, if it uses functions that are
> > not thread-safe in an invalid manner.  I'm not addressing that
> > topic in this mail.
> > 
> >  2. Whether we want additional non-standard xlocale.h functions
> > in our libc.
> > 
> > Regarding topic 2, let me say up front that i do not feel strongly
> > either way.  It seems to me this is mostly a question for porters.
> > If some functions are widely used - even if non-standard - and help
> > avoid porting hassle, then sure, let's have them unless they cause
> > excessive fuss.  But the latter does not seem likely here.
> > 
> > There are a number of functions that seem likely useful to me off
> > the top of my head, for example: querylocale, nl_langinfo_l,
> > MB_CUR_MAX_L, wcwidth_l
> > 
> > In general, i must say i like the whole xlocale.h business not all
> > that much: in a nutshell, it is doubling the number of half the
> > functions under the sun, which usually indicates poor design in the
> > first place, and then the vast majority of these additional functions
> > are almost useless on any operating system and totally useless on
> > OpenBSD.  Like, i mean, btowc_l(3)?  Or even the isdigit_l(3) which
> > we already have?  You must be choking, Mr. Chisnall!  I don't think
> > stuff should be added because it is out there, but only if there is
> > at least some porting benefit.
> > 
> > Regarding the FreeBSD headers, i like them even less.  There are both
> > terrible design choices and terrible implementation choices.  Regarding
> > design, it appears that you *must* #include  after other
> > headers - if you include it before, it won't work.  Regarding
> > implementation, there is quite repulsive macro abuse in xlocale/_ctype.h;
> > but probably that can be unrolled in LibreSSL style.  Either way, none
> > of that hinders providing them if doing so yields benefit.
> > 
> > See below for a list of functions that i drafted extremely quickly,
> > so beware of errors and omissions.  The point isn't to present a
> > definite plan.  The point is merely to demonstrate the sheer fatness
> > of the animal and to give a first impression of the degree to which
> > it stinks and grunts from most of its ends.
> > 
> > If any of this is needed, do porters already know which functions
> > are in particularly high demand?  Do you have any idea how to find
> > out which ones are actually useful for porting purposes?
> 
> Serendipity
> 
> Today, I looked at an older library  for audit purposes.
> 
> Library is called udns.
> 
> If you look at the code, there's a lot of apparently useless reinvention,
> like the code explicitly checks for digits using c >= '0' && c <= '9' patterns
> (or isprint for that matter).
> 
> Thinking some more, I realized why: this is library code, so it can't assume
> it's running under any kind of sensible locale unless it has asserted it 
> itself.
> 
> In that specific context, isdigit_l and friends make a lot of sense.
> Either that, or systematic uselocale wrappers for any function that can be
> called from outside.
> 
> (yes, I now that isdigit/isspace are not a problem on OpenBSD, but they can
> be elsewhere!)



Re: Enable building wsmoused on arm64 and armv7

2020-05-29 Thread Ingo Schwarze
Hi,

Frederic Cambus wrote on Thu, May 28, 2020 at 10:44:59PM +0200:
> On Thu, May 28, 2020 at 10:52:44AM -0600, Theo de Raadt wrote:

>> -MANSUBDIR= i386 amd64 alpha
>> +MANSUBDIR= i386 amd64 arm64 armv7 alpha
>> 
>> Actually, I suggest making this a MI man page.  Delete that line,
>> and see where the files land.  I'll adjust sets.

Good idea.

> Yes, makes sense. Just commited the change, MANSUBDIR is now gone.
> The man page ends up in /usr/share/man/man8 as expected.

Thanks for doing that.


In the medium to long term, i hope to improve support for pages
that apply to several, but not all architectures.  I have some ideas
how to make that much simpler to use and more consistent, but haven't
fully made up my mind yet.  I isn't urgent, so there is no hurry,
and i hope to get it right the first time rather than experimenting
back and forth.

In the meantime, there is indeed nothing wrong with making pages
that apply to many arches simply MI.


In case you are interested in some details:
Stuff like this isn't ideal:

   $ cd /usr/share/man/   
   $ ls -al man8/*/wsmoused.8
  -r--r--r--  1 root  bin  5958 May 27 16:38 man8/alpha/wsmoused.8
  -r--r--r--  1 root  bin  5958 May 27 16:38 man8/amd64/wsmoused.8
  -r--r--r--  1 root  bin  5958 May 27 16:38 man8/i386/wsmoused.8

So there is was one copy of the file page per arch, it wasn't even
hardlinked.  Of course, the waste of space hardly matters, but since
we got rid years ago of identifying additional *names* of pages by
putting those names names into the file system, identifying *arches*
by file names now feels archaic at best.

It also has practical downsides, for example:

   $ man -k wsmoused
  wsmoused(8/alpha) - wsmouse daemon
  wsmoused(8/amd64) - wsmouse daemon
  wsmoused(8/i386) - wsmouse daemon

So, apropos(1) made me believe there might be three different
versions, while they are actually all the same page.  Compare to
this where different pages actually do exist:

   $ ls -al $(man -fw cpu)
  -r--r--r--  1 root  bin  3115 May 27 16:38 /usr/share/man/man4/amd64/cpu.4
  -r--r--r--  1 root  bin  8411 May 27 16:38 /usr/share/man/man4/hppa/cpu.4
  -r--r--r--  1 root  bin  3881 May 27 16:38 /usr/share/man/man4/i386/cpu.4

Consequently, "man -af wsmoused" printed the same page three times,
which felt ugly and confusing.  What i hope to ultimately teach it
is to show the page only once *and*, at the same time, to clearly
indicate which arches it applies to, without needing requiring any
complicated syntax or code.

Yours,
  Ingo



Re: locale thread support

2020-05-25 Thread Ingo Schwarze
Hi,

my impression is there are two totally independent topics in this
thread.

 1. Whether and how to make things safer, ideally terminating the
program in a controlled manner, if it uses functions that are
not thread-safe in an invalid manner.  I'm not addressing that
topic in this mail.

 2. Whether we want additional non-standard xlocale.h functions
in our libc.

Regarding topic 2, let me say up front that i do not feel strongly
either way.  It seems to me this is mostly a question for porters.
If some functions are widely used - even if non-standard - and help
avoid porting hassle, then sure, let's have them unless they cause
excessive fuss.  But the latter does not seem likely here.

There are a number of functions that seem likely useful to me off
the top of my head, for example: querylocale, nl_langinfo_l,
MB_CUR_MAX_L, wcwidth_l

In general, i must say i like the whole xlocale.h business not all
that much: in a nutshell, it is doubling the number of half the
functions under the sun, which usually indicates poor design in the
first place, and then the vast majority of these additional functions
are almost useless on any operating system and totally useless on
OpenBSD.  Like, i mean, btowc_l(3)?  Or even the isdigit_l(3) which
we already have?  You must be choking, Mr. Chisnall!  I don't think
stuff should be added because it is out there, but only if there is
at least some porting benefit.

Regarding the FreeBSD headers, i like them even less.  There are both
terrible design choices and terrible implementation choices.  Regarding
design, it appears that you *must* #include  after other
headers - if you include it before, it won't work.  Regarding
implementation, there is quite repulsive macro abuse in xlocale/_ctype.h;
but probably that can be unrolled in LibreSSL style.  Either way, none
of that hinders providing them if doing so yields benefit.

See below for a list of functions that i drafted extremely quickly,
so beware of errors and omissions.  The point isn't to present a
definite plan.  The point is merely to demonstrate the sheer fatness
of the animal and to give a first impression of the degree to which
it stinks and grunts from most of its ends.

If any of this is needed, do porters already know which functions
are in particularly high demand?  Do you have any idea how to find
out which ones are actually useful for porting purposes?

Yours,
  Ingo


Functions we already have marked are with a '*'.


  useful: querylocale


  useful: nl_langinfo_l*


  useful: MB_CUR_MAX_L
  marginally useful: mbtowc_l mbstowcs_l wctomb_l wcstombs_l
  useless: atof_l atoi_l atol_l atoll_l
   strtod_l strtof_l strtol_l strtold_l strtoll_l strtoul_l strtoull_l
   mblen_l


  useful: wcwidth_l wcswidth_l
  marginally useful: fgetwc_l fgetws_l fputwc_l fputws_l getwc_l getwchar_l
 putwc_l putwchar_l ungetwc_l
 fwprintf_l fwscanf_l swprintf_l swscanf_l
 vfwprintf_l vswprintf_l vwprintf_l wprintf_l wscanf_l
 vfwscanf_l vswscanf_l vwscanf_l
 mbrlen_l mbrtowc_l mbsinit_l mbsrtowcs_l wcrtomb_l
 wcsrtombs_l wctob_l mbsnrtowcs_l wcsnrtombs_l
  useless: btowc_l wcsftime_l wcstod_l wcstol_l wcstoul_l
   wcstof_l wcstold_l wcstoll_l wcstoull_l


  useless: strcasestr_l strcoll_l* strxfrm_l*


  useless: strcasecmp_l* strncasecmp_l*


  useless: strtoimax_l strtoumax_l wcstoimax_l wcstoumax_l


  useless: strfmon_l


  useless: strftime_l* strptime_l


  marginally useful: towlower_l* towupper_l* iswctype_l* towctrans_l*
  not sure: nextwctype_l wctype_l wctrans_l* digittoint_l
  useless: isalnum_l* etc.  tolower_l* toupper_l*

include 
  useless: asprintf_l dprintf_l fprintf_l fscanf_l printf_l
   scanf_l snprintf_l sprintf_l sscanf_l
   vfprintf_l vprintf_l vsprintf_l vfscanf_l vscanf_l
   vsnprintf_l vsscanf_l vdprintf_l vasprintf_l


  not sure: c16rtomb_l c32rtomb_l mbrtoc16_l mbrtoc32_l



Re: Fix manpage links in upgrade67.html

2020-05-24 Thread Ingo Schwarze
Hi Andre,

Andre Stoebe wrote on Sat, May 23, 2020 at 06:10:36PM +0200:

> following patch fixes two manpage links that point to the wrong section.

Committed, thanks.
  Ingo


> Index: faq/upgrade67.html
> ===
> RCS file: /cvs/www/faq/upgrade67.html,v
> retrieving revision 1.9
> diff -u -p -r1.9 upgrade67.html
> --- faq/upgrade67.html20 May 2020 20:23:38 -  1.9
> +++ faq/upgrade67.html23 May 2020 14:00:23 -
> @@ -163,7 +163,7 @@ any post-release fixes.
>Regular users must use the
>https://man.openbsd.org/OpenBSD-6.7/sndioctl.1;>sndioctl(1)
>utility in place of
> -  https://man.openbsd.org/OpenBSD-6.7/mixerctl.1;>mixerctl(1)
> +  https://man.openbsd.org/OpenBSD-6.7/mixerctl.8;>mixerctl(8)
>to adjust the volume, for instance:
>  
>
> @@ -177,7 +177,7 @@ any post-release fixes.
>  
>
>Note that audio devices continue to be configured with
> -  https://man.openbsd.org/OpenBSD-6.7/mixerctl.1;>mixerctl(1)
> +  https://man.openbsd.org/OpenBSD-6.7/mixerctl.8;>mixerctl(8)
>as
>https://man.openbsd.org/OpenBSD-6.7/sndioctl.1;>sndioctl(1)
>doesn't expose all audio device controls.



Re: vi.beginner diff and analysis (vi.advanced to follow)

2020-05-24 Thread Ingo Schwarze
Hi Andras,

Andras Farkas wrote on Sun, May 24, 2020 at 01:27:18AM -0400:

> I went through vi.beginner.

Thanks.

> It works both with vi's regular settings,
> and with the settings applied via EXINIT in vi.tut.csh.

Good.

> I have a diff attached.  I was mostly light and gentle with my
> changes,

That's good, excessive and gratuitous changes usually make patches
harder to review, and consequently they more easily fall through
the cracks.

> but I indeed did change some outdated info and incorrect info.
> 
> I seriously doubt vi.tut.csh's usefulness,

Indeed.  In particular, setting an environment variable right
before exiting a shell makes very little sense to me.

> but I'll ultimately judge
> it after going through vi.advanced.  I'll reply to my own email with a
> vi.advanced diff when I go through it. (not tonight, but some night
> soon)

Sounds good.

> Notably, I wasn't able to attach my diff in Firefox

???

Why would you use a web browser for email?  That's just terrible
from a security standpoint and abysmal in terms of usability on
top of that.  And of course, it very often results in misformatted
mails and the like.

In case that isn't obvious, for handling mail, use a real mail
user agent, like mutt(1) or something similar that you like.

> on the OpenBSD snapshot I used.  Neither could Firefox use the normal
> file: URL type.

Sure, you don't want your web browser messing with your private data,
or do you?  On OpenBSD, firefox uses unveil(2) to prevent privacy
breaches when the browser is buggy, see

  /usr/local/share/doc/pkg-readmes/firefox

> Index: vi.beginner

Committed, thanks.
  Ingo



Re: Add CAVEATS to ldom.conf(5) man page

2020-05-20 Thread Ingo Schwarze
Hi Jason,

Jason McIntyre wrote on Wed, May 20, 2020 at 06:38:18AM +0100:

> i'm fine with the text, but does it really warrant a CAVEATS section? it
> sounds like it should just be part of "this is how it works" text.

I may not be the best person to judge this, but as far as i understand,
this is a manual page about a configuration file, answering the
question "What is the syntax and semantics of that file?"

The new paragraph looks like it warns about a trap you may end up
in in some unusual situations depending on what exactly you put
into the file (unusual because 16 feels like a substantial number
to me, presumably it was chosen because it is sufficient for the
most common purposes).  So CAVEATS doesn't feel wrong to me.

Now, maybe, some manual page somewhere might say something like
"this program uses .Pa /dev/ldom* device nodes (or .Xr ldom 4 devices)
to do foobar; make sure you have enough of those".  Judging from
commands like

   $ man -k any=dev/ldom
   $ man -k any~'\'

it doesn't look like such explanations exist just yet, but if someone
were to write them, they might be appropriate in a DESCRIPTION or
a FILES section of some section 8 manual page, maybe.

Either way, i don't feel strongly about any of that.

Yours,
  Ingo



Re: Add CAVEATS to ldom.conf(5) man page

2020-05-19 Thread Ingo Schwarze
Hi Kurt,

Kurt Mosiejczuk wrote on Tue, May 19, 2020 at 07:49:56PM -0400:
> On Mon, May 18, 2020 at 08:12:00PM +0200, Klemens Nanni wrote:
>> On Mon, May 18, 2020 at 01:20:07PM -0400, Kurt Mosiejczuk wrote:

>>> Learning how LDOMs work on this T4-1 and we only create 8 devices
>>> (each /dev/ldom* and /dev/ttyV*) by default. The now-commonly-available
>>> T4-1 machines can do far more than that pretty easily, so bump up the
>>> number created by default from 8 to 16.

>> Seems reasonable, what about adopting vmd(8) CAVEATS for ldomd(8)?
>> Running out of devices with more guests can be nasty do debug.

> Here's an attempt at doing that. There isn't a man page for the ldom
> devices nor is there one specifically for the ttyV* devices, so I left
> the ".Xr" off of those.

Fair enough as long as those don't exist; if you want to stress that
those correspond to file names (below /dev/ i guess), you can use
".Pa ldom" and/or ".Pa ttyV"; your call.

I can't comment on the content, but:

   $ mandoc -Tlint ldom.conf.5
  mandoc: ldom.conf.5:136:10: WARNING: new sentence, new line
  mandoc: ldom.conf.5:139:1: WARNING: blank line in fill mode, using .sp
  mandoc: ldom.conf.5:140:1: WARNING: blank line in fill mode, using .sp
  mandoc: ldom.conf.5:134:2: WARNING:
sections out of conventional order: Sh CAVEATS

So here is a version that

 * starts the new sentence on a new input line
 * avoids trailing blank lines
 * puts CAVEATS before BUGS, which is the conventional ordering

Feel free to use that, or parts of it as desired.

Yours,
  Ingo

P.S.
Maybe somebody wants to look at writing an ldom(4) manual page,
too?  If i understand correctly, then usually, if there is a device
file, having a manual page explaining its purpose is desirable,
right?  Or should ldom(4) maybe be an additional .Nm in vldc(4),
with a brief explanation what the purposes of the various vldc
device nodes are?  Maybe a FILES section might help there, too?
A bit like audio(4) has an additional name audioctl(4), even though
"audioctl" does not appear in the kernel config files?


Index: ldom.conf.5
===
RCS file: /cvs/src/usr.sbin/ldomctl/ldom.conf.5,v
retrieving revision 1.13
diff -u -r1.13 ldom.conf.5
--- ldom.conf.5 21 Feb 2020 19:39:28 -  1.13
+++ ldom.conf.5 20 May 2020 00:22:39 -
@@ -116,6 +116,12 @@
 .Xr eeprom 8 ,
 .Xr ldomctl 8 ,
 .Xr ldomd 8
+.Sh CAVEATS
+Each guest requires one ldom device and one ttyV device per configured
+logical domain.
+Administrators may need to create additional devices using
+.Xr MAKEDEV 8
+if more than 16 logical domains are configured.
 .Sh BUGS
 The hypervisor requires a machine dependent amount of physical memory that is
 reserved automatically.



Re: ddb(4): missing tags

2020-05-17 Thread Ingo Schwarze
Hi Anton,

Anton Lindqvist wrote on Sun, May 17, 2020 at 10:56:18AM +0200:

> The ddb(4) manual documents a couple of commands which can be
> abbreviated. The diff below adds explicit tags for such commands which
> in turn makes it possible to jump to for instance `examine' from within
> your $PAGER.

I agree this is a typical example of a page where manual tagging
makes sense.

Also note that your placement of the .Tg before the .It is good.
That's the logical place.  It also preserves the automatic tagging
of the .Ic, so ":t p" still works, too:

  p[rint]

> Comments? OK?

OK schwarze@
  Ingo

> Index: ddb.4
> ===
> RCS file: /cvs/src/share/man/man4/ddb.4,v
> retrieving revision 1.96
> diff -u -p -r1.96 ddb.4
> --- ddb.4 14 May 2020 06:58:54 -  1.96
> +++ ddb.4 17 May 2020 08:54:18 -
> @@ -202,6 +202,7 @@ are displayed and no other action is per
>  .It Ic help
>  List the available commands.
>  .\" 
> +.Tg examine
>  .It Xo
>  .Oo Ic e Oc Ns
>  .Ic x Ns Op Ic amine
> @@ -275,6 +276,7 @@ is set to the
>  .Ar addr
>  plus the size of the data examined.
>  .\" 
> +.Tg print
>  .It Xo
>  .Ic p Ns Op Ic rint
>  .Op Cm /axzodurc
> @@ -301,6 +303,7 @@ will print something like this:
>  xx
>  .Ed
>  .\" 
> +.Tg pprint
>  .It Xo
>  .Ic pp Ns Op Ic rint
>  .Op Ar addr
> @@ -316,6 +319,7 @@ as part of building a new kernel.
>  .\" .Op Ar addr
>  .\" .Ar expr Op expr ...
>  .\" .Xc
> +.Tg write
>  .It Xo
>  .Ic w Ns Op Ic rite
>  .Op Cm /bhl
> @@ -394,6 +398,7 @@ allows the breakpoint to be silently hit
>  times before stopping at the
>  break point.
>  .\" 
> +.Tg delete
>  .It Xo
>  .Ic d Ns Op Ic elete
>  .Op Ar addr
> @@ -405,6 +410,7 @@ command.
>  .\" .It Xo Ic s Ns Op Cm /p
>  .\" .Op Ic \&, Ns Ar count
>  .\" .Xc
> +.Tg step
>  .It Xo
>  .Ic s Ns Op Ic tep
>  .Op Cm /p
> @@ -437,6 +443,7 @@ Parentheses may be omitted if the functi
>  The number of arguments is currently limited to 10.
>  .\" 
>  .\" .It Ic c Ns Op Cm /c
> +.Tg continue
>  .It Xo
>  .Ic c Ns Op Ic ontinue
>  .Op Cm /c
> @@ -949,6 +956,7 @@ command.
>  A synonym for
>  .Ic show all procs .
>  .\" 
> +.Tg machine
>  .It Xo
>  .Ic mac Ns Op Ic hine
>  .Ar subcommand Op Ar args ...



Re: WireGuard patchset for OpenBSD

2020-05-12 Thread Ingo Schwarze
Hi Matt,

Matt Dunwoodie wrote on Wed, May 13, 2020 at 01:56:51AM +1000:
> On Tue, 12 May 2020 17:36:15 +0200
> Ingo Schwarze  wrote:

>> I feel somewhat concerned that you recommend the openssl(1) command
>> for production use.  As far as i understand, the LibreSSL developers
>> consider openssl(1) as a low-quality program purely intended for
>> testing purposes that should not be used for production.  But that
>> does not need to be addressed now, it can be improved later.

> This is news to me, but what we are using it for very simply is calling
> arc4random_buf, and then base64 encoding. If this isn't appropriate,
> then perhaps a dedicated utility, or ifconfig integration could work.
> 
> wg (from wireguard-tools) also fills this functionality, however
> getting that vs a simple key generator in base would be more work.
> 
> I'm open to suggestions here.

I'm not saying it is necessarily dangerous in this particular case,
i honestly can't judge that.  But i worry that it might perhaps set
a dubious example.

>From a very naive user perspective, it seems to me there are two
practical use cases:

 1) Bring up an interface once more that already was up at some
point in the past and that some peers already know about, so
it matters to use the same private key again.  In that case,
the existing syntax seems just fine to me, and openssl(1)
isn't needed because you already have the private key.

 2) Bring up a completely new interface, desiring a new, randomly
generated private key.  In that use case, a syntax like

  ifconfig wg0 wgkey random wgpeer ... wgaip ... [wgpsk random]

would seem simple, clear, and user-friendly to me,
similar to:

  ifconfig foobar0 lladdr random

Then again, i may be wrong.  I don't think it is necessary to
sort this out before the initial commit.  But it might be worth
thinking about in the long term.

Yours,
  Ingo



Re: WireGuard patchset for OpenBSD

2020-05-12 Thread Ingo Schwarze
Hi Matt,

again, documentation is not critical for the initial commit,
but why not provide feedback right away.

As we already have an ifconfig(8) manual page, i decided to simply
send an updated version of the ifconfig.8 part of the diff
because sending around diffs of diffs feels awkward, and you
can easily apply my version of the diff and compare to what you
had before.


In addition to my changes, it might be useful to mention the unit
for the wgpka persistent-keepalive option.  Seconds?  Minutes?
Also, what are the defaults for wgport and wgpka, if any?


Finally, what is the meaning of "all previously configured peers and
allowed IPs are overwritten"?  It could be either:

  When execution of wgconf begins, -wgpeerall and -wgaipall
  are applied first.

Or:

  When wgconf encounters a wgpeer for a peer that already exists,
  the configuration of that peer (or just the list of allowed IPs?)
  is cleared first.

Either way, it might be good to describe the effect more precisely.


My changes:

 * Minor macro improvements.
 * A few wording improvements.
 * Sorting the text at a few places.
 * New sentence, new line.

By the way, when working on manual pages, using mandoc(1) -T lint
can help in some respects.

Yours,
  Ingo


Index: ifconfig.8
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
retrieving revision 1.346
diff -u -r1.346 ifconfig.8
--- ifconfig.8  29 Apr 2020 13:13:29 -  1.346
+++ ifconfig.8  12 May 2020 16:46:43 -
@@ -207,7 +207,8 @@
 .Xr tun 4 ,
 .Xr vether 4 ,
 .Xr vlan 4 ,
-.Xr vxlan 4
+.Xr vxlan 4 ,
+.Xr wg 4
 .It Cm debug
 Enable driver-dependent debugging code; usually, this turns on
 extra console error logging.
@@ -2041,6 +2042,166 @@
 Clear the tag value.
 Packets on a VLAN interface without a tag set will use a value of
 0 in their headers.
+.El
+.Sh WIREGUARD
+.nr nS 1
+.Bk -words
+.Nm ifconfig
+.Ar wg-interface
+.Op Cm wgkey Ar privatekey
+.Op Cm wgport Ar port
+.Op Cm wgrtable Ar rtable
+.Oo
+.Oo Fl Oc Ns Cm wgpeer Ar publickey
+.Op Cm wgpsk Ar presharedkey
+.Op Fl wgpsk
+.Op Cm wgpka Ar persistent-keepalive
+.Op Cm wgpip Ar ip port
+.Op Cm wgaip Ar allowed-ip/prefix
+.Op Fl wgaipall
+.Oc
+.Op Fl wgpeerall
+.Op Cm wgconf
+.Ek
+.nr nS 0
+.Pp
+The following options are available for
+.Xr wg 4
+interfaces:
+.Bl -tag -width Ds
+.It Cm wgkey Ar privatekey
+Set the local private key of the interface to
+.Ar privatekey .
+This is a random 32 byte value, encoded as base64.
+It can be generated as follows:
+.Pp
+.Dl $ openssl rand -base64 32
+.Pp
+A valid Curve25519 key is required to have 5 bits set to specific
+values.
+This is done by the interface, so it is safe to provide a random
+32 byte base64 string.
+.Pp
+Once set, the corresponding public key will be displayed
+in the interface status; it must be distributed to peers
+that this interface intends to communicate with.
+.It Cm wgport Ar port
+Set the UDP
+.Ar port
+that the tunnel operates on.
+The interface will bind to
+.Dv INADDR_ANY
+and
+.Dv IN6ADDR_ANY_INIT .
+.It Cm wgrtable Ar rtable
+Use routing table
+.Ar rtable
+instead of the default table for the tunnel.
+The tunnel does not need
+to terminate in the same routing domain as the interface itself.
+.Ar rtable
+can be set to any valid routing table ID; the corresponding routing
+domain is derived from this table.
+.It Cm wgpeer Ar publickey
+Select the peer to perform the subsequent operations on.
+This creates a peer with the associated 32 byte, base64 encoded
+.Ar publickey
+if it does not yet exist.
+This option can be specified multiple times in a single command.
+.It Cm -wgpeer Ar publickey
+Remove the peer with the associated
+.Ar publickey .
+.It Cm -wgpeerall
+Remove all peers from the interface.
+.El
+.Pp
+The following options configure peers for the interface.
+Each interface can have multiple peers.
+In order to add a peer, a
+.Cm wgpeer
+option must be specified, followed by its configuration options.
+.Bl -tag -width Ds
+.It Cm wgpsk Ar presharedkey
+Set the preshared key for the peer.
+This is a random 32 byte, base64 encoded string
+that both ends must agree on.
+It offers a post-quantum resistance to the Diffie-Hellman exchange.
+If there is no preshared key, the exact same handshake is performed,
+however the preshared key is set to all zero.
+This can be generated in the same way as
+.Ar privatekey .
+.It Cm -wgpsk
+Remove the preshared key from the specified peer.
+.It Cm wgpka Ar persistent-keepalive
+Set the interval that a keepalive should be sent at.
+By setting the interval to 0, the functionality is disabled.
+This is often used to ensure a peer will be accessible
+when protected by a firewall, as is when behind a NAT address.
+A value of 25 is commonly used.
+.It Cm wgpip Ar ip port
+Set the IP address and port to send the encapsulated packets to.
+If the peer changes address, the local interface will update the address
+after receiving a correctly authenticated 

Re: WireGuard patchset for OpenBSD

2020-05-12 Thread Ingo Schwarze
Hi Matt,

thanks for doing all this work.  Note that i cannot provide feedback
on the code or concepts, and also note that when the code is ready,
a developer can commit it even if there are still issues to sort out
with the documentation.  We do value good documentation, but the exact
point in time when it is polished matters little.

All the same, i looked through the wg(4) manual page and improved
a few details.  Please apply the following patch to what you sent.


In addition to my changes, this line must be fixed before commit:

  .\" Copyright?

Please just use the normal /usr/share/misc/license.template with
the commenting method adjusted from C to roff(7) comments.  If you
wrote all the manual page text from scratch, use your own name and
year(s).  If it is a derivative work based on the work of others,
retain the original Copyright and license and add your own name and
date(s) if your changes are significant enough.


I feel somewhat concerned that you recommend the openssl(1) command
for production use.  As far as i understand, the LibreSSL developers
consider openssl(1) as a low-quality program purely intended for
testing purposes that should not be used for production.  But that
does not need to be addressed now, it can be improved later.

When providing exactly one example, it isn't ideal that that example
doesn't actually do anything practically useful.  Again, that's not
a critical problem and can be improved later.


Theo is right that .Ek and .nr nS are slightly awkward in manual
pages, but they are still somewhat widespread in particular in
driver and kernel manuals, so they wouldn't be a serious problem.
However, the whole section containing them is useless in the first
place, so the issue just vanishes with no additional effort.


Rationale for my changes:

 * Add the missing $OpenBSD$ tag.
 * New sentence, new line.
 * Minor macro improvements.
 * At a few places, purge phrases that say nothing.
 * A few minor wording improvements.
 * Delete the DIRECTIVES section: it contains no new information
   and ifconfig(8) was already referenced prominently above.
 * Use the standard section name DIAGNOSTICS.

To keep each mail small and focussed, i decided to not talk about
the ifconfig(8) manual page in the same message.

Yours,
  Ingo


--- wg.4.matt   Tue May 12 16:06:34 2020
+++ wg.4Tue May 12 17:20:18 2020
@@ -1,3 +1,4 @@
+.\" $OpenBSD$
 .\" Copyright?
 .Dd $Mdocdate: Feb 14 2020 $
 .Dt WG 4
@@ -18,18 +19,18 @@
 .Pp
 Each interface is able to connect to a number of endpoints, relying on
 an internal routing table to direct outgoing IP traffic to the correct
-endpoint. Incoming traffic is also matched against this routing table
+endpoint.
+Incoming traffic is also matched against this routing table
 and dropped if the source does not match the corresponding output route.
 .Pp
 The interfaces can be created at runtime using the
-.Ic ifconfig wg Ns Ar N Ic create
+.Ic ifconfig Cm wg Ns Ar N Cm create
 command or by setting up a
 .Xr hostname.if 5
 configuration file for
 .Xr netstart 8 .
 The interface itself can be configured with
-.Xr ifconfig 8 ;
-see it's manual page for more information.
+.Xr ifconfig 8 .
 Support is also available in the
 .Nm wireguard-tools
 package by using the
@@ -41,70 +42,75 @@
 .Nm wg
 interfaces support the following
 .Xr ioctl 2 Ns s :
-.Bl -tag -width indent -offset 3n
+.Bl -tag -width Ds -offset indent
 .It Dv SIOCSWG Fa "struct wg_data_io *"
 Set the device configuration.
 .It Dv SIOCGWG Fa "struct wg_data_io *"
 Get the device configuration.
 .El
-.Sh DESIGN
-WireGuard is designed as a small, secure, easy to use VPN. It operates at IP
-level, supporting both IPv4, IPv6.
-
-The following items give a brief overview of WireGuard features:
+.Ss Design
+WireGuard is designed as a small, secure, easy to use VPN.
+It operates at the IP level, supporting both IPv4 and IPv6.
+.Pp
+The following list provides a brief overview of WireGuard terminology:
 .Bl -tag -width indent -offset 3n
 .It Peer
-A peer is a host that the interface creates a connection with. There is
-no concept of client/server as both ends of the connection are equal. An
-interface may have multiple peers.
+A peer is a host that the interface creates a connection with.
+There is no concept of client/server as both ends of the connection
+are equal.
+An interface may have multiple peers.
 .It Key
-Each interface has a private key and corresponding public key. The
-public key is used to identify the interface to other peers.
-.It Preshared-Key
+Each interface has a private key and corresponding public key.
+The public key is used to identify the interface to other peers.
+.It Preshared key
 In addition to the interface keys, each peer pair can have a
-unique preshared key. This key is used in the handshake to provide
-post-quantum security. It is optional, however recommended.
-.It Allowed-IPs
-Allowed-IPs dictate the tunneled IP addresses each peer is allowed to
-send from. After 

Re: Broken links to the usb.org document library

2020-05-12 Thread Ingo Schwarze
Hi,

clematis wrote on Tue, May 12, 2020 at 03:06:40AM +0200:

> - Should we update those links?

For the manual pages, that seems clear: if some document is worth
linking to (which i assume it is unless told otherwise, if a link
is currently present in a manual page), then we should provide the
best possible form of the link.

I did not touch source code files, i'll leave the decision whether
it is desirable to change those files and whether it is worth the
effort to the developers working on the code.

> - Should we only refer to the document name/version and people can go an
>   search on whatever document library will be available in the future?  

If a site is stupid enough to keep changing their URIs again and
again, maintenance might become annoying enough that we could just
stop linking to that site, but i felt no urgent need to go that far
just yet.

> For the couple missing I can try to contact them.

If you contact them, please also remind them that changiung URIs
is a terrible idea, particularly for documentation and standards
that other documentation may need to link to.

> (Otherwise there's mirror available if that's acceptable).

I prefer linking from authoritative documentation (including manual
pages) to other authoritative documentation (including official
upstream websites), but if those official upstream websites are
unusable for some reason (like lack of stable URIs), linking to
other trustworthy sources may in some cases be a viable alternative.
It's a matter of judgement in each individual case what is most
helpful for users without causing excessive pain for maintenance.

For now, i committed the following minimal patch.

Note that i preferred linking to HTML pages over directly linking
to ZIP files, even if the main content of the HTML page is a link
to the ZIP file.  The links in the header and footer of the HTML
page may occasionally help users, too.

OpenBSD USB developers: please feel free to tweak further in
whichever way you consider useful.

Yours,
  Ingo


Index: lib/libusbhid/usbhid.3
===
RCS file: /cvs/src/lib/libusbhid/usbhid.3,v
retrieving revision 1.19
diff -u -p -r1.19 usbhid.3
--- lib/libusbhid/usbhid.3  18 Feb 2019 17:29:43 -  1.19
+++ lib/libusbhid/usbhid.3  12 May 2020 12:58:21 -
@@ -211,7 +211,7 @@ The default HID usage table.
 .\" .Sh EXAMPLES
 .Sh SEE ALSO
 The USB specifications can be found at:
-.Lk http://www.usb.org/developers/docs/
+.Lk https://www.usb.org/documents/
 .Pp
 .Xr uhid 4 ,
 .Xr usb 4
Index: share/man/man4/cdce.4
===
RCS file: /cvs/src/share/man/man4/cdce.4,v
retrieving revision 1.25
diff -u -p -r1.25 cdce.4
--- share/man/man4/cdce.4   9 Aug 2019 21:45:02 -   1.25
+++ share/man/man4/cdce.4   12 May 2020 12:58:21 -
@@ -120,7 +120,7 @@ is running low on mbufs.
 .Xr ifconfig 8
 .Rs
 .%T "Universal Serial Bus Class Definitions for Communication Devices"
-.%U http://www.usb.org/developers/docs/devclass_docs/
+.%U 
https://www.usb.org/document-library/class-definitions-communication-devices-12
 .Re
 .Rs
 .%T "Data sheet Prolific PL-2501 Host-to-Host Bridge/Network Controller"
Index: share/man/man4/upd.4
===
RCS file: /cvs/src/share/man/man4/upd.4,v
retrieving revision 1.4
diff -u -p -r1.4 upd.4
--- share/man/man4/upd.426 Jun 2015 09:00:37 -  1.4
+++ share/man/man4/upd.412 May 2020 12:58:21 -
@@ -70,8 +70,8 @@ NeedReplacement
 .Xr sensorsd 8 ,
 .Xr sysctl 8
 .Pp
-The USB Power Devices specification can be found at
-.Lk http://www.usb.org/developers/hidpage/
+The USB Power Devices specification can be found at:
+.Lk https://www.usb.org/hid
 .Sh HISTORY
 The
 .Nm
Index: share/man/man4/usb.4
===
RCS file: /cvs/src/share/man/man4/usb.4,v
retrieving revision 1.199
diff -u -p -r1.199 usb.4
--- share/man/man4/usb.417 Dec 2019 13:08:54 -  1.199
+++ share/man/man4/usb.412 May 2020 12:58:21 -
@@ -676,8 +676,8 @@ Human Interface Devices (HID).
 .Xr config 8 ,
 .Xr usbdevs 8
 .Pp
-The USB specifications can be found at
-.Lk http://www.usb.org/developers/docs/
+The USB specifications can be found at:
+.Lk https://www.usb.org/documents
 .Sh HISTORY
 The
 .Nm
Index: share/man/man4/umb.4
===
RCS file: /cvs/src/share/man/man4/umb.4,v
retrieving revision 1.10
diff -u -p -r1.10 umb.4
--- share/man/man4/umb.418 Feb 2020 08:09:37 -  1.10
+++ share/man/man4/umb.412 May 2020 12:58:21 -
@@ -61,7 +61,7 @@ The following devices should work:
 .Xr route 8
 .Rs
 .%T "Universal Serial Bus Communications Class Subclass Specification for 
Mobile Broadband Interface Model"
-.%U 

Re: Diff for www:mail

2020-05-05 Thread Ingo Schwarze
Salut Stephane,

b...@stephane-huc.net wrote on Mon, May 04, 2020 at 03:51:33PM +0200:

> Here a diff for www page: mail
> Please, review this diff to add French ML

Committed.

Merci,
  Ingo


> Index: mail.html
> ===
> RCS file: /cvs/www/mail.html,v
> retrieving revision 1.170
> diff -u -r1.170 mail.html
> --- mail.html 24 Apr 2020 21:51:21 -  1.170
> +++ mail.html 4 May 2020 13:38:21 -
> @@ -423,6 +423,14 @@
>   PLEASE KEEP THIS LIST SORTED, EXCEPT FOR TRANSLATIONS, WHERE YOU SHOULD PUT
>   THE LIST IN YOUR LANGUAGE, IF ONE EXISTS, HEAD OF LIST.
>   -->
> +
> +French:
> +bla...@openbsd.fr.eu.org
> +To subscribe, send a message to 
> +mailto:blabla+subscr...@openbsd.fr.eu.org;>
> +blabla+subscr...@openbsd.fr.eu.org
> +
> +(https://openbsd.fr.eu.org/blabla/;>Archive)
>  
>  
>  Spanish:



Re: Mention /etc/examples/ in those config files manpages + FILES short description

2020-05-01 Thread Ingo Schwarze
Hi,

clematis wrote on Fri, May 01, 2020 at 11:37:51AM +0200:
> On Fri, May 01, 2020 at 06:52:27AM +0100, Jason McIntyre wrote:

>> i don;t understand your reference to 28 out of 41 files. i cannot see
>> where we added any expanded FILES entries. can you provide a summary of
>> these inconsistencies, please?

> There's 41 example files in /etc/examples/. 28 of them are listed in the
> FILES section of their respective manpage with a short description.
> The patches I've sent were to do the same for the remaining 13 files.
> Either adding them when there were missing and/or adding the short
> description.

I think there is value in having complete FILES sections, listing all
the files (with full paths if possible) directly related to the topic
of a manual page because it both allows to see at first glance with
files are used by the command and where exactly they located (as a
non-trivial example, consider /etc/mail/smtpd.conf), because it
potentially support "apropos Pa=" searches, and because it costs
almost nothing: exactly one line per file when it's obvious what
the file is all about.  Whether the file needs an explanation of
more than one line should be decided on a case by case basis.
Very often, it does not.

In conclusion, i do support adding the missing FILES sections
dhcpd.conf(5) and httpd.conf(5).

I don't care about changes like these either way:

 .It Pa /etc/man.conf
+Default
+.Xr man 1
+configuration file.
 .It Pa /etc/examples/man.conf
+Example configuration file.

I think both contain exactly the same information.  If jmc@ thinks
either having or not having the trivial one-line explanation is
better for some reason, or that there is value in doing this aspect
consistently, i'm happy to have this made consistent.  If jmc@
thinks both forms are equally legitimate and there is no value in
consistency in this respect, that's fine with me, too.  There is
certainly ample historical precedent for both forms.

I don't really like this particular change:

 .It Pa /etc/examples/man.conf
+Example configuration file illustrating possible search path ordering and
+output options.

The paramount design goal of the new man.conf(5) file format was
to keep it as simple as possible and i think that goal was reached:
there are now exactly two directives, both with an extremely simple
syntax, and the complete formatted manual page fits into 80 lines.

I consider it detrimental to restate what the file is for in the
FILES section because it is obvious and already concisely said right
above, so it just adds gratuitious verbiage.

Explanations longer than one line should be reserved for special,
non-trivial cases where they really matter.  Such cases do occasionally
exist, in particular when a config file format is exceptionally
complicated, the file can do many different things, among them quite
unusual ones, it is not obvious which aspects are those that it is
most commonly used for, and/or the examples file deliberately
focusses on a specific aspect for specific reasons and it is
non-trivial which aspect that is or why.


I think Jason should just go ahead with whatever parts of the diff
he likes and discard the rest.  He is usually very good at deciding
issues of style and consistency.

Yours,
  Ingo



Re: [PATCH] printf: Add %B conversion specifier for binary

2020-04-27 Thread Ingo Schwarze
Hi,

Alejandro Colomar wrote on Mon, Apr 27, 2020 at 08:26:38PM +0200:

> This patch adds a new feature to the ``printf`` family of functions:
> ``%B`` conversion specifier for printing unsigned numbers in binary.

No.  We do not want non-standard extensions to standard functions
unless they provide unusually important benefit and/or are very
widely supported elsewhere.

> I also sent today a patch to add this specifier to glibc.  They are
> concerned about adding a new non-standard specifier, but if more C libs
> are going to add it at the same time, it may become a thing.

Usually, glibc is quite proactive about adding non-standard stuff
even if it is not really useful.  If even they reject the idea,
OpenBSD certainly isn't the place to start campaigning.

Yours,
  Ingo



Re: Update /etc/examples/doas.conf and doas.conf(5)

2020-04-25 Thread Ingo Schwarze
Hi,

clematis wrote on Sat, Apr 25, 2020 at 11:34:14PM +0200:

> But then based on your feedback I wonder if there's even any 
> value of an example file which gives not much more than an already
> well documented manpage?

We had that discussion among developers at least twice in the past.

If i were the only developer, which very fortunately i'm not,
i would probably go for fewer files in /etc/examples/.

But there are also several developers who find it quite
convenient to have a larger number of small example files
around, for various reasons, for example because it is often
quicker to get a general feeling for the basic syntax by looking
at a concise example file compared to reading an abstract
description in the manual page, and because copy and paste
from manual pages is somewhat hindered by indentation (don't
suggest solutions for that, it is not really an issue that
needs solving).

It hasn't been very productive when i proposed such files
for deletion, so i tend to not press such issues again.

But that doesn't mean i'd gladly agree to start tripling or
quadrupling the sizes of existing files without good reasons.

> And again, please don't get me wrong, I don't
> mean to play devil's advocate but do we even need an
> /etc/examples/doas.conf? 
> (for all the very valid reasons you've mentioned as well, clarity,
> existing documentation etc...)

If i remember correctly, there were quite a few developers
who considered it useful to keep a short examples/doas.conf
around and didn't like the idea of deleting it.

> I would be tempted to put a few others in the same boat:

Yes, there are certainly a few others were more or less the
same arguments apply like for doas.conf.

Yours,
  Ingo



Re: Update /etc/examples/doas.conf and doas.conf(5)

2020-04-25 Thread Ingo Schwarze
Hi,

i strongly dislike what you are doing here.  Not just because of one
particular line, but in general.

clematis wrote on Sat, Apr 25, 2020 at 07:41:40PM +0200:

> Looking arround in /etc/examples/ I felt like some of those files
> aren't as "verbose" as others.

The shorter the better.

Yes, there are exceptions, when something is really complicated
and configuring it properly requires a lot of legwork, like for
bgpd.conf(5).  Or if something is hard to explain in abstract
terms and easy to show with an example, like acme-challenge in
httpd.conf(5).

> For example doas.conf doas.conf(5) contains more EXAMPLES
> than /etc/examples/doas.conf

Which is good.  The manual page is the ideal place for documentation.

The program doas(1) is very well-designed.  It is very simple and
very easy to use yet quite powerful (even though it intentionally
does not have all bells and whistles that some might need in special
situations).

Let's not obscure that good design by making the /etc/examples/ file
excessively large and cumbersome.

> Please let me know if there is a prefered way.

Keep it as short as possible, that's the most important point IMHO.

For a program as well designed as doas(1), the primary benefit of
the file is to signal to somebody who looks into the directory:
Look, this is a program you can configure with a file in /etc/.
In addition to that, maybe it could also demonstrate:
this is how a cmment looks like, and this is how the basic
syntactic patterns look like.

Repeating the content of the manual page would be detrimental.

> And there's probably a middle ground that could be find as well.
> I don't know if anyone has a strong opinion or recommandations.

I dislike the idea of a middle ground in this context.

> So if that makes sense for you, please find attached a patch for
> examples/doas.conf adding the main examples from doas.conf(5) and
> bsd.port.mk(5).

Please, no.

Yours,
  Ingo



Re: www/advisories unreferenced files - should they go to the attic?

2020-04-24 Thread Ingo Schwarze
Hi Theo,

Theo de Raadt wrote on Fri, Apr 24, 2020 at 10:57:23AM -0600:

> Somewhere along the line, the web pages were changed to no longer
> reference those pages, and that is a shame.
> 
> It is a shame that the old errata pages don't point at those files.
> 
> They aren't quite in the same format, but why not try to show
> the history of our work?
> 
> If we start saying we don't need to document 20 years ago, what's
> the difference with saying we don't need to document 2 years ago?
> History is nice to have, and absence of historical context is dissapointing.

I agree with what you are saying.

So here is a patch to reference those files from our web pages.

 * Even though the patch creates errata20.html, i don't include
   adding links from all errata pages to errata20.html into this
   patch, to avoid making the patch unreadable from churn.
   If people agree with me that such links should be on each
   page, i can do that afterwards with a separate commit.

 * While most errata entries describe the problem with a few lines
   of text, my aim for this patch was to kept the changes minimal,
   to make it easy to review the patch.  So i'm just saying in
   one word, or in a few words, which subsystem the problem was
   related to.  If people think more text should be added to the
   errata pages in addition to the links, that can be done in
   later patches.

 * sni_20_tgetent.txt is strange in so far as it appeared quite
   late, way after OpenBSD 2.1, but it says that "Versions of OpenBSD
   newer than 2.0 are NOT vulnerable to this problem."  For now, i
   kept in in the chronological order anyway.

 * The files res_random.txt and sni_12_resolverid.txt contain
   essentially the same text, except that each of them contains a
   very small amount of errors or omissions apparent by looking at
   the diff between the two.  I suggest keeping sni_12_resolverid.txt
   because that's the name better fitting the names of other files,
   fixing some incorrect line breaks in that file, and deleting
   the redundant copy res_random.txt.

 * The file ssh_trojan.txt is very special.  It is not about any bug
   in the OpenBSD source code at all, so no patching was ever needed
   for it.  Consequently, i don't think it belongs on the errata*
   pages, and linking it should be done separately if desired, in
   a different way, probably from somewhere below the OpenSSH site.

OK?
  Ingo


Index: errata20.html
===
RCS file: errata20.html
diff -N errata20.html
--- /dev/null   1 Jan 1970 00:00:00 -
+++ errata20.html   24 Apr 2020 20:25:21 -
@@ -0,0 +1,119 @@
+
+
+
+
+OpenBSD 2.0 Errata
+
+
+
+https://www.openbsd.org/errata20.html;>
+
+
+
+OpenBSD
+2.0 Errata
+
+
+
+For errata on a certain release, click below:
+2.1,
+2.2,
+2.3,
+2.4,
+2.5,
+2.6,
+2.7,
+2.8,
+2.9,
+3.0,
+3.1,
+3.2,
+3.3,
+3.4,
+3.5,
+3.6,
+
+3.7,
+3.8,
+3.9,
+4.0,
+4.1,
+4.2,
+4.3,
+4.4,
+4.5,
+4.6,
+4.7,
+4.8,
+4.9,
+5.0,
+5.1,
+5.2,
+
+5.3,
+5.4,
+5.5,
+5.6,
+5.7,
+5.8,
+5.9,
+6.0,
+6.1,
+6.2,
+6.3,
+6.4,
+6.5,
+6.6.
+
+
+
+For the OpenBSD 2.0 release, the formal process for creating and
+distributing errata patches had not been developed yet.
+Nevertheless, a number of security advisories do exist.
+
+
+
+
+SECURITY VULNERABILITY in BIND
+Secure Networks advisory 01
+(November 18, 1996)
+
+
+
+SECURITY VULNERABILITY in Vixie cron
+Secure Networks advisory 02
+(December 16, 1996)
+
+
+
+SECURITY VULNERABILITY in default cron jobs
+Secure Networks advisory 03
+(December 23, 1996)
+
+
+
+SECURITY VULNERABILITY related to source routing
+and TCP spoofing
+Secure Networks advisory 06
+(February 10, 1997)
+
+
+
+SECURITY VULNERABILITY with 4.4BSD NFS file handles
+Secure Networks advisory 10
+(March 7, 1997)
+
+
+
+SECURITY VULNERABILITIES in BIND
+Secure Networks advisory 12
+(April 22, 1997)
+
+
+
+SECURITY VULNERABILITIES in Kerberos V
+Secure Networks advisory 13
+(April 29, 1997)
+
+
+
Index: errata21.html
===
RCS file: /cvs/www/errata21.html,v
retrieving revision 1.84
diff -u -p -r1.84 errata21.html
--- errata21.html   30 Sep 2019 13:17:48 -  1.84
+++ errata21.html   24 Apr 2020 20:25:21 -
@@ -22,6 +22,7 @@
 
 
 For errata on a certain release, click below:
+2.0,
 2.2,
 2.3,
 2.4,
@@ -37,8 +38,8 @@ For errata on a certain release, click b
 3.4,
 3.5,
 3.6,
-3.7,
 
+3.7,
 3.8,
 3.9,
 4.0,
@@ -54,8 +55,8 @@ For errata on a certain release, click b
 5.0,
 5.1,
 5.2,
-5.3,
 
+5.3,
 5.4,
 5.5,
 5.6,
@@ -290,6 +291,37 @@ command without leading colon(s) like:
 
 
 
+
+SECURITY VULNERABILITY in 4.4BSD procfs
+OpenBSD advisory (June 24, 1997)
+
+
+
+SECURITY VULNERABILITY in 4.4BSD rfork
+OpenBSD advisory (August 2, 1997)
+
+
+
+SECURITY VULNERABILITY in vacation
+Secure Networks advisory 18
+(September 1, 1997)
+
+
+
+SECURITY VULNERABILITY in I/O Signal Handling
+OpenBSD advisory (September 15, 

Re: [PATCH] [src] usr.bin/audioctl/audioctl.8, usr.bin/mixerctl/mixerctl.8 - manpages moved to section 8, mark them as such

2020-04-22 Thread Ingo Schwarze
Hi Raf,

Raf Czlonka wrote on Thu, Apr 23, 2020 at 12:58:41AM +0100:

> Recently moved manpages bear section 1 number - update accordingly.

Committed, thanks.
  Ingo


> Index: usr.bin/audioctl/audioctl.8
> ===
> RCS file: /cvs/src/usr.bin/audioctl/audioctl.8,v
> retrieving revision 1.3
> diff -u -p -r1.3 audioctl.8
> --- usr.bin/audioctl/audioctl.8   22 Apr 2020 21:39:21 -  1.3
> +++ usr.bin/audioctl/audioctl.8   22 Apr 2020 23:55:27 -
> @@ -27,7 +27,7 @@
>  .\" POSSIBILITY OF SUCH DAMAGE.
>  .\"
>  .Dd $Mdocdate: April 22 2020 $
> -.Dt AUDIOCTL 1
> +.Dt AUDIOCTL 8
>  .Os
>  .Sh NAME
>  .Nm audioctl
> Index: usr.bin/mixerctl/mixerctl.8
> ===
> RCS file: /cvs/src/usr.bin/mixerctl/mixerctl.8,v
> retrieving revision 1.4
> diff -u -p -r1.4 mixerctl.8
> --- usr.bin/mixerctl/mixerctl.8   22 Apr 2020 21:39:21 -  1.4
> +++ usr.bin/mixerctl/mixerctl.8   22 Apr 2020 23:56:20 -
> @@ -28,7 +28,7 @@
>  .\" POSSIBILITY OF SUCH DAMAGE.
>  .\"
>  .Dd $Mdocdate: April 22 2020 $
> -.Dt MIXERCTL 1
> +.Dt MIXERCTL 8
>  .Os
>  .Sh NAME
>  .Nm mixerctl



Re: Update www/support.html

2020-04-18 Thread Ingo Schwarze
Hi,

clematis wrote on Sat, Apr 18, 2020 at 11:26:51AM +0200:

> I went through the sites listed on support.html to check if they were
> still valid. Caught a few that needs removal or update.

Thank you for doing that work!

> Please find the details below and the changes in the diff attached.
> 
> Removing:
> - Obtuse Systems Corporation: Website down. I did check with Bob Beck
>   offline and he is OK with the removal.
> - Duncan Campbell: neotext.ca isn't the original website anymore. I've
>   emailed the contact but haven't heard back.
> - Alessio Pennasilico: domain name is for sale.
> - James Wright: URL / domain returns empty page. web.archive.org had a
>   snapshot from 2018 I have emailed the contact but haven't heard back.
> - Kirill Ponazdyr: URL rdr to another dead site. No email was provided.
> - Martin Wundram: URL only display "maintenance" for quite some time.
>   I've emailed the contact - no response.
> - Doug Maxwell: blank site. sent email no reply.
>
> Updating:
> - Pierre-Emmanuel Andre: URL returns 404, I've removed the
>   "/prestations-informatiques/" to point to their homepage instead.

I committed those changes.
Thanks for explaining clearly what you did.

> - Todd T. Fries: website down. Browsing around I could find an active
>   github but the homepage listed there is down as well. 
>   No reponse to my email.

>From 1997 to 2015, Todd Fries used to be quite an important
OpenBSD developer, so i want to be extra careful in his case
and have sent him another email.  Feel free to remind me if
nothing happens for more than a week.

Yours,
  Ingo



Re: implement locale(1) charmap argument

2020-04-17 Thread Ingo Schwarze
Hi Stefan and Todd,

Stefan Sperling wrote on Fri, Apr 17, 2020 at 08:55:29AM +0200:
> On Thu, Apr 16, 2020 at 09:35:18PM +0200, Ingo Schwarze wrote:

>>$ locale -m
>>   UTF-8
>>$ locale charmap
>>   UTF-8
>>$ LC_ALL=C locale charmap
>>   US-ASCII
>>$ LC_ALL=POSIX locale charmap
>>   US-ASCII

> I am OK with your diff,

Thanks to both of you for checking, i have put it in.

> and noticed a separate issue with -m which
> is exposed by this change:
> 
> If US-ASCII is an available charmap, shouldn't locale -m list "US-ASCII"
> in addition to "UTF-8"?

I'm not completely sure what "available charmaps" is supposed to mean
in the POSIX standard.

Testing on an old Debian system, is see this:

   $ locale -m > charmaps.loc
   $ wc -l charmaps.loc
  235
   $ ls /usr/share/i18n/charmaps | sed 's/.gz$//' | sort > charmaps.ls 
   $ diff -u charmaps.ls charmaps.loc | grep '^[+-][^+-]'
  +MAC_CENTRALEUROPE
  +NF_Z_62-010_(1973)
  +WIN-SAMI-2
   $ locale charmap
  UTF-8
   $ locale -m | grep UTF
  UTF-8
   $ LC_CTYPE=C locale charmap
  ANSI_X3.4-1968
   $ locale -m | grep 1968
  ANSI_X3.4-1968

So "locale -m" gives almost a directory listing, but not quite;
it produces a few additional entries that aren't in the directory.
The return values from "locale charset" appear in "locale -m".
Then again, Linux is not a certified UNIX system.  So let's try
with something certified:

   > uname -a
  SunOS unstable11s 5.11 11.3 sun4u sparc SUNW,SPARC-Enterprise
   > locale charmap
  UTF-8
   > LC_CTYPE=C locale charmap
  646
   > locale -m | wc
   0   0   0

It's a bit difficult because Solaris 11 does not provide locate(1),
but i failed to find any charmap files there.  Both UTF-8 and
US-ASCII work (i tested that by compiling and running mandoc)
but still "locale -m" returns nothing.

   > uname -a
  SunOS unstable10s 5.10 Generic_150400-17 sun4v sparc 
SUNW,SPARC-Enterprise-T5220
   > locale charmap
  646
   > LC_CTYPE=en_US.UTF-8 locale charmap
  UTF-8
   > locale -m
  iso_8859_1/charmap.src
   > ls -F /usr/lib/localedef/src/
  charmaps/ en_US.UTF-8/  extensions/   iso_8859_1/   locales/
   > ls -F /usr/lib/localedef/src/charmaps/
  charmap.ANSI1251.bz2  charmap.ISO8859-9.bz2 charmap.iso-8859-5.bz2
  charmap.ISO8859-1.bz2 charmap.KOI8-R.bz2charmap.iso-8859-6.bz2
  charmap.ISO8859-13.bz2charmap.UTF-8.bz2 charmap.iso-8859-7.bz2
  charmap.ISO8859-15.bz2charmap.ansi-1251.bz2 charmap.iso-8859-8.bz2
  charmap.ISO8859-2.bz2 charmap.ar.bz2@   charmap.iso-8859-9.bz2
  charmap.ISO8859-4.bz2 charmap.he.bz2@   charmap.koi8-r.bz2
  charmap.ISO8859-5.bz2 charmap.iso-8859-1.bz2charmap.utf-8.bz2
  charmap.ISO8859-6.bz2 charmap.iso-8859-13.bz2   charmap.utf8.bz2@
  charmap.ISO8859-7.bz2 charmap.iso-8859-15.bz2
  charmap.ISO8859-8.bz2 charmap.iso-8859-2.bz2

Same vendor, different version, different behaviour.  Again, both UTF-8
and US-ASCII work, and there are several charmap files, but "locale -m"
returns something that is neither a charmap name nor a filename for any
of the locales, nor a list of anything.

Frankly, i doubt the usefulness of "locale -m" in general, and even more
so on OpenBSD: if i understand correctly, it is supposed to be used to
determine valid input for the -f option of the localedef(1) utility,
which we don't even have.

Naively, it does seem like it would make sense to have "locale -m"
print a list of possible output values of "locale chardef", so i'm
not opposed to adding "US-ASCII" to it.  But that doesn't appear to
be how it works elsewhere, at least not everywhere.  I found no
documentation stating clearly what it is supposed to do, POSIX feels
murky at best.

Also, look at this:

  http://man.bsd.lv/FreeBSD-12.0/locale#BUGS
  http://man.bsd.lv/NetBSD-8.1/locale#BUGS

  "BUGS
   Since FreeBSD does not support charmaps in their POSIX meaning,
   locale emulates the -m option using the CODESETs listing of all
   available locales."

That does look somehwat similar to what you are suggesting,
but *they* call it a bug!

Feel free to add "US-ASCII\n" if you like, it does feel as if it
might add some minor clarity, but i hardly expect any real practical
benefit.

Yours,
  Ingo



implement locale(1) charmap argument

2020-04-16 Thread Ingo Schwarze
Hi,

our locale(1) implementation is intentionally simplistic
and implements only a subset of this POSIX specification:

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/locale.html

However, one feature is missing that is actually useful and arguably
also well-placed inside the locale(1) utility.  If you want to know
from within a C program which character encoding is actually being
used (as opposed to which one the user requested), you can use the
nl_langinfo(3) function.  But i'm not aware of a possibiliy to ask
the same from within a sh(1) program.

POSIX says that "locale charmap" should answer that question.

In the next release of textproc/groff, that feature of locale(1)
will be used in the test suite, and it seems reasonable to do so.

So, here is a very simple patch to support the "charmap" argument.

Testing:

   $ export LC_CTYPE=en_US.UTF-8
   $ locale
  LANG=
  LC_COLLATE="C"
  LC_CTYPE=en_US.UTF-8
  LC_MONETARY="C"
  LC_NUMERIC="C"
  LC_TIME="C"
  LC_MESSAGES="C"
  LC_ALL=

   $ locale -a | wc
  68  68 794
   $ locale -m
  UTF-8
   $ locale charmap
  UTF-8
   $ LC_ALL=C locale charmap
  US-ASCII
   $ LC_ALL=POSIX locale charmap
  US-ASCII

   $ LC_ALL=NonSense locale charmap
  US-ASCII
   $ locale -x
  locale: unknown option -- x
  usage: locale [-a | -m | charmap]
   $ locale nonsense
  usage: locale [-a | -m | charmap]
   $ locale -am 
  usage: locale [-a | -m | charmap]
   $ locale -a charmap
  usage: locale [-a | -m | charmap]
   $ locale -m charmap
  usage: locale [-a | -m | charmap]
   $ locale charmap nonsense
  usage: locale [-a | -m | charmap]

OK?
  Ingo


P.S.
It would be trivial to also support the POSIX -k option, as in
   $ locale -k charmap
  charmap="UTF-8"
but that doesn't actually feel useful and i'm not aware of anything
that might want to use it, so KISS and let's proceed one step at a time.
Supporting "name" arguments other than "charmap" would make little
sense on OpenBSD, nor would the -c option.


Index: locale.1
===
RCS file: /cvs/src/usr.bin/locale/locale.1,v
retrieving revision 1.7
diff -u -p -r1.7 locale.1
--- locale.126 Oct 2016 01:00:27 -  1.7
+++ locale.116 Apr 2020 19:04:25 -
@@ -1,6 +1,6 @@
 .\" $OpenBSD: locale.1,v 1.7 2016/10/26 01:00:27 schwarze Exp $
 .\"
-.\" Copyright 2016 Ingo Schwarze 
+.\" Copyright 2016, 2020 Ingo Schwarze 
 .\" Copyright 2013 Stefan Sperling 
 .\"
 .\" Permission to use, copy, modify, and distribute this software for any
@@ -23,7 +23,7 @@
 .Nd character encoding and localization conventions
 .Sh SYNOPSIS
 .Nm locale
-.Op Fl a | Fl m
+.Op Fl a | Fl m | Cm charmap
 .Sh DESCRIPTION
 If the
 .Nm
@@ -31,7 +31,7 @@ utility is invoked without any arguments
 configuration is shown.
 .Pp
 The options are as follows:
-.Bl -tag -width Ds
+.Bl -tag -width charmap
 .It Fl a
 Display a list of supported locales.
 .It Fl m
@@ -39,6 +39,11 @@ Display a list of supported character en
 On
 .Ox ,
 this always returns UTF-8 only.
+.It Cm charmap
+Display the currently selected character encoding.
+On
+.Ox ,
+this returns either US-ASCII or UTF-8.
 .El
 .Pp
 A locale is a set of environment variables telling programs which
Index: locale.c
===
RCS file: /cvs/src/usr.bin/locale/locale.c,v
retrieving revision 1.12
diff -u -p -r1.12 locale.c
--- locale.c5 Feb 2016 12:59:12 -   1.12
+++ locale.c16 Apr 2020 19:04:25 -
@@ -16,6 +16,7 @@
  */
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -169,7 +170,7 @@ show_locales(void)
 static void
 usage(void)
 {
-   fprintf(stderr, "usage: %s [-a | -m]\n", __progname);
+   fprintf(stderr, "usage: %s [-a | -m | charmap]\n", __progname);
exit(1);
 }
 
@@ -203,12 +204,16 @@ main(int argc, char *argv[])
argc -= optind;
argv += optind;
 
-   if (argc != 0 || (aflag && mflag))
+   if (aflag + mflag + argc > 1)
usage();
else if (aflag)
show_locales();
else if (mflag)
printf("UTF-8\n");
+   else if (strcmp(*argv, "charmap") == 0)
+   printf("%s\n", nl_langinfo(CODESET));
+   else
+   usage();
 
return 0;
 }



Re: Update www/groups.html

2020-04-16 Thread Ingo Schwarze
Hi,

clematis wrote on Thu, Apr 16, 2020 at 12:53:19PM +0200:

> Hello,
> Here's a few suggestions to update www/groups.html
> I was checking if all those links were still existing/active/valid.

Thanks, both for doing the checks and providing details about what
exactly you did.

> Does anyone has any questions or concerns removing those? I guess if
> no-one react within another week we can consider this as "groups
> timeout".

Given that you already waited for feedback from the group contacts
to no avail, i simply committed the change.

If somebody speaks up on the list providing a new web address showing
recent activity and a new, working contact for some group, we can
always put the new information in.

I find it easier that way than trying to keep track of uncommitted
patches.

> It's the first time I am looking at something else than ports/ so
> here is what I've done:
> - remove the lines from build/groups.dat
> - run make
> no error , look at groups.html - changes looked good.

In that step, i usually do "make ../groups.html".

> - run a cvs diff on groups.html to read it for sanity check - looked OK.
> - check via browser locally. OK.
> - cvs diff -uNp groups.dat > www_build_groups.dat.diff (see
>   attached and hopefully that's what you meant by "emailing you the
> changes in the form of SUPPORT.TEMPLATE")
> 
> Please let me know if I've missed a step or anything else that should be
> done.

That's fine, i'd do more or less the same in such a case.

Yours,
  Ingo



Re: [patch]: Change kern_unveil to [] array derefs

2020-04-04 Thread Ingo Schwarze
Hi Martin,

Martin Vahlensieck wrote on Sat, Apr 04, 2020 at 07:16:46PM +0200:

> This makes these array derefs consistent with the others in the file.
> Also I believe this is the preferred way to do this.

That depends.  In mandoc, i certainly prefer "pointer + offset"
over "[offest]", arguing that pointer[offest] is defined
as mere syntactic sugar for *(pointer + offset), so to me
personally, "[offest]" feels like a slight detour.

But it looks like a matter of personal taste to me, and i doubt
that OpenBSD in general prefers one or the other.

Both are certainly correct, and i consider both readable.

Either way, this doesn't feel like an issue important enough
to patch one way or the other.

I can't really speak for the kernel, but that shouldn't matter
because this doesn't feel like a question specific to the kernel.

Yours,
  Ingo


> Index: kern_unveil.c
> ===
> RCS file: /cvs/src/sys/kern/kern_unveil.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 kern_unveil.c
> --- kern_unveil.c 22 Mar 2020 20:23:36 -  1.39
> +++ kern_unveil.c 4 Apr 2020 17:08:00 -
> @@ -204,7 +204,7 @@ unveil_destroy(struct process *ps)
>   size_t i;
>  
>   for (i = 0; ps->ps_uvpaths != NULL && i < ps->ps_uvvcount; i++) {
> - struct unveil *uv = ps->ps_uvpaths + i;
> + struct unveil *uv = >ps_uvpaths[i];
>  
>   struct vnode *vp = uv->uv_vp;
>   /* skip any vnodes zapped by unveil_removevnode */
> @@ -244,8 +244,8 @@ unveil_copy(struct process *parent, stru
>   child->ps_uvncount = 0;
>   for (i = 0; parent->ps_uvpaths != NULL && i < parent->ps_uvvcount;
>i++) {
> - struct unveil *from = parent->ps_uvpaths + i;
> - struct unveil *to = child->ps_uvpaths + i;
> + struct unveil *from = >ps_uvpaths[i];
> + struct unveil *to = >ps_uvpaths[i];
>   struct unvname *unvn, *next;
>  
>   to->uv_vp = from->uv_vp;
> @@ -267,8 +267,8 @@ unveil_copy(struct process *parent, stru
>   }
>   child->ps_uvvcount = parent->ps_uvvcount;
>   if (parent->ps_uvpcwd)
> - child->ps_uvpcwd = child->ps_uvpaths +
> - (parent->ps_uvpcwd - parent->ps_uvpaths);
> + child->ps_uvpcwd =
> + >ps_uvpaths[parent->ps_uvpcwd - parent->ps_uvpaths];
>   child->ps_uvdone = parent->ps_uvdone;
>   child->ps_uvshrink = parent->ps_uvshrink;
>  }



Re: [patch] mandoc: Remove argument names from function prototypes

2020-04-03 Thread Ingo Schwarze
Hi Martin,

Martin Vahlensieck wrote on Thu, Apr 02, 2020 at 10:57:04AM +0200:

> I think these are superfluous.

Correct, and it is irritating to have a general style of not using
argument names in prototypes in mandoc, but then a few scattered
names here and there, so i committed your patch.
Thanks for sending it.

In the future, when sending patches, could you please restore your
last name to the From: header?  I consider it disrespectful forcing
developers who want to commit your patches to waste time on the
extra, useless work of searching for your name.

For an extremely simple, probably not Copyright-worthy patch like
this one, it may not matter much, but as soon as you send a patch
that (even potentially) might meet the threshold of originality
with respect to Copyright, it becomes important that the developer
committing the patch mentions the full real name of the person who
sent in the patch in the commit message, such that it is clear who
owns the Copyright on the changes and, by implication, who the
person is putting these changes under the license displayed at the
top of the file.

Thanks,
  Ingo



Re: bug? in getopt(3) + [PATCH] + testcase

2020-03-30 Thread Ingo Schwarze
Hi,

0xef967...@gmail.com wrote on Wed, Mar 25, 2020 at 05:58:22PM +0200:

> To wrap this up and get if off my plate, here is the updated patch,
[...]
> --- getopt-long.c~2020-03-12 02:23:29.028903616 +0200
> +++ getopt-long.c 2020-03-15 23:46:07.988119523 +0200
> @@ -418,15 +418,7 @@
>   }
>  
>   if ((optchar = (int)*place++) == (int)':' ||
> - (optchar == (int)'-' && *place != '\0') ||
>   (oli = strchr(options, optchar)) == NULL) {
> - /*
> -  * If the user specified "-" and  '-' isn't listed in
> -  * options, return -1 (non-option) as per POSIX.
> -  * Otherwise, it is an unknown option character (or ':').
> -  */
> - if (optchar == (int)'-' && *place == '\0')
> - return (-1);
>   if (!*place)
>   ++optind;
>   if (PRINT_ERROR)

After lots of research, code inspection, and testing, and after
some useful exchange of ideas with martijn@, i came to the conclusion
that this patch is indeed the way to go.

I'm now asking for OKs and for other feedback.

See below for a full rationale, and at the end of this mail for
adjustments to the test suite that i intend to commit afterwards.

In addition to the automated tests, the testing i have done includes
a full run of "make build" and "make release" in both base and
xenocara, which all succeeded.


RATIONALE:

Here is the main bug to fix:
   $ OPTS=ax ./obj/getopt-test -a- -x arg <
  OPT(a)ARG(-a-)ARG(-x)ARG(arg)

This raises the question how '-' as an option character should be
handled.  As POSIX does not require that it be handled at all,
the criterion to decide is compatibility with other systems.
For that reason, i tested with:

 * 4.4BSD-Lite2 (compiled on OpenBSD)
 * glibc (on Debian 8 and 9 and compiled from glibc HEAD on OpenBSD),
   where Debian 9 was tested by martijn@
 * FreeBSD (compiled on OpenBSD)
 * NetBSD (compiled on OpenBSD)
 * DragonFly (compiled on OpenBSD)
 * Illumos (compiled on OpenBSD)
 * Oracle Solaris 11 (tested on the OpenCSW cluster)

The tests are intended to be exhaustive regarding all aspects of
the handling of '-' as an option character.


Overview of test results:
-
"yes" means: the '-' option is supported in this way
"arg" means: treated as an argument, not as an option in this case
"err" means: treated as an invalid option in this case
"--"  means: treated the same as isolated "--"
"bug" means: behaviour clearly makes no sense in this case
"oa"  means: (wrongfully) swallowing an option argument
all caps means: probably undesirable, see below as to why

One case that is unsupported on glibc and Solaris and should be
disregarded in comparisons is indicated by [square brackets].
Some cases of unambiguously buggy behaviour that should be
disregarded in comparisons are indicated by {curly braces}.
The case numbers are arbitrary numbers for referencing the
cases in the analysis below.

   Ox4.4  glibc  Fx  Nx   Dx  Illum Sol11 case number
isolated ("-") yes   yes  [ARG]  yes yes  yes  [ARG ARG]  6
trailing ("-a-")   yes   yes   yes   yes yes  yes   yes yes   1
middle ("-a-b")ERR   yes   yes   yes yes  yes   yes yes   4
leading ("--a")ERR  {--}   yes   yes yes  yes  {BUG BUG}  7

not in optstring:
isolated ("-") arg   arg   arg   arg arg  arg   arg arg   -
trailing ("-a-")  {BUG} {BUG}  err   err err  err   err err   3
middle ("-a-b")err  {ARG}  err   err err  err   err err   3
leading ("--a")err  {--}   err   err err  err  {BUG BUG}  7

with mandatory argument after space:
isolated ("-") yes   yes  [ARG]  yes yes  yes  [ARG ARG]  6
trailing ("-a-")   yes   yes   yes   yes yes  yes   yes yes   2

with mandatory argument wthout space:
isolated ("--xy")  ERR  {--}   yes   yes yes  yes  {BUG BUG}  7
trailing ("-a-xy") ERR   yes   yes   yes yes  yes   yes yes   5

with optional argument, present:
isolated ("--xy")  ERR  {--}   yes   yes yes  yes  {BUG BUG}  7
trailing ("-a-xy") ERR   yes   yes   yes yes  yes   yes yes   5

with optional argument, not present:
isolated ("-") yes  {OA}  [ARG]  yes yes {OA}  [ARG ARG]  8
trailing ("-a-")   yes  {OA}   yes   yes yes {OA}  {OA  OA}   8


Test commands
-
isolated ("-") OPTS=abc- ./getopt-test - -c arg
trailing ("-a-")   OPTS=abc- ./getopt-test -a- -c arg
middle ("-a-b")OPTS=abc- ./getopt-test -a-b -c arg
leading ("--a")OPTS=abc- ./getopt-test --a -c arg

not in optstring:
isolated ("-") OPTS=abc ./getopt-test - -c arg
trailing ("-a-")   OPTS=abc ./getopt-test -a- -c arg
middle ("-a-b")OPTS=abc ./getopt-test -a-b -c arg
leading ("--a")OPTS=abc ./getopt-test --a -c arg

with mandatory argument after space:
isolated ("-") OPTS=abc-: ./getopt-test - -- -c arg
trailing ("-a-")   OPTS=abc-: ./getopt-test -a- -- -c arg

with mandatory argument wthout space:
isolated ("--xy")  

Re: [patch] Tweak libssl manpages

2020-03-30 Thread Ingo Schwarze
Hi Martin,

Martin wrote on Sun, Mar 29, 2020 at 10:22:15PM +0200:

> It seems these are just a coded form for no return value,
> unless this is some libssl slang I am not aware of.

I don't think "does not provide diagnostic information" has any
special meaning in libssl.  Also, the word "diagnostic" doesn't
appear anywhere else in the libssl manuals (well, except for a
weird BUGS entry in SSL_CTX_set_cert_verify_callback(3)).

I checked the source code.  Among these functions, SSL_free(3)
is the only one doing substantial amounts of non-trivial work.
It is possible that something in there might sometimes call
ERR_put_error(3).  But even if so, the sentence deleted claimed
the opposite, if interpreted as talking about the error stack.
Either way, saying that something does *not* call ERR_put_error(3)
isn't useful even when it happens to be true.

The other functions touched here clearly cannot fail, do no
error handling, and don't need any such remark.

So yes, i think this was merely a very confusing way to express
the statement "these are void function".

Consequently, i committed your patch.

Thanks,
  Ingo


> Index: SSL_CTX_set_client_CA_list.3
> ===
> RCS file: /cvs/src/lib/libssl/man/SSL_CTX_set_client_CA_list.3,v
> retrieving revision 1.5
> diff -u -p -r1.5 SSL_CTX_set_client_CA_list.3
> --- SSL_CTX_set_client_CA_list.3  27 Mar 2018 17:35:50 -  1.5
> +++ SSL_CTX_set_client_CA_list.3  29 Mar 2020 20:18:28 -
> @@ -143,11 +143,6 @@ or
>  .Pp
>  These functions are only useful for TLS/SSL servers.
>  .Sh RETURN VALUES
> -.Fn SSL_CTX_set_client_CA_list
> -and
> -.Fn SSL_set_client_CA_list
> -do not return diagnostic information.
> -.Pp
>  .Fn SSL_CTX_add_client_CA
>  and
>  .Fn SSL_add_client_CA
> Index: SSL_CTX_set_quiet_shutdown.3
> ===
> RCS file: /cvs/src/lib/libssl/man/SSL_CTX_set_quiet_shutdown.3,v
> retrieving revision 1.5
> diff -u -p -r1.5 SSL_CTX_set_quiet_shutdown.3
> --- SSL_CTX_set_quiet_shutdown.3  8 Jun 2019 15:25:43 -   1.5
> +++ SSL_CTX_set_quiet_shutdown.3  29 Mar 2020 20:18:28 -
> @@ -144,11 +144,6 @@ This behaviour violates the TLS standard
>  .Pp
>  The default is normal shutdown behaviour as described by the TLS standard.
>  .Sh RETURN VALUES
> -.Fn SSL_CTX_set_quiet_shutdown
> -and
> -.Fn SSL_set_quiet_shutdown
> -do not return diagnostic information.
> -.Pp
>  .Fn SSL_CTX_get_quiet_shutdown
>  and
>  .Fn SSL_get_quiet_shutdown
> Index: SSL_CTX_set_tmp_dh_callback.3
> ===
> RCS file: /cvs/src/lib/libssl/man/SSL_CTX_set_tmp_dh_callback.3,v
> retrieving revision 1.7
> diff -u -p -r1.7 SSL_CTX_set_tmp_dh_callback.3
> --- SSL_CTX_set_tmp_dh_callback.3 27 Mar 2018 17:35:50 -  1.7
> +++ SSL_CTX_set_tmp_dh_callback.3 29 Mar 2020 20:18:29 -
> @@ -175,11 +175,6 @@ and
>  .Fa is_export
>  and simply supply at least 2048-bit parameters in the callback.
>  .Sh RETURN VALUES
> -.Fn SSL_CTX_set_tmp_dh_callback
> -and
> -.Fn SSL_set_tmp_dh_callback
> -do not return diagnostic output.
> -.Pp
>  .Fn SSL_CTX_set_tmp_dh
>  and
>  .Fn SSL_set_tmp_dh
> Index: SSL_free.3
> ===
> RCS file: /cvs/src/lib/libssl/man/SSL_free.3,v
> retrieving revision 1.4
> diff -u -p -r1.4 SSL_free.3
> --- SSL_free.327 Mar 2018 17:35:50 -  1.4
> +++ SSL_free.329 Mar 2020 20:18:29 -
> @@ -103,9 +103,6 @@ was not used to set the
>  .Vt SSL_SENT_SHUTDOWN
>  state, the session will also be removed from the session cache as required by
>  RFC2246.
> -.Sh RETURN VALUES
> -.Fn SSL_free
> -does not provide diagnostic information.
>  .Sh SEE ALSO
>  .Xr ssl 3 ,
>  .Xr SSL_clear 3 ,
> Index: SSL_set_shutdown.3
> ===
> RCS file: /cvs/src/lib/libssl/man/SSL_set_shutdown.3,v
> retrieving revision 1.4
> diff -u -p -r1.4 SSL_set_shutdown.3
> --- SSL_set_shutdown.327 Mar 2018 17:35:50 -  1.4
> +++ SSL_set_shutdown.329 Mar 2020 20:18:29 -
> @@ -122,9 +122,6 @@ or
>  .Fn SSL_set_shutdown
>  itself.
>  .Sh RETURN VALUES
> -.Fn SSL_set_shutdown
> -does not return diagnostic information.
> -.Pp
>  .Fn SSL_get_shutdown
>  returns the current setting.
>  .Sh SEE ALSO



Re: [patch] Remove "do not return a value" from libcrypto/libssl manpages

2020-03-29 Thread Ingo Schwarze
Hi Martin,

Martin Vahlensieck wrote on Sun, Mar 29, 2020 at 01:51:58AM +0100:

> I found some more.

Thanks, committed, also including in lh_stats(3).
  Ingo


> Index: libcrypto/man/RC4.3
> ===
> RCS file: /cvs/src/lib/libcrypto/man/RC4.3,v
> retrieving revision 1.7
> diff -u -p -r1.7 RC4.3
> --- libcrypto/man/RC4.3   6 Jun 2019 01:06:59 -   1.7
> +++ libcrypto/man/RC4.3   29 Mar 2020 00:48:17 -
> @@ -112,11 +112,6 @@ yield a continuous key stream.
>  Since RC4 is a stream cipher (the input is XOR'ed with a pseudo-random
>  key stream to produce the output), decryption uses the same function
>  calls as encryption.
> -.Sh RETURN VALUES
> -.Fn RC4_set_key
> -and
> -.Fn RC4
> -do not return values.
>  .Sh SEE ALSO
>  .Xr blowfish 3 ,
>  .Xr EVP_EncryptInit 3 ,
> Index: libcrypto/man/X509_STORE_CTX_set_verify_cb.3
> ===
> RCS file: /cvs/src/lib/libcrypto/man/X509_STORE_CTX_set_verify_cb.3,v
> retrieving revision 1.4
> diff -u -p -r1.4 X509_STORE_CTX_set_verify_cb.3
> --- libcrypto/man/X509_STORE_CTX_set_verify_cb.3  22 Mar 2018 17:38:08 
> -  1.4
> +++ libcrypto/man/X509_STORE_CTX_set_verify_cb.3  29 Mar 2020 00:48:17 
> -
> @@ -108,9 +108,6 @@ In some cases (such as S/MIME verificati
>  structure is created and destroyed internally and the only way to set a
>  custom verification callback is by inheriting it from the associated
>  .Vt X509_STORE .
> -.Sh RETURN VALUES
> -.Fn X509_STORE_CTX_set_verify_cb
> -does not return a value.
>  .Sh EXAMPLES
>  Default callback operation:
>  .Bd -literal
> Index: libcrypto/man/X509_STORE_set_verify_cb_func.3
> ===
> RCS file: /cvs/src/lib/libcrypto/man/X509_STORE_set_verify_cb_func.3,v
> retrieving revision 1.8
> diff -u -p -r1.8 X509_STORE_set_verify_cb_func.3
> --- libcrypto/man/X509_STORE_set_verify_cb_func.3 27 Mar 2018 17:35:50 
> -  1.8
> +++ libcrypto/man/X509_STORE_set_verify_cb_func.3 29 Mar 2020 00:48:17 
> -
> @@ -86,11 +86,6 @@ structure when it is initialized.
>  This can be used to set the verification callback when the
>  .Vt X509_STORE_CTX
>  is otherwise inaccessible (for example during S/MIME verification).
> -.Sh RETURN VALUES
> -.Fn X509_STORE_set_verify_cb
> -and
> -.Fn X509_STORE_set_verify_cb_func
> -do not return a value.
>  .Sh SEE ALSO
>  .Xr X509_STORE_CTX_set_verify_cb 3 ,
>  .Xr X509_STORE_new 3
> Index: libssl/man/SSL_set_verify_result.3
> ===
> RCS file: /cvs/src/lib/libssl/man/SSL_set_verify_result.3,v
> retrieving revision 1.4
> diff -u -p -r1.4 SSL_set_verify_result.3
> --- libssl/man/SSL_set_verify_result.327 Mar 2018 17:35:50 -  
> 1.4
> +++ libssl/man/SSL_set_verify_result.329 Mar 2020 00:48:17 -
> @@ -79,9 +79,6 @@ The valid codes for
>  .Fa verify_result
>  are documented in
>  .Xr openssl 1 .
> -.Sh RETURN VALUES
> -.Fn SSL_set_verify_result
> -does not provide a return value.
>  .Sh SEE ALSO
>  .Xr openssl 1 ,
>  .Xr ssl 3 ,



Re: [patch] ERR_print_errors.3

2020-03-28 Thread Ingo Schwarze
Hi Martin,

thanks for reporting the issue in the manual page.

Martin Vahlensieck wrote on Sat, Mar 28, 2020 at 09:06:54PM +0100:

> Unless I miss something ERR_print_errors_cb returns no value as well.

Actually, i committed about the opposite, for the reasons explained
in the commit message.  These were stragglers; i already deleted
most such sentences long ago.

The committed patch follows...

Yours,
  Ingo


CVSROOT:/cvs
Module name:src
Changes by: schwa...@cvs.openbsd.org2020/03/28 16:40:58

Modified files:
lib/libcrypto/man: ERR_print_errors.3 ERR_remove_state.3 
   lh_new.3 

Log message:
Be concise: do not say that void functions return no values, that's obvious.
Useless text reported by Martin Vahlensieck (academicsolutions.ch) on tech@.


Index: ERR_print_errors.3
===
RCS file: /cvs/src/lib/libcrypto/man/ERR_print_errors.3,v
retrieving revision 1.7
diff -u -r1.7 ERR_print_errors.3
--- ERR_print_errors.3  27 Mar 2018 17:35:50 -  1.7
+++ ERR_print_errors.3  28 Mar 2020 22:36:47 -
@@ -103,11 +103,6 @@
 .Pp
 If there is no text string registered for the given error code, the
 error string will contain the numeric code.
-.Sh RETURN VALUES
-.Fn ERR_print_errors
-and
-.Fn ERR_print_errors_fp
-return no values.
 .Sh SEE ALSO
 .Xr ERR 3 ,
 .Xr ERR_error_string 3 ,
Index: ERR_remove_state.3
===
RCS file: /cvs/src/lib/libcrypto/man/ERR_remove_state.3,v
retrieving revision 1.6
diff -u -r1.6 ERR_remove_state.3
--- ERR_remove_state.3  27 Mar 2018 17:35:50 -  1.6
+++ ERR_remove_state.3  28 Mar 2020 22:36:48 -
@@ -92,11 +92,6 @@
 .Fn ERR_remove_state
 is equivalent to
 .Fn ERR_remove_thread_state NULL .
-.Sh RETURN VALUES
-.Fn ERR_remove_thread_state
-and
-.Fn ERR_remove_state
-return no value.
 .Sh SEE ALSO
 .Xr ERR 3
 .Sh HISTORY
Index: lh_new.3
===
RCS file: /cvs/src/lib/libcrypto/man/lh_new.3,v
retrieving revision 1.6
diff -u -r1.6 lh_new.3
--- lh_new.310 Jun 2019 09:49:48 -  1.6
+++ lh_new.328 Mar 2020 22:36:48 -
@@ -402,12 +402,6 @@
 .Pp
 .Fn lh__error
 returns 1 if an error occurred in the last operation, or 0 otherwise.
-.Pp
-.Fn lh__free ,
-.Fn lh__doall ,
-and
-.Fn lh__doall_arg
-return no values.
 .Sh NOTES
 The various LHASH macros and callback types exist to make it possible to
 write type-checked code without resorting to function-prototype casting



Re: find.1: Markup primaries with Fl not Cm for easier tags

2020-03-20 Thread Ingo Schwarze
Hi Klemens,

Klemens Nanni wrote on Sat, Mar 21, 2020 at 12:49:20AM +0100:
> On Fri, Mar 20, 2020 at 04:53:19PM +0100, Ingo Schwarze wrote:

>> I don't feel very strongly about that, but i think .Cm does make
>> slightly more sense for primaries than .Fl (and .Ic is probably
>> acceptable, too, even though .Cm might be better because these are
>> command line arguments, not stand-alone commands).

> It does, however because the resulting effect is usually the same,
> I see no problem in using `Fl' for primaries.

Yes, it wouldn't be a big deal.  It might make a difference in
unusual situations though, for example if some website would
customize .Fl and .Cm differently in mandoc.css.

>> I think that is reasonable, yes.  I think it makes sense to consider
>> tags as words, i.e. strings that usually consist of letters and may
>> sometimes contain digits, but never contain whitespace and usually
>> do not start with punctuation characters.  I'm not sure this rule of
>> thumb can be made very strict, but i think it is possible to polish
>> this by handling a small number of common cases.  For example,
>> automatic tagging in man(7) already discards leading whitespace, 
>> some escape sequences, and leading dashes from tags.  Automatic
>> tagging in mdoc(7) already discards leading zero-width spaces
>> and leading backslashes.  I think it would make sense to also
>> skip leading dashes here, see the patch below.
>> 
>> Do you agree with that?

> Yes, I very much do.  Given the fact man(7) already works that way,
> I'm happy to see mdoc(7) follow;  it seems like the right approach, my
> attempt seems more of a workaround for special cases like find(1).
> 
> OK kn

Committed, thanks for pointing out the issue and for the feedback
on the patch.

Yours,
  Ingo



Re: find.1: Markup primaries with Fl not Cm for easier tags

2020-03-20 Thread Ingo Schwarze
Hi Klemens,

Klemens Nanni wrote on Fri, Mar 20, 2020 at 12:12:39AM +0100:

> In both command line usage and manual output format, find's options
> and primaries behave the same,

Not really.  In the POSIX sense, options are indeed options while
primaries are arguments.  That has implications, for example that
all options must precede all primaries.

> but their mdoc(7) markup is different

I don't feel very strongly about that, but i think .Cm does make
slightly more sense for primaries than .Fl (and .Ic is probably
acceptable, too, even though .Cm might be better because these are
command line arguments, not stand-alone commands).

> and therefore causes different tag names:
> 
> -x (option) can be looked up with ":tx" in the manual pager,
> whereas -amin (primary) requires ":t-amin" including the dash.
> 
> I'd like primaries to behave the same like options when it comes to
> tags in manuals, is that a reasonable expectation?

I think that is reasonable, yes.  I think it makes sense to consider
tags as words, i.e. strings that usually consist of letters and may
sometimes contain digits, but never contain whitespace and usually
do not start with punctuation characters.  I'm not sure this rule of
thumb can be made very strict, but i think it is possible to polish
this by handling a small number of common cases.  For example,
automatic tagging in man(7) already discards leading whitespace, 
some escape sequences, and leading dashes from tags.  Automatic
tagging in mdoc(7) already discards leading zero-width spaces
and leading backslashes.  I think it would make sense to also
skip leading dashes here, see the patch below.

Do you agree with that?

Yours,
  Ingo


Index: tag.c
===
RCS file: /cvs/src/usr.bin/mandoc/tag.c,v
retrieving revision 1.29
diff -u -p -r1.29 tag.c
--- tag.c   13 Mar 2020 16:14:14 -  1.29
+++ tag.c   20 Mar 2020 15:50:33 -
@@ -87,8 +87,24 @@ tag_put(const char *s, int prio, struct 
if (n->child == NULL || n->child->type != ROFFT_TEXT)
return;
s = n->child->string;
-   if (s[0] == '\\' && (s[1] == '&' || s[1] == 'e'))
-   s += 2;
+   switch (s[0]) {
+   case '-':
+   s++;
+   break;
+   case '\\':
+   switch (s[1]) {
+   case '&':
+   case '-':
+   case 'e':
+   s += 2;
+   break;
+   default:
+   break;
+   }
+   break;
+   default:
+   break;
+   }
}
 
/*



Re: bt.5: Fix time() description

2020-03-18 Thread Ingo Schwarze
Hi Martin, hi Klemens,

Martin Pieuchot wrote on Wed, Mar 18, 2020 at 09:06:24PM +0100:
> On 18/03/20(Wed) 20:45, Klemens Nanni wrote:

>> It takes a format string, e.g.
>> 
>>  syscall:sysctl:entry {
>>  time("%+\n")
>>  }

I can't comment on the content of bt(5).

> This is indeed an improvement, thanks!  I don't know how to point that
> 'format' or 'timefmt' is the same as describe in strftime(3).

One way to do that might be similar to this:

  .It Fn time format
  print timestamps using
  .Xr strftime 3
  with the
  .Fa format
  argument

the point being that the placeholder "format" is the same one as
the one used in the SYNOPSIS of the strftime(3) page (if i understand
correctly what is going on here).

Your call how exactly you want to do it.

> Does that mean strftime(3) should appear in SEE ALSO?

As a rough rule of thumb (not a strict prescription), put an .Xr
into the text when the reference helps understanding of that specific
point in the text, and put an .Xr into SEE ALSO when the reference
helps understanding of the page as a whole.

So here, a reference below SEE ALSO might not be needed, and Klemens
already included one in the text.

> So unless any man guru jump in, to improve the submission, just go ahead
> with my ok :o) 

Certainly no objection either way.

Yours,
  Ingo

>> Index: bt.5
>> ===
>> RCS file: /cvs/src/usr.sbin/btrace/bt.5,v
>> retrieving revision 1.2
>> diff -u -p -r1.2 bt.5
>> --- bt.5 27 Jan 2020 14:15:25 -  1.2
>> +++ bt.5 18 Mar 2020 19:42:16 -
>> @@ -117,8 +117,9 @@ Print all (key, value) pairs from map
>>  .It Fn printf "fmt" ...
>>  print formatted string
>>  .Va fmt
>> -.It Fn time
>> -print formatted time
>> +.It Fn time timefmt
>> +print timestamps using
>> +.Xr strftime 3
>>  .El
>>  .Sh SEE ALSO
>>  .Xr awk 1 ,
>> 



Re: openssl.1: Tag command names

2020-02-27 Thread Ingo Schwarze
Hi Klemens,

Ingo Schwarze wrote on Tue, Feb 18, 2020 at 04:30:53PM +0100:

> While i don't strongly object to the patch, it might be worth holding
> off a bit on manually tagging of .Sh given that even automatic
> tagging isn't done for that macro yet.  Which would mean postponing
> this patch until automatic tagging for .Sh has been decided and
> implemented.
> 
> Given that this is essentially manual tagging of .Sh, if you decide
> to put this in for the time being until automatic tagging of .Sh
> may arrive, i agree with the two developers who said that
> 
>   .Tg asn1parse
>   .Sh ASN1PARSE
> 
> would feel more natural,

So, you did put this in, which is of course fine with me.

> even though for now, that puts the tag two lines early in
> terminal output.  But at the latest when automatic tagging for
> .Sh gets implemented, i will certainly fix that offset.

I just fixed that two-line offset, mainly because it came almost
for free as a by-product of starting to systematically improve HTML
output, in particular starting to get rid of  elements where
they are not really needed.

I'm appending the committed patch below such that people who want
can easily review how the HTML output now looks like for explicitly
tagged section headers (in the regress/mdoc/Sh/tag.out_html file
contained in the patch).

I also installed the new code on man.openbsd.org:

  https://man.openbsd.org/openssl#s_client
  https://man.openbsd.org/route#add

The following works unchanged because we already had automatic
tagging for section and subsection headers in HTML output:

  https://man.openbsd.org/openssl#COMMON_NOTATION

In terminal output, i did not implement automatic tagging for section
and subsection headers yet.  But given the current state of NODE_ID
support, that's now trivial to implement if people would like to see
it.  The benefit isn't huge, but it might add a bit of consistency.
For example, right now, in ksh(1), these work:

  /^   Quoting
  /^FILES

Adding automatic tagging for .Sh and .Ss would make these two
do just the same:

  :tQuoting
  :tFILES

Yours,
  Ingo


Log Message:
---
Fully support explicit tagging of .Sh and .Ss.
This fixes the offset of two lines in terminal output
and this improves HTML output by putting the id= attribute
and  element into the respective  or  element rather
than writing an additional  element.

To that end, introduce node flags NODE_ID (to make the node a link
target, for example by writing an HTML id= attribute or by calling
tag_put()) and NODE_HREF (to make the node a link source, used only
in HTML output, used only to write an  element).

In particular:
* In the validator, generalize the concept of the "next node"
such that it also works before .Sh and .Ss.
* If the first argument of .Tg is empty, don't forget to complain
if there are additional arguments, which will be ignored.
* In the terminal formatter, support writing of explicit tags
for all kinds of nodes, not just for .Tg.
* In deroff(), allow nodes to have an explicit string representation
even when they aren't text nodes.  Use this for explicitly tagged
section headers.  Suprisingly, this is sufficient to make HTML
output work, without explicit code changes in the HTML formatter.
* In syntax tree output, display NODE_ID and NODE_HREF.

Modified Files:
--
mandoc:
mdoc_term.c
mdoc_validate.c
roff.c
roff.h
tree.c
mandoc/regress/mdoc/Sh:
Makefile

Added Files:
---
mandoc/regress/mdoc/Sh:
tag.in
tag.out_ascii
tag.out_html
tag.out_markdown

Revision Data
-
Index: roff.h
===
RCS file: /home/cvs/mandoc/mandoc/roff.h,v
retrieving revision 1.71
retrieving revision 1.72
diff -Lroff.h -Lroff.h -u -p -r1.71 -r1.72
--- roff.h
+++ roff.h
@@ -522,6 +522,8 @@ struct  roff_node {
 #defineNODE_NOFILL  (1 << 8)  /* Fill mode switched off. */
 #defineNODE_NOSRC   (1 << 9)  /* Generated node, not in input 
file. */
 #defineNODE_NOPRT   (1 << 10) /* Shall not print anything. */
+#defineNODE_ID  (1 << 11) /* Target for deep linking. */
+#defineNODE_HREF(1 << 12) /* Link to another place in this 
page. */
int   prev_font; /* Before entering this node. */
int   aux; /* Decoded node data, type-dependent. */
enum roff_tok tok; /* Request or macro ID. */
Index: tree.c
===
RCS file: /home/cvs/mandoc/mandoc/tree.c,v
retrieving revision 1.85
retrieving revision 1.86
diff -Ltree.c -Ltree.c -u -p -r1.85 -r1.86
--- tree.c
+++ tree.c
@@ -199,6 +199,13 @@ print_mdoc(const struct roff_node *n, in
putchar(')');
if (n->

Re: openssl.1: Tag command names

2020-02-26 Thread Ingo Schwarze
Hi Klemens,

Ingo Schwarze wrote on Tue, Feb 18, 2020 at 04:30:53PM +0100:
> Klemens Nanni wrote on Mon, Feb 17, 2020 at 05:19:27PM +0100:

>> Patch was done with a VIM macro by adding a new line after each `.Sh'
>> line with the respective name but lowercased, so no typos in the added
>> strings.

> That placement triggers an issue that already existed before .Tg arrived.
> In some places in the formatters, nodes are iterated (mainly backwards)
> to see whether there are any other nodes in the same parent, mostly
> for spacing purposes, but sometimes also for other purposes.
> Some nodes ought to be transparent to such iterations.
> 
> Here are two examples; note the bogus blank line in the output after
> "DESCRIPTION" and before "text":
> 
>   $ mandoc -mdoc | ul
> .Sh DESCRIPTION
> .Sm off
> .Bl -tag -width Ds
> .It Cm text : Ar text
> text
> .El
> 
>   $ mandoc -mdoc | ul
> .Sh DESCRIPTION
> .ft B
> .Bd -unfilled
> text
> .Ed
> 
> Yes, this mostly happens with code of dubious style, but the new
> .Tg macro can trigger the issue, too.  With continuously improving
> low-level roff(7) support, more such nodes that need to be transparent
> in this way may appear in the future, so the issue is bound to
> become worse over time, and at least half a dozen such nodes already
> exist.  Hence i'm working on a general solution, but that doesn't
> need to delay any commit, of course.

So, i finally committed a patch for this class of issues.  That
patch turned out to be of somewhat scary size (+318 -252 lines
touching 10 files), but even though i wasn't aware of it, all that
was already broken before .Tg existed, so it needed fixing anyway.
A patch of this size carries a certain risk of regressions, but
given the nature of the changes, if there are any regressions, they
will most likely be minor formatting glitches.  Also, a significant
fraction of the changes is half-mechanical, except that each
individual place needed careful consideration whether or not it had
to be changed (many superficially similar places should not be
changed).  Anyway, the significant amount of automated tests that
i committed with the patch ought to further reduce the risk that
anything goes wrong in some major way.

I committed it right away because this is not the kind of patch
that anybody would want to review.  Even Kristaps would balk at
that, i guess.

Yours,
  Ingo



Re: Update en_US.UTF-8 to Unicode 12.1

2020-02-18 Thread Ingo Schwarze
Hi Andrew,

ironically, the patch did not apply for me because you sent it with

  Content-Type: text/plain; charset=iso-8859-1

so when i saved it to disk, it remained in ISO-Latin-1.
In the license comment you are changing, there is a U+00A9 COPYRIGHT
SIGN encoded as UTF-8.  I suggest you change that to "(c)" in both
files before commit to avoid similar issues in the future.


Andrew Fresh wrote on Sat, Feb 15, 2020 at 11:26:39AM -0800:

> This is two patches, the first updates to the latest Unicode licence

I confirm that the license change is correct.
Nice to see people actually making a license better,
in this case by dropping an awkward modification clause.

> (and adds a comment with the version of unicode this file contains)

That makes sense to me.

> https://www.unicode.org/license.html
> 
> The second is the output of running the script with perl 5.30.1, which I
> usually remember to do earlier.

I compared to the output of the script generated on my machine, and
i looked through the changes and found nothing suspicious.  There
is one new rare latin letter (LATIN CAPITAL LETTER S WITH HOOK),
and some other latin letters that seem even more unusual.  The rest
are mostly new characters and new character properties for non-latin
scripts, and of course lots of new symbols and pictographs, including
a whole block of chess symbols.  There are also some changes that
may plausibly be bugfixes, like an Ethiopic special mark no longer
being iswalpha(3) and one Lepcha letter going in just the opposite
direction.

The most notable changes may include:

 * Surrogates are no longer considered control characters.
   I do not expect fallout from that change because they should
   not appear in UTF-8 text in the first place.
 * There are whole new blocks of Georgian, Tamil, Rohingya, Dogra,
   Gondi, and of some historical scripts.
 
> Comments, OK?

A very minor detail: it appears you only applied parts of the license
change patch before running the script, i got the difference shown
below relative to what you sent.

OK schwarze@ either way, please go ahead!
  Ingo


--- en_US.UTF-8.src.bentley Sun Feb 16 23:39:19 2020
+++ en_US.UTF-8.src Mon Feb 17 07:47:55 2020
@@ -1,11 +1,11 @@
-/* $OpenBSD: en_US.UTF-8.src,v 1.9 2019/02/22 16:35:16 afresh1 Exp $   
*/
+/* $OpenBSD$   */
 
 /*
  * COPYRIGHT AND PERMISSION NOTICE
  *
- * Copyright © 1991-2015 Unicode, Inc. All rights reserved.
+ * Copyright (c) 1991-2019 Unicode, Inc. All rights reserved.
  * Distributed under the Terms of Use in
- * http://www.unicode.org/copyright.html.
+ * https://www.unicode.org/copyright.html.
  *
  * Permission is hereby granted, free of charge, to any person obtaining
  * a copy of the Unicode data files and any associated documentation
@@ -41,7 +41,6 @@
 VARIABLECODESET=UTF-8
 
 /* Unicode Version 12.1.0 */
-
 
 /*
  * U+ - U+007F : Basic Latin



Re: openssl.1: Tag command names

2020-02-18 Thread Ingo Schwarze
Hi,

Steffen Nurpmeso wrote on Tue, Feb 18, 2020 at 04:52:48PM +0100:

> i just want to add that there is still the mdocmx mdoc macro
> extension available, and is working fine for more than half
> a decade.  I have not ported that to groff 1.22.4, but it is
> available for groff 1.22.3.  It can much more than .Tg, of course,

Yes, and i said half a decade ago that it was ill-designed
and the .Mx user interface is bloated.  It deserves to die.

Would you please delete me from the AUTHORS section in

  https://www.sdaoden.eu/code-mdocmx.html

That was mere fooling around with immature ideas, and i clearly
said back then that what we ended up with in 2014 was not yet good
enough to consider implementing it, and i said that the time clearly
wasn't ripe to do a proper design.

Yours,
  Ingo



Re: openssl.1: Tag command names

2020-02-18 Thread Ingo Schwarze
Hi Klemens,

Klemens Nanni wrote on Mon, Feb 17, 2020 at 05:19:27PM +0100:

> I'd like to commit this soon, it allows me to jump to the command I'm
> looking for, e.g. ":tx509" shows me the synopsis right away.
> 
> FWIW, some Linux distributions ship with separate manuals, e.g. x509(1SSL).

Yes, that is how OpenSSL ships these manuals.
Long before the LibreSSL project started, jmc@ already did the work
of combining all the OpenSSL manual pages for subcommands into
one single page, openssl(1).

I think the rationale was that openssl(1) is one single command.
We don't have separate manual pages for e.g. shell builtin
commands either, like alias, cd, readonly, typeset, ...
Besides, openssl(1) is mainly a testing tool, so that one in
particular has no business squatting on lots of namespace.

> Patch was done with a VIM macro by adding a new line after each `.Sh'
> line with the respective name but lowercased, so no typos in the added
> strings.

That placement triggers an issue that already existed before .Tg arrived.
In some places in the formatters, nodes are iterated (mainly backwards)
to see whether there are any other nodes in the same parent, mostly
for spacing purposes, but sometimes also for other purposes.
Some nodes ought to be transparent to such iterations.

Here are two examples; note the bogus blank line in the output after
"DESCRIPTION" and before "text":

  $ mandoc -mdoc | ul
.Sh DESCRIPTION
.Sm off
.Bl -tag -width Ds
.It Cm text : Ar text
text
.El

  $ mandoc -mdoc | ul
.Sh DESCRIPTION
.ft B
.Bd -unfilled
text
.Ed

Yes, this mostly happens with code of dubious style, but the new
.Tg macro can trigger the issue, too.  With continuously improving
low-level roff(7) support, more such nodes that need to be transparent
in this way may appear in the future, so the issue is bound to
become worse over time, and at least half a dozen such nodes already
exist.  Hence i'm working on a general solution, but that doesn't
need to delay any commit, of course.

> Specifying it is required since the markup following the `.Tg' markup
> always starts with "openssl";  the tag must not include it (`.Tg'
> accepts at most one word anyway).

If we follow through on the idea of abandoning the rule that top
level section headers must be all caps in the source code and instead
do that formatting as appropriate for each formatter (groff has
already gone that way, but we haven't fully concluded the discussion
yet whether and how exactly we want to follow), the tagging in all
these cases will become automatic.

Besides, there may be value in waiting a bit before applying .Tg
to manual pages of projects where the -portable version is particularly
important, in particular OpenSSH and LibreSSL.  Right now, groff(1)
reacts as follows (still formatting the page correctly, of course):

  troff: openssl.1:204: warning: macro 'Tg' not defined

I certainly intend to fix that warning in groff in the near future,
but shipping pages in OpenSSH or LibreSSL -portable that throw
warnings with the latest mandoc release and even with groff-current
compiled straight from git might not yet be ideal.


Theo Buehler wrote:

> Without tags I currently achieve pretty much the same by doing "/^X509"
> or "/^S_CLIENT" etc. However, having to know the special trick for each
> page (if there is one) is annoying, so I think this is an improvement.

That's exactly why i implemented no automatic tagging for .Sh in the
past: it doesn't provide much practical benefit.  Besides, the rule
that .Sh arguments must be all caps further diminshed the value of
automatically tagging .Sh.  If that rule gets lifted, automatic tagging
of .Sh becomes more useful (and almost free in terms of implementation
effort).

While i don't strongly object to the patch, it might be worth holding
off a bit on manually tagging of .Sh given that even automatic
tagging isn't done for that macro yet.  Which would mean postponing
this patch until automatic tagging for .Sh has been decided and
implemented.

Given that this is essentially manual tagging of .Sh, if you decide
to put this in for the time being until automatic tagging of .Sh
may arrive, i agree with the two developers who said that

  .Tg asn1parse
  .Sh ASN1PARSE

would feel more natural, even though for now, that puts the tag two
lines early in terminal output.  But at the latest when automatic
tagging for .Sh gets implemented, i will certainly fix that offset.

Yours,
  Ingo



Re: bgpd.conf.5: Tag groups

2020-02-16 Thread Ingo Schwarze
Hi Klemens,

Klemens Nanni wrote on Sun, Feb 16, 2020 at 09:32:52PM +0100:
> On Sun, Feb 16, 2020 at 09:25:51PM +0100, Klemens Nanni wrote:

>> Going through the example with bgpd.conf(5) side by side, jumping to the
>> "group" tag shows
>> 
>>  group descr  Neighbors in this group will be matched.
>>  AS as-number
>>   Neighbors with this AS will be matched.
>>  ...
>> 
>> which is somewhere in the manual that assumes knowing what groups are.
>> mdoc's automatic tagging failed here, so markup `Ic group' in proper
>> section:
>> 
>>  NEIGHBORS AND GROUPS
>>   bgpd(8) establishes TCP connections to other BGP speakers called
>>   neighbors.  A neighbor and its properties are specified by a 
>> neighbor
>>   section:
>> 
>>   neighbor 10.0.0.2 {
>>   remote-as 65002
>>   descr "a neighbor"
>>   }
>> 
>>   Neighbors placed within a group section inherit the properties 
>> common to
>>   that group:
>>   ...
>> 
>> This makes typing ":tgroup" jump to that very last sentence.

> Same for `neighbor' in the very same section as well.
> 
> OK?

Sure.  I'm not convinced you even need OKs for such simple tagging
corrections.

By the way, just

  .Tg
  .Ic neighbor

  .Tg
  .Ic group

is sufficient, the .Tg macro defaults to the first argument of the first
macro on the following line, if any.

Yours,
  Ingo


> Index: bgpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v
> retrieving revision 1.200
> diff -u -p -r1.200 bgpd.conf.5
> --- bgpd.conf.5   9 Feb 2020 14:21:26 -   1.200
> +++ bgpd.conf.5   16 Feb 2020 20:31:46 -
> @@ -683,6 +683,7 @@ can be chosen by the local operator.
>  establishes TCP connections to other BGP speakers called
>  .Em neighbors .
>  A neighbor and its properties are specified by a
> +.Tg neighbor
>  .Ic neighbor
>  section:
>  .Bd -literal -offset indent
> @@ -693,6 +694,7 @@ neighbor 10.0.0.2 {
>  .Ed
>  .Pp
>  Neighbors placed within a
> +.Tg group
>  .Ic group
>  section inherit the properties common to that group:
>  .Bd -literal -offset indent



Re: dd(1) wording and style

2020-02-15 Thread Ingo Schwarze
Hi Jan,

Jan Stary wrote on Sat, Feb 15, 2020 at 10:51:28AM +0100:
> On Feb 14 17:37:27, schwa...@usta.de wrote:
>> Jason McIntyre wrote on Fri, Feb 14, 2020 at 07:28:59AM +:
>>> On Thu, Feb 13, 2020 at 11:25:07PM +0100, Jan Stary wrote:

 -.It Cm seek= Ns Ar n
 +.It Cm seek Ns = Ns Ar n
  Seek
  .Ar n
  blocks from the beginning of the output before copying.
 -On non-tape devices, an
 -.Xr lseek 2
 -operation is used.
 -Otherwise, existing blocks are read and the data discarded.
 -If the user does not have read permission for the tape, it is positioned
 -using the tape
 +On a tape device, existing blocks are read and the data discarded;
 +if the user does not have read permission for the tape,
 +it is positioned using the tape
  .Xr ioctl 2
  function calls.
 +On all other devices devices, an
 +.Xr lseek 2
 +operation is used.
  If the seek operation is past the end of file, space from the current
  end of file to the specified offset is filled with blocks of NUL bytes.

>>> i think this change is ok. however i think non-tape is clearer than "on
>>> all other devices". it's not a biggie, but i think the current stress on
>>> non-tape devices is probably intentional.

>> The patch is misleading.  The sentence "If the seek operation is
>> past the end of file..." only applies to tape devices, so it must
>> not follow a sentence about lseek(2).

> Why does it only apply to tape devices?
> dd if=/dev/zero of=file bs=1 seek=10 count=1
> will seek 10 bytes into the output (filling it with zeros)
> and then write the 1 byte from input, as intended.
> -rw-r--r--  1 hans  wheel  11 Feb 15 10:50 file

You are right, lseek(2) also behaves that way, as documented:

  The lseek() function allows the file offset to be set beyond the end of
  the existing end-of-file of the file.  If data is later written at this
  point, subsequent reads of the data in the gap return bytes of zeros
  (until data is actually written into the gap).

However, the reordering is still unfortunate because it would no longer
be clear that filling the gap also applies to tape devices.  That's
what the dd(1) manual needs to explain because that's what dd(1)
actually implements.  Documenting lseek(2) in the dd(1) manual page,
on the other hand, isn't really needed.

I'm not convinced that the current text for seek= needs further
changes.

Yours,
  Ingo



Re: dd bs= supercede ibs= and obs=

2020-02-15 Thread Ingo Schwarze
Hi,

Jan Stary wrote on Sat, Feb 15, 2020 at 11:07:04AM +0100:
> On Feb 14 17:04:51, schwa...@usta.de wrote:
>> Jason McIntyre wrote on Fri, Feb 14, 2020 at 07:28:59AM +:
>>> On Thu, Feb 13, 2020 at 11:25:07PM +0100, Jan Stary wrote:

 * Fix a factual error in the description of bs: it does not
   supersede ibs/obs, dd will error out when both are specified.

>> In fact, that's a bug in the code.  POSIX requires:
>>   bs=expr
>> Set both input and output block sizes to expr bytes,
>> superseding ibs= and obs=. 
>> (see https://pubs.opengroup.org/onlinepubs/9699919799/utilities/dd.html )

> This is the line that makes it illegal to specify bs=
> if ibs= or obs= (or bs= for that matter) has already been specified.

That appears to be part of the picture, but that is not complete.
As i read POSIX (and our manual page), bs= has to supersede even
ips= and ops= operands that follow it (rather than precede it).

Inspecting the source code, that is also how dd(1) behaved in
4.3BSD-Reno, which got its dd(1) implementation from AT Version 7
UNIX, which already behaved that way.

The buggy code was part of a rewrite checked into SCCS by Keith
Bostic@ on 91/07/26, i.e. after about one third of the working phase
leading from Reno to 4.4BSD.  As our Copyright notice still says,
the code he checked in was written by Keith Muller (UC San Diego).
The authorship is confirmed by an explicit entry in the file
"admin/admin/contrib/contrib".

That means the bug is at least 28.5 years old.

Regarding the fix:  It seems sufficient to just remove the four
flags causing dd(1) to exit.  The argument handling callbacks already
do the right thing: f_bs() already overrides ibs and obs, while
f_ibs() and f_obs() do nothing when bs is already set.

Even though the risk feels low for this particular change, i think
i should test this with a full make build / make release, as a
matter of principle when touching such low-level tools.

Here is the new behaviour, matching v7 and 4.3BSD-Reno:

   $ dd if=/dev/zero of=/dev/null ibs=2048 obs=1024 bs=4096 count=2 
  dd: bs supersedes ibs and obs
  2+0 records in
  2+0 records out
  8192 bytes transferred in 0.000 secs (140961886 bytes/sec)
   $ dd if=/dev/zero of=/dev/null bs=4096 ibs=2048 obs=1024 count=2 
  dd: bs supersedes ibs and obs
  2+0 records in
  2+0 records out
  8192 bytes transferred in 0.000 secs (127456319 bytes/sec)
   $ dd if=/dev/zero of=/dev/null bs=4096 count=2   
  2+0 records in
  2+0 records out
  8192 bytes transferred in 0.000 secs (230364725 bytes/sec)
   $ dd if=/dev/zero of=/dev/null ibs=4096 count=2
  2+0 records in
  16+0 records out
  8192 bytes transferred in 0.000 secs (183780146 bytes/sec)
   $ dd if=/dev/zero of=/dev/null obs=256 count=2  
  2+0 records in
  4+0 records out
  1024 bytes transferred in 0.000 secs (35021718 bytes/sec)

This also matches GNU gdd(1) from the coreutils package, fwiw.

Yours,
  Ingo


Index: args.c
===
RCS file: /cvs/src/bin/dd/args.c,v
retrieving revision 1.31
diff -u -p -r1.31 args.c
--- args.c  16 Feb 2019 10:54:00 -  1.31
+++ args.c  15 Feb 2020 13:47:04 -
@@ -68,14 +68,14 @@ static const struct arg {
void (*f)(char *);
u_int set, noset;
 } args[] = {
-   { "bs", f_bs,   C_BS,C_BS|C_IBS|C_OBS|C_OSYNC },
+   { "bs", f_bs,   C_BS,C_BS|C_OSYNC },
{ "cbs",f_cbs,  C_CBS,   C_CBS },
{ "conv",   f_conv, 0,   0 },
{ "count",  f_count,C_COUNT, C_COUNT },
{ "files",  f_files,C_FILES, C_FILES },
-   { "ibs",f_ibs,  C_IBS,   C_BS|C_IBS },
+   { "ibs",f_ibs,  C_IBS,   C_IBS },
{ "if", f_if,   C_IF,C_IF },
-   { "obs",f_obs,  C_OBS,   C_BS|C_OBS },
+   { "obs",f_obs,  C_OBS,   C_OBS },
{ "of", f_of,   C_OF,C_OF },
{ "seek",   f_seek, C_SEEK,  C_SEEK },
{ "skip",   f_skip, C_SKIP,  C_SKIP },



Re: extern already declared

2020-02-14 Thread Ingo Schwarze
Hi,

Todd C. Miller wrote on Sun, Feb 09, 2020 at 09:49:35AM -0700:
> On Sun, 09 Feb 2020 17:46:51 +0100, Jan Stary wrote:
 
>> Whenever unistd.h declares getopt(3), it also declares
>> the extern optind and optarg, so files including unistd.h
>> don't need to declare those themselves, right?

> Correct.  Most of those date back from before optind and optarg
> were defined for you by unistd.h.

Committed, thanks.
  Ingo



Re: dd(1) wording and style

2020-02-14 Thread Ingo Schwarze
Hi,

Jason McIntyre wrote on Fri, Feb 14, 2020 at 07:28:59AM +:
> On Thu, Feb 13, 2020 at 11:25:07PM +0100, Jan Stary wrote:

>> -.It Cm seek= Ns Ar n
>> +.It Cm seek Ns = Ns Ar n
>>  Seek
>>  .Ar n
>>  blocks from the beginning of the output before copying.
>> -On non-tape devices, an
>> -.Xr lseek 2
>> -operation is used.
>> -Otherwise, existing blocks are read and the data discarded.
>> -If the user does not have read permission for the tape, it is positioned
>> -using the tape
>> +On a tape device, existing blocks are read and the data discarded;
>> +if the user does not have read permission for the tape,
>> +it is positioned using the tape
>>  .Xr ioctl 2
>>  function calls.
>> +On all other devices devices, an
>> +.Xr lseek 2
>> +operation is used.
>>  If the seek operation is past the end of file, space from the current
>>  end of file to the specified offset is filled with blocks of NUL bytes.

> i think this change is ok. however i think non-tape is clearer than "on
> all other devices". it's not a biggie, but i think the current stress on
> non-tape devices is probably intentional.

The patch is misleading.  The sentence "If the seek operation is
past the end of file..." only applies to tape devices, so it must
not follow a sentence about lseek(2).

I'm not convinced the text needs to be changed.

The existing order makes sense because the non-tape case can be
handled quickly, and then all the remaining complicated explanations
apply to tapes only.

Or what would you want to change, and why?


>> @@ -135,14 +137,10 @@ Otherwise, input data is read and discar
>>  For pipes, the correct number of bytes is read.
>>  For all other devices, the correct number of blocks is read without
>>  distinguishing between a partial or complete block being read.
>> -.It Xo
>> -.Sm off
>> -.Cm status= Ar value
>> -.Sm on
>> -.Xc
>> -Where
>> +.It Cm status Ns = Ns Ar value
>> +where

> you're correct that "Where" doesn;t really start a sentence, but this is
> sentence start position. using a small letter is also incorrect.
> 
> in all honesty i would leave this, because it is not easy to rewrite in
> a way that sounds natural. but maybe
> 
>   The
>   .Ar value
>   parameter is one of the symbols from the following list:

That would look ugly by causing the line to wrap, but what about
the patch below?

Yours,
  Ingo


Index: dd.1
===
RCS file: /cvs/src/bin/dd/dd.1,v
retrieving revision 1.36
diff -u -r1.36 dd.1
--- dd.114 Feb 2020 15:55:57 -  1.36
+++ dd.114 Feb 2020 16:33:23 -
@@ -136,9 +136,9 @@
 For all other devices, the correct number of blocks is read without
 distinguishing between a partial or complete block being read.
 .It Cm status Ns = Ns Ar value
-Where
+The
 .Ar value
-is one of the symbols from the following list.
+is one of the symbols from the following list:
 .Bl -tag -width unblock
 .It Cm noxfer
 Do not print the transfer statistics as the last line of status output.
@@ -147,9 +147,9 @@
 Error messages are shown; informational messages are not.
 .El
 .It Cm conv Ns = Ns Ar value Ns Op , Ns Ar value ...
-Where
+Each
 .Ar value
-is one of the symbols from the following list.
+is one of the symbols from the following list:
 .Bl -tag -width unblock
 .It Cm ascii
 The same as the



Re: dd(1) wording and style

2020-02-14 Thread Ingo Schwarze
Hi,

Jason McIntyre wrote on Fri, Feb 14, 2020 at 07:28:59AM +:
> On Thu, Feb 13, 2020 at 11:25:07PM +0100, Jan Stary wrote:

>> * Fix a factual error in the description of bs: it does not
>>   supersede ibs/obs, dd will error out when both are specified.

> i don;t want to comment on this change

In fact, that's a bug in the code.  POSIX requires:

  bs=expr
Set both input and output block sizes to expr bytes,
superseding ibs= and obs=. 

  (see https://pubs.opengroup.org/onlinepubs/9699919799/utilities/dd.html )

Yours,
  Ingo



Re: [PATCH] [www] books.html - remove superfluous angle bracket

2020-02-14 Thread Ingo Schwarze
Hi Raf,

Raf Czlonka wrote on Fri, Feb 14, 2020 at 02:55:20PM +:
> On Mon, Nov 25, 2019 at 11:16:09AM GMT, Raf Czlonka wrote:

>> Index: books.html
>> ===
>> RCS file: /cvs/www/books.html,v
>> retrieving revision 1.117
>> diff -u -p -r1.117 books.html
>> --- books.html   1 Jun 2019 23:12:47 -   1.117
>> +++ books.html   25 Nov 2019 11:15:11 -
>> @@ -355,7 +355,7 @@ Lots of examples and real world code sni
>>  
>>  Network administration
>>  
>> ->Das SSH-Buch
>> +Das SSH-Buch
>>  (German)
>>  by Timo Dotzauer and Tobias Ltticke
>>  ISBN 3-938626-03-8

Committed, thanks.
  Ingo



Re: [PATCH] [www] faq/current.html - be consistent with naming of the 'id' attribute

2020-02-14 Thread Ingo Schwarze
Hi Raf,

Raf Czlonka wrote on Fri, Feb 14, 2020 at 01:53:03PM +:

> Small inconsistency.
> Personally, I prefer id *without* the 'r' but the below is "the odd
> one out" so...

Oops, sorry.

Fixed, thanks for noticing.
  Ingo


> Index: faq/current.html
> ===
> RCS file: /cvs/www/faq/current.html,v
> retrieving revision 1.1029
> diff -u -p -r1.1029 current.html
> --- faq/current.html  13 Feb 2020 16:29:21 -  1.1029
> +++ faq/current.html  14 Feb 2020 13:46:41 -
> @@ -229,7 +229,7 @@ root crontab or /etc/weekly.local<
>  /etc/locate.rc.
>  
>  
> -2020/02/13 - man.conf(5) _whatdb directive no longer 
> supported
> +2020/02/13 - man.conf(5) _whatdb directive no longer 
> supported
>  
>  In https://man.openbsd.org/man.conf.5;>man.conf(5),
>  change lines of the form



Re: setlocale() in cron

2020-02-11 Thread Ingo Schwarze
Hi,

Jan Stary wrote on Mon, Feb 10, 2020 at 01:13:58PM +0100:

> Why does cron(8) and crontab(1) need to setlocale()?

Committed; thanks to millert@ for cross-checking.
  Ingo


> Index: cron.c
> ===
> RCS file: /cvs/src/usr.sbin/cron/cron.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 cron.c
> --- cron.c23 Oct 2017 15:15:22 -  1.77
> +++ cron.c10 Feb 2020 12:12:13 -
> @@ -28,7 +28,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -83,8 +82,6 @@ main(int argc, char *argv[])
>   struct sigaction sact;
>   sigset_t blocked, omask;
>   struct group *grp;
> -
> - setlocale(LC_ALL, "");
>  
>   setvbuf(stdout, NULL, _IOLBF, 0);
>   setvbuf(stderr, NULL, _IOLBF, 0);
> Index: crontab.c
> ===
> RCS file: /cvs/src/usr.sbin/cron/crontab.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 crontab.c
> --- crontab.c 28 Jun 2019 13:32:47 -  1.93
> +++ crontab.c 10 Feb 2020 12:12:13 -
> @@ -26,7 +26,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -92,7 +91,6 @@ main(int argc, char *argv[])
>   user_gid = getgid();
>   crontab_gid = getegid();
>  
> - setlocale(LC_ALL, "");
>   openlog(__progname, LOG_PID, LOG_CRON);
>  
>   setvbuf(stderr, NULL, _IOLBF, 0);



Re: don't try to signal with newsyslog -r

2020-02-10 Thread Ingo Schwarze
Hi,

Jan Stary wrote on Mon, Feb 10, 2020 at 05:12:53PM +0100:

> The -r option of newsyslog(8) removes the requirement
> that newsyslog runs as root. Would it also make sense
> to not try to send the SIGHUP to syslogd in that case?

While i'm not sure that i want to take care of this patch,
given that i'm not quite sure what the point of the -r option
even is in the first place, i'd like to point out that you
are also removing the warning.  Is that intentional?
Naively, getting a warning when files are rotated but the
daemon isn't notified seems useful to me, even if you kind
of requested it with -r.

In that case, wouldn't it make more sense to say something
like (untested, and no code auditing done)

else if (needroot == 0 || kill(pid, signal))

?

That would also avoid the hopeless attempt to send a signal,
but would still print the warning.

Yours,
  Ingo


> Index: newsyslog.8
> ===
> RCS file: /cvs/src/usr.bin/newsyslog/newsyslog.8,v
> retrieving revision 1.54
> diff -u -p -r1.54 newsyslog.8
> --- newsyslog.8   20 Jul 2017 18:39:16 -  1.54
> +++ newsyslog.8   10 Feb 2020 16:08:51 -
> @@ -124,7 +124,7 @@ Removes the restriction that
>  must be running as root.
>  Note that in this mode
>  .Nm
> -will not be able to send a
> +will not try to send a
>  .Dv SIGHUP
>  signal to
>  .Xr syslogd 8 .
> Index: newsyslog.c
> ===
> RCS file: /cvs/src/usr.bin/newsyslog/newsyslog.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 newsyslog.c
> --- newsyslog.c   28 Jun 2019 13:35:02 -  1.112
> +++ newsyslog.c   10 Feb 2020 16:08:51 -
> @@ -394,7 +394,7 @@ send_signal(char *pidfile, int signal)
>   warnx("%s pid file: %s", err, pidfile);
>   else if (noaction)
>   (void)printf("kill -%s %ld\n", sys_signame[signal], (long)pid);
> - else if (kill(pid, signal))
> + else if (needroot && kill(pid, signal))
>   warnx("warning - could not send SIG%s to PID from pid file %s",
>   sys_signame[signal], pidfile);
>  }



Re: setlocale() in cron

2020-02-10 Thread Ingo Schwarze
Hi,

Jan Stary wrote on Mon, Feb 10, 2020 at 01:13:58PM +0100:

> Why does cron(8) and crontab(1) need to setlocale()?

I looked through the *.c files in /usr/src/usr.sbin/cron/
and found the following locale-dependent functions:

atrun.c:  isalpha(3), isupper(3)
cron.c:   strtod(3)
do_command.c: isalnum(3), isprint(3), stravis(3)
entry.c:  isalpha(3), isdigit(3)
env.c:isspace(3)

Neither strftime(3) nor strptime(3) appear to be used, which would
be common traps in this respect, in particular in programs doing
something with dates and times.

Either way, on OpenBSD, none of these functions actually depend on
the locale.

Even if there were something locale-dependent in cron(8), and even
if we consider somebody using these program in a portable way on a
non-OpenBSD system, i believe a daemon started as root should not be
locale-dependent, so i'd like to commit the patch to cron.c in any
case.

While crontab.c does not seem quite as important, i see no possible
harm in cleaning this up, so i'd like to commit that one, too.

OK?
  Ingo


> Index: cron.c
> ===
> RCS file: /cvs/src/usr.sbin/cron/cron.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 cron.c
> --- cron.c23 Oct 2017 15:15:22 -  1.77
> +++ cron.c10 Feb 2020 12:12:13 -
> @@ -28,7 +28,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -83,8 +82,6 @@ main(int argc, char *argv[])
>   struct sigaction sact;
>   sigset_t blocked, omask;
>   struct group *grp;
> -
> - setlocale(LC_ALL, "");
>  
>   setvbuf(stdout, NULL, _IOLBF, 0);
>   setvbuf(stderr, NULL, _IOLBF, 0);
> Index: crontab.c
> ===
> RCS file: /cvs/src/usr.sbin/cron/crontab.c,v
> retrieving revision 1.93
> diff -u -p -r1.93 crontab.c
> --- crontab.c 28 Jun 2019 13:32:47 -  1.93
> +++ crontab.c 10 Feb 2020 12:12:13 -
> @@ -26,7 +26,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -92,7 +91,6 @@ main(int argc, char *argv[])
>   user_gid = getgid();
>   crontab_gid = getegid();
>  
> - setlocale(LC_ALL, "");
>   openlog(__progname, LOG_PID, LOG_CRON);
>  
>   setvbuf(stderr, NULL, _IOLBF, 0);
> 



Re: Add note about example dhclient.conf

2020-02-10 Thread Ingo Schwarze
Hi Kyle,

Kyle Isom wrote on Mon, Feb 10, 2020 at 07:34:25AM -0800:
> On Sat, Feb 8, 2020, at 14:15, Jason McIntyre wrote:

>> - i'm ok with adding the path to these files to a FILES section

> Done.

I already committed a comprehensive diff doing that in a simpler
way earlier today:

  https://marc.info/?l=openbsd-cvs=158134072405241

But thanks for bringig up the issue, anyway.

Yours,
  Ingo


> Index: sbin/dhclient/dhclient.conf.5
> ===
> RCS file: /cvs/src/sbin/dhclient/dhclient.conf.5,v
> retrieving revision 1.49
> diff -u -p -u -p -r1.49 dhclient.conf.5
> --- sbin/dhclient/dhclient.conf.5 17 Dec 2019 14:21:54 -  1.49
> +++ sbin/dhclient/dhclient.conf.5 10 Feb 2020 15:31:41 -
> @@ -288,6 +288,11 @@ instead of the
>  .Ic sname
>  field of the DHCP offer when binding a lease.
>  .El
> +.Sh FILES
> +An example configuration for
> +.Pa dhclient.conf
> +is provided in
> +.Pa /etc/examples/dhclient.conf .
>  .Sh SEE ALSO
>  .Xr dhclient.leases 5 ,
>  .Xr dhcp-options 5 ,
> 
> 



Re: locate.updatedb TMPDIR

2020-02-09 Thread Ingo Schwarze
Hi Joerg,

this is absolutely not OK.

How did you test this?

   $ doas cat /var/log/weekly.part  
  /etc/weekly[79]: no closing quote

With that fixed, i agree with the direction of the change.

Yours,
  Ingo

Joerg Jung wrote on Sun, Feb 09, 2020 at 12:33:42AM +0100:

> I have a machine with a large data storage attached, but it has only 2GB
> of /tmp (which I consider enough usually).  On this machine weekly
> locate.updatedb fails, due to /tmp being full.  To fix this I would like
> to point locate to a different TMPDIR.
>  
> But it seems one can not just set TMPDIR from /etc/locate.rc to point
> elsewhere, since command line args in /etc/weekly override
> /etc/locate.rc settings.
> 
> While it's possible to add a weekly.local to set TMPDIR I believe it
> should be better set in the actual configuration file instead of 
> ignoring locate.rc.
> 
> Thus the diff below proposes to remove command line argument to be able
> to use /etc/locate.rc instead.
> 
> Note, this might break existing setups, where one has set TMPDIR in
> /etc/weekly.local to point elsewhere.  This might be handled with a
> -current upgrade entry?
> 
> Comments? OK?
> 
> Thanks,
> Regards,
> Joerg
> 
> 
> Index: etc/weekly
> ===
> RCS file: /cvs/src/etc/weekly,v
> retrieving revision 1.29
> diff -u -p -r1.29 weekly
> --- etc/weekly30 Dec 2019 16:49:51 -  1.29
> +++ etc/weekly8 Feb 2020 23:13:02 -
> @@ -48,7 +48,7 @@ if [ -f /var/db/locate.database ]; then
>   if TMP=`mktemp /var/db/locate.database.XX`; then
>   trap 'rm -f $TMP; exit 1' 0 1 15
>   UPDATEDB="/usr/libexec/locate.updatedb"
> - echo "${UPDATEDB} --fcodes=- --tmpdir=${TMPDIR:-/tmp}" | \
> + echo "${UPDATEDB} --fcodes=- | \
>   nice -5 su -m nobody 1>$TMP
>   if [ $? -ne 0 ]; then
>   echo "Rebuilding locate database failed"



Re: locate.updatedb TMPDIR

2020-02-09 Thread Ingo Schwarze
Hi Todd,

Todd C. Miller wrote on Sun, Feb 09, 2020 at 07:52:10AM -0700:

> I'm fine with this.

I don't really object, but i'm not sure it is needed either.

It's certainly obvious that command line arguments override defaults.
That's what they always do.  It's their whole point, in general.

Also, command line arguments (at least almost) always override
configuration file settings.  If they don't, i would usually call
that a serious design error.  There may be exceptions, like
configuration settings to enable or disable optional command
line functionality, but that is very unusual.

> However, it doesn't look like we document /etc/locate.rc anywhere.
> I'm not sure it deserves its own man page
> but I think it should at least be documented in locate.updatedb
> in some fashion.

It is mentioned there, but not really documented.

OK?
  Ingo


Index: locate.updatedb.8
===
RCS file: /cvs/src/usr.bin/locate/locate/locate.updatedb.8,v
retrieving revision 1.18
diff -u -r1.18 locate.updatedb.8
--- locate.updatedb.8   10 Dec 2009 00:45:43 -  1.18
+++ locate.updatedb.8   9 Feb 2020 15:14:57 -
@@ -51,10 +51,6 @@
 .Pa /etc/weekly
 script.
 .Pp
-The contents of the newly built database can be controlled by the
-.Pa /etc/locate.rc
-file as well as the command line arguments.
-.Pp
 The options are as follows:
 .Bl -tag -width Ds
 .It Fl -fcodes
@@ -75,6 +71,14 @@
 .It Fl -tmpdir
 Sets the directory temporary files are stored in.
 .El
+.Pp
+The default settings are controlled by the configuration file
+.Pa /etc/locate.rc .
+It is a
+.Xr sh 1
+script that can be used to set variables.
+The names of the variables match the names of the command line
+options, but in all caps.
 .Sh FILES
 .Bl -tag -width /var/db/locate.database -compact
 .It Pa /etc/locate.rc



Re: mention /etc/examples/ in bgpf.conf(5)/bgpd(8)

2020-02-09 Thread Ingo Schwarze
Hi Jason,

Jason McIntyre wrote on Sun, Feb 09, 2020 at 07:49:10AM +:

> - bgpd.8 refers to /etc/bgpd.conf. that file doesn;t exist by default.

I do not consider that a problem, not even a minor one.  ENVIRONMENT
says which variables are inspected if they exist.  FILES says which
files are used if they exists.  If they don't, well, then they are
not used.  I don't see anything that needs to be explained in that
respect.

> - i'm not convinced bgpd.8 needs to refer to the example file.

Fair enough, deleted from my tree.  So we will usually point to the
examples from the file format manuals, but not from the program
manuals, unless there are special considerations for a specific
program.

> - only root can view the example file. i understand why, but in itself
>   it's not that helpful.

I don't see a reasonable way to improve that.  Besides, only root
can configure the daemon, so it's not a very serious obstacle in
practice.

> - we've seen from this thread that people are missing the existence of
>   /etc/examples/bgpd.conf. that's a shame because it looks like a nicely
>   written file.
> 
> the essential issue is we want people to find the examples. the least
> amount of work to achieve that is just for bgpd.conf.5 to mention it.
> adding extra stuff about what's in there is a lot of work and sets us up
> for things going out of date in the future. and although it might work
> well for bgpd - a big, complex, piece of software, it will probably make
> less sense for most other software. leaving us with a divided strategy.
> 
> it'd be good to have a clear idea of which pages we're planning to
> change, and how, so that it's clear for other authors.
> 
> for bgpd i propose:
> 
> - no change to bgpd.8. i don;t have a solution to the fact that we moved
>   its config file to examples/ so there isn;t an /etc/bgpd.conf file. i
>   suppose everyone understands what is happening.
> - for bgpd.conf.5, add an entry to FILES "example configuration file".
>   no more.

For now, that's what i did, even though i like deraadt@'s idea, but
we lack consensus, so i applied the minimum change that everybody
agreed with.

Whether to add additional information as part of a strategy to do
this in several cases where it makes sense (but of course not
everywhere), or merely in this individual case as an exception,
or not at all - that can be decided separately.

Theo has brought forward some very substantial arguments in favour,
but i didn't want to summarily dismiss Jason's reservations that
it increases the risk of information getting out of sync, either.

> - i'm not the right person to judge the existence of individual example
>   files. ratchov already indicated he doesn;t see the point of
>   examples/mixerctl.conf. i guess individual devs get to judge this.

That wouldn't result in a consistent user experience.  For example,
am i supposed to delete /etc/examples/man.conf because i don't like it?
I'll refrain from that because deraadt@, espie@, and others made it
clear that they value the concept in general.

> - i think an addition to afterboot.8 totally makes sense. it's what it's
>   there for.

I assume espie@ will commit that.

> - i don;t think an addition to help.1 makes sense. that is just trying
>   to tell people how to use unix, and no one reads it anyway.

Fair enough, i'll leave it alone.

Yours,
  Ingo



Re: mention /etc/examples/ in bgpf.conf(5)/bgpd(8)

2020-02-09 Thread Ingo Schwarze
Hi Marc,

Marc Espie wrote on Sun, Feb 09, 2020 at 02:27:23PM +0100:

> I still think it's a good idea to put it in afterboot(8).

No more objections, with or without jmc@'s tweaks.
It seems clear that enough people want it in that page.

Yours,
  Ingo


> Index: afterboot.8
> ===
> RCS file: /cvs/src/share/man/man8/afterboot.8,v
> retrieving revision 1.164
> diff -u -p -r1.164 afterboot.8
> --- afterboot.8   28 Sep 2018 18:21:31 -  1.164
> +++ afterboot.8   9 Feb 2020 13:26:29 -
> @@ -60,6 +60,10 @@ command, type:
>  Administrators will rapidly become more familiar with
>  .Ox
>  if they get used to using the high quality manual pages.
> +.Pp
> +Some base programs and subsystems also come with sample configuration 
> +files in
> +.Pa /etc/examples .
>  .Ss Errata
>  By the time that you have installed your system, it is possible that
>  bugs in the release have been found.



Re: mention /etc/examples/ in bgpf.conf(5)/bgpd(8)

2020-02-08 Thread Ingo Schwarze
Hi,

Theo de Raadt wrote on Sat, Feb 08, 2020 at 04:39:42PM -0700:

> For complicated configurations, the text could explain the reason the
> example is valuable -- for instance
> 
> .It Pa /etc/examples/bgpd.conf
> Example configuration file demonstrating IBGP mesh, multiple transits,
> RPKI filtering, and other best practices.

I like that.  It gives the reader an idea of how interesting the file
might be without even opening it.

We have to be careful to not make the comment too long, or FILES
might look bad, but your text still looks reasonable to me.

I think it's sufficient to have this wording in bgpd.conf(5);
in bgpd(8), mentioning the existence of the example seems fair, but
the exact contents seem more tangential there.  I guess network
admins running that beast will have to look at bgpd.conf(5) anyway.


I applied Jason's recommendation to remove the "do not edit";
probably he is right that it isn't required.

Regarding benno@'s comment:  /etc/exanmples/ is already mentioned
in hier(7) and yet some people missed it.  I wouldn't blame them.
Also, you don't really need to look into /etc/examples/ for all
programs; if we point to it from where it matters, that's even
better usability.  Still, i think it would *also* make sense to
mention the directory in a FILES section in help(1), and optionally
also in afterboot(8), though the latter seems less relevant.  I'm
not feeling strongly about that, and one doesn't exclude the other.
Pointers are cheap and can be added whereever they help, only not
in such an amount that they would become a distraction.

Updated patch: still OK?
  Ingo


Index: bgpd.8
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.8,v
retrieving revision 1.62
diff -u -p -r1.62 bgpd.8
--- bgpd.8  10 Nov 2019 20:51:53 -  1.62
+++ bgpd.8  8 Feb 2020 23:52:00 -
@@ -205,11 +205,13 @@ Only check the configuration file for va
 Produce more verbose output.
 .El
 .Sh FILES
-.Bl -tag -width "/var/run/bgpd.sockXXX" -compact
+.Bl -tag -width "/etc/examples/bgpd.conf" -compact
 .It Pa /etc/bgpd.conf
 default
 .Nm
 configuration file
+.It Pa /etc/examples/bgpd.conf
+example configuration file
 .It Pa /var/run/bgpd.sock
 default
 .Nm
Index: bgpd.conf.5
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v
retrieving revision 1.199
diff -u -p -r1.199 bgpd.conf.5
--- bgpd.conf.5 25 Jan 2020 12:07:28 -  1.199
+++ bgpd.conf.5 8 Feb 2020 23:52:00 -
@@ -1883,10 +1883,13 @@ For prefixes with equally long paths, th
 is selected.
 .El
 .Sh FILES
-.Bl -tag -width "/etc/bgpd.conf" -compact
+.Bl -tag -width "/etc/examples/bgpd.conf" -compact
 .It Pa /etc/bgpd.conf
 .Xr bgpd 8
 configuration file
+.It Pa /etc/examples/bgpd.conf
+example configuration file demonstrating IBGP mesh, multiple transits,
+RPKI filtering, and other best practices
 .El
 .Sh SEE ALSO
 .Xr strftime 3 ,



mention /etc/examples/ in bgpf.conf(5)/bgpd(8)

2020-02-08 Thread Ingo Schwarze
Hi,

Jason McIntyre wrote on Sat, Feb 08, 2020 at 10:15:08PM +:

> - i'm ok with adding the path to these files to a FILES section

So, here is a specific patch for bgpf.conf(5) and bgpd(8) such
that we can agree on a general direction for one case where
the example file is particularly important.

Once the direction is agreed, applying it to the relatively
small number of other cases is likely not difficult.

OK?
  Ingo


Index: bgpd.8
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.8,v
retrieving revision 1.62
diff -u -p -r1.62 bgpd.8
--- bgpd.8  10 Nov 2019 20:51:53 -  1.62
+++ bgpd.8  8 Feb 2020 23:24:28 -
@@ -205,11 +205,13 @@ Only check the configuration file for va
 Produce more verbose output.
 .El
 .Sh FILES
-.Bl -tag -width "/var/run/bgpd.sockXXX" -compact
+.Bl -tag -width "/etc/examples/bgpd.conf" -compact
 .It Pa /etc/bgpd.conf
 default
 .Nm
 configuration file
+.It Pa /etc/examples/bgpd.conf
+example file, do not edit
 .It Pa /var/run/bgpd.sock
 default
 .Nm
Index: bgpd.conf.5
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.conf.5,v
retrieving revision 1.199
diff -u -p -r1.199 bgpd.conf.5
--- bgpd.conf.5 25 Jan 2020 12:07:28 -  1.199
+++ bgpd.conf.5 8 Feb 2020 23:24:28 -
@@ -1883,10 +1883,12 @@ For prefixes with equally long paths, th
 is selected.
 .El
 .Sh FILES
-.Bl -tag -width "/etc/bgpd.conf" -compact
+.Bl -tag -width "/etc/examples/bgpd.conf" -compact
 .It Pa /etc/bgpd.conf
 .Xr bgpd 8
 configuration file
+.It Pa /etc/examples/bgpd.conf
+example file, do not edit
 .El
 .Sh SEE ALSO
 .Xr strftime 3 ,



Re: get rid of almost empty /etc/examples/mixerctl.conf

2020-02-08 Thread Ingo Schwarze
Hi Theo,

Theo de Raadt wrote on Sat, Feb 08, 2020 at 03:33:37PM -0700:
> Jason McIntyre  wrote:
 
>> without getting into a discussion about /etc/examples, in this case i
>> personally see neither the point of the example config file (so trivial
>> as to be questionable) nor the addition to the man page (if the example
>> is worthwhile, add it to mixerctl, not the conf page).

> there is additional pieces of "documentation" that occurs here
> 
> - this is a filename, which if placed one directory above., would
>   perform a function
> - the comment format of the file is demonstrated
> - the basic format of the file is demonstrated (is it simply a series
>   of name=value, or have we created a richer DSL)
> 
> you can argue that is in the manual page.  It is.  After you read a
> lot more.  There is a pace-of-learning going on here.  Noone ever
> demanded that learning must come from a man page, additional sources
> of transient information are not prohibited, but I'm picking up a
> disdain for information learned any other way...

Indeed.  When i talk about quality of documentation - which i did
at multiple conferences, last time 2018 in Bucuresti - one of the
first sentences i almost always say is:

  Documentation must be correct, complete, concise, all in one
  place, marked up for display and search, easy to read, and easy
  to write.

  e.g. https://www.openbsd.org/papers/eurobsdcon2018-mandoc.pdf
  first page after the title, second bullet point

The "all in one place" part has two purposes: convenience for the
user to not have to search multiple places to find it and convenience
for the developer to not have to maintain duplicate information.

Of course, you have valid points that examples can be particularly
fast for picking up information (when they are short and unless you
happen to draw incorrect conclusions from them) and that copy-and-paste
can matter.

So, if there is no clear consensus about which arguments matter how
much, we probably best leave /etc/examples/ alone.  I doubt having
a prolonged fight over this is a smart idea.

As long as the manuals are complete and somebody voluntarily
maintains the examples, which seems the case for now, there is
not much harm from the duplication during the time we lack full
consensus.

Yours,
  Ingo



get rid of almost empty /etc/examples/mixerctl.conf

2020-02-08 Thread Ingo Schwarze
Hi Theo,

you have a point, that was a lot of cheap talk and no patch.

I don't aim at changing yacc(1) grammars.  I think most parts of
OpenBSD configuration systems already have sane defaults and most
configuration syntaxes are already good with respect to simplicity
and usability.  At least "most", maybe even all.

> The example files were moved from /etc, so that /etc contained fewer
> files for unconfigured software.

That was a very good step, and it was also good to not encumber it
by trying to do more at the same time.

> Little to no effort was put into curating them.  That's the gap that
> occured.

So, here i'm putting a few pennies where my mouth is, starting from the
shortest file in /etc/examples/.  The example is so short that cut and
paste from the manual page will certainly cause no trouble, and the
file also isn't a special case with respect to permissions,
it is -rw-r--r-- root:wheel.

OK?
  Ingo


P.S.
When preparing this, i noticed i already had the change to
mixerctl.conf.5 in my tree.  It looks like i started it
in the past, then forgot about it.

P.P.S.
Note that i will not propose to get rid of *all* of /etc/examples/,
but just of trivial examples that provide no benefit by being outside
the manual page.  There are certainly some that make sense as they
are.


Index: etc/Makefile
===
RCS file: /cvs/src/etc/Makefile,v
retrieving revision 1.477
diff -u -p -r1.477 Makefile
--- etc/Makefile24 Jan 2020 06:17:37 -  1.477
+++ etc/Makefile8 Feb 2020 21:42:14 -
@@ -46,7 +46,7 @@ MUTABLE=changelist daily etc.${MACHINE}/
 
 # -rw-r--r--
 EXAMPLES=acme-client.conf chio.conf dhclient.conf dhcpd.conf exports \
-   httpd.conf ifstated.conf inetd.conf man.conf mixerctl.conf \
+   httpd.conf ifstated.conf inetd.conf man.conf \
mrouted.conf ntpd.conf printcap rad.conf rbootd.conf \
remote sensorsd.conf wsconsctl.conf
 
Index: share/man/man5/mixerctl.conf.5
===
RCS file: /cvs/src/share/man/man5/mixerctl.conf.5,v
retrieving revision 1.7
diff -u -p -r1.7 mixerctl.conf.5
--- share/man/man5/mixerctl.conf.5  30 Jul 2018 17:24:24 -  1.7
+++ share/man/man5/mixerctl.conf.5  8 Feb 2020 21:42:14 -
@@ -141,6 +141,11 @@ Default audio mixing device.
 .Xr mixerctl 1
 configuration file.
 .El
+.Sh EXAMPLES
+Most audio cards support setting the output volume by adding this line to
+.Nm :
+.Pp
+.Dl outputs.master=200
 .Sh SEE ALSO
 .Xr aucat 1 ,
 .Xr audioctl 1 ,
Index: etc/examples/mixerctl.conf
===
RCS file: etc/examples/mixerctl.conf
diff -N etc/examples/mixerctl.conf
--- etc/examples/mixerctl.conf  16 Jul 2014 13:21:33 -  1.1
+++ /dev/null   1 Jan 1970 00:00:00 -
@@ -1,7 +0,0 @@
-# $OpenBSD: mixerctl.conf,v 1.1 2014/07/16 13:21:33 deraadt Exp $
-#
-# mixerctl(1) configurable parameters. See mixerctl.conf(5) for details.
-#
-
-# output volume value for most audio cards
-#outputs.master=200



Re: Add note about example dhclient.conf

2020-02-08 Thread Ingo Schwarze
Hi,

i think i said it before:  i hate /etc/examples/ and think that the
directory ought to be mostly empty.  With the exception of rare
cases like bgpd(8), where you have to provide a lot of information
before you can start it in any meaningful way, i consider a deamon
ill-designed if the configuration is so complicated that a file in
/etc/examples/ serves any purpose.  Daemons should have a reasonable
default configuration, such that a configuration file is not needed
for typical usage, and if people add a configuration file, they
ought to get away with very few lines.  By the way, acme-client(1)
is an example where i was happy to recently see people work into just
that direction.

Providing example configuration files is an idiotic concept because
if you copy them into /etc/, you no longer easily see what the
defaults are, what generic recommendations are, and what was changed
locally.  It should be made easy to write short configuration files
from scratch such that they contain absolutely nothing except local
changes.

For daemons designed as described, if they have some complicated
configuration commands for special purposes, a few typical
examples of these commands can be shown in the EXAMPLES section of
the manual page.

Note that in the past, there was no consensus about the above, so
i'm not suggesting that we move all the examples into the manual
pages.  I'm only saying *trivial* files in /etc/examples/ should
be deleted and the content, if any, moved to the manual pages.  Some
files will no doubt remain, and there is nothing wrong with that.

Jason McIntyre wrote on Sat, Feb 08, 2020 at 07:22:06AM +:

> if we do reference these config files from the man pages, i guess that
> should be correctly done from a FILES section, since EXAMPLES is really
> showing how to use the tool, rather than how to configure it. it is a
> fine line though.
> 
> i wouldn;t be against adding to FILES - it'd be very brief, make sense,
> and provide the reminder being asked for.

I like that very much, yes.  In general, if there is a file closely
related to the content of the manual page, mention it below FILES.
That also seems like a good idea for files below /etc/examples/.

Yours,
  Ingo



let "man -w" print the manpath

2020-02-06 Thread Ingo Schwarze
p -r1.34 man.1
--- man.1   7 Jan 2020 11:15:12 -   1.34
+++ man.1   6 Feb 2020 19:53:42 -
@@ -3,7 +3,7 @@
 .\" Copyright (c) 1989, 1990, 1993
 .\"The Regents of the University of California.  All rights reserved.
 .\" Copyright (c) 2003, 2007, 2008, 2014 Jason McIntyre 
-.\" Copyright (c) 2010, 2011, 2014-2018 Ingo Schwarze 
+.\" Copyright (c) 2010, 2011, 2014-2020 Ingo Schwarze 
 .\"
 .\" Redistribution and use in source and binary forms, with or without
 .\" modification, are permitted provided that the following conditions
@@ -199,6 +199,9 @@ Kernel internals.
 .It Fl w
 List the pathnames of all matching manual pages instead of displaying
 any of them.
+If no
+.Ar name
+is given, list the directories that would be searched.
 .El
 .Pp
 The options



  1   2   3   4   5   6   7   8   9   >