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. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list