[libvirt] [RFC PATCH 1/6] add ebtables wrapper

[Daniel P. Berrange]

On Fri, Oct 02, 2009 at 03:48:11PM +0200, Gerhard Stenzel wrote:
> +
> +
> +#ifdef ENABLE_ebtabLES_LOKKIT

Something went a little wrong with upper/lower case there I think

> +static void
> +notifyRulesUpdated(const char *table,
> +                   const char *path)
> +{
> +    char arg[PATH_MAX];
> +    const char *argv[4];
> +
> +    snprintf(arg, sizeof(arg), "--custom-rules=ipv4:%s:%s", table, path);
> +
> +    argv[0] = (char *) LOKKIT_PATH;
> +    argv[1] = (char *) "--nostart";
> +    argv[2] = arg;
> +    argv[3] = NULL;

It is desirable not to cast away const-ness, but rather to declare
it correctly - you can also initialize at the same time as declaring
to avoid needing to hardcode array indexes, eg

   const char * const argv[] = {
      LOKKIT_PATH, "--nostart", arg, NULL,
   };


> +ebtablesContext *
> +ebtablesContextNew(void)
> +{
> +    ebtablesContext *ctx;
> +
> +    if (VIR_ALLOC(ctx) < 0)
> +        return NULL;
> +
> +    if (!(ctx->input_filter = ebtRulesNew("filter", "INPUT")))
> +        goto error;
> +
> +    if (!(ctx->forward_filter = ebtRulesNew("filter", "FORWARD")))
> +        goto error;
> +
> +    if (!(ctx->nat_postrouting = ebtRulesNew("nat", "POSTROUTING")))
> +        goto error;
> +
> +    return ctx;
> +
> + error:
> +    ebtablesContextFree(ctx);
> +    return NULL;
> +}

I know you're just following the existing style/pattern used by
the iptables.c/h file here by directly manipulating the main
INPUT/FORWARD/POSTROUTING chains, but I think this could be
improved a little

Add a custom 'libvirt_INPUT' chain as the first entry in
the main 'INPUT' chain, and then put all our rules on this
custom chain. This will make it clearer for admins where all
these extra rules are coming from, and make it slightly safer
for management since there willbe less risk of rules in our
custom chain getting blown away / mis-ordered wrt other rules

I think it would even be desirable to have a custom chain
per libvirt driver, since multiple drivers might want todo
this MAC filtering. Thus I'd suggest adding a 'const char *driverName'
parameter to ebtablesContextNew(const char *driver), and
then creating chains named with that, eg so if QEMU, LXC
and UML drivers all uses this ebtables code, we'd end up
with the INPUT chain having references to 3 custom chains

    libvirt_qemu_INPUT
    libvirt_lxc_INPUT
    libvirt_uml_INPUT

Likewise for FORWARD/POSTROUTING.


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|