[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [RFC PATCH 1/3] add ebtables wrapper



On Tue, Oct 13, 2009 at 12:57:13PM +0200, Gerhard Stenzel wrote:
> +#ifdef ENABLE_EBTABLES_LOKKIT
> +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;

This is better declared upfront as

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

> +
> +    char ebuf[1024];
> +    if (virRun(NULL, argv, NULL) < 0)
> +        VIR_WARN(_("Failed to run '%s %s': %s"),
> +                 LOKKIT_PATH, arg, virStrerror(errno, ebuf, sizeof ebuf));
> +}

> +
> +static void
> +notifyRulesRemoved(const char *table,
> +                   const char *path)
> +{
> +/* 10 MB limit on config file size as a sanity check */
> +#define MAX_FILE_LEN (1024*1024*10)
> +
> +    char arg[PATH_MAX];
> +    char *content;
> +    int len;
> +    FILE *f = NULL;
> +
> +    len = virFileReadAll(SYSCONF_DIR "/sysconfig/system-config-firewall",
> +                         MAX_FILE_LEN, &content);
> +    if (len < 0) {
> +        VIR_WARN("%s", _("Failed to read " SYSCONF_DIR
> +                         "/sysconfig/system-config-firewall"));
> +        return;
> +    }
> +
> +    snprintf(arg, sizeof(arg), "--custom-rules=ipv4:%s:%s", table, path);
> +
> +    if (!stripLine(content, len, arg)) {
> +        VIR_FREE(content);
> +        return;
> +    }
> +
> +    if (!(f = fopen(SYSCONF_DIR "/sysconfig/system-config-firewall", "w")))
> +        goto write_error;
> +
> +    if (fputs(content, f) == EOF)
> +        goto write_error;
> +
> +    if (fclose(f) == EOF) {
> +        f = NULL;
> +        goto write_error;
> +    }

This  fopen/fputs/fclose  triple could be nicely replaced with a
single call to 

    if (virFileWriteStr(SYSCONF_DIR "/sysconfig/system-config-firewall",
                        content) < 0)
       goto write_error;

> +
> +    VIR_FREE(content);
> +
> +    return;
> +
> + write_error:;
> +    char ebuf[1024];
> +    VIR_WARN(_("Failed to write to " SYSCONF_DIR
> +               "/sysconfig/system-config-firewall : %s"),
> +             virStrerror(errno, ebuf, sizeof ebuf));
> +    if (f)
> +        fclose(f);
> +    VIR_FREE(content);
> +
> +#undef MAX_FILE_LEN
> +}

