Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 9 Aug 2018 09:19:09 -0700 (PDT)
From:      "Rodney W. Grimes" <freebsd@pdx.rh.CN85.dnsmgr.net>
To:        "Andrey V. Elsukov" <bu7cher@yandex.ru>
Cc:        rgrimes@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r337536 - head/sbin/ipfw
Message-ID:  <201808091619.w79GJ9lp018293@pdx.rh.CN85.dnsmgr.net>
In-Reply-To: <d166200c-a72a-ff29-4c79-63e71cc3c261@yandex.ru>

next in thread | previous in thread | raw e-mail | index | archive | help
> On 09.08.2018 18:48, Rodney W. Grimes wrote:
> >>> This now means -q has 2 functions, silence most commands,
> >>> and silently ignore errors on delete.
> >>>
> >>> That is a poor implementation of syntax and options.
> >>
> >> I think it makes "delete" command to have the same behavior as described
> >> for commands in "-q" description:
> > 
> > Which is yet another bug in your commit, you did not update the
> > synopsis or the description of the -q flag to include your
> > change.  Though oddly the synopsis does show delete -q, it
> > how ever does not show -q for any of the table commands.
> > 
> >>
> >> -q    Be quiet when executing the add, nat, zero, resetlog or flush
> >>       commands; (implies -f).
> > No mention of what it does on delete, does -q on delete imply -f?
> > 
> >>       This is useful when updating rulesets by
> >>       executing multiple ipfw commands in a script (e.g.,
> >>       ?sh?/etc/rc.firewall?), or by processing a file with many ipfw
> >>       rules across a remote login session.  It also stops a table add
> >>       or delete from failing if the entry already exists or is not
> >>       present.
> > 
> > That suggesting that -q is good for remote login session is
> > poor advice at best, you should redirect both standard and
> > error output to a file, depending on -q is just a loaded
> > gun waiting to go off.
> > 
> >>
> >> table add/delete commands had the same behavior, "nat" already noted in
> >> this list. What is the usage scenario do you use, where you need to fail
> >> on bad delete?
> > 
> > if [ ipfw delete ${1} ]; then
> > 	handle the missing rule
> > fi
> 
> This is mostly unneeded operation, that we wanted to avoid.
> I.e. to be able run in bath mode:
> 
> delete ${n}
> add ${n} ...

That is one use case, but any shell script worth writting
is worth writting to handle error conditions, and not being
able to handle errors while being silent is a PITA.

Though you didnt add to the already bad state of ipfw,
this certainly did not do anything that I would consider
improving that bad state.

> 
> > But more importantly you seem to be ignoring the aspect that
> > your overloading a "silent" option with a "ignore failure"
> > option.  That is bad design.  The description of the -q flag
> > is already 2x as long as it should be in a good design.
> 
> I have a feeling you are watching each my commit and comment it :)

I watch every commit by everyone, and comment when I see a
reason to comment.  I do not play any favors or dis favors
in that respect.   If you feel I am unjustly critizing you
for some reason your mistaken.

> I did not designed this behavior, at work we use another tool to work
> with rules and tables. I'm fine with reverting this change. Do you want
> to restore previous behavior?

I am not asking for a revert, or at least a total revert, there
is atleast a few issues here that do need some fixing.  Like
that fact that "ipfw delete 1" vs "ipfw -q delete 1" do the
exact same thing, and per the man page (arguable interpretation)
the second one should be silent:

(This is an 11.1 system:)
root@x230a:/etc # ipfw add 1 count ip from any to any
00001 count ip from any to any
root@x230a:/etc # ipfw delete 1
root@x230a:/etc # ipfw add 1 count ip from any to any
00001 count ip from any to any
root@x230a:/etc # ipfw -q delete 1
root@x230a:/etc # ipfw delete 1
ipfw: rule 1 not found
root@x230a:/etc # ipfw -q delete 1
ipfw: rule 1 not found

I would like to see the "do not return error" part of this
change removed though.  That is actually changing existing
behavior.

> AFAIR, julian@ complains that ipfw(8) has some error states that should
> be removed.

Yes, I recall that email.  And yes there are probably error
states that should be removed, but IMHO it is almost always
a mistake to have the same option to a command do both silence
and ignore error, that just leads to a lot of 1>/dev/null 2>&1
in shell scripts.

Though one could also argue that there should be no silencing
options to commands as all commands can be silenced by use
of redirection :-)   Typing ipfw -q FOO is only slightly
shorter than typing ipfw FOO >&/dev/null (csh users).


-- 
Rod Grimes                                                 rgrimes@freebsd.org



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201808091619.w79GJ9lp018293>