On 08/15/2012 11:24 AM, Laine Stump wrote: > 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. Also your name/email address needs to be added to AUTHORs, otherwise "make syntax-check" fails. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list