Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-05-08 Thread Klemens Nanni 
On Wed, May 08, 2019 at 05:31:37PM +0200, Ingo Schwarze wrote:
> Thank you again for doing this work.
+1

> Either way, this seems mature enough for getting it in.
> IMHO, no need to resend it again.
I concur;  Ingo's nits are fine.

Feel free to go ahead and commit with OK kn.

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-05-08 Thread Ingo Schwarze 
Hi Alexandr,

Alexandr Nedvedicky wrote on Wed, May 08, 2019 at 04:55:57PM +0200:

> below is third iteration of pf.conf.5 manpage.

OK schwarze@

A few final nits inline.

Thank you again for doing this work.
  Ingo


> diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
[...]
> @@ -1237,6 +1244,22 @@ Various limits can be combined on a single line:
>  .Bd -literal -offset indent
>  set limit { states 2, frags 2000, src-nodes 2000 }
>  .Ed
> +.Pp
> +.Xr pf 4
> +has the following defaults:
> +.Bl -column table-entries PFR_KENTRY_HIWAT_SMALL platform_dependent
> +.It states Ta Dv PFSTATE_HIWAT Ta Pq 10
> +.It tables Ta Dv PFR_KTABLE_HIWAT Ta Pq 1000
> +.It table-entries Ta Dv PFR_KENTRY_HIWAT Ta Pq 20
> +.It table-entries Ta Dv PFR_KENTRY_HIWAT_SMALL Ta Pq 10
> +.It frags Ta Dv NMBCLUSTERS/32 Ta Pq platform dependent

  Ta Dv NMBCLUSTERS Ns /32 Ta

would be even better.

> +.El
> +.Pp
> +.Cm NMBCLUSTERS

Please make this line

  .Dv NMBCLUSTERS

> +defines the total number of packets which can exist in-system at any one 
> time.
> +Refer to
> +.In machine/param.h
> +to find out a value for system sepcific values.

s/sepcific/specific/

The following would sound better; is it accurate?

  Refer to
  .In machine/param.h
  for the platform-specific value.

> @@ -1393,12 +1425,13 @@ set syncookies adaptive (start 25%, end 12%)
>  .It Ic set Cm timeout Ar variable value
>  .Bl -tag -width "src.track" -compact
>  .It Cm frag
> -Seconds before an unassembled fragment is expired.
> +Seconds before an unassembled fragment is expired (60 by default).
>  .It Cm interval
> -Interval between purging expired states and fragments.
> +Interval between purging expired states and fragments (10 seconds by 
> default).
>  .It Cm src.track
>  Length of time to retain a source tracking entry after the last state
> -expires.
> +expires (0 by default, which means there is no global limit.
> +The value is defined by rule which creates state.).

I think articles are needed:

  The value is defined by the rule which creates the state).

> @@ -1493,6 +1526,10 @@ set limit states 10
>  .Pp
>  With 9000 state table entries, the timeout values are scaled to 50%
>  (tcp.first 60, tcp.established 43200).
> +.Pp
> +.Dq pfctl -F Reset
> +restores default values for those options: debug, all limit options,
> +loginterface, reassemble, skip, syncookies, all timeouts.
>  .El
>  .Sh QUEUEING
>  Packets can be assigned to queues for the purpose of bandwidth

I think this would sound better:

  restores default values for the following options: debug, all limit options,
  loginterface, reassemble, skip, syncookies, and all timeouts.

Also, the new lines should go after the .El, not before it, like this:

.Pp
With 9000 state table entries, the timeout values are scaled to 50%
(tcp.first 60, tcp.established 43200).
.El
.Pp
.Dq pfctl -F Reset
restores default values for the following options: debug, all limit options,
loginterface, reassemble, skip, syncookies, and all timeouts.
.Sh QUEUEING


Either way, this seems mature enough for getting it in.
IMHO, no need to resend it again.

Yours,
  Ingo

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-05-08 Thread Alexandr Nedvedicky 
Hello,

below is third iteration of pf.conf.5 manpage. The new addresses
issues as follows:

all excessive "'pfctl -F Reset' ..." are gone.
I've added simple sentence/paragraph at the end of OPTIONS
section:
+.Pp
+.Dq pfctl -F Reset
+restores default values for those options: debug, all limit options,
+loginterface, reassemble, skip, syncookies, all timeouts.
also using '.Dq' instead of cross reference


reference to 'OPTIONS' section in pfctl(8) got fixed to:
+Reset limits, timeouts and other options back to default settings.
+See the OPTIONS section in
+.Xr pf.conf 5
+for details.

those two above should address all comments raised by Ingo, Jason and Klemens.

thanks and
regards
sashan

8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index b7e941991ba..a207f95321c 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -198,7 +198,10 @@ Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
 .It Fl F Cm Reset
-Reset limits, timeouts and options back to default settings.
+Reset limits, timeouts and other options back to default settings.
+See the OPTIONS section in
+.Xr pf.conf 5
+for details.
 .It Fl F Cm all
 Flush all of the above.
 .El
diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
index c3fa0d07e58..9b7b01c4306 100644
--- a/share/man/man5/pf.conf.5
+++ b/share/man/man5/pf.conf.5
@@ -1148,6 +1148,9 @@ A TCP RST is returned for blocked TCP packets,
 an ICMP UNREACHABLE is returned for blocked UDP packets,
 and all other packets are silently dropped.
 .El
+.Pp
+The default value is
+.Cm drop .
 .It Ic set Cm debug Ar level
 Set the debug
 .Ar level ,
@@ -1167,6 +1170,8 @@ and
 These keywords correspond to the similar (LOG_) values specified to the
 .Xr syslog 3
 library routine.
+The default value is
+.Cm err .
 .It Cm set Cm fingerprints Ar filename
 Load fingerprints of known operating systems from the given
 .Ar filename .
@@ -1177,6 +1182,8 @@ but can be overridden via this option.
 Setting this option may leave a small period of time where the fingerprints
 referenced by the currently active ruleset are inconsistent until the new
 ruleset finishes loading.
+The default location for fingerprints is
+.Pa /etc/pf.os .
 .It Ic set Cm hostid Ar number
 The 32-bit hostid
 .Ar number
@@ -1237,6 +1244,22 @@ Various limits can be combined on a single line:
 .Bd -literal -offset indent
 set limit { states 2, frags 2000, src-nodes 2000 }
 .Ed
