[libvirt] [PATCH v3 4/4] bridge driver: don't masquerade local subnet broadcast/multicast packets

[Laszlo Ersek]

On 09/24/13 11:31, Laine Stump wrote:
> On 09/23/2013 08:03 PM, Laszlo Ersek wrote:

>> +/* Fill in preallocated virPfxSocketAddr objects with masquerading exceptions:
>> + *
>> + *  1. do not masquerade packets targeting 224.0.0.0/24
>> + *  2. do not masquerade packets targeting 255.255.255.255/32
>> + *  3. do not masquerade packets targeting the directed local broadcast
>> + *     address
>> + *
>> + * 224.0.0.0/24 is the local network multicast range. Packets are not
>> + * forwarded outside.
>> + *
>> + * 255.255.255.255/32 is the broadcast address of any local network. Again,
>> + * such packets are never forwarded, but strict DHCP clients don't accept
>> + * DHCP replies with changed source ports.
>> + *
>> + * The directed local broadcast address looks like 192.168.122.255/32, and
>> + * behaves like the generic broadcast address 255.255.255.255/32.
>> + *
>> + * Returns 0 on success and -1 on failure.
>> + */
>> +static int networkFillMasqExceptions(const char *bridgeName,
>> +                                     const virPfxSocketAddr *bridge,
>> +                                     virPfxSocketAddr *localMulticast,
>> +                                     virPfxSocketAddr *genericBroadcast,
>> +                                     virPfxSocketAddr *directedBroadcast)
>> +{
>> +    int result;
>> +
>> +    localMulticast->prefix = 24;
>> +    result = virSocketAddrParseIPv4(&localMulticast->addr,
>> +                                    "224.0.0.0");
>> +    sa_assert(result != -1);
> 
> You must have accidentally left this in. libvirt is a library, so it
> must never assert. In a case where the called function is guaranteed to
> never fail (due to the args passed in), you can enclose it in
> ignore_value():
> 
>      ignore_value(cirSocketAddrParseIPv4(.......)

Ah. Good to know!

In fact I had searched the HACKING file for "assert", and there were no
hits. So I grepped the source :)

(BTW there *are* some sa_assert() calls in eg. "src/qemu/qemu_driver.c".

And, as far as I saw, sa_assert() expands to /* empty */ unless
STATIC_ANALYSIS is defined. I kind of didn't understand that, actually;
but I found no naked "assert()" calls in the source. Now that you say
that a library is never supposed to assert() -- I'm not sure I agree
with that FWIW :) -- it makes sense.)

Thanks
Laszlo