From owner-svn-src-head@freebsd.org Thu Aug 9 16:19:12 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id AA462106B8D6; Thu, 9 Aug 2018 16:19:12 +0000 (UTC) (envelope-from freebsd@pdx.rh.CN85.dnsmgr.net) Received: from pdx.rh.CN85.dnsmgr.net (br1.CN84in.dnsmgr.net [69.59.192.140]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 2E1477BFE8; Thu, 9 Aug 2018 16:19:12 +0000 (UTC) (envelope-from freebsd@pdx.rh.CN85.dnsmgr.net) Received: from pdx.rh.CN85.dnsmgr.net (localhost [127.0.0.1]) by pdx.rh.CN85.dnsmgr.net (8.13.3/8.13.3) with ESMTP id w79GJ9Dk018294; Thu, 9 Aug 2018 09:19:09 -0700 (PDT) (envelope-from freebsd@pdx.rh.CN85.dnsmgr.net) Received: (from freebsd@localhost) by pdx.rh.CN85.dnsmgr.net (8.13.3/8.13.3/Submit) id w79GJ9lp018293; Thu, 9 Aug 2018 09:19:09 -0700 (PDT) (envelope-from freebsd) From: "Rodney W. Grimes" Message-Id: <201808091619.w79GJ9lp018293@pdx.rh.CN85.dnsmgr.net> Subject: Re: svn commit: r337536 - head/sbin/ipfw In-Reply-To: To: "Andrey V. Elsukov" Date: Thu, 9 Aug 2018 09:19:09 -0700 (PDT) CC: rgrimes@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Reply-To: rgrimes@freebsd.org X-Mailer: ELM [version 2.4ME+ PL121h (25)] MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=US-ASCII X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Aug 2018 16:19:12 -0000 > 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