On 04/16/2014 07:55 AM, Daniel P. Berrange wrote:
On Wed, Apr 16, 2014 at 07:41:10AM -0400, Stefan Berger wrote:
> On 04/08/2014 11:38 AM, Daniel P. Berrange wrote:
>> Convert the nwfilter ebiptablesTearOldRules method to use the
>> virFirewall object APIs instead of creating shell scripts
>> using virBuffer APIs. This provides a performance improvement
>> through allowing direct use of firewalld dbus APIs and will
>> facilitate automated testing.
>>
>> Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
>>
>
>> static void
>> @@ -4248,46 +4271,31 @@ ebiptablesTearNewRules(const char *ifname)
>> static int
>> ebiptablesTearOldRules(const char *ifname)
>> {
>> - virBuffer buf = VIR_BUFFER_INITIALIZER;
>> -
>> - /* switch to new iptables user defined chains */
>> - if (iptables_cmd_path) {
>> - NWFILTER_SET_IPTABLES_SHELLVAR(&buf);
>> -
>> - iptablesUnlinkRootChains(&buf, ifname);
>> - iptablesRemoveRootChains(&buf, ifname);
>> -
>> - iptablesRenameTmpRootChains(&buf, ifname);
>> - ebiptablesExecCLI(&buf, true, NULL);
>> - }
>> -
>> - if (ip6tables_cmd_path) {
>> - NWFILTER_SET_IP6TABLES_SHELLVAR(&buf);
>> -
>> - iptablesUnlinkRootChains(&buf, ifname);
>> - iptablesRemoveRootChains(&buf, ifname);
>> -
>> - iptablesRenameTmpRootChains(&buf, ifname);
>> - ebiptablesExecCLI(&buf, true, NULL);
>> - }
>> -
>> - if (ebtables_cmd_path) {
>> - NWFILTER_SET_EBTABLES_SHELLVAR(&buf);
>> -
>> - ebtablesUnlinkRootChain(&buf, true, ifname);
>> - ebtablesUnlinkRootChain(&buf, false, ifname);
>> + virFirewallPtr fw = virFirewallNew();
>> + int ret = -1;
>>
>> - ebtablesRemoveSubChains(&buf, ifname);
>> + virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
>>
>> - ebtablesRemoveRootChain(&buf, true, ifname);
>> - ebtablesRemoveRootChain(&buf, false, ifname);
>> + iptablesUnlinkRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
>> + iptablesRemoveRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
>> + iptablesRenameTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
>>
>> - ebtablesRenameTmpSubAndRootChains(&buf, ifname);
>> + iptablesUnlinkRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
>> + iptablesRemoveRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
>> + iptablesRenameTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
>>
>> - ebiptablesExecCLI(&buf, true, NULL);
>> - }
>> + ebtablesUnlinkRootChainFW(fw, true, ifname);
>> + ebtablesUnlinkRootChainFW(fw, false, ifname);
>> + ebtablesRemoveSubChainsFW(fw, ifname);
>> + ebtablesRemoveRootChainFW(fw, true, ifname);
>> + ebtablesRemoveRootChainFW(fw, false, ifname);
>> + ebtablesRenameTmpSubAndRootChainsFW(fw, ifname);
>>
>> - return 0;
>> + virMutexLock(&execCLIMutex);
>> + ret = virFirewallApply(fw);
>> + virMutexUnlock(&execCLIMutex);
>> + virFirewallFree(fw);
>> + return ret;
>> }
> Looks like the transformations I have seen in the other patches -
> really amazing!. I suppose we wouldn't get here if either iptables,
> ip6tables, or ebtables weren't installed?
The RPM will ensure they are all available, and the virfirewall
code will complain if they're missing, so IMHO that's sufficient.
> Besides I see the lock being grabbed here. You shouldn't need this
> lock anymore if you lock in the virFirewall code, where I guess you
> have to have a libvirt-internal centralized lock (possibly 3 locks ,
> one for iptables, ip6tables, and ebtables if they don't mutually
> influence each other -- would need to test this -- or one lock for
> all of them) in case of direct eb/ip/ip6tables execution and none in
> case of firewalld, which should do its own locking.
These locks are just to protect things during the intermediate
part-converted stage. They go away at the end of this series
so we rely on the lock in virfirewall.c, which obsoletes the
execCLIMutex.
With this explanation: ACK