+.Pp
+.Xr pf 4
+has the following defaults:
+.Bl -column table-entries PFR_KENTRY_HIWAT_SMALL platform_dependent
+.It states Ta Dv PFSTATE_HIWAT Ta Pq 10
+.It tables Ta Dv PFR_KTABLE_HIWAT Ta Pq 1000
+.It table-entries Ta Dv PFR_KENTRY_HIWAT Ta Pq 20
+.It table-entries Ta Dv PFR_KENTRY_HIWAT_SMALL Ta Pq 10
+.It frags Ta Dv NMBCLUSTERS/32 Ta Pq platform dependent
+.El
+.Pp
+.Cm NMBCLUSTERS
+defines the total number of packets which can exist in-system at any one time.
+Refer to
+.In machine/param.h
+to find out a value for system sepcific values.
 .It Ic set Cm loginterface Ar interface | Cm none
 Enable collection of packet and byte count statistics for the given
 interface or interface group.
@@ -1253,6 +1276,9 @@ collects statistics on the interface named dc0:
 One can disable the loginterface using:
 .Pp
 .Dl set loginterface none
+.Pp
+The default value is
+.Cm none .
 .It Ic set Cm optimization Ar environment
 Optimize state timeouts for one of the following network environments:
 .Pp
@@ -1275,6 +1301,9 @@ Suitable for almost all networks.
 Alias for
 .Cm high-latency .
 .El
+.Pp
+The default value is
+.Cm normal .
 .It Ic set Cm reassemble yes | no Op Cm no-df
 The
 .Cm reassemble
@@ -1292,6 +1321,8 @@ instead of being dropped;
 the reassembled packet will have the
 .Dq dont-fragment
 bit cleared.
+The default value is
+.Cm yes .
 .It Ic set Cm ruleset-optimization Ar level
 .Bl -tag -width profile -compact
 .It Cm basic
@@ -1341,6 +1372,7 @@ packet filtering is not desired and can have unexpected 
effects.
 .Ar ifspec
 is only evaluated when the ruleset is loaded; interfaces created
 later will not be skipped.
+PF filters traffic on all interfaces by default.
 .It Ic set Cm state-defaults Ar state-option , ...
 The
 .Cm state-defaults
@@ -1393,12 +1425,13 @@ set syncookies adaptive (start 25%, end 12%)
 .It Ic set Cm timeout Ar variable value
 .Bl -tag -width "src.track" -compact
 .It Cm frag
-Seconds before an unassembled fragment is expired.
+Seconds before an unassembled fragment is expired (60 by default).
 .It Cm interval
-Interval between purging expired states and fragments.
+Interval between purging expired states and fragments (10 seconds by default).
 .It Cm src.track
 Length of time to retain a source tracking entry after the last state
-expires.
+expires (0 by default, which means there is no global limit.
+The value is

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-30 Thread Jason McIntyre 
On Tue, Apr 30, 2019 at 02:02:12PM +0200, Ingo Schwarze wrote:
> 
> > or .Li instead of .Dq maybe
> 
> Please don't.  We just deprecated .Li (to simplify the language,
> such that authors need to learn fewer macros) and mdoc(7) now says:
> 
>Li word ...
>  Request a typewriter (literal) font.  Deprecated because on terminal
>  output devices, this is usually indistinguishable from normal text.
>  For literal displays, use Ql (in-line), Dl (single line), or Bd
>  -literal (multi-line) instead.
> 

oops, i forgot about that!

also, ingo is right about my Sx change being incorrect. please follow
his suggestion.

jmc

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-30 Thread Ingo Schwarze 
Hi Alexandr,

here are a few additional minor remarks...

Alexandr Nedvedicky wrote on Mon, Apr 29, 2019 at 08:59:55PM +0200:

> diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
> index b7e941991ba..5e2c57f6bc2 100644
> --- a/sbin/pfctl/pfctl.8
> +++ b/sbin/pfctl/pfctl.8
> @@ -198,7 +198,11 @@ Flush the tables.
>  .It Fl F Cm osfp
>  Flush the passive operating system fingerprints.
>  .It Fl F Cm Reset
> -Reset limits, timeouts and options back to default settings.
> +Reset limits, timeouts and other options back to default settings.
> +See
> +.Xr pf.conf 5
> +.Sx 'OPTIONS'
> +for details.

I know that jmc@ suggested this, but unfortunately, it's incorrect.
You cannot use .Sx to refer to a section in a *different* manual page.
What the above code creates is a link to a section in the pfctl(8)
manual page itself that would have to start with

  .Sh 'OPTIONS'

and that doesn't exist, so it results in a dead link.

Besides, kn@ already mentioned that combining .Sx with quoting is
not a good idea.

I would simply write:

  See the OPTIONS section in
  .Xr pf.conf 5
  for details.

> diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
> index 247ceef40a5..b043a6ed725 100644
> --- a/share/man/man5/pf.conf.5
> +++ b/share/man/man5/pf.conf.5
[...]
> @@ -1235,6 +1245,25 @@ Various limits can be combined on a single line:
>  .Bd -literal -offset indent
>  set limit { states 2, frags 2000, src-nodes 2000 }
>  .Ed
> +.Pp
> +.Xr pf 4
> +has the following defaults
> +.Bl -column table-entries PFR_KENTRY_HIWAT_SMALL platform_dependent
> +.It states Ta Dv PFSTATE_HIWAT Ta Pq 10
> +.It tables Ta Dv PFR_KTABLE_HIWAT Ta Pq 1000
> +.It table-entries Ta Dv PFR_KENTRY_HIWAT Ta Pq 20
> +.It table-entries Ta Dv PFR_KENTRY_HIWAT_SMALL Ta Pq 10
> +.It frags Ta Dv NMBCLUSTERS/32 Ta Pq platform dependent
> +.El
> +.Pp
> +.Cm NMBCLUSTERS
> +defines the total number of packets which can exist in-system at any one 
> time.
> +Refer to
> +.Sy sys/arch/`uname -p`/param.h

I'm still suggesting

  .In machine/param.h

It's easier for users to refer to installed header files than to
search the kernel source tree.

[...]
> @@ -1290,6 +1329,10 @@ instead of being dropped;
>  the reassembled packet will have the
>  .Dq dont-fragment
>  bit cleared.
> +The default value is yes.

  The default value is
  .Cm yes .

[...]
> @@ -1388,15 +1435,20 @@ The thresholds for entering and leaving syncookie 
> mode can be specified using:
>  set syncookies adaptive (start 25%, end 12%)
>  .Ed
>  .El
> +.Pp
> +.Xr pfctl 8
> +.Fl F Cm Reset
> +restores customized setting back to default.
>  .It Ic set Cm timeout Ar variable value
>  .Bl -tag -width "src.track" -compact
>  .It Cm frag
> -Seconds before an unassembled fragment is expired.
> +Seconds before an unassembled fragment is expired (60 by default).
>  .It Cm interval
> -Interval between purging expired states and fragments.
> +Interval between purging expired states and fragments (10 by default).

Maybe "(10 seconds by default)"?
Just in this one case, the others seem fine.

