[libvirt] [PATCH 17/26] Convert nwfilter ebiptablesTearOldRules to virFirewall

Stefan Berger stefanb at linux.vnet.ibm.com
Wed Apr 16 12:08:03 UTC 2014


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 at 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




More information about the libvir-list mailing list