Re: [PATCH 17/26] Convert nwfilter ebiptablesTearOldRules to virFirewall

[Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> >
> > >   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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list