[...]
> @@ -1408,13 +1460,13 @@ Tuning these values may improve the performance of the
>  firewall at the risk of dropping valid idle connections.
>  .Pp
>  .Bl -tag -width Ds -compact
> -.It Cm tcp.closed
> +.It Cm tcp.closed No (90 seconds by default)

  .It Cm tcp.closed Pq 90 seconds by default

would seem simpler and more natural to me and is shorter.
Not a big deal though, your usage of .No isn't incorrect.

Yours,
  Ingo

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-30 Thread Ingo Schwarze 
Hi,

Jason McIntyre wrote on Mon, Apr 29, 2019 at 09:53:03PM +0100:

> ah, so singular is correct. but it needs an article of some sort. how
> about:
> 
>   .Xr pfctl 8
>   .Fl F Cm Reset
>   restores this value to its default.
> 
> to be honest, i don;t like it when we Xr like this, or mark up so much.
> i would go for
> 
>   .Dq pfctl -F Reset
>   restores this value to its default.

I'm OK with both forms.

Note that there is value in having "Cm Reset" in at least one place
in pf.conf.5, such that the following commands bring up pf.conf(5):

   $ man -k Cm=Reset
   $ man -k any~Reset

> or .Li instead of .Dq maybe

Please don't.  We just deprecated .Li (to simplify the language,
such that authors need to learn fewer macros) and mdoc(7) now says:

   Li word ...
 Request a typewriter (literal) font.  Deprecated because on terminal
 output devices, this is usually indistinguishable from normal text.
 For literal displays, use Ql (in-line), Dl (single line), or Bd
 -literal (multi-line) instead.

Typewriter font isn't critical in such a case, so either .Ql or .Dq
is fine (or the full markup with Xr Fl Cm).

 +.Xr pfctl 8
 +.Fl F Cm Reset
 +restores customized settings back to default.
 +.sp

>>> what's the .sp for? if you want vertical blank space, use .Pp

>> I think it was suggested by 'mandoc -T lint',

I suspect you have seen the message
  "blank line in fill mode, using .sp"
