Merge nwfilter createRuleInstance driver into applyNewRules

The current nwfilter tech driver API has a 'createRuleInstance' method
which populates virNWFilterRuleInstPtr with a command line string
containing variable placeholders. The 'applyNewRules' method then
expands the variables and executes the commands. This split of
responsibility won't work when switching to the virFirewallPtr
APIs, since we can't just build up command line strings. This patch
this merges the functionality of 'createRuleInstance' into the
applyNewRules method.

The virNWFilterRuleInstPtr struct is changed from holding an array
of opaque pointers, into holding generic metadata about the rules
to be processed. In essence this is the result of taking a linked
set of virNWFilterDefPtr's and flattening the tree to get a list
of virNWFilterRuleDefPtr's. At the same time we must keep track of
any nested virNWFilterObjPtr instances, so that the locks are held
for the duration of the 'applyNewRules' method.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

src/nwfilter/nwfilter_ebiptables_driver.c

@@ -477,46 +477,6 @@ printCommentVar(virBufferPtr dest, const char *buf)
 }
 
 
-static void
-ebiptablesRuleInstFree(ebiptablesRuleInstPtr inst)
-{
-    if (!inst)
-        return;
-
-    VIR_FREE(inst->commandTemplate);
-    VIR_FREE(inst);
-}
-
-
-static int
-ebiptablesAddRuleInst(virNWFilterRuleInstPtr res,
-                      char *commandTemplate,
-                      const char *neededChain,
-                      virNWFilterChainPriority chainPriority,
-                      char chainprefix,
-                      virNWFilterRulePriority priority,
-                      enum RuleType ruleType)
-{
-    ebiptablesRuleInstPtr inst;
-
-    if (VIR_ALLOC(inst) < 0)
-        return -1;
-
-    inst->commandTemplate = commandTemplate;
-    inst->neededProtocolChain = neededChain;
-    inst->chainPriority = chainPriority;
-    inst->chainprefix = chainprefix;
-    inst->priority = priority;
-    inst->ruleType = ruleType;
-
-    if (VIR_APPEND_ELEMENT(res->data, res->ndata, inst) < 0) {
-        VIR_FREE(inst);
-        return -1;
-    }
-    return 0;
-}
-
-
 static int
 ebtablesHandleEthHdr(virBufferPtr buf,
                      virNWFilterVarCombIterPtr vars,
@@ -2648,13 +2608,18 @@ ebtablesCreateRuleInstance(char chainPrefix,
  * pointed to by res, -1 otherwise
  */
 static int
-ebiptablesCreateRuleInstance(virNWFilterChainPriority chainPriority,
+ebiptablesCreateRuleInstance(const char *chainSuffix,
-                             const char *chainSuffix,
                              virNWFilterRuleDefPtr rule,
                              const char *ifname,
                              virNWFilterVarCombIterPtr vars,
-                             virNWFilterRuleInstPtr res)
+                             char ***templates,
+                             size_t *ntemplates)
 {
+    size_t i;
+
+    *templates = NULL;
+    *ntemplates = 0;
+
     if (virNWFilterRuleIsProtocolEthernet(rule)) {
         if (rule->tt == VIR_NWFILTER_RULE_DIRECTION_OUT ||
             rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT) {
@@ -2666,17 +2631,11 @@ ebiptablesCreateRuleInstance(virNWFilterChainPriority chainPriority,
                                            vars,
                                            rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT,
                                            &template) < 0)
-                return -1;
+                goto error;
 
-            if (ebiptablesAddRuleInst(res,
+            if (VIR_APPEND_ELEMENT(*templates, *ntemplates, template) < 0) {
-                                      template,
-                                      chainSuffix,
-                                      chainPriority,
-                                      CHAINPREFIX_HOST_IN_TEMP,
-                                      rule->priority,
-                                      RT_EBTABLES) < 0) {
                 VIR_FREE(template);
-                return -1;
+                goto error;
             }
         }
 
@@ -2690,24 +2649,15 @@ ebiptablesCreateRuleInstance(virNWFilterChainPriority chainPriority,
                                            vars,
                                            false,
                                            &template) < 0)
-                return -1;
+                goto error;
 
-            if (ebiptablesAddRuleInst(res,
+            if (VIR_APPEND_ELEMENT(*templates, *ntemplates, template) < 0) {
-                                      template,
-                                      chainSuffix,
-                                      chainPriority,
-                                      CHAINPREFIX_HOST_OUT_TEMP,
-                                      rule->priority,
-                                      RT_EBTABLES) < 0) {
                 VIR_FREE(template);
-                return -1;
+                goto error;
             }
         }
     } else {
         bool isIPv6;
-        char **templates = NULL;
-        size_t ntemplates = 0;
-        size_t i, j;
         if (virNWFilterRuleIsProtocolIPv6(rule)) {
             isIPv6 = true;
         } else if (virNWFilterRuleIsProtocolIPv4(rule)) {
@@ -2715,76 +2665,27 @@ ebiptablesCreateRuleInstance(virNWFilterChainPriority chainPriority,
         } else {
             virReportError(VIR_ERR_OPERATION_FAILED,
                            "%s", _("unexpected protocol type"));
-            return -1;
+            goto error;
         }
 
         if (iptablesCreateRuleInstance(rule,
                                        ifname,
                                        vars,
                                        isIPv6,
-                                       &templates,
+                                       templates,
-                                       &ntemplates) < 0)
+                                       ntemplates) < 0)
-            return -1;
+            goto error;
-
-        for (i = 0; i < ntemplates; i++) {
-            if (ebiptablesAddRuleInst(res,
-                                      templates[i],
-                                      chainSuffix,
-                                      chainPriority,
-                                      '\0',
-                                      rule->priority,
-                                      (isIPv6) ? RT_IP6TABLES : RT_IPTABLES) < 0) {
-                for (j = i; j < ntemplates; j++)
-                    VIR_FREE(templates[j]);
-                return -1;
-            }
-        }
     }
 
     return 0;
-}
 
