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

Stefan Berger stefanb at linux.vnet.ibm.com
Tue Nov 22 15:38:28 UTC 2011


On 11/21/2011 06:42 PM, Eric Blake wrote:
> 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.
>
I removed it now.
>>   #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.
>
ok
>>
>>       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.
Fixed.
>> @@ -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)
I replace the occurrences of the above pattern with the line you are 
showing.
>> @@ -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 : "");
>
Fixed.
> 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.
>
Will leave it as-is.




More information about the libvir-list mailing list