[libvirt] [PATCH V1 9/9] Improve error reporting of failures to apply filtering rules

[Eric Blake]

On 10/26/2011 09:12 AM, Stefan Berger wrote:
> Display the executed command and failure message if a command failed to
> execute.
> 
> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
> 
> ---
>  src/nwfilter/nwfilter_ebiptables_driver.c |   82 ++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 32 deletions(-)
> 
> Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
> ===================================================================
> --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c
> +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
> @@ -63,10 +63,10 @@
>  #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} 2>&1 \"\\)" CMD_SEPARATOR

Okay, after turning the C literal into shell, you have:

eval res=\$\("${cmd} 2>&1 "\)

and after the shell eval, you have:

res=$(command 2>&1 )

That space after '2>&1' could be deleted, but it doesn't hurt to leave
it in.

>  #define CMD_STOPONERR(X) \
>      X ? "if [ $? -ne 0 ]; then" \
> -        "  echo \"Failure to execute command '${cmd}'.\";" \
> +        "  echo \"Failure to execute command '${cmd}' : '${res}'.\";" \

Yes, this is a bit more informative.

>          "  exit 1;" \
>          "fi" CMD_SEPARATOR \
>        : ""
> @@ -2785,12 +2785,16 @@ err_exit:
>   */
>  static int
>  ebiptablesExecCLI(virBufferPtr buf,
> -                  int *status)
> +                  int *status, char **errbuf)
>  {
>      char *cmds;
>      char *filename;
>      int rc;
>      const char *argv[] = {NULL, NULL};
> +    virCommandPtr cmd;
> +
> +    if (errbuf)
> +        VIR_FREE(*errbuf);

This has merge conflicts with existing patches that went in during the
meantime.  I'd feel better seeing a v2 to verify proper rebasing.

>  
>      if (virBufferError(buf)) {
>          virReportOOMError();
> @@ -2817,7 +2821,13 @@ ebiptablesExecCLI(virBufferPtr buf,
>  
>      virMutexLock(&execCLIMutex);
>  
> -    rc = virRun(argv, status);
> +    cmd = virCommandNewArgs(argv);
> +    if (errbuf)
> +        virCommandSetOutputBuffer(cmd, errbuf);

It looks a bit odd calling it errbuf, when it is the output collected
from stdout; perhaps calling it outbuf would make more sense.

> @@ -3285,7 +3297,7 @@ ebtablesApplyBasicRules(const char *ifna
>      ebtablesLinkTmpRootChain(&buf, 1, ifname, 1);
>      ebtablesRenameTmpRootChain(&buf, 1, ifname);
>  
> -    if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0)
> +    if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status != 0)

You know, as long as we are cleaning things up, you could pass NULL
instead of &cli_status to enforce that the command has a 0 exit status,
so that you trim lines like this to:

if (ebiptablesExecCLI(&buf, NULL, NULL) < 0)

> @@ -3874,12 +3889,13 @@ tear_down_tmpebchains:
>          ebtablesRemoveTmpRootChain(&buf, 0, ifname);
>      }
>  
> -    ebiptablesExecCLI(&buf, &cli_status);
> +    ebiptablesExecCLI(&buf, &cli_status, NULL);
>  
>      virNWFilterReportError(VIR_ERR_BUILD_FIREWALL,
>                             _("Some rules could not be created for "
> -                             "interface %s."),
> -                           ifname);
> +                             "interface %s : %s"),
> +                           ifname,
> +                           errmsg ? errmsg : "");

That outputs a trailing space if there was no error message.  Better
might be:

virNWFilterReportError(VIR_ERR_BUILD_FIREWALL,
                       _("Some rules could not be created for "
                         "interface %s%s%s"),
                       ifname,
                       errmsg ? ": " : "",
                       errmsg ? errmsg : "");

Alas, we have an i18n nightmare.  errmsg contains English text output by
the shell script, which is not marked for translation by either the
shell script (which itself is tricky - witness the libvirt-guests init
script) nor for translation in the C code that generated the shell
script.  Meanwhile, since we didn't call virCommandAddEnvPassCommon() to
set the virCommand to force LC_ALL=C, that means we passed the libvirtd
setting of LC_MESSAGES on through to the child processes, such that sh
and ebtables output might also be translated.  For an example, that
means someone using LC_MESSAGES=es_ES.UTF-8 could see:

Algunas reglas no han podido crearse para la interfaz eth0 : Failure to
execute command '/path/to/ebtables -t nat ...' : /bin/sh:
/path/to/ebtables: no se encontró la orden

That's a nasty mix of native/English/native output all in one very long
message.  Maybe we should be rethinking the desired output format a bit,
for the sake of generating properly translated messages?  Even breaking
things into multiple messages, so that each message is either translated
or English, rather than mixed, might help.

Is it worth trying to fix part of the translation issue, by defining
CMD_STOPONERROR to something that outputs the results of _("Failure to
execute command '%s'"), then taking that translation and properly
escaping it (via virBufferEscapeShell) so that the entire message will
be output literally by shell (so that translators can't cause arbitrary
shell execution by sneaking ';' into their translation), so that the
generated script has a pre-translated message?  But that sounds hairy.
It also means that CMD_STOPONERROR would no longer be a string literal,
but a complex function call to properly append stuff into each place you
build up the command sequence into &buf.

Maybe this just serves as yet another reason why using the shell as a
script driver is prone to problems, and that anything we can do
long-term to rewrite this into internal libvirt API that bypasses shell
scripting will give better results, and we just live with the poor
diagnostics in the meantime.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111121/bf675ca0/attachment-0001.sig>