On 02/21/2014 04:20 AM, Laine Stump wrote:
On 02/20/2014 07:13 AM, Eric Blake wrote:
While auditing all callers of virCommandRun, I noticed that nwfilter code never paid attention to commands with a non-zero status. In the cases where status was captured, either the callers required that the status was 0, or they discarded any failures from virCommandRun. Collecting status manually means that a non-zero child exit status is not logged, but I could not see the benefit in avoiding the logging in any command issued in the code. Therefore, it was simpler to just nuke the wasted effort of manually checking or ignoring non-zero status.
You need to be careful with this - for some of the external execs in nwfilter (same with viriptables.c), a non-0 status *should* be ignored and not reported. In particular, if a command that is attempting to remove an iptables or ebtables rule fails, that is often because libvirt is attempting to remove a rule that actually isn't there.
As a matter of fact, in all the cases where the 2nd argument to ebiptablesExecCLI is non-NULL, that is exactly what's happening - the code was written that way to avoid putting a bogus and misleading error message in the logs; viriptables.c *does* log these errors, and that has led to many bug reports that incorrectly list the error message about failure to remove a rule as evidence that there is a bug. (I think there may even be a BZ filed requesting that these error logs be removed because they are misleading.)
Because of the experience with viriptables.c, I don't think we should change the code to add back in the logging of these messages.
Then how about I rewrite this patch to instead pass a bool to ebiptablesExecCLI that says whether to ignore non-zero status. That way, it doesn't look as crazy to have a status parameter passed through a lot of the call stack that is otherwise ignored. v2 coming up. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org