[libvirt] [PATCH 23/26] Convert nwfilter ebiptablesApplyNewRules to virFirewall

[Daniel P. Berrange]

On Wed, Apr 16, 2014 at 03:38:39PM -0400, Stefan Berger wrote:
> >      }
> >
> >      if (HAS_ENTRY_ITEM(&ipHdr->dataComment)) {
> >-        printCommentVar(prefix, ipHdr->dataComment.u.string);
> >-
> >          /* keep comments behind everything else -- they are packet eval.
> >             no-ops */
> >-        virBufferAddLit(afterStateMatch,
> >-                        " -m comment --comment \"$" COMMENT_VARNAME "\"");
> >+        virFirewallRuleAddArgList(fw, fwrule,
> >+                                  "-m", "comment",
> >+                                  "--comment", ipHdr->dataComment.u.string,
> >+                                  NULL);
> >      }
> 
> The interesting part about comments was before to ensure that $(foo)
> never executes in a subshell. With  TCK passing it seems this
> concern is addressed.

Well the way we execute iptables now means that the shell is never
involved in any part of the stack, so the issue goes away entirely.


> >          } else {
> >              if (virNWFilterRuleIsProtocolIPv4(rules[i]->def))
> >                  haveIptables = true;
> >-            else if (virNWFilterRuleIsProtocolIPv4(rules[i]->def))
> >+            else if (virNWFilterRuleIsProtocolIPv6(rules[i]->def))
> >                  haveIp6tables = true;
> 
> Ah, this is probably the reason why IPv6 rules didn't work in TCK
> and 23/26 now fixes it. That's probably a typo introduced in 8/26.
> If you want to go back to 8/26 to fix this ... unless this has other
> negative side effects during the surgery. Up to you.

Yep, easy to fix the original. Thanks for finding this.

> >
> >      if (haveIptables) {
> 
> Based on your comment in another patch, now I am surprised to still
> see this check 'haveIptables' here. Wouldn't the rpm also solve this
> here as well?

This boolean is about whether any iptables rules are defined for
the filter. It lets us avoid creating the base chains if there
are no rules to put in them.

> >
> >      if (haveIp6tables) {
> 
> ... also this here.

Likewise.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|