Bug 32953
| Summary: | log-in-vain level should be setable in rc.conf | ||
|---|---|---|---|
| Product: | Base System | Reporter: | David O'Brien <obrien> |
| Component: | bin | Assignee: | Crist J. Clark <cjc> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | ||
| Priority: | Normal | ||
| Version: | 5.0-CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
On Mon, Dec 17, 2001 at 04:28:07PM -0800, David O'Brien wrote: > >Description: > We allow the turning off and on of log-in-vain in rc.conf as seen in > rc.network: > > network_pass4() { > echo -n 'Additional TCP options:' > case ${log_in_vain} in > > Therefor we should also allow the log-in-vain level to be set in > rc.conf also. [snip] We should. How about this simple change which is back compaitible, but does not add more rc.conf knobs: Index: rc.network =================================================================== RCS file: /export/ncvs/src/etc/rc.network,v retrieving revision 1.119 diff -u -r1.119 rc.network --- rc.network 13 Dec 2001 04:21:18 -0000 1.119 +++ rc.network 18 Dec 2001 01:26:07 -0000 @@ -366,7 +366,7 @@ case ${firewall_logging} in [Yy][Ee][Ss] | '') echo 'Firewall logging=YES' - sysctl net.inet.ip.fw.verbose=1 >/dev/null + sysctl net.inet.ip.fw.verbose="${firewall_verbose:-1}" >/dev/null ;; *) ;; @@ -848,9 +848,12 @@ [Nn][Oo] | '') ;; *) - echo -n ' log_in_vain=YES' - sysctl net.inet.tcp.log_in_vain=1 >/dev/null - sysctl net.inet.udp.log_in_vain=1 >/dev/null + if ! expr "${log_in_vain}" : '[0-9]*' >/dev/null 2>&1; then + log_in_vain=1 + fi + echo -n " log_in_vain=${log_in_vain}" + sysctl net.inet.tcp.log_in_vain="${log_in_vain}" >/dev/null + sysctl net.inet.udp.log_in_vain="${log_in_vain}" >/dev/null ;; esac That is, if log_in_vain holds an integer value, use it. If it is anything else, revert to the old behavior of setting it to 1. -- "It's always funny until someone gets hurt. Then it's hilarious." Crist J. Clark | cjclark@alum.mit.edu | cjclark@jhu.edu http://people.freebsd.org/~cjc/ | cjc@freebsd.org On Mon, Dec 17, 2001 at 05:40:02PM -0800, Crist J . Clark wrote: > The following reply was made to PR bin/32953; it has been noted by GNATS. > > From: "Crist J . Clark" <cjc@FreeBSD.ORG> > To: "David O'Brien" <obrien@NUXI.com> > Cc: FreeBSD-gnats-submit@FreeBSD.ORG > Subject: Re: bin/32953: log-in-vain level should be setable in rc.conf > Date: Mon, 17 Dec 2001 17:29:57 -0800 > > On Mon, Dec 17, 2001 at 04:28:07PM -0800, David O'Brien wrote: > > > >Description: > > We allow the turning off and on of log-in-vain in rc.conf as seen in > > rc.network: > > > > network_pass4() { > > echo -n 'Additional TCP options:' > > case ${log_in_vain} in > > > > Therefor we should also allow the log-in-vain level to be set in > > rc.conf also. > [snip] > > We should. How about this simple change which is back compaitible, but > does not add more rc.conf knobs: > > Index: rc.network > =================================================================== > RCS file: /export/ncvs/src/etc/rc.network,v > retrieving revision 1.119 > diff -u -r1.119 rc.network > --- rc.network 13 Dec 2001 04:21:18 -0000 1.119 > +++ rc.network 18 Dec 2001 01:26:07 -0000 > @@ -366,7 +366,7 @@ > case ${firewall_logging} in > [Yy][Ee][Ss] | '') > echo 'Firewall logging=YES' > - sysctl net.inet.ip.fw.verbose=1 >/dev/null > + sysctl net.inet.ip.fw.verbose="${firewall_verbose:-1}" >/dev/null > ;; > *) > ;; Is this a stray change? fw_verbose is boolean, at least in my version of ip_fw.c. > @@ -848,9 +848,12 @@ > [Nn][Oo] | '') > ;; > *) > - echo -n ' log_in_vain=YES' > - sysctl net.inet.tcp.log_in_vain=1 >/dev/null > - sysctl net.inet.udp.log_in_vain=1 >/dev/null > + if ! expr "${log_in_vain}" : '[0-9]*' >/dev/null 2>&1; then > + log_in_vain=1 > + fi > + echo -n " log_in_vain=${log_in_vain}" > + sysctl net.inet.tcp.log_in_vain="${log_in_vain}" >/dev/null > + sysctl net.inet.udp.log_in_vain="${log_in_vain}" >/dev/null > ;; > esac > I think you should add a case for [Yy][Ee][Ss], and treat the fallback case as a numeric value, without expr(1)-checking it. There's nothing wrong here as the only documented values were YES and NO, and people who had it set to the other value (e.g. "YUP") will now have to fix it to the right value. :-) Cheers, -- Ruslan Ermilov Oracle Developer/DBA, ru@sunbay.com Sunbay Software AG, ru@FreeBSD.org FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age On Tue, Dec 18, 2001 at 05:00:03AM -0800, Ruslan Ermilov wrote:
> On Mon, Dec 17, 2001 at 05:40:02PM -0800, Crist J . Clark wrote:
> > The following reply was made to PR bin/32953; it has been noted by GNATS.
> > On Mon, Dec 17, 2001 at 04:28:07PM -0800, David O'Brien wrote:
> >
> > > >Description:
> > > We allow the turning off and on of log-in-vain in rc.conf as seen in
> > > rc.network:
> > >
> > > network_pass4() {
> > > echo -n 'Additional TCP options:'
> > > case ${log_in_vain} in
> > >
> > > Therefor we should also allow the log-in-vain level to be set in
> > > rc.conf also.
> > [snip]
> >
> > We should. How about this simple change which is back compaitible, but
> > does not add more rc.conf knobs:
> >
> > Index: rc.network
> > ===================================================================
> > RCS file: /export/ncvs/src/etc/rc.network,v
> > retrieving revision 1.119
> > diff -u -r1.119 rc.network
> > --- rc.network 13 Dec 2001 04:21:18 -0000 1.119
> > +++ rc.network 18 Dec 2001 01:26:07 -0000
> > @@ -848,9 +848,12 @@
> > [Nn][Oo] | '')
> > ;;
> > *)
> > - echo -n ' log_in_vain=YES'
> > - sysctl net.inet.tcp.log_in_vain=1 >/dev/null
> > - sysctl net.inet.udp.log_in_vain=1 >/dev/null
> > + if ! expr "${log_in_vain}" : '[0-9]*' >/dev/null 2>&1; then
> > + log_in_vain=1
> > + fi
> > + echo -n " log_in_vain=${log_in_vain}"
> > + sysctl net.inet.tcp.log_in_vain="${log_in_vain}" >/dev/null
> > + sysctl net.inet.udp.log_in_vain="${log_in_vain}" >/dev/null
> > ;;
> > esac
> >
> I think you should add a case for [Yy][Ee][Ss], and treat the fallback
> case as a numeric value, without expr(1)-checking it. There's nothing
> wrong here as the only documented values were YES and NO, and people
> who had it set to the other value (e.g. "YUP") will now have to fix it
> to the right value. :-)
Why would they have to fix it? This patch treats all non-numeric values
as a default log level of 1; a "YUP" would be treated just the same
as a "YES".
I personally would prefer the expr regexp to be '[0-9]*$', though,
because '[0-9]*' also catches things like '3foo', which is, strictly
speaking, incorrect, although sysctl(8) silently trims at the first
non-numeric character.
G'luck,
Peter
--
What would this sentence be like if it weren't self-referential?
On Wed, Dec 19, 2001 at 03:46:39PM +0200, Peter Pentchev wrote: > On Tue, Dec 18, 2001 at 05:00:03AM -0800, Ruslan Ermilov wrote: > > On Mon, Dec 17, 2001 at 05:40:02PM -0800, Crist J . Clark wrote: > > > The following reply was made to PR bin/32953; it has been noted by GNATS. > > > On Mon, Dec 17, 2001 at 04:28:07PM -0800, David O'Brien wrote: > > > > > > > >Description: > > > > We allow the turning off and on of log-in-vain in rc.conf as seen in > > > > rc.network: > > > > > > > > network_pass4() { > > > > echo -n 'Additional TCP options:' > > > > case ${log_in_vain} in > > > > > > > > Therefor we should also allow the log-in-vain level to be set in > > > > rc.conf also. > > > [snip] > > > > > > We should. How about this simple change which is back compaitible, but > > > does not add more rc.conf knobs: > > > > > > Index: rc.network > > > =================================================================== > > > RCS file: /export/ncvs/src/etc/rc.network,v > > > retrieving revision 1.119 > > > diff -u -r1.119 rc.network > > > --- rc.network 13 Dec 2001 04:21:18 -0000 1.119 > > > +++ rc.network 18 Dec 2001 01:26:07 -0000 > > > @@ -848,9 +848,12 @@ > > > [Nn][Oo] | '') > > > ;; > > > *) > > > - echo -n ' log_in_vain=YES' > > > - sysctl net.inet.tcp.log_in_vain=1 >/dev/null > > > - sysctl net.inet.udp.log_in_vain=1 >/dev/null > > > + if ! expr "${log_in_vain}" : '[0-9]*' >/dev/null 2>&1; then > > > + log_in_vain=1 > > > + fi > > > + echo -n " log_in_vain=${log_in_vain}" > > > + sysctl net.inet.tcp.log_in_vain="${log_in_vain}" >/dev/null > > > + sysctl net.inet.udp.log_in_vain="${log_in_vain}" >/dev/null > > > ;; > > > esac > > > > > I think you should add a case for [Yy][Ee][Ss], and treat the fallback > > case as a numeric value, without expr(1)-checking it. There's nothing > > wrong here as the only documented values were YES and NO, and people > > who had it set to the other value (e.g. "YUP") will now have to fix it > > to the right value. :-) > > Why would they have to fix it? This patch treats all non-numeric values > as a default log level of 1; a "YUP" would be treated just the same > as a "YES". > The only documented values are "YES" and "NO" (so far), and treating anything else as "YES" is not good, as one day we may add a new value, "MAYBE", that would mean something different from "YES". > I personally would prefer the expr regexp to be '[0-9]*$', though, > because '[0-9]*' also catches things like '3foo', which is, strictly > speaking, incorrect, although sysctl(8) silently trims at the first > non-numeric character. > Yeah, the regexp may stay (it enables the stricter checking), "YES" block should be added, and fallback block should say "invalid value". Cheers, -- Ruslan Ermilov Oracle Developer/DBA, ru@sunbay.com Sunbay Software AG, ru@FreeBSD.org FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age On Wed, Dec 19, 2001 at 04:22:07PM +0200, Ruslan Ermilov wrote:
> On Wed, Dec 19, 2001 at 03:46:39PM +0200, Peter Pentchev wrote:
> > On Tue, Dec 18, 2001 at 05:00:03AM -0800, Ruslan Ermilov wrote:
> > > On Mon, Dec 17, 2001 at 05:40:02PM -0800, Crist J . Clark wrote:
> > > > The following reply was made to PR bin/32953; it has been noted by GNATS.
> > > > On Mon, Dec 17, 2001 at 04:28:07PM -0800, David O'Brien wrote:
> > > >
> > > > > >Description:
> > > > > We allow the turning off and on of log-in-vain in rc.conf as seen in
> > > > > rc.network:
> > > > >
> > > > > network_pass4() {
> > > > > echo -n 'Additional TCP options:'
> > > > > case ${log_in_vain} in
> > > > >
> > > > > Therefor we should also allow the log-in-vain level to be set in
> > > > > rc.conf also.
> > > > [snip]
> > > >
> > > > We should. How about this simple change which is back compaitible, but
> > > > does not add more rc.conf knobs:
> > > >
> > > > Index: rc.network
> > > > ===================================================================
> > > > RCS file: /export/ncvs/src/etc/rc.network,v
> > > > retrieving revision 1.119
> > > > diff -u -r1.119 rc.network
> > > > --- rc.network 13 Dec 2001 04:21:18 -0000 1.119
> > > > +++ rc.network 18 Dec 2001 01:26:07 -0000
> > > > @@ -848,9 +848,12 @@
> > > > [Nn][Oo] | '')
> > > > ;;
> > > > *)
> > > > - echo -n ' log_in_vain=YES'
> > > > - sysctl net.inet.tcp.log_in_vain=1 >/dev/null
> > > > - sysctl net.inet.udp.log_in_vain=1 >/dev/null
> > > > + if ! expr "${log_in_vain}" : '[0-9]*' >/dev/null 2>&1; then
> > > > + log_in_vain=1
> > > > + fi
> > > > + echo -n " log_in_vain=${log_in_vain}"
> > > > + sysctl net.inet.tcp.log_in_vain="${log_in_vain}" >/dev/null
> > > > + sysctl net.inet.udp.log_in_vain="${log_in_vain}" >/dev/null
> > > > ;;
> > > > esac
> > > >
> > > I think you should add a case for [Yy][Ee][Ss], and treat the fallback
> > > case as a numeric value, without expr(1)-checking it. There's nothing
> > > wrong here as the only documented values were YES and NO, and people
> > > who had it set to the other value (e.g. "YUP") will now have to fix it
> > > to the right value. :-)
> >
> > Why would they have to fix it? This patch treats all non-numeric values
> > as a default log level of 1; a "YUP" would be treated just the same
> > as a "YES".
> >
> The only documented values are "YES" and "NO" (so far), and treating
> anything else as "YES" is not good, as one day we may add a new value,
> "MAYBE", that would mean something different from "YES".
>
> > I personally would prefer the expr regexp to be '[0-9]*$', though,
> > because '[0-9]*' also catches things like '3foo', which is, strictly
> > speaking, incorrect, although sysctl(8) silently trims at the first
> > non-numeric character.
> >
> Yeah, the regexp may stay (it enables the stricter checking),
> "YES" block should be added, and fallback block should say
> "invalid value".
Errr.. Maybe I'm not understanding you here. The whole point of
this patch is to change the setting so that it is no longer a boolean.
Are you proposing that it should stay boolean (thus making it impossible
to address David O'Brien's PR without introducing a new variable),
or simply that the case statement be separated into 'NO', 'YES', '^[0-9]*$'
and a default that produces an error message?
G'luck,
Peter
--
This inert sentence is my body, but my soul is alive, dancing in the sparks of your brain.
On Wed, Dec 19, 2001 at 04:30:28PM +0200, Peter Pentchev wrote: > On Wed, Dec 19, 2001 at 04:22:07PM +0200, Ruslan Ermilov wrote: > > On Wed, Dec 19, 2001 at 03:46:39PM +0200, Peter Pentchev wrote: > > > On Tue, Dec 18, 2001 at 05:00:03AM -0800, Ruslan Ermilov wrote: > > > > On Mon, Dec 17, 2001 at 05:40:02PM -0800, Crist J . Clark wrote: > > > > > The following reply was made to PR bin/32953; it has been noted by GNATS. > > > > > On Mon, Dec 17, 2001 at 04:28:07PM -0800, David O'Brien wrote: > > > > > > > > > > > >Description: > > > > > > We allow the turning off and on of log-in-vain in rc.conf as seen in > > > > > > rc.network: > > > > > > > > > > > > network_pass4() { > > > > > > echo -n 'Additional TCP options:' > > > > > > case ${log_in_vain} in > > > > > > > > > > > > Therefor we should also allow the log-in-vain level to be set in > > > > > > rc.conf also. > > > > > [snip] > > > > > > > > > > We should. How about this simple change which is back compaitible, but > > > > > does not add more rc.conf knobs: > > > > > > > > > > Index: rc.network > > > > > =================================================================== > > > > > RCS file: /export/ncvs/src/etc/rc.network,v > > > > > retrieving revision 1.119 > > > > > diff -u -r1.119 rc.network > > > > > --- rc.network 13 Dec 2001 04:21:18 -0000 1.119 > > > > > +++ rc.network 18 Dec 2001 01:26:07 -0000 > > > > > @@ -848,9 +848,12 @@ > > > > > [Nn][Oo] | '') > > > > > ;; > > > > > *) > > > > > - echo -n ' log_in_vain=YES' > > > > > - sysctl net.inet.tcp.log_in_vain=1 >/dev/null > > > > > - sysctl net.inet.udp.log_in_vain=1 >/dev/null > > > > > + if ! expr "${log_in_vain}" : '[0-9]*' >/dev/null 2>&1; then > > > > > + log_in_vain=1 > > > > > + fi > > > > > + echo -n " log_in_vain=${log_in_vain}" > > > > > + sysctl net.inet.tcp.log_in_vain="${log_in_vain}" >/dev/null > > > > > + sysctl net.inet.udp.log_in_vain="${log_in_vain}" >/dev/null > > > > > ;; > > > > > esac > > > > > > > > > I think you should add a case for [Yy][Ee][Ss], and treat the fallback > > > > case as a numeric value, without expr(1)-checking it. There's nothing > > > > wrong here as the only documented values were YES and NO, and people > > > > who had it set to the other value (e.g. "YUP") will now have to fix it > > > > to the right value. :-) > > > > > > Why would they have to fix it? This patch treats all non-numeric values > > > as a default log level of 1; a "YUP" would be treated just the same > > > as a "YES". > > > > > The only documented values are "YES" and "NO" (so far), and treating > > anything else as "YES" is not good, as one day we may add a new value, > > "MAYBE", that would mean something different from "YES". > > > > > I personally would prefer the expr regexp to be '[0-9]*$', though, > > > because '[0-9]*' also catches things like '3foo', which is, strictly > > > speaking, incorrect, although sysctl(8) silently trims at the first > > > non-numeric character. > > > > > Yeah, the regexp may stay (it enables the stricter checking), > > "YES" block should be added, and fallback block should say > > "invalid value". > > Errr.. Maybe I'm not understanding you here. The whole point of > this patch is to change the setting so that it is no longer a boolean. > Are you proposing that it should stay boolean (thus making it impossible > to address David O'Brien's PR without introducing a new variable), > or simply that the case statement be separated into 'NO', 'YES', '^[0-9]*$' > and a default that produces an error message? > No, I'm proposing a stricter value checking, i.e.: 1. [Yy][Ee][Ss] 2. [Nn][Oo] 3. [0-9]*$ Everything else should be considered an error in value, not "YES". Cheers, -- Ruslan Ermilov Oracle Developer/DBA, ru@sunbay.com Sunbay Software AG, ru@FreeBSD.org FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age <<On Wed, 19 Dec 2001 05:50:02 -0800 (PST), Peter Pentchev <roam@ringlet.net> said: > Why would they have to fix it? This patch treats all non-numeric values > as a default log level of 1; a "YUP" would be treated just the same > as a "YES". If I may ask a higher-level question: Should every potentially-useful sysctl(8) setting have a configuration variable in rc.conf? Is this not precisely what sysctl.conf is for? -GAWollman On Wed, Dec 19, 2001 at 11:39:00AM -0500, Garrett Wollman wrote:
> <<On Wed, 19 Dec 2001 05:50:02 -0800 (PST), Peter Pentchev <roam@ringlet.net> said:
>
> > Why would they have to fix it? This patch treats all non-numeric values
> > as a default log level of 1; a "YUP" would be treated just the same
> > as a "YES".
>
> If I may ask a higher-level question:
>
> Should every potentially-useful sysctl(8) setting have a configuration
> variable in rc.conf? Is this not precisely what sysctl.conf is for?
I was about to ask the same question in a further follow-up :)
I, too, think that this is sysctl.conf material, at least in -current.
G'luck,
Peter
--
"yields falsehood, when appended to its quotation." yields falsehood, when appended to its quotation.
On Tue, Dec 18, 2001 at 02:52:49PM +0200, Ruslan Ermilov wrote: > On Mon, Dec 17, 2001 at 05:40:02PM -0800, Crist J . Clark wrote: [snip] > > > > Index: rc.network > > =================================================================== > > RCS file: /export/ncvs/src/etc/rc.network,v > > retrieving revision 1.119 > > diff -u -r1.119 rc.network > > --- rc.network 13 Dec 2001 04:21:18 -0000 1.119 > > +++ rc.network 18 Dec 2001 01:26:07 -0000 > > @@ -366,7 +366,7 @@ > > case ${firewall_logging} in > > [Yy][Ee][Ss] | '') > > echo 'Firewall logging=YES' > > - sysctl net.inet.ip.fw.verbose=1 >/dev/null > > + sysctl net.inet.ip.fw.verbose="${firewall_verbose:-1}" >/dev/null > > ;; > > *) > > ;; > > Is this a stray change? fw_verbose is boolean, at least in my version > of ip_fw.c. Yeah, a localization. I have different verbosity levels in the firewall code here. I'll send that code along if you'd like to see it. ;) I thought I had edited that out of the patch... until I actually saw my post on -bugs. [snip] > I think you should add a case for [Yy][Ee][Ss], and treat the fallback > case as a numeric value, without expr(1)-checking it. There's nothing > wrong here as the only documented values were YES and NO, and people > who had it set to the other value (e.g. "YUP") will now have to fix it > to the right value. :-) So, I guess you're saying, "take out the safety net," Index: rc.network =================================================================== RCS file: /home/cjc/ncvs/src/etc/rc.network,v retrieving revision 1.119 diff -u -r1.119 rc.network --- rc.network 13 Dec 2001 04:21:18 -0000 1.119 +++ rc.network 19 Dec 2001 16:59:16 -0000 @@ -845,14 +845,16 @@ network_pass4() { echo -n 'Additional TCP options:' case ${log_in_vain} in + [Yy][Ee][Ss]) + log_in_vain=1;; [Nn][Oo] | '') - ;; - *) - echo -n ' log_in_vain=YES' - sysctl net.inet.tcp.log_in_vain=1 >/dev/null - sysctl net.inet.udp.log_in_vain=1 >/dev/null - ;; + log_in_vain=0;; esac + if [ ${log_in_vain} -ne 0 ]; then + echo -n " log_in_vain=${log_in_vain}" + sysctl net.inet.tcp.log_in_vain="${log_in_vain}" >/dev/null + sysctl net.inet.udp.log_in_vain="${log_in_vain}" >/dev/null + fi echo '.' network_pass4_done=YES Which will bitch like heck (but your computer won't blow up) if you don't give it a "YES," "NO," or an integer. -- "It's always funny until someone gets hurt. Then it's hilarious." Crist J. Clark | cjclark@alum.mit.edu | cjclark@jhu.edu http://people.freebsd.org/~cjc/ | cjc@freebsd.org State Changed From-To: open->analyzed 'log_in_vain' is an integer in -CURRENT. However, this is just a test before MFC. After that, remove 'log_in_vain' completely in -CURRENT; the functionality is provided by sysctl.conf(5). Responsible Changed From-To: freebsd-bugs->cjc I'll handle MFC and eventual removal of log_in_vain from -CURRENT. State Changed From-To: analyzed->closed MFC'ed. |
We allow the turning off and on of log-in-vain in rc.conf as seen in rc.network: network_pass4() { echo -n 'Additional TCP options:' case ${log_in_vain} in Therefor we should also allow the log-in-vain level to be set in rc.conf also. The following shows '2' is also supported: Value of 1: "Connection attempt to TCP %s:%d from %s:%d\n", dbuf, ntohs(th->th_dport), sbuf, ntohs(th->th_sport)); Value of 2: "Connection attempt to TCP %s:%d from %s:%d flags:0x%x\n", dbuf, ntohs(th->th_dport), sbuf, ntohs(th->th_sport), thflags);