Re: [libvirt] [firewalld PATCHv3] firewalld PATCH v3
On 08/14/2012 02:59 PM, Thomas Woerner wrote:
* configura.ac, spec file: firewalld now defaults to enabled, depends
on
dbus
* fixed comment for with_firewalld define
* bridge_driver, nwfilter_driver: new dbus filters to get FirewallD1.Reloaded
signal and DBus.NameOwnerChanged on org.fedoraproject.FirewallD1
* iptables, ebtables, nwfilter_ebiptables_driver: use firewall-cmd direct
passthrough interface
* spec file changed as requested
---
configure.ac | 17 ++++++++++++++++
libvirt.spec.in | 11 +++++++++++
src/Makefile.am | 4 ++--
src/network/bridge_driver.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
src/util/ebtables.c | 35 +++++++++++++++++++++++++++++++++
src/util/iptables.c | 21 ++++++++++++++++++--
6 files changed, 131 insertions(+), 4 deletions(-)
This isn't ready to push yet:
The previous versions of this patch also made changes to the nwfilter
driver, but this one doesn't. Did you forget to add those to the commit?
This patch is still doing a search for the path of firewall-cmd every
time ebtablesAddRemoveRule or iptablesAddRemoveRule is called.
That's very wasteful. It should instead just do the search once when
libvirtd starts. You should be able to do this with the
VIR_ONCE_GLOBAL_INIT() macro and an associated xxxOnceInit() function.
Also, I don't see any check for whether or not to use firewall-cmd other
than whether or not the binary exists - if firewalld is disabled, will
firewall-cmd still succeed? (by just calling iptables directly maybe?)
If not, then we need to check if firewalld is enabled at libvirtd start
time, and search for/populate (or not) the firewall-cmd path accordingly
(this is needed both for util/(eb|ip)tables.c and for
nwfilter/nwtilter_ebiptables_driver.c).
Finally, in the commit log message, please put a description of what the
patch does, rather than what changes have been made since the previous
revision of the patch.