#1009 closed defect (fixed)

ip_ plugin to be marked as BASH?

Reported by: andol Owned by: nobody
Priority: normal Milestone: Munin 1.4.7
Component: plugins Version: 1.4.4
Severity: minor Keywords:


Regardin the munin plugin ip_


Given the function call in this part of the code, I wonder if it should not be marked as BASH instead of GOODSH?

case $IP in
    *:*) # I know this! This is IPv6!
        # This is a fun hack to make the plugin ip6 compatible.
        # Suggested in ticket #439 by "jodal".
        eval 'function iptables() {
            /sbin/ip6tables "$@"

This is the result I experience on an Ubuntu system, where /bin/sh is a /bin/dash shell.

root@halleck:/etc/munin/plugins# ls -l ip_2001\:ba8\:1f1\:f1d1\:\:2 
lrwxrwxrwx 1 root root 28 Dec 12 19:26 ip_2001:ba8:1f1:f1d1::2 -> /usr/share/munin/plugins/ip_
root@halleck:/etc/munin/plugins# export MUNIN_LIBDIR=/usr/share/munin
root@halleck:/etc/munin/plugins# ./ip_2001\:ba8\:1f1\:f1d1\:\:2 
eval: 1: Syntax error: "(" unexpected

Yet, when I manually modify /usr/share/munin/plugins/ip_ to use a /bin/bash shebang, it runs nicely.

root@halleck:/etc/munin/plugins# ./ip_2001\:ba8\:1f1\:f1d1\:\:2 
in.value 158317482
out.value 86558740

This was with munin 1.4.4 on an Ubuntu 10.04 system, but the ip_ plugin appears to be the same in the current trunk.

Unless /bin/dash isn't a proper posix shell? If so, let me know and I will file this as an Ubuntu bug instead, to be fixed at a packaging level.

Change History (6)

comment:1 Changed at 2011-01-07T12:31:49+01:00 by jo

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in r4076 and r4077 for the 1.4 branch and trunk respectively. The fix will be in 1.4.6. Thanks for the report.

comment:2 Changed at 2011-02-12T15:57:19+01:00 by blueyed

  • Resolution fixed deleted
  • Status changed from closed to reopened

The proper fix for this appears to be removing the bashism ("function" keyword), instead of using bash because of this.

It would have to look just like this:

        eval 'iptables() {
            /sbin/ip6tables "$@"

After applying this, please use GOODSH for this script again.

comment:3 Changed at 2011-04-15T23:53:51+02:00 by Tenzer

The change to simply remove function from the file was made in r4160, the file however has @@BASH@@ in on the first line of the file. It could probably work with @@GOODSH@@ now that it has been fixed - it should first be tested however.

comment:4 Changed at 2012-01-09T08:07:30+01:00 by kenyon

  • Resolution set to fixed
  • Status changed from reopened to closed

bash is much more powerful and safer for scripting than plain Bourne shell, so let's leave this as a bash script. Since this is a Linux plugin, there should be no problem finding the bash executable.

comment:5 Changed at 2012-01-29T14:01:32+01:00 by blueyed

  • Resolution fixed deleted
  • Status changed from closed to reopened

kenyon, there's no reason to require bash when not using any bashisms / bash features.

Plain sh (e.g. via "dash") is often used for performance and portability reasons.

comment:6 Changed at 2012-01-29T21:43:34+01:00 by kenyon

  • Resolution set to fixed
  • Status changed from reopened to closed

No, as I said, there are reasons to prefer bash over plain sh. Unless you can provide some evidence that there are any performance or portability impacts here, I prefer the more powerful and safer bash. I would prefer perl or something else for the same reasons, over any shell. Looking at the number of calls to awk in this script, it might even be better in awk or perl. iptables is really going to be the limitng performance factor in this script. Also show me where using plain sh helps with portability in this case. It's already limited to Linux due to iptables. It also uses "awk --posix", which wouldn't work with BSD's awk, for example. Your argument is not convincing.

Note: See TracTickets for help on using tickets.