[libvirt] [PATCH] BZ 657918 Default iptables setup in libvirt breaks mDNS

[Laine Stump]

On 12/11/2012 07:05 PM, Brian J. Murrell wrote:
> On 12-12-11 06:24 PM, Eric Blake wrote:
>> Thanks!
> NP.  Had it just lying around here anyway.  :-)
>  
>> Comment should now mention 4 rules.
> Doh!  Missed that in the patch port.  Updated in my local copy (which I
> will of course resend once all of the initial review is done).

The one thing I would ask beyond Eric's suggestions is that you use git
send-email to produce the patches - the patch you've sent doesn't apply
with git am, which would make it a pain to properly attribute to you.

If you're unfamiliar with using git, here's what you would do:

1) git clone git://libvirt.org/libvirt.git

2) cd libvirt

3) edit the files.

4) make check && make syntax-check (and fix any problems they find)

4) git add $list-of-files

5) git commit

   (give a nice descriptive log message with subject of
    "network: support mDNS on NAT networks")

6) git send-email -1

   (tell it to send to libvir-list at redhat.com)

I can then directly apply the patch with git am.

>
>>> +    /* exempt multicast traffic */
>>> +    if (iptablesAddForwardMasqueradeExempt(driver->iptables) < 0) {
>>> +        virReportError(VIR_ERR_SYSTEM_ERROR,
>>> +                           _("failed to add iptables rule to exempt multicast traffic from masquerading"));
>> Indentation is a bit off,
> OK.  Fixed (again, locally).
>
>> and you need a "%s" argument to keep the
>> syntax-checker happy about a message with no other % operand.
> Hrm.  There is no argument to substitute into a %s though.  There appear
> to be lots of other "virReportError()" calls with no %s in them if
> there is no argument such as:
>
>         virReportError(VIR_ERR_SYSTEM_ERROR,
>                        forwardIf ?
>                        _("failed to add iptables rule to enable masquerading to %s") :
>                        _("failed to add iptables rule to enable masquerading"),
>                        forwardIf);
>
> Notice if forwardIf is NULL, it will use the:
>
>                        _("failed to add iptables rule to enable masquerading"),
>
> branch.  Of course I could be missing something.

I'm surprised that doesn't generate a compile error, other cases of _()
with no %whatever will. It must be the ?: that's messing up the
compiler's checking. At any rate, that should be fixed (separate from
your patch though, of course)

>> Do we need an IPv6 counterpart?  (Or am I just showing my ignorance of
>> what IPv6 does as a counterpart to IPv4 multicast?)
> Hrm.  I wouldn't think so.  NAT (which is what masquerading is)
> isn't supposed to exist in IPv6.  Billions of addresses and all that.
> :-)  Unless my understanding is incorrect that is.
>

Well, there is now at least a proposal for some sort of IPv6 NAT, but
libvirt networks only do routed IPv6 networks, so it shouldn't be necessary.