-static int
+ error:
-ebiptablesCreateRuleInstanceIterate(virNWFilterDefPtr nwfilter,
+    for (i = 0; i < *ntemplates; i++)
-                                    virNWFilterRuleDefPtr rule,
+        VIR_FREE((*templates)[i]);
-                                    const char *ifname,
+    VIR_FREE(*templates);
-                                    virNWFilterHashTablePtr vars,
+    *templates = NULL;
-                                    virNWFilterRuleInstPtr res)
+    *ntemplates = 0;
-{
-    int rc = 0;
-    virNWFilterVarCombIterPtr vciter, tmp;
-
-    /* rule->vars holds all the variables names that this rule will access.
-     * iterate over all combinations of the variables' values and instantiate
-     * the filtering rule with each combination.
-     */
-    tmp = vciter = virNWFilterVarCombIterCreate(vars,
-                                                rule->varAccess, rule->nVarAccess);
-    if (!vciter)
     return -1;
-
-    do {
-        rc = ebiptablesCreateRuleInstance(nwfilter->chainPriority,
-                                          nwfilter->chainsuffix,
-                                          rule,
-                                          ifname,
-                                          tmp,
-                                          res);
-        if (rc < 0)
-            break;
-        tmp = virNWFilterVarCombIterNext(tmp);
-    } while (tmp != NULL);
-
-    virNWFilterVarCombIterFree(vciter);
-
-    return rc;
-}
-
-static int
-ebiptablesFreeRuleInstance(void *_inst)
-{
-    ebiptablesRuleInstFree((ebiptablesRuleInstPtr)_inst);
-    return 0;
 }
 
 
@@ -3562,12 +3463,40 @@ ebiptablesRuleOrderSort(const void *a, const void *b)
     return insta->priority - instb->priority;
 }
 
+
 static int
-ebiptablesRuleOrderSortPtr(const void *a, const void *b)
+virNWFilterRuleInstSort(const void *a, const void *b)
 {
-    ebiptablesRuleInst * const *insta = a;
+    const virNWFilterRuleInst *insta = a;
-    ebiptablesRuleInst * const *instb = b;
+    const virNWFilterRuleInst *instb = b;
-    return ebiptablesRuleOrderSort(*insta, *instb);
+    const char *root = virNWFilterChainSuffixTypeToString(
+                                     VIR_NWFILTER_CHAINSUFFIX_ROOT);
+    bool root_a = STREQ(insta->chainSuffix, root);
+    bool root_b = STREQ(instb->chainSuffix, root);
+
+    /* ensure root chain commands appear before all others since
+       we will need them to create the child chains */
+    if (root_a) {
+        if (root_b) {
+            goto normal;
+        }
+        return -1; /* a before b */
+    }
+    if (root_b) {
+        return 1; /* b before a */
+    }
+ normal:
+    /* priorities are limited to range [-1000, 1000] */
+    return insta->priority - instb->priority;
+}
+
+
+static int
+virNWFilterRuleInstSortPtr(const void *a, const void *b)
+{
+    virNWFilterRuleInst * const *insta = a;
+    virNWFilterRuleInst * const *instb = b;
+    return virNWFilterRuleInstSort(*insta, *instb);
 }
 
 static int
