Re: [libvirt] [PATCH 03/13] Fix logging of failed iptables commands
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.