[libvirt] [PATCH 0/8] Misc ebtables cleanup patches
In doing some work on the firewall code in libvirt I came across lots of cruft which could usefully be cleaned up.. Daniel P. Berrange (8): Remove many decls from bridge driver platform header Remove decl of method which doesn't exist in virebtables.h Make ebtablesForwardPolicyReject static Remove unused variables from ebtablesContext Remove data structure holding list of ebtables rules Remove worthless ebtRules data structure Remove unused ebtablesRemoveForwardPolicyReject method Remove broken error reporting in QEMU mac filtering src/Makefile.am | 4 +- src/network/bridge_driver_linux.c | 42 ++++--- src/network/bridge_driver_nop.c | 42 ------- src/network/bridge_driver_platform.h | 22 ---- src/qemu/qemu_bridge_filter.c | 104 ---------------- src/qemu/qemu_bridge_filter.h | 37 ------ src/qemu/qemu_command.c | 11 +- src/qemu/qemu_conf.c | 1 - src/qemu/qemu_driver.c | 7 +- src/qemu/qemu_hotplug.c | 11 +- src/qemu/qemu_process.c | 10 +- src/util/virebtables.c | 233 +++-------------------------------- src/util/virebtables.h | 24 ---- 13 files changed, 60 insertions(+), 488 deletions(-) delete mode 100644 src/qemu/qemu_bridge_filter.c delete mode 100644 src/qemu/qemu_bridge_filter.h -- 1.8.5.3
The bridge_driver_platform.h defines many functions that a platform driver must implement. Only two of these functions are actually called from the main bridge driver code. The remainder can be made internal to the linux driver only. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/network/bridge_driver_linux.c | 42 ++++++++++++++++++++++++------------ src/network/bridge_driver_nop.c | 42 ------------------------------------ src/network/bridge_driver_platform.h | 22 ------------------- 3 files changed, 28 insertions(+), 78 deletions(-) diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index a2f0248..ff62cb3 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -130,8 +130,9 @@ out: static const char networkLocalMulticast[] = "224.0.0.0/24"; static const char networkLocalBroadcast[] = "255.255.255.255/32"; -int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef) +static int +networkAddMasqueradingFirewallRules(virNetworkObjPtr network, + virNetworkIpDefPtr ipdef) { int prefix = virNetworkIpDefPrefix(ipdef); const char *forwardIf = virNetworkDefForwardIf(network->def, 0); @@ -322,8 +323,9 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, return -1; } -void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef) +static void +networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network, + virNetworkIpDefPtr ipdef) { int prefix = virNetworkIpDefPrefix(ipdef); const char *forwardIf = virNetworkDefForwardIf(network->def, 0); @@ -367,8 +369,9 @@ void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network, } } -int networkAddRoutingFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef) +static int +networkAddRoutingFirewallRules(virNetworkObjPtr network, + virNetworkIpDefPtr ipdef) { int prefix = virNetworkIpDefPrefix(ipdef); const char *forwardIf = virNetworkDefForwardIf(network->def, 0); @@ -413,8 +416,10 @@ routeerr1: return -1; } -void networkRemoveRoutingFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef) + +static void +networkRemoveRoutingFirewallRules(virNetworkObjPtr network, + virNetworkIpDefPtr ipdef) { int prefix = virNetworkIpDefPrefix(ipdef); const char *forwardIf = virNetworkDefForwardIf(network->def, 0); @@ -534,7 +539,9 @@ networkRemoveGeneralIp6tablesRules(virNetworkObjPtr network) iptablesRemoveForwardRejectOut(AF_INET6, network->def->bridge); } -int networkAddGeneralFirewallRules(virNetworkObjPtr network) + +static int +networkAddGeneralFirewallRules(virNetworkObjPtr network) { size_t i; virNetworkIpDefPtr ipv4def; @@ -664,7 +671,9 @@ err1: return -1; } -void networkRemoveGeneralFirewallRules(virNetworkObjPtr network) + +static void +networkRemoveGeneralFirewallRules(virNetworkObjPtr network) { size_t i; virNetworkIpDefPtr ipv4def; @@ -694,8 +703,10 @@ void networkRemoveGeneralFirewallRules(virNetworkObjPtr network) iptablesRemoveTcpInput(AF_INET, network->def->bridge, 67); } -int networkAddIpSpecificFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef) + +static int +networkAddIpSpecificFirewallRules(virNetworkObjPtr network, + virNetworkIpDefPtr ipdef) { /* NB: in the case of IPv6, routing rules are added when the * forward mode is NAT. This is because IPv6 has no NAT. @@ -712,8 +723,10 @@ int networkAddIpSpecificFirewallRules(virNetworkObjPtr network, return 0; } -void networkRemoveIpSpecificFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef) + +static void +networkRemoveIpSpecificFirewallRules(virNetworkObjPtr network, + virNetworkIpDefPtr ipdef) { if (network->def->forward.type == VIR_NETWORK_FORWARD_NAT) { if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) @@ -725,6 +738,7 @@ void networkRemoveIpSpecificFirewallRules(virNetworkObjPtr network, } } + /* Add all rules for all ip addresses (and general rules) on a network */ int networkAddFirewallRules(virNetworkObjPtr network) { diff --git a/src/network/bridge_driver_nop.c b/src/network/bridge_driver_nop.c index 23c712d..b8aeaba 100644 --- a/src/network/bridge_driver_nop.c +++ b/src/network/bridge_driver_nop.c @@ -26,48 +26,6 @@ int networkCheckRouteCollision(virNetworkObjPtr network ATTRIBUTE_UNUSED) return 0; } -int networkAddMasqueradingFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED, - virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED) -{ - return 0; -} - -void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED, - virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED) -{ -} - -int networkAddRoutingFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED, - virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED) -{ - return 0; -} - -void networkRemoveRoutingFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED, - virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED) -{ -} - -int networkAddGeneralFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED) -{ - return 0; -} - -void networkRemoveGeneralFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED) -{ -} - -int networkAddIpSpecificFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED, - virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED) -{ - return 0; -} - -void networkRemoveIpSpecificFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED, - virNetworkIpDefPtr ipdef ATTRIBUTE_UNUSED) -{ -} - int networkAddFirewallRules(virNetworkObjPtr network ATTRIBUTE_UNUSED) { return 0; diff --git a/src/network/bridge_driver_platform.h b/src/network/bridge_driver_platform.h index 82d96f6..13d2fce 100644 --- a/src/network/bridge_driver_platform.h +++ b/src/network/bridge_driver_platform.h @@ -53,28 +53,6 @@ typedef virNetworkDriverState *virNetworkDriverStatePtr; int networkCheckRouteCollision(virNetworkObjPtr network); -int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef); - -void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef); - -int networkAddRoutingFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef); - -void networkRemoveRoutingFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef); - -int networkAddGeneralFirewallRules(virNetworkObjPtr network); - -void networkRemoveGeneralFirewallRules(virNetworkObjPtr network); - -int networkAddIpSpecificFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef); - -void networkRemoveIpSpecificFirewallRules(virNetworkObjPtr network, - virNetworkIpDefPtr ipdef); - int networkAddFirewallRules(virNetworkObjPtr network); void networkRemoveFirewallRules(virNetworkObjPtr network); -- 1.8.5.3
There is no impl of the ebtablesSaveRules method and nothing attempts to use it. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virebtables.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virebtables.h b/src/util/virebtables.h index c0d443d..91ef8b5 100644 --- a/src/util/virebtables.h +++ b/src/util/virebtables.h @@ -50,8 +50,6 @@ typedef struct _ebtablesContext ebtablesContext; ebtablesContext *ebtablesContextNew (const char *driver); void ebtablesContextFree (ebtablesContext *ctx); -void ebtablesSaveRules (ebtablesContext *ctx); - int ebtablesAddForwardAllowIn (ebtablesContext *ctx, const char *iface, const virMacAddr *mac); -- 1.8.5.3
The ebtablesForwardPolicyReject method is only used internally to the ebtables code and thus should have been static. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virebtables.c | 28 ++++++++++++++-------------- src/util/virebtables.h | 3 --- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/src/util/virebtables.c b/src/util/virebtables.c index ce5e813..9848f50 100644 --- a/src/util/virebtables.c +++ b/src/util/virebtables.c @@ -381,20 +381,7 @@ ebtablesContextFree(ebtablesContext *ctx) VIR_FREE(ctx); } -int -ebtablesAddForwardPolicyReject(ebtablesContext *ctx) -{ - return ebtablesForwardPolicyReject(ctx, ADD); -} - - -int -ebtablesRemoveForwardPolicyReject(ebtablesContext *ctx) -{ - return ebtablesForwardPolicyReject(ctx, REMOVE); -} - -int +static int ebtablesForwardPolicyReject(ebtablesContext *ctx, int action) { @@ -416,6 +403,19 @@ ebtablesForwardPolicyReject(ebtablesContext *ctx, NULL); } +int +ebtablesAddForwardPolicyReject(ebtablesContext *ctx) +{ + return ebtablesForwardPolicyReject(ctx, ADD); +} + + +int +ebtablesRemoveForwardPolicyReject(ebtablesContext *ctx) +{ + return ebtablesForwardPolicyReject(ctx, REMOVE); +} + /* * Allow all traffic destined to the bridge, with a valid network address */ diff --git a/src/util/virebtables.h b/src/util/virebtables.h index 91ef8b5..b0ca09f 100644 --- a/src/util/virebtables.h +++ b/src/util/virebtables.h @@ -61,7 +61,4 @@ int ebtablesAddForwardPolicyReject(ebtablesContext *ctx); int ebtablesRemoveForwardPolicyReject(ebtablesContext *ctx); -int ebtablesForwardPolicyReject(ebtablesContext *ctx, - int action); - #endif /* __QEMUD_ebtabLES_H__ */ -- 1.8.5.3
The input_filter and nat_postrouting variables were never used to create any firewall rules. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virebtables.c | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/src/util/virebtables.c b/src/util/virebtables.c index 9848f50..6f28b4b 100644 --- a/src/util/virebtables.c +++ b/src/util/virebtables.c @@ -86,9 +86,7 @@ VIR_ONCE_GLOBAL_INIT(virEbTables) struct _ebtablesContext { - ebtRules *input_filter; ebtRules *forward_filter; - ebtRules *nat_postrouting; }; enum { @@ -324,34 +322,21 @@ ebtablesContextNew(const char *driver) { bool success = false; ebtablesContext *ctx = NULL; - char *input_chain = NULL; char *forward_chain = NULL; - char *nat_chain = NULL; if (VIR_ALLOC(ctx) < 0) return NULL; - if (virAsprintf(&input_chain, "libvirt_%s_INPUT", driver) < 0 || - virAsprintf(&forward_chain, "libvirt_%s_FORWARD", driver) < 0 || - virAsprintf(&nat_chain, "libvirt_%s_POSTROUTING", driver) < 0) { - goto cleanup; - } - - if (!(ctx->input_filter = ebtRulesNew("filter", input_chain))) + if (virAsprintf(&forward_chain, "libvirt_%s_FORWARD", driver) < 0) goto cleanup; if (!(ctx->forward_filter = ebtRulesNew("filter", forward_chain))) goto cleanup; - if (!(ctx->nat_postrouting = ebtRulesNew("nat", nat_chain))) - goto cleanup; - success = true; cleanup: - VIR_FREE(input_chain); VIR_FREE(forward_chain); - VIR_FREE(nat_chain); if (!success) { ebtablesContextFree(ctx); @@ -372,12 +357,8 @@ ebtablesContextFree(ebtablesContext *ctx) { if (!ctx) return; - if (ctx->input_filter) - ebtRulesFree(ctx->input_filter); if (ctx->forward_filter) ebtRulesFree(ctx->forward_filter); - if (ctx->nat_postrouting) - ebtRulesFree(ctx->nat_postrouting); VIR_FREE(ctx); } -- 1.8.5.3
When adding/removing ebtables rules, the code would keep an array of all rules in memory. This list of rules was never used for any purpose and would be lost if libvirtd restarted. Delete all the unused code. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virebtables.c | 88 -------------------------------------------------- src/util/virebtables.h | 11 ------- 2 files changed, 99 deletions(-) diff --git a/src/util/virebtables.c b/src/util/virebtables.c index 6f28b4b..13ab51e 100644 --- a/src/util/virebtables.c +++ b/src/util/virebtables.c @@ -97,83 +97,13 @@ enum { INSERT }; -static void -ebtRuleFree(ebtRule *rule) -{ - VIR_FREE(rule->rule); - - if (rule->argv) { - size_t i = 0; - while (rule->argv[i]) - VIR_FREE(rule->argv[i++]); - VIR_FREE(rule->argv); - } -} - -static int -ebtRulesAppend(ebtRules *rules, - char *rule, - char **argv, - int command_idx) -{ - if (VIR_REALLOC_N(rules->rules, rules->nrules+1) < 0) { - size_t i = 0; - while (argv[i]) - VIR_FREE(argv[i++]); - VIR_FREE(argv); - return ENOMEM; - } - - rules->rules[rules->nrules].rule = rule; - rules->rules[rules->nrules].argv = argv; - rules->rules[rules->nrules].command_idx = command_idx; - - rules->nrules++; - - return 0; -} - -static int -ebtRulesRemove(ebtRules *rules, - char *rule) -{ - size_t i; - - for (i = 0; i < rules->nrules; i++) - if (STREQ(rules->rules[i].rule, rule)) - break; - - if (i >= rules->nrules) - return EINVAL; - - ebtRuleFree(&rules->rules[i]); - - memmove(&rules->rules[i], - &rules->rules[i+1], - (rules->nrules - i - 1) * sizeof(ebtRule)); - - rules->nrules--; - - return 0; -} static void ebtRulesFree(ebtRules *rules) { - size_t i; - VIR_FREE(rules->table); VIR_FREE(rules->chain); - if (rules->rules) { - for (i = 0; i < rules->nrules; i++) - ebtRuleFree(&rules->rules[i]); - - VIR_FREE(rules->rules); - - rules->nrules = 0; - } - VIR_FREE(rules); } @@ -192,9 +122,6 @@ ebtRulesNew(const char *table, if (VIR_STRDUP(rules->chain, chain) < 0) goto error; - rules->rules = NULL; - rules->nrules = 0; - return rules; error: @@ -208,7 +135,6 @@ ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...) va_list args; int retval = ENOMEM; char **argv; - char *rule = NULL; const char *s; int n, command_idx; @@ -273,9 +199,6 @@ ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...) va_end(args); - if (!(rule = virArgvToString((const char **) &argv[command_idx]))) - goto error; - if (action == REMOVE) { VIR_FREE(argv[command_idx]); if (VIR_STRDUP(argv[command_idx], "--delete") < 0) @@ -287,18 +210,7 @@ ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...) goto error; } - if (action == ADD || action == CREATE || action == POLICY || - action == INSERT) { - retval = ebtRulesAppend(rules, rule, argv, command_idx); - rule = NULL; - argv = NULL; - } else { - retval = ebtRulesRemove(rules, rule); - } - error: - VIR_FREE(rule); - if (argv) { n = 0; while (argv[n]) diff --git a/src/util/virebtables.h b/src/util/virebtables.h index b0ca09f..7a93a6d 100644 --- a/src/util/virebtables.h +++ b/src/util/virebtables.h @@ -30,19 +30,8 @@ typedef struct { - char *rule; - char **argv; - int command_idx; -} ebtRule; - -typedef struct -{ char *table; char *chain; - - int nrules; - ebtRule *rules; - } ebtRules; typedef struct _ebtablesContext ebtablesContext; -- 1.8.5.3
The ebtRules data structure serves no useful purpose as the table name is never used and only 1 single chain name needs to be stored. Just store the chain name directly in the ebtablesContext instead. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virebtables.c | 104 ++++++++----------------------------------------- src/util/virebtables.h | 6 --- 2 files changed, 17 insertions(+), 93 deletions(-) diff --git a/src/util/virebtables.c b/src/util/virebtables.c index 13ab51e..25410a8 100644 --- a/src/util/virebtables.c +++ b/src/util/virebtables.c @@ -86,57 +86,23 @@ VIR_ONCE_GLOBAL_INIT(virEbTables) struct _ebtablesContext { - ebtRules *forward_filter; + char *chain; }; enum { ADD = 0, REMOVE, - CREATE, - POLICY, - INSERT }; -static void -ebtRulesFree(ebtRules *rules) -{ - VIR_FREE(rules->table); - VIR_FREE(rules->chain); - - VIR_FREE(rules); -} - -static ebtRules * -ebtRulesNew(const char *table, - const char *chain) -{ - ebtRules *rules; - - if (VIR_ALLOC(rules) < 0) - return NULL; - - if (VIR_STRDUP(rules->table, table) < 0) - goto error; - - if (VIR_STRDUP(rules->chain, chain) < 0) - goto error; - - return rules; - - error: - ebtRulesFree(rules); - return NULL; -} - static int ATTRIBUTE_SENTINEL -ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...) +ebtablesAddRemoveRule(const char *arg, ...) { va_list args; int retval = ENOMEM; char **argv; const char *s; - int n, command_idx; + int n; n = 1 + /* /sbin/ebtables */ 2 + /* --table foo */ @@ -175,16 +141,6 @@ ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...) if (VIR_STRDUP(argv[n++], EBTABLES_PATH) < 0) goto error; - command_idx = n; - - if (action == ADD || action == REMOVE) { - if (VIR_STRDUP(argv[n++], "--insert") < 0) - goto error; - - if (VIR_STRDUP(argv[n++], rules->chain) < 0) - goto error; - } - if (VIR_STRDUP(argv[n++], arg) < 0) goto error; @@ -199,12 +155,6 @@ ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...) va_end(args); - if (action == REMOVE) { - VIR_FREE(argv[command_idx]); - if (VIR_STRDUP(argv[command_idx], "--delete") < 0) - goto error; - } - if (virRun((const char **)argv, NULL) < 0) { retval = errno; goto error; @@ -232,27 +182,14 @@ ebtablesAddRemoveRule(ebtRules *rules, int action, const char *arg, ...) ebtablesContext * ebtablesContextNew(const char *driver) { - bool success = false; ebtablesContext *ctx = NULL; - char *forward_chain = NULL; if (VIR_ALLOC(ctx) < 0) return NULL; - if (virAsprintf(&forward_chain, "libvirt_%s_FORWARD", driver) < 0) - goto cleanup; - - if (!(ctx->forward_filter = ebtRulesNew("filter", forward_chain))) - goto cleanup; - - success = true; - -cleanup: - VIR_FREE(forward_chain); - - if (!success) { - ebtablesContextFree(ctx); - ctx = NULL; + if (virAsprintf(&ctx->chain, "libvirt_%s_FORWARD", driver) < 0) { + VIR_FREE(ctx); + return NULL; } return ctx; @@ -269,8 +206,7 @@ ebtablesContextFree(ebtablesContext *ctx) { if (!ctx) return; - if (ctx->forward_filter) - ebtRulesFree(ctx->forward_filter); + VIR_FREE(ctx->chain); VIR_FREE(ctx); } @@ -280,19 +216,13 @@ ebtablesForwardPolicyReject(ebtablesContext *ctx, { /* create it, if it does not exist */ if (action == ADD) { - ebtablesAddRemoveRule(ctx->forward_filter, - CREATE, - "--new-chain", ctx->forward_filter->chain, NULL, + ebtablesAddRemoveRule("--new-chain", ctx->chain, NULL, NULL); - ebtablesAddRemoveRule(ctx->forward_filter, - INSERT, - "--insert", "FORWARD", "--jump", - ctx->forward_filter->chain, NULL); + ebtablesAddRemoveRule("--insert", "FORWARD", "--jump", + ctx->chain, NULL); } - return ebtablesAddRemoveRule(ctx->forward_filter, - POLICY, - "-P", ctx->forward_filter->chain, "DROP", + return ebtablesAddRemoveRule("-P", ctx->chain, "DROP", NULL); } @@ -318,12 +248,12 @@ ebtablesForwardAllowIn(ebtablesContext *ctx, const char *macaddr, int action) { - return ebtablesAddRemoveRule(ctx->forward_filter, - action, - "--in-interface", iface, - "--source", macaddr, - "--jump", "ACCEPT", - NULL); + return ebtablesAddRemoveRule(action == ADD ? "--insert" : "--delete", + ctx->chain, + "--in-interface", iface, + "--source", macaddr, + "--jump", "ACCEPT", + NULL); } /** diff --git a/src/util/virebtables.h b/src/util/virebtables.h index 7a93a6d..246d0dc 100644 --- a/src/util/virebtables.h +++ b/src/util/virebtables.h @@ -28,12 +28,6 @@ # include "virmacaddr.h" -typedef struct -{ - char *table; - char *chain; -} ebtRules; - typedef struct _ebtablesContext ebtablesContext; ebtablesContext *ebtablesContextNew (const char *driver); -- 1.8.5.3
The ebtablesRemoveForwardPolicyReject method was unused and would not do anything useful even if called. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virebtables.c | 28 ++++++---------------------- src/util/virebtables.h | 2 -- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/src/util/virebtables.c b/src/util/virebtables.c index 25410a8..01fb15e 100644 --- a/src/util/virebtables.c +++ b/src/util/virebtables.c @@ -210,35 +210,19 @@ ebtablesContextFree(ebtablesContext *ctx) VIR_FREE(ctx); } -static int -ebtablesForwardPolicyReject(ebtablesContext *ctx, - int action) -{ - /* create it, if it does not exist */ - if (action == ADD) { - ebtablesAddRemoveRule("--new-chain", ctx->chain, NULL, - NULL); - ebtablesAddRemoveRule("--insert", "FORWARD", "--jump", - ctx->chain, NULL); - } - - return ebtablesAddRemoveRule("-P", ctx->chain, "DROP", - NULL); -} int ebtablesAddForwardPolicyReject(ebtablesContext *ctx) { - return ebtablesForwardPolicyReject(ctx, ADD); + ebtablesAddRemoveRule("--new-chain", ctx->chain, NULL, + NULL); + ebtablesAddRemoveRule("--insert", "FORWARD", "--jump", + ctx->chain, NULL); + return ebtablesAddRemoveRule("-P", ctx->chain, "DROP", + NULL); } -int -ebtablesRemoveForwardPolicyReject(ebtablesContext *ctx) -{ - return ebtablesForwardPolicyReject(ctx, REMOVE); -} - /* * Allow all traffic destined to the bridge, with a valid network address */ diff --git a/src/util/virebtables.h b/src/util/virebtables.h index 246d0dc..ba2a761 100644 --- a/src/util/virebtables.h +++ b/src/util/virebtables.h @@ -42,6 +42,4 @@ int ebtablesRemoveForwardAllowIn (ebtablesContext *ctx, int ebtablesAddForwardPolicyReject(ebtablesContext *ctx); -int ebtablesRemoveForwardPolicyReject(ebtablesContext *ctx); - #endif /* __QEMUD_ebtabLES_H__ */ -- 1.8.5.3
The qemu_bridge_filter.c file had some helpers for calling the ebtablesXXX functions todo bridge filtering. The only thing these helpers did was to overwrite the original error message from the ebtables code. For added fun, the callers of these helpers overwrote the errors yet again. For even more fun, one of the helpers called another helper and overwrite its errors too. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/Makefile.am | 4 +- src/qemu/qemu_bridge_filter.c | 104 ------------------------------------------ src/qemu/qemu_bridge_filter.h | 37 --------------- src/qemu/qemu_command.c | 11 ++--- src/qemu/qemu_conf.c | 1 - src/qemu/qemu_driver.c | 7 +-- src/qemu/qemu_hotplug.c | 11 ++--- src/qemu/qemu_process.c | 10 ++-- 8 files changed, 12 insertions(+), 173 deletions(-) delete mode 100644 src/qemu/qemu_bridge_filter.c delete mode 100644 src/qemu/qemu_bridge_filter.h diff --git a/src/Makefile.am b/src/Makefile.am index 25d0370..1708eb7 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -687,9 +687,7 @@ QEMU_DRIVER_SOURCES = \ qemu/qemu_monitor_text.h \ qemu/qemu_monitor_json.c \ qemu/qemu_monitor_json.h \ - qemu/qemu_driver.c qemu/qemu_driver.h \ - qemu/qemu_bridge_filter.c \ - qemu/qemu_bridge_filter.h + qemu/qemu_driver.c qemu/qemu_driver.h XENAPI_DRIVER_SOURCES = \ xenapi/xenapi_driver.c xenapi/xenapi_driver.h \ diff --git a/src/qemu/qemu_bridge_filter.c b/src/qemu/qemu_bridge_filter.c deleted file mode 100644 index 49954c6..0000000 --- a/src/qemu/qemu_bridge_filter.c +++ /dev/null @@ -1,104 +0,0 @@ -/* - * Copyright (C) 2007-2009, 2013 Red Hat, Inc. - * Copyright (C) 2009 IBM Corp. - * - * 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, see - * <http://www.gnu.org/licenses/>. - * - * Authors: - * Gerhard Stenzel <gerhard.stenzel@de.ibm.com> - */ - -#include <config.h> - -#include "virebtables.h" -#include "qemu_conf.h" -#include "qemu_driver.h" -#include "virerror.h" -#include "virlog.h" - -#include "qemu_bridge_filter.h" - -#define VIR_FROM_THIS VIR_FROM_QEMU - -int -networkAddEbtablesRules(virQEMUDriverPtr driver) { - int err; - - /* Set forward policy to DROP */ - if ((err = ebtablesAddForwardPolicyReject(driver->ebtables))) { - virReportSystemError(err, - _("failed to add ebtables rule to set default policy to drop on '%s'"), - __FILE__); - return err; - } - - return 0; -} - - -int -networkDisableAllFrames(virQEMUDriverPtr driver) { - int err; - - /* add default rules */ - if ((err = networkAddEbtablesRules(driver))) { - virReportSystemError(err, - _("cannot filter mac addresses on bridge '%s'"), - __FILE__); - return err; - } - return 0; -} - -int -networkAllowMacOnPort(virQEMUDriverPtr driver, - const char * ifname, - const virMacAddr *mac) -{ - int err; - - /* allow this combination of macaddr and ifname */ - ebtablesContext * ebtablescontext = driver->ebtables; - if ((err = ebtablesAddForwardAllowIn(ebtablescontext, - ifname, - mac))) { - virReportSystemError(err, - _("failed to add ebtables rule to allow routing to '%s'"), - ifname); - } - - return 0; -} - - -int -networkDisallowMacOnPort(virQEMUDriverPtr driver, - const char * ifname, - const virMacAddr *mac) -{ - int err; - - /* disallow this combination of macaddr and ifname */ - ebtablesContext * ebtablescontext = driver->ebtables; - if ((err = ebtablesRemoveForwardAllowIn(ebtablescontext, - ifname, - mac))) { - virReportSystemError(err, - _("failed to add ebtables rule to allow routing to '%s'"), - ifname); - } - - return 0; -} diff --git a/src/qemu/qemu_bridge_filter.h b/src/qemu/qemu_bridge_filter.h deleted file mode 100644 index bacced8..0000000 --- a/src/qemu/qemu_bridge_filter.h +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright (C) 2007-2009, 2013 Red Hat, Inc. - * Copyright (C) 2009 IBM Corp. - * - * 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, see - * <http://www.gnu.org/licenses/>. - * - * Authors: - * Gerhard Stenzel <gerhard.stenzel@de.ibm.com> - */ - -#ifndef __QEMUD_BRIDGE_FILTER_H__ -# define __QEMUD_BRIDGE_FILTER_H__ - - -int networkAllowMacOnPort(virQEMUDriverPtr driver, - const char *ifname, - const virMacAddr *mac); -int networkDisallowMacOnPort(virQEMUDriverPtr driver, - const char *ifname, - const virMacAddr *mac); -int networkDisableAllFrames(virQEMUDriverPtr driver); -int networkAddEbtablesRules(virQEMUDriverPtr driver); - - -#endif /* __QEMUD_BRIDGE_FILTER_H__ */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 775e139..dbb8499 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -26,7 +26,6 @@ #include "qemu_command.h" #include "qemu_hostdev.h" #include "qemu_capabilities.h" -#include "qemu_bridge_filter.h" #include "cpu/cpu.h" #include "dirname.h" #include "passfd.h" @@ -380,12 +379,10 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, virDomainAuditNetDevice(def, net, "/dev/net/tun", true); if (cfg->macFilter && - (ret = networkAllowMacOnPort(driver, net->ifname, &net->mac)) < 0) { - virReportSystemError(ret, - _("failed to add ebtables rule " - "to allow MAC address on '%s'"), - net->ifname); - } + ebtablesAddForwardAllowIn(driver->ebtables, + net->ifname, + &net->mac) < 0) + goto cleanup; if (virNetDevBandwidthSet(net->ifname, virDomainNetGetActualBandwidth(net), diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 20fd62d..bdba7d4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -38,7 +38,6 @@ #include "qemu_conf.h" #include "qemu_command.h" #include "qemu_capabilities.h" -#include "qemu_bridge_filter.h" #include "viruuid.h" #include "virbuffer.h" #include "virconf.h" diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8a54b8a..2a1adec 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -53,7 +53,6 @@ #include "qemu_hostdev.h" #include "qemu_hotplug.h" #include "qemu_monitor.h" -#include "qemu_bridge_filter.h" #include "qemu_process.h" #include "qemu_migration.h" @@ -663,12 +662,8 @@ qemuStateInitialize(bool privileged, goto error; } - if ((errno = networkDisableAllFrames(qemu_driver))) { - virReportSystemError(errno, - _("failed to add rule to drop all frames in '%s'"), - __FILE__); + if (ebtablesAddForwardPolicyReject(qemu_driver->ebtables) < 0) goto error; - } } /* Allocate bitmap for remote display port reservations. We cannot diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6703c92..47996da 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -29,7 +29,6 @@ #include "qemu_capabilities.h" #include "qemu_domain.h" #include "qemu_command.h" -#include "qemu_bridge_filter.h" #include "qemu_hostdev.h" #include "domain_audit.h" #include "domain_nwfilter.h" @@ -2702,13 +2701,9 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, } if (cfg->macFilter && (net->ifname != NULL)) { - if ((errno = networkDisallowMacOnPort(driver, - net->ifname, - &net->mac))) { - virReportSystemError(errno, - _("failed to remove ebtables rule on '%s'"), - net->ifname); - } + ignore_value(ebtablesRemoveForwardAllowIn(driver->ebtables, + net->ifname, + &net->mac)); } vport = virDomainNetGetActualVirtPortProfile(net); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ffa939a..1f00840 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -41,7 +41,6 @@ #include "qemu_command.h" #include "qemu_hostdev.h" #include "qemu_hotplug.h" -#include "qemu_bridge_filter.h" #include "qemu_migration.h" #include "cpu/cpu.h" @@ -4280,12 +4279,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainNetDefPtr net = def->nets[i]; if (net->ifname == NULL) continue; - if ((errno = networkDisallowMacOnPort(driver, net->ifname, - &net->mac))) { - virReportSystemError(errno, - _("failed to remove ebtables rule to allow MAC address on '%s'"), - net->ifname); - } + ignore_value(ebtablesRemoveForwardAllowIn(driver->ebtables, + net->ifname, + &net->mac)); } } -- 1.8.5.3
On 03/10/2014 02:29 PM, Daniel P. Berrange wrote:
In doing some work on the firewall code in libvirt I came across lots of cruft which could usefully be cleaned up..
Daniel P. Berrange (8): Remove many decls from bridge driver platform header Remove decl of method which doesn't exist in virebtables.h Make ebtablesForwardPolicyReject static Remove unused variables from ebtablesContext Remove data structure holding list of ebtables rules Remove worthless ebtRules data structure Remove unused ebtablesRemoveForwardPolicyReject method Remove broken error reporting in QEMU mac filtering
src/Makefile.am | 4 +- src/network/bridge_driver_linux.c | 42 ++++--- src/network/bridge_driver_nop.c | 42 ------- src/network/bridge_driver_platform.h | 22 ---- src/qemu/qemu_bridge_filter.c | 104 ---------------- src/qemu/qemu_bridge_filter.h | 37 ------ src/qemu/qemu_command.c | 11 +- src/qemu/qemu_conf.c | 1 - src/qemu/qemu_driver.c | 7 +- src/qemu/qemu_hotplug.c | 11 +- src/qemu/qemu_process.c | 10 +- src/util/virebtables.c | 233 +++-------------------------------- src/util/virebtables.h | 24 ---- 13 files changed, 60 insertions(+), 488 deletions(-) delete mode 100644 src/qemu/qemu_bridge_filter.c delete mode 100644 src/qemu/qemu_bridge_filter.h
ACK series Jan
participants (2)
-
Daniel P. Berrange -
Ján Tomko