[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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->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.

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]