> +
> +static ebtRules *
> +ebtRulesNew(const char *table,
> +            const char *chain)
> +{
> +    ebtRules *rules;
> +
> +    if (VIR_ALLOC(rules) < 0)
> +        return NULL;
> +
> +    if (!(rules->table = strdup(table)))
> +        goto error;
> +
> +    if (!(rules->chain = strdup(chain)))
> +        goto error;
> +
> +    rules->rules = NULL;
> +    rules->nrules = 0;
> +
> +#ifdef ENABLE_EBTABLES_LOKKIT
> +    if (virFileBuildPath(LOCAL_STATE_DIR "/lib/libvirt/ebtables", table, NULL,
> +                         rules->dir, sizeof(rules->dir)) < 0)
> +        goto error;
> +
> +    if (virFileBuildPath(rules->dir, chain, ".chain", rules->path, sizeof(rules->path)) < 0)
> +        goto error;
> +#endif /* ENABLE_EBTABLES_LOKKIT */

Don't forget to add a rule to src/Makefile.am to ensure that the
new ebtables directory is created by the 'install-data-local' rule

> index 0000000..a3f403a
> --- /dev/null
> +++ b/src/util/ebtables.h
> @@ -0,0 +1,134 @@
> +/*
> + * Copyright (C) 2009 IBM Corp.
> + * Copyright (C) 2007, 2008 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
> + *
> + * based on iptables.h
> + * Authors:
> + *     Gerhard Stenzel <gerhard stenzel de ibm com>
> + */
> +
> +#ifndef __QEMUD_EBTABLES_H__
> +#define __QEMUD_EBTABLES_H__
> +
> +typedef struct
> +{
> +    char  *rule;
> +    const char **argv;
> +    int    command_idx;
> +} ebtRule;
> +
> +typedef struct
> +{
> +    char  *table;
> +    char  *chain;
> +
> +    int      nrules;
> +    ebtRule *rules;
> +
> +#ifdef ENABLE_EBTABLES_LOKKIT
> +
> +    char   dir[PATH_MAX];
> +    char   path[PATH_MAX];
> +
> +#endif /* ENABLE_EBTABLES_LOKKIT */
> +
> +} ebtRules;
> +
> +struct _ebtablesContext
> +{
> +    ebtRules *input_filter;
> +    ebtRules *forward_filter;
> +    ebtRules *nat_postrouting;
> +};

Since the caller of the API does not need to know the internals
of the struct, it is better to move these 3 struct definitions
into the ebtables.c file.

> +typedef struct _ebtablesContext ebtablesContext;
> +
> +ebtablesContext *ebtablesContextNew              (const char *driver);
> +void             ebtablesContextFree             (ebtablesContext *ctx);
> +
> +void             ebtablesSaveRules               (ebtablesContext *ctx);
> +void             ebtablesReloadRules             (ebtablesContext *ctx);
> +
> +int              ebtablesAddTcpInput             (ebtablesContext *ctx,
> +                                                  const char *iface,
> +                                                  int port);
> +int              ebtablesRemoveTcpInput          (ebtablesContext *ctx,
> +                                                  const char *iface,
> +                                                  int port);
> +
> +int              ebtablesAddUdpInput             (ebtablesContext *ctx,
> +                                                  const char *iface,
> +                                                  int port);
> +int              ebtablesRemoveUdpInput          (ebtablesContext *ctx,
> +                                                  const char *iface,
> +                                                  int port);
> +
> +int              ebtablesAddForwardAllowOut      (ebtablesContext *ctx,
> +                                                  const char *network,
> +                                                  const char *iface,
> +                                                  const char *physdev);
> +int              ebtablesRemoveForwardAllowOut   (ebtablesContext *ctx,
> +                                                  const char *network,
> +                                                  const char *iface,
> +                                                  const char *physdev);
> +
> +int              ebtablesAddForwardAllowRelatedIn(ebtablesContext *ctx,
> +                                                  const char *network,
> +                                                  const char *iface,
> +                                                  const char *physdev);
> +int              ebtablesRemoveForwardAllowRelatedIn(ebtablesContext *ctx,
> +                                                  const char *network,
> +                                                  const char *iface,
> +                                                  const char *physdev);
> +
> +int              ebtablesAddForwardAllowIn       (ebtablesContext *ctx,
> +                                                  const char *iface,
> +                                                  const char *physdev);
> +int              ebtablesRemoveForwardAllowIn    (ebtablesContext *ctx,
> +                                                  const char *iface,
> +                                                  const char *physdev);
> +
> +int              ebtablesAddForwardAllowCross    (ebtablesContext *ctx,
> +                                                  const char *iface);
> +int              ebtablesRemoveForwardAllowCross (ebtablesContext *ctx,
> +                                                  const char *iface);
> +
> +int              ebtablesAddForwardRejectOut     (ebtablesContext *ctx,
> +                                                  const char *iface);
> +int              ebtablesRemoveForwardRejectOut  (ebtablesContext *ctx,
> +                                                  const char *iface);
> +
> +int              ebtablesAddForwardRejectIn      (ebtablesContext *ctx,
> +                                                  const char *iface);
> +int              ebtablesRemoveForwardRejectIn   (ebtablesContext *ctx,
> +                                                  const char *iface);
> +
> +int              ebtablesAddForwardMasquerade    (ebtablesContext *ctx,
> +                                                  const char *network,
> +                                                  const char *physdev);
> +int              ebtablesRemoveForwardMasquerade (ebtablesContext *ctx,
> +                                                  const char *network,
> +                                                  const char *physdev);
> +
> +int              ebtablesAddForwardPolicyReject(ebtablesContext *ctx);
> +
> +int              ebtablesRemoveForwardPolicyReject(ebtablesContext *ctx);
> +
> +int              ebtablesForwardPolicyReject(ebtablesContext *ctx,
> +                                                  int action);

There seem like there's quite a lot of methods here that we don't 
actually need for MAC address filtering. It'd be a little clearer
if you removed the ones that aren't relevant. Alot of the complexity
of the iptables.h/c file on which this is based is relating to the
masquerading, and punching holes for DHCP/DNS. ebtables ought to be
alot simpler, since we're pretty much just doing a 'ALLOW <mac>'
followed by  'DENY ALL' on the tap device in question. 

Regards,
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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]