Re: [libvirt] [PATCH] BZ 657918 Default iptables setup in libvirt breaks mDNS
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(a)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.