[libvirt] [PATCH v3 2/4] util/viriptables: add/remove rules that short-circuit masquerading

[Laszlo Ersek]

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

>> diff --git a/src/util/viriptables.h b/src/util/viriptables.h
>> index 447f4a8..fcff5f8 100644
>> --- a/src/util/viriptables.h
>> +++ b/src/util/viriptables.h
>> @@ -26,6 +26,11 @@
>>  
>>  # include "virsocketaddr.h"
>>  
>> +typedef struct {
>> +    virSocketAddr addr;
>> +    unsigned int prefix;
>> +} virPfxSocketAddr;
>> +
> 
> I'm conflicted about whether or not it's really beneficial to have this
> struct (is the setup really worth it just to save one arg?), 

It is one arg per address (ie. one for src, one for dst), and this is
multiplied by the number of function calls that pass them.

> but I
> suppose it doesn't hurt anything, so I'll go along with it if nobody
> else objects. It *does* make these functions' APIs inconsistent with the
> other iptables functions that take a separate addr and prefix, though -
> if we keep this then we should probably update the rest of the API to be
> consistent (not in this patch though).

I don't insist of course -- it seemed to save a lot of boilerplate for
me, but avoiding inconsistency (or extra churn) can easily trump that.

>> +/* Don't masquerade traffic coming from the network associated with the bridge
>> + * if said traffic targets @destaddr/@destprefix.
>> + */
>> +static int
>> +iptablesDontMasquerade(const virPfxSocketAddr *src,
>> +                       const char *physdev,
>> +                       const virPfxSocketAddr *dst,
>> +                       int action)
>> +{
>> +    int ret = -1;
>> +    char *srcStr = NULL;
>> +    char *dstStr = NULL;
>> +    virCommandPtr cmd = NULL;
>> +
>> +    if (!(srcStr = iptablesFormatNetwork(&src->addr, src->prefix)))
>> +        return -1;
>> +
>> +    if (!(dstStr = iptablesFormatNetwork(&dst->addr, dst->prefix)))
>> +        goto cleanup;
>> +
>> +    if (!VIR_SOCKET_ADDR_IS_FAMILY(&src->addr, AF_INET)) {
>> +        /* Higher level code *should* guaranteee it's impossible to get here. */
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Attempted to NAT '%s'. NAT is only supported for IPv4."),
>> +                       srcStr);
>> +        goto cleanup;
>> +    }
> 
> Don't you need to do the same check for dst?

Hm, yeah I do.

But, since we don't need the "machinery" for 192.168.122.255/32: how
about I go back to v2, do the renames that you asked for, and submit a
v4 just like that? No fiddling with pointer typedefs, no runtime
computation of addresses, and so on. Stick to the bare minimum?

Thanks!
Laszlo