@@ -3673,13 +3602,106 @@ ebtablesCreateTmpRootAndSubChains(virBufferPtr buf,
     return rc;
 }
 
+
+static int
+iptablesRuleInstCommand(virBufferPtr buf,
+                        const char *ifname,
+                        virNWFilterRuleInstPtr rule,
+                        char cmd, int pos)
+{
+    virNWFilterVarCombIterPtr vciter, tmp;
+    char **templates = NULL;
+    size_t ntemplates = 0;
+    size_t i;
+    int ret = -1;
+
+    /* rule->vars holds all the variables names that this rule will access.
+     * iterate over all combinations of the variables' values and instantiate
+     * the filtering rule with each combination.
+     */
+    tmp = vciter = virNWFilterVarCombIterCreate(rule->vars,
+                                                rule->def->varAccess,
+                                                rule->def->nVarAccess);
+    if (!vciter)
+        return -1;
+
+    do {
+        if (ebiptablesCreateRuleInstance(rule->chainSuffix,
+                                         rule->def,
+                                         ifname,
+                                         tmp,
+                                         &templates,
+                                         &ntemplates) < 0)
+            goto cleanup;
+        tmp = virNWFilterVarCombIterNext(tmp);
+    } while (tmp != NULL);
+
+    for (i = 0; i < ntemplates; i++)
+        iptablesInstCommand(buf, templates[i], cmd, pos);
+
+    ret = 0;
+ cleanup:
+    for (i = 0; i < ntemplates; i++)
+        VIR_FREE(templates[i]);
+    VIR_FREE(templates);
+    virNWFilterVarCombIterFree(vciter);
+    return ret;
+}
+
+
+static int
+ebtablesRuleInstCommand(virBufferPtr buf,
+                        const char *ifname,
+                        virNWFilterRuleInstPtr rule,
+                        char cmd, int pos,
+                        bool stopOnError)
+{
+    virNWFilterVarCombIterPtr vciter, tmp;
+    char **templates = NULL;
+    size_t ntemplates = 0;
+    size_t i;
+    int ret = -1;
+
+    /* rule->vars holds all the variables names that this rule will access.
+     * iterate over all combinations of the variables' values and instantiate
+     * the filtering rule with each combination.
+     */
+    tmp = vciter = virNWFilterVarCombIterCreate(rule->vars,
+                                                rule->def->varAccess,
+                                                rule->def->nVarAccess);
+    if (!vciter)
+        return -1;
+
+    do {
+        if (ebiptablesCreateRuleInstance(rule->chainSuffix,
+                                         rule->def,
+                                         ifname,
+                                         tmp,
+                                         &templates,
+                                         &ntemplates) < 0)
+            goto cleanup;
+        tmp = virNWFilterVarCombIterNext(tmp);
+    } while (tmp != NULL);
+
+    for (i = 0; i < ntemplates; i++)
+        ebiptablesInstCommand(buf, templates[i], cmd, pos, stopOnError);
+
+    ret = 0;
+ cleanup:
+    for (i = 0; i < ntemplates; i++)
+        VIR_FREE(templates[i]);
+    VIR_FREE(templates);
+    virNWFilterVarCombIterFree(vciter);
+    return ret;
+}
+
+
 static int
 ebiptablesApplyNewRules(const char *ifname,
-                        int nruleInstances,
+                        virNWFilterRuleInstPtr *rules,
-                        void **_inst)
+                        size_t nrules)
 {
     size_t i, j;
-    ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
     virHashTablePtr chains_in_set  = virHashCreate(10, NULL);
     virHashTablePtr chains_out_set = virHashCreate(10, NULL);
@@ -3689,28 +3711,27 @@ ebiptablesApplyNewRules(const char *ifname,
     int nEbtChains = 0;
     char *errmsg = NULL;
 
-    if (inst == NULL)
-        nruleInstances = 0;
-
     if (!chains_in_set || !chains_out_set)
         goto exit_free_sets;
 
-    if (nruleInstances > 1 && inst)
+    if (nrules)
-        qsort(inst, nruleInstances, sizeof(inst[0]),
+        qsort(rules, nrules, sizeof(rules[0]),
-              ebiptablesRuleOrderSortPtr);
+              virNWFilterRuleInstSortPtr);
 
     /* scan the rules to see which chains need to be created */
-    for (i = 0; i < nruleInstances; i++) {
+    for (i = 0; i < nrules; i++) {
-        sa_assert(inst);
+        if (virNWFilterRuleIsProtocolEthernet(rules[i]->def)) {
-        if (inst[i]->ruleType == RT_EBTABLES) {
+            const char *name = rules[i]->chainSuffix;
-            const char *name = inst[i]->neededProtocolChain;
+            if (rules[i]->def->tt == VIR_NWFILTER_RULE_DIRECTION_OUT ||
-            if (inst[i]->chainprefix == CHAINPREFIX_HOST_IN_TEMP) {
+                rules[i]->def->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT) {
                 if (virHashUpdateEntry(chains_in_set, name,
-                                       &inst[i]->chainPriority) < 0)
+                                       &rules[i]->chainPriority) < 0)
                     goto exit_free_sets;
-            } else {
+            }
+            if (rules[i]->def->tt == VIR_NWFILTER_RULE_DIRECTION_IN ||
+                rules[i]->def->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT) {
                 if (virHashUpdateEntry(chains_out_set, name,
-                                       &inst[i]->chainPriority) < 0)
+                                       &rules[i]->chainPriority) < 0)
                     goto exit_free_sets;
             }
         }
@@ -3759,37 +3780,34 @@ ebiptablesApplyNewRules(const char *ifname,
      * priority -500 and the chain with priority -500 will
      * then be created before it.
      */
-    for (i = 0; i < nruleInstances; i++) {
+    for (i = 0; i < nrules; i++) {
-        if (inst[i]->chainPriority > inst[i]->priority &&
+        if (rules[i]->chainPriority > rules[i]->priority &&
-            !strstr("root", inst[i]->neededProtocolChain)) {
+            !strstr("root", rules[i]->chainSuffix)) {
 
-             inst[i]->priority = inst[i]->chainPriority;
+             rules[i]->priority = rules[i]->chainPriority;
         }
     }
 
     /* process ebtables commands; interleave commands from filters with
        commands for creating and connecting ebtables chains */
     j = 0;
-    for (i = 0; i < nruleInstances; i++) {
+    for (i = 0; i < nrules; i++) {
-        sa_assert(inst);
+        if (virNWFilterRuleIsProtocolEthernet(rules[i]->def)) {
-        switch (inst[i]->ruleType) {
-        case RT_EBTABLES:
             while (j < nEbtChains &&
-                   ebtChains[j].priority <= inst[i]->priority) {
+                   ebtChains[j].priority <= rules[i]->priority) {
                 ebiptablesInstCommand(&buf,
                                       ebtChains[j++].commandTemplate,
                                       'A', -1, true);
             }
-            ebiptablesInstCommand(&buf,
+            ebtablesRuleInstCommand(&buf,
-                                  inst[i]->commandTemplate,
+                                    ifname,
+                                    rules[i],
                                     'A', -1, true);
-        break;
+        } else {
-        case RT_IPTABLES:
+            if (virNWFilterRuleIsProtocolIPv4(rules[i]->def))
                 haveIptables = true;
-        break;
+            else if (virNWFilterRuleIsProtocolIPv6(rules[i]->def))
-        case RT_IP6TABLES:
                 haveIp6tables = true;
-        break;
         }
     }
 
@@ -3828,11 +3846,11 @@ ebiptablesApplyNewRules(const char *ifname,
 
         NWFILTER_SET_IPTABLES_SHELLVAR(&buf);
 
-        for (i = 0; i < nruleInstances; i++) {
+        for (i = 0; i < nrules; i++) {
-            sa_assert(inst);
+            if (virNWFilterRuleIsProtocolIPv4(rules[i]->def))
-            if (inst[i]->ruleType == RT_IPTABLES)
+                iptablesRuleInstCommand(&buf,
-                iptablesInstCommand(&buf,
+                                        ifname,
-                                    inst[i]->commandTemplate,
+                                        rules[i],
                                         'A', -1);
         }
 
@@ -3869,10 +3887,11 @@ ebiptablesApplyNewRules(const char *ifname,
 
         NWFILTER_SET_IP6TABLES_SHELLVAR(&buf);
 
-        for (i = 0; i < nruleInstances; i++) {
+        for (i = 0; i < nrules; i++) {
-            if (inst[i]->ruleType == RT_IP6TABLES)
+            if (virNWFilterRuleIsProtocolIPv6(rules[i]->def))
-                iptablesInstCommand(&buf,
+                iptablesRuleInstCommand(&buf,
-                                    inst[i]->commandTemplate,
+                                        ifname,
+                                        rules[i],
                                         'A', -1);
         }
 
@@ -4095,12 +4114,10 @@ virNWFilterTechDriver ebiptables_driver = {
     .init     = ebiptablesDriverInit,
     .shutdown = ebiptablesDriverShutdown,
 
-    .createRuleInstance  = ebiptablesCreateRuleInstanceIterate,
     .applyNewRules       = ebiptablesApplyNewRules,
     .tearNewRules        = ebiptablesTearNewRules,
     .tearOldRules        = ebiptablesTearOldRules,
     .allTeardown         = ebiptablesAllTeardown,
-    .freeRuleInstance    = ebiptablesFreeRuleInstance,
 
     .canApplyBasicRules  = ebiptablesCanApplyBasicRules,
     .applyBasicRules     = ebtablesApplyBasicRules,

src/nwfilter/nwfilter_gentech_driver.c

@@ -119,14 +119,10 @@ virNWFilterTechDriverForName(const char *name)
 static void
 virNWFilterRuleInstFree(virNWFilterRuleInstPtr inst)
 {
-    size_t i;
     if (!inst)
         return;
 
-    for (i = 0; i < inst->ndata; i++)
+    virNWFilterHashTableFree(inst->vars);
-        inst->techdriver->freeRuleInstance(inst->data[i]);
-
-    VIR_FREE(inst->data);
     VIR_FREE(inst);
 }
 
@@ -272,51 +268,6 @@ virNWFilterPrintVars(virHashTablePtr vars,
 }
 
 
-/**
- * virNWFilterRuleInstantiate:
- * @techdriver: the driver to use for instantiation
- * @filter: The filter the rule is part of
- * @rule : The rule that is to be instantiated
- * @ifname: The name of the interface
- * @vars: map containing variable names and value used for instantiation
- *
- * Returns virNWFilterRuleInst object on success, NULL on error with
- * error reported.
- *
- * Instantiate a single rule. Return a pointer to virNWFilterRuleInst
- * object that will hold an array of driver-specific data resulting
- * from the instantiation. Returns NULL on error with error reported.
- */
-static virNWFilterRuleInstPtr
-virNWFilterRuleInstantiate(virNWFilterTechDriverPtr techdriver,
-                           virNWFilterDefPtr filter,
-                           virNWFilterRuleDefPtr rule,
-                           const char *ifname,
-                           virNWFilterHashTablePtr vars)
-{
-    int rc;
-    size_t i;
-    virNWFilterRuleInstPtr ret;
-
-    if (VIR_ALLOC(ret) < 0)
-        return NULL;
-
-    ret->techdriver = techdriver;
-
-    rc = techdriver->createRuleInstance(filter,
-                                        rule, ifname, vars, ret);
-
-    if (rc) {
-        for (i = 0; i < ret->ndata; i++)
-            techdriver->freeRuleInstance(ret->data[i]);
-        VIR_FREE(ret);
-        ret = NULL;
-    }
-
-    return ret;
-}
-
-
 /**
  * virNWFilterCreateVarsFrom:
  * @vars1: pointer to hash table
@@ -350,94 +301,115 @@ virNWFilterCreateVarsFrom(virNWFilterHashTablePtr vars1,
 }
 
 
-/**
+typedef struct _virNWFilterInst virNWFilterInst;
- * _virNWFilterInstantiateRec:
+typedef virNWFilterInst *virNWFilterInstPtr;
- * @techdriver: The driver to use for instantiation
+struct _virNWFilterInst {
- * @filter: The filter to instantiate
+    virNWFilterObjPtr *filters;
- * @ifname: The name of the interface to apply the rules to
+    size_t nfilters;
- * @vars: A map holding variable names and values used for instantiating
+    virNWFilterRuleInstPtr *rules;
- *  the filter and its subfilters.
+    size_t nrules;
- * @nEntries: number of virNWFilterInst objects collected
+};
- * @insts: pointer to array for virNWFilterIns object pointers
+
- * @useNewFilter: instruct whether to use a newDef pointer rather than a
+
- *  def ptr which is useful during a filter update
+static void
- * @foundNewFilter: pointer to int indivating whether a newDef pointer was
+virNWFilterInstReset(virNWFilterInstPtr inst)
- *  ever used; variable expected to be initialized to 0 by caller
+{
- *
+    size_t i;
- * Returns 0 on success, a value otherwise.
+
- *
+    for (i = 0; i < inst->nfilters; i++)
- * Recursively instantiate a filter by instantiating the given filter along
+        virNWFilterObjUnlock(inst->filters[i]);
- * with all its subfilters in a depth-first traversal of the tree of
+    VIR_FREE(inst->filters);
- * referenced filters. The name of the interface to which the rules belong
+    inst->nfilters = 0;
- * must be provided. Apply the values of variables as needed. Terminate with
+
- * error when a referenced filter is missing or a variable could not be
+    for (i = 0; i < inst->nrules; i++)
- * resolved -- among other reasons.
+        virNWFilterRuleInstFree(inst->rules[i]);
- */
+    VIR_FREE(inst->rules);
+}
+
+
+
 static int
-_virNWFilterInstantiateRec(virNWFilterTechDriverPtr techdriver,
+virNWFilterDefToInst(virNWFilterDriverStatePtr driver,
-                           virNWFilterDefPtr filter,
+                     virNWFilterDefPtr def,
-                           const char *ifname,
                      virNWFilterHashTablePtr vars,
-                           size_t *nEntries,
+                     enum instCase useNewFilter,
-                           virNWFilterRuleInstPtr **insts,
+                     bool *foundNewFilter,
-                           enum instCase useNewFilter, bool *foundNewFilter,
+                     virNWFilterInstPtr inst);
-                           virNWFilterDriverStatePtr driver)
+
+static int
+virNWFilterRuleDefToRuleInst(virNWFilterDefPtr def,
+                             virNWFilterRuleDefPtr rule,
+                             virNWFilterHashTablePtr vars,
+                             virNWFilterInstPtr inst)
+{
+    virNWFilterRuleInstPtr ruleinst;
+    int ret = -1;
+
+    if (VIR_ALLOC(ruleinst) < 0)
+        goto cleanup;
+
+    ruleinst->chainSuffix = def->chainsuffix;
+    ruleinst->chainPriority = def->chainPriority;
+    ruleinst->def = rule;
+    ruleinst->priority = rule->priority;
+    if (!(ruleinst->vars = virNWFilterHashTableCreate(0)))
+        goto cleanup;
+    if (virNWFilterHashTablePutAll(vars, ruleinst->vars) < 0)
+        goto cleanup;
+
+    if (VIR_APPEND_ELEMENT(inst->rules,
+                           inst->nrules,
+                           ruleinst) < 0)
+        goto cleanup;
+    inst = NULL;
+
+    ret = 0;
+ cleanup:
+    virNWFilterRuleInstFree(ruleinst);
+    return ret;
+}
+
+
+static int
+virNWFilterIncludeDefToRuleInst(virNWFilterDriverStatePtr driver,
+                                virNWFilterIncludeDefPtr inc,
+                                virNWFilterHashTablePtr vars,
+                                enum instCase useNewFilter,
+                                bool *foundNewFilter,
+                                virNWFilterInstPtr inst)
 {
     virNWFilterObjPtr obj;
-    int rc = 0;
+    virNWFilterHashTablePtr tmpvars = NULL;
-    size_t i;
+    virNWFilterDefPtr childdef;
-    virNWFilterRuleInstPtr inst;
+    int ret = -1;
-    virNWFilterDefPtr next_filter;
 
-    for (i = 0; i < filter->nentries; i++) {
-        virNWFilterRuleDefPtr    rule = filter->filterEntries[i]->rule;
-        virNWFilterIncludeDefPtr inc  = filter->filterEntries[i]->include;
-        if (rule) {
-            inst = virNWFilterRuleInstantiate(techdriver,
-                                              filter,
-                                              rule,
-                                              ifname,
-                                              vars);
-            if (!inst) {
-                rc = -1;
-                break;
-            }
-
-            if (VIR_APPEND_ELEMENT_COPY(*insts, *nEntries, inst) < 0) {
-                rc = -1;
-                break;
-            }
-
-        } else if (inc) {
     VIR_DEBUG("Instantiating filter %s", inc->filterref);
-            obj = virNWFilterObjFindByName(&driver->nwfilters, inc->filterref);
+    obj = virNWFilterObjFindByName(&driver->nwfilters,
-            if (obj) {
+                                   inc->filterref);
-
+    if (!obj) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("referenced filter '%s' is missing"),
+                       inc->filterref);
+        goto cleanup;
+    }
     if (obj->wantRemoved) {
         virReportError(VIR_ERR_NO_NWFILTER,
                        _("Filter '%s' is in use."),
                        inc->filterref);
-                    rc = -1;
+        goto cleanup;
-                    virNWFilterObjUnlock(obj);
-                    break;
     }
 
     /* create a temporary hashmap for depth-first tree traversal */
-                virNWFilterHashTablePtr tmpvars =
+    if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params,
-                                      virNWFilterCreateVarsFrom(inc->params,
+                                              vars)))
-                                                                vars);
+        goto cleanup;
-                if (!tmpvars) {
-                    rc = -1;
-                    virNWFilterObjUnlock(obj);
-                    break;
-                }
 
-                next_filter = obj->def;
+    childdef = obj->def;
 
     switch (useNewFilter) {
     case INSTANTIATE_FOLLOW_NEWFILTER:
         if (obj->newDef) {
-                        next_filter = obj->newDef;
+            childdef = obj->newDef;
             *foundNewFilter = true;
         }
         break;
@@ -445,30 +417,82 @@ _virNWFilterInstantiateRec(virNWFilterTechDriverPtr techdriver,
         break;
     }
 
-                rc = _virNWFilterInstantiateRec(techdriver,
+    if (VIR_APPEND_ELEMENT(inst->filters,
-                                                next_filter,
+                           inst->nfilters,
-                                                ifname,
+                           obj) < 0)
+        goto cleanup;
+    obj = NULL;
+
+    if (virNWFilterDefToInst(driver,
+                             childdef,
                              tmpvars,
-                                                nEntries, insts,
                              useNewFilter,
                              foundNewFilter,
-                                                driver);
+                             inst) < 0)
+        goto cleanup;
 
+    ret = 0;
+ cleanup:
+    if (ret < 0)
+        virNWFilterInstReset(inst);
     virNWFilterHashTableFree(tmpvars);
-
+    if (obj)
         virNWFilterObjUnlock(obj);
-                if (rc < 0)
+    return ret;
-                    break;
+}
-            } else {
+
-                virReportError(VIR_ERR_INTERNAL_ERROR,
+
-                               _("referenced filter '%s' is missing"),
+/**
-                               inc->filterref);
+ * virNWFilterDefToInst:
-                rc = -1;
+ * @driver: the driver state pointer
-                break;
+ * @def: The filter to instantiate
+ * @vars: A map holding variable names and values used for instantiating
+ *  the filter and its subfilters.
+ * @useNewFilter: instruct whether to use a newDef pointer rather than a
+ *  def ptr which is useful during a filter update
+ * @foundNewFilter: pointer to int indivating whether a newDef pointer was
+ *  ever used; variable expected to be initialized to 0 by caller
+ * @rulesout: array to be filled with rule instance
+ * @nrulesout: counter to be filled with number of rule instances
+ *
+ * Recursively expand a nested filter into a flat list of rule instances,
+ * in a depth-first traversal of the tree.
+ *
+ * Returns 0 on success, -1 on error
+ */
+static int
+virNWFilterDefToInst(virNWFilterDriverStatePtr driver,
+                     virNWFilterDefPtr def,
+                     virNWFilterHashTablePtr vars,
+                     enum instCase useNewFilter,
+                     bool *foundNewFilter,
+                     virNWFilterInstPtr inst)
+{
+    size_t i;
+    int ret = -1;
+
+    for (i = 0; i < def->nentries; i++) {
+        if (def->filterEntries[i]->rule) {
+            if (virNWFilterRuleDefToRuleInst(def,
+                                             def->filterEntries[i]->rule,
+                                             vars,
+                                             inst) < 0)
+                goto cleanup;
+        } else if (def->filterEntries[i]->include) {
+            if (virNWFilterIncludeDefToRuleInst(driver,
+                                                def->filterEntries[i]->include,
+                                                vars,
+                                                useNewFilter, foundNewFilter,
+                                                inst) < 0)
+                goto cleanup;
         }
     }
-    }
+
-    return rc;
+    ret = 0;
+ cleanup:
+    if (ret < 0)
+        virNWFilterInstReset(inst);
+    return ret;
 }
 
 
@@ -578,35 +602,6 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
 }
 
 
-static int
-virNWFilterRuleInstancesToArray(int nEntries,
-                                virNWFilterRuleInstPtr *insts,
-                                void ***ptrs,
-                                int *nptrs)
-{
-    size_t i, j;
-
-    *nptrs = 0;
-
-    for (j = 0; j < nEntries; j++)
-        (*nptrs) += insts[j]->ndata;
-
-    if ((*nptrs) == 0)
-        return 0;
-
-    if (VIR_ALLOC_N((*ptrs), (*nptrs)) < 0)
-        return -1;
-
-    (*nptrs) = 0;
-
-    for (j = 0; j < nEntries; j++)
-        for (i = 0; i < insts[j]->ndata; i++)
-            (*ptrs)[(*nptrs)++] = insts[j]->data[i];
-
-    return 0;
-}
-
-
 /**
  * virNWFilterInstantiate:
  * @vmuuid: The UUID of the VM
@@ -642,11 +637,7 @@ virNWFilterInstantiate(const unsigned char *vmuuid ATTRIBUTE_UNUSED,
                        bool forceWithPendingReq)
 {
     int rc;
-    size_t j;
+    virNWFilterInst inst;
-    int nptrs;
-    size_t nEntries = 0;
-    virNWFilterRuleInstPtr *insts = NULL;
-    void **ptrs = NULL;
     bool instantiate = true;
     char *buf;
     virNWFilterVarValuePtr lv;
@@ -654,6 +645,9 @@ virNWFilterInstantiate(const unsigned char *vmuuid ATTRIBUTE_UNUSED,
     bool reportIP = false;
 
     virNWFilterHashTablePtr missing_vars = virNWFilterHashTableCreate(0);
+
+    memset(&inst, 0, sizeof(inst));
+
     if (!missing_vars) {
         rc = -1;
         goto err_exit;
@@ -717,13 +711,11 @@ virNWFilterInstantiate(const unsigned char *vmuuid ATTRIBUTE_UNUSED,
         goto err_exit;
     }
 
-    rc = _virNWFilterInstantiateRec(techdriver,
+    rc = virNWFilterDefToInst(driver,
                               filter,
-                                    ifname,
                               vars,
-                                    &nEntries, &insts,
                               useNewFilter, foundNewFilter,
-                                    driver);
+                              &inst);
 
     if (rc < 0)
         goto err_exit;
@@ -738,16 +730,10 @@ virNWFilterInstantiate(const unsigned char *vmuuid ATTRIBUTE_UNUSED,
     }
 
     if (instantiate) {
-
-        rc = virNWFilterRuleInstancesToArray(nEntries, insts,
-                                             &ptrs, &nptrs);
-        if (rc < 0)
-            goto err_exit;
-
         if (virNWFilterLockIface(ifname) < 0)
             goto err_exit;
 
-        rc = techdriver->applyNewRules(ifname, nptrs, ptrs);
+        rc = techdriver->applyNewRules(ifname, inst.rules, inst.nrules);
 
         if (teardownOld && rc == 0)
             techdriver->tearOldRules(ifname);
@@ -763,13 +749,7 @@ virNWFilterInstantiate(const unsigned char *vmuuid ATTRIBUTE_UNUSED,
     }
 
  err_exit:
-
+    virNWFilterInstReset(&inst);
-    for (j = 0; j < nEntries; j++)
-        virNWFilterRuleInstFree(insts[j]);
-
-    VIR_FREE(insts);
-    VIR_FREE(ptrs);
-
     virNWFilterHashTableFree(missing_vars);
 
     return rc;

src/nwfilter/nwfilter_tech_driver.h

@@ -35,24 +35,20 @@ typedef virNWFilterTechDriver *virNWFilterTechDriverPtr;
 typedef struct _virNWFilterRuleInst virNWFilterRuleInst;
 typedef virNWFilterRuleInst *virNWFilterRuleInstPtr;
 struct _virNWFilterRuleInst {
-   size_t ndata;
+    const char *chainSuffix;
-   void **data;
+    virNWFilterChainPriority chainPriority;
-   virNWFilterTechDriverPtr techdriver;
+    virNWFilterRuleDefPtr def;
+    virNWFilterRulePriority priority;
+    virNWFilterHashTablePtr vars;
 };
 
 
 typedef int (*virNWFilterTechDrvInit)(bool privileged);
 typedef void (*virNWFilterTechDrvShutdown)(void);
 
-typedef int (*virNWFilterRuleCreateInstance)(virNWFilterDefPtr filter,
-                                             virNWFilterRuleDefPtr rule,
-                                             const char *ifname,
-                                             virNWFilterHashTablePtr vars,
-                                             virNWFilterRuleInstPtr res);
-
 typedef int (*virNWFilterRuleApplyNewRules)(const char *ifname,
-                                            int nruleInstances,
+                                            virNWFilterRuleInstPtr *rules,
-                                            void **_inst);
+                                            size_t nrules);
 
 typedef int (*virNWFilterRuleTeardownNewRules)(const char *ifname);
 
@@ -60,8 +56,6 @@ typedef int (*virNWFilterRuleTeardownOldRules)(const char *ifname);
 
 typedef int (*virNWFilterRuleAllTeardown)(const char *ifname);
 
-typedef int (*virNWFilterRuleFreeInstanceData)(void * _inst);
-
 typedef int (*virNWFilterCanApplyBasicRules)(void);
 
 typedef int (*virNWFilterApplyBasicRules)(const char *ifname,
@@ -87,12 +81,10 @@ struct _virNWFilterTechDriver {
     virNWFilterTechDrvInit init;
     virNWFilterTechDrvShutdown shutdown;
 
-    virNWFilterRuleCreateInstance createRuleInstance;
     virNWFilterRuleApplyNewRules applyNewRules;
     virNWFilterRuleTeardownNewRules tearNewRules;
     virNWFilterRuleTeardownOldRules tearOldRules;
     virNWFilterRuleAllTeardown allTeardown;
-    virNWFilterRuleFreeInstanceData freeRuleInstance;
 
     virNWFilterCanApplyBasicRules canApplyBasicRules;
     virNWFilterApplyBasicRules applyBasicRules;