[libvirt] [PATCH 03/13] Fix logging of failed iptables commands

[Laine Stump]

On 12/20/2010 06:57 PM, Eric Blake wrote:
> On 12/20/2010 01:03 AM, Laine Stump wrote:
>> The functions in iptables.c all return -1 on failure, but all their
>> callers (which all happen to be in bridge_driver.c) assume that they
>> are returning an errno, and the logging is done accordingly. This
>> patch fixes all the error checking and logging to assume<  0 is an
>> error, and nothing else.
>> ---
>>   src/network/bridge_driver.c |  183 +++++++++++++++++++++----------------------
>>   1 files changed, 91 insertions(+), 92 deletions(-)
> Do any of the iptables.c functions guarantee that errno is a sane value
> when returning -1?

There are only two possible reasons for any of the functions in 
iptables.c to fail: 1) out of memory, or 2) the exec of the iptables 
command fails or returns a non-0 status.

In all OOM cases, a message has already been logged before returning, 
and it's likely the act of doing that reporting will trash errno (I 
haven't checked). (And by the time that happens, you're so hosed that 
it's not going to matter that a later error message overwrites that 
message or whatever).

In the case of a problem exec'ing iptables, there could be a valid 
errno, or not (if the command just returned a non-0 exit code.

At any rate, nobody has been looking at errno on return from the 
iptables functions; they've been attempting to interpret a -1 return 
code (placed into the local "err") as an errno-like value, which simply 
isn't correct. This patch fixes that misconception.

>> -        virReportSystemError(err,
>> -                             _("failed to add iptables rule to allow forwarding from '%s'"),
>> -                             network->def->bridge);
>> +    if (iptablesAddForwardAllowOut(driver->iptables,
>> +&network->def->ipAddress,
>> +&network->def->netmask,
>> +                                   network->def->bridge,
>> +                                   network->def->forwardDev)<  0) {
>> +        networkReportError(VIR_ERR_SYSTEM_ERROR,
>> +                           _("failed to add iptables rule to allow forwarding from '%s'"),
>> +                           network->def->bridge);
> or are we okay with this slightly less-informative message, if we happen
> to be ignoring a valid errno?  Then again, given that the old code was
> using strerror(-1), which doesn't convey any information, we aren't
> really losing anything in practice from the old code by not displaying
> errno, even if iptables guaranteed that errno was useful.  Therefore:
>
> ACK as-is.
>