[libvirt] [PATCH] nwfilter: use /bin/sh rather than requiring bash
* src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesWriteToTempFile): Use /bin/sh. (bash_cmd_path): Delete. (ebiptablesDriverInit, ebiptablesDriverShutdown): No need to search for bash. (CMD_EXEC): Prefer $() over ``, since we can assume POSIX. (iptablesSetupVirtInPost): Use portable 'test' syntax. (iptablesLinkIPTablesBaseChain): Use POSIX $(()) syntax. --- It turns out that the current use of test x == x in nwfilter was technically safe after all, since we were requiring the existence of bash (and that was also relatively reasonable, since nwfilter is only compiled for Linux). But that still feels awkward, when it is easier to just fix things to work with /bin/sh. Here, I went the easy route, and assumed that since things are Linux, we can use shell constructs that aren't strictly portable to all /bin/sh implementations, so long as they are portable to POSIX (and will thus work with dash or busybox sh). src/nwfilter/nwfilter_ebiptables_driver.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 20d1ba3..c3a0d3e 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -52,12 +52,16 @@ #define CHAINPREFIX_HOST_IN_TEMP 'J' #define CHAINPREFIX_HOST_OUT_TEMP 'P' +/* This file generates a temporary shell script. Since ebiptables is + Linux-specific, we can be reasonably certain that /bin/sh is more + or less POSIX-compliant, so we can use $() and $(()). However, we + cannot assume that /bin/sh is bash, so stick to POSIX syntax. */ #define CMD_SEPARATOR "\n" #define CMD_DEF_PRE "cmd='" #define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "eval res=\\`\"${cmd}\"\\`" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\$(\"${cmd}\")" CMD_SEPARATOR #define CMD_STOPONERR(X) \ X ? "if [ $? -ne 0 ]; then" \ " echo \"Failure to execute command '${cmd}'.\";" \ @@ -76,7 +80,6 @@ static char *ebtables_cmd_path; static char *iptables_cmd_path; static char *ip6tables_cmd_path; -static char *bash_cmd_path; static char *grep_cmd_path; static char *gawk_cmd_path; @@ -427,7 +430,7 @@ static int iptablesLinkIPTablesBaseChain(const char *iptables_cmd, " " CMD_DEF("%s -I %s %d -j %s") CMD_SEPARATOR " " CMD_EXEC " %s" - " let r=r+1\n" + " r=$(( $r + 1 ))\n" " " CMD_DEF("%s -D %s ${r}") CMD_SEPARATOR " " CMD_EXEC " %s" @@ -650,7 +653,7 @@ iptablesSetupVirtInPost(const char *iptables_cmd, virBufferVSprintf(buf, "res=$(%s -n -L " VIRT_IN_POST_CHAIN " | grep \"\\%s %s\")\n" - "if [ \"${res}\" == \"\" ]; then " + "if [ \"${res}\" = \"\" ]; then " CMD_DEF("%s" " -A " VIRT_IN_POST_CHAIN " %s %s -j ACCEPT") CMD_SEPARATOR @@ -2431,7 +2434,7 @@ ebiptablesDisplayRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED, * * Write the string into a temporary file and return the name of * the temporary file. The string is assumed to contain executable - * commands. A line '#!/bin/bash' will automatically be written + * commands. A line '#!/bin/sh' will automatically be written * as the first line in the file. The permissions of the file are * set so that the file can be run as an executable script. */ @@ -2444,7 +2447,7 @@ ebiptablesWriteToTempFile(const char *string) { char *header; size_t written; - virBufferVSprintf(&buf, "#!%s\n", bash_cmd_path); + virBufferAddLit(&buf, "#!/bin/sh\n"); if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -2513,10 +2516,10 @@ err_exit: * commands executed via the script the was run. * * Returns 0 in case of success, != 0 in case of an error. The returned - * value is NOT the result of running the commands inside the bash + * value is NOT the result of running the commands inside the shell * script. * - * Execute a sequence of commands (held in the given buffer) as a bash + * Execute a sequence of commands (held in the given buffer) as a /bin/sh * script and return the status of the execution. */ static int @@ -3657,7 +3660,6 @@ ebiptablesDriverInit(void) if (virMutexInit(&execCLIMutex)) return EINVAL; - bash_cmd_path = virFindFileInPath("bash"); gawk_cmd_path = virFindFileInPath("gawk"); grep_cmd_path = virFindFileInPath("grep"); @@ -3701,9 +3703,9 @@ ebiptablesDriverInit(void) VIR_FREE(ip6tables_cmd_path); } - /* ip(6)tables support needs bash, gawk & grep, ebtables doesn't */ + /* ip(6)tables support needs gawk & grep, ebtables doesn't */ if ((iptables_cmd_path != NULL || ip6tables_cmd_path != NULL) && - (!grep_cmd_path || !bash_cmd_path || !gawk_cmd_path)) { + (!grep_cmd_path || !gawk_cmd_path)) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("essential tools to support ip(6)tables " "firewalls could not be located")); @@ -3730,7 +3732,6 @@ static void ebiptablesDriverShutdown() { VIR_FREE(gawk_cmd_path); - VIR_FREE(bash_cmd_path); VIR_FREE(grep_cmd_path); VIR_FREE(ebtables_cmd_path); VIR_FREE(iptables_cmd_path); -- 1.7.3.2
On 11/13/2010 04:52 PM, Eric Blake wrote:
* src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesWriteToTempFile): Use /bin/sh. (bash_cmd_path): Delete. (ebiptablesDriverInit, ebiptablesDriverShutdown): No need to search for bash. (CMD_EXEC): Prefer $() over ``, since we can assume POSIX. (iptablesSetupVirtInPost): Use portable 'test' syntax. (iptablesLinkIPTablesBaseChain): Use POSIX $(()) syntax. ---
It turns out that the current use of test x == x in nwfilter was technically safe after all, since we were requiring the existence of bash (and that was also relatively reasonable, since nwfilter is only compiled for Linux). But that still feels awkward, when it is easier to just fix things to work with /bin/sh. Here, I went the easy route, and assumed that since things are Linux, we can use shell constructs that aren't strictly portable to all /bin/sh implementations, so long as they are portable to POSIX (and will thus work with dash or busybox sh).
src/nwfilter/nwfilter_ebiptables_driver.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 20d1ba3..c3a0d3e 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -52,12 +52,16 @@ #define CHAINPREFIX_HOST_IN_TEMP 'J' #define CHAINPREFIX_HOST_OUT_TEMP 'P'
+/* This file generates a temporary shell script. Since ebiptables is + Linux-specific, we can be reasonably certain that /bin/sh is more + or less POSIX-compliant, so we can use $() and $(()). However, we + cannot assume that /bin/sh is bash, so stick to POSIX syntax. */
#define CMD_SEPARATOR "\n" #define CMD_DEF_PRE "cmd='" #define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "eval res=\\`\"${cmd}\"\\`" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\$(\"${cmd}\")" CMD_SEPARATOR #define CMD_STOPONERR(X) \ X ? "if [ $? -ne 0 ]; then" \ " echo \"Failure to execute command '${cmd}'.\";" \ @@ -76,7 +80,6 @@ static char *ebtables_cmd_path; static char *iptables_cmd_path; static char *ip6tables_cmd_path; -static char *bash_cmd_path; static char *grep_cmd_path; static char *gawk_cmd_path;
@@ -427,7 +430,7 @@ static int iptablesLinkIPTablesBaseChain(const char *iptables_cmd, " " CMD_DEF("%s -I %s %d -j %s") CMD_SEPARATOR " " CMD_EXEC " %s" - " let r=r+1\n" + " r=$(( $r + 1 ))\n"
/bin/sh on my system points to bash. What else is it allowed to point to? Obviously csh would be incompatible to /bin/sh.
" " CMD_DEF("%s -D %s ${r}") CMD_SEPARATOR " " CMD_EXEC " %s" @@ -650,7 +653,7 @@ iptablesSetupVirtInPost(const char *iptables_cmd, virBufferVSprintf(buf, "res=$(%s -n -L " VIRT_IN_POST_CHAIN " | grep \"\\%s %s\")\n" - "if [ \"${res}\" == \"\" ]; then " + "if [ \"${res}\" = \"\" ]; then " CMD_DEF("%s" " -A " VIRT_IN_POST_CHAIN " %s %s -j ACCEPT") CMD_SEPARATOR @@ -2431,7 +2434,7 @@ ebiptablesDisplayRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED, * * Write the string into a temporary file and return the name of * the temporary file. The string is assumed to contain executable - * commands. A line '#!/bin/bash' will automatically be written + * commands. A line '#!/bin/sh' will automatically be written * as the first line in the file. The permissions of the file are * set so that the file can be run as an executable script. */ @@ -2444,7 +2447,7 @@ ebiptablesWriteToTempFile(const char *string) { char *header; size_t written;
- virBufferVSprintf(&buf, "#!%s\n", bash_cmd_path); + virBufferAddLit(&buf, "#!/bin/sh\n");
if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); @@ -2513,10 +2516,10 @@ err_exit: * commands executed via the script the was run. * * Returns 0 in case of success, != 0 in case of an error. The returned - * value is NOT the result of running the commands inside the bash + * value is NOT the result of running the commands inside the shell * script. * - * Execute a sequence of commands (held in the given buffer) as a bash + * Execute a sequence of commands (held in the given buffer) as a /bin/sh * script and return the status of the execution. */ static int @@ -3657,7 +3660,6 @@ ebiptablesDriverInit(void) if (virMutexInit(&execCLIMutex)) return EINVAL;
- bash_cmd_path = virFindFileInPath("bash"); gawk_cmd_path = virFindFileInPath("gawk"); grep_cmd_path = virFindFileInPath("grep");
@@ -3701,9 +3703,9 @@ ebiptablesDriverInit(void) VIR_FREE(ip6tables_cmd_path); }
- /* ip(6)tables support needs bash, gawk& grep, ebtables doesn't */ + /* ip(6)tables support needs gawk& grep, ebtables doesn't */ if ((iptables_cmd_path != NULL || ip6tables_cmd_path != NULL)&& - (!grep_cmd_path || !bash_cmd_path || !gawk_cmd_path)) { + (!grep_cmd_path || !gawk_cmd_path)) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("essential tools to support ip(6)tables " "firewalls could not be located")); @@ -3730,7 +3732,6 @@ static void ebiptablesDriverShutdown() { VIR_FREE(gawk_cmd_path); - VIR_FREE(bash_cmd_path); VIR_FREE(grep_cmd_path); VIR_FREE(ebtables_cmd_path); VIR_FREE(iptables_cmd_path);
Since you are the expert with shells and I trust that the TCK tests now pass with all possible /bin/sh's, you get my ACK. Stefan
On 11/13/2010 05:04 PM, Stefan Berger wrote:
@@ -427,7 +430,7 @@ static int iptablesLinkIPTablesBaseChain(const char *iptables_cmd, " " CMD_DEF("%s -I %s %d -j %s") CMD_SEPARATOR " " CMD_EXEC " %s" - " let r=r+1\n" + " r=$(( $r + 1 ))\n"
/bin/sh on my system points to bash. What else is it allowed to point to?
Debian-based systems set it to dash, which doesn't understand let; and we do have examples of people in this configuration. Also, someone going for a minimal-weight hypervisor might want to use busybox sh, another POSIX shell that lacks let; but I'm not sure of anyone using both busybox and kvm-qemu at the same time.
Since you are the expert with shells and I trust that the TCK tests now pass with all possible /bin/sh's, you get my ACK.
Hmm - I guess I'd better test with dash and the TCK before actually pushing this. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 11/13/2010 05:04 PM, Stefan Berger wrote:
On 11/13/2010 04:52 PM, Eric Blake wrote:
* src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesWriteToTempFile): Use /bin/sh. (bash_cmd_path): Delete. (ebiptablesDriverInit, ebiptablesDriverShutdown): No need to search for bash. (CMD_EXEC): Prefer $() over ``, since we can assume POSIX. (iptablesSetupVirtInPost): Use portable 'test' syntax. (iptablesLinkIPTablesBaseChain): Use POSIX $(()) syntax.
Since you are the expert with shells and I trust that the TCK tests now pass with all possible /bin/sh's, you get my ACK.
Well, at least with all POSIX-compliant /bin/sh, which is what Linux is likely to have, and nwfilter requires Linux. It won't work with Solaris /bin/sh, but that's a much harder portability target since it is so old (bourne shell, which pre-dates POSIX). But thankfully not an issue for this patch. I've gone ahead and pushed it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 11/15/2010 07:18 PM, Eric Blake wrote:
* src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesWriteToTempFile): Use /bin/sh. (bash_cmd_path): Delete. (ebiptablesDriverInit, ebiptablesDriverShutdown): No need to search for bash. (CMD_EXEC): Prefer $() over ``, since we can assume POSIX. (iptablesSetupVirtInPost): Use portable 'test' syntax. (iptablesLinkIPTablesBaseChain): Use POSIX $(()) syntax. Since you are the expert with shells and I trust that the TCK tests now
On 11/13/2010 04:52 PM, Eric Blake wrote: pass with all possible /bin/sh's, you get my ACK. Well, at least with all POSIX-compliant /bin/sh, which is what Linux is
On 11/13/2010 05:04 PM, Stefan Berger wrote: likely to have, and nwfilter requires Linux.
It won't work with Solaris /bin/sh, but that's a much harder portability target since it is so old (bourne shell, which pre-dates POSIX). But thankfully not an issue for this patch.
I've gone ahead and pushed it.
Unfortunately it doesn't work. The code produces a file like this now for some basic probing. The file now has a syntax error. #!/bin/sh cmd='/sbin/iptables -n -L FORWARD' eval res=\$("${cmd}") if [ $? -ne 0 ]; then echo "Failure to execute command '${cmd}'."; exit 1;fi Stefan
On 11/15/2010 07:18 PM, Stefan Berger wrote:
I've gone ahead and pushed it.
Unfortunately it doesn't work. The code produces a file like this now for some basic probing. The file now has a syntax error.
Aaargh. I thought I had got my libvirt-tck setup up-to-date, but it was testing the pre-patch version of libvirt.
#!/bin/sh cmd='/sbin/iptables -n -L FORWARD' eval res=\$("${cmd}")
Yep - the ( and ) need quoting. I don't have time to test right now, but you can feel free to push this if it fixes the problem for you: diff --git i/src/nwfilter/nwfilter_ebiptables_driver.c w/src/nwfilter/nwfilter_ebiptables_driver.c index c3a0d3e..7b2a505 100644 --- i/src/nwfilter/nwfilter_ebiptables_driver.c +++ w/src/nwfilter/nwfilter_ebiptables_driver.c @@ -61,7 +61,7 @@ #define CMD_DEF_PRE "cmd='" #define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "eval res=\\$(\"${cmd}\")" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\$\\(\"${cmd}\"\\)" CMD_SEPARATOR #define CMD_STOPONERR(X) \ X ? "if [ $? -ne 0 ]; then" \ " echo \"Failure to execute command '${cmd}'.\";" \ -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 11/15/2010 09:54 PM, Eric Blake wrote:
On 11/15/2010 07:18 PM, Stefan Berger wrote:
I've gone ahead and pushed it.
diff --git i/src/nwfilter/nwfilter_ebiptables_driver.c w/src/nwfilter/nwfilter_ebiptables_driver.c index c3a0d3e..7b2a505 100644 --- i/src/nwfilter/nwfilter_ebiptables_driver.c +++ w/src/nwfilter/nwfilter_ebiptables_driver.c @@ -61,7 +61,7 @@ #define CMD_DEF_PRE "cmd='" #define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "eval res=\\$(\"${cmd}\")" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\$\\(\"${cmd}\"\\)" CMD_SEPARATOR #define CMD_STOPONERR(X) \ X ? "if [ $? -ne 0 ]; then" \ " echo \"Failure to execute command '${cmd}'.\";" \
ACK. Works for me and passes the TCK tests. Stefan
On 11/16/2010 04:30 AM, Stefan Berger wrote:
On 11/15/2010 09:54 PM, Eric Blake wrote:
On 11/15/2010 07:18 PM, Stefan Berger wrote:
I've gone ahead and pushed it.
diff --git i/src/nwfilter/nwfilter_ebiptables_driver.c w/src/nwfilter/nwfilter_ebiptables_driver.c index c3a0d3e..7b2a505 100644 --- i/src/nwfilter/nwfilter_ebiptables_driver.c +++ w/src/nwfilter/nwfilter_ebiptables_driver.c @@ -61,7 +61,7 @@ #define CMD_DEF_PRE "cmd='" #define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "eval res=\\$(\"${cmd}\")" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\$\\(\"${cmd}\"\\)" CMD_SEPARATOR #define CMD_STOPONERR(X) \ X ? "if [ $? -ne 0 ]; then" \ " echo \"Failure to execute command '${cmd}'.\";" \
ACK. Works for me and passes the TCK tests.
Pushed now. Thanks for testing; and sorry for the breakage. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Stefan Berger