with an earlier version of your patch.  That message does *not*
intend to ask you to insert an .sp request (you should never use
.sp in a manual page, it is low level roff) - it merely explains
how a blank line (which shouldn't be there) is treated by mandoc.
I just improved the description of the message in the mandoc(1)
manual page to avoid the misunderstanding you succumbed to:

  Warnings related to plain text
blank line in fill mode, using .sp
  (mdoc) The meaning of blank input lines is only well-defined
  in non-fill mode: In fill mode, line breaks of text input
  lines are not supposed to be significant.  However, for
  compatibility with groff, blank lines in fill mode are formatted
  like sp requests.  To request a paragraph break, use Pp instead
  of a blank line.

> ok, .sp, which i always forget about, is basically roff for .Pp. just
> use .Pp if you need the vertical spacing.

Correct.

Right before .It in a non-compat list, it's redundant, though.

  (tcp.first 60, tcp.established 43200).
 +.Pp
 +.Xr pfctl 8
 +.Fl F Cm Reset
 +restores customized any timeout setting back to default.

>>> this reads weird!

>> I've fixed that to:
>>  restores any customized timeout setting back to default.

> maybe
>   restores this setting to its default value.

The sentence applies to the whole, long "set timeout variable value"
list item, so plural (settings) is needed.  Maybe:

   restores all
   .Cm timeout
   settings to their respective defaults.

Yours,
  Ingo

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-29 Thread Klemens Nanni 
On Mon, Apr 29, 2019 at 08:59:55PM +0200, Alexandr Nedvedicky wrote:
> attaching updated diff this time.
Lots of good stuff, thanks!  I'm sold on mentioning the various defaults
but not quite sure yet whether it's worthwhile to vary on
"pfctl -F Reset restores the default" over and over again (I count 7).

We could leave out those repititions and, if at all necessary, further
improve the commands description itself to clarify what is reset.

 
> +.Xr pf.conf 5
> +.Sx 'OPTIONS'
These quotes can go, the section name has no space.  This is consistent
with every usage of `Sx' in base.

> +.Pp
> +.Xr pf 4
> +has the following defaults
I'd end this line with a double colon.

> +Refer to
> +.Sy sys/arch/`uname -p`/param.h
This should be an absoloute `Pa' path even though it contains shell bits;
other manuals such as bgpd.conf(5) and bsd.port.mk(5) do it as well and
it allows semantic searches like "what mentions kernel source?" with
`man -k pa=/sys'.

Also, is `uname -p' or `uname -m'?  I think they differ at least on ARM
platforms.

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-29 Thread Jason McIntyre 
On Mon, Apr 29, 2019 at 08:55:40PM +0200, Alexandr Nedvedicky wrote:
> > > @@ -1165,6 +1168,11 @@ and
> > >  These keywords correspond to the similar (LOG_) values specified to the
> > >  .Xr syslog 3
> > >  library routine.
> > > +The default value is
> > > +.Cm err .
> > > +.Xr pfctl 8
> > > +.Fl F Cm Reset
> > > +restores customized value back to default.
> > 
> > i want to be sure this text is grammatically correct. does Reset work on
> > a number of changed settings at once? if so, then "restores customized
> > values back to their defaults" or sth like that.
> 
> in case of 'set debug' option the paragraph talks single option, which can
> be set to 7 (or so) different values.  hence I'm using single instead of
> plural. I'm keeping my version, just let me know if I should change it.
> 

ah, so singular is correct. but it needs an article of some sort. how
about:

.Xr pfctl 8
.Fl F Cm Reset
restores this value to its default.

to be honest, i don;t like it when we Xr like this, or mark up so much.
i would go for

.Dq pfctl -F Reset
restores this value to its default.

or .Li instead of .Dq maybe

> > 
> > > +.Xr pfctl 8
> > > +.Fl F Cm Reset
> > > +restores customized settings back to default.
> > > +.sp
> > 
> > what's the .sp for? if you want vertical blank space, use .Pp
> 
> I think it was suggested by 'mandoc -T lint', I've removed
> that and lint seems to be still OK.
> 

ok, .sp, which i always forget about, is basically roff for .Pp. just
use .Pp if you need the vertical spacing.

> 
> 
> > >  (tcp.first 60, tcp.established 43200).
> > > +.Pp
> > > +.Xr pfctl 8
> > > +.Fl F Cm Reset
> > > +restores customized any timeout setting back to default.
> > 
> > this reads weird!
> > 
> 
> I've fixed that to:
> 
>   restores any customized timeout setting back to default.
> 

maybe

restores this setting to its default value.

> 
> thank you for helping me to get my changes to shape. I'll chase some PF
> folks for OKs next week.
> 

cool, thanks for working on this.

jmc

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-29 Thread Alexandr Nedvedicky 
Hello,

hit sent too fast...
attaching updated diff this time.

thanks and 
regards
sashan

8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index b7e941991ba..5e2c57f6bc2 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -198,7 +198,11 @@ Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
 .It Fl F Cm Reset
-Reset limits, timeouts and options back to default settings.
+Reset limits, timeouts and other options back to default settings.
+See
+.Xr pf.conf 5
+.Sx 'OPTIONS'
+for details.
 .It Fl F Cm all
 Flush all of the above.
 .El
diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
index 247ceef40a5..b043a6ed725 100644
--- a/share/man/man5/pf.conf.5
+++ b/share/man/man5/pf.conf.5
@@ -1146,6 +1146,9 @@ A TCP RST is returned for blocked TCP packets,
 an ICMP UNREACHABLE is returned for blocked UDP packets,
 and all other packets are silently dropped.
 .El
+.Pp
+The default value is
+.Cm drop .
 .It Ic set Cm debug Ar level
 Set the debug
 .Ar level ,
@@ -1165,6 +1168,11 @@ and
 These keywords correspond to the similar (LOG_) values specified to the
 .Xr syslog 3
 library routine.
+The default value is
+.Cm err .
+.Xr pfctl 8
+.Fl F Cm Reset
+restores customized value back to default.
 .It Cm set Cm fingerprints Ar filename
 Load fingerprints of known operating systems from the given
 .Ar filename .
@@ -1175,6 +1183,8 @@ but can be overridden via this option.
 Setting this option may leave a small period of time where the fingerprints
 referenced by the currently active ruleset are inconsistent until the new
 ruleset finishes loading.
+The default location for fingerprints is
+.Pa /etc/pf.os .
 .It Ic set Cm hostid Ar number
 The 32-bit hostid
 .Ar number
@@ -1235,6 +1245,25 @@ Various limits can be combined on a single line:
 .Bd -literal -offset indent
 set limit { states 2, frags 2000, src-nodes 2000 }
 .Ed
+.Pp
+.Xr pf 4
+has the following defaults
+.Bl -column table-entries PFR_KENTRY_HIWAT_SMALL platform_dependent
+.It states Ta Dv PFSTATE_HIWAT Ta Pq 10
+.It tables Ta Dv PFR_KTABLE_HIWAT Ta Pq 1000
+.It table-entries Ta Dv PFR_KENTRY_HIWAT Ta Pq 20
+.It table-entries Ta Dv PFR_KENTRY_HIWAT_SMALL Ta Pq 10
+.It frags Ta Dv NMBCLUSTERS/32 Ta Pq platform dependent
+.El
+.Pp
+.Cm NMBCLUSTERS
+defines the total number of packets which can exist in-system at any one time.
+Refer to
+.Sy sys/arch/`uname -p`/param.h
+to find out a value for system sepcific values.
+.Xr pfctl 8
+.Fl F Cm Reset
+restores customized settings back to default.
 .It Ic set Cm loginterface Ar interface | Cm none
 Enable collection of packet and byte count statistics for the given
 interface or interface group.
@@ -1251,6 +1280,13 @@ collects statistics on the interface named dc0:
 One can disable the loginterface using:
 .Pp
 .Dl set loginterface none
+.Pp
+The default value is
+.Cm none .
+.sp
+.Xr pfctl 8
+.Fl F Cm Reset
+restores customized settings back to default.
 .It Ic set Cm optimization Ar environment
 Optimize state timeouts for one of the following network environments:
 .Pp
@@ -1273,6 +1309,9 @@ Suitable for almost all networks.
 Alias for
 .Cm high-latency .
 .El
+.Pp
+The default value is
+.Cm normal .
 .It Ic set Cm reassemble yes | no Op Cm no-df
 The
 .Cm reassemble
@@ -1290,6 +1329,10 @@ instead of being dropped;
 the reassembled packet will have the
 .Dq dont-fragment
 bit cleared.
+The default value is yes.
+.Xr pfctl 8
+.Fl F Cm Reset
+restores customized settings back to default.
 .It Ic set Cm ruleset-optimization Ar level
 .Bl -tag -width profile -compact
 .It Cm basic
@@ -1339,6 +1382,10 @@ packet filtering is not desired and can have unexpected 
effects.
 .Ar ifspec
 is only evaluated when the ruleset is loaded; interfaces created
 later will not be skipped.
+PF filters traffic on all interfaces by default.
+.Xr pfctl 8
+.Fl F Cm Reset
+makes PF filter on all interfaces again.
 .It Ic set Cm state-defaults Ar state-option , ...
 The
 .Cm state-defaults
@@ -1388,15 +1435,20 @@ The thresholds for entering and leaving syncookie mode 
can be specified using:
 set syncookies adaptive (start 25%, end 12%)
 .Ed
 .El
+.Pp
+.Xr pfctl 8
+.Fl F Cm Reset
+restores customized setting back to default.
 .It Ic set Cm timeout Ar variable value
 .Bl -tag -width "src.track" -compact
 .It Cm frag
-Seconds before an unassembled fragment is expired.
+Seconds before an unassembled fragment is expired (60 by default).
 .It Cm interval
-Interval between purging expired states and fragments.
+Interval between purging expired states and fragments (10 by default).
 .It Cm src.track
 Length of time to retain a source tracking entry after the last state
-expires.
+expires (0 by default, which means there is no global limit.
+The value is defined by rule which creates state.).
 .El
 .Pp
 When a packet matches a stateful connection, the seconds to live for the
@@ -1408,13 +1460,13 @@ Tuning

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-29 Thread Alexandr Nedvedicky 
Hello,

comments inline.

> > +section 'OPTIONS', for details.
> 
> probably just
> 
>   See
>   .Xr pf.conf.5
>   .Sx OPTIONS
>   for details.

fixed



> >  Set the debug
> >  .Ar level ,
> > @@ -1165,6 +1168,11 @@ and
> >  These keywords correspond to the similar (LOG_) values specified to the
> >  .Xr syslog 3
> >  library routine.
> > +The default value is
> > +.Cm err .
> > +.Xr pfctl 8
> > +.Fl F Cm Reset
> > +restores customized value back to default.
> 
> i want to be sure this text is grammatically correct. does Reset work on
> a number of changed settings at once? if so, then "restores customized
> values back to their defaults" or sth like that.

in case of 'set debug' option the paragraph talks single option, which can
be set to 7 (or so) different values.  hence I'm using single instead of
plural. I'm keeping my version, just let me know if I should change it.


> >  referenced by the currently active ruleset are inconsistent until the new
> >  ruleset finishes loading.
> > +The default location for fingerprints is /etc/pf.os file.
> 
>   ...is
>   .Pa /etc/pf.os .

fixed


> > +.Xr pf 4
> > +uses defaults as follows:
> 
> "has the following defaults"?

fixed


> > +.Pp
> > +.Cm NMBCLUSTERS
> > +defines the total number of packets, which can exist in system at any
> 
>   ...of packets which can exist in-system at any one time.
> 

fixed

> > +instance of the time.
> > +Refer to
> > +.Sy sys/arch/`uname -p`/param.h
> > +to find out a value for your platform.
> 
>   for system specific values.

fixed
> 
> > +.Xr pfctl 8
> > +.Fl F Cm Reset
> > +restores customized settings back to default.
> > +.sp
> 
> what's the .sp for? if you want vertical blank space, use .Pp

I think it was suggested by 'mandoc -T lint', I've removed
that and lint seems to be still OK.



> > +.Cm normal .
> >  .It Ic set Cm reassemble yes | no Op Cm no-df
> >  The
> >  .Cm reassemble
> > @@ -1290,6 +1330,10 @@ instead of being dropped;
> >  the reassembled packet will have the
> >  .Dq dont-fragment
> >  bit cleared.
> > +Default value is yes.
> 
> above you said "The default value is". here "Default value is". use one
> text. i think "The default value is" is better.

fixed


> > +PF filters traffic on all interfaces by default.
> > +.Xr pfctl 8
> > +.Fl F Cm Reset
> > +makes PF to filter on all interfaces again.
> 
> s/to//

fixed


> >  .It Cm src.track
> >  Length of time to retain a source tracking entry after the last state
> > -expires.
> > +expires (0 by default, which means there is no global limit.
> > +Value is defined by rule, which creates state).
> 
> i think this should read
> 
>   The value is defined by the rule which creates state.

fixed


> >  (tcp.first 60, tcp.established 43200).
> > +.Pp
> > +.Xr pfctl 8
> > +.Fl F Cm Reset
> > +restores customized any timeout setting back to default.
> 
> this reads weird!
> 

I've fixed that to:

restores any customized timeout setting back to default.

> >  .El
> >  .Sh QUEUEING
> >  Packets can be assigned to queues for the purpose of bandwidth
> > 
> 
> i'm fine with this direction, but oks from people who know this stuff
> would be good.

thank you for helping me to get my changes to shape. I'll chase some PF
folks for OKs next week.

regards
sashan

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-29 Thread Jason McIntyre 
On Mon, Apr 29, 2019 at 07:48:35PM +0200, Alexandr Nedvedicky wrote:
> Hello,
> 
> sorry for extra delay.
> 
> 
> > i have to say upfront that i dislike this idea of dividing options into
> > classes and then for every option, altering the text to something
> > unwieldy like:
> > 
> > This runtime option...
> > 
> > it reads very poorly, and this page is big enough as is without fleshing
> > it out more.
> 
> thanks for that comment it helps to keep the manpage practical.
> I've killed that taxonomy (runtime vs. parser)
> 
> 
> > 
> > you've suggested another idea, which is to add an option to display the
> > defaults. so i don;t really want to dig in to your diff until i see
> > whether this stuff is going in or not. but i think if it does, i'd like
> > to find another way to do it.
> 
> let's forget that idea at least for now. I feel the plan is
> controversial. we can always revive that idea later.
> 
> 
> > 
> > another possibility would be to just add a text such as "Can be Reset".
> 
> that's the way I've opted for in my next diff below.
> 
> thanks and
> regards
> sashan
> 

hi.

this looks better, i think. i've inlined some comments on your text.
jmc

> 8<---8<---8<--8<
> diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
> index b7e941991ba..064308548d9 100644
> --- a/sbin/pfctl/pfctl.8
> +++ b/sbin/pfctl/pfctl.8
> @@ -198,7 +198,10 @@ Flush the tables.
>  .It Fl F Cm osfp
>  Flush the passive operating system fingerprints.
>  .It Fl F Cm Reset
> -Reset limits, timeouts and options back to default settings.
> +Reset limits, timeouts and other options back to default settings.
> +See
> +.Xr pf.conf 5 ,
> +section 'OPTIONS', for details.

probably just

See
.Xr pf.conf.5
.Sx OPTIONS
for details.

>  .It Fl F Cm all
>  Flush all of the above.
>  .El
> diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
> index 247ceef40a5..7b9f2356271 100644
> --- a/share/man/man5/pf.conf.5
> +++ b/share/man/man5/pf.conf.5
> @@ -1146,6 +1146,9 @@ A TCP RST is returned for blocked TCP packets,
>  an ICMP UNREACHABLE is returned for blocked UDP packets,
>  and all other packets are silently dropped.
>  .El
> +.Pp
> +The default value is
> +.Cm drop .
>  .It Ic set Cm debug Ar level
>  Set the debug
>  .Ar level ,
> @@ -1165,6 +1168,11 @@ and
>  These keywords correspond to the similar (LOG_) values specified to the
>  .Xr syslog 3
>  library routine.
> +The default value is
> +.Cm err .
> +.Xr pfctl 8
> +.Fl F Cm Reset
> +restores customized value back to default.

i want to be sure this text is grammatically correct. does Reset work on
a number of changed settings at once? if so, then "restores customized
values back to their defaults" or sth like that.

>  .It Cm set Cm fingerprints Ar filename
>  Load fingerprints of known operating systems from the given
>  .Ar filename .
> @@ -1175,6 +1183,7 @@ but can be overridden via this option.
>  Setting this option may leave a small period of time where the fingerprints
>  referenced by the currently active ruleset are inconsistent until the new
>  ruleset finishes loading.
> +The default location for fingerprints is /etc/pf.os file.

...is
.Pa /etc/pf.os .

>  .It Ic set Cm hostid Ar number
>  The 32-bit hostid
>  .Ar number
> @@ -1235,6 +1244,27 @@ Various limits can be combined on a single line:
>  .Bd -literal -offset indent
>  set limit { states 2, frags 2000, src-nodes 2000 }
>  .Ed
> +.Pp
> +.Xr pf 4
> +uses defaults as follows:

"has the following defaults"?

> +.Bl -column table-entries PFR_KENTRY_HIWAT_SMALL platform_dependent
> +.It states Ta Dv PFSTATE_HIWAT Ta Pq 10
> +.It tables Ta Dv PFR_KTABLE_HIWAT Ta Pq 1000
> +.It table-entries Ta Dv PFR_KENTRY_HIWAT Ta Pq 20
> +.It table-entries Ta Dv PFR_KENTRY_HIWAT_SMALL Ta Pq 10
> +.It frags Ta Dv NMBCLUSTERS/32 Ta Pq platform dependent
> +.El
> +.Pp
> +.Cm NMBCLUSTERS
> +defines the total number of packets, which can exist in system at any

...of packets which can exist in-system at any one time.

> +instance of the time.
> +Refer to
> +.Sy sys/arch/`uname -p`/param.h
> +to find out a value for your platform.

for system specific values.

> +.Xr pfctl 8
> +.Fl F Cm Reset
> +restores customized settings back to default.
> +.sp

what's the .sp for? if you want vertical blank space, use .Pp

>  .It Ic set Cm loginterface Ar interface | Cm none
>  Enable collection of packet and byte count statistics for the given
>  interface or interface group.
> @@ -1251,6 +1281,13 @@ collects statistics on the interface named dc0:
>  One can disable the loginterface using:
>  .Pp
>  .Dl set loginterface none
> +.Pp
> +The default value is
> +.Cm none .
> +.sp
> +.Xr pfctl 8
> +.Fl F Cm Reset
> +restores customized settings back to default.
>  .It Ic set Cm optimization Ar environment
>  Optimize state timeouts for one of the following network environments:
>

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-29 Thread Alexandr Nedvedicky 
Hello,

sorry for extra delay.


> i have to say upfront that i dislike this idea of dividing options into
> classes and then for every option, altering the text to something
> unwieldy like:
> 
>   This runtime option...
> 
> it reads very poorly, and this page is big enough as is without fleshing
> it out more.

thanks for that comment it helps to keep the manpage practical.
I've killed that taxonomy (runtime vs. parser)


> 
> you've suggested another idea, which is to add an option to display the
> defaults. so i don;t really want to dig in to your diff until i see
> whether this stuff is going in or not. but i think if it does, i'd like
> to find another way to do it.

let's forget that idea at least for now. I feel the plan is
controversial. we can always revive that idea later.


> 
> another possibility would be to just add a text such as "Can be Reset".

that's the way I've opted for in my next diff below.

thanks and
regards
sashan

8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index b7e941991ba..064308548d9 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -198,7 +198,10 @@ Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
 .It Fl F Cm Reset
-Reset limits, timeouts and options back to default settings.
+Reset limits, timeouts and other options back to default settings.
+See
+.Xr pf.conf 5 ,
+section 'OPTIONS', for details.
 .It Fl F Cm all
 Flush all of the above.
 .El
diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
index 247ceef40a5..7b9f2356271 100644
--- a/share/man/man5/pf.conf.5
+++ b/share/man/man5/pf.conf.5
@@ -1146,6 +1146,9 @@ A TCP RST is returned for blocked TCP packets,
 an ICMP UNREACHABLE is returned for blocked UDP packets,
 and all other packets are silently dropped.
 .El
+.Pp
+The default value is
+.Cm drop .
 .It Ic set Cm debug Ar level
 Set the debug
 .Ar level ,
@@ -1165,6 +1168,11 @@ and
 These keywords correspond to the similar (LOG_) values specified to the
 .Xr syslog 3
 library routine.
+The default value is
+.Cm err .
+.Xr pfctl 8
+.Fl F Cm Reset
+restores customized value back to default.
 .It Cm set Cm fingerprints Ar filename
 Load fingerprints of known operating systems from the given
 .Ar filename .
@@ -1175,6 +1183,7 @@ but can be overridden via this option.
 Setting this option may leave a small period of time where the fingerprints
 referenced by the currently active ruleset are inconsistent until the new
 ruleset finishes loading.
+The default location for fingerprints is /etc/pf.os file.
 .It Ic set Cm hostid Ar number
 The 32-bit hostid
 .Ar number
@@ -1235,6 +1244,27 @@ Various limits can be combined on a single line:
 .Bd -literal -offset indent
 set limit { states 2, frags 2000, src-nodes 2000 }
 .Ed
+.Pp
+.Xr pf 4
+uses defaults as follows:
+.Bl -column table-entries PFR_KENTRY_HIWAT_SMALL platform_dependent
+.It states Ta Dv PFSTATE_HIWAT Ta Pq 10
+.It tables Ta Dv PFR_KTABLE_HIWAT Ta Pq 1000
+.It table-entries Ta Dv PFR_KENTRY_HIWAT Ta Pq 20
+.It table-entries Ta Dv PFR_KENTRY_HIWAT_SMALL Ta Pq 10
+.It frags Ta Dv NMBCLUSTERS/32 Ta Pq platform dependent
+.El
+.Pp
+.Cm NMBCLUSTERS
+defines the total number of packets, which can exist in system at any
+instance of the time.
+Refer to
+.Sy sys/arch/`uname -p`/param.h
+to find out a value for your platform.
+.Xr pfctl 8
+.Fl F Cm Reset
+restores customized settings back to default.
+.sp
 .It Ic set Cm loginterface Ar interface | Cm none
 Enable collection of packet and byte count statistics for the given
 interface or interface group.
@@ -1251,6 +1281,13 @@ collects statistics on the interface named dc0:
 One can disable the loginterface using:
 .Pp
 .Dl set loginterface none
+.Pp
+The default value is
+.Cm none .
+.sp
+.Xr pfctl 8
+.Fl F Cm Reset
+restores customized settings back to default.
 .It Ic set Cm optimization Ar environment
 Optimize state timeouts for one of the following network environments:
 .Pp
@@ -1273,6 +1310,9 @@ Suitable for almost all networks.
 Alias for
 .Cm high-latency .
 .El
+.Pp
+The default value is
+.Cm normal .
 .It Ic set Cm reassemble yes | no Op Cm no-df
 The
 .Cm reassemble
@@ -1290,6 +1330,10 @@ instead of being dropped;
 the reassembled packet will have the
 .Dq dont-fragment
 bit cleared.
+Default value is yes.
+.Xr pfctl 8
+.Fl F Cm Reset
+restores customized settings back to default.
 .It Ic set Cm ruleset-optimization Ar level
 .Bl -tag -width profile -compact
 .It Cm basic
@@ -1339,6 +1383,10 @@ packet filtering is not desired and can have unexpected 
effects.
 .Ar ifspec
 is only evaluated when the ruleset is loaded; interfaces created
 later will not be skipped.
+PF filters traffic on all interfaces by default.
+.Xr pfctl 8
+.Fl F Cm Reset
+makes PF to filter on all interfaces again.
 .It Ic set Cm state-defaults Ar state-option , ...
 The
 .Cm state-defaults
@@ -1388,15 +1436,20 @@

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-17 Thread Jason McIntyre 
On Thu, Apr 18, 2019 at 02:40:09AM +0200, Alexandr Nedvedicky wrote:
> Hello Ingo,
> 
> thank you for all your suggestions. I've accepted all of them.
> updated diff is below.
> 
> let me just share some thoughts and clarifications here.
> 

morning.

i have to say upfront that i dislike this idea of dividing options into
classes and then for every option, altering the text to something
unwieldy like:

This runtime option...

it reads very poorly, and this page is big enough as is without fleshing
it out more.

on the other hand, i do like that you are concretely documenting
defaults. even if that causes us some work, i think it's helpful enough
to justify it.

you've suggested another idea, which is to add an option to display the
defaults. so i don;t really want to dig in to your diff until i see
whether this stuff is going in or not. but i think if it does, i'd like
to find another way to do it.

one possibility is to not make anyone worry about what kind of option
they are dealing with, and just list in the description of Reset exactly
what is affected. i admit i don;t know if that is practical.

another possibility would be to just add a text such as "Can be Reset".

jmc

> 
> > 
> > I don't feel strongly about mentioning the defaults either way.
> > But i tend to think that if something is important enough to provide
> > users with a knob to tweak it, then they will probably need to know what
> 
> I have a same feeling about having defaults in manpage. It's useful
> for administrators, but it's pain for developers to keep them up-to-date.
> 
> I was thinking on how to address the potential 'out-of-date' problem with
> keeping manpage in sync with definitions of defaults in source code. How
> people would feel about adding yet another option to pfctl, something
> like:
> 
>   pfctl -s defaults
> 
> This would make pfctl to print all compile time defaults. The manpage will
> just contain a reference on how to quickly find them. Such information 
> will
> be always up-to-date and consistent with given platform. If we say yes to
> 'pfctl -s defaults', then I can update diff accordingly. I would just
> remove the default values from manpage and put reference to 'pfctl -s
> defaults' there (and also would extend pfctl to show them).
> 
> 
> > 
> > > +Packets passing in or out on such interfaces are passed as if pf was 
> > > disabled,
> > > +i.e. pf does not process them in any way.  This can be useful on 
> > > loopback and
> > > +other virtual interfaces, when packet filtering is not desired and can 
> > > have
> > > +unexpected effects.
> > 
> > Why are you changing these four lines?
> > It seems to me you are only making lines too long and violating
> > the rule "new sentence, new line".
> 
> I believe I let vim to format the lines for me. I've adjusted the diff
> to minimize divergence from cvs tree. Thanks for pointing that out.
> 
> thanks and
> regards
> sashan
> 
> 8<---8<---8<--8<
> diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
> index b7e941991ba..aa24a5de561 100644
> --- a/sbin/pfctl/pfctl.8
> +++ b/sbin/pfctl/pfctl.8
> @@ -198,7 +198,7 @@ Flush the tables.
>  .It Fl F Cm osfp
>  Flush the passive operating system fingerprints.
>  .It Fl F Cm Reset
> -Reset limits, timeouts and options back to default settings.
> +Reset limits, timeouts and other runtime options back to default settings.
>  .It Fl F Cm all
>  Flush all of the above.
>  .El
> diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
> index 247ceef40a5..96bef3e020f 100644
> --- a/share/man/man5/pf.conf.5
> +++ b/share/man/man5/pf.conf.5
> @@ -1130,11 +1130,23 @@ can be used.
>  may be tuned for various situations using the
>  .Ic set
>  command.
> +There are two kinds of options:
> +.Em Runtime
> +options, which define parameters for the
> +.Xr pf 4
> +driver and
> +.Em parser
> +options, which fine-tune interpretation of rules, while
> +they are being loaded from the file.
> +The runtime options may be restored to their default values using the
> +.Xr pfctl 8
> +.Fl F Cm Reset
> +option.
>  .Bl -tag -width Ds
>  .It Ic set Cm block-policy drop | return
>  The
>  .Cm block-policy
> -option sets the default behaviour for the packet
> +parser option sets the default behaviour for the packet
>  .Ic block
>  action:
>  .Pp
> @@ -1146,8 +1158,13 @@ A TCP RST is returned for blocked TCP packets,
>  an ICMP UNREACHABLE is returned for blocked UDP packets,
>  and all other packets are silently dropped.
>  .El
> +.Pp
> +The default value is
> +.Cm drop .
>  .It Ic set Cm debug Ar level
> -Set the debug
> +The
> +.Cm debug
> +runtime option sets the debug 
>  .Ar level ,
>  which limits the severity of log messages printed by
>  .Xr pf 4 .
> @@ -1165,8 +1182,11 @@ and
>  These keywords correspond to the similar (LOG_) values specified to the
>  .Xr syslog 3
>  library routine.
> +The default value is
>

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-17 Thread Alexandr Nedvedicky 
Hello Ingo,

thank you for all your suggestions. I've accepted all of them.
updated diff is below.

let me just share some thoughts and clarifications here.


> 
> I don't feel strongly about mentioning the defaults either way.
> But i tend to think that if something is important enough to provide
> users with a knob to tweak it, then they will probably need to know what

I have a same feeling about having defaults in manpage. It's useful
for administrators, but it's pain for developers to keep them up-to-date.

I was thinking on how to address the potential 'out-of-date' problem with
keeping manpage in sync with definitions of defaults in source code. How
people would feel about adding yet another option to pfctl, something
like:

pfctl -s defaults

This would make pfctl to print all compile time defaults. The manpage will
just contain a reference on how to quickly find them. Such information will
be always up-to-date and consistent with given platform. If we say yes to
'pfctl -s defaults', then I can update diff accordingly. I would just
remove the default values from manpage and put reference to 'pfctl -s
defaults' there (and also would extend pfctl to show them).


> 
> > +Packets passing in or out on such interfaces are passed as if pf was 
> > disabled,
> > +i.e. pf does not process them in any way.  This can be useful on loopback 
> > and
> > +other virtual interfaces, when packet filtering is not desired and can have
> > +unexpected effects.
> 
> Why are you changing these four lines?
> It seems to me you are only making lines too long and violating
> the rule "new sentence, new line".

I believe I let vim to format the lines for me. I've adjusted the diff
to minimize divergence from cvs tree. Thanks for pointing that out.

thanks and
regards
sashan

8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index b7e941991ba..aa24a5de561 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -198,7 +198,7 @@ Flush the tables.
 .It Fl F Cm osfp
 Flush the passive operating system fingerprints.
 .It Fl F Cm Reset
-Reset limits, timeouts and options back to default settings.
+Reset limits, timeouts and other runtime options back to default settings.
 .It Fl F Cm all
 Flush all of the above.
 .El
diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
index 247ceef40a5..96bef3e020f 100644
--- a/share/man/man5/pf.conf.5
+++ b/share/man/man5/pf.conf.5
@@ -1130,11 +1130,23 @@ can be used.
 may be tuned for various situations using the
 .Ic set
 command.
+There are two kinds of options:
+.Em Runtime
+options, which define parameters for the
+.Xr pf 4
+driver and
+.Em parser
+options, which fine-tune interpretation of rules, while
+they are being loaded from the file.
+The runtime options may be restored to their default values using the
+.Xr pfctl 8
+.Fl F Cm Reset
+option.
 .Bl -tag -width Ds
 .It Ic set Cm block-policy drop | return
 The
 .Cm block-policy
-option sets the default behaviour for the packet
+parser option sets the default behaviour for the packet
 .Ic block
 action:
 .Pp
@@ -1146,8 +1158,13 @@ A TCP RST is returned for blocked TCP packets,
 an ICMP UNREACHABLE is returned for blocked UDP packets,
 and all other packets are silently dropped.
 .El
+.Pp
+The default value is
+.Cm drop .
 .It Ic set Cm debug Ar level
-Set the debug
+The
+.Cm debug
+runtime option sets the debug 
 .Ar level ,
 which limits the severity of log messages printed by
 .Xr pf 4 .
@@ -1165,8 +1182,11 @@ and
 These keywords correspond to the similar (LOG_) values specified to the
 .Xr syslog 3
 library routine.
+The default value is
+.Cm err .
 .It Cm set Cm fingerprints Ar filename
-Load fingerprints of known operating systems from the given
+This parser option loads fingerprints of known operating systems
+from the given
 .Ar filename .
 By default fingerprints of known operating systems are automatically
 loaded from
@@ -1175,10 +1195,11 @@ but can be overridden via this option.
 Setting this option may leave a small period of time where the fingerprints
 referenced by the currently active ruleset are inconsistent until the new
 ruleset finishes loading.
+The default location for fingerprints is /etc/pf.os file.
 .It Ic set Cm hostid Ar number
-The 32-bit hostid
-.Ar number
-identifies this firewall's state table entries to other firewalls
+This runtime option specifies a 32-bit hostid
+.Ar number ,
+which identifies this firewall's state table entries to other firewalls
 in a
 .Xr pfsync 4
 failover cluster.
@@ -1186,11 +1207,18 @@ By default the hostid is set to a pseudo-random value, 
however it may be
 desirable to manually configure it, for example to more easily identify the
 source of state table entries.
 The hostid may be specified in either decimal or hexadecimal.
+The
+.Cm hostid
+option value does not get changed by
+.Xr pfctl 8
+.Fl F
+.Cm Reset .
 .It Ic set

Re: update to PF pfctl(8) and pf.conf(5) manpages

2019-04-16 Thread Ingo Schwarze 
Hi Alexandr,

Alexandr Nedvedicky wrote on Wed, Apr 17, 2019 at 12:09:10AM +0200:

> my oracle fellow pointed out [1] a PF documentation can be improved
> a bit, when it comes to newly introduced 'pfctl -FR' (a reset flush
> modifier).  I've decided to make manpage changes in separate diff as
> I expect some discussion on how much detailed the manpage should be.
> The diff here is my proposal.

First off, thank you for working on the pf.conf(5) manual page.
I appreciate that very much.  To me, the pf.conf(5) manual page
feels like a manual page of considerably above-average importance,
yet i have often felt it could probably be improved.  Improving it
isn't easy though: the manual is long and complex, and the subject
matter is more complicated than for most other manual pages.
So this is a place where expert attention from somebody who
actively works on the code is particularly helpful.

[...]
> From 'pfctl -FR' perspective the most important is to document whether
> particular option is parameter for PF driver (a.k.a. runtime option)
> or is just interpreted by parser as rules are being loaded.

> Documenting default values is just bonus, which I think might be
> useful for administrators.  On the other hand it adds more work to
> keep pf.conf(5) up-to-date when doing changes in code.
> I full understand the trade here between being precise and up-to-date.

I don't feel strongly about mentioning the defaults either way.
But i tend to think that if something is important enough to provide
users with a knob to tweak it, then they will probably need to know what
the default is before they can even start to figure out whether
they might need to tweak it.  So my guess would be that every time
people read the description of the option, they will also profit
from seeing the default right away.

If others feel the risk is too high that these defaults might get
outdated in the manual page, is there a central place in the code
you can point readers to for looking up the defaults?  If so,
pointing to that place might be a viable compromise.  If no such
place exists (which might imply the defaults are scattered / hard
to find in the code?) that would strengthen the argument to document
them.

The below nits are not objections, nor conditions, nor reproaches.
You are welcome to incorporate them; then again, such details can also
be fixed after commit when needed.  The main job, in particular in
a tricky page like this, is getting the content correct!


[...]
> diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
> index 247ceef40a5..ab6b8c0447e 100644
> --- a/share/man/man5/pf.conf.5
> +++ b/share/man/man5/pf.conf.5
> @@ -1129,12 +1129,24 @@ can be used.
>  .Xr pf 4
>  may be tuned for various situations using the
>  .Ic set
> -command.
> +command. Two sorts of options should be distinguished.

New sentence, new line: this needs to be

   command.
  +Two sorts...

The same small problem occurs again below, several times.
I'm not annotating all the instances.  For spotting such trivial
mistakes,

  mandoc -T lint pf.conf.5

can be useful.

The word "should" is often somewhat confusing because it can leave
doubt whether something is a syntax requirement, or a stylistic
recommendation, or even a plan to change the code and the rules
in the future.

I'd prefer stating facts, for example:

   command.
  +There are two kinds of options:
  +.Em runtime

> +.Em runtime

A capital letter would be needed at the beginning of the sentence,
but instead, i'd prefer the colon as shown above.

> +options, which define parameters for 

The word "the" is missing here.

> +.Xr pf 4
> +driver and

  +driver, and

> +.Em parser
> +options, which fine-tune interpretation of rules, while
> +they are being loaded from file. The runtime options

s/from file/from the file/

... and again, new sentence, new line.

> +may be restored to their default values using:
> +.Pp
> +.Dl # pfctl -FReset
> +.Pp
> + 

Please show a blank between an option and its argument, unless
the syntax *requires* that there be no blank.

Blank lines are not allowed in mdoc(7), except inside .Bd -unfilled,
and .Pp is needless before .Bl without -compact, so:

  +may be restored to their default values using:
  +.Pp
  +.Dl # pfctl -F Reset
   .Bl -tag -width Ds

Alternatively, you could say, interrupting the text flow less:

  +may be restored to their default values using the
  +.Xr pfctl 8
  +.Fl F Cm Reset
  option.

>  .Bl -tag -width Ds
>  .It Ic set Cm block-policy drop | return
>  The
>  .Cm block-policy
> -option sets the default behaviour for the packet
> +parser option sets the default behaviour for the packet

That's indeed a nice, concise way to make the distinction.

>  .Ic block
>  action:
>  .Pp
> @@ -1146,8 +1158,13 @@ A TCP RST is returned for blocked TCP packets,
>  an ICMP UNREACHABLE is returned for blocked UDP packets,
>  and all other packets are silently dropped.
>  .El
> +.Pp
> +The default value is
> +.Cm drop .
>  .It Ic set Cm