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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list