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>