[libvirt] [firewalld PATCHv3] firewalld PATCH v3

[Laine Stump]

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.