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.