[libvirt] [PATCH] nwfilter: use /bin/sh rather than requiring bash

[Stefan Berger]

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