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

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(a)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.